mbox series

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

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

Message

Arnaud POULIQUEN Jan. 26, 2022, 4:24 p.m. UTC
Update from V2 [1]:
In order to better handle error cases and to have something more symmetrical between
the functions in charge of rvdev initialization/deletion, the patchset has been reworked.
 - Introduction in the first patch, of rproc_vdev_data structure which allows to better
   decorrelate the rproc from the management of the rvdev structure. This structure is reused
   in the last patch of the series for the creation of the remoteproc virtio platform device.
 - In addition to the previous version, the management of the vring lifecycle has been fully
   migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring, rproc_free_vring)

[1] https://lkml.org/lkml/2021/12/22/111

Patchset description:

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

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 [3][4]

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

In term of device tree this would result in such hiearchy (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 prob 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 virtio device add/remove functions
  remoteproc: core: Introduce rproc_register_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     | 159 +++----------------
 drivers/remoteproc/remoteproc_internal.h |  33 +++-
 drivers/remoteproc/remoteproc_virtio.c   | 193 ++++++++++++++++++++---
 include/linux/remoteproc.h               |   6 +-
 4 files changed, 227 insertions(+), 164 deletions(-)

Comments

Peng Fan Feb. 15, 2022, 8:34 a.m. UTC | #1
> Subject: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc VirtIO
> device
> 
> Update from V2 [1]:
> In order to better handle error cases and to have something more
> symmetrical between the functions in charge of rvdev initialization/deletion,
> the patchset has been reworked.
>  - Introduction in the first patch, of rproc_vdev_data structure which allows
> to better
>    decorrelate the rproc from the management of the rvdev structure. This
> structure is reused
>    in the last patch of the series for the creation of the remoteproc virtio
> platform device.
>  - In addition to the previous version, the management of the vring lifecycle
> has been fully
>    migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring,
> rproc_free_vring)
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
> g%2Flkml%2F2021%2F12%2F22%2F111&amp;data=04%7C01%7Cpeng.fan%4
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000&amp;sdata=bFfSxpPMpPRGYcMBcwxaQ152mRzf3c
> fwoFPjiJ0SIgw%3D&amp;reserved=0
> 
> Patchset description:
> 
> This series is a part of the work initiated a long time ago in the series
> "remoteproc: Decorelate virtio from core"[2]
> 
> Objective of the work:
> - Update the remoteproc VirtIO device creation (use platform device)
> - Allow to declare remoteproc VirtIO device in DT

This means not using resource table anymore with new approach?
If yes, would that introduce a problem that different M-core images
requires different dtb?

>     - 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.

As my understanding virtio-i2c is a i2c bus, you could declare a i2c device
in the virtual bus without your patchset, would you please share more?

Thanks,
Peng.

