mbox series

[v8,0/4] remoteproc: restructure the remoteproc VirtIO device

Message ID 20220826115232.2163130-1-arnaud.pouliquen@foss.st.com (mailing list archive)
Headers show
Series remoteproc: restructure the remoteproc VirtIO device | expand

Message

Arnaud POULIQUEN Aug. 26, 2022, 11:52 a.m. UTC
1) Update from V7 [1]:

- rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc: imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
  The updates take into account the integration of the
  commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after rproc_shutdown")
- add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according to reviews on V7


[1] https://lkml.org/lkml/2022/7/13/663
[2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next

2) Patchset description:

This series is a part of the work initiated a long time ago in 
the series "remoteproc: Decorelate virtio from core"[3]

Objective of the work:
- Update the remoteproc VirtIO device creation (use platform device)
- Allow to declare remoteproc VirtIO device in DT
    - declare resources associated to a remote proc VirtIO
    - declare a list of VirtIO supported by the platform.
- Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
  For instance be able to declare a I2C device in a virtio-i2C node.
- Keep the legacy working!
- Try to improve the picture about concerns reported by Christoph Hellwing [4][5]

[3] https://lkml.org/lkml/2020/4/16/1817
[4] https://lkml.org/lkml/2021/6/23/607
[5] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/

In term of device tree this would result in such hierarchy (stm32mp1 example with 2 virtio RPMSG):

	m4_rproc: m4@10000000 {
		compatible = "st,stm32mp1-m4";
		reg = <0x10000000 0x40000>,
		      <0x30000000 0x40000>,
		      <0x38000000 0x10000>;
        memory-region = <&retram>, <&mcuram>,<&mcuram2>;
        mboxes = <&ipcc 2>, <&ipcc 3>;
        mbox-names = "shutdown", "detach";
        status = "okay";

        #address-cells = <1>;
        #size-cells = <0>;
        
        vdev@0 {
		compatible = "rproc-virtio";
		reg = <0>;
		virtio,id = <7>;  /* RPMSG */
		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
		mboxes = <&ipcc 0>, <&ipcc 1>;
		mbox-names = "vq0", "vq1";
		status = "okay";
        };

        vdev@1 {
		compatible = "rproc-virtio";
		reg = <1>;
		virtio,id = <7>;  /*RPMSG */
		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
		mboxes = <&ipcc 4>, <&ipcc 5>;
		mbox-names = "vq0", "vq1";
		status = "okay";
        };
};

I have divided the work in 4 steps to simplify the review, This series implements only
the step 1:
step 1: Redefine the remoteproc VirtIO device as a platform device
  - migrate rvdev management in remoteproc virtio.c,
  - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
step 2: Add possibility to declare and probe a VirtIO sub node
  - VirtIO bindings declaration,
  - multi DT VirtIO devices support,
  - introduction of a remote proc virtio bind device mechanism ,
=> https://github.com/arnopo/linux/commits/step2-virtio-in-DT
step 3: Add memory declaration in VirtIO subnode
=> https://github.com/arnopo/linux/commits/step3-virtio-memories
step 4: Add mailbox declaration in VirtIO subnode
=> https://github.com/arnopo/linux/commits/step4-virtio-mailboxes

Arnaud Pouliquen (4):
  remoteproc: core: Introduce rproc_rvdev_add_device function
  remoteproc: core: Introduce rproc_add_rvdev function
  remoteproc: Move rproc_vdev management to remoteproc_virtio.c
  remoteproc: virtio: Create platform device for the remoteproc_virtio

 drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
 drivers/remoteproc/remoteproc_internal.h |  23 ++-
 drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
 include/linux/remoteproc.h               |   6 +-
 4 files changed, 210 insertions(+), 162 deletions(-)

Comments

Mathieu Poirier Sept. 19, 2022, 10:30 p.m. UTC | #1
Hi,

On Fri, Aug 26, 2022 at 01:52:28PM +0200, Arnaud Pouliquen wrote:
> 1) Update from V7 [1]:
> 
> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc: imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
>   The updates take into account the integration of the
>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after rproc_shutdown")
> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according to reviews on V7
> 
> 
> [1] https://lkml.org/lkml/2022/7/13/663
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next
> 
> 2) Patchset description:
> 
> This series is a part of the work initiated a long time ago in 
> the series "remoteproc: Decorelate virtio from core"[3]
> 
> Objective of the work:
> - Update the remoteproc VirtIO device creation (use platform device)
> - Allow to declare remoteproc VirtIO device in DT
>     - declare resources associated to a remote proc VirtIO
>     - declare a list of VirtIO supported by the platform.
> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
>   For instance be able to declare a I2C device in a virtio-i2C node.
> - Keep the legacy working!
> - Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
> 
> [3] https://lkml.org/lkml/2020/4/16/1817
> [4] https://lkml.org/lkml/2021/6/23/607
> [5] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/
> 
> In term of device tree this would result in such hierarchy (stm32mp1 example with 2 virtio RPMSG):
> 
> 	m4_rproc: m4@10000000 {
> 		compatible = "st,stm32mp1-m4";
> 		reg = <0x10000000 0x40000>,
> 		      <0x30000000 0x40000>,
> 		      <0x38000000 0x10000>;
>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
>         mboxes = <&ipcc 2>, <&ipcc 3>;
>         mbox-names = "shutdown", "detach";
>         status = "okay";
> 
>         #address-cells = <1>;
>         #size-cells = <0>;
>         
>         vdev@0 {
> 		compatible = "rproc-virtio";
> 		reg = <0>;
> 		virtio,id = <7>;  /* RPMSG */
> 		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
> 		mboxes = <&ipcc 0>, <&ipcc 1>;
> 		mbox-names = "vq0", "vq1";
> 		status = "okay";
>         };
> 
>         vdev@1 {
> 		compatible = "rproc-virtio";
> 		reg = <1>;
> 		virtio,id = <7>;  /*RPMSG */
> 		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
> 		mboxes = <&ipcc 4>, <&ipcc 5>;
> 		mbox-names = "vq0", "vq1";
> 		status = "okay";
>         };
> };

I was in the process of applying this set when the last patch gave me a
checkpatch warning about "virtio,rproc" not being documented.

I suggest to introduce a new "virtio-rproc.yaml" based on this work[1], with the
above in the example sections.

Thanks,
Mathieu

[1]. https://elixir.bootlin.com/linux/v6.0-rc6/source/Documentation/devicetree/bindings/virtio/virtio-device.yaml


> 
> I have divided the work in 4 steps to simplify the review, This series implements only
> the step 1:
> step 1: Redefine the remoteproc VirtIO device as a platform device
>   - migrate rvdev management in remoteproc virtio.c,
>   - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
> step 2: Add possibility to declare and probe a VirtIO sub node
>   - VirtIO bindings declaration,
>   - multi DT VirtIO devices support,
>   - introduction of a remote proc virtio bind device mechanism ,
> => https://github.com/arnopo/linux/commits/step2-virtio-in-DT
> step 3: Add memory declaration in VirtIO subnode
> => https://github.com/arnopo/linux/commits/step3-virtio-memories
> step 4: Add mailbox declaration in VirtIO subnode
> => https://github.com/arnopo/linux/commits/step4-virtio-mailboxes
> 
> Arnaud Pouliquen (4):
>   remoteproc: core: Introduce rproc_rvdev_add_device function
>   remoteproc: core: Introduce rproc_add_rvdev function
>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
>   remoteproc: virtio: Create platform device for the remoteproc_virtio
> 
>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
>  include/linux/remoteproc.h               |   6 +-
>  4 files changed, 210 insertions(+), 162 deletions(-)
> 
> -- 
> 2.24.3
>
Arnaud POULIQUEN Sept. 20, 2022, 1:44 p.m. UTC | #2
Hi Mathieu,

