Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH 1/2] drivers: clk: renesas: rzg2l-cpg: Add SDHI clk
> mux support
>
> Hi Biju,
>
> On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > Add SDHI clk mux support to select SDHI clock from different clock
> > sources.
> >
> > As per HW manual, direct clock switching from 533MHz to 400MHz and
> > vice versa is not recommended. So added support for handling this in
> > mux.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -55,6 +55,15 @@
> > #define GET_REG_SAMPLL_CLK1(val) ((val >> 22) & 0xfff)
> > #define GET_REG_SAMPLL_CLK2(val) ((val >> 12) & 0xfff)
> >
> > +struct sd_hw_data {
> > + struct clk_hw hw;
> > + u32 conf;
> > + u32 mux_flags;
>
> Do you need mux_flags? Or can this be hardcoded to zero?
OK will hardcoded to zero.
>
> > + struct rzg2l_cpg_priv *priv;
> > +};
> > +
> > +#define to_sd_hw_data(_hw) container_of(_hw, struct sd_hw_data, hw)
> > +
> > /**
> > * struct rzg2l_cpg_priv - Clock Pulse Generator Private Data
> > *
> > @@ -150,6 +159,100 @@ rzg2l_cpg_mux_clk_register(const struct
> cpg_core_clk *core,
> > return clk_hw->clk;
> > }
> >
> > +static int rzg2l_cpg_sd_clk_mux_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request
> > +*req) {
> > + struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> > +
> > + return clk_mux_determine_rate_flags(hw, req,
> > +hwdata->mux_flags); }
> > +
> > +static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8
> > +index) {
> > + struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> > + struct rzg2l_cpg_priv *priv = hwdata->priv;
> > + u32 off = GET_REG_OFFSET(hwdata->conf);
> > + u32 shift = GET_SHIFT(hwdata->conf);
> > + const u32 clk_src_266 = 2;
> > + u32 bitmask;
> > +
> > + /*
> > + * 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(hwdata->conf) - 1, 0) << shift) <<
> 16;
> > + if (index != clk_src_266)
> > + writel(bitmask | ((clk_src_266 + 1) << shift),
> > + priv->base + off);
>
> I'm wondering if you should poll (using readl_poll_timeout()) until the
> CPG_CLKSTATUS.SELSDHIx_STS bit is cleared, to indicate switching has
> completed?
OK will check this.
>
> > +
> > + writel(bitmask | ((index + 1) << shift), priv->base + off);
> > +
> > + return 0;
> > +}
> > +
> > +static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) {
> > + struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> > + struct rzg2l_cpg_priv *priv = hwdata->priv;
> > + u32 val = readl(priv->base + GET_REG_OFFSET(hwdata->conf));
> > +
> > + val >>= GET_SHIFT(hwdata->conf);
> > + val &= GENMASK(GET_WIDTH(hwdata->conf) - 1, 0);
> > + if (val)
> > + val--;
> > + else
> > + /* Prohibited clk source, change it to 533 MHz(reset
> value) */
> > + rzg2l_cpg_sd_clk_mux_set_parent(hw, 0);
>
> Please add curly braces (to both branches).
OK.
Regards,
Biju
>
> > +
> > + return val;
> > +}
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> 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
