RE: [PATCH v4 11/17] media: rzg2l-cru: Add register mapping support

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: 27 March 2025 10:16
> Subject: Re: [PATCH v4 11/17] media: rzg2l-cru: Add register mapping support
> 
> Hi Tommaso,
> 
> Thanks for being patient (and reminding me about this). Apparently, Embedded World is bad for e-mail
> backlogs.
> 
> On Thu, Mar 13, 2025 at 01:01:24PM +0100, Tommaso Merciai wrote:
> > On Wed, Mar 12, 2025 at 01:37:25PM +0000, Biju Das wrote:
> > > On 03 March 2025 16:08, 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
> > > >
> > > >  .../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 eed9d2bd0841..abc2a979833a 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.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 u32 rzg2l_cru_read(struct rzg2l_cru_dev *cru, u32 offset)
> > > > +static inline void
> > > > +__rzg2l_cru_write_constant(struct rzg2l_cru_dev *cru, u32 offset,
> > > > +u32 value)
> > > >  {
> > > > -	return ioread32(cru->base + offset);
> > > > +	const u16 *regs = cru->info->regs;
> > > > +
> > > > +	BUILD_BUG_ON(offset >= RZG2L_CRU_MAX_REG);
> > > > +
> > > > +	iowrite32(value, cru->base + regs[offset]);
> > >
> > > Do you need this code as the purpose is to test compile time
> > > constant and It won't execute at run time?
> 
> Biju, I'm not sure to understan this comment.
> __rzg2l_cru_write_constant() is called at runtime, with a compile-time constant offset. The
> BUILD_BUG_ON() verifies at compile time that the offset is valid, causing compilation errors if it
> isn't.

Thanks for explanation. Now got it. I don't see any drivers using this way. Hence the confusion.

> 
> __rzg2l_cru_write(), on the other hand, is called when the offset is not known at compile time,
> because it's computed dynamically. That's a small subset of the calls. It needs to check the offset at
> runtime for overflows.

OK.

> 
> What do you mean by "won't execute at runtime", and what code do you think is not needed ?

I got confused with BUILD_BUG_ON() on a frequently called function and the one without __rzg2l_cru_write()

Cheers,
Biju