Re: [RFC PATCH 2/4] clk: renesas: rzg2l-cpg: Add support to stack the resets instead of indexing

[Lad, Prabhakar]

Hi Biju,

Thank you for the review.

On Thu, May 5, 2022 at 8:48 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
>
> Hi Lad Prabhakar,
>
> Thanks for the patch.
>
> > Subject: [RFC PATCH 2/4] clk: renesas: rzg2l-cpg: Add support to stack the
> > resets instead of indexing
> >
> > Instead of indexing the resets, stack them and instead create an id member
> > in struct rzg2l_reset to store the index. With this approach for every id
> > we will have to loop through the resets array to match the id.
> >
> > This in preparation to add support for Renesas RZ/Five CPG in r9a07g043-
> > cpg.c file where the resets array will be split up into three i.e. common
> > and two SoC specific.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> >  drivers/clk/renesas/rzg2l-cpg.c | 76 ++++++++++++++++++++++++++-------
> > drivers/clk/renesas/rzg2l-cpg.h |  4 +-
> >  2 files changed, 63 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-
> > cpg.c index 1ce35f65682b..94fe307ec4c5 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -681,14 +681,37 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk
> > *mod,
> >
> >  #define rcdev_to_priv(x)     container_of(x, struct rzg2l_cpg_priv,
> > rcdev)
> >
> > +static const struct rzg2l_reset
> > +*rzg2l_get_reset_ptr(struct rzg2l_cpg_priv *priv,
> > +                  unsigned long id)
> > +
> > +{
> > +     const struct rzg2l_cpg_info *info = priv->info;
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < priv->num_resets; i++) {
> > +             if (info->resets[i].id == id)
> > +                     return &info->resets[i];
> > +     }
>
> Is it not possible to use shared reset like RZ/G2L and RZ/V2L?, which
> has optimal memory and performance wise we can avoid bigger loop.
>
> Like adding Last index of RZ/Five as last reset index and
> Handle RZ/G2UL specific as invalid reset index in xlate??
>
So we will have to maintain an array id's which are invalid to RZ/Five
SoC. For this too we will have to loop at runtime itself. The array
for invalid index will be big too.

