| Message ID | 20230912045157.177966-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK | expand |
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > This patch series adds initial support for The Renesas RZ/G3S (R9A08G045{S33}) > SoC. The RZ/G3S device is a general-purpose microprocessor with a > single-core Arm® Cortex®-A55 (1.1GHz) and a dual-core Arm® Cortex®-M33 (250MHz), > perfect for an IOT gateway controller. I saw some of the patches are fixes. I expect that you and Geert figure these out so I can get a separate pull request for those ASAP. (Unless they are nonurgent.) For new code try to use <linux/cleanup.h>. Or if you prefer take a sweep and introduce scoped guards everywhere (for spinlocks, mutexes..). Yours, Linus Walleij
Hi Linus, On Tue, Sep 12, 2023 at 10:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > For new code try to use <linux/cleanup.h>. > Or if you prefer take a sweep and introduce scoped guards > everywhere (for spinlocks, mutexes..). Hmmm, <linux/cleanup.h> is only available in v6.5 and later. I don't know whether the CiP machinery is planning to backport <linux/cleanup.h> to e.g. v6.1 LTS... Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 11:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Sep 12, 2023 at 10:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > For new code try to use <linux/cleanup.h>. > > Or if you prefer take a sweep and introduce scoped guards > > everywhere (for spinlocks, mutexes..). > > Hmmm, <linux/cleanup.h> is only available in v6.5 and later. > I don't know whether the CiP machinery is planning to backport > <linux/cleanup.h> to e.g. v6.1 LTS... Only for new code! (for-v6.7+) Yours, Linus Walleij
On 9/12/23 7:51 AM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Initial value of CPG_PL2SDHI_DSEL bits 0..1 or 4..6 is 01b. Hardware user's > manual (r01uh0914ej0130-rzg2l-rzg2lc.pdf) specifies that setting 0 is > prohibited. The rzg2l_cpg_sd_clk_mux_get_parent() should just read > CPG_PL2SDHI_DSEL, trust the value and return the proper clock parent index > based on the read value. Do this. > > Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/clk/renesas/rzg2l-cpg.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c > index 1195d4b1f545..d0d086d6dc51 100644 > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -239,14 +239,8 @@ static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) > > 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); > - } > > - return val; > + return val ? --val : val; return val ? val - 1 : 0;
Hi, Linus, On 12.09.2023 12:05, Linus Walleij wrote: > On Tue, Sep 12, 2023 at 11:03 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Tue, Sep 12, 2023 at 10:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > >>> For new code try to use <linux/cleanup.h>. >>> Or if you prefer take a sweep and introduce scoped guards >>> everywhere (for spinlocks, mutexes..). >> >> Hmmm, <linux/cleanup.h> is only available in v6.5 and later. >> I don't know whether the CiP machinery is planning to backport >> <linux/cleanup.h> to e.g. v6.1 LTS... > > Only for new code! (for-v6.7+) Would you prefer <linux/cleanup.h> even if the new code just uses the already existing spinlocks, mutexes? Or only for new code that introduces new spinlocks, mutexes? Thank you, Claudiu Beznea > > Yours, > Linus Walleij
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add support to identify the RZ/G3S (R9A08G045) SoC. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-devel for v6.7. 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
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Remove blank lines. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-devel for v6.7. 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
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Hardware user manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf, > chapter 7.4.7 Procedure for Switching Clocks by the Dynamic Switching > Frequency Selectors) specifies that we need to check CPG_PL2SDHI_DSEL for > SD clock switching status. > > Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -188,7 +188,8 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > u32 off = GET_REG_OFFSET(hwdata->conf); > u32 shift = GET_SHIFT(hwdata->conf); > const u32 clk_src_266 = 2; > - u32 bitmask; > + u32 msk, val, bitmask; > + int ret; > > /* > * As per the HW manual, we should not directly switch from 533 MHz to > @@ -203,9 +204,6 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > */ > bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16; > if (index != clk_src_266) { > - u32 msk, val; > - int ret; > - > writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off); > > msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS; > @@ -221,7 +219,13 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > > writel(bitmask | ((index + 1) << shift), priv->base + off); > > - return 0; > + ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, > + !(val & msk), 100, "msk" may be uninitialized. > + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); > + if (ret) > + dev_err(priv->dev, "failed to switch clk source\n"); > + > + return ret; This is now (supposed to be) doing the same thing twice, once using clk_src_266, and then again with the wanted index, so why not introduce a small helper? That would have avoided the uninitialized variable, too. I know you're rewriting this code in "[PATCH 18/37] clk: renesas: rzg2l: refactor sd mux driver", but even after that, you always do a register write before calling rzg2l_cpg_wait_clk_update_done(), so it may still be a net win. > } > > static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > SD MUX output (SD0) is further divided by 4 in G2{L, UL}. The divided > clock is SD0_DIV4. SD0_DIV4 is registered with CLK_SET_RATE_PARENT which > means a rate request for it is propagated to the MUX and could reach > rzg2l_cpg_sd_clk_mux_set_parent() concurrently with the users of SD0. > Add proper locking to avoid concurrent access on SD MUX set rate > registers. > > Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -189,6 +189,7 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > u32 shift = GET_SHIFT(hwdata->conf); > const u32 clk_src_266 = 2; > u32 msk, val, bitmask; > + unsigned long flags; > int ret; > > /* > @@ -203,25 +204,27 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > * the index to value mapping is done by adding 1 to the index. > */ > bitmask = (GENMASK(GET_WIDTH(hwdata->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; > > - ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, > - !(val & msk), 100, > - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); > - if (ret) { > - dev_err(priv->dev, "failed to switch clk source\n"); > - return ret; > - } > + ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val, > + !(val & msk), 100, According to the read_poll_timeout_atomic() documentation, delay_us should be less than ~10us. > + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US = 20 ms, which is a long timeout for an atomic poll. > + if (ret) > + goto unlock; > } > > writel(bitmask | ((index + 1) << shift), priv->base + off); > > - ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, > - !(val & msk), 100, > - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); > + ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val, > + !(val & msk), 100, > + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); Likewise. > +unlock: > + spin_unlock_irqrestore(&priv->rmw_lock, flags); > + > if (ret) > dev_err(priv->dev, "failed to switch clk source\n"); The rest LGTM. Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 6:43 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > On 9/12/23 7:51 AM, Claudiu wrote: > > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Initial value of CPG_PL2SDHI_DSEL bits 0..1 or 4..6 is 01b. Hardware user's > > manual (r01uh0914ej0130-rzg2l-rzg2lc.pdf) specifies that setting 0 is > > prohibited. The rzg2l_cpg_sd_clk_mux_get_parent() should just read > > CPG_PL2SDHI_DSEL, trust the value and return the proper clock parent index > > based on the read value. Do this. > > > > Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > --- a/drivers/clk/renesas/rzg2l-cpg.c > > +++ b/drivers/clk/renesas/rzg2l-cpg.c > > @@ -239,14 +239,8 @@ static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) > > > > 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); > > - } > > > > - return val; > > + return val ? --val : val; > > return val ? val - 1 : 0; Definitely, mixing multiple users of the same variable and pre-decrement is ill-defined. Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) > the computation formula for PLL rate is as follows: > > Fout = ((m + k/65536) * Fin) / (p * 2^s) > > and k has values in range [-32768, 32767]. Dividing k by 65536 with > integer variables leads all the time to zero. Thus we may have slight > differences b/w what has been set vs. what is displayed. Thus, > get rid of this and decompose the formula before dividing k by 65536. > > Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, > struct pll_clk *pll_clk = to_pll(hw); > struct rzg2l_cpg_priv *priv = pll_clk->priv; > unsigned int val1, val2; > - unsigned int mult = 1; > - unsigned int div = 1; > + unsigned int div; > + u64 rate; > + s16 kdiv; > > if (pll_clk->type != CLK_TYPE_SAM_PLL) > return parent_rate; > > val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); > val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); > - mult = MDIV(val1) + KDIV(val1) / 65536; > + kdiv = KDIV(val1); > div = PDIV(val1) << SDIV(val2); > > - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); > + rate = (u64)MDIV(val1) * parent_rate; > + rate += ((long long)parent_rate * kdiv) / 65536; As the division is a binary shift, you can use the mul_u64_u32_shr() helper, and incorporate the sdiv shift at the same time: rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); You can save a multiplication by premultiplying mdiv by 65536: rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), 16 + SDIV(val2)); > + > + return DIV_ROUND_CLOSEST_ULL(rate, div); return DIV_ROUND_CLOSEST_ULL(rate, PDIV(val1)); > } > > static const struct clk_ops rzg2l_cpg_pll_ops = { Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > core->name already contains the clock name thus, there is no > need to check the GET_SHIFT(core->conf) to decide on it. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -266,7 +266,7 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core, > clk_hw_data->priv = priv; > clk_hw_data->conf = core->conf; > > - init.name = GET_SHIFT(core->conf) ? "sd1" : "sd0"; > + init.name = core->name; Note that this does change the case of the names (e.g. "SD0" => "sd0"). I guess no one cares... > init.ops = &rzg2l_cpg_sd_clk_mux_ops; > init.flags = 0; > init.num_parents = core->num_parents; Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The bitmask << 16 is anyway set on both branches of if thus move it > before the if and set the lower bits of registers only in case clock is > enabled. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-clk-for-v6.7. Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses > to hardware register. There is no need to protect the instructions that set > temporary variable which will be then written to register. Thus limit the > spinlock only to the hardware register access. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable) > > dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk, > enable ? "ON" : "OFF"); > - spin_lock_irqsave(&priv->rmw_lock, flags); > > value = bitmask << 16; > if (enable) > value |= bitmask; > - writel(value, priv->base + CLK_ON_R(reg)); > > + spin_lock_irqsave(&priv->rmw_lock, flags); > + writel(value, priv->base + CLK_ON_R(reg)); > spin_unlock_irqrestore(&priv->rmw_lock, flags); After this, it becomes obvious there is nothing to protect at all, so the locking can just be removed from this function? Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Use FIELD_GET() for PLL register fields. This is its purpose. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-clk-for-v6.7. Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > flag and mux_flags are intended to keep bit masks. Use u32 type for it. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.h > +++ b/drivers/clk/renesas/rzg2l-cpg.h > @@ -92,8 +92,8 @@ struct cpg_core_clk { > unsigned int conf; > const struct clk_div_table *dtable; > const char * const *parent_names; > - int flag; > - int mux_flags; > + u32 flag; "flag" is used for several purposes, which expected different types: - clk_init_data.flags is unsigned long, - The clk_divider_flags parameter of clk_hw_register_divider_table() is u8, - The clk_divider_flags parameter of __clk_hw_register_divider() is u8, - The flags parameter of __devm_clk_hw_register_mux() is unsigned long. > + u32 mux_flags; Actually the clk_mux_flags parameter of __devm_clk_hw_register_mux() is u8. > int num_parents; > }; I guess u32 is fine for all. Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add support for reading the frequency of PLL1/4/6 available on RZ/G3S. > The computation formula for PLL frequency is as follows: > Fout = (nir + nfr / 4096) * Fin / (mr * pr) > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -718,11 +718,43 @@ static const struct clk_ops rzg2l_cpg_pll_ops = { > .recalc_rate = rzg2l_cpg_pll_clk_recalc_rate, > }; > > +static unsigned long rzg3s_cpg_pll_clk_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct pll_clk *pll_clk = to_pll(hw); > + struct rzg2l_cpg_priv *priv = pll_clk->priv; > + u32 nir, nfr, mr, pr, val; > + u64 rate; > + > + if (pll_clk->type != CLK_TYPE_G3S_SAM_PLL) > + return parent_rate; > + > + val = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); > + > + pr = 1 << FIELD_GET(GENMASK(28, 26), val); Please add defines for the various GENMASK(...) fields. > + /* Hardware interprets values higher than 8 as p = 16. */ > + if (pr > 8) > + pr = 16; > + > + mr = FIELD_GET(GENMASK(25, 22), val) + 1; > + nir = FIELD_GET(GENMASK(21, 13), val) + 1; > + nfr = FIELD_GET(GENMASK(12, 1), val); > + > + rate = DIV_ROUND_CLOSEST_ULL((u64)parent_rate * nfr, 4096); > + rate += (u64)parent_rate * nir; When rewriting the formula as: Fout = (4096 * nir + nfr) * Fin / (4096 * mr * pr) you can simplify to: rate = mul_u64_u32_shr(parent_rate, 4096 * nir + nfr, 12); > + return DIV_ROUND_CLOSEST_ULL(rate, (mr + pr)); mr * pr > +} > --- a/drivers/clk/renesas/rzg2l-cpg.h > +++ b/drivers/clk/renesas/rzg2l-cpg.h > @@ -102,6 +102,7 @@ enum clk_types { > CLK_TYPE_IN, /* External Clock Input */ > CLK_TYPE_FF, /* Fixed Factor Clock */ > CLK_TYPE_SAM_PLL, > + CLK_TYPE_G3S_SAM_PLL, CLK_TYPE_G3S_PLL, as the documentation doesn't use SAM? > > /* Clock with divider */ > CLK_TYPE_DIV, > @@ -129,6 +130,8 @@ enum clk_types { > DEF_TYPE(_name, _id, _type, .parent = _parent) > #define DEF_SAMPLL(_name, _id, _parent, _conf) \ > DEF_TYPE(_name, _id, CLK_TYPE_SAM_PLL, .parent = _parent, .conf = _conf) > +#define DEF_G3S_SAMPLL(_name, _id, _parent, _conf) \ DEF_G3S_PLL > + DEF_TYPE(_name, _id, CLK_TYPE_G3S_SAM_PLL, .parent = _parent, .conf = _conf) > #define DEF_INPUT(_name, _id) \ > DEF_TYPE(_name, _id, CLK_TYPE_IN) > #define DEF_FIXED(_name, _id, _parent, _mult, _div) \ Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add clk_hw_data struct that keeps the core part of a clock data. The > sd_hw_data embeds a member of type struct clk_hw_data along with other > members (in the next commits). This commit prepares the field for > refactoring the SD MUX clock driver. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Remove CPG_SDHI_DSEL and its bits form generic header as RZ/G3S has from > different offset register and bits for this, thus avoid mixing them. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/clk/renesas/r9a07g043-cpg.c > +++ b/drivers/clk/renesas/r9a07g043-cpg.c > @@ -14,6 +14,13 @@ > > #include "rzg2l-cpg.h" > > +/* Specific registers. */ > +#define G2UL_CPG_PL2SDHI_DSEL (0x218) > + > +/* Clock select configuration. */ > +#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) As all three above are local to this file, the "G2UL_" prefix is not really needed. Removing the prefix (in all files affected) would make this patch smaller, though, and would make the DEF_SD_MUX() declarations (which keep on growing) shorter. 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
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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@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
Hi, Geert, On 14.09.2023 14:42, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Hardware user manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf, >> chapter 7.4.7 Procedure for Switching Clocks by the Dynamic Switching >> Frequency Selectors) specifies that we need to check CPG_PL2SDHI_DSEL for >> SD clock switching status. >> >> Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -188,7 +188,8 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> u32 off = GET_REG_OFFSET(hwdata->conf); >> u32 shift = GET_SHIFT(hwdata->conf); >> const u32 clk_src_266 = 2; >> - u32 bitmask; >> + u32 msk, val, bitmask; >> + int ret; >> >> /* >> * As per the HW manual, we should not directly switch from 533 MHz to >> @@ -203,9 +204,6 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> */ >> bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16; >> if (index != clk_src_266) { >> - u32 msk, val; >> - int ret; >> - >> writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off); >> >> msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS; >> @@ -221,7 +219,13 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> >> writel(bitmask | ((index + 1) << shift), priv->base + off); >> >> - return 0; >> + ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, >> + !(val & msk), 100, > > "msk" may be uninitialized. Indeed! I'll update it in next version. > >> + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); >> + if (ret) >> + dev_err(priv->dev, "failed to switch clk source\n"); >> + >> + return ret; > > This is now (supposed to be) doing the same thing twice, once using > clk_src_266, and then again with the wanted index, so why not introduce > a small helper? That would have avoided the uninitialized variable, too. Initially I thought about it but I found it too much for this stage as it is only about the readl_poll_timeout() and the debug message. I may keep the debug message in a local variable if you think worth it (but FMPOV it the code will look a bit... unusual). Moreover, as the code is rewritten in patch "[PATCH 18/37] clk: renesas: rzg2l: refactor sd mux driver" I thought it doesn't worth introducing a new helper in this patch. Thank you, Claudiu Beznea > > I know you're rewriting this code in "[PATCH 18/37] clk: renesas: > rzg2l: refactor sd mux driver", but even after that, you always do > a register write before calling rzg2l_cpg_wait_clk_update_done(), > so it may still be a net win. > >> } >> >> static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) > > Gr{oetje,eeting}s, > > Geert >
On 14.09.2023 15:13, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> SD MUX output (SD0) is further divided by 4 in G2{L, UL}. The divided >> clock is SD0_DIV4. SD0_DIV4 is registered with CLK_SET_RATE_PARENT which >> means a rate request for it is propagated to the MUX and could reach >> rzg2l_cpg_sd_clk_mux_set_parent() concurrently with the users of SD0. >> Add proper locking to avoid concurrent access on SD MUX set rate >> registers. >> >> Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -189,6 +189,7 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> u32 shift = GET_SHIFT(hwdata->conf); >> const u32 clk_src_266 = 2; >> u32 msk, val, bitmask; >> + unsigned long flags; >> int ret; >> >> /* >> @@ -203,25 +204,27 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> * the index to value mapping is done by adding 1 to the index. >> */ >> bitmask = (GENMASK(GET_WIDTH(hwdata->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; >> >> - ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, >> - !(val & msk), 100, >> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); >> - if (ret) { >> - dev_err(priv->dev, "failed to switch clk source\n"); >> - return ret; >> - } >> + ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val, >> + !(val & msk), 100, > > According to the read_poll_timeout_atomic() documentation, > delay_us should be less than ~10us. I'll update it, thanks for pointing it. > >> + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); > > CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US = 20 ms, which is a long timeout > for an atomic poll. I'll have to find the the rationale behind the original timeout. It may be random, experimental or hardware related. > >> + if (ret) >> + goto unlock; >> } >> >> writel(bitmask | ((index + 1) << shift), priv->base + off); >> >> - ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, >> - !(val & msk), 100, >> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); >> + ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val, >> + !(val & msk), 100, >> + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); > > Likewise. > >> +unlock: >> + spin_unlock_irqrestore(&priv->rmw_lock, flags); >> + >> if (ret) >> dev_err(priv->dev, "failed to switch clk source\n"); > > The rest LGTM. > > Gr{oetje,eeting}s, > > Geert >
On 14.09.2023 16:04, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> core->name already contains the clock name thus, there is no >> need to check the GET_SHIFT(core->conf) to decide on it. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -266,7 +266,7 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core, >> clk_hw_data->priv = priv; >> clk_hw_data->conf = core->conf; >> >> - init.name = GET_SHIFT(core->conf) ? "sd1" : "sd0"; >> + init.name = core->name; > > Note that this does change the case of the names (e.g. "SD0" => "sd0"). > I guess no one cares... As of my experiments and investigation we should be good with it. > >> init.ops = &rzg2l_cpg_sd_clk_mux_ops; >> init.flags = 0; >> init.num_parents = core->num_parents; > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert >
On 14.09.2023 16:12, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses >> to hardware register. There is no need to protect the instructions that set >> temporary variable which will be then written to register. Thus limit the >> spinlock only to the hardware register access. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable) >> >> dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk, >> enable ? "ON" : "OFF"); >> - spin_lock_irqsave(&priv->rmw_lock, flags); >> >> value = bitmask << 16; >> if (enable) >> value |= bitmask; >> - writel(value, priv->base + CLK_ON_R(reg)); >> >> + spin_lock_irqsave(&priv->rmw_lock, flags); >> + writel(value, priv->base + CLK_ON_R(reg)); >> spin_unlock_irqrestore(&priv->rmw_lock, flags); > > After this, it becomes obvious there is nothing to protect at all, > so the locking can just be removed from this function? I tend to be paranoid when writing to hardware resources thus I kept it. Would you prefer to remove it at all? > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Fri, Sep 15, 2023 at 7:51 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 14.09.2023 16:12, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses > >> to hardware register. There is no need to protect the instructions that set > >> temporary variable which will be then written to register. Thus limit the > >> spinlock only to the hardware register access. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- a/drivers/clk/renesas/rzg2l-cpg.c > >> +++ b/drivers/clk/renesas/rzg2l-cpg.c > >> @@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable) > >> > >> dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk, > >> enable ? "ON" : "OFF"); > >> - spin_lock_irqsave(&priv->rmw_lock, flags); > >> > >> value = bitmask << 16; > >> if (enable) > >> value |= bitmask; > >> - writel(value, priv->base + CLK_ON_R(reg)); > >> > >> + spin_lock_irqsave(&priv->rmw_lock, flags); > >> + writel(value, priv->base + CLK_ON_R(reg)); > >> spin_unlock_irqrestore(&priv->rmw_lock, flags); > > > > After this, it becomes obvious there is nothing to protect at all, > > so the locking can just be removed from this function? > > I tend to be paranoid when writing to hardware resources thus I kept it. > Would you prefer to remove it at all? Yes please. I guess this was copied from R-Car and friends, where there is a RMW operation on an MSTPCR register. Gr{oetje,eeting}s, Geert
Hi, Geert, On 14.09.2023 18:18, Geert Uytterhoeven wrote: > Hi Claudiu, > > Thanks for your patch! > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.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. Ok. > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.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. Ok, I ususlly tend to guard everything with a proper namespace. > >> + >> 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) ok > >> + >> 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. That should work too but .set_rate() will be bulky for both mux and div. The idea was to have the .set_rate() common to the mux and the platform specificities implemented as notifiers and only the needed platforms to instantiate the notifier. And the same approach to be used by the divider (patch "[PATCH 19/37] clk: renesas: rzg2l: add a divider clock for RZ/G3S") With this it looked to me that the final code is more compact .set_rate being simple and platform specificities being implemented in notifier (valid for both MUX and DIV). The infrastructure is already there for notifier to be called before .set_rate(). > >> + >> 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. Appologies about that, I'll try to adapt it in the next version. > >> - >> 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? It looked to me like a better name to be used in platform specific drivers. > What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly? Nothing, just that it is a longer than SD_MUX_NOTIF. > > 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
Hi Claudiu, On Fri, Sep 15, 2023 at 9:30 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 14.09.2023 18:18, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.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. > > Ok. > > > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.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. > > Ok, I ususlly tend to guard everything with a proper namespace. Sure, in many cases, that makes good sense. In this case, not having the prefix makes it easier to compare clock tables: soc-dts-diff -b drivers/clk/renesas/r9a07g04[34]-cpg.c (soc-dts-diff ignores the SoC part number, and can be found at https://github.com/geertu/linux-scripts) > >> +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. > > That should work too but .set_rate() will be bulky for both mux and div. > > The idea was to have the .set_rate() common to the mux and the platform > specificities implemented as notifiers and only the needed platforms to > instantiate the notifier. And the same approach to be used by the divider > (patch "[PATCH 19/37] clk: renesas: rzg2l: add a divider clock for RZ/G3S") > > With this it looked to me that the final code is more compact .set_rate > being simple and platform specificities being implemented in notifier > (valid for both MUX and DIV). The infrastructure is already there for > notifier to be called before .set_rate(). TBH, I am not that familiar with clock notifiers, so I could use some guidance from the clock maintainers. Mike/Stephen: Are clock notifiers the right approach, here and in [PATCH 19.37]? > >> --- a/drivers/clk/renesas/rzg2l-cpg.h > >> +++ b/drivers/clk/renesas/rzg2l-cpg.h > >> @@ -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? > > It looked to me like a better name to be used in platform specific drivers. > > > What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly? > > Nothing, just that it is a longer than SD_MUX_NOTIF. It adds another level of indirection for the casual reviewer, and needs replacement when an SoC arrives that needs a different SD mux notifier. Thanks! Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add minimal clock and reset support for RZ/G3S SoC to be able to boot > Linux from SD Card/eMMC. This includes necessary core clocks for booting > and GIC, SCIF, GPIO, SD0 mod clocks and resets. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- /dev/null > +++ b/drivers/clk/renesas/r9a08g045-cpg.c > @@ -0,0 +1,217 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * RZ/G3S CPG driver > + * > + * Copyright (C) 2023 Renesas Electronics Corp. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/device.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > + > +#include <dt-bindings/clock/r9a08g045-cpg.h> > + > +#include "rzg2l-cpg.h" > + > +/* RZ/G3S Specific registers. */ > +#define G3S_CPG_PL2_DDIV (0x204) > +#define G3S_CPG_SDHI_DDIV (0x218) > +#define G3S_CPG_PLL_DSEL (0x240) > +#define G3S_CPG_SDHI_DSEL (0x244) > +#define G3S_CLKSELSTATUS (0x284) > + > +/* RZ/G3S Specific division configuration. */ > +#define G3S_DIVPL2B DDIV_PACK(G3S_CPG_PL2_DDIV, 4, 3) > +#define G3S_DIV_SDHI0 DDIV_PACK(G3S_CPG_SDHI_DDIV, 0, 1) > + > +/* RZ/G3S Clock status configuration. */ > +#define G3S_DIVPL1A_STS DDIV_PACK(CPG_CLKSTATUS, 0, 1) > +#define G3S_DIVPL2B_STS DDIV_PACK(CPG_CLKSTATUS, 5, 1) > +#define G3S_DIVPL3A_STS DDIV_PACK(CPG_CLKSTATUS, 8, 1) > +#define G3S_DIVPL3B_STS DDIV_PACK(CPG_CLKSTATUS, 9, 1) > +#define G3S_DIVPL3C_STS DDIV_PACK(CPG_CLKSTATUS, 10, 1) > +#define G3S_DIV_SDHI0_STS DDIV_PACK(CPG_CLKSTATUS, 24, 1) The register at offset 0x280 is called CPG_CLKDIVSTATUS, so you probably want to add and use a G3S-specific definition. > +#define G3S_SEL_PLL4_STS SEL_PLL_PACK(G3S_CLKSELSTATUS, 6, 1) > +#define G3S_SEL_SDHI0_STS SEL_PLL_PACK(G3S_CLKSELSTATUS, 16, 1) > + > +/* RZ/G3S Specific clocks select. */ > +#define G3S_SEL_PLL4 SEL_PLL_PACK(G3S_CPG_PLL_DSEL, 6, 1) > +#define G3S_SEL_SDHI0 SEL_PLL_PACK(G3S_CPG_SDHI_DSEL, 0, 2) > + > +/* PLL 1/4/6 configuration registers macro. */ > +#define G3S_PLL146_CONF(clk1, clk2) ((clk1) << 22 | (clk2) << 12) > + > +#define DEF_G3S_MUX(_name, _id, _conf, _parent_names, _mux_flags, _clk_flags) \ > + DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = (_conf), \ > + .parent_names = (_parent_names), \ > + .num_parents = ARRAY_SIZE((_parent_names)), \ > + .mux_flags = CLK_MUX_HIWORD_MASK | (_mux_flags), \ > + .flag = (_clk_flags)) > + > +enum clk_ids { > + /* Core Clock Outputs exported to DT */ > + LAST_DT_CORE_CLK = R9A08G045_SWD, > + > + /* External Input Clocks */ > + CLK_EXTAL, > + > + /* Internal Core Clocks */ > + CLK_OSC_DIV1000, > + CLK_PLL1, > + CLK_PLL2, > + CLK_PLL2_DIV2, > + CLK_PLL2_DIV2_8, > + CLK_PLL2_DIV6, > + CLK_PLL3, > + CLK_PLL3_DIV2, > + CLK_PLL3_DIV2_2, Do you need CLK_PLL3_DIV2_2? When adding support for R9A07G043_CLK_AT later, you can define it as CLK_PLL3_DIV2 / 2. > + CLK_PLL3_DIV2_4, > + CLK_PLL3_DIV2_8, > + CLK_PLL3_DIV6, > + CLK_PLL4, > + CLK_PLL6, > + CLK_PLL6_DIV2, > + CLK_SEL_SDHI0, > + CLK_SEL_PLL4, > + CLK_P1_DIV2, > + CLK_P3_DIV2, Do you need CLK_P1_DIV2 and CLK_P3_DIV2? I don't see them in Figure 7.3 ("Clock System Diagram (2)"). > + CLK_SD0_DIV, CLK_SD0_DIV is unused. > + CLK_SD0_DIV4, > + CLK_S0_DIV2, CLK_S0_DIV2 is unused. > + > + /* Module Clocks */ > + MOD_CLK_BASE, > +}; > + > +/* Divider tables */ > +static const struct clk_div_table dtable_1_2[] = { > + {0, 1}, "{ 0, 1 }," etc... > + {1, 2}, > + {0, 0}, > +}; > + > +static const struct clk_div_table dtable_1_8[] = { > + {0, 1}, > + {1, 2}, > + {2, 4}, > + {3, 8}, > + {0, 0}, > +}; > + > +static const struct clk_div_table dtable_1_32[] = { > + {0, 1}, > + {1, 2}, > + {2, 4}, > + {3, 8}, > + {4, 32}, > + {0, 0}, > +}; > + > +/* Mux clock names tables. */ > +static const char * const sel_sdhi[] = { ".pll2_div2", ".pll6", ".pll2_div6" }; > +static const char * const sel_pll4[] = { ".osc_div1000", ".pll4" }; > + > +/* Mux clock indexes tables. */ indices > +static const u32 mtable_sd[] = { 0, 2, 3 }; > +static const u32 mtable_pll4[] = { 0, 1 }; > + > +static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = { > + /* External Clock Inputs */ > + DEF_INPUT("extal", CLK_EXTAL), > + > + /* Internal Core Clocks */ > + DEF_FIXED(".osc", R9A08G045_OSCCLK, CLK_EXTAL, 1, 1), "OSC", as this is not an internal core clock. > + DEF_FIXED(".osc2", R9A08G045_OSCCLK2, CLK_EXTAL, 1, 3), "OSC2" > + DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000, CLK_EXTAL, 1, 1000), > + DEF_G3S_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL, G3S_PLL146_CONF(0x4, 0x8)), > + DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3), > + DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 200, 3), > + DEF_FIXED(".pll4", CLK_PLL4, CLK_EXTAL, 100, 3), > + DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6), > + DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2), > + DEF_FIXED(".pll2_div2_8", CLK_PLL2_DIV2_8, CLK_PLL2_DIV2, 1, 8), > + DEF_FIXED(".pll2_div6", CLK_PLL2_DIV6, CLK_PLL2, 1, 6), > + DEF_FIXED(".pll3_div2", CLK_PLL3_DIV2, CLK_PLL3, 1, 2), > + DEF_FIXED(".pll3_div2_2", CLK_PLL3_DIV2_2, CLK_PLL3_DIV2, 1, 2), > + DEF_FIXED(".pll3_div2_4", CLK_PLL3_DIV2_4, CLK_PLL3_DIV2, 1, 4), > + DEF_FIXED(".pll3_div2_8", CLK_PLL3_DIV2_8, CLK_PLL3_DIV2, 1, 8), > + DEF_FIXED(".pll3_div6", CLK_PLL3_DIV6, CLK_PLL3, 1, 6), > + DEF_FIXED(".pll6_div2", CLK_PLL6_DIV2, CLK_PLL6, 1, 2), > + DEF_SD_MUX(".sel_sd0", CLK_SEL_SDHI0, G3S_SEL_SDHI0, G3S_SEL_SDHI0_STS, sel_sdhi, > + mtable_sd, 0, NULL), > + DEF_SD_MUX(".sel_pll4", CLK_SEL_PLL4, G3S_SEL_PLL4, G3S_SEL_PLL4_STS, sel_pll4, > + mtable_pll4, CLK_SET_PARENT_GATE, NULL), > + > + /* Core output clk */ > + DEF_G3S_DIV("I", R9A08G045_CLK_I, CLK_PLL1, DIVPL1A, G3S_DIVPL1A_STS, dtable_1_8, > + 0, 0, NULL), > + DEF_G3S_DIV("P0", R9A08G045_CLK_P0, CLK_PLL2_DIV2_8, G3S_DIVPL2B, G3S_DIVPL2B_STS, > + dtable_1_32, 0, 0, NULL), > + DEF_G3S_DIV("SD0", R9A08G045_CLK_SD0, CLK_SEL_SDHI0, G3S_DIV_SDHI0, G3S_DIV_SDHI0_STS, > + dtable_1_2, 800000000UL, CLK_SET_RATE_PARENT, DIV_NOTIF), > + DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A08G045_CLK_SD0, 1, 4), ".sd0_div4", as this is not a public core clock. > + DEF_FIXED("M0", R9A08G045_CLK_M0, CLK_PLL3_DIV2_4, 1, 1), > + DEF_G3S_DIV("P1", R9A08G045_CLK_P1, CLK_PLL3_DIV2_4, DIVPL3A, G3S_DIVPL3A_STS, > + dtable_1_32, 0, 0, NULL), > + DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A08G045_CLK_P1, 1, 2), > + DEF_G3S_DIV("P2", R9A08G045_CLK_P2, CLK_PLL3_DIV2_8, DIVPL3B, G3S_DIVPL3B_STS, > + dtable_1_32, 0, 0, NULL), > + DEF_G3S_DIV("P3", R9A08G045_CLK_P3, CLK_PLL3_DIV2_4, DIVPL3C, G3S_DIVPL3C_STS, > + dtable_1_32, 0, 0, NULL), > + DEF_FIXED("P3_DIV2", CLK_P3_DIV2, R9A08G045_CLK_P3, 1, 2), > + DEF_FIXED("S0", R9A08G045_CLK_S0, CLK_SEL_PLL4, 1, 2), > + DEF_FIXED("S0_DIV2", CLK_S0_DIV2, R9A08G045_CLK_S0, 1, 2), > +}; Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add initial DTSI for RZ/G3S SoC. Files in commit has the following > meaning: > r9a08g045.dtsi RZ/G3S family SoC common parts > r9a08g045s33.dtsi RZ/G3S R0A08G045S33 SoC specific parts > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add initial support for RZ/G3S SMARC Carrier-II SoM. SoM contains the following > devices: > - RZ/G3S microcontroller: Renesas R9A08G045S33GBG > - 9-channel PMIC: Renesas RAA215300 > - Clock Generator: Renesas 5L35023B > - 128M QSPI Flash: Renesas AT25QL128A > - 8G LPDDR4 SDRAM: Micron MT53D512M16D1DS-046 That's an 8 Gib part, so 1 GiB? > - 64GB eMMC Flash: Micron MTFC64GBCAQTC > - 2x Gigabit Ethernet Transceiver: Microchip KSZ9131RNX > - 5x Current Monitors: Renesas ISL28025FR12Z > > The following interfaces are available on SoM board: > - 2 uSD interfaces > - 12-pin, 1.0mm pitch connector to the RZ/G3S ADC IO > - 4-pin, 1.0mm pitch connector to the RZ/G3S I3C IO > - JTAG connector Please drop the description of parts you are not adding to the DTS yet. > At the moment the 24MHz output of 5L35023B, memory SD ch0 (with all its > bits) were described in device tree. > > SD channel 0 of RZ/G3S is connected to an uSD card interface > and an eMMC. The selection b/w them is done though a hardware switch. > The DT will select b/w uSD and eMMC though SW_SD0_DEV_SEL build flag. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- /dev/null > +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi > @@ -0,0 +1,147 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +/* > + * Device Tree Source for the R9A08G045S33 SMARC Carrier-II's SoM board. > + * > + * Copyright (C) 2023 Renesas Electronics Corp. > + */ > + > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h> > + > +/* > + * Signals of SW_CONFIG switches: > + * @SW_SD0_DEV_SEL: > + * 0 - SD0 is connected to eMMC > + * 1 - SD0 is connected to uSD0 card > + */ > +#define SW_SD0_DEV_SEL 1 > + > +/ { > + aliases { > + mmc0 = &sdhi0; > + }; > + > + chosen { > + bootargs = "ignore_loglevel"; > + stdout-path = "serial0:115200n8"; > + }; > + > + memory@48000000 { > + device-type = "memory"; > + /* First 128MB is reserved for secure area. */ > + reg = <0x0 0x48000000 0x0 0x38000000>; > + }; > + > + reg_3p3v: regulator0 { > + compatible = "regulator-fixed"; > + regulator-name = "fixed-3.3V"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > +#if SW_SD0_DEV_SEL > + vccq_sdhi0: regulator1 { > + compatible = "regulator-gpio"; > + regulator-name = "SDHI0 VccQ"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + gpios = <&pinctrl RZG2L_GPIO(2, 2) GPIO_ACTIVE_HIGH>; > + gpios-states = <1>; > + states = <3300000 1>, <1800000 0>; > + }; > +#else > + reg_1p8v: regulator1 { > + compatible = "regulator-fixed"; > + regulator-name = "fixed-1.8V"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + regulator-boot-on; > + regulator-always-on; > + }; > +#endif > +}; > + > +&extal_clk { > + clock-frequency = <24000000>; > +}; > + > +#if SW_SD0_DEV_SEL > +/* SD0 slot */ > +&sdhi0 { > + pinctrl-0 = <&sdhi0_pins>; > + pinctrl-1 = <&sdhi0_uhs_pins>; > + pinctrl-names = "default", "state_uhs"; > + vmmc-supply = <®_3p3v>; > + vqmmc-supply = <&vccq_sdhi0>; > + bus-width = <4>; > + sd-uhs-sdr50; > + sd-uhs-sdr104; > + max-frequency = <125000000>; > + status = "okay"; > +}; > +#else > +/* eMMC */ > +&sdhi0 { > + pinctrl-0 = <&sdhi0_emmc_pins>; > + pinctrl-1 = <&sdhi0_emmc_pins>; > + pinctrl-names = "default", "state_uhs"; > + vmmc-supply = <®_3p3v>; > + vqmmc-supply = <®_1p8v>; > + bus-width = <8>; > + mmc-hs200-1_8v; > + non-removable; > + fixed-emmc-driver-type = <1>; > + max-frequency = <125000000>; > + status = "okay"; > +}; > +#endif > + > +&pinctrl { > + sd0-pwr-en-hog { > + gpio-hog; > + gpios = <RZG2L_GPIO(2, 1) GPIO_ACTIVE_HIGH>; According to the schematics, P2_1 controls power to the uSD slot. Hence shouldn't reg_3p3v above be modelled using regulator-gpio, with enable-gpios pointing to P2_1? > + output-high; > + line-name = "sd0_pwr_en"; > + }; Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add initial device tree for RZ SMARC Carrier-II. At the moment it > contains only serial interface (and its pins definition). > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- /dev/null > +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +/* > + * Device Tree Source for the RZ SMARC Carrier-II Board. > + * > + * Copyright (C) 2023 Renesas Electronics Corp. > + */ > + > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h> > + > +/ { > + aliases { > + serial0 = &scif0; > + }; > +}; > + > +&pinctrl { > + scif0_pins: scif0 { > + pinmux = <RZG2L_PORT_PINMUX(6, 3, 1)>, /* TXD */ RXD > + <RZG2L_PORT_PINMUX(6, 4, 1)>; /* RXD */ TXD > + }; > +}; The rest LGTM. Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Enable config flag for Renesas RZ/G3S (R9A08G045) SoC. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi, Geert, On 15.09.2023 17:28, Geert Uytterhoeven wrote: > Hi Claudiu, > > Thanks for your patch! > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Add initial support for RZ/G3S SMARC Carrier-II SoM. SoM contains the following >> devices: >> - RZ/G3S microcontroller: Renesas R9A08G045S33GBG >> - 9-channel PMIC: Renesas RAA215300 >> - Clock Generator: Renesas 5L35023B >> - 128M QSPI Flash: Renesas AT25QL128A >> - 8G LPDDR4 SDRAM: Micron MT53D512M16D1DS-046 > > That's an 8 Gib part, so 1 GiB? > >> - 64GB eMMC Flash: Micron MTFC64GBCAQTC >> - 2x Gigabit Ethernet Transceiver: Microchip KSZ9131RNX >> - 5x Current Monitors: Renesas ISL28025FR12Z >> >> The following interfaces are available on SoM board: >> - 2 uSD interfaces >> - 12-pin, 1.0mm pitch connector to the RZ/G3S ADC IO >> - 4-pin, 1.0mm pitch connector to the RZ/G3S I3C IO >> - JTAG connector > > Please drop the description of parts you are not adding to the DTS yet. > >> At the moment the 24MHz output of 5L35023B, memory SD ch0 (with all its >> bits) were described in device tree. >> >> SD channel 0 of RZ/G3S is connected to an uSD card interface >> and an eMMC. The selection b/w them is done though a hardware switch. >> The DT will select b/w uSD and eMMC though SW_SD0_DEV_SEL build flag. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> --- /dev/null >> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi >> @@ -0,0 +1,147 @@ >> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +/* >> + * Device Tree Source for the R9A08G045S33 SMARC Carrier-II's SoM board. >> + * >> + * Copyright (C) 2023 Renesas Electronics Corp. >> + */ >> + >> +#include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h> >> + >> +/* >> + * Signals of SW_CONFIG switches: >> + * @SW_SD0_DEV_SEL: >> + * 0 - SD0 is connected to eMMC >> + * 1 - SD0 is connected to uSD0 card >> + */ >> +#define SW_SD0_DEV_SEL 1 >> + >> +/ { >> + aliases { >> + mmc0 = &sdhi0; >> + }; >> + >> + chosen { >> + bootargs = "ignore_loglevel"; >> + stdout-path = "serial0:115200n8"; >> + }; >> + >> + memory@48000000 { >> + device-type = "memory"; >> + /* First 128MB is reserved for secure area. */ >> + reg = <0x0 0x48000000 0x0 0x38000000>; >> + }; >> + >> + reg_3p3v: regulator0 { >> + compatible = "regulator-fixed"; >> + regulator-name = "fixed-3.3V"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> +#if SW_SD0_DEV_SEL >> + vccq_sdhi0: regulator1 { >> + compatible = "regulator-gpio"; >> + regulator-name = "SDHI0 VccQ"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <3300000>; >> + gpios = <&pinctrl RZG2L_GPIO(2, 2) GPIO_ACTIVE_HIGH>; >> + gpios-states = <1>; >> + states = <3300000 1>, <1800000 0>; >> + }; >> +#else >> + reg_1p8v: regulator1 { >> + compatible = "regulator-fixed"; >> + regulator-name = "fixed-1.8V"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> +#endif >> +}; >> + >> +&extal_clk { >> + clock-frequency = <24000000>; >> +}; >> + >> +#if SW_SD0_DEV_SEL >> +/* SD0 slot */ >> +&sdhi0 { >> + pinctrl-0 = <&sdhi0_pins>; >> + pinctrl-1 = <&sdhi0_uhs_pins>; >> + pinctrl-names = "default", "state_uhs"; >> + vmmc-supply = <®_3p3v>; >> + vqmmc-supply = <&vccq_sdhi0>; >> + bus-width = <4>; >> + sd-uhs-sdr50; >> + sd-uhs-sdr104; >> + max-frequency = <125000000>; >> + status = "okay"; >> +}; >> +#else >> +/* eMMC */ >> +&sdhi0 { >> + pinctrl-0 = <&sdhi0_emmc_pins>; >> + pinctrl-1 = <&sdhi0_emmc_pins>; >> + pinctrl-names = "default", "state_uhs"; >> + vmmc-supply = <®_3p3v>; >> + vqmmc-supply = <®_1p8v>; >> + bus-width = <8>; >> + mmc-hs200-1_8v; >> + non-removable; >> + fixed-emmc-driver-type = <1>; >> + max-frequency = <125000000>; >> + status = "okay"; >> +}; >> +#endif >> + >> +&pinctrl { >> + sd0-pwr-en-hog { >> + gpio-hog; >> + gpios = <RZG2L_GPIO(2, 1) GPIO_ACTIVE_HIGH>; > > According to the schematics, P2_1 controls power to the uSD slot. > Hence shouldn't reg_3p3v above be modelled using regulator-gpio, > with enable-gpios pointing to P2_1? That should work. I'll check it, thanks! > >> + output-high; >> + line-name = "sd0_pwr_en"; >> + }; > Gr{oetje,eeting}s, > > Geert >
Hi, Geert, On 15.09.2023 15:52, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Add minimal clock and reset support for RZ/G3S SoC to be able to boot >> Linux from SD Card/eMMC. This includes necessary core clocks for booting >> and GIC, SCIF, GPIO, SD0 mod clocks and resets. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- /dev/null >> +++ b/drivers/clk/renesas/r9a08g045-cpg.c >> @@ -0,0 +1,217 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * RZ/G3S CPG driver >> + * >> + * Copyright (C) 2023 Renesas Electronics Corp. >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/device.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> + >> +#include <dt-bindings/clock/r9a08g045-cpg.h> >> + >> +#include "rzg2l-cpg.h" >> + >> +/* RZ/G3S Specific registers. */ >> +#define G3S_CPG_PL2_DDIV (0x204) >> +#define G3S_CPG_SDHI_DDIV (0x218) >> +#define G3S_CPG_PLL_DSEL (0x240) >> +#define G3S_CPG_SDHI_DSEL (0x244) >> +#define G3S_CLKSELSTATUS (0x284) >> + >> +/* RZ/G3S Specific division configuration. */ >> +#define G3S_DIVPL2B DDIV_PACK(G3S_CPG_PL2_DDIV, 4, 3) >> +#define G3S_DIV_SDHI0 DDIV_PACK(G3S_CPG_SDHI_DDIV, 0, 1) >> + >> +/* RZ/G3S Clock status configuration. */ >> +#define G3S_DIVPL1A_STS DDIV_PACK(CPG_CLKSTATUS, 0, 1) >> +#define G3S_DIVPL2B_STS DDIV_PACK(CPG_CLKSTATUS, 5, 1) >> +#define G3S_DIVPL3A_STS DDIV_PACK(CPG_CLKSTATUS, 8, 1) >> +#define G3S_DIVPL3B_STS DDIV_PACK(CPG_CLKSTATUS, 9, 1) >> +#define G3S_DIVPL3C_STS DDIV_PACK(CPG_CLKSTATUS, 10, 1) >> +#define G3S_DIV_SDHI0_STS DDIV_PACK(CPG_CLKSTATUS, 24, 1) > > The register at offset 0x280 is called CPG_CLKDIVSTATUS, so > you probably want to add and use a G3S-specific definition. I just used the already definition as there is no conflict at the moment, it points to the same offset and is almost identical in name. With this would you still prefer to have it separately ? > >> +#define G3S_SEL_PLL4_STS SEL_PLL_PACK(G3S_CLKSELSTATUS, 6, 1) >> +#define G3S_SEL_SDHI0_STS SEL_PLL_PACK(G3S_CLKSELSTATUS, 16, 1) >> + >> +/* RZ/G3S Specific clocks select. */ >> +#define G3S_SEL_PLL4 SEL_PLL_PACK(G3S_CPG_PLL_DSEL, 6, 1) >> +#define G3S_SEL_SDHI0 SEL_PLL_PACK(G3S_CPG_SDHI_DSEL, 0, 2) >> + >> +/* PLL 1/4/6 configuration registers macro. */ >> +#define G3S_PLL146_CONF(clk1, clk2) ((clk1) << 22 | (clk2) << 12) >> + >> +#define DEF_G3S_MUX(_name, _id, _conf, _parent_names, _mux_flags, _clk_flags) \ >> + DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = (_conf), \ >> + .parent_names = (_parent_names), \ >> + .num_parents = ARRAY_SIZE((_parent_names)), \ >> + .mux_flags = CLK_MUX_HIWORD_MASK | (_mux_flags), \ >> + .flag = (_clk_flags)) >> + >> +enum clk_ids { >> + /* Core Clock Outputs exported to DT */ >> + LAST_DT_CORE_CLK = R9A08G045_SWD, >> + >> + /* External Input Clocks */ >> + CLK_EXTAL, >> + >> + /* Internal Core Clocks */ >> + CLK_OSC_DIV1000, >> + CLK_PLL1, >> + CLK_PLL2, >> + CLK_PLL2_DIV2, >> + CLK_PLL2_DIV2_8, >> + CLK_PLL2_DIV6, >> + CLK_PLL3, >> + CLK_PLL3_DIV2, >> + CLK_PLL3_DIV2_2, > > Do you need CLK_PLL3_DIV2_2? > When adding support for R9A07G043_CLK_AT later, you can define it > as CLK_PLL3_DIV2 / 2. That's true. I kept it here as I saw it as a core clock. I can remove it. > >> + CLK_PLL3_DIV2_4, >> + CLK_PLL3_DIV2_8, >> + CLK_PLL3_DIV6, >> + CLK_PLL4, >> + CLK_PLL6, >> + CLK_PLL6_DIV2, >> + CLK_SEL_SDHI0, >> + CLK_SEL_PLL4, >> + CLK_P1_DIV2, >> + CLK_P3_DIV2, > > Do you need CLK_P1_DIV2 and CLK_P3_DIV2? > I don't see them in Figure 7.3 ("Clock System Diagram (2)"). > >> + CLK_SD0_DIV, > > CLK_SD0_DIV is unused. Ok, I'll remove it. > >> + CLK_SD0_DIV4, >> + CLK_S0_DIV2, > > CLK_S0_DIV2 is unused. ok. > >> + >> + /* Module Clocks */ >> + MOD_CLK_BASE, >> +}; >> + >> +/* Divider tables */ >> +static const struct clk_div_table dtable_1_2[] = { >> + {0, 1}, > > "{ 0, 1 }," etc... ok. > >> + {1, 2}, >> + {0, 0}, >> +}; >> + >> +static const struct clk_div_table dtable_1_8[] = { >> + {0, 1}, >> + {1, 2}, >> + {2, 4}, >> + {3, 8}, >> + {0, 0}, >> +}; >> + >> +static const struct clk_div_table dtable_1_32[] = { >> + {0, 1}, >> + {1, 2}, >> + {2, 4}, >> + {3, 8}, >> + {4, 32}, >> + {0, 0}, >> +}; >> + >> +/* Mux clock names tables. */ >> +static const char * const sel_sdhi[] = { ".pll2_div2", ".pll6", ".pll2_div6" }; >> +static const char * const sel_pll4[] = { ".osc_div1000", ".pll4" }; >> + >> +/* Mux clock indexes tables. */ > > indices > >> +static const u32 mtable_sd[] = { 0, 2, 3 }; >> +static const u32 mtable_pll4[] = { 0, 1 }; >> + >> +static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = { >> + /* External Clock Inputs */ >> + DEF_INPUT("extal", CLK_EXTAL), >> + >> + /* Internal Core Clocks */ >> + DEF_FIXED(".osc", R9A08G045_OSCCLK, CLK_EXTAL, 1, 1), > > "OSC", as this is not an internal core clock. ok, I wasn't aware of this convention. > >> + DEF_FIXED(".osc2", R9A08G045_OSCCLK2, CLK_EXTAL, 1, 3), > > "OSC2" ok. > >> + DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000, CLK_EXTAL, 1, 1000), >> + DEF_G3S_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL, G3S_PLL146_CONF(0x4, 0x8)), >> + DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3), >> + DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 200, 3), >> + DEF_FIXED(".pll4", CLK_PLL4, CLK_EXTAL, 100, 3), >> + DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6), >> + DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2), >> + DEF_FIXED(".pll2_div2_8", CLK_PLL2_DIV2_8, CLK_PLL2_DIV2, 1, 8), >> + DEF_FIXED(".pll2_div6", CLK_PLL2_DIV6, CLK_PLL2, 1, 6), >> + DEF_FIXED(".pll3_div2", CLK_PLL3_DIV2, CLK_PLL3, 1, 2), >> + DEF_FIXED(".pll3_div2_2", CLK_PLL3_DIV2_2, CLK_PLL3_DIV2, 1, 2), >> + DEF_FIXED(".pll3_div2_4", CLK_PLL3_DIV2_4, CLK_PLL3_DIV2, 1, 4), >> + DEF_FIXED(".pll3_div2_8", CLK_PLL3_DIV2_8, CLK_PLL3_DIV2, 1, 8), >> + DEF_FIXED(".pll3_div6", CLK_PLL3_DIV6, CLK_PLL3, 1, 6), >> + DEF_FIXED(".pll6_div2", CLK_PLL6_DIV2, CLK_PLL6, 1, 2), >> + DEF_SD_MUX(".sel_sd0", CLK_SEL_SDHI0, G3S_SEL_SDHI0, G3S_SEL_SDHI0_STS, sel_sdhi, >> + mtable_sd, 0, NULL), >> + DEF_SD_MUX(".sel_pll4", CLK_SEL_PLL4, G3S_SEL_PLL4, G3S_SEL_PLL4_STS, sel_pll4, >> + mtable_pll4, CLK_SET_PARENT_GATE, NULL), >> + >> + /* Core output clk */ >> + DEF_G3S_DIV("I", R9A08G045_CLK_I, CLK_PLL1, DIVPL1A, G3S_DIVPL1A_STS, dtable_1_8, >> + 0, 0, NULL), >> + DEF_G3S_DIV("P0", R9A08G045_CLK_P0, CLK_PLL2_DIV2_8, G3S_DIVPL2B, G3S_DIVPL2B_STS, >> + dtable_1_32, 0, 0, NULL), >> + DEF_G3S_DIV("SD0", R9A08G045_CLK_SD0, CLK_SEL_SDHI0, G3S_DIV_SDHI0, G3S_DIV_SDHI0_STS, >> + dtable_1_2, 800000000UL, CLK_SET_RATE_PARENT, DIV_NOTIF), >> + DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A08G045_CLK_SD0, 1, 4), > > ".sd0_div4", as this is not a public core clock. ok. > >> + DEF_FIXED("M0", R9A08G045_CLK_M0, CLK_PLL3_DIV2_4, 1, 1), >> + DEF_G3S_DIV("P1", R9A08G045_CLK_P1, CLK_PLL3_DIV2_4, DIVPL3A, G3S_DIVPL3A_STS, >> + dtable_1_32, 0, 0, NULL), >> + DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A08G045_CLK_P1, 1, 2), >> + DEF_G3S_DIV("P2", R9A08G045_CLK_P2, CLK_PLL3_DIV2_8, DIVPL3B, G3S_DIVPL3B_STS, >> + dtable_1_32, 0, 0, NULL), >> + DEF_G3S_DIV("P3", R9A08G045_CLK_P3, CLK_PLL3_DIV2_4, DIVPL3C, G3S_DIVPL3C_STS, >> + dtable_1_32, 0, 0, NULL), >> + DEF_FIXED("P3_DIV2", CLK_P3_DIV2, R9A08G045_CLK_P3, 1, 2), >> + DEF_FIXED("S0", R9A08G045_CLK_S0, CLK_SEL_PLL4, 1, 2), >> + DEF_FIXED("S0_DIV2", CLK_S0_DIV2, R9A08G045_CLK_S0, 1, 2), >> +}; > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Mon, Sep 18, 2023 at 8:20 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 15.09.2023 15:52, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> Add minimal clock and reset support for RZ/G3S SoC to be able to boot > >> Linux from SD Card/eMMC. This includes necessary core clocks for booting > >> and GIC, SCIF, GPIO, SD0 mod clocks and resets. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- /dev/null > >> +++ b/drivers/clk/renesas/r9a08g045-cpg.c > >> @@ -0,0 +1,217 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * RZ/G3S CPG driver > >> + * > >> + * Copyright (C) 2023 Renesas Electronics Corp. > >> + */ > >> + > >> +#include <linux/clk-provider.h> > >> +#include <linux/device.h> > >> +#include <linux/init.h> > >> +#include <linux/kernel.h> > >> + > >> +#include <dt-bindings/clock/r9a08g045-cpg.h> > >> + > >> +#include "rzg2l-cpg.h" > >> + > >> +/* RZ/G3S Specific registers. */ > >> +#define G3S_CPG_PL2_DDIV (0x204) > >> +#define G3S_CPG_SDHI_DDIV (0x218) > >> +#define G3S_CPG_PLL_DSEL (0x240) > >> +#define G3S_CPG_SDHI_DSEL (0x244) > >> +#define G3S_CLKSELSTATUS (0x284) > >> + > >> +/* RZ/G3S Specific division configuration. */ > >> +#define G3S_DIVPL2B DDIV_PACK(G3S_CPG_PL2_DDIV, 4, 3) > >> +#define G3S_DIV_SDHI0 DDIV_PACK(G3S_CPG_SDHI_DDIV, 0, 1) > >> + > >> +/* RZ/G3S Clock status configuration. */ > >> +#define G3S_DIVPL1A_STS DDIV_PACK(CPG_CLKSTATUS, 0, 1) > >> +#define G3S_DIVPL2B_STS DDIV_PACK(CPG_CLKSTATUS, 5, 1) > >> +#define G3S_DIVPL3A_STS DDIV_PACK(CPG_CLKSTATUS, 8, 1) > >> +#define G3S_DIVPL3B_STS DDIV_PACK(CPG_CLKSTATUS, 9, 1) > >> +#define G3S_DIVPL3C_STS DDIV_PACK(CPG_CLKSTATUS, 10, 1) > >> +#define G3S_DIV_SDHI0_STS DDIV_PACK(CPG_CLKSTATUS, 24, 1) > > > > The register at offset 0x280 is called CPG_CLKDIVSTATUS, so > > you probably want to add and use a G3S-specific definition. > > I just used the already definition as there is no conflict at the moment, > it points to the same offset and is almost identical in name. With this > would you still prefer to have it separately ? I think that would be clearer for the casual reader. On RZ/G2L, there is a single CPG_CLKSTATUS register to monitor frequency dividers and selectors. On RZ/G3S, this register was split into separate registers to monitor frequency dividers (CPG_CLKDIVSTATUS) and selectors (CPG_CLKSELSTATUS). You had to add a new definition for the latter anyway. Gr{oetje,eeting}s, Geert
Hi, Geert, On 15.09.2023 15:52, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Add minimal clock and reset support for RZ/G3S SoC to be able to boot >> Linux from SD Card/eMMC. This includes necessary core clocks for booting >> and GIC, SCIF, GPIO, SD0 mod clocks and resets. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > [ ... ] >> + CLK_PLL3_DIV2_4, >> + CLK_PLL3_DIV2_8, >> + CLK_PLL3_DIV6, >> + CLK_PLL4, >> + CLK_PLL6, >> + CLK_PLL6_DIV2, >> + CLK_SEL_SDHI0, >> + CLK_SEL_PLL4, >> + CLK_P1_DIV2, >> + CLK_P3_DIV2, > > Do you need CLK_P1_DIV2 and CLK_P3_DIV2? > I don't see them in Figure 7.3 ("Clock System Diagram (2)"). > P1_DIV2 is clock source for MHU_PCLK or OTFDE_DDR_PCLK. P3_DIV2 is clock source for DMAC_PCLK, OTFDE_SPI_PCLK. These are expressed in clock list document (RZG3S_clock_list_r1.00_20230602.xlsx). It is true the functionality could be preserved even w/o these 2 clocks but I kept them here as I saw them as core clocks even though they are not present in the Clock System Diagram from HW manual. With these, would you prefer to keep these clocks or just remove them? Thank you, Claudiu Beznea
On Fri, Sep 15, 2023 at 7:47 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 14.09.2023 16:04, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> core->name already contains the clock name thus, there is no > >> need to check the GET_SHIFT(core->conf) to decide on it. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- a/drivers/clk/renesas/rzg2l-cpg.c > >> +++ b/drivers/clk/renesas/rzg2l-cpg.c > >> @@ -266,7 +266,7 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core, > >> clk_hw_data->priv = priv; > >> clk_hw_data->conf = core->conf; > >> > >> - init.name = GET_SHIFT(core->conf) ? "sd1" : "sd0"; > >> + init.name = core->name; > > > > Note that this does change the case of the names (e.g. "SD0" => "sd0"). > > I guess no one cares... > > As of my experiments and investigation we should be good with it. Thx, will queue in renesas-clk-for-v6.7. Gr{oetje,eeting}s, Geert
On Thu, Sep 14, 2023 at 3:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > flag and mux_flags are intended to keep bit masks. Use u32 type for it. > > > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/clk/renesas/rzg2l-cpg.h > > +++ b/drivers/clk/renesas/rzg2l-cpg.h > > @@ -92,8 +92,8 @@ struct cpg_core_clk { > > unsigned int conf; > > const struct clk_div_table *dtable; > > const char * const *parent_names; > > - int flag; > > - int mux_flags; > > + u32 flag; > > "flag" is used for several purposes, which expected different types: > - clk_init_data.flags is unsigned long, > - The clk_divider_flags parameter of clk_hw_register_divider_table() is u8, > - The clk_divider_flags parameter of __clk_hw_register_divider() is u8, > - The flags parameter of __devm_clk_hw_register_mux() is unsigned long. > > > + u32 mux_flags; > > Actually the clk_mux_flags parameter of __devm_clk_hw_register_mux() is u8. > > > int num_parents; > > }; > > I guess u32 is fine for all. > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thx, will queue in renesas-clk-for-v6.7. Gr{oetje,eeting}s, Geert
Hi Claudiu, On Mon, Sep 18, 2023 at 9:50 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 15.09.2023 15:52, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> Add minimal clock and reset support for RZ/G3S SoC to be able to boot > >> Linux from SD Card/eMMC. This includes necessary core clocks for booting > >> and GIC, SCIF, GPIO, SD0 mod clocks and resets. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > > [ ... ] > > >> + CLK_PLL3_DIV2_4, > >> + CLK_PLL3_DIV2_8, > >> + CLK_PLL3_DIV6, > >> + CLK_PLL4, > >> + CLK_PLL6, > >> + CLK_PLL6_DIV2, > >> + CLK_SEL_SDHI0, > >> + CLK_SEL_PLL4, > >> + CLK_P1_DIV2, > >> + CLK_P3_DIV2, > > > > Do you need CLK_P1_DIV2 and CLK_P3_DIV2? > > I don't see them in Figure 7.3 ("Clock System Diagram (2)"). > > P1_DIV2 is clock source for MHU_PCLK or OTFDE_DDR_PCLK. > P3_DIV2 is clock source for DMAC_PCLK, OTFDE_SPI_PCLK. > These are expressed in clock list document > (RZG3S_clock_list_r1.00_20230602.xlsx). > > It is true the functionality could be preserved even w/o these 2 clocks but > I kept them here as I saw them as core clocks even though they are not > present in the Clock System Diagram from HW manual. I don't think you can, as the module clock abstraction does not support specifying a divider. Hence you do need an internal core clock between P1 and the module clock, to take care of the divider. > With these, would you prefer to keep these clocks or just remove them? Yes, as I expect that at least the DMAC_PCLK will be added, eventually. Gr{oetje,eeting}s, Geert
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > To get address that needs to be read/write for specific port > functionalities the P(), PM(), PMC(), PFC(), PIN(), IOLH() IEN(), ISEL() > macros are used. Some of these macros received as argument the hardware > port identifier, some hardware port offset address (e.g. ISEL() received > port identifier, IOLH() received port offset address). This makes hard to > extend the current driver for SoCs were port identifiers are not continuous > in memory map of pin controller. This is the case for RZ/G3S pin controller > were ports are mapped as follows: > > port offset port identifier > ----------- --------------- > 0x20 P0 > 0x21 P5 > 0x22 P6 > 0x23 P11 > 0x24 P12 > 0x25 P13 > 0x26 P14 > 0x27 P15 > 0x28 P16 > 0x29 P17 > 0x2a P18 > 0x30 P1 > 0x31 P2 > 0x32 P3 > 0x33 P4 > 0x34 P7 > 0x35 P8 > 0x36 P8 > 0x37 P10 > > To make this achievable change all the above macros used to get the address > of a port register for specific port functionality based on port hardware > address. Shortly, all the above macros will get as argument the port > offset address listed in the above table. > > With this RZG2L_SINGLE_PIN_GET_PORT_OFFSET() and > RZG2L_PIN_ID_TO_PORT_OFFSET() were replaced by and RZG2L_GPIO_PORT_GET_INDEX()? > RZG2L_PIN_CFG_TO_PORT_OFFSET(), RZG2L_SINGLE_PIN_GET_CFGS() and > RZG2L_GPIO_PORT_GET_CFGS() were replaced by RZG2L_PIN_CFG_TO_CAPS(). > > Also rzg2l_pinctrl_set_pfc_mode() don't need port argument anymore. > Also rzg2l_gpio_direction_input() and rzg2l_gpio_direction_output() don't > need to translate port and bit locally as this can be done by > rzg2l_gpio_set_direction(). > > To use the same naming for port, bit/pin and register offset the > port_offset variable names in different places was replaced by variable > named off and there is no need to initialize anymore cfg and bit in > different code places. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> This looks like a nice cleanup, thanks a lot! Prabhakar: do you like it, too? Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> You can find a few suggestions for improvement below... > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -202,9 +202,11 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > unsigned int group_selector) > { > struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct pinctrl_pin_desc *pin_desc; > + unsigned int i, *psel_val, *pin_data; > struct function_desc *func; > - unsigned int i, *psel_val; > struct group_desc *group; > + u32 port, pin, off; Please move the new variable declarations inside the for(), and combine them with their initialization. > int *pins; > > func = pinmux_generic_get_function(pctldev, func_selector); > @@ -218,11 +220,17 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > pins = group->pins; > > for (i = 0; i < group->num_pins; i++) { > - dev_dbg(pctrl->dev, "port:%u pin: %u PSEL:%u\n", > - RZG2L_PIN_ID_TO_PORT(pins[i]), RZG2L_PIN_ID_TO_PIN(pins[i]), > - psel_val[i]); > - rzg2l_pinctrl_set_pfc_mode(pctrl, RZG2L_PIN_ID_TO_PORT(pins[i]), > - RZG2L_PIN_ID_TO_PIN(pins[i]), psel_val[i]); > + pin_desc = &pctrl->desc.pins[pins[i]]; > + pin_data = pin_desc->drv_data; > + > + port = RZG2L_PIN_ID_TO_PORT(pins[i]); As port is unused but in the debug print, please drop the variable, and use RZG2L_PIN_ID_TO_PORT() in the debug print below. > + pin = RZG2L_PIN_ID_TO_PIN(pins[i]); > + off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); > + > + dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port, > + pin, off, psel_val[i]); > + > + rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]); > } 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
Hi Geert, On Wed, Sep 20, 2023 at 2:22 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Claudiu, > > Thanks for your patch! > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > To get address that needs to be read/write for specific port > > functionalities the P(), PM(), PMC(), PFC(), PIN(), IOLH() IEN(), ISEL() > > macros are used. Some of these macros received as argument the hardware > > port identifier, some hardware port offset address (e.g. ISEL() received > > port identifier, IOLH() received port offset address). This makes hard to > > extend the current driver for SoCs were port identifiers are not continuous > > in memory map of pin controller. This is the case for RZ/G3S pin controller > > were ports are mapped as follows: > > > > port offset port identifier > > ----------- --------------- > > 0x20 P0 > > 0x21 P5 > > 0x22 P6 > > 0x23 P11 > > 0x24 P12 > > 0x25 P13 > > 0x26 P14 > > 0x27 P15 > > 0x28 P16 > > 0x29 P17 > > 0x2a P18 > > 0x30 P1 > > 0x31 P2 > > 0x32 P3 > > 0x33 P4 > > 0x34 P7 > > 0x35 P8 > > 0x36 P8 > > 0x37 P10 > > > > To make this achievable change all the above macros used to get the address > > of a port register for specific port functionality based on port hardware > > address. Shortly, all the above macros will get as argument the port > > offset address listed in the above table. > > > > With this RZG2L_SINGLE_PIN_GET_PORT_OFFSET() and > > RZG2L_PIN_ID_TO_PORT_OFFSET() were replaced by > > and RZG2L_GPIO_PORT_GET_INDEX()? > > > RZG2L_PIN_CFG_TO_PORT_OFFSET(), RZG2L_SINGLE_PIN_GET_CFGS() and > > RZG2L_GPIO_PORT_GET_CFGS() were replaced by RZG2L_PIN_CFG_TO_CAPS(). > > > > Also rzg2l_pinctrl_set_pfc_mode() don't need port argument anymore. > > Also rzg2l_gpio_direction_input() and rzg2l_gpio_direction_output() don't > > need to translate port and bit locally as this can be done by > > rzg2l_gpio_set_direction(). > > > > To use the same naming for port, bit/pin and register offset the > > port_offset variable names in different places was replaced by variable > > named off and there is no need to initialize anymore cfg and bit in > > different code places. > > > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > This looks like a nice cleanup, thanks a lot! > Prabhakar: do you like it, too? > Yes indeed, I loved it when I reviewed it internally. This makes it easier for me to add those extra port pins present on rz/five ;) Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Cheers, Prabhakar > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > You can find a few suggestions for improvement below... > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > @@ -202,9 +202,11 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > > unsigned int group_selector) > > { > > struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + const struct pinctrl_pin_desc *pin_desc; > > + unsigned int i, *psel_val, *pin_data; > > struct function_desc *func; > > - unsigned int i, *psel_val; > > struct group_desc *group; > > + u32 port, pin, off; > > Please move the new variable declarations inside the for(), and > combine them with their initialization. > > > int *pins; > > > > func = pinmux_generic_get_function(pctldev, func_selector); > > @@ -218,11 +220,17 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > > pins = group->pins; > > > > for (i = 0; i < group->num_pins; i++) { > > - dev_dbg(pctrl->dev, "port:%u pin: %u PSEL:%u\n", > > - RZG2L_PIN_ID_TO_PORT(pins[i]), RZG2L_PIN_ID_TO_PIN(pins[i]), > > - psel_val[i]); > > - rzg2l_pinctrl_set_pfc_mode(pctrl, RZG2L_PIN_ID_TO_PORT(pins[i]), > > - RZG2L_PIN_ID_TO_PIN(pins[i]), psel_val[i]); > > + pin_desc = &pctrl->desc.pins[pins[i]]; > > + pin_data = pin_desc->drv_data; > > + > > + port = RZG2L_PIN_ID_TO_PORT(pins[i]); > > As port is unused but in the debug print, please drop the variable, > and use RZG2L_PIN_ID_TO_PORT() in the debug print below. > > > + pin = RZG2L_PIN_ID_TO_PIN(pins[i]); > > + off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); > > + > > + dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port, > > + pin, off, psel_val[i]); > > + > > + rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]); > > } > > 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 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > SD, PWPR power registers have different offsets b/w RZ/G2L and RZ/G3S. > Commit adds a per SoC configuration data structure that is initialized with > proper register offset for individual SoCs. The struct rzg2l_hwcfg will be > further extended in next commits. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > On RZ/G3S PFC register allow setting 8 functions for individual ports > (function1 to function8). For function1 register need to be configured > with 0, for function8 register need to be configured with 7. > We cannot use zero based addressing when requesting functions from > different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx) > states explicitly that function0 has different meaning. According to that table, function0 is GPIO. > For this add a new member to struct rzg2l_hwcfg that will keep the > offset that need to be substracted before applying a value to PFC register. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> But one question below... > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -136,9 +136,11 @@ struct rzg2l_register_offsets { > /** > * struct rzg2l_hwcfg - hardware configuration data structure > * @regs: hardware specific register offsets > + * @func_base: base number for port function (see register PFC) > */ > struct rzg2l_hwcfg { > const struct rzg2l_register_offsets regs; > + u8 func_base; > }; > > struct rzg2l_dedicated_configs { > @@ -221,6 +223,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > unsigned int group_selector) > { > struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; > const struct pinctrl_pin_desc *pin_desc; > unsigned int i, *psel_val, *pin_data; > struct function_desc *func; > @@ -247,9 +250,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); > > dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port, > - pin, off, psel_val[i]); > + pin, off, psel_val[i] - hwcfg->func_base); > > - rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]); > + rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base); > } > > return 0; Perhaps the adjustment should be done in rzg2l_dt_subnode_to_map() instead, when obtaining MUX_FUNC() from DT? That would allow you to do some basic validation on it too, which is currently completely missing (reject out-of-range values overflowing into adjacent PFC fields, reject zero on RZ/G3S). 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
Hi Claudiu, On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Move drive strength and output impedance values to SoC specific > configuration data structure (struct rzg2l_hwcfg). This allows extending > the drive strength support for RZ/G3S. Along with this the DS values > were converted to uA for simple integration with RZ/G3S support. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -133,13 +133,27 @@ struct rzg2l_register_offsets { > u16 sd_ch; > }; > > +/** > + * enum rzg2l_iolh_index - starting indexes in IOLH specific arrays indices > + * @RZG2L_IOLH_IDX_3V3: starting index for 3V3 power source > + * @RZG2L_IOLH_IDX_MAX: maximum index > + */ > +enum rzg2l_iolh_index { > + RZG2L_IOLH_IDX_3V3 = 0, > + RZG2L_IOLH_IDX_MAX = 4, > +}; > + > /** > * struct rzg2l_hwcfg - hardware configuration data structure > * @regs: hardware specific register offsets > + * @iolh_groupa_ua: IOLH group A micro amps specific values uA (or µA ;-) > + * @iolh_groupb_oi: IOLH group B output impedance specific values > * @func_base: base number for port function (see register PFC) > */ > struct rzg2l_hwcfg { > const struct rzg2l_register_offsets regs; > + u16 iolh_groupa_ua[RZG2L_IOLH_IDX_MAX]; > + u16 iolh_groupb_oi[RZG2L_IOLH_IDX_MAX]; > u8 func_base; > }; > > @@ -708,11 +719,11 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, > if (!(cfg & PIN_CFG_IOLH_A)) > return -EINVAL; > > - for (index = 0; index < ARRAY_SIZE(iolh_groupa_mA); index++) { > - if (arg == iolh_groupa_mA[index]) > + for (index = RZG2L_IOLH_IDX_3V3; index < RZG2L_IOLH_IDX_3V3 + 4; index++) { I'm not so fond of the hardcoded "+ 4", here and below. Please add and use a #define. > + if (arg == (hwcfg->iolh_groupa_ua[index] / 1000)) > break; > } > - if (index >= ARRAY_SIZE(iolh_groupa_mA)) > + if (index == (RZG2L_IOLH_IDX_3V3 + 4)) > return -EINVAL; > > rzg2l_rmw_pin_config(pctrl, IOLH(off), bit, IOLH_MASK, index); 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
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > RZ/G3S supports different drive strenght values for different power sources strength > and pin groups (A, B, C). On each group there could be up to 4 drive > strength values per power source. Available power sources are 1v8, 2v5, > 3v3. Drive strength values are fine tuned than what was previously > available on the driver thus the necessity of having micro-amp support. > As drive strength and power source values are linked togheter the together > hardware setup for these was moved at the end of > rzg2l_pinctrl_pinconf_set() to ensure proper validation of the new > values. > > The drive strength values are expected to be initialized though SoC > specific hardware configuration data structure. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -133,27 +135,40 @@ struct rzg2l_register_offsets { > u16 sd_ch; > }; > > +/* Value to be passed on drive strength arrays as invalid value. */ > +#define RZG2L_INVALID_IOLH_VAL (0xffff) I think you can do without this (see below). > + > /** > * enum rzg2l_iolh_index - starting indexes in IOLH specific arrays > + * @RZG2L_IOLH_IDX_1V8: starting index for 1V8 power source > + * @RZG2L_IOLH_IDX_2V5: starting index for 2V5 power source > * @RZG2L_IOLH_IDX_3V3: starting index for 3V3 power source > * @RZG2L_IOLH_IDX_MAX: maximum index > */ > enum rzg2l_iolh_index { > - RZG2L_IOLH_IDX_3V3 = 0, > - RZG2L_IOLH_IDX_MAX = 4, > + RZG2L_IOLH_IDX_1V8 = 0, > + RZG2L_IOLH_IDX_2V5 = 4, > + RZG2L_IOLH_IDX_3V3 = 8, > + RZG2L_IOLH_IDX_MAX = 12, > }; > > /** > * struct rzg2l_hwcfg - hardware configuration data structure > * @regs: hardware specific register offsets > * @iolh_groupa_ua: IOLH group A micro amps specific values > + * @iolh_groupb_ua: IOLH group B micro amps specific values > + * @iolh_groupc_ua: IOLH group C micro amps specific values uA > * @iolh_groupb_oi: IOLH group B output impedance specific values > + * @drive_strength_ua: driver strenght in ua is supported (otherwise mA is supported) drive strength in uA > * @func_base: base number for port function (see register PFC) > */ > struct rzg2l_hwcfg { > const struct rzg2l_register_offsets regs; > u16 iolh_groupa_ua[RZG2L_IOLH_IDX_MAX]; > + u16 iolh_groupb_ua[RZG2L_IOLH_IDX_MAX]; > + u16 iolh_groupc_ua[RZG2L_IOLH_IDX_MAX]; > u16 iolh_groupb_oi[RZG2L_IOLH_IDX_MAX]; > + bool drive_strength_ua; > u8 func_base; > }; > > @@ -555,6 +584,164 @@ static void rzg2l_rmw_pin_config(struct rzg2l_pinctrl *pctrl, u32 offset, > spin_unlock_irqrestore(&pctrl->lock, flags); > } > > +static int rzg2l_get_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps) > +{ > + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; > + const struct rzg2l_register_offsets *regs = &hwcfg->regs; > + unsigned long flags; > + void __iomem *addr; > + u32 pwr_reg; > + u16 ps; > + > + if (caps & PIN_CFG_IO_VMC_SD0) > + pwr_reg = SD_CH(regs->sd_ch, 0); > + else if (caps & PIN_CFG_IO_VMC_SD1) > + pwr_reg = SD_CH(regs->sd_ch, 1); > + else if (caps & PIN_CFG_IO_VMC_QSPI) > + pwr_reg = QSPI; > + else if (!(caps & PIN_CFG_SOFT_PS)) > + return -EINVAL; > + > + spin_lock_irqsave(&pctrl->lock, flags); No need to take this spinlock (it was just moved, and wasn't needed before). > + if (caps & PIN_CFG_SOFT_PS) { > + ps = pctrl->settings[pin].power_source; > + } else { > + addr = pctrl->base + pwr_reg; > + ps = (readl(addr) & PVDD_MASK) ? 1800 : 3300; > + } > + spin_unlock_irqrestore(&pctrl->lock, flags); I think the above can be simplified using a new caps_to_pwr_reg() helper: if (caps & PIN_CFG_SOFT_PS) return pctrl->settings[pin].power_source; addr = pctrl->base + caps_to_pwr_reg(caps); if (addr == (u32)-1) return -EINVAL; return (readl(addr) & PVDD_MASK) ? 1800 : 3300; BTW, if it wasn't for the initialization of settings[pin].power_source in rzg2l_pinctrl_register() using rzg2l_get_power_source() too, you could always return the cached value. > + > + return ps; > +} > + > +static int rzg2l_set_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps, u32 ps) > +{ > + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; > + const struct rzg2l_register_offsets *regs = &hwcfg->regs; > + unsigned long flags; > + void __iomem *addr; > + u32 pwr_reg; > + > + if (caps & PIN_CFG_IO_VMC_SD0) > + pwr_reg = SD_CH(regs->sd_ch, 0); > + else if (caps & PIN_CFG_IO_VMC_SD1) > + pwr_reg = SD_CH(regs->sd_ch, 1); > + else if (caps & PIN_CFG_IO_VMC_QSPI) > + pwr_reg = QSPI; > + else if (!(caps & PIN_CFG_SOFT_PS)) > + return -EINVAL; > + > + addr = pctrl->base + pwr_reg; > + spin_lock_irqsave(&pctrl->lock, flags); > + if (!(caps & PIN_CFG_SOFT_PS)) > + writel((ps == 1800) ? PVDD_1800 : PVDD_3300, addr); > + pctrl->settings[pin].power_source = ps; > + spin_unlock_irqrestore(&pctrl->lock, flags); No need to take this spinlock (it was just moved, and wasn't needed before). > + > + return 0; This function can be simplified in a similar way. > +} > +static u16 rzg2l_iolh_ua_to_val(const struct rzg2l_hwcfg *hwcfg, u32 caps, > + enum rzg2l_iolh_index ps_index, u16 ua) > +{ > + const u16 *array = NULL; > + u16 i; > + > + if (caps & PIN_CFG_IOLH_A) > + array = &hwcfg->iolh_groupa_ua[ps_index]; > + > + if (caps & PIN_CFG_IOLH_B) > + array = &hwcfg->iolh_groupb_ua[ps_index]; > + > + if (caps & PIN_CFG_IOLH_C) > + array = &hwcfg->iolh_groupc_ua[ps_index]; > + > + if (!array) > + return RZG2L_INVALID_IOLH_VAL; Just make the function return int, and return -EINVAL. > + > + for (i = 0; i < 4; i++) { > + if (array[i] == ua) > + return i; > + } > + > + return RZG2L_INVALID_IOLH_VAL; > +} > + > +static bool rzg2l_ds_supported(struct rzg2l_pinctrl *pctrl, u32 caps, rzg2l_ds_is_supported(), for consistency with rzg2l_ps_is_supported() > + enum rzg2l_iolh_index iolh_idx, > + u16 ds) > +{ > + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; > + const u16 *array = NULL; > + u16 i; > + > + if (caps & PIN_CFG_IOLH_A) > + array = hwcfg->iolh_groupa_ua; > + > + if (caps & PIN_CFG_IOLH_B) > + array = hwcfg->iolh_groupb_ua; > + > + if (caps & PIN_CFG_IOLH_C) > + array = hwcfg->iolh_groupc_ua; > + > + /* Should not happen. */ > + if (!array) > + return false; > + > + if (array[iolh_idx] == RZG2L_INVALID_IOLH_VAL) If zero uA is considered an invalid value, this can be simplified to if (!array[iolh_idx]) > + return false; > + > + for (i = 0; i < 4; i++) { > + if (array[iolh_idx + i] == ds) > + return true; > + } > + > + return false; > +} > + > static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev, > unsigned int _pin, > unsigned long *config) > @@ -594,40 +779,50 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev, > return -EINVAL; > break; > > - case PIN_CONFIG_POWER_SOURCE: { > - u32 pwr_reg = 0x0; > - > - if (cfg & PIN_CFG_IO_VMC_SD0) > - pwr_reg = SD_CH(regs->sd_ch, 0); > - else if (cfg & PIN_CFG_IO_VMC_SD1) > - pwr_reg = SD_CH(regs->sd_ch, 1); > - else if (cfg & PIN_CFG_IO_VMC_QSPI) > - pwr_reg = QSPI; > - else > - return -EINVAL; > - > - spin_lock_irqsave(&pctrl->lock, flags); > - addr = pctrl->base + pwr_reg; > - arg = (readl(addr) & PVDD_MASK) ? 1800 : 3300; > - spin_unlock_irqrestore(&pctrl->lock, flags); > + case PIN_CONFIG_POWER_SOURCE: > + ret = rzg2l_get_power_source(pctrl, _pin, cfg); > + if (ret < 0) > + return ret; > + arg = ret; > break; > - } > > case PIN_CONFIG_DRIVE_STRENGTH: { > unsigned int index; > > - if (!(cfg & PIN_CFG_IOLH_A)) > + if (!(cfg & PIN_CFG_IOLH_A) || hwcfg->drive_strength_ua) > return -EINVAL; > > index = rzg2l_read_pin_config(pctrl, IOLH(off), bit, IOLH_MASK); > + /* > + * Drive strenght mA is supported only by group A and only > + * for 3V3 port source. > + */ > arg = hwcfg->iolh_groupa_ua[index + RZG2L_IOLH_IDX_3V3] / 1000; > break; > } > > + case PIN_CONFIG_DRIVE_STRENGTH_UA: { > + enum rzg2l_iolh_index iolh_idx; > + u8 val; > + > + if (!(cfg & (PIN_CFG_IOLH_A | PIN_CFG_IOLH_B | PIN_CFG_IOLH_C)) || > + !hwcfg->drive_strength_ua) > + return -EINVAL; > + > + ret = rzg2l_get_power_source(pctrl, _pin, cfg); > + if (ret < 0) > + return ret; > + iolh_idx = rzg2l_ps_to_iolh_idx(ret); > + val = rzg2l_read_pin_config(pctrl, IOLH(off), bit, IOLH_MASK); > + arg = rzg2l_iolh_val_to_ua(hwcfg, cfg, iolh_idx + val); > + break; > + } > + > case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: { > unsigned int index; > > - if (!(cfg & PIN_CFG_IOLH_B)) > + if (!(cfg & PIN_CFG_IOLH_B) || > + hwcfg->iolh_groupb_oi[0] == RZG2L_INVALID_IOLH_VAL) !hwcfg->iolh_groupb_oi[0] > return -EINVAL; > > index = rzg2l_read_pin_config(pctrl, IOLH(off), bit, IOLH_MASK); > @@ -730,11 +904,20 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, > break; > } > > + case PIN_CONFIG_DRIVE_STRENGTH_UA: > + if (!(cfg & (PIN_CFG_IOLH_A | PIN_CFG_IOLH_B | PIN_CFG_IOLH_C)) || > + !hwcfg->drive_strength_ua) > + return -EINVAL; > + > + settings.drive_strength_ua = pinconf_to_config_argument(_configs[i]); > + break; > + > case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: { > unsigned int arg = pinconf_to_config_argument(_configs[i]); > unsigned int index; > > - if (!(cfg & PIN_CFG_IOLH_B)) > + if (!(cfg & PIN_CFG_IOLH_B) || > + hwcfg->iolh_groupb_oi[0] == RZG2L_INVALID_IOLH_VAL) !iolh_groupb_oi[0] > return -EINVAL; > > for (index = 0; index < ARRAY_SIZE(hwcfg->iolh_groupb_oi); index++) { > @@ -753,6 +936,47 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, > } > } > > + /* Apply drive strength and power source. */ > + if (memcmp(&settings, &pctrl->settings[_pin], sizeof(settings))) { I'd rather invert the logic and return early here, so you can decrease indentation below... > + enum rzg2l_iolh_index iolh_idx; > + unsigned long flags; > + int ret; > + u16 val; > + > + if (settings.power_source == pctrl->settings[_pin].power_source) > + goto apply_drive_strength; ... and invert the logic here to avoid the goto: if (settings.power_source != pctrl->settings[_pin].power_source)) { ... > + > + ret = rzg2l_ps_is_supported(settings.power_source); > + if (!ret) > + return -EINVAL; > + > + /* Apply power source. */ > + ret = rzg2l_set_power_source(pctrl, _pin, cfg, settings.power_source); > + if (ret) > + return ret; > + } > +apply_drive_strength: > + if (settings.drive_strength_ua == pctrl->settings[_pin].drive_strength_ua) > + return 0; Same here: if (settings.drive_strength_ua != pctrl->settings[_pin].drive_strength_ua) { ... > + > + iolh_idx = rzg2l_ps_to_iolh_idx(settings.power_source); > + ret = rzg2l_ds_supported(pctrl, cfg, iolh_idx, > + settings.drive_strength_ua); > + if (!ret) > + return -EINVAL; > + > + /* Get register value for this PS/DS tuple. */ > + val = rzg2l_iolh_ua_to_val(hwcfg, cfg, iolh_idx, settings.drive_strength_ua); > + if (val == RZG2L_INVALID_IOLH_VAL) > + return -EINVAL; Make val int, and return val if it is a negative error code. > + > + /* Apply drive strength. */ > + rzg2l_rmw_pin_config(pctrl, IOLH(off), bit, IOLH_MASK, val); > + spin_lock_irqsave(&pctrl->lock, flags); > + pctrl->settings[_pin].drive_strength_ua = settings.drive_strength_ua; > + spin_unlock_irqrestore(&pctrl->lock, flags); No need to take the spinlock. > + } > + And after that, you'll realize the memcmp() can just be dropped ;-) > return 0; > } > > @@ -1459,6 +1683,7 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl) > { > + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; > struct pinctrl_pin_desc *pins; > unsigned int i, j; > u32 *pin_data; > @@ -1501,6 +1726,22 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl) > pins[index].drv_data = &pin_data[index]; > } > > + pctrl->settings = devm_kzalloc(pctrl->dev, sizeof(*pctrl->settings) * pctrl->desc.npins, > + GFP_KERNEL); devm_kcalloc() > + if (!pctrl->settings) > + return -ENOMEM; > + > + for (i = 0; hwcfg->drive_strength_ua && i < pctrl->desc.npins; i++) { > + if (pin_data[i] & PIN_CFG_SOFT_PS) { > + pctrl->settings[i].power_source = 3300; > + } else { > + ret = rzg2l_get_power_source(pctrl, i, pin_data[i]); > + if (ret < 0) > + continue; > + pctrl->settings[i].power_source = ret; > + } > + } > + > ret = devm_pinctrl_register_and_init(pctrl->dev, &pctrl->desc, pctrl, > &pctrl->pctl); > if (ret) { > @@ -1574,6 +1815,8 @@ static const struct rzg2l_hwcfg rzg2l_hwcfg = { > .sd_ch = 0x3000, > }, > .iolh_groupa_ua = { > + /* 1v8, 2v5 power source */ > + [RZG2L_IOLH_IDX_1V8 ... RZG2L_IOLH_IDX_3V3 - 1] = RZG2L_INVALID_IOLH_VAL, If zero uA is considered an invalid value, the initialization above can be dropped. > /* 3v3 power source */ > [RZG2L_IOLH_IDX_3V3] = 2000, 4000, 8000, 12000, > }, 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
On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > struct rzg2l_pinctrl_data::dedicated_pins is constant thus mark it so. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-pinctrl-for-v6.7. 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
Hi Claudiu, On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add basic support for RZ/G3S to be able to boot from SD card, have a > running console port and use GPIOs. RZ/G3S has 82 general-purpose IO > ports. Support for the remaining pin functions (e.g. Ethernet, XSPI) > will be added along with controller specific support. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -1330,6 +1336,36 @@ static const u32 r9a07g043_gpio_configs[] = { > RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS), > }; > > +static const u32 r9a08g045_gpio_configs[] = { > + RZG2L_GPIO_PORT_PACK(4, 0x20, RZG3S_MPXED_PIN_FUNCS(A)), /* P0 */ > + RZG2L_GPIO_PORT_PACK(5, 0x30, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH0)), /* P1 */ P1_0 and P7_0 have IEN functionality. I don't know how to represent that... > + RZG2L_GPIO_PORT_PACK(4, 0x31, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH0)), /* P2 */ > + RZG2L_GPIO_PORT_PACK(4, 0x32, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH0)), /* P3 */ > + RZG2L_GPIO_PORT_PACK(6, 0x33, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH0)), /* P4 */ > + RZG2L_GPIO_PORT_PACK(5, 0x21, RZG3S_MPXED_PIN_FUNCS(A)), /* P5 */ > + RZG2L_GPIO_PORT_PACK(5, 0x22, RZG3S_MPXED_PIN_FUNCS(A)), /* P6 */ > + RZG2L_GPIO_PORT_PACK(5, 0x34, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH1)), /* P7 */ > + RZG2L_GPIO_PORT_PACK(5, 0x35, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH1)), /* P8 */ > + RZG2L_GPIO_PORT_PACK(4, 0x36, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH1)), /* P9 */ > + RZG2L_GPIO_PORT_PACK(5, 0x37, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > + PIN_CFG_IO_VMC_ETH1)), /* P10 */ > + RZG2L_GPIO_PORT_PACK(4, 0x23, RZG3S_MPXED_PIN_FUNCS(B) | PIN_CFG_IEN), /* P11 */ P11_0 does not have IEN functionality. I don't know how to represent that... > + RZG2L_GPIO_PORT_PACK(2, 0x24, RZG3S_MPXED_PIN_FUNCS(B) | PIN_CFG_IEN), /* P12 */ > + RZG2L_GPIO_PORT_PACK(5, 0x25, RZG3S_MPXED_PIN_FUNCS(A)), /* P13 */ > + RZG2L_GPIO_PORT_PACK(3, 0x26, RZG3S_MPXED_PIN_FUNCS(A)), /* P14 */ > + RZG2L_GPIO_PORT_PACK(4, 0x27, RZG3S_MPXED_PIN_FUNCS(A)), /* P15 */ > + RZG2L_GPIO_PORT_PACK(2, 0x28, RZG3S_MPXED_PIN_FUNCS(A)), /* P16 */ > + RZG2L_GPIO_PORT_PACK(4, 0x29, RZG3S_MPXED_PIN_FUNCS(A)), /* P17 */ > + RZG2L_GPIO_PORT_PACK(6, 0x2a, RZG3S_MPXED_PIN_FUNCS(A)), /* P18 */ > +}; > + > static const struct { > struct rzg2l_dedicated_configs common[35]; > struct rzg2l_dedicated_configs rzg2l_pins[7]; > @@ -1416,6 +1452,46 @@ static const struct { > } > }; > > +static const struct rzg2l_dedicated_configs rzg3s_dedicated_pins[] = { > + { "NMI", RZG2L_SINGLE_PIN_PACK(0x0, 0, (PIN_CFG_FILONOFF | PIN_CFG_FILNUM | > + PIN_CFG_FILCLKSEL)) }, > + { "TMS/SWDIO", RZG2L_SINGLE_PIN_PACK(0x1, 0, (PIN_CFG_IOLH_A | PIN_CFG_IEN | > + PIN_CFG_SOFT_PS)) }, > + { "TDO", RZG2L_SINGLE_PIN_PACK(0x1, 1, (PIN_CFG_IOLH_A | PIN_CFG_SOFT_PS)) }, > + { "WDTOVF_PERROUT#", RZG2L_SINGLE_PIN_PACK(0x6, 0, PIN_CFG_IOLH_A | PIN_CFG_SOFT_PS) }, > + { "SD0_CLK", RZG2L_SINGLE_PIN_PACK(0x10, 0, (PIN_CFG_IOLH_B | PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_CMD", RZG2L_SINGLE_PIN_PACK(0x10, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_RST#", RZG2L_SINGLE_PIN_PACK(0x10, 2, (PIN_CFG_IOLH_B | PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA0", RZG2L_SINGLE_PIN_PACK(0x11, 0, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA1", RZG2L_SINGLE_PIN_PACK(0x11, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA2", RZG2L_SINGLE_PIN_PACK(0x11, 2, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA3", RZG2L_SINGLE_PIN_PACK(0x11, 3, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA4", RZG2L_SINGLE_PIN_PACK(0x11, 4, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA5", RZG2L_SINGLE_PIN_PACK(0x11, 5, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA6", RZG2L_SINGLE_PIN_PACK(0x11, 6, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD0_DATA7", RZG2L_SINGLE_PIN_PACK(0x11, 7, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD0)) }, > + { "SD1_CLK", RZG2L_SINGLE_PIN_PACK(0x12, 0, (PIN_CFG_IOLH_B | PIN_CFG_IO_VMC_SD1)) }, > + { "SD1_CMD", RZG2L_SINGLE_PIN_PACK(0x12, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD1)) }, > + { "SD1_DATA0", RZG2L_SINGLE_PIN_PACK(0x13, 0, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD1)) }, > + { "SD1_DATA1", RZG2L_SINGLE_PIN_PACK(0x13, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD1)) }, > + { "SD1_DATA2", RZG2L_SINGLE_PIN_PACK(0x13, 2, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD1)) }, > + { "SD1_DATA3", RZG2L_SINGLE_PIN_PACK(0x13, 3, (PIN_CFG_IOLH_B | PIN_CFG_IEN | > + PIN_CFG_IO_VMC_SD1)) }, Is there any specific reason you left out the XSPI, Audio clock, and I3C pins? > +}; > + > static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct rzg2l_pinctrl_data *data) > { > unsigned int gpioint; > @@ -1823,6 +1899,40 @@ static const struct rzg2l_hwcfg rzg2l_hwcfg = { > .iolh_groupb_oi = { 100, 66, 50, 33, }, > }; > > +static const struct rzg2l_hwcfg rzg3s_hwcfg = { > + .regs = { > + .pwpr = 0x3000, > + .sd_ch = 0x3004, > + }, > + .iolh_groupa_ua = { > + /* 1v8 power source */ > + [RZG2L_IOLH_IDX_1V8] = 2200, 4400, 9000, 10000, > + /* 2v5 power source */ > + [RZG2L_IOLH_IDX_2V5 ... RZG2L_IOLH_IDX_3V3 - 1] = RZG2L_INVALID_IOLH_VAL, Can be dropped once zero means invalid. > + /* 3v3 power source */ > + [RZG2L_IOLH_IDX_3V3] = 1900, 4000, 8000, 9000, > + }, > + .iolh_groupb_ua = { > + /* 1v8 power source */ > + [RZG2L_IOLH_IDX_1V8] = 7000, 8000, 9000, 10000, > + /* 2v5 power source */ > + [RZG2L_IOLH_IDX_2V5 ... RZG2L_IOLH_IDX_3V3 - 1] = RZG2L_INVALID_IOLH_VAL, Can be dropped once zero means invalid. > + /* 3v3 power source */ > + [RZG2L_IOLH_IDX_3V3] = 4000, 6000, 8000, 9000, > + }, > + .iolh_groupc_ua = { > + /* 1v8 power source */ > + [RZG2L_IOLH_IDX_1V8] = 5200, 6000, 6550, 6800, > + /* 2v5 source */ > + [RZG2L_IOLH_IDX_2V5] = 4700, 5300, 5800, 6100, > + /* 3v3 power source */ > + [RZG2L_IOLH_IDX_3V3] = 4500, 5200, 5700, 6050, > + }, > + .drive_strength_ua = true, > + .iolh_groupb_oi = { [0 ... 3] = RZG2L_INVALID_IOLH_VAL, }, > + .func_base = 1, > +}; > + > static struct rzg2l_pinctrl_data r9a07g043_data = { > .port_pins = rzg2l_gpio_names, > .port_pin_configs = r9a07g043_gpio_configs, > @@ -1844,6 +1954,16 @@ static struct rzg2l_pinctrl_data r9a07g044_data = { > .hwcfg = &rzg2l_hwcfg, > }; > > +static struct rzg2l_pinctrl_data r9a08g045_data = { > + .port_pins = rzg2l_gpio_names, > + .port_pin_configs = r9a08g045_gpio_configs, > + .n_ports = ARRAY_SIZE(r9a08g045_gpio_configs), > + .dedicated_pins = rzg3s_dedicated_pins, > + .n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT, > + .n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins), > + .hwcfg = &rzg3s_hwcfg, > +}; > + > static const struct of_device_id rzg2l_pinctrl_of_table[] = { > { > .compatible = "renesas,r9a07g043-pinctrl", Please add a BUILD_BUG_ON() check for RZ/G3S to the rzg2l_pinctrl_probe() function, as is done for the other SoCs in the family. The rest LGTM. Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add initial device tree for RZ/G3S SMARC EVK board. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- /dev/null > +++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc.dts > @@ -0,0 +1,17 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +/* > + * Device Tree Source for the RZ/G3S SMARC EVK board > + * > + * Copyright (C) 2023 Renesas Electronics Corp. > + */ > + > +/dts-v1/; > + > +#include "r9a08g045s33.dtsi" > +#include "rzg3s-smarc-som.dtsi" > +#include "rzg3s-smarc.dtsi" > + > +/ { > + model = "Renesas SMARC EVK version 2 based on r9a08g045s33"; > + compatible = "renesas,smarc2-evk", "renesas,r9a08g045s33", "renesas,r9a08g045"; > +}; Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Of course any updates to the DT bindings should be reflected here. Gr{oetje,eeting}s, Geert
Hi, Geert, On 21.09.2023 15:51, Geert Uytterhoeven wrote: > Hi Claudiu, > > Thanks for your patch! > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> On RZ/G3S PFC register allow setting 8 functions for individual ports >> (function1 to function8). For function1 register need to be configured >> with 0, for function8 register need to be configured with 7. >> We cannot use zero based addressing when requesting functions from >> different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx) >> states explicitly that function0 has different meaning. > > According to that table, function0 is GPIO. Yes, I'll mention it like this in the next version. > >> For this add a new member to struct rzg2l_hwcfg that will keep the >> offset that need to be substracted before applying a value to PFC register. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > But one question below... > >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c >> @@ -136,9 +136,11 @@ struct rzg2l_register_offsets { >> /** >> * struct rzg2l_hwcfg - hardware configuration data structure >> * @regs: hardware specific register offsets >> + * @func_base: base number for port function (see register PFC) >> */ >> struct rzg2l_hwcfg { >> const struct rzg2l_register_offsets regs; >> + u8 func_base; >> }; >> >> struct rzg2l_dedicated_configs { >> @@ -221,6 +223,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, >> unsigned int group_selector) >> { >> struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); >> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; >> const struct pinctrl_pin_desc *pin_desc; >> unsigned int i, *psel_val, *pin_data; >> struct function_desc *func; >> @@ -247,9 +250,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, >> off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); >> >> dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port, >> - pin, off, psel_val[i]); >> + pin, off, psel_val[i] - hwcfg->func_base); >> >> - rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]); >> + rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base); >> } >> >> return 0; > > Perhaps the adjustment should be done in rzg2l_dt_subnode_to_map() > instead, when obtaining MUX_FUNC() from DT? That would allow you to do > some basic validation on it too, which is currently completely missing > (reject out-of-range values overflowing into adjacent PFC fields, > reject zero on RZ/G3S). I'll have a look on this. I see .set_mux() can also be called from sysfs though pinmux-select exported file thus, I don't know at the moment if validating it on rzg2l_dt_subnode_to_map() will be enough. Would it be OK to have this outside of this series or you would prefer it now? Thank you, Claudiu Beznea > > 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
Hi, Geert, On 21.09.2023 17:58, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Add basic support for RZ/G3S to be able to boot from SD card, have a >> running console port and use GPIOs. RZ/G3S has 82 general-purpose IO >> ports. Support for the remaining pin functions (e.g. Ethernet, XSPI) >> will be added along with controller specific support. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c >> @@ -1330,6 +1336,36 @@ static const u32 r9a07g043_gpio_configs[] = { >> RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS), >> }; >> >> +static const u32 r9a08g045_gpio_configs[] = { >> + RZG2L_GPIO_PORT_PACK(4, 0x20, RZG3S_MPXED_PIN_FUNCS(A)), /* P0 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x30, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH0)), /* P1 */ > > P1_0 and P7_0 have IEN functionality. > I don't know how to represent that... I think Prabhakar's series at [1] may help (or make a step forward) in supporting this. I have in mind to wait for it and adapt RZ/G3S afterwards. [1] https://lore.kernel.org/all/20230630120433.49529-2-prabhakar.mahadev-lad.rj@bp.renesas.com/ > >> + RZG2L_GPIO_PORT_PACK(4, 0x31, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH0)), /* P2 */ >> + RZG2L_GPIO_PORT_PACK(4, 0x32, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH0)), /* P3 */ >> + RZG2L_GPIO_PORT_PACK(6, 0x33, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH0)), /* P4 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x21, RZG3S_MPXED_PIN_FUNCS(A)), /* P5 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x22, RZG3S_MPXED_PIN_FUNCS(A)), /* P6 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x34, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH1)), /* P7 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x35, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH1)), /* P8 */ >> + RZG2L_GPIO_PORT_PACK(4, 0x36, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH1)), /* P9 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x37, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | >> + PIN_CFG_IO_VMC_ETH1)), /* P10 */ >> + RZG2L_GPIO_PORT_PACK(4, 0x23, RZG3S_MPXED_PIN_FUNCS(B) | PIN_CFG_IEN), /* P11 */ > > P11_0 does not have IEN functionality. > I don't know how to represent that... Same here. > >> + RZG2L_GPIO_PORT_PACK(2, 0x24, RZG3S_MPXED_PIN_FUNCS(B) | PIN_CFG_IEN), /* P12 */ >> + RZG2L_GPIO_PORT_PACK(5, 0x25, RZG3S_MPXED_PIN_FUNCS(A)), /* P13 */ >> + RZG2L_GPIO_PORT_PACK(3, 0x26, RZG3S_MPXED_PIN_FUNCS(A)), /* P14 */ >> + RZG2L_GPIO_PORT_PACK(4, 0x27, RZG3S_MPXED_PIN_FUNCS(A)), /* P15 */ >> + RZG2L_GPIO_PORT_PACK(2, 0x28, RZG3S_MPXED_PIN_FUNCS(A)), /* P16 */ >> + RZG2L_GPIO_PORT_PACK(4, 0x29, RZG3S_MPXED_PIN_FUNCS(A)), /* P17 */ >> + RZG2L_GPIO_PORT_PACK(6, 0x2a, RZG3S_MPXED_PIN_FUNCS(A)), /* P18 */ >> +}; >> + >> static const struct { >> struct rzg2l_dedicated_configs common[35]; >> struct rzg2l_dedicated_configs rzg2l_pins[7]; >> @@ -1416,6 +1452,46 @@ static const struct { >> } >> }; >> >> +static const struct rzg2l_dedicated_configs rzg3s_dedicated_pins[] = { >> + { "NMI", RZG2L_SINGLE_PIN_PACK(0x0, 0, (PIN_CFG_FILONOFF | PIN_CFG_FILNUM | >> + PIN_CFG_FILCLKSEL)) }, >> + { "TMS/SWDIO", RZG2L_SINGLE_PIN_PACK(0x1, 0, (PIN_CFG_IOLH_A | PIN_CFG_IEN | >> + PIN_CFG_SOFT_PS)) }, >> + { "TDO", RZG2L_SINGLE_PIN_PACK(0x1, 1, (PIN_CFG_IOLH_A | PIN_CFG_SOFT_PS)) }, >> + { "WDTOVF_PERROUT#", RZG2L_SINGLE_PIN_PACK(0x6, 0, PIN_CFG_IOLH_A | PIN_CFG_SOFT_PS) }, >> + { "SD0_CLK", RZG2L_SINGLE_PIN_PACK(0x10, 0, (PIN_CFG_IOLH_B | PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_CMD", RZG2L_SINGLE_PIN_PACK(0x10, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_RST#", RZG2L_SINGLE_PIN_PACK(0x10, 2, (PIN_CFG_IOLH_B | PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA0", RZG2L_SINGLE_PIN_PACK(0x11, 0, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA1", RZG2L_SINGLE_PIN_PACK(0x11, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA2", RZG2L_SINGLE_PIN_PACK(0x11, 2, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA3", RZG2L_SINGLE_PIN_PACK(0x11, 3, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA4", RZG2L_SINGLE_PIN_PACK(0x11, 4, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA5", RZG2L_SINGLE_PIN_PACK(0x11, 5, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA6", RZG2L_SINGLE_PIN_PACK(0x11, 6, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD0_DATA7", RZG2L_SINGLE_PIN_PACK(0x11, 7, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD0)) }, >> + { "SD1_CLK", RZG2L_SINGLE_PIN_PACK(0x12, 0, (PIN_CFG_IOLH_B | PIN_CFG_IO_VMC_SD1)) }, >> + { "SD1_CMD", RZG2L_SINGLE_PIN_PACK(0x12, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD1)) }, >> + { "SD1_DATA0", RZG2L_SINGLE_PIN_PACK(0x13, 0, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD1)) }, >> + { "SD1_DATA1", RZG2L_SINGLE_PIN_PACK(0x13, 1, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD1)) }, >> + { "SD1_DATA2", RZG2L_SINGLE_PIN_PACK(0x13, 2, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD1)) }, >> + { "SD1_DATA3", RZG2L_SINGLE_PIN_PACK(0x13, 3, (PIN_CFG_IOLH_B | PIN_CFG_IEN | >> + PIN_CFG_IO_VMC_SD1)) }, > > Is there any specific reason you left out the XSPI, Audio clock, and I3C pins? I kept only the necessary support for booting and having SDs, GPIO functional as a way of proving that all that has been added has been tested (similar to clock support). Thus, with e.g. XSPI support I will add at the same time clocks and pinctrl. > >> +}; >> + >> static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct rzg2l_pinctrl_data *data) >> { >> unsigned int gpioint; >> @@ -1823,6 +1899,40 @@ static const struct rzg2l_hwcfg rzg2l_hwcfg = { >> .iolh_groupb_oi = { 100, 66, 50, 33, }, >> }; >> >> +static const struct rzg2l_hwcfg rzg3s_hwcfg = { >> + .regs = { >> + .pwpr = 0x3000, >> + .sd_ch = 0x3004, >> + }, >> + .iolh_groupa_ua = { >> + /* 1v8 power source */ >> + [RZG2L_IOLH_IDX_1V8] = 2200, 4400, 9000, 10000, >> + /* 2v5 power source */ >> + [RZG2L_IOLH_IDX_2V5 ... RZG2L_IOLH_IDX_3V3 - 1] = RZG2L_INVALID_IOLH_VAL, > > Can be dropped once zero means invalid. > >> + /* 3v3 power source */ >> + [RZG2L_IOLH_IDX_3V3] = 1900, 4000, 8000, 9000, >> + }, >> + .iolh_groupb_ua = { >> + /* 1v8 power source */ >> + [RZG2L_IOLH_IDX_1V8] = 7000, 8000, 9000, 10000, >> + /* 2v5 power source */ >> + [RZG2L_IOLH_IDX_2V5 ... RZG2L_IOLH_IDX_3V3 - 1] = RZG2L_INVALID_IOLH_VAL, > > Can be dropped once zero means invalid. > >> + /* 3v3 power source */ >> + [RZG2L_IOLH_IDX_3V3] = 4000, 6000, 8000, 9000, >> + }, >> + .iolh_groupc_ua = { >> + /* 1v8 power source */ >> + [RZG2L_IOLH_IDX_1V8] = 5200, 6000, 6550, 6800, >> + /* 2v5 source */ >> + [RZG2L_IOLH_IDX_2V5] = 4700, 5300, 5800, 6100, >> + /* 3v3 power source */ >> + [RZG2L_IOLH_IDX_3V3] = 4500, 5200, 5700, 6050, >> + }, >> + .drive_strength_ua = true, >> + .iolh_groupb_oi = { [0 ... 3] = RZG2L_INVALID_IOLH_VAL, }, >> + .func_base = 1, >> +}; >> + >> static struct rzg2l_pinctrl_data r9a07g043_data = { >> .port_pins = rzg2l_gpio_names, >> .port_pin_configs = r9a07g043_gpio_configs, >> @@ -1844,6 +1954,16 @@ static struct rzg2l_pinctrl_data r9a07g044_data = { >> .hwcfg = &rzg2l_hwcfg, >> }; >> >> +static struct rzg2l_pinctrl_data r9a08g045_data = { >> + .port_pins = rzg2l_gpio_names, >> + .port_pin_configs = r9a08g045_gpio_configs, >> + .n_ports = ARRAY_SIZE(r9a08g045_gpio_configs), >> + .dedicated_pins = rzg3s_dedicated_pins, >> + .n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT, >> + .n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins), >> + .hwcfg = &rzg3s_hwcfg, >> +}; >> + >> static const struct of_device_id rzg2l_pinctrl_of_table[] = { >> { >> .compatible = "renesas,r9a07g043-pinctrl", > > Please add a BUILD_BUG_ON() check for RZ/G3S to the > rzg2l_pinctrl_probe() function, as is done for the other SoCs in > the family. Ok. > > The rest LGTM. > > Gr{oetje,eeting}s, > > Geert >
Hi, Geert, On 14.09.2023 15:55, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) >> the computation formula for PLL rate is as follows: >> >> Fout = ((m + k/65536) * Fin) / (p * 2^s) >> >> and k has values in range [-32768, 32767]. Dividing k by 65536 with >> integer variables leads all the time to zero. Thus we may have slight >> differences b/w what has been set vs. what is displayed. Thus, >> get rid of this and decompose the formula before dividing k by 65536. >> >> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, >> struct pll_clk *pll_clk = to_pll(hw); >> struct rzg2l_cpg_priv *priv = pll_clk->priv; >> unsigned int val1, val2; >> - unsigned int mult = 1; >> - unsigned int div = 1; >> + unsigned int div; >> + u64 rate; >> + s16 kdiv; >> >> if (pll_clk->type != CLK_TYPE_SAM_PLL) >> return parent_rate; >> >> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); >> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); >> - mult = MDIV(val1) + KDIV(val1) / 65536; >> + kdiv = KDIV(val1); >> div = PDIV(val1) << SDIV(val2); >> >> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); >> + rate = (u64)MDIV(val1) * parent_rate; >> + rate += ((long long)parent_rate * kdiv) / 65536; > > As the division is a binary shift, you can use the mul_u64_u32_shr() helper, > and incorporate the sdiv shift at the same time: > > rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); > > You can save a multiplication by premultiplying mdiv by 65536: > > rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), > 16 + SDIV(val2)); Looking again at this: KDIV (aka DIV_K) could have negative values thus mul_u64_u32_shr() cannot be used here. > >> + >> + return DIV_ROUND_CLOSEST_ULL(rate, div); > > return DIV_ROUND_CLOSEST_ULL(rate, PDIV(val1)); > >> } >> >> static const struct clk_ops rzg2l_cpg_pll_ops = { > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Tue, Sep 26, 2023 at 11:55 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 21.09.2023 15:51, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> On RZ/G3S PFC register allow setting 8 functions for individual ports > >> (function1 to function8). For function1 register need to be configured > >> with 0, for function8 register need to be configured with 7. > >> We cannot use zero based addressing when requesting functions from > >> different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx) > >> states explicitly that function0 has different meaning. > > > > According to that table, function0 is GPIO. > > Yes, I'll mention it like this in the next version. > > >> For this add a new member to struct rzg2l_hwcfg that will keep the > >> offset that need to be substracted before applying a value to PFC register. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > But one question below... > > > >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > >> @@ -136,9 +136,11 @@ struct rzg2l_register_offsets { > >> /** > >> * struct rzg2l_hwcfg - hardware configuration data structure > >> * @regs: hardware specific register offsets > >> + * @func_base: base number for port function (see register PFC) > >> */ > >> struct rzg2l_hwcfg { > >> const struct rzg2l_register_offsets regs; > >> + u8 func_base; > >> }; > >> > >> struct rzg2l_dedicated_configs { > >> @@ -221,6 +223,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > >> unsigned int group_selector) > >> { > >> struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > >> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; > >> const struct pinctrl_pin_desc *pin_desc; > >> unsigned int i, *psel_val, *pin_data; > >> struct function_desc *func; > >> @@ -247,9 +250,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, > >> off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); > >> > >> dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port, > >> - pin, off, psel_val[i]); > >> + pin, off, psel_val[i] - hwcfg->func_base); > >> > >> - rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]); > >> + rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base); > >> } > >> > >> return 0; > > > > Perhaps the adjustment should be done in rzg2l_dt_subnode_to_map() > > instead, when obtaining MUX_FUNC() from DT? That would allow you to do > > some basic validation on it too, which is currently completely missing > > (reject out-of-range values overflowing into adjacent PFC fields, > > reject zero on RZ/G3S). > > I'll have a look on this. I see .set_mux() can also be called from sysfs > though pinmux-select exported file thus, I don't know at the moment if > validating it on rzg2l_dt_subnode_to_map() will be enough. OK, that's a good reason to keep it as-is. > Would it be OK to have this outside of this series or you would prefer it now? That can be done later. I believe currently there is no validation against the register field size limit anyway. Thanks! Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 26, 2023 at 12:58 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 21.09.2023 17:58, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> Add basic support for RZ/G3S to be able to boot from SD card, have a > >> running console port and use GPIOs. RZ/G3S has 82 general-purpose IO > >> ports. Support for the remaining pin functions (e.g. Ethernet, XSPI) > >> will be added along with controller specific support. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > >> @@ -1330,6 +1336,36 @@ static const u32 r9a07g043_gpio_configs[] = { > >> RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS), > >> }; > >> > >> +static const u32 r9a08g045_gpio_configs[] = { > >> + RZG2L_GPIO_PORT_PACK(4, 0x20, RZG3S_MPXED_PIN_FUNCS(A)), /* P0 */ > >> + RZG2L_GPIO_PORT_PACK(5, 0x30, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | > >> + PIN_CFG_IO_VMC_ETH0)), /* P1 */ > > > > P1_0 and P7_0 have IEN functionality. > > I don't know how to represent that... > > I think Prabhakar's series at [1] may help (or make a step forward) in > supporting this. I have in mind to wait for it and adapt RZ/G3S afterwards. OK. > [1] > https://lore.kernel.org/all/20230630120433.49529-2-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > Is there any specific reason you left out the XSPI, Audio clock, and I3C pins? > > I kept only the necessary support for booting and having SDs, GPIO > functional as a way of proving that all that has been added has been tested > (similar to clock support). Thus, with e.g. XSPI support I will add at the > same time clocks and pinctrl. IC. I all fairness, you did write in your patch description that support for e.g. XSPI will be added later, so I'm to blame here. Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 26, 2023 at 1:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 14.09.2023 15:55, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) > >> the computation formula for PLL rate is as follows: > >> > >> Fout = ((m + k/65536) * Fin) / (p * 2^s) > >> > >> and k has values in range [-32768, 32767]. Dividing k by 65536 with > >> integer variables leads all the time to zero. Thus we may have slight > >> differences b/w what has been set vs. what is displayed. Thus, > >> get rid of this and decompose the formula before dividing k by 65536. > >> > >> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- a/drivers/clk/renesas/rzg2l-cpg.c > >> +++ b/drivers/clk/renesas/rzg2l-cpg.c > >> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, > >> struct pll_clk *pll_clk = to_pll(hw); > >> struct rzg2l_cpg_priv *priv = pll_clk->priv; > >> unsigned int val1, val2; > >> - unsigned int mult = 1; > >> - unsigned int div = 1; > >> + unsigned int div; > >> + u64 rate; > >> + s16 kdiv; > >> > >> if (pll_clk->type != CLK_TYPE_SAM_PLL) > >> return parent_rate; > >> > >> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); > >> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); > >> - mult = MDIV(val1) + KDIV(val1) / 65536; > >> + kdiv = KDIV(val1); > >> div = PDIV(val1) << SDIV(val2); > >> > >> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); > >> + rate = (u64)MDIV(val1) * parent_rate; > >> + rate += ((long long)parent_rate * kdiv) / 65536; > > > > As the division is a binary shift, you can use the mul_u64_u32_shr() helper, > > and incorporate the sdiv shift at the same time: > > > > rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); [1]^ > > > > You can save a multiplication by premultiplying mdiv by 65536: > > > > rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), > > 16 + SDIV(val2)); [2]^ > > Looking again at this: KDIV (aka DIV_K) could have negative values thus > mul_u64_u32_shr() cannot be used here. That means you can indeed not use [1]. But you can still use [2], as MDIV() must be in the range 64..533[3], so "(MDIV(val1) << 16)) + (s16)KDIV(val1)" is always positive. Note that you do need the cast to s16 (which I had missed before), or the intermediate variable kdiv of type s16 (like in your patch). [3] As the current PLL driver is read-only, there is no calculation or validation of the PLL parameters. > >> + > >> + return DIV_ROUND_CLOSEST_ULL(rate, div); > > > > return DIV_ROUND_CLOSEST_ULL(rate, PDIV(val1)); Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 26, 2023 at 4:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Sep 26, 2023 at 1:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > > On 14.09.2023 15:55, Geert Uytterhoeven wrote: > > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >> > > >> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) > > >> the computation formula for PLL rate is as follows: > > >> > > >> Fout = ((m + k/65536) * Fin) / (p * 2^s) > > >> > > >> and k has values in range [-32768, 32767]. Dividing k by 65536 with > > >> integer variables leads all the time to zero. Thus we may have slight > > >> differences b/w what has been set vs. what is displayed. Thus, > > >> get rid of this and decompose the formula before dividing k by 65536. > > >> > > >> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > >> --- a/drivers/clk/renesas/rzg2l-cpg.c > > >> +++ b/drivers/clk/renesas/rzg2l-cpg.c > > >> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, > > >> struct pll_clk *pll_clk = to_pll(hw); > > >> struct rzg2l_cpg_priv *priv = pll_clk->priv; > > >> unsigned int val1, val2; > > >> - unsigned int mult = 1; > > >> - unsigned int div = 1; > > >> + unsigned int div; > > >> + u64 rate; > > >> + s16 kdiv; > > >> > > >> if (pll_clk->type != CLK_TYPE_SAM_PLL) > > >> return parent_rate; > > >> > > >> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); > > >> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); > > >> - mult = MDIV(val1) + KDIV(val1) / 65536; > > >> + kdiv = KDIV(val1); > > >> div = PDIV(val1) << SDIV(val2); > > >> > > >> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); > > >> + rate = (u64)MDIV(val1) * parent_rate; > > >> + rate += ((long long)parent_rate * kdiv) / 65536; > > > > > > As the division is a binary shift, you can use the mul_u64_u32_shr() helper, > > > and incorporate the sdiv shift at the same time: > > > > > > rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); > > [1]^ > > > > > > > You can save a multiplication by premultiplying mdiv by 65536: > > > > > > rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), > > > 16 + SDIV(val2)); > > [2]^ > > > > > Looking again at this: KDIV (aka DIV_K) could have negative values thus > > mul_u64_u32_shr() cannot be used here. > > That means you can indeed not use [1]. > > But you can still use [2], as MDIV() must be in the range 64..533[3], > so "(MDIV(val1) << 16)) + (s16)KDIV(val1)" is always positive. > Note that you do need the cast to s16 (which I had missed before), or > the intermediate variable kdiv of type s16 (like in your patch). Or include the cast to a signed type in the definition of KDIV(). Gr{oetje,eeting}s, Geert
Hi, Geert, On 27.09.2023 11:00, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 26, 2023 at 4:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Tue, Sep 26, 2023 at 1:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: >>> On 14.09.2023 15:55, Geert Uytterhoeven wrote: >>>> On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>> >>>>> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) >>>>> the computation formula for PLL rate is as follows: >>>>> >>>>> Fout = ((m + k/65536) * Fin) / (p * 2^s) >>>>> >>>>> and k has values in range [-32768, 32767]. Dividing k by 65536 with >>>>> integer variables leads all the time to zero. Thus we may have slight >>>>> differences b/w what has been set vs. what is displayed. Thus, >>>>> get rid of this and decompose the formula before dividing k by 65536. >>>>> >>>>> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") >>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> Thanks for your patch! >>>> >>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c >>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c >>>>> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, >>>>> struct pll_clk *pll_clk = to_pll(hw); >>>>> struct rzg2l_cpg_priv *priv = pll_clk->priv; >>>>> unsigned int val1, val2; >>>>> - unsigned int mult = 1; >>>>> - unsigned int div = 1; >>>>> + unsigned int div; >>>>> + u64 rate; >>>>> + s16 kdiv; >>>>> >>>>> if (pll_clk->type != CLK_TYPE_SAM_PLL) >>>>> return parent_rate; >>>>> >>>>> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); >>>>> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); >>>>> - mult = MDIV(val1) + KDIV(val1) / 65536; >>>>> + kdiv = KDIV(val1); >>>>> div = PDIV(val1) << SDIV(val2); >>>>> >>>>> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); >>>>> + rate = (u64)MDIV(val1) * parent_rate; >>>>> + rate += ((long long)parent_rate * kdiv) / 65536; >>>> >>>> As the division is a binary shift, you can use the mul_u64_u32_shr() helper, >>>> and incorporate the sdiv shift at the same time: >>>> >>>> rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); >> >> [1]^ >> >>>> >>>> You can save a multiplication by premultiplying mdiv by 65536: >>>> >>>> rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), >>>> 16 + SDIV(val2)); >> >> [2]^ >> >>> >>> Looking again at this: KDIV (aka DIV_K) could have negative values thus >>> mul_u64_u32_shr() cannot be used here. >> >> That means you can indeed not use [1]. You're right. Thanks for the input! >> >> But you can still use [2], as MDIV() must be in the range 64..533[3], >> so "(MDIV(val1) << 16)) + (s16)KDIV(val1)" is always positive. >> Note that you do need the cast to s16 (which I had missed before), or >> the intermediate variable kdiv of type s16 (like in your patch). > > Or include the cast to a signed type in the definition of KDIV(). > > Gr{oetje,eeting}s, > > Geert >
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Hi, This patch series adds initial support for The Renesas RZ/G3S (R9A08G045{S33}) SoC. The RZ/G3S device is a general-purpose microprocessor with a single-core Arm® Cortex®-A55 (1.1GHz) and a dual-core Arm® Cortex®-M33 (250MHz), perfect for an IOT gateway controller. This includes: - SoC identification; - clocks (core clocks, pin controller clock, serial interface, SD ch0 clock) and corresponding resets; - minimal device tree for SoM and carrier boards. With this series Linux can boot from eMMC or SD card. The eMMC and uSD interface are multiplexed on the SoM; selection is made using a hardware switch. Patches are gouped as follows: - 01-04 adds SoC identification support; - 05 is a simple cleanup on SoC identification support - 06-09 contain fixes on clock drivers identified while adding RZ/G3S support - 10-14 clock cleanups identifies while adding support for RZ/G3S - 15-22 clock changes needed by RZ/G3S - 23-30 pinctrl changes needed by RZ/G3S - 31 document SDHI for RZ/G3S - 32-37 device tree support for RZ/G3S Thank you, Claudiu Beznea Claudiu Beznea (37): dt-bindings: serial: renesas,scif: document r9a08g045 support dt-bindings: soc: renesas: document Renesas RZ/G3S SoC variants dt-bindings: soc: renesas: renesas,rzg2l-sysc: document RZ/G3S SoC soc: renesas: identify RZ/G3S SoC soc: renesas: remove blank lines clk: renesas: rzg2l: wait for status bit of SD mux before continuing clk: renesas: rzg2l: lock around writes to mux register clk: renesas: rzg2l: trust value returned by hardware clk: renesas: rzg2l: fix computation formula clk: renesas: rzg2l: use core->name for clock name clk: renesas: rzg2l: simplify a bit the logic in rzg2l_mod_clock_endisable() clk: renesas: rzg2l: reduce the critical area clk: renesas: rzg2l: use FIELD_GET() for PLL register fields clk: renesas: rzg2l: use u32 for flag and mux_flags clk: renesas: rzg2l: add support for RZ/G3S PLL clk: renesas: rzg2l: add struct clk_hw_data clk: renesas: rzg2l: remove CPG_SDHI_DSEL from generic header clk: renesas: rzg2l: refactor sd mux driver clk: renesas: rzg2l: add a divider clock for RZ/G3S dt-bindings: clock: renesas,rzg2l-cpg: document RZ/G3S SoC dt-bindings: clock: add r9a08g045 CPG clocks and resets definitions clk: renesas: add minimal boot support for RZ/G3S SoC pinctrl: renesas: rzg2l: index all registers based on port offset pinctrl: renesas: rzg2l: adapt for different SD/PWPR register offsets pinctrl: renesas: rzg2l: adapt function number for RZ/G3S pinctrl: renesas: rzg2l: move ds and oi to SoC specific configuration pinctrl: renesas: rzg2l: add support for different ds values on different groups pinctrl: renesas: rzg2l: make struct rzg2l_pinctrl_data::dedicated_pins constant dt-bindings: pinctrl: renesas: document RZ/G3S SoC pinctrl: renesas: rzg2l: add support for RZ/G3S SoC dt-bindings: mmc: renesas,sdhi: Document RZ/G3S support arm64: dts: renesas: add initial DTSI for RZ/G3S SoC arm64: dts: renesas: rzg3l-smarc-som: add initial support for RZ/G3S SMARC Carrier-II SoM arm64: dts: renesas: rzg3s-smarc: add initial device tree for RZ SMARC Carrier-II Board dt-bindings: arm: renesas: document SMARC Carrier-II EVK arm64: dts: renesas: r9a08g045s33-smarc: add initial device tree for RZ/G3S SMARC EVK board arm64: defconfig: enable RZ/G3S (R9A08G045) SoC .../bindings/clock/renesas,rzg2l-cpg.yaml | 1 + .../devicetree/bindings/mmc/renesas,sdhi.yaml | 2 + .../pinctrl/renesas,rzg2l-pinctrl.yaml | 26 +- .../bindings/serial/renesas,scif.yaml | 1 + .../soc/renesas/renesas,rzg2l-sysc.yaml | 1 + .../bindings/soc/renesas/renesas.yaml | 8 + arch/arm64/boot/dts/renesas/Makefile | 2 + arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 139 ++++ .../boot/dts/renesas/r9a08g045s33-smarc.dts | 17 + arch/arm64/boot/dts/renesas/r9a08g045s33.dtsi | 14 + .../boot/dts/renesas/rzg3s-smarc-som.dtsi | 147 ++++ arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 28 + arch/arm64/configs/defconfig | 1 + drivers/clk/renesas/Kconfig | 7 +- drivers/clk/renesas/Makefile | 1 + drivers/clk/renesas/r9a07g043-cpg.c | 19 +- drivers/clk/renesas/r9a07g044-cpg.c | 19 +- drivers/clk/renesas/r9a08g045-cpg.c | 217 ++++++ drivers/clk/renesas/rzg2l-cpg.c | 495 ++++++++++-- drivers/clk/renesas/rzg2l-cpg.h | 39 +- drivers/pinctrl/renesas/pinctrl-rzg2l.c | 728 ++++++++++++++---- drivers/soc/renesas/Kconfig | 6 + drivers/soc/renesas/renesas-soc.c | 15 +- include/dt-bindings/clock/r9a08g045-cpg.h | 243 ++++++ 24 files changed, 1924 insertions(+), 252 deletions(-) create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045.dtsi create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045s33-smarc.dts create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045s33.dtsi create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi create mode 100644 drivers/clk/renesas/r9a08g045-cpg.c create mode 100644 include/dt-bindings/clock/r9a08g045-cpg.h