| Message ID | 20240127003607.501086-4-andre.draszik@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/5] clk: samsung: gs101: gpio_peric0_pclk needs to be kept on | expand |
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 >
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 >>
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'
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 --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"; };
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(-)