Message-ID: <CAPLW+4=jSr6ZsB7XekXsiUBm0SmVpVFnqpgjViuF3=HpT4gRAg@mail.gmail.com>
Date: Thu, 29 Feb 2024 18:13:46 -0600
From: Sam Protsenko <semen.protsenko@...aro.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: krzysztof.kozlowski@...aro.org, s.nawrocki@...sung.com, 
	cw00.choi@...sung.com, alim.akhtar@...sung.com, mturquette@...libre.com, 
	sboyd@...nel.org, peter.griffin@...aro.org, andre.draszik@...aro.org, 
	linux-samsung-soc@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	willmcvicker@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH 4/4] clk: samsung: exynos850: fix propagation of SPI IPCLK rate

On Thu, Feb 29, 2024 at 6:20 AM Tudor Ambarus <tudor.ambarus@...aroorg> 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]

Powered by blists - more mailing lists