> - Keep the legacy working!
> - Try to improve the picture about concerns reported by Christoph Hellwing
> [3][4]
> 
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
> g%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=04%7C01%7Cpeng.fan%4
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000&amp;sdata=O2BZw5PCY19eD5xMGxrGUKC%2Fty1
> Sdc3LE6rhK4cSXvs%3D&amp;reserved=0
> [3]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
> g%2Flkml%2F2021%2F6%2F23%2F607&amp;data=04%7C01%7Cpeng.fan%40
> nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CTW
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C3000&amp;sdata=xqX50iDeL%2BtFBOgyADnEUE5HH4gogK
> C0MwyqZSxVqNo%3D&amp;reserved=0
> [4]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2FAOKowLclCbO
> CKxyiJ71WeNyuAAj2q8EUtxrXbyky5E%40cp7-web-042.plabs.ch%2F&amp;da
> ta=04%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e85
> 5e2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> 757786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=mvSm3wM
> LgQ%2BDFhqjXIkG8de58zFjwPSURzw55JhGNaA%3D&amp;reserved=0
> 
> In term of device tree this would result in such hiearchy (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 prob 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%2Fgithub.
> com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-DT&amp;data=04%7
> C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757786
> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=X%2B462681gcxe6
> 2GP%2BV7ji2nef%2FuTbQVvIlddcMQwtmg%3D&amp;reserved=0
> step 3: Add memory declaration in VirtIO subnode =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-memories&amp;data=0
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=eMlXgCgrV6l46
> h3Ywv1%2BCoX3gLBabdTZs9ybsm4t4ys%3D&amp;reserved=0
> step 4: Add mailbox declaration in VirtIO subnode =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-mailboxes&amp;data=0
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=75hApOwihqMZ
> UUKz1VcitY2VPDc6KAIwAvH8enEZOPY%3D&amp;reserved=0
> 
> Arnaud Pouliquen (4):
>   remoteproc: core: Introduce virtio device add/remove functions
>   remoteproc: core: Introduce rproc_register_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     | 159 +++----------------
>  drivers/remoteproc/remoteproc_internal.h |  33 +++-
>  drivers/remoteproc/remoteproc_virtio.c   | 193
> ++++++++++++++++++++---
>  include/linux/remoteproc.h               |   6 +-
>  4 files changed, 227 insertions(+), 164 deletions(-)
> 
> --
> 2.25.1
Arnaud POULIQUEN Feb. 15, 2022, 11:11 a.m. UTC | #2
On 2/15/22 09:34, Peng Fan wrote:
>> Subject: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc VirtIO
>> device
>>
>> Update from V2 [1]:
>> In order to better handle error cases and to have something more
>> symmetrical between the functions in charge of rvdev initialization/deletion,
>> the patchset has been reworked.
>>  - Introduction in the first patch, of rproc_vdev_data structure which allows
>> to better
>>    decorrelate the rproc from the management of the rvdev structure. This
>> structure is reused
>>    in the last patch of the series for the creation of the remoteproc virtio
>> platform device.
>>  - In addition to the previous version, the management of the vring lifecycle
>> has been fully
>>    migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring,
>> rproc_free_vring)
>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
>> g%2Flkml%2F2021%2F12%2F22%2F111&amp;data=04%7C01%7Cpeng.fan%4
>> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
>> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
>> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
>> JXVCI6Mn0%3D%7C3000&amp;sdata=bFfSxpPMpPRGYcMBcwxaQ152mRzf3c
>> fwoFPjiJ0SIgw%3D&amp;reserved=0
>>
>> Patchset description:
>>
>> This series is a part of the work initiated a long time ago in the series
>> "remoteproc: Decorelate virtio from core"[2]
>>
>> Objective of the work:
>> - Update the remoteproc VirtIO device creation (use platform device)
>> - Allow to declare remoteproc VirtIO device in DT
> 
> This means not using resource table anymore with new approach?
> If yes, would that introduce a problem that different M-core images
> requires different dtb?

The resource table still exists. The main difference is that the virtio devices
would be predefined in the DT with their own resources ( memories , mailboxes,...)
No need to inherit from the rproc device.


On resource table parsing, the remoteproc looks first for pre registered 
rproc_virtio devices. If found then it uses it. Else it instantiates a new 
one (legacy method).  


> 
>>     - 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.
> 
> As my understanding virtio-i2c is a i2c bus, you could declare a i2c device
> in the virtual bus without your patchset, would you please share more?

Yes virtio-i2c is a bus, There is different methods to declare I2C device on
a bus[1].

In ST we rely on DT to statically declare an I2C device,as child of the I2C
adapter node.
I haven't implemented the virtio-I2C part yet, but it would make sense to have
such an implementation.

Which alternative have you in mind?  

[1] https://www.kernel.org/doc/html/latest/i2c/instantiating-devices.html

Thanks,
Arnaud

