| Message ID | 1367941941-19152-2-git-send-email-valentin.longchamp@keymile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/07/2013 05:52 PM, Valentin Longchamp wrote: > The kirkwood device found in the Prestera SoCs does not have all the > peripherals of its the usual kirkwood SoCs. There are hence missing > clocks in the SoCs. > > This patch registers another gate controller for the kirkwood that > registers only the available clocks of this kirkwood variant. Hi Valentin, Overall the patch set looks OK, however I don't understand why you need to declare a new gate controller. The list you delcared seemed to be just a subset of the Kirkwood one, why can't you use this one? Note that I am not an expert for the kirkwood hardware, so maybe I missed something. Regards, > > Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> > --- > .../devicetree/bindings/clock/mvebu-gated-clock.txt | 17 +++++++++++++++++ > drivers/clk/mvebu/clk-gating-ctrl.c | 19 +++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt > index cffc93d..7f494e90 100644 > --- a/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt > +++ b/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt > @@ -92,10 +92,27 @@ ID Clock Peripheral > 19 ge1 Gigabit Ethernet 1 > 20 tdm Time Division Mplx > > +The following is a list of provided IDs for Kirkwood Prestera variant: > +ID Clock Peripheral > +----------------------------------- > +0 ge0 Gigabit Ethernet 0 > +2 pex0 PCIe Cntrl 0 > +3 usb0 USB Host 0 > +4 sdio SDIO Cntrl > +5 tsu Transp. Stream Unit > +6 dunit SDRAM Cntrl > +7 runit Runit > +8 xor0 XOR DMA 0 > +16 xor1 XOR DMA 1 > +17 crypto CESA engine > +19 ge1 Gigabit Ethernet 1 > + > + > Required properties: > - compatible : shall be one of the following: > "marvell,dove-gating-clock" - for Dove SoC clock gating > "marvell,kirkwood-gating-clock" - for Kirkwood SoC clock gating > + "marvell,prestera-kw-gating-clock" - for Preseta SoC clock gating > - reg : shall be the register address of the Clock Gating Control register > - #clock-cells : from common clock binding; shall be set to 1 > > diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c > index ebf141d..ba37802 100644 > --- a/drivers/clk/mvebu/clk-gating-ctrl.c > +++ b/drivers/clk/mvebu/clk-gating-ctrl.c > @@ -203,6 +203,21 @@ static const struct mvebu_soc_descr __initconst kirkwood_gating_descr[] = { > { "tdm", NULL, 20 }, > { } > }; > + > +static const struct mvebu_soc_descr __initconst prestera_kw_gating_descr[] = { > + { "ge0", NULL, 0 }, > + { "pex0", NULL, 2 }, > + { "usb0", NULL, 3 }, > + { "sdio", NULL, 4 }, > + { "tsu", NULL, 5 }, > + { "runit", NULL, 7 }, > + { "xor0", NULL, 8 }, > + { "powersave", "cpuclk", 11 }, > + { "xor1", NULL, 16 }, > + { "crypto", NULL, 17 }, > + { "ge1", NULL, 19 }, > + { } > +}; > #endif > > static const __initdata struct of_device_id clk_gating_match[] = { > @@ -232,6 +247,10 @@ static const __initdata struct of_device_id clk_gating_match[] = { > .compatible = "marvell,kirkwood-gating-clock", > .data = kirkwood_gating_descr, > }, > + { > + .compatible = "marvell,prestera-kw-gating-clock", > + .data = prestera_kw_gating_descr, > + }, > #endif > > { } >
On Tue, May 07, 2013 at 06:11:44PM +0200, Gregory CLEMENT wrote: > On 05/07/2013 05:52 PM, Valentin Longchamp wrote: > > The kirkwood device found in the Prestera SoCs does not have all the > > peripherals of its the usual kirkwood SoCs. There are hence missing > > clocks in the SoCs. > > > > This patch registers another gate controller for the kirkwood that > > registers only the available clocks of this kirkwood variant. > > Hi Valentin, > > Overall the patch set looks OK, however I don't understand why you need > to declare a new gate controller. The list you delcared seemed to be > just a subset of the Kirkwood one, why can't you use this one? Hi Gregory Take a look at the code which gets deleted in patch 3/3. /* * Our variant of kirkwood (integrated in the Bobcat) hangs on accessing * SATA bits (14-15) of the Clock Gating Control Register. Since these * devices are also not present in this variant, their clocks get * disabled because unused when clk_disable_unused() gets called. * That's why we change the flags to these clocks to CLK_IGNORE_UNUSED */ By not defining these clocks in the first place, they never get turned off... Andrew
On 05/07/2013 06:11 PM, Gregory CLEMENT wrote: > On 05/07/2013 05:52 PM, Valentin Longchamp wrote: >> The kirkwood device found in the Prestera SoCs does not have all the >> peripherals of its the usual kirkwood SoCs. There are hence missing >> clocks in the SoCs. >> >> This patch registers another gate controller for the kirkwood that >> registers only the available clocks of this kirkwood variant. > > Hi Valentin, > > Overall the patch set looks OK, however I don't understand why you need > to declare a new gate controller. The list you delcared seemed to be > just a subset of the Kirkwood one, why can't you use this one? > > Note that I am not an expert for the kirkwood hardware, so maybe I > missed something. I just checked my mails from late 2012 and there Valentin and I agreed, that not the missing bits in clock gating control registers was the root cause of km_kirkwood to hang. It was rather the phy gates (that do not/ not yet exist on DT) that caused it to hang, i.e. when accessing SATA PHY registers. Valentin, I am not against an extra clock-gating-ctrl for Prestera but maybe having an kirkwood.dtsi fork without SATA and other peripherals will also help? And: Does DT-enabled km_kirkwood ever hang without the hack in it's board setup? There should be no PHY gates on DT boards.. Sebastian
On 05/07/2013 06:36 PM, Sebastian Hesselbarth wrote: > On 05/07/2013 06:11 PM, Gregory CLEMENT wrote: >> On 05/07/2013 05:52 PM, Valentin Longchamp wrote: >>> The kirkwood device found in the Prestera SoCs does not have all the >>> peripherals of its the usual kirkwood SoCs. There are hence missing >>> clocks in the SoCs. >>> >>> This patch registers another gate controller for the kirkwood that >>> registers only the available clocks of this kirkwood variant. >> >> Hi Valentin, >> >> Overall the patch set looks OK, however I don't understand why you need >> to declare a new gate controller. The list you delcared seemed to be >> just a subset of the Kirkwood one, why can't you use this one? >> >> Note that I am not an expert for the kirkwood hardware, so maybe I >> missed something. > > I just checked my mails from late 2012 and there Valentin and I agreed, > that not the missing bits in clock gating control registers was the root > cause of km_kirkwood to hang. It was rather the phy gates (that do not/ > not yet exist on DT) that caused it to hang, i.e. when accessing SATA > PHY registers. Yeah you are right, I had forgotten about this test. I just had a look at the board-km_kirkwood.c and I tried to figure out a way to make sure the clock gates bits never get read/written, so never get initialized. But as you say, that's not the real cause of the hang but > > Valentin, I am not against an extra clock-gating-ctrl for Prestera but > maybe having an kirkwood.dtsi fork without SATA and other peripherals > will also help? Since it's not the real problem having the extra clock-gating-ctrl would me more consistent with the real Prestera HW, but maybe it's better to keep only one that works for both as it's already the case, it's less code to maintain. A better approach would be to work on the kirkwood.dtsi as you mention it. The SATA node in kirkwood.dtsi is by default disabled, is this enough or should it be moved to kirkwood-6281.dtsi for instance ? > > And: Does DT-enabled km_kirkwood ever hang without the hack in it's > board setup? There should be no PHY gates on DT boards.. > I have just tested it and it does not hang, so I will submit and patch that simply removes this workaround since the PHY gates are not accessed anymore (thanks to DT clock gates and that km_kirkwood does not enable the sata of node). Valentin
On 05/08/2013 09:04 AM, Valentin Longchamp wrote: > On 05/07/2013 06:36 PM, Sebastian Hesselbarth wrote: >> I just checked my mails from late 2012 and there Valentin and I agreed, >> that not the missing bits in clock gating control registers was the root >> cause of km_kirkwood to hang. It was rather the phy gates (that do not/ >> not yet exist on DT) that caused it to hang, i.e. when accessing SATA >> PHY registers. > > Yeah you are right, I had forgotten about this test. I just had a look at the > board-km_kirkwood.c and I tried to figure out a way to make sure the clock gates > bits never get read/written, so never get initialized. But as you say, that's > not the real cause of the hang but > >> Valentin, I am not against an extra clock-gating-ctrl for Prestera but >> maybe having an kirkwood.dtsi fork without SATA and other peripherals >> will also help? > > Since it's not the real problem having the extra clock-gating-ctrl would me more > consistent with the real Prestera HW, but maybe it's better to keep only one > that works for both as it's already the case, it's less code to maintain. > > A better approach would be to work on the kirkwood.dtsi as you mention it. The > SATA node in kirkwood.dtsi is by default disabled, is this enough or should it > be moved to kirkwood-6281.dtsi for instance ? Valentin, I guess for the long run, we will re-introduce phy gates either by abusing clk gates or some other way. But they will depend on existing OF nodes. Considering this, it would be best to fork kirkwood.dtsi for Prestera and leave out all OF nodes that are not implemented there. We could have kirkwood.dtsi -+-> kirkwood-kirkwood.dtsi -+-> kirkwood-6281.dtsi +-> kirkwood-prestera.dtsi +-> kirkwood-6282.dtsi or any other naming scheme. Maybe Andrew or Jason can comment on this. >> And: Does DT-enabled km_kirkwood ever hang without the hack in it's >> board setup? There should be no PHY gates on DT boards.. > > I have just tested it and it does not hang, so I will submit and patch that > simply removes this workaround since the PHY gates are not accessed anymore > (thanks to DT clock gates and that km_kirkwood does not enable the sata of node). Great! I suggest to also submit the dtsi changes above within the same patch set. Sebastian
On Wed, May 08, 2013 at 09:21:40AM +0200, Sebastian Hesselbarth wrote: > On 05/08/2013 09:04 AM, Valentin Longchamp wrote: > >On 05/07/2013 06:36 PM, Sebastian Hesselbarth wrote: > >>I just checked my mails from late 2012 and there Valentin and I agreed, > >>that not the missing bits in clock gating control registers was the root > >>cause of km_kirkwood to hang. It was rather the phy gates (that do not/ > >>not yet exist on DT) that caused it to hang, i.e. when accessing SATA > >>PHY registers. > > > >Yeah you are right, I had forgotten about this test. I just had a look at the > >board-km_kirkwood.c and I tried to figure out a way to make sure the clock gates > >bits never get read/written, so never get initialized. But as you say, that's > >not the real cause of the hang but > > > >>Valentin, I am not against an extra clock-gating-ctrl for Prestera but > >>maybe having an kirkwood.dtsi fork without SATA and other peripherals > >>will also help? > > > >Since it's not the real problem having the extra clock-gating-ctrl would me more > >consistent with the real Prestera HW, but maybe it's better to keep only one > >that works for both as it's already the case, it's less code to maintain. > > > >A better approach would be to work on the kirkwood.dtsi as you mention it. The > >SATA node in kirkwood.dtsi is by default disabled, is this enough or should it > >be moved to kirkwood-6281.dtsi for instance ? > > Valentin, > > I guess for the long run, we will re-introduce phy gates either by > abusing clk gates or some other way. But they will depend on existing > OF nodes. > > Considering this, it would be best to fork kirkwood.dtsi for Prestera > and leave out all OF nodes that are not implemented there. We could > have > > kirkwood.dtsi -+-> kirkwood-kirkwood.dtsi -+-> kirkwood-6281.dtsi > +-> kirkwood-prestera.dtsi +-> kirkwood-6282.dtsi or, kirkwood.dtsi prestera.dtsi -pinctrl- kirkwood-98dx4122.dtsi (should rename to prestera-, but seems churn-ish) kirkwood-6281.dtsi kirkwood-6282.dtsi -dts- kirkwood-km_kirkwood.dts (rename/churn prestera-?) |-prestera.dtsi \-kirkwood-98dx4122.dtsi or, honestly, how many boards are going to be based on prestera? If it's just the one, why not combine all the nodes into kirkwood-km_kirkwood.dts? thx, Jason.
diff --git a/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt index cffc93d..7f494e90 100644 --- a/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt +++ b/Documentation/devicetree/bindings/clock/mvebu-gated-clock.txt @@ -92,10 +92,27 @@ ID Clock Peripheral 19 ge1 Gigabit Ethernet 1 20 tdm Time Division Mplx +The following is a list of provided IDs for Kirkwood Prestera variant: +ID Clock Peripheral +----------------------------------- +0 ge0 Gigabit Ethernet 0 +2 pex0 PCIe Cntrl 0 +3 usb0 USB Host 0 +4 sdio SDIO Cntrl +5 tsu Transp. Stream Unit +6 dunit SDRAM Cntrl +7 runit Runit +8 xor0 XOR DMA 0 +16 xor1 XOR DMA 1 +17 crypto CESA engine +19 ge1 Gigabit Ethernet 1 + + Required properties: - compatible : shall be one of the following: "marvell,dove-gating-clock" - for Dove SoC clock gating "marvell,kirkwood-gating-clock" - for Kirkwood SoC clock gating + "marvell,prestera-kw-gating-clock" - for Preseta SoC clock gating - reg : shall be the register address of the Clock Gating Control register - #clock-cells : from common clock binding; shall be set to 1 diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c index ebf141d..ba37802 100644 --- a/drivers/clk/mvebu/clk-gating-ctrl.c +++ b/drivers/clk/mvebu/clk-gating-ctrl.c @@ -203,6 +203,21 @@ static const struct mvebu_soc_descr __initconst kirkwood_gating_descr[] = { { "tdm", NULL, 20 }, { } }; + +static const struct mvebu_soc_descr __initconst prestera_kw_gating_descr[] = { + { "ge0", NULL, 0 }, + { "pex0", NULL, 2 }, + { "usb0", NULL, 3 }, + { "sdio", NULL, 4 }, + { "tsu", NULL, 5 }, + { "runit", NULL, 7 }, + { "xor0", NULL, 8 }, + { "powersave", "cpuclk", 11 }, + { "xor1", NULL, 16 }, + { "crypto", NULL, 17 }, + { "ge1", NULL, 19 }, + { } +}; #endif static const __initdata struct of_device_id clk_gating_match[] = { @@ -232,6 +247,10 @@ static const __initdata struct of_device_id clk_gating_match[] = { .compatible = "marvell,kirkwood-gating-clock", .data = kirkwood_gating_descr, }, + { + .compatible = "marvell,prestera-kw-gating-clock", + .data = prestera_kw_gating_descr, + }, #endif { }
The kirkwood device found in the Prestera SoCs does not have all the peripherals of its the usual kirkwood SoCs. There are hence missing clocks in the SoCs. This patch registers another gate controller for the kirkwood that registers only the available clocks of this kirkwood variant. Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> --- .../devicetree/bindings/clock/mvebu-gated-clock.txt | 17 +++++++++++++++++ drivers/clk/mvebu/clk-gating-ctrl.c | 19 +++++++++++++++++++ 2 files changed, 36 insertions(+)