| Message ID | 20240229122021.1901785-5-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: samsung: introduce nMUX to reparent MUX clocks | expand |
On Thu, Feb 29, 2024 at 6:20 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Fix propagation of SPI IPCLK rate by allowing MUX reparenting for the > dedicated USI MUX clocks. Since these muxes feed just the USI blocks, > reparenting of the muxes do not affect other IPs. > It was actually done for a reason (at least in case of clk-exynos850 driver). Those top-level MUXes use CLK_SET_RATE_NO_REPARENT flag via MUX() / MUX_F() macros to avoid re-parenting. See below for the explanation on why. > Do not propagate the rate change the from USI muxes to the common bus > dividers (dout_apm_bus and dout_peri_ip). The leaf clocks (HSI2C, I3C) The propagation to those dividers was implemented intentionally, because AFAIU this is precisely their purpose. Not using those for the derivation of HSI2C/SPI clock rates makes them effectively useless, which as I understand wasn't the HW designer's intention. It's all explained in details in the commit message of the patch which adds this propagation. > that are derived from the common bus dividers are no longer affected by > the SPI clock rate change. > > This change involves the following clock path propagation: > > usi_spi_0: > Clock Div range MUX Selection > --------------------------------------------------------------------- > gout_spi0_ipclk - - > dout_peri_spi0 /1..32 - > mout_peri_spi_user - { oscclk (26 MHz), dout_peri_ip } AFAIK, the OSCCLK only purpose is to be used during suspend (PM state). When implementing clk-exynos850.c I specifically avoided using OSCCLK clock for the regular use-cases, and I believe other existing Exynos clock drivers don't use OSCCLK during normal operation too. It's easy to see from the clock diagrams in the TRM: all CMUs have top-level MUXes that have two parents (normal clock and OSCCLK). In fact, the TRM mentions it: "All CMUs have MUXs to change the OSCCLK during power-down mode" Even if OSCCLK can be used in some cases for driving HW blocks, the top-level MUXes are not related to those cases. > > *Note that the clock rate is no longer propagated to dout_peri_ip. > > usi_cmgp0: > > Clock Div range MUX Selection > --------------------------------------------------------------------- > gout_cmgp_usi0_ipclk - - > dout_cmgp_usi0 /1..32 - > mout_cmgp_usi0 - { clk_rco_cmgp (49.152 MHz) I'm not sure the RCO should be used during normal operation either. RCO purpose seems to be similar OSCCLK -- to serve as a substitute clock during suspend, or maybe for calibration/debugging purposes. But from the clock diagram it's clear they are not intended for the regular operation. The only difference from OSSCLK is the frequency, which is usually 49.152 MHz or 24.576 MHz for RCO (basically multiples of 32,768 Hz), which hints those clocks are designed to drive some 1 Hz (1 sec) based timers. > gout_clkcmu_cmgp_bus } > > *Note that the clock rate is no longer propagated to > gout_clkcmu_cmgp_bus and dout_apm_bus. > > usi_cmgp1: > > Clock Div range MUX Selection > --------------------------------------------------------------------- > gout_cmgp_usi1_ipclk - - > dout_cmgp_usi1 /1..32 - > mout_cmgp_usi1 - { clk_rco_cmgp (49.152 MHz) > gout_clkcmu_cmgp_bus } > > *Note that the clock rate is no longer propagated to > gout_clkcmu_cmgp_bus and dout_apm_bus. > > This comes with no significant clock range modification. Before this > patch the claimed clock ranges are: > > SPI0: 200 kHz ... 49.9 MHz > SPI1/2: 400 kHz ... 49.9 MHz > > After this patch the clock ranges are: > SPI0: 203.125 kHz ... 49.9 MHz > SPI1/2: 384 kHz ... 49.9 MHz > So as I see it, instead of using dividers designed exactly for the purpose of changing I2C/SPI clock rates this patch instead uses OSCCLK clock, which is not intended for normal I2C/SPI operation. > For SPI1/2 we get an even lower frequency than what was before. For SPI0 > the benefit of not modifying common bus clocks, thus other leaf IP nodes > is greater than the change in frequency from 200 to ~203 KHz. > > Not tested, the patch was written solely by reading the code. > > Fixes: 67c15187d491 ("clk: samsung: exynos850: Propagate SPI IPCLK rate change") I fail to see how this patch fixes anything. Instead it looks to me it replaces the (already) correctly implemented logic with incorrect one. The SPI clocks are already functional and work exactly as intended without this patch. The motivation was explained in commit 67c15187d491 ("clk: samsung: exynos850: Propagate SPI IPCLK rate change"), which was thoroughly tested on E850-96 for all 3 SPI instances, for all possible DMA/IRQ/polling combinations, with all possible clock frequencies, and it seems to cover all possible SPI cases. This patch seems to just change the behavior to something else without solid examples of how the already implemented scheme (where I specifically avoided doing what's done in this patch) could be broken or sub-optimal. [snip]
Hi, Sam, On 3/1/24 00:13, Sam Protsenko wrote: >> mout_peri_spi_user - { oscclk (26 MHz), dout_peri_ip } > AFAIK, the OSCCLK only purpose is to be used during suspend (PM > state). When implementing clk-exynos850.c I specifically avoided using > OSCCLK clock for the regular use-cases, and I believe other existing Ok. > Exynos clock drivers don't use OSCCLK during normal operation too. I saw. > It's easy to see from the clock diagrams in the TRM: all CMUs have > top-level MUXes that have two parents (normal clock and OSCCLK). In > fact, the TRM mentions it: > > "All CMUs have MUXs to change the OSCCLK during power-down mode" > typo in datasheet, s/the/to, but I get what you're saying. > Even if OSCCLK can be used in some cases for driving HW blocks, the > top-level MUXes are not related to those cases. This is what I'm challenging, yes. Do you know why we can't use oscclk to drive hw blocks in normal operation mode, i.e. not low power modes? Since the datasheet does not specify any other usage of oscclk, I think too that it's safer to not use it to drive HW blocks. So unless someone else intervenes and clarifies this aspect, please ignore the entire patch set. Re-parenting the MUX to oscclk allows the same clock range as before and with the benefit of not affecting the clock rates of HSI2C/I3C for SPI clock rates below 500 KHz. This is what I'm trying to fix here, I think it's not a good idea to allow SPI to modify the clock rate of HSI2C/I3C at run-time. How about specifying CLK_SET_RATE_GATE to the common bus divider? It will prevent SPI from changing the rate while the clock is prepared. Thus HSI2C/I3C will no longer be affected behind the curtains. Thanks, ta
Hi, Sam! On 3/1/24 00:13, Sam Protsenko wrote: > I fail to see how this patch fixes anything. Instead it looks to me it > replaces the (already) correctly implemented logic with incorrect one. I opened another thread asking for feedback on whether it's safe to re-parent the USI MUX to OSCCLK at run-time, find it here: https://lore.kernel.org/linux-samsung-soc/71df1d6b-f40b-4896-a672-c5f0f526fb1f@linaro.org/T/#m588abb87eb5fd8817d71d06b94c91eb84928e06b Jaewon came up with the idea on verifying what the downstream clock driver does. I added some prints in the driver, and indeed the USI MUX re-parents to OSCCLK on low SPI clock rates in the GS101 case. Thus I'll respin this patch set fixing GS101 on low USI clock rates by re-parenting the USI MUX to OSCCLK. I'll leave exynos850 out if I don't hear back from you, but I think it deserves the same fix. Allowing SPI to modify the clock rate of HSI2C/I3C at run-time is bad IMO. Re-parenting the USI MUX to OSCCLK fixes this problem, HSI2C/I3C will no longer be affected on low SPI clock rates. Cheers, ta
On Fri, Mar 22, 2024 at 4:39 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, Sam! > > On 3/1/24 00:13, Sam Protsenko wrote: > > I fail to see how this patch fixes anything. Instead it looks to me it > > replaces the (already) correctly implemented logic with incorrect one. > > I opened another thread asking for feedback on whether it's safe to > re-parent the USI MUX to OSCCLK at run-time, find it here: > https://lore.kernel.org/linux-samsung-soc/71df1d6b-f40b-4896-a672-c5f0f526fb1f@linaro.org/T/#m588abb87eb5fd8817d71d06b94c91eb84928e06b > > Jaewon came up with the idea on verifying what the downstream clock > driver does. I added some prints in the driver, and indeed the USI MUX > re-parents to OSCCLK on low SPI clock rates in the GS101 case. > > Thus I'll respin this patch set fixing GS101 on low USI clock rates by > re-parenting the USI MUX to OSCCLK. I'll leave exynos850 out if I don't > hear back from you, but I think it deserves the same fix. Allowing SPI > to modify the clock rate of HSI2C/I3C at run-time is bad IMO. > Re-parenting the USI MUX to OSCCLK fixes this problem, HSI2C/I3C will no > longer be affected on low SPI clock rates. > Yes, please leave Exynos850 out of it, if possible. It's fine with me if you send it for gs101, as it's you who is going to maintain that platform further, so it's for the maintainers to decide. I'll refrain from reviewing that particular patch. For Exynos850 driver, I'm convinced the SPI clock derivation is already implemented in the correct way (exactly as it was designed in HW), and doing anything else would be a hack, and frankly this sole fact is already enough of argumentation for me. There is also the whole bunch of use-cases which I think could be affected by using OSCCLK, e.g.: clock signal integrity, runtime PM concerns, possible interference in case of automatic clock control enablement, etc. I don't even want to think about all possible pitfalls which implementation of this non-standard and undocumented behavior could create. So as the only person who currently supports Exynos850 drivers (apart from the maintainers, of course), I would strictly oppose this particular OSCCLK change. > Cheers, > ta
On 3/22/24 18:09, Sam Protsenko wrote:
> Yes, please leave Exynos850 out of it, if possible.
Sure, Sam, thanks for the feedback!
Cheers,
ta
diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c index 82cfa22c0788..42b4b4075aeb 100644 --- a/drivers/clk/samsung/clk-exynos850.c +++ b/drivers/clk/samsung/clk-exynos850.c @@ -605,7 +605,7 @@ static const struct samsung_div_clock apm_div_clks[] __initconst = { static const struct samsung_gate_clock apm_gate_clks[] __initconst = { GATE(CLK_GOUT_CLKCMU_CMGP_BUS, "gout_clkcmu_cmgp_bus", "dout_apm_bus", - CLK_CON_GAT_CLKCMU_CMGP_BUS, 21, CLK_SET_RATE_PARENT, 0), + CLK_CON_GAT_CLKCMU_CMGP_BUS, 21, 0, 0), GATE(CLK_GOUT_CLKCMU_CHUB_BUS, "gout_clkcmu_chub_bus", "mout_clkcmu_chub_bus", CLK_CON_GAT_GATE_CLKCMU_CHUB_BUS, 21, 0, 0), @@ -974,10 +974,10 @@ static const struct samsung_fixed_rate_clock cmgp_fixed_clks[] __initconst = { static const struct samsung_mux_clock cmgp_mux_clks[] __initconst = { MUX(CLK_MOUT_CMGP_ADC, "mout_cmgp_adc", mout_cmgp_adc_p, CLK_CON_MUX_CLK_CMGP_ADC, 0, 1), - MUX_F(CLK_MOUT_CMGP_USI0, "mout_cmgp_usi0", mout_cmgp_usi0_p, - CLK_CON_MUX_MUX_CLK_CMGP_USI_CMGP0, 0, 1, CLK_SET_RATE_PARENT, 0), - MUX_F(CLK_MOUT_CMGP_USI1, "mout_cmgp_usi1", mout_cmgp_usi1_p, - CLK_CON_MUX_MUX_CLK_CMGP_USI_CMGP1, 0, 1, CLK_SET_RATE_PARENT, 0), + nMUX(CLK_MOUT_CMGP_USI0, "mout_cmgp_usi0", mout_cmgp_usi0_p, + CLK_CON_MUX_MUX_CLK_CMGP_USI_CMGP0, 0, 1), + nMUX(CLK_MOUT_CMGP_USI1, "mout_cmgp_usi1", mout_cmgp_usi1_p, + CLK_CON_MUX_MUX_CLK_CMGP_USI_CMGP1, 0, 1), }; static const struct samsung_div_clock cmgp_div_clks[] __initconst = { @@ -1557,9 +1557,8 @@ static const struct samsung_mux_clock peri_mux_clks[] __initconst = { mout_peri_uart_user_p, PLL_CON0_MUX_CLKCMU_PERI_UART_USER, 4, 1), MUX(CLK_MOUT_PERI_HSI2C_USER, "mout_peri_hsi2c_user", mout_peri_hsi2c_user_p, PLL_CON0_MUX_CLKCMU_PERI_HSI2C_USER, 4, 1), - MUX_F(CLK_MOUT_PERI_SPI_USER, "mout_peri_spi_user", - mout_peri_spi_user_p, PLL_CON0_MUX_CLKCMU_PERI_SPI_USER, 4, 1, - CLK_SET_RATE_PARENT, 0), + nMUX(CLK_MOUT_PERI_SPI_USER, "mout_peri_spi_user", + mout_peri_spi_user_p, PLL_CON0_MUX_CLKCMU_PERI_SPI_USER, 4, 1), }; static const struct samsung_div_clock peri_div_clks[] __initconst = {
Fix propagation of SPI IPCLK rate by allowing MUX reparenting for the dedicated USI MUX clocks. Since these muxes feed just the USI blocks, reparenting of the muxes do not affect other IPs. Do not propagate the rate change the from USI muxes to the common bus dividers (dout_apm_bus and dout_peri_ip). The leaf clocks (HSI2C, I3C) that are derived from the common bus dividers are no longer affected by the SPI clock rate change. This change involves the following clock path propagation: usi_spi_0: Clock Div range MUX Selection --------------------------------------------------------------------- gout_spi0_ipclk - - dout_peri_spi0 /1..32 - mout_peri_spi_user - { oscclk (26 MHz), dout_peri_ip } *Note that the clock rate is no longer propagated to dout_peri_ip. usi_cmgp0: Clock Div range MUX Selection --------------------------------------------------------------------- gout_cmgp_usi0_ipclk - - dout_cmgp_usi0 /1..32 - mout_cmgp_usi0 - { clk_rco_cmgp (49.152 MHz) gout_clkcmu_cmgp_bus } *Note that the clock rate is no longer propagated to gout_clkcmu_cmgp_bus and dout_apm_bus. usi_cmgp1: Clock Div range MUX Selection --------------------------------------------------------------------- gout_cmgp_usi1_ipclk - - dout_cmgp_usi1 /1..32 - mout_cmgp_usi1 - { clk_rco_cmgp (49.152 MHz) gout_clkcmu_cmgp_bus } *Note that the clock rate is no longer propagated to gout_clkcmu_cmgp_bus and dout_apm_bus. This comes with no significant clock range modification. Before this patch the claimed clock ranges are: SPI0: 200 kHz ... 49.9 MHz SPI1/2: 400 kHz ... 49.9 MHz After this patch the clock ranges are: SPI0: 203.125 kHz ... 49.9 MHz SPI1/2: 384 kHz ... 49.9 MHz For SPI1/2 we get an even lower frequency than what was before. For SPI0 the benefit of not modifying common bus clocks, thus other leaf IP nodes is greater than the change in frequency from 200 to ~203 KHz. Not tested, the patch was written solely by reading the code. Fixes: 67c15187d491 ("clk: samsung: exynos850: Propagate SPI IPCLK rate change") Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/clk/samsung/clk-exynos850.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)