RE: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support
From: Biju Das
Date: Wed Apr 09 2025 - 03:26:13 EST
Hi Laurent,
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > > > > > > > >
> > > > > > > > > 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@xxxxxxxxxxxxxx>
> > > > > > > > > Signed-off-by: Tommaso Merciai
> > > > > > > > > <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > > > > > > > > ---
> > > > > > > > > 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.
Cheers,
Biju