> 
> Thanks,
> Peng.
> 
>> - Keep the legacy working!
>> - Try to improve the picture about concerns reported by Christoph Hellwing
>> [3][4]
>>
>> [2]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
>> g%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=04%7C01%7Cpeng.fan%4
>> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
>> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
>> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
>> JXVCI6Mn0%3D%7C3000&amp;sdata=O2BZw5PCY19eD5xMGxrGUKC%2Fty1
>> Sdc3LE6rhK4cSXvs%3D&amp;reserved=0
>> [3]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
>> g%2Flkml%2F2021%2F6%2F23%2F607&amp;data=04%7C01%7Cpeng.fan%40
>> nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa9
>> 2cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CTW
>> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
>> VCI6Mn0%3D%7C3000&amp;sdata=xqX50iDeL%2BtFBOgyADnEUE5HH4gogK
>> C0MwyqZSxVqNo%3D&amp;reserved=0
>> [4]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
>> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2FAOKowLclCbO
>> CKxyiJ71WeNyuAAj2q8EUtxrXbyky5E%40cp7-web-042.plabs.ch%2F&amp;da
>> ta=04%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e85
>> 5e2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
>> 757786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
>> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=mvSm3wM
>> LgQ%2BDFhqjXIkG8de58zFjwPSURzw55JhGNaA%3D&amp;reserved=0
>>
>> In term of device tree this would result in such hiearchy (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 prob 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%2Fgithub.
>> com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-DT&amp;data=04%7
>> C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C
>> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757786
>> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
>> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=X%2B462681gcxe6
>> 2GP%2BV7ji2nef%2FuTbQVvIlddcMQwtmg%3D&amp;reserved=0
>> step 3: Add memory declaration in VirtIO subnode =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>> com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-memories&amp;data=0
>> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
>> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=eMlXgCgrV6l46
>> h3Ywv1%2BCoX3gLBabdTZs9ybsm4t4ys%3D&amp;reserved=0
>> step 4: Add mailbox declaration in VirtIO subnode =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>> com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-mailboxes&amp;data=0
>> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
>> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=75hApOwihqMZ
>> UUKz1VcitY2VPDc6KAIwAvH8enEZOPY%3D&amp;reserved=0
>>
>> Arnaud Pouliquen (4):
>>   remoteproc: core: Introduce virtio device add/remove functions
>>   remoteproc: core: Introduce rproc_register_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     | 159 +++----------------
>>  drivers/remoteproc/remoteproc_internal.h |  33 +++-
>>  drivers/remoteproc/remoteproc_virtio.c   | 193
>> ++++++++++++++++++++---
>>  include/linux/remoteproc.h               |   6 +-
>>  4 files changed, 227 insertions(+), 164 deletions(-)
>>
>> --
>> 2.25.1
>
Peng Fan Feb. 22, 2022, 1:07 p.m. UTC | #3
> Subject: Re: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc
> VirtIO device
> 
> 
> 
> On 2/15/22 09:34, Peng Fan wrote:
> >> Subject: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc
> >> VirtIO device
> >>
> >> Update from V2 [1]:
> >> In order to better handle error cases and to have something more
> >> symmetrical between the functions in charge of rvdev
> >> initialization/deletion, the patchset has been reworked.
> >>  - Introduction in the first patch, of rproc_vdev_data structure
> >> which allows to better
> >>    decorrelate the rproc from the management of the rvdev structure.
> >> This structure is reused
> >>    in the last patch of the series for the creation of the remoteproc
> >> virtio platform device.
> >>  - In addition to the previous version, the management of the vring
> >> lifecycle has been fully
> >>    migrated to the remoteproc_virtio.c (rproc_parse_vring,
> >> rproc_alloc_vring,
> >> rproc_free_vring)
> >>
> >> [1]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >>
> l.or%2F&amp;data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a
> 845cbf0
> >>
> 8d9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63780
> 5203140
> >>
> 739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLC
> >>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=MDamNuBkyFebgG
> BuP5shcU9
> >> aw%2FdMuM9GBTEEzffQQkA%3D&amp;reserved=0
> >>
> g%2Flkml%2F2021%2F12%2F22%2F111&amp;data=04%7C01%7Cpeng.fan%4
> >>
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> >>
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> >>
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> >>
> JXVCI6Mn0%3D%7C3000&amp;sdata=bFfSxpPMpPRGYcMBcwxaQ152mRzf3c
> >> fwoFPjiJ0SIgw%3D&amp;reserved=0
> >>
> >> Patchset description:
> >>
> >> This series is a part of the work initiated a long time ago in the
> >> series
> >> "remoteproc: Decorelate virtio from core"[2]
> >>
> >> Objective of the work:
> >> - Update the remoteproc VirtIO device creation (use platform device)
> >> - Allow to declare remoteproc VirtIO device in DT
> >
> > This means not using resource table anymore with new approach?
> > If yes, would that introduce a problem that different M-core images
> > requires different dtb?
> 
> The resource table still exists. The main difference is that the virtio devices
> would be predefined in the DT with their own resources ( memories ,
> mailboxes,...) No need to inherit from the rproc device.
> 
> 
> On resource table parsing, the remoteproc looks first for pre registered
> rproc_virtio devices. If found then it uses it. Else it instantiates a new one
> (legacy method).
> 
> 
> >
> >>     - 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.
> >
> > As my understanding virtio-i2c is a i2c bus, you could declare a i2c
> > device in the virtual bus without your patchset, would you please share
> more?
> 
> Yes virtio-i2c is a bus, There is different methods to declare I2C device on a
> bus[1].
> 
> In ST we rely on DT to statically declare an I2C device,as child of the I2C
> adapter node.
> I haven't implemented the virtio-I2C part yet, but it would make sense to
> have such an implementation.

I misunderstood. Virtio-i2c bus with I2C device in DT is preferred.

> 
> Which alternative have you in mind?

No. NXP use same method, we have a rpmsg i2c driver, rpmsg i2c bus node
and i2c device in DT.

Regards,
Peng.

> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> kernel.org%2Fdoc%2Fhtml%2Flatest%2Fi2c%2Finstantiating-devices.html&a
> mp;data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a845cbf08d
> 9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378052
> 03140739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=C1S
> BPEtDhp7Y9XLB8wHgTLaQ%2BBE6T%2BD8eUr34SFRJYQ%3D&amp;reserved
> =0
> 
> Thanks,
> Arnaud
> 
> >
> > Thanks,
> > Peng.
> >
> >> - Keep the legacy working!
> >> - Try to improve the picture about concerns reported by Christoph
> >> Hellwing [3][4]
> >>
> >> [2]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >>
> l.or%2F&amp;data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a
> 845cbf0
> >>
> 8d9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63780
> 5203140
> >>
> 739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLC
> >>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=MDamNuBkyFebgG
> BuP5shcU9
> >> aw%2FdMuM9GBTEEzffQQkA%3D&amp;reserved=0
> >>
> g%2Flkml%2F2020%2F4%2F16%2F1817&amp;data=04%7C01%7Cpeng.fan%4
> >>
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> >>
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> >>
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> >>
> JXVCI6Mn0%3D%7C3000&amp;sdata=O2BZw5PCY19eD5xMGxrGUKC%2Fty1
> >> Sdc3LE6rhK4cSXvs%3D&amp;reserved=0
> >> [3]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >>
> l.or%2F&amp;data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a
> 845cbf0
> >>
> 8d9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63780
> 5203140
> >>
> 739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLC
> >>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=MDamNuBkyFebgG
> BuP5shcU9
> >> aw%2FdMuM9GBTEEzffQQkA%3D&amp;reserved=0
> >>
> g%2Flkml%2F2021%2F6%2F23%2F607&amp;data=04%7C01%7Cpeng.fan%40
> >>
> nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa9
> >>
> 2cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CTW
> >>
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> >>
> VCI6Mn0%3D%7C3000&amp;sdata=xqX50iDeL%2BtFBOgyADnEUE5HH4gogK
> >> C0MwyqZSxVqNo%3D&amp;reserved=0
> >> [4]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >> ch
> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2FAOKowLclCbO
> >>
> CKxyiJ71WeNyuAAj2q8EUtxrXbyky5E%40cp7-web-042.plabs.ch%2F&amp;da
> >>
> ta=04%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e85
> >>
> 5e2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> >>
> 757786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> >>
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=mvSm3wM
> >> LgQ%2BDFhqjXIkG8de58zFjwPSURzw55JhGNaA%3D&amp;reserved=0
> >>
> >> In term of device tree this would result in such hiearchy (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 prob 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%2Fgithub.
> >>
> com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-DT&amp;data=04%7
> >>
> C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C
> >>
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757786
> >> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiL
> >>
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=X%2B462681gcxe6
> >> 2GP%2BV7ji2nef%2FuTbQVvIlddcMQwtmg%3D&amp;reserved=0
> >> step 3: Add memory declaration in VirtIO subnode =>
> >>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> >>
> com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-memories&amp;data=0
> >>
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> >> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> 757
> >>
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> >>
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=eMlXgCgrV6l46
> >> h3Ywv1%2BCoX3gLBabdTZs9ybsm4t4ys%3D&amp;reserved=0
> >> step 4: Add mailbox declaration in VirtIO subnode =>
> >>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> >>
> com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-mailboxes&amp;data=0
> >>
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> >> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> 757
> >>
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> >>
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=75hApOwihqMZ
> >> UUKz1VcitY2VPDc6KAIwAvH8enEZOPY%3D&amp;reserved=0
> >>
> >> Arnaud Pouliquen (4):
> >>   remoteproc: core: Introduce virtio device add/remove functions
> >>   remoteproc: core: Introduce rproc_register_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     | 159 +++----------------
> >>  drivers/remoteproc/remoteproc_internal.h |  33 +++-
> >>  drivers/remoteproc/remoteproc_virtio.c   | 193
> >> ++++++++++++++++++++---
> >>  include/linux/remoteproc.h               |   6 +-
> >>  4 files changed, 227 insertions(+), 164 deletions(-)
> >>
> >> --
> >> 2.25.1
> >
Mathieu Poirier Feb. 28, 2022, 6:58 p.m. UTC | #4
Good day,

On Wed, 26 Jan 2022 at 09:24, Arnaud Pouliquen
<arnaud.pouliquen@foss.st.com> wrote:
>
> Update from V2 [1]:
> In order to better handle error cases and to have something more symmetrical between
> the functions in charge of rvdev initialization/deletion, the patchset has been reworked.
>  - Introduction in the first patch, of rproc_vdev_data structure which allows to better
>    decorrelate the rproc from the management of the rvdev structure. This structure is reused
>    in the last patch of the series for the creation of the remoteproc virtio platform device.
>  - In addition to the previous version, the management of the vring lifecycle has been fully
>    migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring, rproc_free_vring)
>
> [1] https://lkml.org/lkml/2021/12/22/111
>
> Patchset description:
>
> This series is a part of the work initiated a long time ago in
> the series "remoteproc: Decorelate virtio from core"[2]
>
> 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 [3][4]
>

I started working on this set - comments to follow throughout the week.

Thanks,
Mathieu

> [2] https://lkml.org/lkml/2020/4/16/1817
> [3] https://lkml.org/lkml/2021/6/23/607
> [4] https://patchwork.kernel.org/project/linux-remoteproc/patch/AOKowLclCbOCKxyiJ71WeNyuAAj2q8EUtxrXbyky5E@cp7-web-042.plabs.ch/
>
> In term of device tree this would result in such hiearchy (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 prob 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 virtio device add/remove functions
>   remoteproc: core: Introduce rproc_register_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     | 159 +++----------------
>  drivers/remoteproc/remoteproc_internal.h |  33 +++-
>  drivers/remoteproc/remoteproc_virtio.c   | 193 ++++++++++++++++++++---
>  include/linux/remoteproc.h               |   6 +-
>  4 files changed, 227 insertions(+), 164 deletions(-)
>
> --
> 2.25.1
>