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

[Herve Codina <herve.codina@bootlin.com>]

On Mon, 17 Feb 2025 17:03:34 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

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

Hum, I am a bit lost.
Some DT example (base and overlay) could help me to understand.

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

Support for PWM has been recently accepted.
  https://lore.kernel.org/all/ufl4kwrjyp4zid4muvghefevqc6hk3zyvxnsu72fxd4f46fzg6@hufkci2dzjid/

For i2c, nexus node is not suitable.

Nexus node works well when resources are indexed (gpio line in a gpio chip
for instance). For bus controller there is no index.
I mean we never refer a i2c bus controller using <&i2c-ctrl 12>.

For i2c, I proposed i2c bus extension:
  https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/

Best regards,
Hervé