Hi Geert-san,
Thank you for your review!
> From: Geert Uytterhoeven, Sent: Wednesday, November 24, 2021 10:48 PM
>
> Hi Shimoda-san,
>
> Thanks for your patch!
>
> On Tue, Nov 16, 2021 at 8:42 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > Initial support for R-Car S4-8 (r8a779f0), including core, module
> > clocks, resets, and register access, because register specification
> > differs from R-Car Gen2/3. The register layout of V3U is a similar
> > with R-Car S4-8 so that renames CLK_REG_LAYOUT_RCAR_V3U as
> > CLK_REG_LAYOUT_RCAR_GEN4. However, PLL names differ between V3U
> > and S4-8.
>
> This is a small difference, so I think more code can be shared between
> R-Car V3U and S4-8 (see below).
I got it.
> > Inspired by patches in the BSP by LUU HOAI.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> > drivers/clk/renesas/Kconfig | 10 ++
> > drivers/clk/renesas/Makefile | 2 +
> > drivers/clk/renesas/r8a779a0-cpg-mssr.c | 2 +-
> > drivers/clk/renesas/r8a779f0-cpg-mssr.c | 188 ++++++++++++++++++++++++
> > drivers/clk/renesas/rcar-gen4-cpg.c | 141 ++++++++++++++++++
> > drivers/clk/renesas/rcar-gen4-cpg.h | 76 ++++++++++
> > drivers/clk/renesas/renesas-cpg-mssr.c | 42 ++++--
> > drivers/clk/renesas/renesas-cpg-mssr.h | 3 +-
> > 8 files changed, 448 insertions(+), 16 deletions(-)
> > create mode 100644 drivers/clk/renesas/r8a779f0-cpg-mssr.c
> > create mode 100644 drivers/clk/renesas/rcar-gen4-cpg.c
> > create mode 100644 drivers/clk/renesas/rcar-gen4-cpg.h
>
> Just like for the SYSC driver, I think it wouldn't hurt to split this
> patch in two parts: one patch to generalize r8a779a0-cpg-mssr.c for
> R-Car Gen4, and a second patch to introduce support for R-Car S4-8.
I got it. I'll split this patch in two parts in v2.
> > +static const struct cpg_core_clk r8a779f0_core_clks[] __initconst = {
> > + /* External Clock Inputs */
> > + DEF_INPUT("extal", CLK_EXTAL),
> > + DEF_INPUT("extalr", CLK_EXTALR),
> > +
> > + /* Internal Core Clocks */
> > + DEF_BASE(".main", CLK_MAIN, CLK_TYPE_GEN4_MAIN, CLK_EXTAL),
> > + DEF_BASE(".pll1", CLK_PLL1, CLK_TYPE_GEN4_PLL1, CLK_MAIN),
> > + DEF_BASE(".pll3", CLK_PLL3, CLK_TYPE_GEN4_PLL3, CLK_MAIN),
> > + DEF_BASE(".pll2", CLK_PLL2, CLK_TYPE_GEN4_PLL2, CLK_MAIN),
> > + DEF_BASE(".pll6", CLK_PLL6, CLK_TYPE_GEN4_PLL6, CLK_MAIN),
> > + DEF_BASE(".pll5", CLK_PLL5, CLK_TYPE_GEN4_PLL5, CLK_MAIN),
>
> Please sort PLLn by index.
Oops. I'll fix the order.
> > +
> > + DEF_FIXED(".pll1_div2", CLK_PLL1_DIV2, CLK_PLL1, 2, 1),
> > + DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 2, 1),
> > + DEF_FIXED(".pll3_div2", CLK_PLL3_DIV2, CLK_PLL3, 2, 1),
> > + DEF_FIXED(".pll5_div2", CLK_PLL5_DIV2, CLK_PLL5, 2, 1),
> > + DEF_FIXED(".pll5_div4", CLK_PLL5_DIV4, CLK_PLL5_DIV2, 2, 1),
> > + DEF_FIXED(".pll6_div2", CLK_PLL6_DIV2, CLK_PLL6, 2, 1),
> > + DEF_FIXED(".s0", CLK_S0, CLK_PLL1_DIV2, 2, 1),
> > + DEF_FIXED(".sdsrc", CLK_SDSRC, CLK_PLL5_DIV2, 2, 1),
>
> This relies on the default setting of the SD-IF0 Clock Frequency
> Control Register 1 (SD0CKCR1)?
You're correct. So, we should not use DEF_FIXED for SDSRC...
> > + DEF_RATE(".oco", CLK_OCO, 32768),
> > +
> > + DEF_BASE(".rpcsrc", CLK_RPCSRC, CLK_TYPE_GEN4_RPCSRC, CLK_PLL5),
> > + DEF_BASE(".rpc", R8A779F0_CLK_RPC, CLK_TYPE_GEN4_RPC, CLK_RPCSRC),
> > + DEF_BASE("rpcd2", R8A779F0_CLK_RPCD2, CLK_TYPE_GEN4_RPCD2, R8A779F0_CLK_RPC),
> > +
> > + /* Core Clock Outputs */
> > + DEF_FIXED("s0d2", R8A779F0_CLK_S0D2, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3", R8A779F0_CLK_S0D3, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d4", R8A779F0_CLK_S0D4, CLK_S0, 4, 1),
> > + DEF_FIXED("cl16m", R8A779F0_CLK_CL16M, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d2_mm", R8A779F0_CLK_S0D2_MM, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3_mm", R8A779F0_CLK_S0D3_MM, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d4_mm", R8A779F0_CLK_S0D4_MM, CLK_S0, 4, 1),
> > + DEF_FIXED("cl16m_mm", R8A779F0_CLK_CL16M_MM, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d2_rt", R8A779F0_CLK_S0D2_RT, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3_rt", R8A779F0_CLK_S0D3_RT, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d4_rt", R8A779F0_CLK_S0D4_RT, CLK_S0, 4, 1),
> > + DEF_FIXED("s0d6_rt", R8A779F0_CLK_S0D6_RT, CLK_S0, 6, 1),
> > + DEF_FIXED("cl16m_rt", R8A779F0_CLK_CL16M_RT, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d3_per", R8A779F0_CLK_S0D3_PER, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d6_per", R8A779F0_CLK_S0D6_PER, CLK_S0, 6, 1),
> > + DEF_FIXED("s0d12_per", R8A779F0_CLK_S0D12_PER, CLK_S0, 12, 1),
> > + DEF_FIXED("s0d24_per", R8A779F0_CLK_S0D24_PER, CLK_S0, 24, 1),
> > + DEF_FIXED("cl16m_per", R8A779F0_CLK_CL16M_PER, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d2_hsc", R8A779F0_CLK_S0D2_HSC, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3_hsc", R8A779F0_CLK_S0D3_HSC, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d4_hsc", R8A779F0_CLK_S0D4_HSC, CLK_S0, 4, 1),
> > + DEF_FIXED("s0d6_hsc", R8A779F0_CLK_S0D6_HSC, CLK_S0, 6, 1),
> > + DEF_FIXED("s0d12_hsc", R8A779F0_CLK_S0D12_HSC, CLK_S0, 12, 1),
> > + DEF_FIXED("cl16m_hsc", R8A779F0_CLK_CL16M_HSC, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d2_cc", R8A779F0_CLK_S0D2_CC, CLK_S0, 2, 1),
> > + DEF_FIXED("rsw", R8A779F0_CLK_RSW2, CLK_PLL5, 2, 1),
>
> "rsw2"?
Yes. I'll fix it.
> > + DEF_FIXED("cbfusa", R8A779F0_CLK_CBFUSA, CLK_EXTAL, 2, 1),
> > + DEF_FIXED("cpex", R8A779F0_CLK_CPEX, CLK_EXTAL, 2, 1),
> > +
> > + DEF_GEN4_SD("sd0", R8A779F0_CLK_SD0, CLK_SDSRC, 0x870),
> > + DEF_DIV6P1("mso", R8A779F0_CLK_MSO, CLK_PLL5_DIV4, 0x087C),
>
> 0x87c
Oops. I'll fix it.
> > +
> > + DEF_GEN4_OSC("osc", R8A779F0_CLK_OSC, CLK_EXTAL, 8),
> > + DEF_GEN4_MDSEL("r", R8A779F0_CLK_R, 29, CLK_EXTALR, 1, CLK_OCO, 1),
> > +};
> > +
> > +static const struct mssr_mod_clk r8a779f0_mod_clks[] __initconst = {
> > + DEF_MOD("scif0", 702, R8A779F0_CLK_S0D12_PER),
> > + DEF_MOD("scif1", 703, R8A779F0_CLK_S0D12_PER),
> > + DEF_MOD("scif3", 704, R8A779F0_CLK_S0D12_PER),
> > + DEF_MOD("scif4", 705, R8A779F0_CLK_S0D12_PER),
> > +};
> > +
> > +/*
> > + * CPG Clock Data
> > + */
> > +/*
> > + * MD EXTAL PLL1 PLL2 PLL3 PLL5 PLL6 OSC
> > + * 14 13 (MHz)
> > + * ----------------------------------------------------------------
> > + * 0 0 16.66 / 1 x200 x150 x200 x200 x134 /15
>
> EXTAL is 16 MHz?
Oops. You're correct. I'll fix it.
> > + * 0 1 20 / 1 x160 x120 x160 x160 x106 /19
> > + * 1 0 Prohibited setting
> > + * 1 1 40 / 2 x160 x120 x160 x160 x106 /38
> > + */
> > +#define CPG_PLL_CONFIG_INDEX(md) ((((md) & BIT(14)) >> 13) | \
> > + (((md) & BIT(13)) >> 13))
> > +
> > +static const struct rcar_gen4_cpg_pll_config cpg_pll_configs[4] = {
> > + /* EXTAL div PLL1 mult/div PLL2 mult/div PLL3 mult/div PLL5 mult/div PLL6 mult/div OSC prediv */
> > + { 1, 200, 1, 150, 1, 200, 1, 200, 1, 134, 1, 16, },
>
> OSC prediv is 15?
Thank you for the indicate. I'll fix it.
> > + { 1, 160, 1, 120, 1, 160, 1, 160, 1, 106, 1, 19, },
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, },
> > + { 2, 160, 1, 120, 1, 160, 1, 160, 1, 106, 1, 38, },
> > +};
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rcar-gen4-cpg.c
> > @@ -0,0 +1,141 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * R-Car Gen4 Clock Pulse Generator
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + *
> > + * Based on rcar-gen3-cpg.c
> > + *
> > + * Copyright (C) 2015-2018 Glider bvba
> > + * Copyright (C) 2019 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/bug.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/sys_soc.h>
>
> Several of these includes are not needed.
I got it. I'll remove unnecessary header files.
> > +
> > +#include "renesas-cpg-mssr.h"
> > +#include "rcar-gen4-cpg.h"
> > +#include "rcar-cpg-lib.h"
> > +
> > +static const struct clk_div_table cpg_rpcsrc_div_table[] = {
> > + { 2, 5 }, { 3, 6 }, { 0, 0 },
>
> The datasheet says { 0, 4 } and { 1, 6} are also supported,
> just like on R-Car V3U.
I got it.
> > +};
>
> > +struct clk * __init rcar_gen4_cpg_clk_register(struct device *dev,
> > + const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> > + struct clk **clks, void __iomem *base,
> > + struct raw_notifier_head *notifiers)
> > +{
> > + const struct clk *parent;
> > + unsigned int mult = 1;
> > + unsigned int div = 1;
> > +
> > + parent = clks[core->parent & 0xffff]; /* some types use high bits */
> > + if (IS_ERR(parent))
> > + return ERR_CAST(parent);
> > +
> > + switch (core->type) {
> > + case CLK_TYPE_GEN4_MAIN:
> > + div = cpg_pll_config->extal_div;
> > + break;
> > +
> > + case CLK_TYPE_GEN4_PLL1:
> > + mult = cpg_pll_config->pll1_mult;
> > + div = cpg_pll_config->pll1_div;
> > + break;
> > +
> > + case CLK_TYPE_GEN4_PLL2:
> > + mult = cpg_pll_config->pll2_mult;
> > + div = cpg_pll_config->pll2_div;
> > + break;
> > +
> > + case CLK_TYPE_GEN4_PLL3:
> > + mult = cpg_pll_config->pll3_mult;
> > + div = cpg_pll_config->pll3_div;
> > + break;
> > +
> > + case CLK_TYPE_GEN4_PLL5:
> > + mult = cpg_pll_config->pll5_mult;
> > + div = cpg_pll_config->pll5_div;
> > + break;
> > +
> > + case CLK_TYPE_GEN4_PLL6:
> > + mult = cpg_pll_config->pll6_mult;
> > + div = cpg_pll_config->pll6_div;
> > + break;
>
> The Z clock handling for R-Car S4-8 seems to be the same as for R-Car
> V3U, so you can move that here, too.
>
> That leaves us with the different PLLn handling:
> 1. I think you can just move the PLL2X_3C clock type for R-Car
> V3U here, too. It's only 4 lines of code. Then rcar-gen4-cpg.c
> can be shared by R-Car V3U and R-Car S4-8.
I got it. I'll modify it.
> 2. Future full PLLn handling for R-Car S4-8 (using the PLLnCR1
> registers) will need switching from CLK_TYPE_GEN4_PLLn to a
> new clock type anyway.
I got it.
> > + case CLK_TYPE_GEN4_SD:
> > + return cpg_sd_clk_register(core->name, base, core->offset,
> > + __clk_get_name(parent), notifiers,
> > + 0);
>
> This should be changed to:
>
> return cpg_sd_clk_register(core->name, base + core->offset,
> __clk_get_name(parent));
>
> due to the recent changes in renesas-clk to handle the SDH clock.
Thank you for the comment. I'll rebase it on the recent renesas-clk.
> > +
> > + default:
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + return clk_register_fixed_factor(NULL, core->name,
> > + __clk_get_name(parent), 0, mult, div);
> > +}
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rcar-gen4-cpg.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * R-Car Gen4 Clock Pulse Generator
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + *
> > + */
> > +
> > +#ifndef __CLK_RENESAS_RCAR_GEN4_CPG_H__
> > +#define __CLK_RENESAS_RCAR_GEN4_CPG_H__
> > +
> > +enum rcar_gen4_clk_types {
> > + CLK_TYPE_GEN4_MAIN = CLK_TYPE_CUSTOM,
> > + CLK_TYPE_GEN4_PLL1,
> > + CLK_TYPE_GEN4_PLL2,
> > + CLK_TYPE_GEN4_PLL3,
> > + CLK_TYPE_GEN4_PLL5,
> > + CLK_TYPE_GEN4_PLL6,
> > + CLK_TYPE_GEN4_SD,
> > + CLK_TYPE_GEN4_R,
>
> So far unused.
I'll remove CLK_TYPE_GEN4_R.
> > + CLK_TYPE_GEN4_MDSEL, /* Select parent/divider using mode pin */
> > + CLK_TYPE_GEN4_Z,
> > + CLK_TYPE_GEN4_ZG,
>
> Both unused.
I'll remove CLK_TYPE_GEN4_Z[G].
> > + CLK_TYPE_GEN4_OSC, /* OSC EXTAL predivider and fixed divider */
> > + CLK_TYPE_GEN4_RPCSRC,
> > + CLK_TYPE_GEN4_RPC,
> > + CLK_TYPE_GEN4_RPCD2,
> > +
> > + /* SoC specific definitions start here */
> > + CLK_TYPE_GEN4_SOC_BASE,
> > +};
> > +
> > +#define DEF_GEN4_SD(_name, _id, _parent, _offset) \
> > + DEF_BASE(_name, _id, CLK_TYPE_GEN4_SD, _parent, .offset = _offset)
> > +
> > +#define DEF_GEN4_MDSEL(_name, _id, _md, _parent0, _div0, _parent1, _div1) \
> > + DEF_BASE(_name, _id, CLK_TYPE_GEN4_MDSEL, \
> > + (_parent0) << 16 | (_parent1), \
> > + .div = (_div0) << 16 | (_div1), .offset = _md)
> > +
> > +#define DEF_GEN4_PE(_name, _id, _parent_clean, _div_clean, _parent_sscg, \
> > + _div_sscg) \
> > + DEF_GEN4_MDSEL(_name, _id, 12, _parent_clean, _div_clean, \
> > + _parent_sscg, _div_sscg)
>
> R-Car S4 does not have a PE clock, so please drop this.
I got it.
Best regards,
Yoshihiro Shimoda
