mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Francesco Valla <francesco@valla.it>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Kees Cook <kees@kernel.org>,
	 "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	 Robin Murphy <robin.murphy@arm.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh@kernel.org>,
	 Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>, Frank Li <Frank.Li@nxp.com>,
	 Peng Fan <peng.fan@nxp.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	 linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org,  virtualization@lists.linux.dev,
	imx@lists.linux.dev, iommu@lists.linux.dev,
	 linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RFC 12/12] PoC: arm64: dts: imx93-11x11-frdm: add multiple vdevs
Date: Wed, 23 Sep 2026 20:42:23 +0200	[thread overview]
Message-ID: <arP5AvA51GHrI8aN@bywater> (raw)
In-Reply-To: <arP06mWDehsIDh5s@p14s>

On Wed, Sep 23, 2026 at 09:48:58AM -0600, Mathieu Poirier wrote:
> On Tue, Sep 22, 2026 at 10:19:48PM +0200, Francesco Valla wrote:
> > On Tue, Sep 22, 2026 at 09:43:52AM -0600, Mathieu Poirier wrote:
> > > On Wed, Sep 16, 2026 at 11:10:57PM +0200, Francesco Valla wrote:
> > > > Add rings for multiple vdevs, as well as the required virtio nodes for
> > > > I2C, SPI and GPIO functionalities. On top of that, add example
> > > > peripherals using all of them.
> > > > 
> > > > NOTE: this is a Proof-Of-Concept, not meant to be integrated!
> > > > 
> > > > Signed-off-by: Francesco Valla <francesco@valla.it>
> > > > ---
> > > >  arch/arm64/boot/dts/freescale/imx93-11x11-frdm.dts | 128 +++++++++++++++++++--
> > > >  1 file changed, 119 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx93-11x11-frdm.dts b/arch/arm64/boot/dts/freescale/imx93-11x11-frdm.dts
> > > > index bd14ba28690c..dfa3b122ac5f 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx93-11x11-frdm.dts
> > > > +++ b/arch/arm64/boot/dts/freescale/imx93-11x11-frdm.dts
> > > > @@ -53,6 +53,32 @@ button-k3 {
> > > >  		};
> > > >  	};
> > > >  
> > > > +	gpio-keys-virtio {
> > > > +		compatible = "gpio-keys-polled";
> > > > +		poll-interval = <100>;
> > > > +
> > > > +		button-v1 {
> > > > +			label = "Button V1";
> > > > +			linux,code = <BTN_3>;
> > > > +			gpios = <&v_gpio 23 GPIO_ACTIVE_LOW>;
> > > > +		};
> > > > +
> > > > +		button-v2 {
> > > > +			label = "Button V2";
> > > > +			linux,code = <BTN_4>;
> > > > +			gpios = <&v_gpio 24 GPIO_ACTIVE_LOW>;
> > > > +		};
> > > > +	};
> > > > +
> > > > +	leds {
> > > > +		compatible = "gpio-leds";
> > > > +
> > > > +		led {
> > > > +			gpios = <&v_gpio 18 GPIO_ACTIVE_HIGH>;
> > > > +			label = "LED V";
> > > > +		};
> > > > +	};
> > > > +
> > > >  	reg_usdhc2_vmmc: regulator-usdhc2 {
> > > >  		compatible = "regulator-fixed";
> > > >  		off-on-delay-us = <12000>;
> > > > @@ -89,11 +115,6 @@ linux,cma {
> > > >  			linux,cma-default;
> > > >  		};
> > > >  
> > > > -		rsc_table: rsc-table@2021e000 {
> > > > -			reg = <0 0x2021e000 0 0x1000>;
> > > > -			no-map;
> > > > -		};
> > > > -
> > > 
> > > Why is the resource table removed?  There is no mention of that in the
> > > changelog...
> > >
> > 
> > You are obviously right, the commit message here should have been a
> > poem, not a form of hermetic poetry. My bad.
> > 
> > The resource table here is causing problems with how Zephyr is managing
> > it at its side. If it is kept in a separate memory location and copied
> > there at runtime by the remote processor firmware during its startup
> > (which is the current Zephyr behavior), then there might be a race
> > condition when the aforesaid firmware is loaded and started by Linux
> > *and* at least one of the vdev drivers (here including rpmsg_bus) is
> > built-in. In this case, the copy of the resource table done by the
> > remote processor might - depending on the async execution of the two
> > processors - overwrite the status bit set by the Linux driver:
> > 
> >   Firmware load and startup (echo start > /sys/.../state)
> >                            |
> > 			   |
> > 			   V
> >   The vdev devices get registered (by register_virtio_device())
> >                            |
> > 			   |
> > 			   V
> >   If a driver is built-in, it probes and sets the vdev status
> >   inside the resource table @rsc-table.
> >                            .
> > 			   . (in the mean time)
> > 			   .
> >   The remote processor starts up and copies the resource table from its
> >   dedicated section to @rsc-table.
> > 
> > Depending on the system load and the complexity of the firmware, the two
> > operations can happen in whatever sequence, causing a race condition.
> 
> This would happen regardless of this patchset.
>

Correct, *if* the remote processor is copying the resource table to a
specific location and expects the host to use that. On Zephyr (which is
clearly outside the scope here) this can be enabled through the
CONFIG_OPENAMP_COPY_RSC_TABLE option. I am keeping that disabled, and
removing the rsc-table node here.

I am planning to reason on this and propose a proper fix in a separate
patchset.

> > 
> > This is somewhat masked if vdev drivers are built as modules, as the
> > devices does not probe immediately but only after the modules have been
> > loaded, giving the remote processor time to start. Note that this is not
> > a solution! but a workaround.
> > 
> > If the rsc-table node is not there, the startup logic falls back to the
> > classic rproc_elf_find_loaded_rsc_table().
> 
> We can't remove @rsc-table to make a problem go away.  
> 

I need to re-take a look at the NXP SDK to understand what's the real
purpose of having the rsc-table here. Judging from the commit message
that introduced support for such facility [1], it seems the SDK is not
really using it.

Maybe someone from NXP can comment on this?

> > 
> > This is specific to i.MX platforms [1] and is probably not normally an
> > issue because - as stated in [1] - the offical SDK from NXP seems not
> > to check the status inside the resource table.
> > 
> > > >  		vdev0vring0: vdev0vring0@a4000000 {
> > > >  			reg = <0 0xa4000000 0 0x8000>;
> > > >  			no-map;
> > > > @@ -105,12 +126,42 @@ vdev0vring1: vdev0vring1@a4008000 {
> > > >  		};
> > > >  
> > > >  		vdev1vring0: vdev1vring0@a4010000 {
> > > > -			reg = <0 0xa4010000 0 0x8000>;
> > > > +			reg = <0 0xa4010000 0 0x1000>;
> > > > +			no-map;
> > > > +		};
> > > > +
> > > > +		vdev2vring0: vdev2vring0@a4011000 {
> > > > +			reg = <0 0xa4011000 0 0x2000>;
> > > > +			no-map;
> > > > +		};
> > > > +
> > > > +		vdev2vring1: vdev2vring1@a4013000 {
> > > > +			reg = <0 0xa4013000 0 0x2000>;
> > > > +			no-map;
> > > > +		};
> > > > +
> > > > +		vdev3vring0: vdev3vring0@a4015000 {
> > > > +			reg = <0 0xa4015000 0 0x2000>;
> > > > +			no-map;
> > > > +		};
> > > > +
> > > > +		vdev4vring0: vdev4vring0@a4017000 {
> > > > +			reg = <0 0xa4017000 0 0x4000>;
> > > > +			no-map;
> > > > +		};
> > > > +
> > > > +		vdev5vring0: vdev5vring0@a401B000 {
> > > > +			reg = <0 0xa401B000 0 0x2000>;
> > > > +			no-map;
> > > > +		};
> > > > +
> > > > +		vdev5vring1: vdev5vring1@a401D000 {
> > > > +			reg = <0 0xa401D000 0 0x2000>;
> > > >  			no-map;
> > > >  		};
> > > >  
> > > > -		vdev1vring1: vdev1vring1@a4018000 {
> > > > -			reg = <0 0xa4018000 0 0x8000>;
> > > > +		vdev5vring2: vdev5vring2@a401F000 {
> > > > +			reg = <0 0xa401F000 0 0x1000>;
> > > >  			no-map;
> > > >  		};
> > > >  
> > > > @@ -149,8 +200,67 @@ &cm33 {
> > > >  		 <&mu1 3 1>;
> > > >  	mbox-names = "tx", "rx", "rxdb";
> > > >  	memory-region = <&vdevbuffer>, <&vdev0vring0>, <&vdev0vring1>,
> > > > -			<&vdev1vring0>, <&vdev1vring1>, <&rsc_table>;
> > > > +			<&vdev1vring0>, <&vdev2vring0>, <&vdev2vring1>,
> > > > +			<&vdev3vring0>, <&vdev4vring0>,
> > > > +			<&vdev5vring0>, <&vdev5vring1>, <&vdev5vring2>;
> > > 
> > > Who is using vdev5 vrings?  
> > > 
> > 
> > Another thing that should have been in the commit message. Vdevs are
> > defined, in the Zephyr application I am using as PoC, as follows:
> > 
> >  - vdev0: RPMSG (tx and rx vrings)
> 
> To be backward compatible, there has to be a way to make vdev0 defaulting to
> RPMSG.  It would also be nice if bindings were define for RPMSG so that it can
> show up in the list of remoteproc-virtio devices like i2c, gpio and others.
>

What vdev type is inside vdev0 is up to the resource table (i.e., to the
firmware running on the remote processor), not the remoteproc-virtio
framework.

Defining bindings for rpmsg would today be... pointless? Since no
hardware needs to be described for it. The same goes for virtio-can and
virtio-entropy.

> 
> >  - vdev1: entropy (single request vring)
> >  - vdev2: GPIO (request and event vrings)
> >  - vdev3: I2C (single request vring)
> >  - vdev4: SPI (single request vring)
> >  - vdev5: CAN (tx, rx and control vrings)
> > 
> > vdev5 is not represented inside the devicetree because the can-virtio
> > driver registers a single CAN network device and has thus no need for
> > such representation.
> 
> Then why is it part of the remoteproc's memory-regions?
> 

Because a vring description needs to be present for it, or the
remoteproc-virtio won't be able to fill the entry inside the resource
table.

> > 
> > > >  	status = "okay";
> > > > +
> > > > +	virtio {
> > > > +		#address-cells = <1>;
> > > > +		#size-cells = <0>;
> > > > +
> > > > +		vdev@2 {
> > > > +			reg = <2>;
> > > > +
> > > > +			v_gpio: gpio {
> > > > +				compatible = "virtio,device29";
> > > > +				gpio-controller;
> > > > +				#gpio-cells = <2>;
> > > > +				interrupt-controller;
> > > > +				#interrupt-cells = <2>;
> > > > +			};
> > > > +		};
> > > > +
> > > > +		vdev@3 {
> > > > +			reg = <3>;
> > > > +
> > > > +			i2c {
> > > > +				compatible = "virtio,device22";
> > > > +				#address-cells = <1>;
> > > > +				#size-cells = <0>;
> > > > +
> > > > +				eeprom@50 {
> > > > +					compatible = "atmel,24c1025";
> > > > +					reg = <0x50>;
> > > > +				};
> > > > +			};
> > > > +		};
> > > > +
> > > > +		vdev@4 {
> > > > +			reg = <4>;
> > > > +
> > > > +			spi {
> > > > +				compatible = "virtio,device2d";
> > > > +				#address-cells = <1>;
> > > > +				#size-cells = <0>;
> > > > +
> > > > +				sram@0 {
> > > > +					compatible = "microchip,mchp23k256";
> > > > +					reg = <0>;
> > > > +					spi-max-frequency = <20000000>;
> > > > +				};
> > > > +
> > > > +				lcd@1 {
> > > > +					compatible = "adafruit,yx240qv29", "ilitek,ili9341";
> > > > +					reg = <1>;
> > > > +					spi-max-frequency = <10000000>;
> > > > +					dc-gpios = <&v_gpio 21 GPIO_ACTIVE_HIGH>;
> > > > +					reset-gpios = <&v_gpio 20 GPIO_ACTIVE_HIGH>;
> > > > +					rotation = <90>;
> > > > +				};
> > > > +			};
> > > > +		};
> > > > +	};
> > > >  };
> > > >  
> > > >  &eqos {
> > > > 
> > > > -- 
> > > > 2.55.0
> > > >
> > 
> > Thank you!
> > 
> > Regards,
> > Francesco
> > 
> > 

Thank you for the discussion - is helping in defining further
requirements and constraints.

Regards,
Francesco

[1] https://lore.kernel.org/all/20240719-imx_rproc-v2-2-10d0268c7eb1@nxp.com/



  reply	other threads:[~2026-09-23 18:42 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-16 21:10 [PATCH RFC 00/12] remoteproc: add support for any virtio device Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 01/12] remoteproc: virtio: cleanup rproc_add_virtio_dev error path Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 02/12] remoteproc: virtio: replace commas with semicolons Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 03/12] remoteproc: virtio: support dynamic number of vrings Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 04/12] dma-coherent: add base and size APIs Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 05/12] remoteproc: always report VIRTIO_F_VERSION_1 feature Francesco Valla
