| Message ID | 20220505193143.31826-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add CPG wrapper for Renesas RZ/Five SoC | expand |
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@bp.renesas.com> > --- > 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?? > + > + 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
> 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 <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > 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
Hi Biju, Thank you for the review. On Thu, May 5, 2022 at 8:48 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > > --- > > 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 >
On Fri, May 6, 2022 at 7:17 AM Biju Das <biju.das.jz@bp.renesas.com> 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 <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > 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 >
Hi Prabhakar, > Subject: Re: [RFC PATCH 2/4] clk: renesas: rzg2l-cpg: Add support to stack > the resets instead of indexing > > Hi Biju, > > Thank you for the review. > > On Thu, May 5, 2022 at 8:48 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > > > --- > > > 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. As per [1], it will be 25 resets. if you invalidate RZ/G2L specific resets in probe, there is no runtime overhead. when a device match found, the info->reset_callback() which is mentioned in the next mail and invalidate the resets(resets[id].off = 0) ie, if(info->reset_callback) info->reset_callback(); and on r9a07g043-cpg.c, make resets[id].off = 0 to invalidate the resets. https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tree/include/dt-bindings/clock/r9a07g043-cpg.h > > > > > > + > > > + 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) Not now, But eventually we will fill all the resets, that is reason it is declared in header file. Cheers, Biju > > > > > u16 off; > > > u8 bit; > > > }; > > > > > > > > #define DEF_RST(_id, _off, _bit) \ > > > - [_id] = { \ > > > + { \ > > > + .id = (_id), \ > > > .off = (_off), \ > > > .bit = (_bit) \ > > > } > > > -- > > > 2.25.1 > >
Hi Biju, On Fri, May 6, 2022 at 1:11 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > Subject: Re: [RFC PATCH 2/4] clk: renesas: rzg2l-cpg: Add support to stack > > the resets instead of indexing > > > > Hi Biju, > > > > Thank you for the review. > > > > On Thu, May 5, 2022 at 8:48 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > > > > --- > > > > 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. > > As per [1], it will be 25 resets. > > if you invalidate RZ/G2L specific resets in probe, there is no runtime overhead. > when a device match found, the info->reset_callback() which is mentioned in the next mail > and invalidate the resets(resets[id].off = 0) > Ahh right got that. I'll wait for Geert if he has more cunning ideas. If not I'll go with your suggested approach. > ie, > > if(info->reset_callback) > info->reset_callback(); > > and on r9a07g043-cpg.c, make resets[id].off = 0 to invalidate the resets. > OK. > https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tree/include/dt-bindings/clock/r9a07g043-cpg.h > > Cheers, Prabhakar
Hi Prabhakar, On Thu, May 5, 2022 at 9:32 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > 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@bp.renesas.com> Thanks for your patch! An obvious alternative would be to allocate an array with pointers to the individual resets, like is done for clocks. Please see the suggestion in my reply to "[RFC PATCH 3/4] clk: renesas: r9a07g043: Split up core, module and resets array", which would make this patch unnecessary. 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
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]; + } + + 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; u16 off; u8 bit; }; #define DEF_RST(_id, _off, _bit) \ - [_id] = { \ + { \ + .id = (_id), \ .off = (_off), \ .bit = (_bit) \ }
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@bp.renesas.com> --- drivers/clk/renesas/rzg2l-cpg.c | 76 ++++++++++++++++++++++++++------- drivers/clk/renesas/rzg2l-cpg.h | 4 +- 2 files changed, 63 insertions(+), 17 deletions(-)