diff mbox series

[V2,5/5] remoteproc: imx_rproc: change to ioremap_wc for dram

Message ID 20210707094033.1959752-5-aisheng.dong@nxp.com (mailing list archive)
State Superseded
Headers show
Series [V2,1/5] remoteproc: fix the wrong default value of is_iomem | expand

Commit Message

Aisheng Dong July 7, 2021, 9:40 a.m. UTC
DRAM is not io memory, so changed to ioremap_wc. This is also
aligned with core io accessories. e.g. memcpy/memset and cpu direct
access.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Fixes: 5e4c1243071d ("remoteproc: imx_rproc: support remote cores booted before Linux Kernel")
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v1->v2:
 * new patch
---
 drivers/remoteproc/imx_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dong Aisheng July 8, 2021, 8:12 a.m. UTC | #1
On Wed, Jul 7, 2021 at 5:41 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> DRAM is not io memory, so changed to ioremap_wc. This is also
> aligned with core io accessories. e.g. memcpy/memset and cpu direct
> access.
>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

[...]

> Fixes: 5e4c1243071d ("remoteproc: imx_rproc: support remote cores booted before Linux Kernel")
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Above two lines were added by mistake. (copied from patch 4)
I'm going to wait for review comments first and resend with changes if any.

Regards
Aisheng

> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v1->v2:
>  * new patch
> ---
>  drivers/remoteproc/imx_rproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index ff620688fad9..4ae416ba5080 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -597,7 +597,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>                         break;
>
>                 /* Not use resource version, because we might share region */
> -               priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
> +               priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev, res.start, resource_size(&res));
>                 if (!priv->mem[b].cpu_addr) {
>                         dev_err(dev, "failed to remap %pr\n", &res);
>                         return -ENOMEM;
> --
> 2.25.1
>
Mathieu Poirier July 13, 2021, 5:06 p.m. UTC | #2
On Wed, Jul 07, 2021 at 05:40:33PM +0800, Dong Aisheng wrote:
> DRAM is not io memory, so changed to ioremap_wc. This is also
> aligned with core io accessories. e.g. memcpy/memset and cpu direct
> access.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Fixes: 5e4c1243071d ("remoteproc: imx_rproc: support remote cores booted before Linux Kernel")
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v1->v2:
>  * new patch

It's a new patch and yet Peng's RB tag is already on it...

> ---
>  drivers/remoteproc/imx_rproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index ff620688fad9..4ae416ba5080 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -597,7 +597,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  			break;
>  
>  		/* Not use resource version, because we might share region */
> -		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
> +		priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev, res.start, resource_size(&res));

How was it working before?  Will it really work for all platforms and was this
extensively tested?

Peng - I will need an explicit reply from you that you are in agreement with
this change.  I will also need you to review patch 01 and 02 of this set.

Thanks,
Mathieu

>  		if (!priv->mem[b].cpu_addr) {
>  			dev_err(dev, "failed to remap %pr\n", &res);
>  			return -ENOMEM;
> -- 
> 2.25.1
>
Peng Fan July 14, 2021, 12:59 a.m. UTC | #3
Hi Mathieu,

> Subject: Re: [PATCH V2 5/5] remoteproc: imx_rproc: change to ioremap_wc
> for dram
> 
> On Wed, Jul 07, 2021 at 05:40:33PM +0800, Dong Aisheng wrote:
> > DRAM is not io memory, so changed to ioremap_wc. This is also aligned
> > with core io accessories. e.g. memcpy/memset and cpu direct access.
> >
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Fixes: 5e4c1243071d ("remoteproc: imx_rproc: support remote cores
> > booted before Linux Kernel")
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v1->v2:
> >  * new patch
> 
> It's a new patch and yet Peng's RB tag is already on it...

Aisheng's reply in the other email.
"
Above two lines were added by mistake. (copied from patch 4)
I'm going to wait for review comments first and resend with changes if any.

Regards
Aisheng
"

> 
> > ---
> >  drivers/remoteproc/imx_rproc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index ff620688fad9..4ae416ba5080
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -597,7 +597,7 @@ static int imx_rproc_addr_init(struct imx_rproc
> *priv,
> >  			break;
> >
> >  		/* Not use resource version, because we might share region */
> > -		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start,
> resource_size(&res));
> > +		priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev,
> res.start,
> > +resource_size(&res));
> 
> How was it working before?  

we took all memory used by M-core as io memory, including TCM/SRAM/DRAM.

Will it really work for all platforms and was this
> extensively tested?

Aisheng may confirm, I not test this patchset.

> 
> Peng - I will need an explicit reply from you that you are in agreement with
> this change. 

I not test, but it should work per my understanding.
i.MX TCM area has limitation that not use ioremap_wc, DRAM has no such
limitation. Just think this not deserve a fix tag.

> I will also need you to review patch 01 and 02 of this set.

Sure, I'll review.

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> >  		if (!priv->mem[b].cpu_addr) {
> >  			dev_err(dev, "failed to remap %pr\n", &res);
> >  			return -ENOMEM;
> > --
> > 2.25.1
> >
Dong Aisheng July 14, 2021, 10:15 a.m. UTC | #4
On Wed, Jul 14, 2021 at 1:06 AM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Wed, Jul 07, 2021 at 05:40:33PM +0800, Dong Aisheng wrote:
> > DRAM is not io memory, so changed to ioremap_wc. This is also
> > aligned with core io accessories. e.g. memcpy/memset and cpu direct
> > access.
> >
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Fixes: 5e4c1243071d ("remoteproc: imx_rproc: support remote cores booted before Linux Kernel")
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v1->v2:
> >  * new patch
>
> It's a new patch and yet Peng's RB tag is already on it...
>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > index ff620688fad9..4ae416ba5080 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -597,7 +597,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
> >                       break;
> >
> >               /* Not use resource version, because we might share region */
> > -             priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
> > +             priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev, res.start, resource_size(&res));
>
> How was it working before?  Will it really work for all platforms and was this
> extensively tested?

Here it is only used for accessing resource tables in DRAM which is
published by M core.

Why it works before is because:
1. the default memory access in remoteproc core (.e.g memcpy or direct
access by pointer
by CPU) seems also work well even it's device memory type mapped by
devm_ioremap.
e.g. cpu direct access
offset = rproc->table_ptr->offset[i]

2.  It will not work with meset() in rproc_elf_load_segments() which
has cache operations internally.
e.g. arch/arm64/lib/memset.S
However, it's lucky that for IMX cases, the resource table in DRAM are
currently used by early boot
(e.g. uboot/scfw loading M4 firmware), no chance to run into
rproc_elf_load_segments(),
so no issues so far.

Then the question is should we change the mapping type of resource
table mem to normal
memory (ioremap_wc) as remoteproc core are using normal memory accessories?

I guess we should do that, that's how this patch comes out.

Regards
Aisheng

>
> Peng - I will need an explicit reply from you that you are in agreement with
> this change.  I will also need you to review patch 01 and 02 of this set.
>
> Thanks,
> Mathieu
>
> >               if (!priv->mem[b].cpu_addr) {
> >                       dev_err(dev, "failed to remap %pr\n", &res);
> >                       return -ENOMEM;
> > --
> > 2.25.1
> >
Dong Aisheng July 14, 2021, 10:21 a.m. UTC | #5
On Wed, Jul 14, 2021 at 8:59 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Mathieu,
>
> > Subject: Re: [PATCH V2 5/5] remoteproc: imx_rproc: change to ioremap_wc
> > for dram
> >
> > On Wed, Jul 07, 2021 at 05:40:33PM +0800, Dong Aisheng wrote:
> > > DRAM is not io memory, so changed to ioremap_wc. This is also aligned
> > > with core io accessories. e.g. memcpy/memset and cpu direct access.
> > >
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > Fixes: 5e4c1243071d ("remoteproc: imx_rproc: support remote cores
> > > booted before Linux Kernel")
> > > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > > v1->v2:
> > >  * new patch
> >
> > It's a new patch and yet Peng's RB tag is already on it...
>
> Aisheng's reply in the other email.
> "
> Above two lines were added by mistake. (copied from patch 4)
> I'm going to wait for review comments first and resend with changes if any.
>
> Regards
> Aisheng
> "
>
> >
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index ff620688fad9..4ae416ba5080
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -597,7 +597,7 @@ static int imx_rproc_addr_init(struct imx_rproc
> > *priv,
> > >                     break;
> > >
> > >             /* Not use resource version, because we might share region */
> > > -           priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start,
> > resource_size(&res));
> > > +           priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev,
> > res.start,
> > > +resource_size(&res));
> >
> > How was it working before?
>
> we took all memory used by M-core as io memory, including TCM/SRAM/DRAM.
>
> Will it really work for all platforms and was this
> > extensively tested?
>
> Aisheng may confirm, I not test this patchset.
>
> >
> > Peng - I will need an explicit reply from you that you are in agreement with
> > this change.
>
> I not test, but it should work per my understanding.
> i.MX TCM area has limitation that not use ioremap_wc, DRAM has no such
> limitation. Just think this not deserve a fix tag.
>

I tested with MX8MQ, i could find more to test although it should work
theoretically.
Why adding fixes tag is because using normal memory accessories (e.g. memset)
for io memory (device type) may have issues.
Does it make sense?

Regards
Aisheng


> > I will also need you to review patch 01 and 02 of this set.
>
> Sure, I'll review.
>
> Thanks,
> Peng.
>
> >
> > Thanks,
> > Mathieu
> >
> > >             if (!priv->mem[b].cpu_addr) {
> > >                     dev_err(dev, "failed to remap %pr\n", &res);
> > >                     return -ENOMEM;
> > > --
> > > 2.25.1
> > >
diff mbox series

Patch

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index ff620688fad9..4ae416ba5080 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -597,7 +597,7 @@  static int imx_rproc_addr_init(struct imx_rproc *priv,
 			break;
 
 		/* Not use resource version, because we might share region */
-		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
+		priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev, res.start, resource_size(&res));
 		if (!priv->mem[b].cpu_addr) {
 			dev_err(dev, "failed to remap %pr\n", &res);
 			return -ENOMEM;