[PATCH 0/2] mmc: renesas: Trivial fixes

[PATCH 0/2] mmc: renesas: Trivial fixes

[Lad Prabhakar]

Hi All,

This patch series adds trivial fixes to renesas mmc driver.

Cheers,
Prabhakar

Lad Prabhakar (2):
  mmc: renesas_sdhi: Jump to error path instead of returning directly
  mmc: renesas_internal_dmac: Fix typo's

 drivers/mmc/host/renesas_sdhi_core.c          | 6 ++++--
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

[Lad Prabhakar]

Jump to error path "edisclk" instead of returning directly in case of
devm_reset_control_get_optional_exclusive() failure.

Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible")
Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 2797a9c0f17d..cddb0185f5fb 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 		goto efree;
 
 	priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
-	if (IS_ERR(priv->rstc))
-		return PTR_ERR(priv->rstc);
+	if (IS_ERR(priv->rstc)) {
+		ret = PTR_ERR(priv->rstc);
+		goto edisclk;
+	}
 
 	ver = sd_ctrl_read16(host, CTL_VERSION);
 	/* GEN2_SDR104 is first known SDHI to use 32bit block count */
-- 
2.17.1

RE: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

[Biju Das]

Hi Prabhakar and Pavel,

Thanks for the patch.

> Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of
> returning directly
> 
> Jump to error path "edisclk" instead of returning directly in case of
> devm_reset_control_get_optional_exclusive() failure.
> 
> Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible")
> Reported-by: Pavel Machek <pavel@denx.de>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> b/drivers/mmc/host/renesas_sdhi_core.c
> index 2797a9c0f17d..cddb0185f5fb 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device
> *pdev,
>  		goto efree;
> 
>  	priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev,
> NULL);
> -	if (IS_ERR(priv->rstc))
> -		return PTR_ERR(priv->rstc);
> +	if (IS_ERR(priv->rstc)) {
> +		ret = PTR_ERR(priv->rstc);
> +		goto edisclk;
> +	}

Why can't devm_reset_control_get_optional_exclusive to be moved up before devm_clk_get?

Cheers,
Biju

> 
>  	ver = sd_ctrl_read16(host, CTL_VERSION);
>  	/* GEN2_SDR104 is first known SDHI to use 32bit block count */
> --
> 2.17.1

Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

[Lad, Prabhakar]

Hi Biju,

Thank you for the review.

On Mon, Apr 4, 2022 at 7:02 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar and Pavel,
>
> Thanks for the patch.
>
> > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of
> > returning directly
> >
> > Jump to error path "edisclk" instead of returning directly in case of
> > devm_reset_control_get_optional_exclusive() failure.
> >
> > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible")
> > Reported-by: Pavel Machek <pavel@denx.de>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > b/drivers/mmc/host/renesas_sdhi_core.c
> > index 2797a9c0f17d..cddb0185f5fb 100644
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device
> > *pdev,
> >               goto efree;
> >
> >       priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev,
> > NULL);
> > -     if (IS_ERR(priv->rstc))
> > -             return PTR_ERR(priv->rstc);
> > +     if (IS_ERR(priv->rstc)) {
> > +             ret = PTR_ERR(priv->rstc);
> > +             goto edisclk;
> > +     }
>
> Why can't devm_reset_control_get_optional_exclusive to be moved up before devm_clk_get?
>
In that case we will have to jump to the "efree" label Or if you don't
want goto at all this can be moved to the very beginning of the probe.

Wolfram, what is your preference on the above?

Cheers,
Prabhakar

RE: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

[Biju Das]

Hi Prabhkar,