>
> > +
> > +     return NULL;
> > +}
> > +
> >  static int rzg2l_cpg_reset(struct reset_controller_dev *rcdev,
> >                          unsigned long id)
> >  {
> >       struct rzg2l_cpg_priv *priv = rcdev_to_priv(rcdev);
> > -     const struct rzg2l_cpg_info *info = priv->info;
> > -     unsigned int reg = info->resets[id].off;
> > -     u32 dis = BIT(info->resets[id].bit);
> > -     u32 we = dis << 16;
> > +     const struct rzg2l_reset *reset;
> > +     unsigned int reg;
> > +     u32 dis, we;
> > +
> > +     reset = rzg2l_get_reset_ptr(priv, id);
> > +     if (!reset)
> > +             return -EINVAL;
> > +
> > +     reg = reset->off;
> > +     dis = BIT(reset->bit);
> > +     we = dis << 16;
> >
> >       dev_dbg(rcdev->dev, "reset id:%ld offset:0x%x\n", id,
> > CLK_RST_R(reg));
> >
> > @@ -708,9 +731,16 @@ static int rzg2l_cpg_assert(struct
> > reset_controller_dev *rcdev,
> >                           unsigned long id)
> >  {
> >       struct rzg2l_cpg_priv *priv = rcdev_to_priv(rcdev);
> > -     const struct rzg2l_cpg_info *info = priv->info;
> > -     unsigned int reg = info->resets[id].off;
> > -     u32 value = BIT(info->resets[id].bit) << 16;
> > +     const struct rzg2l_reset *reset;
> > +     unsigned int reg;
> > +     u32 value;
> > +
> > +     reset = rzg2l_get_reset_ptr(priv, id);
> > +     if (!reset)
> > +             return -EINVAL;
> > +
> > +     reg = reset->off;
> > +     value = BIT(reset->bit) << 16;
> >
> >       dev_dbg(rcdev->dev, "assert id:%ld offset:0x%x\n", id,
> > CLK_RST_R(reg));
> >
> > @@ -722,11 +752,17 @@ static int rzg2l_cpg_deassert(struct
> > reset_controller_dev *rcdev,
> >                             unsigned long id)
> >  {
> >       struct rzg2l_cpg_priv *priv = rcdev_to_priv(rcdev);
> > -     const struct rzg2l_cpg_info *info = priv->info;
> > -     unsigned int reg = info->resets[id].off;
> > -     u32 dis = BIT(info->resets[id].bit);
> > -     u32 value = (dis << 16) | dis;
> > +     const struct rzg2l_reset *reset;
> > +     unsigned int reg;
> > +     u32 dis, value;
> > +
> > +     reset = rzg2l_get_reset_ptr(priv, id);
> > +     if (!reset)
> > +             return -EINVAL;
> >
> > +     reg = reset->off;
> > +     dis = BIT(reset->bit);
> > +     value = (dis << 16) | dis;
> >       dev_dbg(rcdev->dev, "deassert id:%ld offset:0x%x\n", id,
> >               CLK_RST_R(reg));
> >
> > @@ -738,9 +774,16 @@ static int rzg2l_cpg_status(struct
> > reset_controller_dev *rcdev,
> >                           unsigned long id)
> >  {
> >       struct rzg2l_cpg_priv *priv = rcdev_to_priv(rcdev);
> > -     const struct rzg2l_cpg_info *info = priv->info;
> > -     unsigned int reg = info->resets[id].off;
> > -     u32 bitmask = BIT(info->resets[id].bit);
> > +     const struct rzg2l_reset *reset;
> > +     unsigned int reg;
> > +     u32 bitmask;
> > +
> > +     reset = rzg2l_get_reset_ptr(priv, id);
> > +     if (!reset)
> > +             return -EINVAL;
> > +
> > +     reg = reset->off;
> > +     bitmask = BIT(reset->bit);
> >
> >       return !(readl(priv->base + CLK_MRST_R(reg)) & bitmask);  } @@ -
> > 756,10 +799,11 @@ static int rzg2l_cpg_reset_xlate(struct
> > reset_controller_dev *rcdev,
> >                                const struct of_phandle_args *reset_spec)  {
> >       struct rzg2l_cpg_priv *priv = rcdev_to_priv(rcdev);
> > -     const struct rzg2l_cpg_info *info = priv->info;
> >       unsigned int id = reset_spec->args[0];
> > +     const struct rzg2l_reset *reset;
> >
> > -     if (id >= rcdev->nr_resets || !info->resets[id].off) {
> > +     reset = rzg2l_get_reset_ptr(priv, id);
> > +     if (!reset) {
> >               dev_err(rcdev->dev, "Invalid reset index %u\n", id);
> >               return -EINVAL;
> >       }
> > diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-
> > cpg.h index 92c88f42ca7f..a99f2ba7868f 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.h
> > +++ b/drivers/clk/renesas/rzg2l-cpg.h
> > @@ -152,12 +152,14 @@ struct rzg2l_mod_clk {
> >   * @bit: reset bit
> >   */
> >  struct rzg2l_reset {
> > +     unsigned int id;
>
> Now you are adding 4 bytes to each reset entry in the LUT.
>
Agreed, on the other hand we are saving space on the entries (for
example not all the reset entries are listed in the array and the
array length will always be equal to last index)

Cheers,
Prabhakar

> Cheers,
> Biju
>
> >       u16 off;
> >       u8 bit;
> >  };
>
> >
> >  #define DEF_RST(_id, _off, _bit)     \
> > -     [_id] = { \
> > +     { \
> > +             .id = (_id), \
> >               .off = (_off), \
> >               .bit = (_bit) \
> >       }
> > --
> > 2.25.1
>