2026-09-21 15:47   ` Mathieu Poirier
2026-09-22  6:32     ` Francesco Valla
2026-09-22 15:10       ` Mathieu Poirier
2026-09-16 21:10 ` [PATCH RFC 06/12] remoteproc: virtio: add bounce buffering for data buffers Francesco Valla
2026-09-22 15:58   ` Mathieu Poirier
2026-09-22 19:39     ` Francesco Valla
2026-09-23 14:44       ` Mathieu Poirier
2026-09-23 16:05         ` Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 07/12] dt-bindings: spi: add bindings for spi-virtio Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 08/12] dt-bindings: remoteproc: add remoteproc-virtio Francesco Valla
2026-09-22 15:40   ` Mathieu Poirier
2026-09-22 19:44     ` Francesco Valla
2026-09-23 14:56       ` Mathieu Poirier
2026-09-16 21:10 ` [PATCH RFC 09/12] remoteproc: search for a fwnode during vdev registration Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 10/12] remoteproc: imx_rproc: always use non-blocking mailboxes Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 11/12] dt-bindings: remoteproc: imx-rproc: support virtio Francesco Valla
2026-09-16 21:10 ` [PATCH RFC 12/12] PoC: arm64: dts: imx93-11x11-frdm: add multiple vdevs Francesco Valla
2026-09-22 15:43   ` Mathieu Poirier
2026-09-22 20:19     ` Francesco Valla
2026-09-23 15:48       ` Mathieu Poirier
2026-09-23 18:42         ` Francesco Valla [this message]
2026-09-18 16:53 ` [PATCH RFC 00/12] remoteproc: add support for any virtio device Mathieu Poirier
2026-09-19  7:33   ` Francesco Valla
2026-09-21  3:31     ` Mathieu Poirier
2026-09-22  6:28       ` Francesco Valla
2026-09-22 13:53         ` Mathieu Poirier
2026-09-23 15:13 ` Robin Murphy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=arP5AvA51GHrI8aN@bywater \
    --to=francesco@valla.it \
    --cc=Frank.Li@nxp.com \
    --cc=andersson@kernel.org \
    --cc=broonie@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gustavoars@kernel.org \
    --cc=imx@lists.linux.dev \
    --cc=iommu@lists.linux.dev \
    --cc=kees@kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=m.szyprowski@samsung.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=peng.fan@nxp.com \
    --cc=robh@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=s.hauer@pengutronix.de \
    --cc=virtualization@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®