diff mbox series

[4/4] clk: samsung: exynos850: fix propagation of SPI IPCLK rate

Message ID 20240229122021.1901785-5-tudor.ambarus@linaro.org (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: samsung: introduce nMUX to reparent MUX clocks | expand

Commit Message

Tudor Ambarus Feb. 29, 2024, 12:20 p.m. UTC
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(-)

Comments

Sam Protsenko March 1, 2024, 12:13 a.m. UTC | #1
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]
Tudor Ambarus March 1, 2024, 11:28 a.m. UTC | #2
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
Tudor Ambarus March 22, 2024, 9:39 a.m. UTC | #3
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
Sam Protsenko March 22, 2024, 6:09 p.m. UTC | #4
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
Tudor Ambarus March 25, 2024, 7:15 a.m. UTC | #5
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 mbox series

Patch

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 = {