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

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

Mar 26, 2024, 3:28 PM

Post #1 of 5 (23 views)

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 [ In reply to ]

dan.carpenter at linaro

Mar 27, 2024, 12:58 AM

Post #2 of 5 (23 views)

Permalink

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 [ In reply to ]

prabhakar.csengg at gmail

Mar 27, 2024, 1:13 AM

Post #3 of 5 (21 views)

Permalink

Hi Dan,

Thank you for the review.

On Wed, Mar 27, 2024 at 7:58?AM Dan Carpenter <dan.carpenter@linaroorg> 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

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

claudiu.beznea at tuxon

Mar 28, 2024, 1:01 AM

Post #4 of 5 (20 views)

Permalink

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 [ In reply to ]

prabhakar.csengg at gmail

Mar 28, 2024, 12:40 PM

Post #5 of 5 (19 views)

Permalink

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