Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

Phil Elwell
2025-02-17T17:03:48Z

Hi Hervé,

On Mon, 17 Feb 2025 at 15:53, Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Phil,
>
> On Thu, 13 Feb 2025 21:12:43 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>
> > On Thu, 13 Feb 2025, 21:06 Herve Codina, <herve.codina@bootlin.com> wrote:
> > >
> > > Hi Phil,
> > >
> > > On Thu, 13 Feb 2025 20:15:06 +0000
> > > Phil Elwell <phil@raspberrypi.com> wrote:
> > >
> > > > Once more, with plain text, which I'd hoped the Android GMail client
> > > > would work out for itself.
> > > >
> > > > On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:
> > > > >
> > > > > Hi Phil,
> > > > >
> > > > > On Thu, 13 Feb 2025 17:57:37 +0000
> > > > > Phil Elwell <phil@raspberrypi.com> wrote:
> > > > >
> > > > > > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > > > >
> > > > > > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > > > > > includes all these pieces?
> > > > > > > >
> > > > > > > > That depends on whether you count the Raspberry Pi 5 as a custom board.
> > > > > > >
> > > > > > > So you mean the Pi 5 board would itself make use of the resources the
> > > > > > > RP1 device has? They are not simply connected to headers for plugin
> > > > > > > boards, but used by the main board? Hence you want to describe them in
> > > > > > > the board .DTS file.
> > > > > >
> > > > > > That's correct. But even for plug-in devices, those which are on
> > > > > > non-discoverable buses need overlays to declare them, which causes a
> > > > > > problem when the overlay application happens before the kernel is
> > > > > > started.
> > > > > >
> > > > >
> > > > > Hum, I see.
> > > > >
> > > > > We worked on overlay usage on non-discoverable buses wired to a connector
> > > > > and we did a talk about issues we are facing on at Plumber [0].
> > > > >
> > > > > You can also find our big picture in [1] and a last contribution introducing
> > > > > export-symbols feature in [2]. export-symbols is also under discussion on
> > > > > some other threads.
> > > > >
> > > > > Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> > > > > an addon board to add devices on an i2c bus provided by a base board and
> > > > > wired to an connector the addon board is connected to.
> > > > >
> > > > > Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> > > > > addon board and used by the base board using also nexus node.
> > > > >
> > > > > We use a nexus node [4] (not presented at the Plumbers talk because the idea
> > > > > came during 'out of talk' discussions in Plumbers) in order to allow our
> > > > > addon board to use resources provided by the base board.
> > > > >
> > > > > In your case, if I understood, you are in the other direction but why not
> > > > > using also a nexus node to decouple and translate resources in this other
> > > > > direction ?
> > > > >
> > > > > Don't know if this idea can help but feel free to ask for some more
> > > > > information if needed.
> > > >
> > > > Nexus nodes look interesting - I see them as adding a layer of
> > > > abstraction such that, for example, boards can declare which of their
> > > > specific resources performs a common function so that clients can
> > > > treat them all the same. We do the same thing in a limited way by
> > > > using common labels on nodes, but this goes much further.
> > > >
> > > > In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
> > > > dtb declares the connector node and the overlay fills in the content
> > > > with references to its GPIO controller, PWM controller etc. However, I
> > > > think the overlay would also have to be board specific because it's
> > > > not possible to patch part of a property from an overlay, so you'd end
> > > > up overwriting the GPIO number as well as the controller reference.
> > > >
> > > > What is needed to make this work is the ability to cope with
> > > > unresolved references in the base dtb, to be resolved as each overlay
> > > > is applied, with runtime checking that each reference is resolved
> > > > before it is used, all of which sounds like a nightmare. Plus, we
> > > > really don't want to have to change the way all our camera and display
> > > > overlays work on all Raspberry Pis just to accommodate somebody's idea
> > > > of how RP1 should be handled.
> > >
> > > Just to be clear, my comments were not there to tell you how RP1 should
> > > work. I just proposed ideas without trying to force anything and I can
> > > fully understand that ideas proposed don't feed your needs.
> > >
> > > Sorry if my approach was misunderstood.
> >
> > I feel I've been misunderstood - I appreciate your ideas.
> >
> > Perhaps it would help if you could outline how you think we could
> > apply your suggestions?
> >
>
> I was thinking about what your mentioned, i.e. the overlay fill the nexus node.
> No sure to understand why the overlay should patch some properties.
> Also where are the unresolved references in that case. The base DT refers to
> the Nexus node.
> The issue will probably be that the translation performed by the nexus node is
> not available until the overlay is applied. The consumer will see errors other
> than PROBE_DEFER when if probes while the overlay is not applied.

The job of the nexus node would be to translate a generic request for
a numbered resource to a specific request for an RP1 resource with
arbitrary properties. The arbitrary properties could be GPIO offsets,
which are board specific, while the node supplying the resource is
provided by the overlay. This means that an entry in the table,
described by a single property, could have contributions from the base
DT and the overlay, which is not possible since overlays overwrite
whole properties.

Perhaps that particular problem could be overcome by creating a
single-entry map, using the map-mask feature to pass through all of
the GPIO offset and flags to the parent, so that the whole table
becomes a proxy for RP1's GPIO controller. Is that what you had in
mind?

> Also, the solution will lead to memory leak at runtime. Indeed, the overlay
> add properties in an already existing node.
> If the overlay is applied by the Kernel itself, this lead to memory leak when
> the overlay is removed.
> Indeed, an overlay can add/remove node without any issue but it cannot
> add/remove properties to/from existing nodes.

Fortunately for me I'm not arguing _for_ the use of an overlay.

> In the case described here, the nexus node is already present in the DT and the
> overlay add/remove properties to/from this existing node.

I think I can see how that could be made to work for GPIOs. It looks
as though the GPIO subsystem is the only one making use of
of_parse_phandle_with_args_map. Interrupts seem to have an open-coded
equivalent, and iommus. What about I2C and PWM?

Phil