Received-SPF: pass (google.com: domain of linux-kernel+bounces-122833-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240326172813.801470-1-tudor.ambarus@linaro.org> <20240326172813.801470-3-tudor.ambarus@linaro.org>
In-Reply-To: <20240326172813.801470-3-tudor.ambarus@linaro.org>
From: Peter Griffin <peter.griffin@linaro.org>
Date: Thu, 28 Mar 2024 11:33:35 +0000
Message-ID: <CADrjBPqxbgSxEGdor059eGXfpmE4O4e49EroXkpRW0vZEAqGJA@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] clk: samsung: gs101: propagate PERIC1 USI SPI
 clock rate
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: krzysztof.kozlowski@linaro.org, alim.akhtar@samsung.com, 
	linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, andre.draszik@linaro.org, 
	willmcvicker@google.com, kernel-team@android.com, s.nawrocki@samsung.com, 
	cw00.choi@samsung.com, mturquette@baylibre.com, sboyd@kernel.org, 
	semen.protsenko@linaro.org, linux-clk@vger.kernel.org, 
	jaewon02.kim@samsung.com
Content-Type: text/plain; charset="UTF-8"

Hi Tudor,

On Tue, 26 Mar 2024 at 17:28, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> When SPI transfer is being prepared, the spi-s3c64xx driver will call
> clk_set_rate() to change the rate of SPI source clock (IPCLK). But IPCLK
> is a gate (leaf) clock, so it must propagate the rate change up the
> clock tree, so that corresponding MUX/DIV clocks can actually change
> their values. Add CLK_SET_RATE_PARENT flag to corresponding clocks for
> all USI instances in GS101 PERIC1: USI{0, 9, 10, 11, 12, 13}. This change
> involves the following clocks:
>
> PERIC1 USI*:
>
>     Clock                              Div range    MUX Selection
>     -------------------------------------------------------------------
>     gout_peric1_peric1_top0_ipclk_*    -            -
>     dout_peric1_usi*_usi               /1..16       -
>     mout_peric1_usi*_usi_user          -            {24.5 MHz, 400 MHz}
>
> With input clock of 400 MHz this scheme provides the following IPCLK
> rate range, for each USI block:
>
>     PERIC1 USI*:       1.5 MHz ... 400 MHz
>
> Accounting for internal /4 divider in SPI blocks, and because the max
> SPI frequency is limited at 50 MHz, it gives us next SPI SCK rates:
>
>     PERIC1 USI_SPI*:   384 KHz ... 49.9 MHz
>
> Which shall be fine for the applications of the SPI bus.
>
> Note that with this we allow the reparenting of the MUX_USIx clocks to
> OSCCLK. Each instance of the USI IP has its own MUX_USI clock, thus the
> reparenting of a MUX_USI clock corresponds to a single instance of the
> USI IP. The datasheet mentions OSCCLK just in the low-power mode
> context, but the downstream driver reparents too the MUX_USI clocks to
> OSCCLK. Follow the downstream driver and do the same.
>
> Fixes: 63b4bd1259d9 ("clk: samsung: gs101: add support for cmu_peric1")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Thanks Tudor for looking into this! I agree with the approach outlined above.
Given that
1) Samsung engineers OK'd it
2) Downstream clock driver does it (and also has other features
enabled like automatic clock gating enabled, which implies it should
not cause any issues there if we enable it in the future upstream).
3) We don't want to change clock frequencies of other IP instances

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>


>  drivers/clk/samsung/clk-gs101.c | 90 ++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> index d065e343a85d..ddf2d57eed68 100644
> --- a/drivers/clk/samsung/clk-gs101.c
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -3230,47 +3230,53 @@ static const struct samsung_mux_clock peric1_mux_clks[] __initconst = {
>         MUX(CLK_MOUT_PERIC1_I3C_USER,
>             "mout_peric1_i3c_user", mout_peric1_nonbususer_p,
>             PLL_CON0_MUX_CLKCMU_PERIC1_I3C_USER, 4, 1),
> -       MUX(CLK_MOUT_PERIC1_USI0_USI_USER,
> -           "mout_peric1_usi0_usi_user", mout_peric1_nonbususer_p,
> -           PLL_CON0_MUX_CLKCMU_PERIC1_USI0_USI_USER, 4, 1),
> -       MUX(CLK_MOUT_PERIC1_USI10_USI_USER,
> -           "mout_peric1_usi10_usi_user", mout_peric1_nonbususer_p,
> -           PLL_CON0_MUX_CLKCMU_PERIC1_USI10_USI_USER, 4, 1),
> -       MUX(CLK_MOUT_PERIC1_USI11_USI_USER,
> -           "mout_peric1_usi11_usi_user", mout_peric1_nonbususer_p,
> -           PLL_CON0_MUX_CLKCMU_PERIC1_USI11_USI_USER, 4, 1),
> -       MUX(CLK_MOUT_PERIC1_USI12_USI_USER,
> -           "mout_peric1_usi12_usi_user", mout_peric1_nonbususer_p,
> -           PLL_CON0_MUX_CLKCMU_PERIC1_USI12_USI_USER, 4, 1),
> -       MUX(CLK_MOUT_PERIC1_USI13_USI_USER,
> -           "mout_peric1_usi13_usi_user", mout_peric1_nonbususer_p,
> -           PLL_CON0_MUX_CLKCMU_PERIC1_USI13_USI_USER, 4, 1),
> -       MUX(CLK_MOUT_PERIC1_USI9_USI_USER,
> -           "mout_peric1_usi9_usi_user", mout_peric1_nonbususer_p,
> -           PLL_CON0_MUX_CLKCMU_PERIC1_USI9_USI_USER, 4, 1),
> +       nMUX(CLK_MOUT_PERIC1_USI0_USI_USER,
> +            "mout_peric1_usi0_usi_user", mout_peric1_nonbususer_p,
> +            PLL_CON0_MUX_CLKCMU_PERIC1_USI0_USI_USER, 4, 1),
> +       nMUX(CLK_MOUT_PERIC1_USI10_USI_USER,
> +            "mout_peric1_usi10_usi_user", mout_peric1_nonbususer_p,
> +            PLL_CON0_MUX_CLKCMU_PERIC1_USI10_USI_USER, 4, 1),
> +       nMUX(CLK_MOUT_PERIC1_USI11_USI_USER,
> +            "mout_peric1_usi11_usi_user", mout_peric1_nonbususer_p,
> +            PLL_CON0_MUX_CLKCMU_PERIC1_USI11_USI_USER, 4, 1),
> +       nMUX(CLK_MOUT_PERIC1_USI12_USI_USER,
> +            "mout_peric1_usi12_usi_user", mout_peric1_nonbususer_p,
> +            PLL_CON0_MUX_CLKCMU_PERIC1_USI12_USI_USER, 4, 1),
> +       nMUX(CLK_MOUT_PERIC1_USI13_USI_USER,
> +            "mout_peric1_usi13_usi_user", mout_peric1_nonbususer_p,
> +            PLL_CON0_MUX_CLKCMU_PERIC1_USI13_USI_USER, 4, 1),
> +       nMUX(CLK_MOUT_PERIC1_USI9_USI_USER,
> +            "mout_peric1_usi9_usi_user", mout_peric1_nonbususer_p,
> +            PLL_CON0_MUX_CLKCMU_PERIC1_USI9_USI_USER, 4, 1),
>  };
>
>  static const struct samsung_div_clock peric1_div_clks[] __initconst = {
>         DIV(CLK_DOUT_PERIC1_I3C, "dout_peric1_i3c", "mout_peric1_i3c_user",
>             CLK_CON_DIV_DIV_CLK_PERIC1_I3C, 0, 4),
> -       DIV(CLK_DOUT_PERIC1_USI0_USI,
> -           "dout_peric1_usi0_usi", "mout_peric1_usi0_usi_user",
> -           CLK_CON_DIV_DIV_CLK_PERIC1_USI0_USI, 0, 4),
> -       DIV(CLK_DOUT_PERIC1_USI10_USI,
> -           "dout_peric1_usi10_usi", "mout_peric1_usi10_usi_user",
> -           CLK_CON_DIV_DIV_CLK_PERIC1_USI10_USI, 0, 4),
> -       DIV(CLK_DOUT_PERIC1_USI11_USI,
> -           "dout_peric1_usi11_usi", "mout_peric1_usi11_usi_user",
> -           CLK_CON_DIV_DIV_CLK_PERIC1_USI11_USI, 0, 4),
> -       DIV(CLK_DOUT_PERIC1_USI12_USI,
> -           "dout_peric1_usi12_usi", "mout_peric1_usi12_usi_user",
> -           CLK_CON_DIV_DIV_CLK_PERIC1_USI12_USI, 0, 4),
> -       DIV(CLK_DOUT_PERIC1_USI13_USI,
> -           "dout_peric1_usi13_usi", "mout_peric1_usi13_usi_user",
> -           CLK_CON_DIV_DIV_CLK_PERIC1_USI13_USI, 0, 4),
> -       DIV(CLK_DOUT_PERIC1_USI9_USI,
> -           "dout_peric1_usi9_usi", "mout_peric1_usi9_usi_user",
> -           CLK_CON_DIV_DIV_CLK_PERIC1_USI9_USI, 0, 4),
> +       DIV_F(CLK_DOUT_PERIC1_USI0_USI,
> +             "dout_peric1_usi0_usi", "mout_peric1_usi0_usi_user",
> +             CLK_CON_DIV_DIV_CLK_PERIC1_USI0_USI, 0, 4,
> +             CLK_SET_RATE_PARENT, 0),
> +       DIV_F(CLK_DOUT_PERIC1_USI10_USI,
> +             "dout_peric1_usi10_usi", "mout_peric1_usi10_usi_user",
> +             CLK_CON_DIV_DIV_CLK_PERIC1_USI10_USI, 0, 4,
> +             CLK_SET_RATE_PARENT, 0),
> +       DIV_F(CLK_DOUT_PERIC1_USI11_USI,
> +             "dout_peric1_usi11_usi", "mout_peric1_usi11_usi_user",
> +             CLK_CON_DIV_DIV_CLK_PERIC1_USI11_USI, 0, 4,
> +             CLK_SET_RATE_PARENT, 0),
> +       DIV_F(CLK_DOUT_PERIC1_USI12_USI,
> +             "dout_peric1_usi12_usi", "mout_peric1_usi12_usi_user",
> +             CLK_CON_DIV_DIV_CLK_PERIC1_USI12_USI, 0, 4,
> +             CLK_SET_RATE_PARENT, 0),
> +       DIV_F(CLK_DOUT_PERIC1_USI13_USI,
> +             "dout_peric1_usi13_usi", "mout_peric1_usi13_usi_user",
> +             CLK_CON_DIV_DIV_CLK_PERIC1_USI13_USI, 0, 4,
> +             CLK_SET_RATE_PARENT, 0),
> +       DIV_F(CLK_DOUT_PERIC1_USI9_USI,
> +             "dout_peric1_usi9_usi", "mout_peric1_usi9_usi_user",
> +             CLK_CON_DIV_DIV_CLK_PERIC1_USI9_USI, 0, 4,
> +             CLK_SET_RATE_PARENT, 0),
>  };
>
>  static const struct samsung_gate_clock peric1_gate_clks[] __initconst = {
> @@ -3305,27 +3311,27 @@ static const struct samsung_gate_clock peric1_gate_clks[] __initconst = {
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_1,
>              "gout_peric1_peric1_top0_ipclk_1", "dout_peric1_usi0_usi",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_1,
> -            21, 0, 0),
> +            21, CLK_SET_RATE_PARENT, 0),
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_2,
>              "gout_peric1_peric1_top0_ipclk_2", "dout_peric1_usi9_usi",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_2,
> -            21, 0, 0),
> +            21, CLK_SET_RATE_PARENT, 0),
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_3,
>              "gout_peric1_peric1_top0_ipclk_3", "dout_peric1_usi10_usi",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_3,
> -            21, 0, 0),
> +            21, CLK_SET_RATE_PARENT, 0),
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_4,
>              "gout_peric1_peric1_top0_ipclk_4", "dout_peric1_usi11_usi",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_4,
> -            21, 0, 0),
> +            21, CLK_SET_RATE_PARENT, 0),
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_5,
>              "gout_peric1_peric1_top0_ipclk_5", "dout_peric1_usi12_usi",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_5,
> -            21, 0, 0),
> +            21, CLK_SET_RATE_PARENT, 0),
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_6,
>              "gout_peric1_peric1_top0_ipclk_6", "dout_peric1_usi13_usi",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_6,
> -            21, 0, 0),
> +            21, CLK_SET_RATE_PARENT, 0),
>         GATE(CLK_GOUT_PERIC1_PERIC1_TOP0_IPCLK_8,
>              "gout_peric1_peric1_top0_ipclk_8", "dout_peric1_i3c",
>              CLK_CON_GAT_GOUT_BLK_PERIC1_UID_PERIC1_TOP0_IPCLKPORT_IPCLK_8,
> --
> 2.44.0.396.g6e790dbe36-goog
>