> Subject: RE: [RFC PATCH 2/4] clk: renesas: rzg2l-cpg: Add support to stack
> the resets instead of indexing
>
> 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 <[email protected]>
> > ---
> > 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??
Or add a invalidate_resets() callback to info structure and in probe, call this
Callback,if present to invalidate the resets which are not specific to this SoC.
>
>
> > +
> > + 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.
>
> Cheers,
> Biju
>
> > u16 off;
> > u8 bit;
> > };
>
> >
> > #define DEF_RST(_id, _off, _bit) \
> > - [_id] = { \
> > + { \
> > + .id = (_id), \
> > .off = (_off), \
> > .bit = (_bit) \
> > }
> > --
> > 2.25.1
On Fri, May 6, 2022 at 7:17 AM Biju Das <[email protected]> wrote:
>
> > Subject: RE: [RFC PATCH 2/4] clk: renesas: rzg2l-cpg: Add support to stack
> > the resets instead of indexing
> >
> > 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 <[email protected]>
> > > ---
> > > 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??
>
> Or add a invalidate_resets() callback to info structure and in probe, call this
> Callback,if present to invalidate the resets which are not specific to this SoC.
>
Could you please elaborate on this.
Cheers,
Prabhakar
>
> >
> >
> > > +
> > > + 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.
> >
> > Cheers,
> > Biju
> >
> > > u16 off;
> > > u8 bit;
> > > };
> >
> > >
> > > #define DEF_RST(_id, _off, _bit) \
> > > - [_id] = { \
> > > + { \
> > > + .id = (_id), \
> > > .off = (_off), \
> > > .bit = (_bit) \
> > > }
> > > --
> > > 2.25.1
>