Message-ID: <20250115172304.5jxgot6enijumbqy@thinkpad>
Date: Wed, 15 Jan 2025 22:53:04 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Krishna Chaitanya Chundru <quic_krichai@...cinc.com>,
	andersson@...nel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	cros-qcom-dts-watchers@...omium.org,
	Jingoo Han <jingoohan1@...il.com>,
	Bartosz Golaszewski <brgl@...ev.pl>, quic_vbadigan@...cinc.com,
	linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615

On Tue, Jan 07, 2025 at 04:42:44PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 24, 2024 at 11:49:42AM +0200, Dmitry Baryshkov wrote:
> > On Tue, Dec 24, 2024 at 02:41:10PM +0530, Krishna Chaitanya Chundru wrote:
> > > On 12/5/2024 2:55 AM, Bjorn Helgaas wrote:
> > > > On Tue, Nov 12, 2024 at 08:31:33PM +0530, Krishna chaitanya chundru wrote:
> > > > > Add binding describing the Qualcomm PCIe switch, QPS615,
> > > > > which provides Ethernet MAC integrated to the 3rd downstream port
> > > > > and two downstream PCIe ports.
> 
> > > > > +    pcie {
> > > > > +        #address-cells = <3>;
> > > > > +        #size-cells = <2>;
> > > > > +
> > > > > +        pcie@0 {
> > > > > +            device_type = "pci";
> > > > > +            reg = <0x0 0x0 0x0 0x0 0x0>;
> > > > > +
> > > > > +            #address-cells = <3>;
> > > > > +            #size-cells = <2>;
> > > > > +            ranges;
> > > > > +            bus-range = <0x01 0xff>;
> > > > > +
> > > > > +            pcie@0,0 {
> > > > > +                compatible = "pci1179,0623";
> > > > > +                reg = <0x10000 0x0 0x0 0x0 0x0>;
> > > > > +                device_type = "pci";
> > > > > +                #address-cells = <3>;
> > > > > +                #size-cells = <2>;
> > > > > +                ranges;
> > > > > +                bus-range = <0x02 0xff>;
> > > > 
> > > > This binding describes a switch.  I don't think bus-range should
> > > > appear here at all because it is not a feature of the hardware (unless
> > > > the switch ports are broken and their Secondary/Subordinate Bus
> > > > Numbers are hard-wired).
> > > > 
> > > > The Primary/Secondary/Subordinate Bus Numbers of all switch ports
> > > > should be writable and the PCI core knows how to manage them.
> > > 
> > > The dt binding check is throwing an error if we don't keep bus-range
> > > property for that reason we added it, from dt binding perspective i think it
> > > is mandatory to add this property.
> > 
> > Could you please provide an error message? I don't see any of the PCIe
> > bindingins declaring bus-range as mandatory. I might be missing it
> > though.
> 
> I think the warning message is like this:
> 
>   Warning (pci_device_bus_num): /soc@...cie@...0000/pcie@...ifi@0: PCI bus number 1 out of range, expected (0 - 0)
> 
> and only happens if there's a device below a Root Port or a Switch.
> In that case the device "reg" property apparently has to include the
> bus/device/function.
> 
> IIUC, in this case, we're describing a Switch with an integrated
> Ethernet MAC:
> 
>   pcie@0 {
>     device_type = "pci";
>     reg = <0x0 0x0 0x0 0x0 0x0>;           # 00:00.0 RP to [bus 01-ff]
>     bus-range = <0x01 0xff>;
> 
>     pcie@0,0 {
>       compatible = "pci1179,0623";
>       reg = <0x10000 0x0 0x0 0x0 0x0>;     # 01:00.0 Switch USP to [bus 02-ff]
>       device_type = "pci";
>       bus-range = <0x02 0xff>;
> 
>       pcie@1,0 {
>         reg = <0x20800 0x0 0x0 0x0 0x0>;   # 02:01.0 Switch DSP to [bus 03-ff]
>         device_type = "pci";
>         bus-range = <0x03 0xff>;
>         qcom,no-dfe-support;
>       };
> 
>       pcie@2,0 {
>         reg = <0x21000 0x0 0x0 0x0 0x0>;   # 02:02.0 Switch DSP to [bus 04-ff]
>         device_type = "pci";
>         bus-range = <0x04 0xff>;
>         qcom,nfts = <10>;
>       };
> 
>       pcie@3,0 {
>         reg = <0x21800 0x0 0x0 0x0 0x0>;   # 02:02.1 Switch DSP to [bus 05-ff]
>         device_type = "pci";
>         bus-range = <0x05 0xff>;
>         qcom,tx-amplitude-millivolt = <10>;
> 
>         pcie@0,0 {
>           reg = <0x50000 0x0 0x0 0x0 0x0>; # 05:00.0 Ethernet MAC, I guess?
>           device_type = "pci";
>           qcom,l1-entry-delay-ns = <10>;
>         };
> 
>         ...
>       };
>     };
>   };
> 
> So I think the bus-range properties are needed to match the reg
> properties of the downstream devices.
> 
> I do think the bus-ranges of the Switch Downstream Ports look bogus
> because they all extend to bus ff, so they overlap.  The Switch
> wouldn't know how to route config transactions to the correct DSP.
> I suppose the PCI core would fix these overlaps at boot time, but 
> it seems wrong to describe them this way here.
> 

Yeah, max bus range is really a dynamic value. But I don't know how we could
define a legal value other than 'ff' statically.

Open firmware PCI bus binding defines 'bus-range' as:

"Two integers, each encoded as with encode-int, the first representing the bus
number of the PCI bus implemented by the bus controller represented by this node
(the secondary bus number in PCI-to-PCI bridge nomenclature), and the second
representing the largest bus number of any PCI bus in the portion of the PCI
domain that is subordinate to this node (the subordinate bus number in
PCI-to-PCI bridge nomenclature)."

https://www.openfirmware.info/data/docs/bus.pci.pdf

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists