Message-ID: <DB6PR0402MB27606B563E19D96EB89C6B1488A70@DB6PR0402MB2760.eurprd04.prod.outlook.com>
Date:   Fri, 15 Jan 2021 03:29:43 +0000
From:   Peng Fan <peng.fan@....com>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
CC:     "ohad@...ery.com" <ohad@...ery.com>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "o.rempel@...gutronix.de" <o.rempel@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "paul@...pouillou.net" <paul@...pouillou.net>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "agross@...nel.org" <agross@...nel.org>,
        "patrice.chotard@...com" <patrice.chotard@...com>
Subject: RE: [PATCH V5 7/8] remoteproc: imx_rproc: ignore mapping vdev regions

> Subject: Re: [PATCH V5 7/8] remoteproc: imx_rproc: ignore mapping vdev
> regions
> 
> On Thu, Jan 14, 2021 at 09:52:13AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V5 7/8] remoteproc: imx_rproc: ignore mapping
> > > vdev regions
> > >
> > > On Wed, Jan 13, 2021 at 02:19:32AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH V5 7/8] remoteproc: imx_rproc: ignore
> > > > > mapping vdev regions
> > > > >
> > > > > On Tue, Jan 12, 2021 at 09:41:12AM +0000, Peng Fan wrote:
> > > > > > > Subject: Re: [PATCH V5 7/8] remoteproc: imx_rproc: ignore
> > > > > > > mapping vdev regions
> > > > > > >
> > > > > > > On Tue, Dec 29, 2020 at 11:30:18AM +0800, peng.fan@....com
> > > wrote:
> > > > > > > > From: Peng Fan <peng.fan@....com>
> > > > > > > >
> > > > > > > > vdev regions are vdev0vring0, vdev0vring1, vdevbuffer and
> similar.
> > > > > > > > They are handled by remoteproc common code, no need to map
> > > > > > > > in imx rproc driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Peng Fan <peng.fan@....com>
> > > > > > > > ---
> > > > > > > >  drivers/remoteproc/imx_rproc.c | 3 +++
> > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > > > > > > b/drivers/remoteproc/imx_rproc.c index
> > > > > > > > f80428afb8a7..e62a53ee128e
> > > > > > > > 100644
> > > > > > > > --- a/drivers/remoteproc/imx_rproc.c
> > > > > > > > +++ b/drivers/remoteproc/imx_rproc.c
> > > > > > > > @@ -417,6 +417,9 @@ static int imx_rproc_addr_init(struct
> > > > > > > > imx_rproc
> > > > > > > *priv,
> > > > > > > >  		struct resource res;
> > > > > > > >
> > > > > > > >  		node = of_parse_phandle(np, "memory-region", a);
> > > > > > > > +		/* Not map vdev region */
> > > > > > > > +		if (!strcmp(node->name, "vdev"))
> > > > > > > > +			continue;
> > > > > > >
> > > > > > > I am very confused and because I don't see an example for
> > > > > > > the DT in the bindings document I have to guess what is going on.
> > > > > >
> > > > > > V6 will include the DT yaml.
> > > > > >
> > > > > > >
> > > > > > > So I am guessing that you have laid out the memory regions
> > > > > > > for the vrings and the vdev0buffer in the DT "memory-region".
> > > > > >
> > > > > > The dts part will be similar as following:
> > > > > >
> > > > > > +    #include <dt-bindings/clock/imx8mm-clock.h>
> > > > > > +    rsc_table: rsc_table@...ff000 {
> > > > > > +      no-map;
> > > > > > +      reg = <0x550ff000 0x1000>;
> > > > > > +    };
> > > > > > +
> > > > > > +    vdev0vring0: vdev0vring0@...00000 {
> > > > > > +      no-map;
> > > > > > +      reg = <0x55000000 0x8000>;
> > > > > > +    };
> > > > > > +
> > > > > > +    vdev0vring1: vdev0vring1@...08000 {
> > > > > > +      reg = <0x55008000 0x8000>;
> > > > > > +      no-map;
> > > > > > +    };
> > > > > > +
> > > > > > +    vdevbuffer: vdevbuffer@...00000 {
> > > > > > +      compatible = "shared-dma-pool";
> > > > > > +      reg = <0x55400000 0x100000>;
> > > > > > +      no-map;
> > > > > > +    };
> > > > > > +
> > > > > > +    imx8mm-cm4 {
> > > > > > +      compatible = "fsl,imx8mm-cm4";
> > > > > > +      clocks = <&clk IMX8MM_CLK_M4_DIV>;
> > > > > > +      mbox-names = "tx", "rx", "rxdb";
> > > > > > +      mboxes = <&mu 0 1
> > > > > > +                &mu 1 1
> > > > > > +                &mu 3 1>;
> > > > > > +      memory-region = <&vdevbuffer>, <&vdev0vring0>,
> > > > > > + <&vdev0vring1>,
> > > > > <&rsc_table>;
> > > > > > +      syscon = <&src>;
> > > > > > +    };
> > > > > >
> > > > > > >
> > > > > > > For the vrings I don't see the allocation of a carveout,
> > > > > > > which means that you will take the memory out of the DMA
> > > > > > > pool and the reserve memory will be wasted.
> > > > > >
> > > > > > imx_rproc_parse_memory_regions will alloc carveout.
> > > > >
> > > > > They _will_ but for now they don't and as such there a
> > > > > discrepancy between the bindings and the code that was published
> > > > > in V6.  At this point you can either drop the vrings in the DT
> > > > > or send another revision with the carveouts allocated.  I would
> > > > > definitely prefer the latter because it wouldn't involve yet
> > > > > another modification of the
> > > bindings.
> > > >
> > > > You mean I drop patch v5 7/8 and send v7, right?
> > > >
> > >
> > > If you do that than the implementation won't reflect the bindings.
> > >
> > > > Or are there other changes that I need to do?
> > >
> > > If you want to keep the bindings the same way you have them in V6,
> > > carveouts are required in the implementation.
> >
> > ok. Could you share more info about carveouts are required?
> > imx_rproc_parse_memory_regions in my pachset already have
> > rproc_add_carveout. I am not clear what else needs to be done.
> 
> Ah yes, I see now.  In your answer above you wrote
> "imx_rproc_parse_memory_regions will alloc carveout", which I interpreted
> as "will do so in a future patchset".
> 
> I will review V6 - no need to change anything at this time.

Thanks, since we are almost clear and no major code change, I'll post V7
after addressing the comments in V6.

Thanks,
Peng.

> 
> >
> > Thanks,
> > Peng.
> >
> > >
> > > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > For the vdev0buffer, what you have will work *only* if that
> > > > > > > entry is the first one in the list of memory regions, as we
> > > > > > > agreed here
> > > [2].
> > > > > >
> > > > > > Yes. I agree and follow this rule from then.
> > > > > >
> > > > > > Thanks,
> > > > > > Peng.
> > > > > >
> > > > > > >
> > > > > > > [1].
> > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A
> > > > > > > %2F%25
> > > > > > > 2Fel
> > > > > > > ixir.b
> > > > > > >
> > > > >
> > >
> ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre
> > > > > > >
> > > > >
> > >
> moteproc_core.c%23L321&amp;data=04%7C01%7Cpeng.fan%40nxp.com%7
> > > > > > >
> > > > >
> > >
> C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c
> > > > > > >
> > > > >
> > >
> 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3
> > > > > > >
> > > > >
> > >
> d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > > > > > > %3D%7C1000&amp;sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAIt
> qZ
> > > m66
> > > > > b4
> > > > > > > m20%3D&amp;reserved=0
> > > > > > > [2].
> > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A
> > > > > > > %2F%25
> > > > > > > 2Fpa
> > > > > > > tch
> > > > > > >
> > > > >
> > >
> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315
> > > > > > >
> > > > >
> > >
> 43.7024-1-peng.fan%40nxp.com%2F&amp;data=04%7C01%7Cpeng.fan%40n
> > > > > > >
> > > > >
> > >
> xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9
> > > > > > >
> > > > >
> > >
> 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW
> > > > > > >
> > > > >
> > >
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> > > > > > >
> > > > >
> > >
> VCI6Mn0%3D%7C1000&amp;sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2
> > > > > > > FHjwA6a8x58geY7eE%3D&amp;reserved=0
> > > > > > >
> > > > > > > >  		err = of_address_to_resource(node, 0, &res);
> > > > > > > >  		if (err) {
> > > > > > > >  			dev_err(dev, "unable to resolve memory
> region\n");
> > > > > > > > --
> > > > > > > > 2.28.0
> > > > > > > >

Powered by blists - more mailing lists