On 9/20/22 00:30, Mathieu Poirier wrote:
> Hi,
> 
> On Fri, Aug 26, 2022 at 01:52:28PM +0200, Arnaud Pouliquen wrote:
>> 1) Update from V7 [1]:
>>
>> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc: imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
>>   The updates take into account the integration of the
>>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after rproc_shutdown")
>> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according to reviews on V7
>>
>>
>> [1] https://lkml.org/lkml/2022/7/13/663
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next
>>
>> 2) Patchset description:
>>
>> This series is a part of the work initiated a long time ago in 
>> the series "remoteproc: Decorelate virtio from core"[3]
>>
>> Objective of the work:
>> - Update the remoteproc VirtIO device creation (use platform device)
>> - Allow to declare remoteproc VirtIO device in DT
>>     - declare resources associated to a remote proc VirtIO
>>     - declare a list of VirtIO supported by the platform.
>> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
>>   For instance be able to declare a I2C device in a virtio-i2C node.
>> - Keep the legacy working!
>> - Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
>>
>> [3] https://lkml.org/lkml/2020/4/16/1817
>> [4] https://lkml.org/lkml/2021/6/23/607
>> [5] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/
>>
>> In term of device tree this would result in such hierarchy (stm32mp1 example with 2 virtio RPMSG):
>>
>> 	m4_rproc: m4@10000000 {
>> 		compatible = "st,stm32mp1-m4";
>> 		reg = <0x10000000 0x40000>,
>> 		      <0x30000000 0x40000>,
>> 		      <0x38000000 0x10000>;
>>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
>>         mboxes = <&ipcc 2>, <&ipcc 3>;
>>         mbox-names = "shutdown", "detach";
>>         status = "okay";
>>
>>         #address-cells = <1>;
>>         #size-cells = <0>;
>>         
>>         vdev@0 {
>> 		compatible = "rproc-virtio";
>> 		reg = <0>;
>> 		virtio,id = <7>;  /* RPMSG */
>> 		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
>> 		mboxes = <&ipcc 0>, <&ipcc 1>;
>> 		mbox-names = "vq0", "vq1";
>> 		status = "okay";
>>         };
>>
>>         vdev@1 {
>> 		compatible = "rproc-virtio";
>> 		reg = <1>;
>> 		virtio,id = <7>;  /*RPMSG */
>> 		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
>> 		mboxes = <&ipcc 4>, <&ipcc 5>;
>> 		mbox-names = "vq0", "vq1";
>> 		status = "okay";
>>         };
>> };
> 
> I was in the process of applying this set when the last patch gave me a
> checkpatch warning about "virtio,rproc" not being documented.
> 
> I suggest to introduce a new "virtio-rproc.yaml" based on this work[1], with the
> above in the example sections.

Yes I saw the warning, but for this first series it is not possible to declare
the associated "rproc-virtio" device  in device tree.
So at this step it seems not make senses to create the devicetree bindings file.
More than that I don't know how I could justify the properties in bindings if
there is not driver code associated.

So i would be in favor of not adding the bindings in this series but to define
bindings in the first patch of my "step 2" series; as done on my github:
https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f

Please let me know your preference.

Regards,
Arnaud


> 
> Thanks,
> Mathieu
> 
> [1]. https://elixir.bootlin.com/linux/v6.0-rc6/source/Documentation/devicetree/bindings/virtio/virtio-device.yaml
> 
> 
>>
>> I have divided the work in 4 steps to simplify the review, This series implements only
>> the step 1:
>> step 1: Redefine the remoteproc VirtIO device as a platform device
>>   - migrate rvdev management in remoteproc virtio.c,
>>   - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
>> step 2: Add possibility to declare and probe a VirtIO sub node
>>   - VirtIO bindings declaration,
>>   - multi DT VirtIO devices support,
>>   - introduction of a remote proc virtio bind device mechanism ,
>> => https://github.com/arnopo/linux/commits/step2-virtio-in-DT
>> step 3: Add memory declaration in VirtIO subnode
>> => https://github.com/arnopo/linux/commits/step3-virtio-memories
>> step 4: Add mailbox declaration in VirtIO subnode
>> => https://github.com/arnopo/linux/commits/step4-virtio-mailboxes
>>
>> Arnaud Pouliquen (4):
>>   remoteproc: core: Introduce rproc_rvdev_add_device function
>>   remoteproc: core: Introduce rproc_add_rvdev function
>>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
>>   remoteproc: virtio: Create platform device for the remoteproc_virtio
>>
>>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
>>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
>>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
>>  include/linux/remoteproc.h               |   6 +-
>>  4 files changed, 210 insertions(+), 162 deletions(-)
>>
>> -- 
>> 2.24.3
>>
Mathieu Poirier Sept. 20, 2022, 8:22 p.m. UTC | #3
On Tue, Sep 20, 2022 at 03:44:18PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 9/20/22 00:30, Mathieu Poirier wrote:
> > Hi,
> > 
> > On Fri, Aug 26, 2022 at 01:52:28PM +0200, Arnaud Pouliquen wrote:
> >> 1) Update from V7 [1]:
> >>
> >> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc: imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
> >>   The updates take into account the integration of the
> >>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after rproc_shutdown")
> >> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according to reviews on V7
> >>
> >>
> >> [1] https://lkml.org/lkml/2022/7/13/663
> >> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next
> >>
> >> 2) Patchset description:
> >>
> >> This series is a part of the work initiated a long time ago in 
> >> the series "remoteproc: Decorelate virtio from core"[3]
> >>
> >> Objective of the work:
> >> - Update the remoteproc VirtIO device creation (use platform device)
> >> - Allow to declare remoteproc VirtIO device in DT
> >>     - declare resources associated to a remote proc VirtIO
> >>     - declare a list of VirtIO supported by the platform.
> >> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
> >>   For instance be able to declare a I2C device in a virtio-i2C node.
> >> - Keep the legacy working!
> >> - Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
> >>
> >> [3] https://lkml.org/lkml/2020/4/16/1817
> >> [4] https://lkml.org/lkml/2021/6/23/607
> >> [5] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/
> >>
> >> In term of device tree this would result in such hierarchy (stm32mp1 example with 2 virtio RPMSG):
> >>
> >> 	m4_rproc: m4@10000000 {
> >> 		compatible = "st,stm32mp1-m4";
> >> 		reg = <0x10000000 0x40000>,
> >> 		      <0x30000000 0x40000>,
> >> 		      <0x38000000 0x10000>;
> >>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
> >>         mboxes = <&ipcc 2>, <&ipcc 3>;
> >>         mbox-names = "shutdown", "detach";
> >>         status = "okay";
> >>
> >>         #address-cells = <1>;
> >>         #size-cells = <0>;
> >>         
> >>         vdev@0 {
> >> 		compatible = "rproc-virtio";
> >> 		reg = <0>;
> >> 		virtio,id = <7>;  /* RPMSG */
> >> 		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
> >> 		mboxes = <&ipcc 0>, <&ipcc 1>;
> >> 		mbox-names = "vq0", "vq1";
> >> 		status = "okay";
> >>         };
> >>
> >>         vdev@1 {
> >> 		compatible = "rproc-virtio";
> >> 		reg = <1>;
> >> 		virtio,id = <7>;  /*RPMSG */
> >> 		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
> >> 		mboxes = <&ipcc 4>, <&ipcc 5>;
> >> 		mbox-names = "vq0", "vq1";
> >> 		status = "okay";
> >>         };
> >> };
> > 
> > I was in the process of applying this set when the last patch gave me a
> > checkpatch warning about "virtio,rproc" not being documented.
> > 
> > I suggest to introduce a new "virtio-rproc.yaml" based on this work[1], with the
> > above in the example sections.
> 
> Yes I saw the warning, but for this first series it is not possible to declare
> the associated "rproc-virtio" device  in device tree.

I understand and agree with your position.

