* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes @ 2018-01-30 11:14 Gregory CLEMENT 2018-01-30 11:21 ` Baruch Siach 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2018-01-30 11:14 UTC (permalink / raw) To: linux-arm-kernel This extra clock is needed to access the registers of the SPI controller used on Armada 7K/8K SoCs. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi index a8af4136dbe7..0ab921861a2f 100644 --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi @@ -257,7 +257,9 @@ reg = <0x700600 0x50>; #address-cells = <0x1>; #size-cells = <0x0>; - clocks = <&CP110_LABEL(clk) 1 21>; + clock-names = "core", "axi"; + clocks = <&CP110_LABEL(clk) 1 21>, + <&CP110_LABEL(clk) 1 17>; status = "disabled"; }; @@ -266,7 +268,9 @@ reg = <0x700680 0x50>; #address-cells = <1>; #size-cells = <0>; - clocks = <&CP110_LABEL(clk) 1 21>; + clock-names = "core", "axi"; + clocks = <&CP110_LABEL(clk) 1 21>, + <&CP110_LABEL(clk) 1 17>; status = "disabled"; }; -- 2.15.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes 2018-01-30 11:14 [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes Gregory CLEMENT @ 2018-01-30 11:21 ` Baruch Siach 2018-01-30 12:26 ` Baruch Siach 0 siblings, 1 reply; 7+ messages in thread From: Baruch Siach @ 2018-01-30 11:21 UTC (permalink / raw) To: linux-arm-kernel Hi Gregory, On Tue, Jan 30, 2018 at 12:14:55PM +0100, Gregory CLEMENT wrote: > This extra clock is needed to access the registers of the SPI controller > used on Armada 7K/8K SoCs. Don't we need this also for I2C and UART? baruch > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > --- > arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > index a8af4136dbe7..0ab921861a2f 100644 > --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > @@ -257,7 +257,9 @@ > reg = <0x700600 0x50>; > #address-cells = <0x1>; > #size-cells = <0x0>; > - clocks = <&CP110_LABEL(clk) 1 21>; > + clock-names = "core", "axi"; > + clocks = <&CP110_LABEL(clk) 1 21>, > + <&CP110_LABEL(clk) 1 17>; > status = "disabled"; > }; > > @@ -266,7 +268,9 @@ > reg = <0x700680 0x50>; > #address-cells = <1>; > #size-cells = <0>; > - clocks = <&CP110_LABEL(clk) 1 21>; > + clock-names = "core", "axi"; > + clocks = <&CP110_LABEL(clk) 1 21>, > + <&CP110_LABEL(clk) 1 17>; > status = "disabled"; > }; -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes 2018-01-30 11:21 ` Baruch Siach @ 2018-01-30 12:26 ` Baruch Siach 2018-01-30 12:42 ` Gregory CLEMENT 0 siblings, 1 reply; 7+ messages in thread From: Baruch Siach @ 2018-01-30 12:26 UTC (permalink / raw) To: linux-arm-kernel Hi Gregory, On Tue, Jan 30, 2018 at 01:21:38PM +0200, Baruch Siach wrote: > On Tue, Jan 30, 2018 at 12:14:55PM +0100, Gregory CLEMENT wrote: > > This extra clock is needed to access the registers of the SPI controller > > used on Armada 7K/8K SoCs. > > Don't we need this also for I2C and UART? So you posted a patch for I2C as well. Looking at the cp110-system-controller.c driver (cp110_syscon_common_probe()), I see that clock gate #17 (CP110_GATE_MAIN) is automatically enabled when #21 (CP110_GATE_SLOW_IO) is enabled. So this additional clock specifier should not be needed. Also, as I understand, 'clocks' and 'clock-names' properties are meant to describe the clock input from the peripheral device point of view. But gate #17 is an internal clock controller gate that is not visible to the SPI master or I2C master device. What problem is this patch meant to solve? baruch > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > --- > > arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > > index a8af4136dbe7..0ab921861a2f 100644 > > --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > > +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > > @@ -257,7 +257,9 @@ > > reg = <0x700600 0x50>; > > #address-cells = <0x1>; > > #size-cells = <0x0>; > > - clocks = <&CP110_LABEL(clk) 1 21>; > > + clock-names = "core", "axi"; > > + clocks = <&CP110_LABEL(clk) 1 21>, > > + <&CP110_LABEL(clk) 1 17>; > > status = "disabled"; > > }; > > > > @@ -266,7 +268,9 @@ > > reg = <0x700680 0x50>; > > #address-cells = <1>; > > #size-cells = <0>; > > - clocks = <&CP110_LABEL(clk) 1 21>; > > + clock-names = "core", "axi"; > > + clocks = <&CP110_LABEL(clk) 1 21>, > > + <&CP110_LABEL(clk) 1 17>; > > status = "disabled"; > > }; -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes 2018-01-30 12:26 ` Baruch Siach @ 2018-01-30 12:42 ` Gregory CLEMENT 2018-01-30 12:53 ` Thomas Petazzoni 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2018-01-30 12:42 UTC (permalink / raw) To: linux-arm-kernel Hi Baruch, On mar., janv. 30 2018, Baruch Siach <baruch@tkos.co.il> wrote: > Hi Gregory, > > On Tue, Jan 30, 2018 at 01:21:38PM +0200, Baruch Siach wrote: >> On Tue, Jan 30, 2018 at 12:14:55PM +0100, Gregory CLEMENT wrote: >> > This extra clock is needed to access the registers of the SPI controller >> > used on Armada 7K/8K SoCs. >> >> Don't we need this also for I2C and UART? > > So you posted a patch for I2C as well. > > Looking at the cp110-system-controller.c driver (cp110_syscon_common_probe()), > I see that clock gate #17 (CP110_GATE_MAIN) is automatically enabled when #21 > (CP110_GATE_SLOW_IO) is enabled. So this additional clock specifier should not > be needed. Actually this is the reason of these changes. The clock driver is wrong, now that we got new documentation about the clocks, we saw that the clock tree descried in this driver was not correct. There is no relation between clock 17 and clock 21 for instance. But in order to be able to fix the clock driver, first we have to make sure that all the driver of the peripherals really select their own clocks. I have already the patch fixing the clock ready and once I will have converted the remaining peripheral I will be able to submit it. Gregory > > Also, as I understand, 'clocks' and 'clock-names' properties are meant to > describe the clock input from the peripheral device point of view. But gate > #17 is an internal clock controller gate that is not visible to the SPI master > or I2C master device. > > What problem is this patch meant to solve? > > baruch > >> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> > --- >> > arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi >> > index a8af4136dbe7..0ab921861a2f 100644 >> > --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi >> > +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi >> > @@ -257,7 +257,9 @@ >> > reg = <0x700600 0x50>; >> > #address-cells = <0x1>; >> > #size-cells = <0x0>; >> > - clocks = <&CP110_LABEL(clk) 1 21>; >> > + clock-names = "core", "axi"; >> > + clocks = <&CP110_LABEL(clk) 1 21>, >> > + <&CP110_LABEL(clk) 1 17>; >> > status = "disabled"; >> > }; >> > >> > @@ -266,7 +268,9 @@ >> > reg = <0x700680 0x50>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > - clocks = <&CP110_LABEL(clk) 1 21>; >> > + clock-names = "core", "axi"; >> > + clocks = <&CP110_LABEL(clk) 1 21>, >> > + <&CP110_LABEL(clk) 1 17>; >> > status = "disabled"; >> > }; > > -- > http://baruch.siach.name/blog/ ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes 2018-01-30 12:42 ` Gregory CLEMENT @ 2018-01-30 12:53 ` Thomas Petazzoni 2018-01-30 13:03 ` Gregory CLEMENT 0 siblings, 1 reply; 7+ messages in thread From: Thomas Petazzoni @ 2018-01-30 12:53 UTC (permalink / raw) To: linux-arm-kernel Hello, On Tue, 30 Jan 2018 13:42:41 +0100, Gregory CLEMENT wrote: > > On Tue, Jan 30, 2018 at 01:21:38PM +0200, Baruch Siach wrote: > >> On Tue, Jan 30, 2018 at 12:14:55PM +0100, Gregory CLEMENT wrote: > >> > This extra clock is needed to access the registers of the SPI controller > >> > used on Armada 7K/8K SoCs. > >> > >> Don't we need this also for I2C and UART? > > > > So you posted a patch for I2C as well. > > > > Looking at the cp110-system-controller.c driver (cp110_syscon_common_probe()), > > I see that clock gate #17 (CP110_GATE_MAIN) is automatically enabled when #21 > > (CP110_GATE_SLOW_IO) is enabled. So this additional clock specifier should not > > be needed. > > Actually this is the reason of these changes. The clock driver is wrong, > now that we got new documentation about the clocks, we saw that the > clock tree descried in this driver was not correct. There is no relation > between clock 17 and clock 21 for instance. But in order to be able to > fix the clock driver, first we have to make sure that all the driver of > the peripherals really select their own clocks. > > I have already the patch fixing the clock ready and once I will have > converted the remaining peripheral I will be able to submit it. Of course I do agree with Gr?gory here, since we discussed this at length. However, I think Baruch has a point in that this should be explained in the commit log. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes 2018-01-30 12:53 ` Thomas Petazzoni @ 2018-01-30 13:03 ` Gregory CLEMENT 2018-01-30 13:04 ` Thomas Petazzoni 0 siblings, 1 reply; 7+ messages in thread From: Gregory CLEMENT @ 2018-01-30 13:03 UTC (permalink / raw) To: linux-arm-kernel Hi Thomas, On mar., janv. 30 2018, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Tue, 30 Jan 2018 13:42:41 +0100, Gregory CLEMENT wrote: > >> > On Tue, Jan 30, 2018 at 01:21:38PM +0200, Baruch Siach wrote: >> >> On Tue, Jan 30, 2018 at 12:14:55PM +0100, Gregory CLEMENT wrote: >> >> > This extra clock is needed to access the registers of the SPI controller >> >> > used on Armada 7K/8K SoCs. >> >> >> >> Don't we need this also for I2C and UART? >> > >> > So you posted a patch for I2C as well. >> > >> > Looking at the cp110-system-controller.c driver (cp110_syscon_common_probe()), >> > I see that clock gate #17 (CP110_GATE_MAIN) is automatically enabled when #21 >> > (CP110_GATE_SLOW_IO) is enabled. So this additional clock specifier should not >> > be needed. >> >> Actually this is the reason of these changes. The clock driver is wrong, >> now that we got new documentation about the clocks, we saw that the >> clock tree descried in this driver was not correct. There is no relation >> between clock 17 and clock 21 for instance. But in order to be able to >> fix the clock driver, first we have to make sure that all the driver of >> the peripherals really select their own clocks. >> >> I have already the patch fixing the clock ready and once I will have >> converted the remaining peripheral I will be able to submit it. > > Of course I do agree with Gr?gory here, since we discussed this at > length. However, I think Baruch has a point in that this should be > explained in the commit log. It was explained in the series modifying the drivers. But here it is only about correctly describing the hardware ressource in the device tree, the fact that the linux clock driver may or not automatically select some of the clock should not be taking into account. Gregory > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes 2018-01-30 13:03 ` Gregory CLEMENT @ 2018-01-30 13:04 ` Thomas Petazzoni 0 siblings, 0 replies; 7+ messages in thread From: Thomas Petazzoni @ 2018-01-30 13:04 UTC (permalink / raw) To: linux-arm-kernel Hello, On Tue, 30 Jan 2018 14:03:24 +0100, Gregory CLEMENT wrote: > >> Actually this is the reason of these changes. The clock driver is wrong, > >> now that we got new documentation about the clocks, we saw that the > >> clock tree descried in this driver was not correct. There is no relation > >> between clock 17 and clock 21 for instance. But in order to be able to > >> fix the clock driver, first we have to make sure that all the driver of > >> the peripherals really select their own clocks. > >> > >> I have already the patch fixing the clock ready and once I will have > >> converted the remaining peripheral I will be able to submit it. > > > > Of course I do agree with Gr?gory here, since we discussed this at > > length. However, I think Baruch has a point in that this should be > > explained in the commit log. > > It was explained in the series modifying the drivers. But here it is > only about correctly describing the hardware ressource in the device > tree, the fact that the linux clock driver may or not automatically > select some of the clock should not be taking into account. Agreed, but there is no reference to the driver commits in your commit log, and I still think a better commit log would be nice. The simple fact that Baruch had to ask is a good indication that the commit log is not detailed enough, IMO. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-30 13:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-30 11:14 [PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes Gregory CLEMENT 2018-01-30 11:21 ` Baruch Siach 2018-01-30 12:26 ` Baruch Siach 2018-01-30 12:42 ` Gregory CLEMENT 2018-01-30 12:53 ` Thomas Petazzoni 2018-01-30 13:03 ` Gregory CLEMENT 2018-01-30 13:04 ` Thomas Petazzoni
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.