| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdX9PYdESi6OAFSUS1NsFs9oPPV7Cd+uMHTtFRhkaq3Xzw@mail.gmail.com>
Date: Thu, 14 Sep 2023 17:18:46 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
ulf.hansson@...aro.org, linus.walleij@...aro.org,
gregkh@...uxfoundation.org, jirislaby@...nel.org,
magnus.damm@...il.com, catalin.marinas@....com, will@...nel.org,
prabhakar.mahadev-lad.rj@...renesas.com,
biju.das.jz@...renesas.com, quic_bjorande@...cinc.com,
arnd@...db.de, konrad.dybcio@...aro.org, neil.armstrong@...aro.org,
nfraprado@...labora.com, rafal@...ecki.pl,
wsa+renesas@...g-engineering.com,
linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mmc@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-serial@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 18/37] clk: renesas: rzg2l: refactor sd mux driver
Hi Claudiu,
Thanks for your patch!
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> Refactor SD MUX driver to be able to reuse the same code on RZ/G3S.
> RZ/G2{L, UL} has a limitation with regards to switching the clock source
> for SD MUX (MUX clock source has to be switched to 266MHz before switching
> b/w 533MHz and 400MHz). This limitation has been introduced as a clock
> notifier that is registered on platform based initialization data thus the
> SD MUX code could be reused on RZ/G3S.
>
> As both RZ/G2{L, UL} and RZ/G3S has specific bits in specific registers
> to check if the clock switching has been done, this configuration (register
> offset, register bits and bits width) is now passed though
> struct cpg_core_clk::sconf (status configuration) from platform specific
> initialization code.
>
> Along with struct cpg_core_clk::sconf the mux table indexes is also
indices are
> passed from platform specific initialization code.
Please also mention the passing of the mux flags, which is added so
you can pass CLK_SET_PARENT_GATE for G3S_SEL_PLL4 later.
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> --- a/drivers/clk/renesas/r9a07g043-cpg.c
> +++ b/drivers/clk/renesas/r9a07g043-cpg.c
> @@ -21,6 +21,10 @@
> #define G2UL_SEL_SDHI0 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
> #define G2UL_SEL_SDHI1 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)
>
> +/* Clock status configuration. */
> +#define G2UL_SEL_SDHI0_STS SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
> +#define G2UL_SEL_SDHI1_STS SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
Just like in [PATCH 17/37], there is no real need for the "G2UL_"-prefix.
> +
> enum clk_ids {
> /* Core Clock Outputs exported to DT */
> LAST_DT_CORE_CLK = R9A07G043_CLK_P0_DIV2,
> @@ -85,6 +89,8 @@ static const char * const sel_pll3_3[] = { ".pll3_533", ".pll3_400" };
> static const char * const sel_pll6_2[] = { ".pll6_250", ".pll5_250" };
> static const char * const sel_shdi[] = { ".clk_533", ".clk_400", ".clk_266" };
>
> +static const u32 mtable_sdhi[] = {1, 2, 3};
{ 1, 2, 3 };
(everywhere)
> +
> static const struct cpg_core_clk r9a07g043_core_clks[] __initconst = {
> /* External Clock Inputs */
> DEF_INPUT("extal", CLK_EXTAL),
> @@ -137,6 +141,77 @@ static void rzg2l_cpg_del_clk_provider(void *data)
> of_clk_del_provider(data);
> }
>
> +/* Must be called in atomic context. */
> +static int rzg2l_cpg_wait_clk_update_done(void __iomem *base, u32 conf)
> +{
> + u32 bitmask = GENMASK(GET_WIDTH(conf) - 1, 0) << GET_SHIFT(conf);
> + u32 off = GET_REG_OFFSET(conf);
> + u32 val;
> +
> + return readl_poll_timeout_atomic(base + off, val, !(val & bitmask), 100, 20000);
> +}
> +
> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event,
> + void *data)
> +{
> + struct clk_notifier_data *cnd = data;
> + struct clk_hw *hw = __clk_get_hw(cnd->clk);
> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> + u32 off = GET_REG_OFFSET(clk_hw_data->conf);
> + u32 shift = GET_SHIFT(clk_hw_data->conf);
> + const u32 clk_src_266 = 3;
> + unsigned long flags;
> + u32 bitmask;
> + int ret;
> +
> + if (event != PRE_RATE_CHANGE || (cnd->new_rate / MEGA == 266))
> + return 0;
> +
> + spin_lock_irqsave(&priv->rmw_lock, flags);
> +
> + /*
> + * As per the HW manual, we should not directly switch from 533 MHz to
> + * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> + * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> + * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> + * (400 MHz)).
> + * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> + * switching register is prohibited.
> + * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
> + * the index to value mapping is done by adding 1 to the index.
> + */
> + bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
> + writel(bitmask | (clk_src_266 << shift), priv->base + off);
> +
> + /* Wait for the update done. */
> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
> +
> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +
> + if (ret)
> + dev_err(priv->dev, "failed to switch to safe clk source\n");
> +
> + return ret;
> +}
> +
> +static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
> + struct rzg2l_cpg_priv *priv)
> +{
> + struct notifier_block *nb;
> +
> + if (!core->notifier)
> + return 0;
> +
> + nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
> + if (!nb)
> + return -ENOMEM;
> +
> + nb->notifier_call = core->notifier;
> +
> + return clk_notifier_register(hw->clk, nb);
> +}
I am not sure a notifier is the best solution. Basically on RZ/G2L,
when changing the parent clock, you need to switch to a fixed
intermediate parent first.
What about just replacing the fixed clk_src_266 in the old
rzg2l_cpg_sd_mux_clk_set_parent() by a (signed) integer in
sd_mux_hw_data (specified in DEF_SD_MUX()), representing the index
of the intermediate clock?
-1 would mean an intermediate parent is not needed.
> +
> static struct clk * __init
> rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
> struct clk **clks,
> @@ -197,72 +272,54 @@ rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
> return clk_hw->clk;
> }
>
> -static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +static u8 rzg2l_cpg_sd_mux_clk_get_parent(struct clk_hw *hw)
> +{
> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> + struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> + u32 val;
> +
> + val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
> + val >>= GET_SHIFT(clk_hw_data->conf);
> + val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
> +
> + return clk_mux_val_to_index(hw, sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, val);
> +}
> +
> +static int rzg2l_cpg_sd_mux_clk_set_parent(struct clk_hw *hw, u8 index)
> {
> struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> + struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
> struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> u32 off = GET_REG_OFFSET(clk_hw_data->conf);
> u32 shift = GET_SHIFT(clk_hw_data->conf);
> - const u32 clk_src_266 = 2;
> - u32 msk, val, bitmask;
> unsigned long flags;
> + u32 bitmask, val;
> int ret;
>
> - /*
> - * As per the HW manual, we should not directly switch from 533 MHz to
> - * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> - * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> - * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> - * (400 MHz)).
> - * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> - * switching register is prohibited.
> - * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
> - * the index to value mapping is done by adding 1 to the index.
> - */
> + val = clk_mux_index_to_val(sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, index);
> +
> bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
> +
> spin_lock_irqsave(&priv->rmw_lock, flags);
> - if (index != clk_src_266) {
> - writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off);
>
> - msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS;
> + writel(bitmask | (val << shift), priv->base + off);
>
> - ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
> - !(val & msk), 100,
> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
> - if (ret)
> - goto unlock;
> - }
> + /* Wait for the update done. */
> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
>
> - writel(bitmask | ((index + 1) << shift), priv->base + off);
> -
> - ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
> - !(val & msk), 100,
> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
> -unlock:
> spin_unlock_irqrestore(&priv->rmw_lock, flags);
>
> if (ret)
> - dev_err(priv->dev, "failed to switch clk source\n");
> + dev_err(priv->dev, "Failed to switch parent\n");
>
> return ret;
> }
>
> -static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)
> -{
> - struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> - struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> - u32 val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
> -
> - val >>= GET_SHIFT(clk_hw_data->conf);
> - val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
> -
> - return val ? --val : val;
> -}
This would be easier to review if you kept the order and name of the
.[gs]et_parent() callbacks.
> -
> static const struct clk_ops rzg2l_cpg_sd_clk_mux_ops = {
> .determine_rate = __clk_mux_determine_rate_closest,
> - .set_parent = rzg2l_cpg_sd_clk_mux_set_parent,
> - .get_parent = rzg2l_cpg_sd_clk_mux_get_parent,
> + .set_parent = rzg2l_cpg_sd_mux_clk_set_parent,
> + .get_parent = rzg2l_cpg_sd_mux_clk_get_parent,
> };
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -86,8 +88,11 @@ struct cpg_core_clk {
> unsigned int mult;
> unsigned int type;
> unsigned int conf;
> + unsigned int sconf;
> const struct clk_div_table *dtable;
> + const u32 *mtable;
> const char * const *parent_names;
> + notifier_fn_t notifier;
FTR, this is growing each core clock entry by 24 bytes (on arm64).
We really should start using unions, but that is a bigger overhaul...
> u32 flag;
> u32 mux_flags;
> int num_parents;
> @@ -272,4 +278,9 @@ extern const struct rzg2l_cpg_info r9a07g044_cpg_info;
> extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
> extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
>
> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
> +
> +/* Macros to be used in platform specific initialization code. */
> +#define SD_MUX_NOTIF (&rzg2l_cpg_sd_mux_clk_notifier)
Any specific reason you are adding this macro?
What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists