Re: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

Hi Laurent, Biju, Prabhakar,

Thanks all for the inputs!

On Wed, Apr 09, 2025 at 07:25:43AM +0000, Biju Das wrote:
> Hi Laurent,
> 
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: 09 April 2025 02:29
> > Subject: Re: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support
> > 
> > On Mon, Apr 07, 2025 at 04:55:33PM +0000, Lad, Prabhakar wrote:
> > > On Wed, Apr 2, 2025 at 10:39 AM Biju Das wrote:
> > > > On 02 April 2025 10:26, Laurent Pinchart wrote:
> > > > > On Wed, Apr 02, 2025 at 08:25:06AM +0000, Lad, Prabhakar wrote:
> > > > > > On Wed, Apr 2, 2025 at 9:20 AM Biju Das wrote:
> > > > > > > On 02 April 2025 08:35, Lad, Prabhakar wrote:
> > > > > > > > On Wed, Apr 2, 2025 at 7:31 AM Biju Das wrote:
> > > > > > > > > > On 28 March 2025 17:30, Tommaso Merciai wrote:
> > > > > > > > > > From: Lad Prabhakar
> > > > > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > > > >
> > > > > > > > > > Prepare for adding support for RZ/G3E and RZ/V2HP SoCs,
> > > > > > > > > > which have a CRU-IP that is mostly identical to RZ/G2L
> > > > > > > > > > but with different register offsets and additional
> > > > > > > > > > registers. Introduce a flexible register mapping
> > > > > > > > > > mechanism to handle these variations.
> > > > > > > > > >
> > > > > > > > > > Define the `rzg2l_cru_info` structure to store register
> > > > > > > > > > mappings and pass it as part of the OF match data.
> > > > > > > > > > Update the read/write functions to check out-of-bound
> > > > > > > > > > accesses and use indexed register offsets from
> > > > > > > > > > `rzg2l_cru_info`, ensuring compatibility across different SoC variants.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Lad Prabhakar
> > > > > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > > > > Signed-off-by: Tommaso Merciai
> > > > > > > > > > <tommaso.merciai.xr@bp.renesas.com>
> > > > > > > > > > ---
> > > > > > > > > > Changes since v2:
> > > > > > > > > >  - Implemented new rzg2l_cru_write/read() that now are checking out-of-bound
> > > > > > > > > >    accesses as suggested by LPinchart.
> > > > > > > > > >  - Fixed AMnMBxADDRL() and AMnMBxADDRH() as suggested by LPinchart.
> > > > > > > > > >  - Update commit body
> > > > > > > > > >
> > > > > > > > > > Changes since v4:
> > > > > > > > > >  - Mark __rzg2l_cru_write_constant/__rzg2l_cru_read_constant
> > > > > > > > > >    as __always_inline
> > > > > > > > > >
> > > > > > > > > >  .../platform/renesas/rzg2l-cru/rzg2l-core.c   | 46 ++++++++++++-
> > > > > > > > > >  .../renesas/rzg2l-cru/rzg2l-cru-regs.h        | 66 ++++++++++---------
> > > > > > > > > >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  4 ++
> > > > > > > > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 58
> > > > > > > > > > ++++++++++++++--
> > > > > > > > > >  4 files changed, 139 insertions(+), 35 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git
> > > > > > > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > > > index eed9d2bd08414..abc2a979833aa 100644
> > > > > > > > > > ---
> > > > > > > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cor
> > > > > > > > > > +++ e.c
> > > > > > > > > > @@ -22,6 +22,7 @@
> > > > > > > > > >  #include <media/v4l2-mc.h>
> > > > > > > > > >
> > > > > > > > > >  #include "rzg2l-cru.h"
> > > > > > > > > > +#include "rzg2l-cru-regs.h"
> > > > > > > > > >
> > > > > > > > > >  static inline struct rzg2l_cru_dev
> > > > > > > > > > *notifier_to_cru(struct v4l2_async_notifier *n)  { @@
> > > > > > > > > > -269,6 +270,9 @@ static int rzg2l_cru_probe(struct
> > > > > > > > > > platform_device *pdev)
> > > > > > > > > >
> > > > > > > > > >       cru->dev = dev;
> > > > > > > > > >       cru->info = of_device_get_match_data(dev);
> > > > > > > > > > +     if (!cru->info)
> > > > > > > > > > +             return dev_err_probe(dev, -EINVAL,
> > > > > > > > > > +                                  "Failed to get OF
> > > > > > > > > > + match data\n");
> > > > > > > > > >
> > > > > > > > > >       irq = platform_get_irq(pdev, 0);
> > > > > > > > > >       if (irq < 0)
> > > > > > > > > > @@ -317,8 +321,48 @@ static void rzg2l_cru_remove(struct platform_device *pdev)
> > > > > > > > > >       rzg2l_cru_dma_unregister(cru);  }
> > > > > > > > > >
> > > > > > > > > > +static const u16 rzg2l_cru_regs[] = {
> > > > > > > > > > +     [CRUnCTRL] = 0x0,
> > > > > > > > > > +     [CRUnIE] = 0x4,
> > > > > > > > > > +     [CRUnINTS] = 0x8,
> > > > > > > > > > +     [CRUnRST] = 0xc,
> > > > > > > > > > +     [AMnMB1ADDRL] = 0x100,
> > > > > > > > > > +     [AMnMB1ADDRH] = 0x104,
> > > > > > > > > > +     [AMnMB2ADDRL] = 0x108,
> > > > > > > > > > +     [AMnMB2ADDRH] = 0x10c,
> > > > > > > > > > +     [AMnMB3ADDRL] = 0x110,
> > > > > > > > > > +     [AMnMB3ADDRH] = 0x114,
> > > > > > > > > > +     [AMnMB4ADDRL] = 0x118,
> > > > > > > > > > +     [AMnMB4ADDRH] = 0x11c,
> > > > > > > > > > +     [AMnMB5ADDRL] = 0x120,
> > > > > > > > > > +     [AMnMB5ADDRH] = 0x124,
> > > > > > > > > > +     [AMnMB6ADDRL] = 0x128,
> > > > > > > > > > +     [AMnMB6ADDRH] = 0x12c,
> > > > > > > > > > +     [AMnMB7ADDRL] = 0x130,
> > > > > > > > > > +     [AMnMB7ADDRH] = 0x134,
> > > > > > > > > > +     [AMnMB8ADDRL] = 0x138,
> > > > > > > > > > +     [AMnMB8ADDRH] = 0x13c,
> > > > > > > > > > +     [AMnMBVALID] = 0x148,
> > > > > > > > > > +     [AMnMBS] = 0x14c,
> > > > > > > > > > +     [AMnAXIATTR] = 0x158,
> > > > > > > > > > +     [AMnFIFOPNTR] = 0x168,
> > > > > > > > > > +     [AMnAXISTP] = 0x174,
> > > > > > > > > > +     [AMnAXISTPACK] = 0x178,
> > > > > > > > > > +     [ICnEN] = 0x200,
> > > > > > > > > > +     [ICnMC] = 0x208,
> > > > > > > > > > +     [ICnMS] = 0x254,
> > > > > > > > > > +     [ICnDMR] = 0x26c,
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > Do we need enum, can't we use struct instead with all these entries instead?
> > > > > > > > >
> > > > > > > > What benefit do you foresee when using struct? With the
> > > > > > > > current approach being used a minimal diff is generated when
> > > > > > > > switched to struct there will be lots of changes.
> > > > > > >
> > > > > > > The mapping is convinient when you want to iterate throught it.
> > > > > > > Here, if you just want to access the offset value from its
> > > > > > > name, a structure looks more appropriate.
> > > > > >
> > > > > > Thanks, as this patch has been reviewed by Laurent a couple of
> > > > > > times we will change this to struct If he insists.
> > > > >
> > > > > How would a struct look like ? I'm not sure what is being proposed.
> > > >
> > > > It will be
> > > >
> > > > struct rzg2l_cru_regs {
> > > >         u16 cru_n_ctrl;
> > > >         u16 cru_n_ie;
> > > >         u16 cru_n_ints;
> > > >         u16 cru_n_rst;
> > > > };
> > > >
> > > > static const struct rzg2l_cru_regs rzg2l_cru_regs = {
> > > >         .cru_n_ctrl = 0x0,
> > > >         .cru_n_ie = 0x4,
> > > >         .cru_n_ints = 0x8,
> > > >         .cru_n_rst = 0xc,
> > > > };
> > > >
> > > > You can access it using info->regs->cru_n_ctrl instead of
> > > > info->regs[CRUnCTRL] This is proposal.
> > >
> > > Are you OK with the above proposal?
> > 
> > I may be missing something, but I don't see why this would be a significantly better option. It seems
> > it would make the callers more complex, and decrease readability.
> 
> 
> Basically,
> I guess sruct  will allow us to avoid (WARN_ON(offset >= RZG2L_CRU_MAX_REG) and
>    BUILD_BUG_ON(offset >= RZG2L_CRU_MAX_REG); checks as there is no array, so there is no
>    buffer overrun condition and also we can drop enum aswell.
> 
> So, if using struct decreases readability and makes the code complex,
> then current patch is fine.

For v6 I'm going to keep the current registers mapping implementation
adding fixes suggested by Laurent.

Thanks & Regards,
Tommaso

> 
> Cheers,
> Biju
> 
> 
>