> Subject: Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of
> returning directly
> 
> Hi Biju,
> 
> Thank you for the review.
> 
> On Mon, Apr 4, 2022 at 7:02 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >
> > Hi Prabhakar and Pavel,
> >
> > Thanks for the patch.
> >
> > > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead
> > > of returning directly
> > >
> > > Jump to error path "edisclk" instead of returning directly in case
> > > of
> > > devm_reset_control_get_optional_exclusive() failure.
> > >
> > > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if
> > > possible")
> > > Reported-by: Pavel Machek <pavel@denx.de>
> > > Signed-off-by: Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > > b/drivers/mmc/host/renesas_sdhi_core.c
> > > index 2797a9c0f17d..cddb0185f5fb 100644
> > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device
> > > *pdev,
> > >               goto efree;
> > >
> > >       priv->rstc =
> > > devm_reset_control_get_optional_exclusive(&pdev->dev,
> > > NULL);
> > > -     if (IS_ERR(priv->rstc))
> > > -             return PTR_ERR(priv->rstc);
> > > +     if (IS_ERR(priv->rstc)) {
> > > +             ret = PTR_ERR(priv->rstc);
> > > +             goto edisclk;
> > > +     }
> >
> > Why can't devm_reset_control_get_optional_exclusive to be moved up
> before devm_clk_get?
> >
> In that case we will have to jump to the "efree" label Or if you don't
> want goto at all this can be moved to the very beginning of the probe.

I guess it has to move up, first get reset handle and clock handle and return error
directly in case of error, Then do clk/reset ops. 

> 
> Wolfram, what is your preference on the above?
> 
> Cheers,
> Prabhakar

Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

[Lad, Prabhakar]

Hi Biju,

On Mon, Apr 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhkar,
>
> > Subject: Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of
> > returning directly
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Mon, Apr 4, 2022 at 7:02 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > >
> > > Hi Prabhakar and Pavel,
> > >
> > > Thanks for the patch.
> > >
> > > > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead
> > > > of returning directly
> > > >
> > > > Jump to error path "edisclk" instead of returning directly in case
> > > > of
> > > > devm_reset_control_get_optional_exclusive() failure.
> > > >
> > > > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if
> > > > possible")
> > > > Reported-by: Pavel Machek <pavel@denx.de>
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > > > b/drivers/mmc/host/renesas_sdhi_core.c
> > > > index 2797a9c0f17d..cddb0185f5fb 100644
> > > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device
> > > > *pdev,
> > > >               goto efree;
> > > >
> > > >       priv->rstc =
> > > > devm_reset_control_get_optional_exclusive(&pdev->dev,
> > > > NULL);
> > > > -     if (IS_ERR(priv->rstc))
> > > > -             return PTR_ERR(priv->rstc);
> > > > +     if (IS_ERR(priv->rstc)) {
> > > > +             ret = PTR_ERR(priv->rstc);
> > > > +             goto edisclk;
> > > > +     }
> > >
> > > Why can't devm_reset_control_get_optional_exclusive to be moved up
> > before devm_clk_get?
> > >
> > In that case we will have to jump to the "efree" label Or if you don't
> > want goto at all this can be moved to the very beginning of the probe.
>
> I guess it has to move up, first get reset handle and clock handle and return error
> directly in case of error, Then do clk/reset ops.
>
Fine by me.

Cheers,
Prabhakar

> >
> > Wolfram, what is your preference on the above?
> >
> > Cheers,
> > Prabhakar

Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]


> I guess it has to move up, first get reset handle and clock handle and return error
> directly in case of error, Then do clk/reset ops. 
> 
> > 
> > Wolfram, what is your preference on the above?

Yes, moving up makes sense. First check all the handles before we
actually initialize the hardware.

Thanks for pointing all this out.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] mmc: renesas_internal_dmac: Fix typo's

[Lad Prabhakar]

