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

[Rob Herring]

On Tue, Sep 3, 2024 at 4:33 AM Andrea della Porta <andrea.porta@xxxxxxxx> wrote:
>
> Hi Herve,
>
> On 11:09 Tue 03 Sep     , Herve Codina wrote:
> > Hi,
> >
> > On Fri, 30 Aug 2024 14:37:54 -0500
> > Rob Herring <robh@xxxxxxxxxx> 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:

Might not be the last time I forget this...

> I second this, on RP1 there are multiple BARs too, but for this minimal
> implementation we need only one. Splitting them in one bus per BAR or
> merging them with multiple ranges entries depend on whether the peripherals
> can access different BARs simultaneously. Besides this contraint, I would
> say both approach are viable.

Okay. You all define what you need as you understand the devices better than me.


> >       ...
> >       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.
> >
>
> Agreed for your scenario. Anyway, since the dtbo and driver are shipped together,
> we are free to change the name anytime without impacting anything.

Indeed, no one should care what the nodename is. However, that doesn't
mean you don't have to define something. Really, just 'bus' should be
enough as node names should only be what class a node is. If it is not
enough, then really you need some sort of compatible to identify the
kind of 'bus'. If you want 'pci-ep-bus', then that's fine.


Rob