1. Patchew
  2. linux
  3. [RFC PATCH 00/13] Add PFC support for Renesas RZ/V2H(P) SoC
  4. View patch

[RFC PATCH 07/13] pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and ETH

Prabhakar posted 13 patches 1 year, 10 months ago
There is a newer version of this series
[RFC PATCH 07/13] pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and ETH
Posted by Prabhakar 1 year, 10 months ago 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

On RZ/V2H(P) SoC, the power registers for SD and ETH do not exist,
resulting in invalid register offsets. Ensure that the register offsets
are valid before any read/write operations are performed. If the power
registers are not available, both SD and ETH will be set to -EINVAL.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 348fdccaff72..705372faaeff 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -184,8 +184,8 @@
  */
 struct rzg2l_register_offsets {
 	u16 pwpr;
-	u16 sd_ch;
-	u16 eth_poc;
+	int sd_ch;
+	int eth_poc;
 };
 
 /**
@@ -2567,8 +2567,10 @@ static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
 	rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
 
 	for (u8 i = 0; i < 2; i++) {
-		cache->sd_ch[i] = readb(pctrl->base + SD_CH(regs->sd_ch, i));
-		cache->eth_poc[i] = readb(pctrl->base + ETH_POC(regs->eth_poc, i));
+		if (regs->sd_ch != -EINVAL)
+			cache->sd_ch[i] = readb(pctrl->base + SD_CH(regs->sd_ch, i));
+		if (regs->eth_poc != -EINVAL)
+			cache->eth_poc[i] = readb(pctrl->base + ETH_POC(regs->eth_poc, i));
 	}
 
 	cache->qspi = readb(pctrl->base + QSPI);
@@ -2599,8 +2601,10 @@ static int rzg2l_pinctrl_resume_noirq(struct device *dev)
 	writeb(cache->qspi, pctrl->base + QSPI);
 	writeb(cache->eth_mode, pctrl->base + ETH_MODE);
 	for (u8 i = 0; i < 2; i++) {
-		writeb(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
-		writeb(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
+		if (regs->sd_ch != -EINVAL)
+			writeb(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
+		if (regs->eth_poc != -EINVAL)
+			writeb(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
 	}
 
 	rzg2l_pinctrl_pm_setup_pfc(pctrl);
-- 
2.34.1
Re: [RFC PATCH 07/13] pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and ETH
Posted by claudiu beznea 1 year, 10 months ago 
Hi, Prabhakar,

On 27.03.2024 00:28, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On RZ/V2H(P) SoC, the power registers for SD and ETH do not exist,
> resulting in invalid register offsets. Ensure that the register offsets
> are valid before any read/write operations are performed. If the power
> registers are not available, both SD and ETH will be set to -EINVAL.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 348fdccaff72..705372faaeff 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -184,8 +184,8 @@
>   */
>  struct rzg2l_register_offsets {
>  	u16 pwpr;
> -	u16 sd_ch;
> -	u16 eth_poc;
> +	int sd_ch;
> +	int eth_poc;
>  };
>  
>  /**
> @@ -2567,8 +2567,10 @@ static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
>  	rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
>  
>  	for (u8 i = 0; i < 2; i++) {
> -		cache->sd_ch[i] = readb(pctrl->base + SD_CH(regs->sd_ch, i));
> -		cache->eth_poc[i] = readb(pctrl->base + ETH_POC(regs->eth_poc, i));
> +		if (regs->sd_ch != -EINVAL)

As of my knowledge, the current users of this driver uses SD and ETH
offsets different from zero. To avoid populating these values for all the
SoCs and avoid increasing the size of these fields I think you can add
checks like these:

if (regs->sd_ch)
	// set sd_ch


Same for the rest.

> +			cache->sd_ch[i] = readb(pctrl->base + SD_CH(regs->sd_ch, i));
> +		if (regs->eth_poc != -EINVAL)
> +			cache->eth_poc[i] = readb(pctrl->base + ETH_POC(regs->eth_poc, i));
>  	}
>  
>  	cache->qspi = readb(pctrl->base + QSPI);
> @@ -2599,8 +2601,10 @@ static int rzg2l_pinctrl_resume_noirq(struct device *dev)
>  	writeb(cache->qspi, pctrl->base + QSPI);
>  	writeb(cache->eth_mode, pctrl->base + ETH_MODE);
>  	for (u8 i = 0; i < 2; i++) {
> -		writeb(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
> -		writeb(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
> +		if (regs->sd_ch != -EINVAL)
> +			writeb(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
> +		if (regs->eth_poc != -EINVAL)
> +			writeb(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
>  	}
>  
>  	rzg2l_pinctrl_pm_setup_pfc(pctrl);
Re: [RFC PATCH 07/13] pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and ETH
Posted by Lad, Prabhakar 1 year, 10 months ago 
Hi Claudiu,

Thank you for the review.

On Thu, Mar 28, 2024 at 8:01 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Prabhakar,
>
> On 27.03.2024 00:28, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On RZ/V2H(P) SoC, the power registers for SD and ETH do not exist,
> > resulting in invalid register offsets. Ensure that the register offsets
> > are valid before any read/write operations are performed. If the power
> > registers are not available, both SD and ETH will be set to -EINVAL.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index 348fdccaff72..705372faaeff 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -184,8 +184,8 @@
> >   */
> >  struct rzg2l_register_offsets {
> >       u16 pwpr;
> > -     u16 sd_ch;
> > -     u16 eth_poc;
> > +     int sd_ch;
> > +     int eth_poc;
> >  };
> >
> >  /**
> > @@ -2567,8 +2567,10 @@ static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
> >       rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
> >
> >       for (u8 i = 0; i < 2; i++) {
> > -             cache->sd_ch[i] = readb(pctrl->base + SD_CH(regs->sd_ch, i));
> > -             cache->eth_poc[i] = readb(pctrl->base + ETH_POC(regs->eth_poc, i));
> > +             if (regs->sd_ch != -EINVAL)
>
> As of my knowledge, the current users of this driver uses SD and ETH
> offsets different from zero. To avoid populating these values for all the
> SoCs and avoid increasing the size of these fields I think you can add
> checks like these:
>
> if (regs->sd_ch)
>         // set sd_ch
>
Agreed.

>
> Same for the rest.
>
OK.

Cheers,
Prabhakar
Re: [RFC PATCH 07/13] pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and ETH
Posted by Dan Carpenter 1 year, 10 months ago 
On Tue, Mar 26, 2024 at 10:28:38PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On RZ/V2H(P) SoC, the power registers for SD and ETH do not exist,
> resulting in invalid register offsets. Ensure that the register offsets
> are valid before any read/write operations are performed. If the power
                                                            ^^^^^^^^^^^^
> registers are not available, both SD and ETH will be set to -EINVAL.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Where does this happen?  It doesn't seem to be a part of this patchset.
-EINVAL seems weird here, but it's hard to judge without actually seeing
it.

regards,
dan carpenter
Re: [RFC PATCH 07/13] pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and ETH
Posted by Lad, Prabhakar 1 year, 10 months ago 
Hi Dan,

Thank you for the review.

On Wed, Mar 27, 2024 at 7:58 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Tue, Mar 26, 2024 at 10:28:38PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On RZ/V2H(P) SoC, the power registers for SD and ETH do not exist,
> > resulting in invalid register offsets. Ensure that the register offsets
> > are valid before any read/write operations are performed. If the power
>                                                             ^^^^^^^^^^^^
> > registers are not available, both SD and ETH will be set to -EINVAL.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Where does this happen?  It doesn't seem to be a part of this patchset.
> -EINVAL seems weird here, but it's hard to judge without actually seeing
> it.
>
Good catch, in patch 13/13 it should be below instead of sd_ch and
eth_poc assigned to 0.

+static const struct rzg2l_hwcfg rzv2h_hwcfg = {
+       .regs = {
+               .pwpr = 0x3c04,
+               .sd_ch = -EINVAL,
+               .eth_poc = -EINVAL,
+       },
+};

Cheers,
Prabhakar

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.