Re: [PATCH 04/11] of: address: Preserve the flags portion on 1:1 dma-ranges mapping

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

Hi,

On Fri, 30 Aug 2024 14:37:54 -0500
Rob Herring <robh@kernel.org> wrote:

...

> > this view is much like Bootlin's approach, also my pci-ep-bus node now would look
> > like this:
> >  ...
> >  pci-ep-bus@0 {
> >         ranges = <0xc0 0x40000000
> >                   0x01 0x00 0x00000000
> >                   0x00 0x00400000>;
> >         ...
> >  };
> >
> > and also the correct unit address here is 0 again, since the parent address in
> > ranges is 0x01 0x00 0x00000000 (0x01 is the flags and in this case represent
> > BAR1, I assume that for the unit address I should use only the address part that
> > is 0, right?).  
> 
> No, it should be 1 for BAR1. It's 1 node per BAR.

It should be 1 node per BAR but in some cases it is not.

Indeed, in the LAN966x case, the pci-ep-bus need to have access to several
BARs and we have:
	...
	pci-ep-bus@0 {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <1>;

		/*
		 * map @0xe2000000 (32MB) to BAR0 (CPU)
		 * map @0xe0000000 (16MB) to BAR1 (AMBA)
		 */
		ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
		          0xe0000000 0x01 0x00 0x00 0x1000000>;
	...

Some devices under this bus need to use both BARs and use two regs values
in their reg properties to access BAR0 and BAR1.


> > > > > The assumption so far with all of this is that you have some specific
> > > > > PCI device (and therefore a driver). The simple-buses under it are
> > > > > defined per BAR. Not really certain if that makes sense in all cases,
> > > > > but since the address assignment is dynamic, it may have to. I'm also
> > > > > not completely convinced we should reuse 'simple-bus' here or define
> > > > > something specific like 'pci-bar-bus' or something.  
> > > >
> > > > Good point. Labeling a new bus for this kind of 'appliance' could be
> > > > beneficial to unify the dt overlay approach, and I guess it could be
> > > > adopted by the aforementioned Bootlin's Microchip patchset too.
> > > > However, since the difference with simple-bus would be basically non
> > > > existent, I believe that this could be done in a future patch due to
> > > > the fact that the dtbo is contained into the driver itself, so we do
> > > > not suffer from the proliferation that happens when dtb are managed
> > > > outside.  
> > >
> > > It's an ABI, so we really need to decide first.  
> >
> > Okay. How should we proceed?  
> 
> I think simple-bus where you have it is fine. It is really 1 level up
> that needs to be specified. Basically something that's referenced from
> the specific PCI device's schema (e.g. the RP1 schema (which you are
> missing)).
> 
> That schema needs to roughly look like this:
> 
> properties:
>   "#address-cells":
>     const: 3
>   "#size-cells":
>     const: 2
>   ranges:
>     minItems: 1
>     maxItems: 6
>     items:
>       additionalItems: true
>       items:
>         - maximum: 5  # The BAR number
>         - const: 0
>         - const: 0
>         - # TODO: valid PCI memory flags
> 
> patternProperties:
>   "^bar-bus@[0-5]$":
>     type: object
>     additionalProperties: true
>     properties:
>       compatible:
>         const: simple-bus
>       ranges: true
> 

IMHO, the node should not have 'bar' in the name.
In the LAN966x PCI use case, multiple BARs have to be accessed by devices
under this simple-bus. That's why I choose pci-ep-bus for this node name.

Best regards,
Hervé