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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <TY3PR01MB11346B76B957547A350BDED6E86A12@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Thu, 27 Mar 2025 10:29:06 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: laurent.pinchart <laurent.pinchart@...asonboard.com>, Tommaso Merciai
	<tommaso.merciai.xr@...renesas.com>
CC: Tommaso Merciai <tomm.merciai@...il.com>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>, Prabhakar
 Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>, Mauro Carvalho Chehab
	<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Geert Uytterhoeven
	<geert+renesas@...der.be>, Magnus Damm <magnus.damm@...il.com>, Hans Verkuil
	<hverkuil@...all.nl>, Uwe Kleine-König
	<u.kleine-koenig@...libre.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4 11/17] media: rzg2l-cru: Add register mapping support

Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> 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@...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@...renesas.com>
> > > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@...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
> > > >
> > > >  .../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