diff mbox series

[3/5] arm64: dts: exynos: gs101: use correct clocks for usi8

Message ID 20240127003607.501086-4-andre.draszik@linaro.org
State New
Headers
Series [1/5] clk: samsung: gs101: gpio_peric0_pclk needs to be kept on |

Commit Message

André Draszik Jan. 27, 2024, 12:35 a.m. UTC 
  Wrong pclk clocks have been used in this usi8 instance here. For USI
and I2C, we need the ipclk and pclk, where pclk is the bus clock.
Without it, nothing can work.
It is unclear what exactly is using USI8_USI_CLK, but it is not
required for the IP to be operational at this stage, while pclk is.
This also brings the DT in line with the clock names expected by the
usi and i2c drivers.

Update the DTSI accordingly.

Fixes: 6d44d1a1fb62 ("arm64: dts: exynos: gs101: define USI8 with I2C configuration")
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Sam Protsenko Jan. 27, 2024, 3:22 a.m. UTC | #1 
On Fri, Jan 26, 2024 at 6:37 PM André Draszik <andre.draszik@linaro.org> wrote:
>
> Wrong pclk clocks have been used in this usi8 instance here. For USI
> and I2C, we need the ipclk and pclk, where pclk is the bus clock.
> Without it, nothing can work.

Empty line is missing here?

> It is unclear what exactly is using USI8_USI_CLK, but it is not
> required for the IP to be operational at this stage, while pclk is.

From [1] it looks like DIV_CLK_PERIC0_USI8_USI is a common parrent for
these two leaf gate clocks:
  1. GOUT_BLK_PERIC0_UID_RSTNSYNC_CLK_PERIC0_USI8_USI_IPCLKPORT_CLK
  2. GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_7

So IIUC, you replace clock #1 with clock #2 in this patch? If so, I
think that's a right move, because in my experience RSTNSYNC clocks
shouldn't be used at all for consumer IP-cores. That's why I never
added RSTNSYNC clocks in clk-exynos850 driver at all -- I only see
them useful for store/restore ops during suspend/resume.

[1] https://android.googlesource.com/kernel/gs/+/refs/tags/android-12.0.0_r0.17/drivers/soc/google/cal-if/gs101/cmucal-node.c#2793