Fix typo's,
* difference -> different
* alignment -> aligned

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 1685df00863b..b468368a4539 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -310,7 +310,7 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {
 }
 
 /*
- * renesas_sdhi_internal_dmac_map() will be called with two difference
+ * renesas_sdhi_internal_dmac_map() will be called with two different
  * sg pointers in two mmc_data by .pre_req(), but tmio host can have a single
  * sg_ptr only. So, renesas_sdhi_internal_dmac_{un}map() should use a sg
  * pointer in a mmc_data instead of host->sg_ptr.
@@ -344,7 +344,7 @@ renesas_sdhi_internal_dmac_map(struct tmio_mmc_host *host,
 
 	data->host_cookie = cookie;
 
-	/* This DMAC cannot handle if buffer is not 128-bytes alignment */
+	/* This DMAC cannot handle if buffer is not 128-bytes aligned */
 	if (!IS_ALIGNED(sg_dma_address(data->sg), 128)) {
 		renesas_sdhi_internal_dmac_unmap(host, data, cookie);
 		return false;
-- 
2.17.1

Re: [PATCH 2/2] mmc: renesas_internal_dmac: Fix typo's

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]


> -	/* This DMAC cannot handle if buffer is not 128-bytes alignment */
> +	/* This DMAC cannot handle if buffer is not 128-bytes aligned */

Well, as we are here now, I think this can be further improved.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: renesas_internal_dmac: Fix typo's

[Lad, Prabhakar]

On Mon, Apr 4, 2022 at 7:21 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > -     /* This DMAC cannot handle if buffer is not 128-bytes alignment */
> > +     /* This DMAC cannot handle if buffer is not 128-bytes aligned */
>
> Well, as we are here now, I think this can be further improved.
>
"The internal DMAC supports 128-bytes aligned buffers only", does this
sound good?

Cheers,
Prabhakar

Re: [PATCH 2/2] mmc: renesas_internal_dmac: Fix typo's

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]


> > > -     /* This DMAC cannot handle if buffer is not 128-bytes alignment */
> > > +     /* This DMAC cannot handle if buffer is not 128-bytes aligned */
> >
> > Well, as we are here now, I think this can be further improved.
> >
> "The internal DMAC supports 128-bytes aligned buffers only", does this
> sound good?

I'd think it should be "128 byte aligned"? But I can't explain why, just
a gut feeling.

Otherwise good!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: renesas_internal_dmac: Fix typo's

[Lad, Prabhakar]

On Tue, Apr 5, 2022 at 5:45 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > > > -     /* This DMAC cannot handle if buffer is not 128-bytes alignment */
> > > > +     /* This DMAC cannot handle if buffer is not 128-bytes aligned */
> > >
> > > Well, as we are here now, I think this can be further improved.
> > >
> > "The internal DMAC supports 128-bytes aligned buffers only", does this
> > sound good?
>
> I'd think it should be "128 byte aligned"? But I can't explain why, just
> a gut feeling.
>
Fine by me.

Let me know your thoughts on patch 1/2, I'll send a v2 with updated comment.

Cheers,
Prabhakar

Re: [PATCH 2/2] mmc: renesas_internal_dmac: Fix typo's

[Geert Uytterhoeven]

On Tue, Apr 5, 2022 at 6:51 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > > -     /* This DMAC cannot handle if buffer is not 128-bytes alignment */
> > > > +     /* This DMAC cannot handle if buffer is not 128-bytes aligned */
> > >
> > > Well, as we are here now, I think this can be further improved.
> > >
> > "The internal DMAC supports 128-bytes aligned buffers only", does this
> > sound good?
>
> I'd think it should be "128 byte aligned"? But I can't explain why, just
> a gut feeling.

I'd say "128-byte aligned".
But let the native speakers^Wwriters decide ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/2] mmc: renesas: Trivial fixes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 120 bytes --]

> This patch series adds trivial fixes to renesas mmc driver.

Did I miss if there is anything left to discuss for v2?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] mmc: renesas: Trivial fixes

[Lad, Prabhakar]

Hi Wolfram,

On Tue, Jun 21, 2022 at 2:35 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> > This patch series adds trivial fixes to renesas mmc driver.
>
> Did I miss if there is anything left to discuss for v2?
>
Sorry I missed to follow up. I'll send a v2 soon.

Cheers,
Prabhakar