[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Gregory CLEMENT]

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

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Baruch Siach]

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 -

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Baruch Siach]

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 -

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Gregory CLEMENT]

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

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Thomas Petazzoni]

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

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Gregory CLEMENT]

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

[PATCH] ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes

[Thomas Petazzoni]

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