I am going ahead and merging this set in order for it to get some exposure in
linux-next.  That said be on the ready to address potential problems it may
cause.

> So at this step it seems not make senses to create the devicetree bindings file.
> More than that I don't know how I could justify the properties in bindings if
> there is not driver code associated.
> 
> So i would be in favor of not adding the bindings in this series but to define
> bindings in the first patch of my "step 2" series; as done on my github:
> https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
> 
> Please let me know your preference.
> 
> Regards,
> Arnaud
> 
> 
> > 
> > Thanks,
> > Mathieu
> > 
> > [1]. https://elixir.bootlin.com/linux/v6.0-rc6/source/Documentation/devicetree/bindings/virtio/virtio-device.yaml
> > 
> > 
> >>
> >> I have divided the work in 4 steps to simplify the review, This series implements only
> >> the step 1:
> >> step 1: Redefine the remoteproc VirtIO device as a platform device
> >>   - migrate rvdev management in remoteproc virtio.c,
> >>   - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
> >> step 2: Add possibility to declare and probe a VirtIO sub node
> >>   - VirtIO bindings declaration,
> >>   - multi DT VirtIO devices support,
> >>   - introduction of a remote proc virtio bind device mechanism ,
> >> => https://github.com/arnopo/linux/commits/step2-virtio-in-DT
> >> step 3: Add memory declaration in VirtIO subnode
> >> => https://github.com/arnopo/linux/commits/step3-virtio-memories
> >> step 4: Add mailbox declaration in VirtIO subnode
> >> => https://github.com/arnopo/linux/commits/step4-virtio-mailboxes
> >>
> >> Arnaud Pouliquen (4):
> >>   remoteproc: core: Introduce rproc_rvdev_add_device function
> >>   remoteproc: core: Introduce rproc_add_rvdev function
> >>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
> >>   remoteproc: virtio: Create platform device for the remoteproc_virtio
> >>
> >>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
> >>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
> >>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
> >>  include/linux/remoteproc.h               |   6 +-
> >>  4 files changed, 210 insertions(+), 162 deletions(-)
> >>
> >> -- 
> >> 2.24.3
> >>
Mathieu Poirier Sept. 20, 2022, 8:51 p.m. UTC | #4
On Tue, Sep 20, 2022 at 02:22:01PM -0600, Mathieu Poirier wrote:
> On Tue, Sep 20, 2022 at 03:44:18PM +0200, Arnaud POULIQUEN wrote:
> > Hi Mathieu,
> > 
> > On 9/20/22 00:30, Mathieu Poirier wrote:
> > > Hi,
> > > 
> > > On Fri, Aug 26, 2022 at 01:52:28PM +0200, Arnaud Pouliquen wrote:
> > >> 1) Update from V7 [1]:
> > >>
> > >> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc: imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
> > >>   The updates take into account the integration of the
> > >>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after rproc_shutdown")
> > >> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according to reviews on V7
> > >>
> > >>
> > >> [1] https://lkml.org/lkml/2022/7/13/663
> > >> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next
> > >>
> > >> 2) Patchset description:
> > >>
> > >> This series is a part of the work initiated a long time ago in 
> > >> the series "remoteproc: Decorelate virtio from core"[3]
> > >>
> > >> Objective of the work:
> > >> - Update the remoteproc VirtIO device creation (use platform device)
> > >> - Allow to declare remoteproc VirtIO device in DT
> > >>     - declare resources associated to a remote proc VirtIO
> > >>     - declare a list of VirtIO supported by the platform.
> > >> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
> > >>   For instance be able to declare a I2C device in a virtio-i2C node.
> > >> - Keep the legacy working!
> > >> - Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
> > >>
> > >> [3] https://lkml.org/lkml/2020/4/16/1817
> > >> [4] https://lkml.org/lkml/2021/6/23/607
> > >> [5] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/
> > >>
> > >> In term of device tree this would result in such hierarchy (stm32mp1 example with 2 virtio RPMSG):
> > >>
> > >> 	m4_rproc: m4@10000000 {
> > >> 		compatible = "st,stm32mp1-m4";
> > >> 		reg = <0x10000000 0x40000>,
> > >> 		      <0x30000000 0x40000>,
> > >> 		      <0x38000000 0x10000>;
> > >>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
> > >>         mboxes = <&ipcc 2>, <&ipcc 3>;
> > >>         mbox-names = "shutdown", "detach";
> > >>         status = "okay";
> > >>
> > >>         #address-cells = <1>;
> > >>         #size-cells = <0>;
> > >>         
> > >>         vdev@0 {
> > >> 		compatible = "rproc-virtio";
> > >> 		reg = <0>;
> > >> 		virtio,id = <7>;  /* RPMSG */
> > >> 		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
> > >> 		mboxes = <&ipcc 0>, <&ipcc 1>;
> > >> 		mbox-names = "vq0", "vq1";
> > >> 		status = "okay";
> > >>         };
> > >>
> > >>         vdev@1 {
> > >> 		compatible = "rproc-virtio";
> > >> 		reg = <1>;
> > >> 		virtio,id = <7>;  /*RPMSG */
> > >> 		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
> > >> 		mboxes = <&ipcc 4>, <&ipcc 5>;
> > >> 		mbox-names = "vq0", "vq1";
> > >> 		status = "okay";
> > >>         };
> > >> };
> > > 
> > > I was in the process of applying this set when the last patch gave me a
> > > checkpatch warning about "virtio,rproc" not being documented.
> > > 
> > > I suggest to introduce a new "virtio-rproc.yaml" based on this work[1], with the
> > > above in the example sections.
> > 
> > Yes I saw the warning, but for this first series it is not possible to declare
> > the associated "rproc-virtio" device  in device tree.
> 
> I understand and agree with your position.
> 
> I am going ahead and merging this set in order for it to get some exposure in
> linux-next.  That said be on the ready to address potential problems it may
> cause.

I am getting conflicts because of the patches previously applied to rproc-next.
Please resent a series that applies to "7d7f8fe4e399" and I'll move forward with
the merge.

> 
> > So at this step it seems not make senses to create the devicetree bindings file.
> > More than that I don't know how I could justify the properties in bindings if
> > there is not driver code associated.
> > 
> > So i would be in favor of not adding the bindings in this series but to define
> > bindings in the first patch of my "step 2" series; as done on my github:
> > https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
> > 
> > Please let me know your preference.
> > 
> > Regards,
> > Arnaud
> > 
> > 
> > > 
> > > Thanks,
> > > Mathieu
> > > 
> > > [1]. https://elixir.bootlin.com/linux/v6.0-rc6/source/Documentation/devicetree/bindings/virtio/virtio-device.yaml
> > > 
> > > 
> > >>
> > >> I have divided the work in 4 steps to simplify the review, This series implements only
> > >> the step 1:
> > >> step 1: Redefine the remoteproc VirtIO device as a platform device
> > >>   - migrate rvdev management in remoteproc virtio.c,
> > >>   - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
> > >> step 2: Add possibility to declare and probe a VirtIO sub node
> > >>   - VirtIO bindings declaration,
> > >>   - multi DT VirtIO devices support,
> > >>   - introduction of a remote proc virtio bind device mechanism ,
> > >> => https://github.com/arnopo/linux/commits/step2-virtio-in-DT
> > >> step 3: Add memory declaration in VirtIO subnode
> > >> => https://github.com/arnopo/linux/commits/step3-virtio-memories
> > >> step 4: Add mailbox declaration in VirtIO subnode
> > >> => https://github.com/arnopo/linux/commits/step4-virtio-mailboxes
> > >>
> > >> Arnaud Pouliquen (4):
> > >>   remoteproc: core: Introduce rproc_rvdev_add_device function
> > >>   remoteproc: core: Introduce rproc_add_rvdev function
> > >>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
> > >>   remoteproc: virtio: Create platform device for the remoteproc_virtio
> > >>
> > >>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
> > >>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
> > >>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
> > >>  include/linux/remoteproc.h               |   6 +-
> > >>  4 files changed, 210 insertions(+), 162 deletions(-)
> > >>
> > >> -- 
> > >> 2.24.3
> > >>
Peng Fan Sept. 21, 2022, 8:54 a.m. UTC | #5
Hi Arnaud,

