ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Subsystems (2)
PATCH 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
This extra clock is needed to access the registers of the SPI controller used on Armada 7K/8K SoCs.
--- arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/marvell/armada-cp110.dtsi +6 -2
@@ -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"; };
Signature · 1 line
2.15.1
PATCH 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Hi Gregory,
Quoted 2 lines · Gregory CLEMENT
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
Quoted 30 lines
Signed-off-by Gregory CLEMENT <[email protected]>--- arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm64/boot/dts/marvell/armada-cp110.dtsi +6
-2@@ -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"; };
Signature · 3 lines
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 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Hi Gregory,
Quoted 5 lines · Baruch Siach
Quoted 2 lines · Gregory CLEMENT
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
Quoted 30 lines
Quoted 30 lines
Signed-off-by Gregory CLEMENT <[email protected]>--- arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm64/boot/dts/marvell/armada-cp110.dtsi +6
-2@@ -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"; };
Signature · 3 lines
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 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Hi Baruch,
Quoted 7 lines · Baruch Siach
Hi Gregory,
Quoted 4 lines · Baruch Siach
Quoted 2 lines · Gregory CLEMENT
This extra clock is needed to access the registers of the SPI controller used on Armada 7K/8K SoCs.
Quoted 8 lines
Quoted 1 line
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.cdriver (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
Quoted 45 lines
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
Quoted 30 lines
Quoted 30 lines
Signed-off-by Gregory CLEMENT <[email protected]>--- arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm64/boot/dts/marvell/armada-cp110.dtsi +6
-2@@ -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"; };Signature · 3 lines
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 -
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Hello,
Quoted 21 lines · Gregory CLEMENT
Quoted 5 lines · Baruch Siach
Quoted 4 lines · Gregory CLEMENT
Quoted 2 lines
This extra clock is needed to access the registers of the SPI controller used on Armada 7K/8K SoCs.
Quoted 8 lines
Quoted 1 line
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.cdriver (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.
Quoted 2 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Hi Thomas,
Quoted 25 lines · Thomas Petazzoni
Hello,
Quoted 21 lines · Gregory CLEMENT
Quoted 5 lines · Baruch Siach
Quoted 4 lines · Gregory CLEMENT
Quoted 2 lines
This extra clock is needed to access the registers of the SPI controller used on Armada 7K/8K SoCs.
Quoted 8 lines
Quoted 1 line
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.cdriver (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.
Quoted 6 lines
Quoted 2 lines
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
Quoted 11 lines
Thomas
Signature · 8 lines
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
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH 1/1 ARM64: dts: marvell: armada-cp110: Add registers clock for SPI nodes
Hello,
Quoted 10 lines · Gregory CLEMENT
Quoted 10 lines
Quoted 9 lines
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.
Quoted 8 lines
Quoted 3 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com