Message-ID: <ZtbX2NZ6A6ATqQLh@apocalypse>
Date: Tue, 3 Sep 2024 11:33:12 +0200
From: Andrea della Porta <andrea.porta@...e.com>
To: Herve Codina <herve.codina@...tlin.com>
Cc: Rob Herring <robh@...nel.org>,
	Andrea della Porta <andrea.porta@...e.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Derek Kiernan <derek.kiernan@....com>,
	Dragan Cvetic <dragan.cvetic@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Saravana Kannan <saravanak@...gle.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, netdev@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-arch@...r.kernel.org,
	Lee Jones <lee@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	Stefan Wahren <wahrenst@....net>,
	Luca Ceresoli <luca.ceresoli@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 04/11] of: address: Preserve the flags portion on 1:1
 dma-ranges mapping

Hi Herve,

On 11:09 Tue 03 Sep     , Herve Codina wrote:
> Hi,
> 
> On Fri, 30 Aug 2024 14:37:54 -0500
> Rob Herring <robh@...nel.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:

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.

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

Many thanks,
Andrea
 
> Best regards,
> Hervé

Powered by blists - more mailing lists