> Subject: [PATCH v8 0/4] remoteproc: restructure the remoteproc VirtIO
> device

Sorry to get in at this late time, just try to catch up.
Not reviewing comments, just have a question,
Does remote core firmware requires changes to use this new feature?
Does your 4 branches listed below still work with linux-6.x?
Could the multiple vdev still share same mbox channel?

I not own i.MX remote core firmware development, so if no need
firmware change, I would like give a try and see how it works.

Thanks,
Peng.

> 
> 1) Update from V7 [1]:
> 
> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc:
> imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
>   The updates take into account the integration of the
>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after
> rproc_shutdown")
> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according
> to reviews on V7
> 
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> org%2Flkml%2F2022%2F7%2F13%2F663&amp;data=05%7C01%7Cpeng.fan%
> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=77pEuwAI7Lh61hx1%2B
> Hs79Cu0G5KOa6mzQ0PnTC5r8Xk%3D&amp;reserved=0
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ke
> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fremoteproc%2Flinux.g
> it%2Flog%2F%3Fh%3Dfor-
> next&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e7
> 439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&amp;sdata=iWUSzKkN9BpHwqbO62awIcyVf9PXcftcdt2kytWVR78%3D
> &amp;reserved=0
> 
> 2) Patchset description:
> 
> This series is a part of the work initiated a long time ago in the series
> "remoteproc: Decorelate virtio from core"[3]
> 
> Objective of the work:
> - Update the remoteproc VirtIO device creation (use platform device)
> - Allow to declare remoteproc VirtIO device in DT
>     - declare resources associated to a remote proc VirtIO
>     - declare a list of VirtIO supported by the platform.
> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
>   For instance be able to declare a I2C device in a virtio-i2C node.
> - Keep the legacy working!
> - Try to improve the picture about concerns reported by Christoph Hellwing
> [4][5]
> 
> [3]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> org%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=05%7C01%7Cpeng.fan
> %40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
> iLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=oPWSfUweLdhUFK5X9
> 2YcGHem8s%2Bfelcr%2FHx9JAlKG%2BI%3D&amp;reserved=0
> [4]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> org%2Flkml%2F2021%2F6%2F23%2F607&amp;data=05%7C01%7Cpeng.fan%
> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HPpnlaykes8R1Kz1dEN
> nirEHkDNr7JvRs%2FcsaDPuLdI%3D&amp;reserved=0
> [5]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> hwork.kernel.org%2Fproject%2Flinux-
> remoteproc%2Fpatch%2FAOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E
> %40cp7-web-
> 042.plabs.ch%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e520
> 0d739a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&amp;sdata=GtNruefDreOoogL%2BlntAC7GBfk6E1Goq4j%
> 2BYXt36RdI%3D&amp;reserved=0
> 
> In term of device tree this would result in such hierarchy (stm32mp1
> example with 2 virtio RPMSG):
> 
> 	m4_rproc: m4@10000000 {
> 		compatible = "st,stm32mp1-m4";
> 		reg = <0x10000000 0x40000>,
> 		      <0x30000000 0x40000>,
> 		      <0x38000000 0x10000>;
>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
>         mboxes = <&ipcc 2>, <&ipcc 3>;
>         mbox-names = "shutdown", "detach";
>         status = "okay";
> 
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         vdev@0 {
> 		compatible = "rproc-virtio";
> 		reg = <0>;
> 		virtio,id = <7>;  /* RPMSG */
> 		memory-region = <&vdev0vring0>, <&vdev0vring1>,
> <&vdev0buffer>;
> 		mboxes = <&ipcc 0>, <&ipcc 1>;
> 		mbox-names = "vq0", "vq1";
> 		status = "okay";
>         };
> 
>         vdev@1 {
> 		compatible = "rproc-virtio";
> 		reg = <1>;
> 		virtio,id = <7>;  /*RPMSG */
> 		memory-region = <&vdev1vring0>, <&vdev1vring1>,
> <&vdev1buffer>;
> 		mboxes = <&ipcc 4>, <&ipcc 5>;
> 		mbox-names = "vq0", "vq1";
> 		status = "okay";
>         };
> };
> 
> I have divided the work in 4 steps to simplify the review, This series
> implements only the step 1:
> step 1: Redefine the remoteproc VirtIO device as a platform device
>   - migrate rvdev management in remoteproc virtio.c,
>   - create a remotproc virtio config ( can be disabled for platform that not
> use VirtIO IPC.
> step 2: Add possibility to declare and probe a VirtIO sub node
>   - VirtIO bindings declaration,
>   - multi DT VirtIO devices support,
>   - introduction of a remote proc virtio bind device mechanism , =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-
> DT&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e74
> 39508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&amp;sdata=XtF%2FQnml3QXFL7rgqST1Z2FotUzoj%2FD57WfiuAVMnr8
> %3D&amp;reserved=0
> step 3: Add memory declaration in VirtIO subnode =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-
> memories&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> 7C%7C%7C&amp;sdata=6gq28c6a1TJ%2FdkvokcEjgy6FKQcKTXSz%2BNAbJPo
> mjac%3D&amp;reserved=0
> step 4: Add mailbox declaration in VirtIO subnode =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-
> mailboxes&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> 7C%7C%7C&amp;sdata=wfy2euuOPoMmBMIH3BOsGcsEYGSTWsDaRr7ENN
> QCK70%3D&amp;reserved=0
> 
> Arnaud Pouliquen (4):
>   remoteproc: core: Introduce rproc_rvdev_add_device function
>   remoteproc: core: Introduce rproc_add_rvdev function
>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
>   remoteproc: virtio: Create platform device for the remoteproc_virtio
> 
>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
>  include/linux/remoteproc.h               |   6 +-
>  4 files changed, 210 insertions(+), 162 deletions(-)
> 
> --
> 2.24.3
Arnaud POULIQUEN Sept. 21, 2022, 1:56 p.m. UTC | #6
Hi Mathieu,

On 9/20/22 22:51, Mathieu Poirier wrote:
> On Tue, Sep 20, 2022 at 02:22:01PM -0600, Mathieu Poirier wrote:
>> On Tue, Sep 20, 2022 at 03:44:18PM +0200, Arnaud POULIQUEN wrote:
>>> Hi Mathieu,
>>>
>>> On 9/20/22 00:30, Mathieu Poirier wrote:
>>>> Hi,
>>>>
>>>> On Fri, Aug 26, 2022 at 01:52:28PM +0200, Arnaud Pouliquen wrote:
>>>>> 1) Update from V7 [1]:
>>>>>
>>>>> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc: imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
>>>>>   The updates take into account the integration of the
>>>>>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after rproc_shutdown")
>>>>> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according to reviews on V7
>>>>>
>>>>>
>>>>> [1] https://lkml.org/lkml/2022/7/13/663
>>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next
>>>>>
>>>>> 2) Patchset description:
>>>>>
>>>>> This series is a part of the work initiated a long time ago in 
>>>>> the series "remoteproc: Decorelate virtio from core"[3]
>>>>>
>>>>> Objective of the work:
>>>>> - Update the remoteproc VirtIO device creation (use platform device)
>>>>> - Allow to declare remoteproc VirtIO device in DT
>>>>>     - declare resources associated to a remote proc VirtIO
>>>>>     - declare a list of VirtIO supported by the platform.
>>>>> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
>>>>>   For instance be able to declare a I2C device in a virtio-i2C node.
>>>>> - Keep the legacy working!
>>>>> - Try to improve the picture about concerns reported by Christoph Hellwing [4][5]
>>>>>
>>>>> [3] https://lkml.org/lkml/2020/4/16/1817
>>>>> [4] https://lkml.org/lkml/2021/6/23/607
>>>>> [5] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/
>>>>>
>>>>> In term of device tree this would result in such hierarchy (stm32mp1 example with 2 virtio RPMSG):
>>>>>
>>>>> 	m4_rproc: m4@10000000 {
>>>>> 		compatible = "st,stm32mp1-m4";
>>>>> 		reg = <0x10000000 0x40000>,
>>>>> 		      <0x30000000 0x40000>,
>>>>> 		      <0x38000000 0x10000>;
>>>>>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
>>>>>         mboxes = <&ipcc 2>, <&ipcc 3>;
>>>>>         mbox-names = "shutdown", "detach";
>>>>>         status = "okay";
>>>>>
>>>>>         #address-cells = <1>;
>>>>>         #size-cells = <0>;
>>>>>         
>>>>>         vdev@0 {
>>>>> 		compatible = "rproc-virtio";
>>>>> 		reg = <0>;
>>>>> 		virtio,id = <7>;  /* RPMSG */
>>>>> 		memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
>>>>> 		mboxes = <&ipcc 0>, <&ipcc 1>;
>>>>> 		mbox-names = "vq0", "vq1";
>>>>> 		status = "okay";
>>>>>         };
>>>>>
>>>>>         vdev@1 {
>>>>> 		compatible = "rproc-virtio";
>>>>> 		reg = <1>;
>>>>> 		virtio,id = <7>;  /*RPMSG */
>>>>> 		memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
>>>>> 		mboxes = <&ipcc 4>, <&ipcc 5>;
>>>>> 		mbox-names = "vq0", "vq1";
>>>>> 		status = "okay";
>>>>>         };
>>>>> };
>>>>
>>>> I was in the process of applying this set when the last patch gave me a
>>>> checkpatch warning about "virtio,rproc" not being documented.
>>>>
>>>> I suggest to introduce a new "virtio-rproc.yaml" based on this work[1], with the
>>>> above in the example sections.
>>>
>>> Yes I saw the warning, but for this first series it is not possible to declare
>>> the associated "rproc-virtio" device  in device tree.
>>
>> I understand and agree with your position.
>>
>> I am going ahead and merging this set in order for it to get some exposure in
>> linux-next.  That said be on the ready to address potential problems it may
>> cause.