> This also brings the DT in line with the clock names expected by the
> usi and i2c drivers.
>
> Update the DTSI accordingly.
>
> Fixes: 6d44d1a1fb62 ("arm64: dts: exynos: gs101: define USI8 with I2C configuration")
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> index bc251e565be6..e5b665be2d62 100644
> --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> @@ -380,7 +380,7 @@ usi8: usi@109700c0 {
>                         ranges;
>                         #address-cells = <1>;
>                         #size-cells = <1>;
> -                       clocks = <&cmu_peric0 CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK>,
> +                       clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7>,
>                                  <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7>;
>                         clock-names = "pclk", "ipclk";
>                         samsung,sysreg = <&sysreg_peric0 0x101c>;
> @@ -397,7 +397,7 @@ hsi2c_8: i2c@10970000 {
>                                 pinctrl-names = "default";
>                                 pinctrl-0 = <&hsi2c8_bus>;
>                                 clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7>,
> -                                        <&cmu_peric0 CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK>;
> +                                        <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7>;
>                                 clock-names = "hsi2c", "hsi2c_pclk";
>                                 status = "disabled";
>                         };
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Tudor Ambarus Jan. 27, 2024, 4 a.m. UTC | #2 
On 1/27/24 03:22, Sam Protsenko wrote:
> On Fri, Jan 26, 2024 at 6:37 PM André Draszik <andre.draszik@linaro.org> wrote:
>>
>> Wrong pclk clocks have been used in this usi8 instance here. For USI
>> and I2C, we need the ipclk and pclk, where pclk is the bus clock.
>> Without it, nothing can work.
> 
> Empty line is missing here?
> 
>> It is unclear what exactly is using USI8_USI_CLK, but it is not
>> required for the IP to be operational at this stage, while pclk is.
> 
> From [1] it looks like DIV_CLK_PERIC0_USI8_USI is a common parrent for
> these two leaf gate clocks:
>   1. GOUT_BLK_PERIC0_UID_RSTNSYNC_CLK_PERIC0_USI8_USI_IPCLKPORT_CLK
>   2. GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_7
> 
> So IIUC, you replace clock #1 with clock #2 in this patch? If so, I

No, GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_7 is already used by
IPCLK, the one that controls the clock frequency.

So I understand Andre' replaces a child of the USI8 with something else.

I don't think this works. We shall at least test it. I tested my usi8
patches with the eeprom that's populated on the battery connector. I'll
sync with Andre' offline and redo the tests on Monday.

> think that's a right move, because in my experience RSTNSYNC clocks
> shouldn't be used at all for consumer IP-cores. That's why I never
> added RSTNSYNC clocks in clk-exynos850 driver at all -- I only see
> them useful for store/restore ops during suspend/resume.
> 
> [1] https://android.googlesource.com/kernel/gs/+/refs/tags/android-12.0.0_r0.17/drivers/soc/google/cal-if/gs101/cmucal-node.c#2793
> 
>> This also brings the DT in line with the clock names expected by the
>> usi and i2c drivers.
>>
>> Update the DTSI accordingly.
>>
>> Fixes: 6d44d1a1fb62 ("arm64: dts: exynos: gs101: define USI8 with I2C configuration")
>> Signed-off-by: André Draszik <andre.draszik@linaro.org>
>> ---
> 
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> 
>>  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
>> index bc251e565be6..e5b665be2d62 100644
>> --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
>> +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
>> @@ -380,7 +380,7 @@ usi8: usi@109700c0 {
>>                         ranges;
>>                         #address-cells = <1>;
>>                         #size-cells = <1>;
>> -                       clocks = <&cmu_peric0 CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK>,
>> +                       clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7>,
>>                                  <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7>;
>>                         clock-names = "pclk", "ipclk";
>>                         samsung,sysreg = <&sysreg_peric0 0x101c>;
>> @@ -397,7 +397,7 @@ hsi2c_8: i2c@10970000 {
>>                                 pinctrl-names = "default";
>>                                 pinctrl-0 = <&hsi2c8_bus>;
>>                                 clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7>,
>> -                                        <&cmu_peric0 CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK>;
>> +                                        <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7>;
>>                                 clock-names = "hsi2c", "hsi2c_pclk";
>>                                 status = "disabled";
>>                         };
>> --
>> 2.43.0.429.g432eaa2c6b-goog
>>
André Draszik Jan. 29, 2024, 2:01 p.m. UTC | #3 
Hi Sam,

On Fri, 2024-01-26 at 21:22 -0600, Sam Protsenko wrote:
> From [1] it looks like DIV_CLK_PERIC0_USI8_USI is a common parrent for
> these two leaf gate clocks:
>   1. GOUT_BLK_PERIC0_UID_RSTNSYNC_CLK_PERIC0_USI8_USI_IPCLKPORT_CLK
>   2. GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_7
> 
> So IIUC, you replace clock #1 with clock #2 in this patch? If so, I
> think that's a right move, because in my experience RSTNSYNC clocks

That is correct.

Cheers,
Andre'
Tudor Ambarus Jan. 29, 2024, 4:33 p.m. UTC | #4 
On 1/27/24 04:00, Tudor Ambarus wrote:
>>> Wrong pclk clocks have been used in this usi8 instance here. For USI
>>> and I2C, we need the ipclk and pclk, where pclk is the bus clock.
>>> Without it, nothing can work.
>> Empty line is missing here?
>>
>>> It is unclear what exactly is using USI8_USI_CLK, but it is not
>>> required for the IP to be operational at this stage, while pclk is.
>> From [1] it looks like DIV_CLK_PERIC0_USI8_USI is a common parrent for
>> these two leaf gate clocks:
>>   1. GOUT_BLK_PERIC0_UID_RSTNSYNC_CLK_PERIC0_USI8_USI_IPCLKPORT_CLK
>>   2. GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_7
>>
>> So IIUC, you replace clock #1 with clock #2 in this patch? If so, I
> No, GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_7 is already used by
> IPCLK, the one that controls the clock frequency.
> 
> So I understand Andre' replaces a child of the USI8 with something else.
> 
> I don't think this works. We shall at least test it. I tested my usi8
> patches with the eeprom that's populated on the battery connector. I'll
> sync with Andre' offline and redo the tests on Monday.

Andre' is right, I messed up the bus clocks for USI. I tested the IPCLK,
the one that feeds USI clients, but I failed to correctly test the bus
clock. I retested by removing the clk_ignore_unused bootargs param and
verified that the patch is correct.

Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Tested-by: Tudor Ambarus <tudor.ambarus@linaro.org>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index bc251e565be6..e5b665be2d62 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -380,7 +380,7 @@  usi8: usi@109700c0 {
 			ranges;
 			#address-cells = <1>;
 			#size-cells = <1>;
-			clocks = <&cmu_peric0 CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK>,
+			clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7>,
 				 <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7>;
 			clock-names = "pclk", "ipclk";
 			samsung,sysreg = <&sysreg_peric0 0x101c>;
@@ -397,7 +397,7 @@  hsi2c_8: i2c@10970000 {
 				pinctrl-names = "default";
 				pinctrl-0 = <&hsi2c8_bus>;
 				clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7>,
-					 <&cmu_peric0 CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK>;
+					 <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7>;
 				clock-names = "hsi2c", "hsi2c_pclk";
 				status = "disabled";
 			};