| Message ID | 20201229033019.25899-8-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: imx_rproc: support iMX8MQ/M | expand |
On Tue, Dec 29, 2020 at 11:30:18AM +0800, peng.fan@nxp.com wrote: > From: Peng Fan <peng.fan@nxp.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@nxp.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. So I am guessing that you have laid out the memory regions for the vrings and the vdev0buffer in the DT "memory-region". 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. 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]. [1]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/remoteproc/remoteproc_core.c#L321 [2]. https://patchwork.kernel.org/project/linux-remoteproc/patch/20200722131543.7024-1-peng.fan@nxp.com/ > err = of_address_to_resource(node, 0, &res); > if (err) { > dev_err(dev, "unable to resolve memory region\n"); > -- > 2.28.0 >
> 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@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { + no-map; + reg = <0x550ff000 0x1000>; + }; + + vdev0vring0: vdev0vring0@55000000 { + no-map; + reg = <0x55000000 0x8000>; + }; + + vdev0vring1: vdev0vring1@55008000 { + reg = <0x55008000 0x8000>; + no-map; + }; + + vdevbuffer: vdevbuffer@55400000 { + 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. > > 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%2Felixir.b > ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre > moteproc_core.c%23L321&data=04%7C01%7Cpeng.fan%40nxp.com%7 > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAItqZm66b4 > m20%3D&reserved=0 > [2]. > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315 > 43.7024-1-peng.fan%40nxp.com%2F&data=04%7C01%7Cpeng.fan%40n > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > FHjwA6a8x58geY7eE%3D&reserved=0 > > > err = of_address_to_resource(node, 0, &res); > > if (err) { > > dev_err(dev, "unable to resolve memory region\n"); > > -- > > 2.28.0 > >
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@nxp.com wrote: > > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { > + no-map; > + reg = <0x550ff000 0x1000>; > + }; > + > + vdev0vring0: vdev0vring0@55000000 { > + no-map; > + reg = <0x55000000 0x8000>; > + }; > + > + vdev0vring1: vdev0vring1@55008000 { > + reg = <0x55008000 0x8000>; > + no-map; > + }; > + > + vdevbuffer: vdevbuffer@55400000 { > + 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. > > > > > 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%2Felixir.b > > ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre > > moteproc_core.c%23L321&data=04%7C01%7Cpeng.fan%40nxp.com%7 > > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAItqZm66b4 > > m20%3D&reserved=0 > > [2]. > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > > work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315 > > 43.7024-1-peng.fan%40nxp.com%2F&data=04%7C01%7Cpeng.fan%40n > > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > > FHjwA6a8x58geY7eE%3D&reserved=0 > > > > > err = of_address_to_resource(node, 0, &res); > > > if (err) { > > > dev_err(dev, "unable to resolve memory region\n"); > > > -- > > > 2.28.0 > > >
> 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@nxp.com wrote: > > > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { > > + no-map; > > + reg = <0x550ff000 0x1000>; > > + }; > > + > > + vdev0vring0: vdev0vring0@55000000 { > > + no-map; > > + reg = <0x55000000 0x8000>; > > + }; > > + > > + vdev0vring1: vdev0vring1@55008000 { > > + reg = <0x55008000 0x8000>; > > + no-map; > > + }; > > + > > + vdevbuffer: vdevbuffer@55400000 { > > + 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? Or are there other changes that I need to do? 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%2Fel > > > ixir.b > > > > ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre > > > > moteproc_core.c%23L321&data=04%7C01%7Cpeng.fan%40nxp.com%7 > > > > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > > > > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > > > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > > > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAItqZm66 > b4 > > > m20%3D&reserved=0 > > > [2]. > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa > > > tch > > > > work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315 > > > > 43.7024-1-peng.fan%40nxp.com%2F&data=04%7C01%7Cpeng.fan%40n > > > > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > > > > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > > > FHjwA6a8x58geY7eE%3D&reserved=0 > > > > > > > err = of_address_to_resource(node, 0, &res); > > > > if (err) { > > > > dev_err(dev, "unable to resolve memory region\n"); > > > > -- > > > > 2.28.0 > > > >
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@nxp.com wrote: > > > > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { > > > + no-map; > > > + reg = <0x550ff000 0x1000>; > > > + }; > > > + > > > + vdev0vring0: vdev0vring0@55000000 { > > > + no-map; > > > + reg = <0x55000000 0x8000>; > > > + }; > > > + > > > + vdev0vring1: vdev0vring1@55008000 { > > > + reg = <0x55008000 0x8000>; > > > + no-map; > > > + }; > > > + > > > + vdevbuffer: vdevbuffer@55400000 { > > > + 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. > > 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%2Fel > > > > ixir.b > > > > > > ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre > > > > > > moteproc_core.c%23L321&data=04%7C01%7Cpeng.fan%40nxp.com%7 > > > > > > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > > > > > > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > > > > > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > > > > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAItqZm66 > > b4 > > > > m20%3D&reserved=0 > > > > [2]. > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa > > > > tch > > > > > > work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315 > > > > > > 43.7024-1-peng.fan%40nxp.com%2F&data=04%7C01%7Cpeng.fan%40n > > > > > > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > > > > > > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > > > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > > > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > > > > FHjwA6a8x58geY7eE%3D&reserved=0 > > > > > > > > > err = of_address_to_resource(node, 0, &res); > > > > > if (err) { > > > > > dev_err(dev, "unable to resolve memory region\n"); > > > > > -- > > > > > 2.28.0 > > > > >
> 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@nxp.com > wrote: > > > > > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { > > > > + no-map; > > > > + reg = <0x550ff000 0x1000>; > > > > + }; > > > > + > > > > + vdev0vring0: vdev0vring0@55000000 { > > > > + no-map; > > > > + reg = <0x55000000 0x8000>; > > > > + }; > > > > + > > > > + vdev0vring1: vdev0vring1@55008000 { > > > > + reg = <0x55008000 0x8000>; > > > > + no-map; > > > > + }; > > > > + > > > > + vdevbuffer: vdevbuffer@55400000 { > > > > + 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. 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% > > > > > 2Fel > > > > > ixir.b > > > > > > > > > ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre > > > > > > > > > moteproc_core.c%23L321&data=04%7C01%7Cpeng.fan%40nxp.com%7 > > > > > > > > > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > > > > > > > > > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > > > > > > > > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > > > > > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAItqZ > m66 > > > b4 > > > > > m20%3D&reserved=0 > > > > > [2]. > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F% > > > > > 2Fpa > > > > > tch > > > > > > > > > work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315 > > > > > > > > > 43.7024-1-peng.fan%40nxp.com%2F&data=04%7C01%7Cpeng.fan%40n > > > > > > > > > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > > > > > > > > > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > > > > > > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > > > > > > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > > > > > FHjwA6a8x58geY7eE%3D&reserved=0 > > > > > > > > > > > err = of_address_to_resource(node, 0, &res); > > > > > > if (err) { > > > > > > dev_err(dev, "unable to resolve memory region\n"); > > > > > > -- > > > > > > 2.28.0 > > > > > >
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@nxp.com > > wrote: > > > > > > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { > > > > > + no-map; > > > > > + reg = <0x550ff000 0x1000>; > > > > > + }; > > > > > + > > > > > + vdev0vring0: vdev0vring0@55000000 { > > > > > + no-map; > > > > > + reg = <0x55000000 0x8000>; > > > > > + }; > > > > > + > > > > > + vdev0vring1: vdev0vring1@55008000 { > > > > > + reg = <0x55008000 0x8000>; > > > > > + no-map; > > > > > + }; > > > > > + > > > > > + vdevbuffer: vdevbuffer@55400000 { > > > > > + 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, > 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% > > > > > > 2Fel > > > > > > ixir.b > > > > > > > > > > > > ootlin.com%2Flinux%2Fv5.11-rc3%2Fsource%2Fdrivers%2Fremoteproc%2Fre > > > > > > > > > > > > moteproc_core.c%23L321&data=04%7C01%7Cpeng.fan%40nxp.com%7 > > > > > > > > > > > > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > > > > > > > > > > > > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > > > > > > > > > > > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > > > > > > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAItqZ > > m66 > > > > b4 > > > > > > m20%3D&reserved=0 > > > > > > [2]. > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F% > > > > > > 2Fpa > > > > > > tch > > > > > > > > > > > > work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2F202007221315 > > > > > > > > > > > > 43.7024-1-peng.fan%40nxp.com%2F&data=04%7C01%7Cpeng.fan%40n > > > > > > > > > > > > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > > > > > > > > > > > > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > > > > > > > > > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > > > > > > > > > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > > > > > > FHjwA6a8x58geY7eE%3D&reserved=0 > > > > > > > > > > > > > err = of_address_to_resource(node, 0, &res); > > > > > > > if (err) { > > > > > > > dev_err(dev, "unable to resolve memory region\n"); > > > > > > > -- > > > > > > > 2.28.0 > > > > > > >
> 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@nxp.com > > > wrote: > > > > > > > > From: Peng Fan <peng.fan@nxp.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@nxp.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@550ff000 { > > > > > > + no-map; > > > > > > + reg = <0x550ff000 0x1000>; > > > > > > + }; > > > > > > + > > > > > > + vdev0vring0: vdev0vring0@55000000 { > > > > > > + no-map; > > > > > > + reg = <0x55000000 0x8000>; > > > > > > + }; > > > > > > + > > > > > > + vdev0vring1: vdev0vring1@55008000 { > > > > > > + reg = <0x55008000 0x8000>; > > > > > > + no-map; > > > > > > + }; > > > > > > + > > > > > > + vdevbuffer: vdevbuffer@55400000 { > > > > > > + 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&data=04%7C01%7Cpeng.fan%40nxp.com%7 > > > > > > > > > > > > > > > > C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa92cd99c5c > > > > > > > > > > > > > > > > 301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTWFpbGZsb3 > > > > > > > > > > > > > > > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > > > > > > > %3D%7C1000&sdata=Qur6YiTWlak0ZRnrUZRzawfoO38EBrAIt > qZ > > > m66 > > > > > b4 > > > > > > > m20%3D&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&data=04%7C01%7Cpeng.fan%40n > > > > > > > > > > > > > > > > xp.com%7C581784529b1646b9d34b08d8b67ae8c7%7C686ea1d3bc2b4c6fa9 > > > > > > > > > > > > > > > > 2cd99c5c301635%7C0%7C0%7C637459986311799770%7CUnknown%7CTW > > > > > > > > > > > > > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > > > > > > > > > > > > > VCI6Mn0%3D%7C1000&sdata=b%2F8muWtb3yxKIsnXmKmRGYYV33%2 > > > > > > > FHjwA6a8x58geY7eE%3D&reserved=0 > > > > > > > > > > > > > > > err = of_address_to_resource(node, 0, &res); > > > > > > > > if (err) { > > > > > > > > dev_err(dev, "unable to resolve memory > region\n"); > > > > > > > > -- > > > > > > > > 2.28.0 > > > > > > > >
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; err = of_address_to_resource(node, 0, &res); if (err) { dev_err(dev, "unable to resolve memory region\n");