Yes sure!

> 
> I am getting conflicts because of the patches previously applied to rproc-next.
> Please resent a series that applies to "7d7f8fe4e399" and I'll move forward with
> the merge.
> 

I just sent the V9 to address the rebase.

Thanks,
Arnaud


>>
>>> So at this step it seems not make senses to create the devicetree bindings file.
>>> More than that I don't know how I could justify the properties in bindings if
>>> there is not driver code associated.
>>>
>>> So i would be in favor of not adding the bindings in this series but to define
>>> bindings in the first patch of my "step 2" series; as done on my github:
>>> https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
>>>
>>> Please let me know your preference.
>>>
>>> Regards,
>>> Arnaud
>>>
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>> [1]. https://elixir.bootlin.com/linux/v6.0-rc6/source/Documentation/devicetree/bindings/virtio/virtio-device.yaml
>>>>
>>>>
>>>>>
>>>>> I have divided the work in 4 steps to simplify the review, This series implements only
>>>>> the step 1:
>>>>> step 1: Redefine the remoteproc VirtIO device as a platform device
>>>>>   - migrate rvdev management in remoteproc virtio.c,
>>>>>   - create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
>>>>> step 2: Add possibility to declare and probe a VirtIO sub node
>>>>>   - VirtIO bindings declaration,
>>>>>   - multi DT VirtIO devices support,
>>>>>   - introduction of a remote proc virtio bind device mechanism ,
>>>>> => https://github.com/arnopo/linux/commits/step2-virtio-in-DT
>>>>> step 3: Add memory declaration in VirtIO subnode
>>>>> => https://github.com/arnopo/linux/commits/step3-virtio-memories
>>>>> step 4: Add mailbox declaration in VirtIO subnode
>>>>> => https://github.com/arnopo/linux/commits/step4-virtio-mailboxes
>>>>>
>>>>> Arnaud Pouliquen (4):
>>>>>   remoteproc: core: Introduce rproc_rvdev_add_device function
>>>>>   remoteproc: core: Introduce rproc_add_rvdev function
>>>>>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
>>>>>   remoteproc: virtio: Create platform device for the remoteproc_virtio
>>>>>
>>>>>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
>>>>>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
>>>>>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
>>>>>  include/linux/remoteproc.h               |   6 +-
>>>>>  4 files changed, 210 insertions(+), 162 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.24.3
>>>>>
Arnaud POULIQUEN Sept. 21, 2022, 2:07 p.m. UTC | #7
Hi Peng,

On 9/21/22 10:54, Peng Fan wrote:
> Hi Arnaud,
> 
>> Subject: [PATCH v8 0/4] remoteproc: restructure the remoteproc VirtIO
>> device
> 
> Sorry to get in at this late time, just try to catch up.
> Not reviewing comments, just have a question,
> Does remote core firmware requires changes to use this new feature?

For this series, it is not.
For the whole work, it should not, but it will probably depend on the
evolutions related to the reviews and requirements that will come.

> Does your 4 branches listed below still work with linux-6.x?

I have to rebase them. Today my github branches are based on v5.18.rc1
I plan to do this end of this week or next week.
   
> Could the multiple vdev still share same mbox channel?

Yes I'm trying to keep the legacy support of the mailbox in the
remoteproc platform driver.
If no mailbox is declared in the virtio subnode it calls the rproc->ops->kick

> 
> I not own i.MX remote core firmware development, so if no need
> firmware change, I would like give a try and see how it works.

Great! That would be nice to have your feedback. 
Mailbox management is one point, I'm also ineterested in having feedback on 
the memory regions management
I will ping you when my work will be rebased on 6.0

Thanks,
Arnaud

