Hi Geert,
Thank you for the review.
On Wed, Jun 26, 2024 at 11:07 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Tue, Jun 11, 2024 at 1:32 AM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add family-specific clock driver for RZ/V2H(P) SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v1->v2
> > - Introduced family specific config option
> > - Now using register indexes for CLKON/CLKMON/RST/RSTMON
> > - Introduced PLL_CONF macro
> > - Dropped function pointer to get PLL_CLK1/2 offsets
> > - Added range check for core clks
> > - Dropped NULLified clocks check
> > - Updated commit description
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > +/**
> > + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> > + *
> > + * @rcdev: Reset controller entity
> > + * @dev: CPG device
> > + * @base: CPG register block base address
> > + * @clks: Array containing all Core and Module Clocks
> > + * @num_core_clks: Number of Core Clocks in clks[]
> > + * @num_mod_clks: Number of Module Clocks in clks[]
> > + * @num_resets: Number of Module Resets in info->resets[]
> > + * @num_hw_resets: Number of resets supported by HW
> > + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> > + * @info: Pointer to platform data
> > + */
> > +struct rzv2h_cpg_priv {
> > + struct reset_controller_dev rcdev;
> > + struct device *dev;
> > + void __iomem *base;
> > +
> > + struct clk **clks;
> > + unsigned int num_core_clks;
> > + unsigned int num_mod_clks;
> > + unsigned int num_resets;
> > + unsigned int num_hw_resets;
>
> This is not really used, so please drop it.
>
OK, I will drop it.
> > + unsigned int last_dt_core_clk;
> > +
> > + const struct rzv2h_cpg_info *info;
> > +};
>
> > +static struct clk
> > +*rzv2h_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
> > + void *data)
> > +{
> > + unsigned int clkidx = clkspec->args[1];
> > + struct rzv2h_cpg_priv *priv = data;
> > + struct device *dev = priv->dev;
> > + const char *type;
> > + int range_check;
> > + struct clk *clk;
> > +
> > + switch (clkspec->args[0]) {
> > + case CPG_CORE:
> > + type = "core";
> > + if (clkidx > priv->last_dt_core_clk) {
> > + dev_err(dev, "Invalid %s clock index %u\n", type, clkidx);
> > + return ERR_PTR(-EINVAL);
> > + }
> > + clk = priv->clks[clkidx];
> > + break;
> > +
> > + case CPG_MOD:
> > + type = "module";
> > + range_check = 15 - (clkidx % 16);
> > + if (range_check < 0 || clkidx >= priv->num_mod_clks) {
>
> range_check is never negative
> (leftover from sparse number space?)
>
Agreed (we are doing % 16). I will drop this check.
> > + dev_err(dev, "Invalid %s clock index %u\n", type,
> > + clkidx);
> > + return ERR_PTR(-EINVAL);
> > + }
> > + clk = priv->clks[priv->num_core_clks + clkidx];
> > + break;
> > +
> > + default:
> > + dev_err(dev, "Invalid CPG clock type %u\n", clkspec->args[0]);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (IS_ERR(clk))
> > + dev_err(dev, "Cannot get %s clock %u: %ld", type, clkidx,
> > + PTR_ERR(clk));
> > + else
> > + dev_dbg(dev, "clock (%u, %u) is %pC at %lu Hz\n",
> > + clkspec->args[0], clkspec->args[1], clk,
> > + clk_get_rate(clk));
> > + return clk;
> > +}
>
> > +/**
> > + * struct mod_clock - Module clock
> > + *
> > + * @hw: handle between common and hardware-specific interfaces
> > + * @off: register offset
> > + * @bit: ON/MON bit
> > + * @monoff: monitor register offset
> > + * @monbit: montor bit
> > + * @priv: CPG private data
> > + */
> > +struct mod_clock {
> > + struct clk_hw hw;
> > + u8 on_index;
> > + u8 on_bit;
> > + u16 mon_index;
> > + u8 mon_bit;
>
> I noticed clock on and clock mon bits are related.
> Clock on bits use only the lower 16 bits in a register, while clock
> monitor bits use all 32 bits, hence:
>
> mon_index = on_index / 2
> mon_bit = (on_index % 2) * 16 + on_bit
>
> Except for clocks without monitor bits, and for CGC_SPI_clk_spi and
> CGC_SPI_clk_spix2, which share an on-bit, but have separate mon-bits.
> So you cannot use these formulas.
>
Ahh we could have saved some memory!
> Reset bits do not have such a relationship, as resets marked reserved
> are skipped in the reset monitoring bit range.
>
Yep.
>
> > + struct rzv2h_cpg_priv *priv;
> > +};
> > +
> > +#define to_mod_clock(_hw) container_of(_hw, struct mod_clock, hw)
> > +
> > +static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable)
> > +{
> > + struct mod_clock *clock = to_mod_clock(hw);
> > + unsigned int reg = GET_CLK_ON_OFFSET(clock->on_index);
> > + struct rzv2h_cpg_priv *priv = clock->priv;
> > + u32 bitmask = BIT(clock->on_bit);
> > + struct device *dev = priv->dev;
> > + u32 value;
> > + int error;
> > +
> > + dev_dbg(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk,
> > + enable ? "ON" : "OFF");
> > +
> > + value = bitmask << 16;
> > + if (enable)
> > + value |= bitmask;
> > +
> > + writel(value, priv->base + reg);
> > +
> > + if (!enable)
> > + return 0;
> > +
> > + reg = GET_CLK_MON_OFFSET(clock->mon_index);
>
> What if a clock does not have a clock monitor bit?
> Clock bits in registers CPG_CLKON_22 and later do not have corresponding
> clock monitor bits.
>
Oops I had missed this case.
I'll introduce a macro (NO_MON_REG_INDEX) for clocks which do not have
monitor support and add a check above to skip clk monitor operation if
clock->mon_index == NO_MON_REG_INDEX.
/* monitor index for clocks which do not have CLKMON support */
#define NO_MON_REG_INDEX 0xff
Does this sound OK?
> > + bitmask = BIT(clock->mon_bit);
> > + error = readl_poll_timeout_atomic(priv->base + reg, value,
> > + value & bitmask, 0, 10);
> > + if (error)
> > + dev_err(dev, "Failed to enable CLK_ON %p\n",
> > + priv->base + reg);
> > +
> > + return error;
> > +}
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
>
> > +/**
> > + * struct rzv2h_cpg_info - SoC-specific CPG Description
> > + *
> > + * @core_clks: Array of Core Clock definitions
> > + * @num_core_clks: Number of entries in core_clks[]
> > + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> > + * @num_total_core_clks: Total number of Core Clocks (exported + internal)
> > + *
> > + * @mod_clks: Array of Module Clock definitions
> > + * @num_mod_clks: Number of entries in mod_clks[]
> > + * @num_hw_mod_clks: Number of Module Clocks supported by the hardware
> > + *
> > + * @resets: Array of Module Reset definitions
> > + * @num_resets: Number of entries in resets[]
> > + * @num_hw_resets: Number of resets supported by the hardware
> > + *
> > + * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
> > + * should not be disabled without a knowledgeable driver
> > + * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
> > + */
> > +struct rzv2h_cpg_info {
> > + /* Core Clocks */
> > + const struct cpg_core_clk *core_clks;
> > + unsigned int num_core_clks;
> > + unsigned int last_dt_core_clk;
> > + unsigned int num_total_core_clks;
> > +
> > + /* Module Clocks */
> > + const struct rzv2h_mod_clk *mod_clks;
> > + unsigned int num_mod_clks;
> > + unsigned int num_hw_mod_clks;
> > +
> > + /* Resets */
> > + const struct rzv2h_reset *resets;
> > + unsigned int num_resets;
> > + unsigned int num_hw_resets;
>
> This is not really used, so please drop it.
>
OK.
Cheers,
Prabhakar