> 
> Thanks,
> Peng.
> 
>>
>> 1) Update from V7 [1]:
>>
>> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc:
>> imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
>>   The updates take into account the integration of the
>>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after
>> rproc_shutdown")
>> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according
>> to reviews on V7
>>
>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
>> org%2Flkml%2F2022%2F7%2F13%2F663&amp;data=05%7C01%7Cpeng.fan%
>> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
>> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
>> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
>> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=77pEuwAI7Lh61hx1%2B
>> Hs79Cu0G5KOa6mzQ0PnTC5r8Xk%3D&amp;reserved=0
>> [2]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ke
>> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fremoteproc%2Flinux.g
>> it%2Flog%2F%3Fh%3Dfor-
>> next&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e7
>> 439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
>> C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
>> C%7C&amp;sdata=iWUSzKkN9BpHwqbO62awIcyVf9PXcftcdt2kytWVR78%3D
>> &amp;reserved=0
>>
>> 2) Patchset description:
>>
>> This series is a part of the work initiated a long time ago in the series
>> "remoteproc: Decorelate virtio from core"[3]
>>
>> Objective of the work:
>> - Update the remoteproc VirtIO device creation (use platform device)
>> - Allow to declare remoteproc VirtIO device in DT
>>     - declare resources associated to a remote proc VirtIO
>>     - declare a list of VirtIO supported by the platform.
>> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
>>   For instance be able to declare a I2C device in a virtio-i2C node.
>> - Keep the legacy working!
>> - Try to improve the picture about concerns reported by Christoph Hellwing
>> [4][5]
>>
>> [3]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
>> org%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=05%7C01%7Cpeng.fan
>> %40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4
>> c6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7
>> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
>> iLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=oPWSfUweLdhUFK5X9
>> 2YcGHem8s%2Bfelcr%2FHx9JAlKG%2BI%3D&amp;reserved=0
>> [4]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
>> org%2Flkml%2F2021%2F6%2F23%2F607&amp;data=05%7C01%7Cpeng.fan%
>> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
>> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
>> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
>> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HPpnlaykes8R1Kz1dEN
>> nirEHkDNr7JvRs%2FcsaDPuLdI%3D&amp;reserved=0
>> [5]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
>> hwork.kernel.org%2Fproject%2Flinux-
>> remoteproc%2Fpatch%2FAOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E
>> %40cp7-web-
>> 042.plabs.ch%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e520
>> 0d739a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%
>> 7C0%7C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIj
>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
>> 3000%7C%7C%7C&amp;sdata=GtNruefDreOoogL%2BlntAC7GBfk6E1Goq4j%
>> 2BYXt36RdI%3D&amp;reserved=0
>>
>> In term of device tree this would result in such hierarchy (stm32mp1
>> example with 2 virtio RPMSG):
>>
>> 	m4_rproc: m4@10000000 {
>> 		compatible = "st,stm32mp1-m4";
>> 		reg = <0x10000000 0x40000>,
>> 		      <0x30000000 0x40000>,
>> 		      <0x38000000 0x10000>;
>>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
>>         mboxes = <&ipcc 2>, <&ipcc 3>;
>>         mbox-names = "shutdown", "detach";
>>         status = "okay";
>>
>>         #address-cells = <1>;
>>         #size-cells = <0>;
>>
>>         vdev@0 {
>> 		compatible = "rproc-virtio";
>> 		reg = <0>;
>> 		virtio,id = <7>;  /* RPMSG */
>> 		memory-region = <&vdev0vring0>, <&vdev0vring1>,
>> <&vdev0buffer>;
>> 		mboxes = <&ipcc 0>, <&ipcc 1>;
>> 		mbox-names = "vq0", "vq1";
>> 		status = "okay";
>>         };
>>
>>         vdev@1 {
>> 		compatible = "rproc-virtio";
>> 		reg = <1>;
>> 		virtio,id = <7>;  /*RPMSG */
>> 		memory-region = <&vdev1vring0>, <&vdev1vring1>,
>> <&vdev1buffer>;
>> 		mboxes = <&ipcc 4>, <&ipcc 5>;
>> 		mbox-names = "vq0", "vq1";
>> 		status = "okay";
>>         };
>> };
>>
>> I have divided the work in 4 steps to simplify the review, This series
>> implements only the step 1:
>> step 1: Redefine the remoteproc VirtIO device as a platform device
>>   - migrate rvdev management in remoteproc virtio.c,
>>   - create a remotproc virtio config ( can be disabled for platform that not
>> use VirtIO IPC.
>> step 2: Add possibility to declare and probe a VirtIO sub node
>>   - VirtIO bindings declaration,
>>   - multi DT VirtIO devices support,
>>   - introduction of a remote proc virtio bind device mechanism , =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-
>> DT&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e74
>> 39508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
>> 637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
>> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
>> %7C&amp;sdata=XtF%2FQnml3QXFL7rgqST1Z2FotUzoj%2FD57WfiuAVMnr8
>> %3D&amp;reserved=0
>> step 3: Add memory declaration in VirtIO subnode =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-
>> memories&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
>> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
>> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
>> 7C%7C%7C&amp;sdata=6gq28c6a1TJ%2FdkvokcEjgy6FKQcKTXSz%2BNAbJPo
>> mjac%3D&amp;reserved=0
>> step 4: Add mailbox declaration in VirtIO subnode =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-
>> mailboxes&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
>> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
>> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
>> 7C%7C%7C&amp;sdata=wfy2euuOPoMmBMIH3BOsGcsEYGSTWsDaRr7ENN
>> QCK70%3D&amp;reserved=0
>>
>> Arnaud Pouliquen (4):
>>   remoteproc: core: Introduce rproc_rvdev_add_device function
>>   remoteproc: core: Introduce rproc_add_rvdev function
>>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
>>   remoteproc: virtio: Create platform device for the remoteproc_virtio
>>
>>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
>>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
>>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
>>  include/linux/remoteproc.h               |   6 +-
>>  4 files changed, 210 insertions(+), 162 deletions(-)
>>
>> --
>> 2.24.3
>
Arnaud POULIQUEN Sept. 23, 2022, 10:08 a.m. UTC | #8
Hi Peng,

On 9/21/22 16:07, Arnaud POULIQUEN wrote:
> Hi Peng,
> 
> On 9/21/22 10:54, Peng Fan wrote:
>> Hi Arnaud,
>>
>>> Subject: [PATCH v8 0/4] remoteproc: restructure the remoteproc VirtIO
>>> device
>>
>> Sorry to get in at this late time, just try to catch up.
>> Not reviewing comments, just have a question,
>> Does remote core firmware requires changes to use this new feature?
> 
> For this series, it is not.
> For the whole work, it should not, but it will probably depend on the
> evolutions related to the reviews and requirements that will come.
> 
>> Does your 4 branches listed below still work with linux-6.x?
> 
> I have to rebase them. Today my github branches are based on v5.18.rc1
> I plan to do this end of this week or next week.
>    
>> Could the multiple vdev still share same mbox channel?
> 
> Yes I'm trying to keep the legacy support of the mailbox in the
> remoteproc platform driver.
> If no mailbox is declared in the virtio subnode it calls the rproc->ops->kick
> 
>>
>> I not own i.MX remote core firmware development, so if no need
>> firmware change, I would like give a try and see how it works.
> 
> Great! That would be nice to have your feedback. 
> Mailbox management is one point, I'm also ineterested in having feedback on 
> the memory regions management
> I will ping you when my work will be rebased on 6.0

My github branches have been rebased on top of thre rproc_next(1d7b61c06dc3)

As a first step you should be able to rebase on my step4-virtio-mailboxes[1]
without any update of your driver. If I did my dev well, I kept the
compatibility with the legacy.

[1] https://github.com/arnopo/linux/commits/step4-virtio-mailboxes

Regards,
Arnaud

> 
> Thanks,
> Arnaud
> 
>>
>> Thanks,
>> Peng.
>>
>>>
>>> 1) Update from V7 [1]:
>>>
>>> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc:
>>> imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
>>>   The updates take into account the integration of the
>>>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after
>>> rproc_shutdown")
>>> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> according
>>> to reviews on V7
>>>
>>>
>>> [1]
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
>>> org%2Flkml%2F2022%2F7%2F13%2F663&amp;data=05%7C01%7Cpeng.fan%
>>> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
>>> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
>>> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
>>> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=77pEuwAI7Lh61hx1%2B
>>> Hs79Cu0G5KOa6mzQ0PnTC5r8Xk%3D&amp;reserved=0
>>> [2]
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ke
>>> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fremoteproc%2Flinux.g
>>> it%2Flog%2F%3Fh%3Dfor-
>>> next&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e7
>>> 439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
>>> C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
>>> C%7C&amp;sdata=iWUSzKkN9BpHwqbO62awIcyVf9PXcftcdt2kytWVR78%3D
>>> &amp;reserved=0
>>>
>>> 2) Patchset description:
>>>
>>> This series is a part of the work initiated a long time ago in the series
>>> "remoteproc: Decorelate virtio from core"[3]
>>>
>>> Objective of the work:
>>> - Update the remoteproc VirtIO device creation (use platform device)
>>> - Allow to declare remoteproc VirtIO device in DT
>>>     - declare resources associated to a remote proc VirtIO
>>>     - declare a list of VirtIO supported by the platform.
>>> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
>>>   For instance be able to declare a I2C device in a virtio-i2C node.
>>> - Keep the legacy working!
>>> - Try to improve the picture about concerns reported by Christoph Hellwing
>>> [4][5]
>>>
>>> [3]
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
>>> org%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=05%7C01%7Cpeng.fan
>>> %40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4
>>> c6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7
>>> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
>>> iLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=oPWSfUweLdhUFK5X9
>>> 2YcGHem8s%2Bfelcr%2FHx9JAlKG%2BI%3D&amp;reserved=0
>>> [4]
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
>>> org%2Flkml%2F2021%2F6%2F23%2F607&amp;data=05%7C01%7Cpeng.fan%
>>> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
>>> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
>>> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
>>> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HPpnlaykes8R1Kz1dEN
>>> nirEHkDNr7JvRs%2FcsaDPuLdI%3D&amp;reserved=0
>>> [5]
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
>>> hwork.kernel.org%2Fproject%2Flinux-
>>> remoteproc%2Fpatch%2FAOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E
>>> %40cp7-web-
>>> 042.plabs.ch%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e520
>>> 0d739a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%
>>> 7C0%7C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIj
>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
>>> 3000%7C%7C%7C&amp;sdata=GtNruefDreOoogL%2BlntAC7GBfk6E1Goq4j%
>>> 2BYXt36RdI%3D&amp;reserved=0
>>>
>>> In term of device tree this would result in such hierarchy (stm32mp1
>>> example with 2 virtio RPMSG):
>>>
>>> 	m4_rproc: m4@10000000 {
>>> 		compatible = "st,stm32mp1-m4";
>>> 		reg = <0x10000000 0x40000>,
>>> 		      <0x30000000 0x40000>,
>>> 		      <0x38000000 0x10000>;
>>>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
>>>         mboxes = <&ipcc 2>, <&ipcc 3>;
>>>         mbox-names = "shutdown", "detach";
>>>         status = "okay";
>>>
>>>         #address-cells = <1>;
>>>         #size-cells = <0>;
>>>
>>>         vdev@0 {
>>> 		compatible = "rproc-virtio";
>>> 		reg = <0>;
>>> 		virtio,id = <7>;  /* RPMSG */
>>> 		memory-region = <&vdev0vring0>, <&vdev0vring1>,
>>> <&vdev0buffer>;
>>> 		mboxes = <&ipcc 0>, <&ipcc 1>;
>>> 		mbox-names = "vq0", "vq1";
>>> 		status = "okay";
>>>         };
>>>
>>>         vdev@1 {
>>> 		compatible = "rproc-virtio";
>>> 		reg = <1>;
>>> 		virtio,id = <7>;  /*RPMSG */
>>> 		memory-region = <&vdev1vring0>, <&vdev1vring1>,
>>> <&vdev1buffer>;
>>> 		mboxes = <&ipcc 4>, <&ipcc 5>;
>>> 		mbox-names = "vq0", "vq1";
>>> 		status = "okay";
>>>         };
>>> };
>>>
>>> I have divided the work in 4 steps to simplify the review, This series
>>> implements only the step 1:
>>> step 1: Redefine the remoteproc VirtIO device as a platform device
>>>   - migrate rvdev management in remoteproc virtio.c,
>>>   - create a remotproc virtio config ( can be disabled for platform that not
>>> use VirtIO IPC.
>>> step 2: Add possibility to declare and probe a VirtIO sub node
>>>   - VirtIO bindings declaration,
>>>   - multi DT VirtIO devices support,
>>>   - introduction of a remote proc virtio bind device mechanism , =>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
>>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-
>>> DT&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e74
>>> 39508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
>>> 637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
>>> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
>>> %7C&amp;sdata=XtF%2FQnml3QXFL7rgqST1Z2FotUzoj%2FD57WfiuAVMnr8
>>> %3D&amp;reserved=0
>>> step 3: Add memory declaration in VirtIO subnode =>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
>>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-
>>> memories&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
>>> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>>> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
>>> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
>>> 7C%7C%7C&amp;sdata=6gq28c6a1TJ%2FdkvokcEjgy6FKQcKTXSz%2BNAbJPo
>>> mjac%3D&amp;reserved=0
>>> step 4: Add mailbox declaration in VirtIO subnode =>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
>>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-
>>> mailboxes&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
>>> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>>> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
>>> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
>>> 7C%7C%7C&amp;sdata=wfy2euuOPoMmBMIH3BOsGcsEYGSTWsDaRr7ENN
>>> QCK70%3D&amp;reserved=0
>>>
>>> Arnaud Pouliquen (4):
>>>   remoteproc: core: Introduce rproc_rvdev_add_device function
>>>   remoteproc: core: Introduce rproc_add_rvdev function
>>>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
>>>   remoteproc: virtio: Create platform device for the remoteproc_virtio
>>>
>>>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
>>>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
>>>  drivers/remoteproc/remoteproc_virtio.c   | 189 ++++++++++++++++++++---
>>>  include/linux/remoteproc.h               |   6 +-
>>>  4 files changed, 210 insertions(+), 162 deletions(-)
>>>
>>> --
>>> 2.24.3
>>
Peng Fan Sept. 26, 2022, 12:47 a.m. UTC | #9
> Subject: Re: [PATCH v8 0/4] remoteproc: restructure the remoteproc VirtIO
> device
> 
> Hi Peng,
> 
> On 9/21/22 16:07, Arnaud POULIQUEN wrote:
> > Hi Peng,
> >
> > On 9/21/22 10:54, Peng Fan wrote:
> >> Hi Arnaud,
> >>
> >>> Subject: [PATCH v8 0/4] remoteproc: restructure the remoteproc
> >>> VirtIO device
> >>
> >> Sorry to get in at this late time, just try to catch up.
> >> Not reviewing comments, just have a question, Does remote core
> >> firmware requires changes to use this new feature?
> >
> > For this series, it is not.
> > For the whole work, it should not, but it will probably depend on the
> > evolutions related to the reviews and requirements that will come.
> >
> >> Does your 4 branches listed below still work with linux-6.x?
> >
> > I have to rebase them. Today my github branches are based on v5.18.rc1
> > I plan to do this end of this week or next week.
> >
> >> Could the multiple vdev still share same mbox channel?
> >
> > Yes I'm trying to keep the legacy support of the mailbox in the
> > remoteproc platform driver.
> > If no mailbox is declared in the virtio subnode it calls the
> > rproc->ops->kick
> >
> >>
> >> I not own i.MX remote core firmware development, so if no need
> >> firmware change, I would like give a try and see how it works.
> >
> > Great! That would be nice to have your feedback.
> > Mailbox management is one point, I'm also ineterested in having
> > feedback on the memory regions management I will ping you when my
> work
> > will be rebased on 6.0
> 
> My github branches have been rebased on top of thre
> rproc_next(1d7b61c06dc3)
> 
> As a first step you should be able to rebase on my step4-virtio-mailboxes[1]
> without any update of your driver. If I did my dev well, I kept the
> compatibility with the legacy.

Thanks for the quick action. I will give a try on i.MX, and see how it works,
but I may not respond quick.

Thanks,
Peng.

> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-
> mailboxes&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C50e9c0e75117
> 4c09328808da9d4bab9b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637995245590228209%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&amp;sdata=Jbwa0aPM4aoR8H1cyTLFKw4lK9VUcCyHwzkleVsnkRc
> %3D&amp;reserved=0
> 
> Regards,
> Arnaud
> 
> >
> > Thanks,
> > Arnaud
> >
> >>
> >> Thanks,
> >> Peng.
> >>
> >>>
> >>> 1) Update from V7 [1]:
> >>>
> >>> - rebase on rproc-next branch [2], commit 729c16326b7f ("remoteproc:
> >>> imx_dsp_rproc: fix argument 2 of rproc_mem_entry_init")
> >>>   The updates take into account the integration of the
> >>>   commit 1404acbb7f68 ("remoteproc: Fix dma_mem leak after
> >>> rproc_shutdown")
> >>> - add Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>> according to reviews on V7
> >>>
> >>>
> >>> [1]
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> >>>
> org%2Flkml%2F2022%2F7%2F13%2F663&amp;data=05%7C01%7Cpeng.fan%
> >>>
> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
> >>>
> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
> >>>
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> >>>
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=77pEuwAI7Lh61hx1%2B
> >>> Hs79Cu0G5KOa6mzQ0PnTC5r8Xk%3D&amp;reserved=0
> >>> [2]
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >>>
> t.ke%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C50e9c0e751174c
> 093288
> >>>
> 08da9d4bab9b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637
> 9952455
> >>>
> 90228209%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjo
> iV2luMzI
> >>>
> iLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=bF
> M3T6bY1c
> >>> p%2FvyRyZo6mdXGeN7%2BTCMfcmeY3OFpksWA%3D&amp;reserved=0
> >>>
> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fremoteproc%2Flinux.g
> >>> it%2Flog%2F%3Fh%3Dfor-
> >>>
> next&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e7
> >>>
> 439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> >>>
> C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> >>>
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> >>>
> C%7C&amp;sdata=iWUSzKkN9BpHwqbO62awIcyVf9PXcftcdt2kytWVR78%3D
> >>> &amp;reserved=0
> >>>
> >>> 2) Patchset description:
> >>>
> >>> This series is a part of the work initiated a long time ago in the
> >>> series
> >>> "remoteproc: Decorelate virtio from core"[3]
> >>>
> >>> Objective of the work:
> >>> - Update the remoteproc VirtIO device creation (use platform device)
> >>> - Allow to declare remoteproc VirtIO device in DT
> >>>     - declare resources associated to a remote proc VirtIO
> >>>     - declare a list of VirtIO supported by the platform.
> >>> - Prepare the enhancement to more VirtIO devices (e.g I2C, audio,
> video, ...).
> >>>   For instance be able to declare a I2C device in a virtio-i2C node.
> >>> - Keep the legacy working!
> >>> - Try to improve the picture about concerns reported by Christoph
> >>> Hellwing [4][5]
> >>>
> >>> [3]
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> >>>
> org%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=05%7C01%7Cpeng.fan
> >>> %40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3b
> c2b4
> >>>
> c6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7
> >>>
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
> >>>
> iLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=oPWSfUweLdhUFK5X9
> >>> 2YcGHem8s%2Bfelcr%2FHx9JAlKG%2BI%3D&amp;reserved=0
> >>> [4]
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> >>>
> org%2Flkml%2F2021%2F6%2F23%2F607&amp;data=05%7C01%7Cpeng.fan%
> >>>
> 40nxp.com%7Ce0e5200d739a48e7439508da87599d14%7C686ea1d3bc2b4c
> >>>
> 6fa92cd99c5c301635%7C0%7C0%7C637971116202643149%7CUnknown%7C
> >>>
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> >>>
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HPpnlaykes8R1Kz1dEN
> >>> nirEHkDNr7JvRs%2FcsaDPuLdI%3D&amp;reserved=0
> >>> [5]
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> >>> tc
> >>> hwork.kernel.org%2Fproject%2Flinux-
> >>>
> remoteproc%2Fpatch%2FAOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E
> >>> %40cp7-web-
> >>>
> 042.plabs.ch%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e520
> >>>
> 0d739a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> >>>
> 7C0%7C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> >>>
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> >>>
> 3000%7C%7C%7C&amp;sdata=GtNruefDreOoogL%2BlntAC7GBfk6E1Goq4j%
> >>> 2BYXt36RdI%3D&amp;reserved=0
> >>>
> >>> In term of device tree this would result in such hierarchy (stm32mp1
> >>> example with 2 virtio RPMSG):
> >>>
> >>> 	m4_rproc: m4@10000000 {
> >>> 		compatible = "st,stm32mp1-m4";
> >>> 		reg = <0x10000000 0x40000>,
> >>> 		      <0x30000000 0x40000>,
> >>> 		      <0x38000000 0x10000>;
> >>>         memory-region = <&retram>, <&mcuram>,<&mcuram2>;
> >>>         mboxes = <&ipcc 2>, <&ipcc 3>;
> >>>         mbox-names = "shutdown", "detach";
> >>>         status = "okay";
> >>>
> >>>         #address-cells = <1>;
> >>>         #size-cells = <0>;
> >>>
> >>>         vdev@0 {
> >>> 		compatible = "rproc-virtio";
> >>> 		reg = <0>;
> >>> 		virtio,id = <7>;  /* RPMSG */
> >>> 		memory-region = <&vdev0vring0>, <&vdev0vring1>,
> <&vdev0buffer>;
> >>> 		mboxes = <&ipcc 0>, <&ipcc 1>;
> >>> 		mbox-names = "vq0", "vq1";
> >>> 		status = "okay";
> >>>         };
> >>>
> >>>         vdev@1 {
> >>> 		compatible = "rproc-virtio";
> >>> 		reg = <1>;
> >>> 		virtio,id = <7>;  /*RPMSG */
> >>> 		memory-region = <&vdev1vring0>, <&vdev1vring1>,
> <&vdev1buffer>;
> >>> 		mboxes = <&ipcc 4>, <&ipcc 5>;
> >>> 		mbox-names = "vq0", "vq1";
> >>> 		status = "okay";
> >>>         };
> >>> };
> >>>
> >>> I have divided the work in 4 steps to simplify the review, This
> >>> series implements only the step 1:
> >>> step 1: Redefine the remoteproc VirtIO device as a platform device
> >>>   - migrate rvdev management in remoteproc virtio.c,
> >>>   - create a remotproc virtio config ( can be disabled for platform
> >>> that not use VirtIO IPC.
> >>> step 2: Add possibility to declare and probe a VirtIO sub node
> >>>   - VirtIO bindings declaration,
> >>>   - multi DT VirtIO devices support,
> >>>   - introduction of a remote proc virtio bind device mechanism , =>
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >>> thu
> >>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-
> >>>
> DT&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739a48e74
> >>>
> 39508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> >>>
> 637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> >>>
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> >>> %7C&amp;sdata=XtF%2FQnml3QXFL7rgqST1Z2FotUzoj%2FD57WfiuAVM
> nr8
> >>> %3D&amp;reserved=0
> >>> step 3: Add memory declaration in VirtIO subnode =>
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >>> thu
> >>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-
> >>>
> memories&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
> >>>
> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> >>>
> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> >>>
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> >>>
> 7C%7C%7C&amp;sdata=6gq28c6a1TJ%2FdkvokcEjgy6FKQcKTXSz%2BNAbJPo
> >>> mjac%3D&amp;reserved=0
> >>> step 4: Add mailbox declaration in VirtIO subnode =>
> >>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >>> thu
> >>> b.com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-
> >>>
> mailboxes&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7Ce0e5200d739
> >>>
> a48e7439508da87599d14%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> >>>
> C0%7C637971116202643149%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> >>>
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> >>>
> 7C%7C%7C&amp;sdata=wfy2euuOPoMmBMIH3BOsGcsEYGSTWsDaRr7ENN
> >>> QCK70%3D&amp;reserved=0
> >>>
> >>> Arnaud Pouliquen (4):
> >>>   remoteproc: core: Introduce rproc_rvdev_add_device function
> >>>   remoteproc: core: Introduce rproc_add_rvdev function
> >>>   remoteproc: Move rproc_vdev management to remoteproc_virtio.c
> >>>   remoteproc: virtio: Create platform device for the
> >>> remoteproc_virtio
> >>>
> >>>  drivers/remoteproc/remoteproc_core.c     | 154 +++---------------
> >>>  drivers/remoteproc/remoteproc_internal.h |  23 ++-
> >>>  drivers/remoteproc/remoteproc_virtio.c   | 189
> ++++++++++++++++++++---
> >>>  include/linux/remoteproc.h               |   6 +-
> >>>  4 files changed, 210 insertions(+), 162 deletions(-)
> >>>
> >>> --
> >>> 2.24.3
> >>