Re: [PATCH v2 01/21] arm64: dts: qcom: sm8250: Add PCIe bridge node

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Nícolas]

On Sun, Jan 05, 2025 at 03:46:12PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jan 03, 2025 at 03:05:31PM -0600, Bjorn Helgaas wrote:
> > On Thu, Mar 21, 2024 at 04:46:21PM +0530, Manivannan Sadhasivam wrote:
> > > On Qcom SoCs, the PCIe host bridge is connected to a single PCIe bridge
> > > for each controller instance. Hence, add a node to represent the bridge.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sm8250.dtsi | 30 ++++++++++++++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > index 39bd8f0eba1e..fe5485256b22 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 {
> > >  			dma-coherent;
> > >  
> > >  			status = "disabled";
> > > +
> > > +			pcie@0 {
> > > +				device_type = "pci";
> > > +				reg = <0x0 0x0 0x0 0x0 0x0>;
> > > +				bus-range = <0x01 0xff>;
> > 
> > Hi Mani, most or all of the patches in this series add this
> > "bus-range" property.  IIUC, these are all Root Ports and hence the
> > secondary/subordinate bus numbers should be programmable.
> 
> Right. It is not a functional dependency.
> 
> > If that's the case, I don't think we need to include "bus-range" in DT
> > for them, do we?
> 
> We mostly include it to silence the below bindings check for the
> endpoint device node:
> 
> Warning (pci_device_bus_num): /soc@0/pcie@1c00000/pcie@0/wifi@0: PCI bus number 1 out of range, expected (0 - 0)
> 
> DTC check is happy if the 'bus-range' property is absent in the
> bridge node. But while validating the endpoint node (if defined), it
> currently relies on the parent 'bus-range' property to verify the
> bus number provided in the endpoint 'reg' property.
> 
> I don't know else the check can verify the correctness of the
> endpoint bus number. So deferring to Rob here.

I should know more about how this works in DT, but I don't.

I guess https://git.kernel.org/linus/83d2a0a1e2b9 ("arm64: dts: qcom:
sm8250: Add PCIe bridge node") added this (subsequently renamed to
"pcieport0"):

  +			pcie@0 {
  +				device_type = "pci";
  +				reg = <0x0 0x0 0x0 0x0 0x0>;
  +				bus-range = <0x01 0xff>;

which is used at places like
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?id=v6.12#n788:

  &pcieport0 {
	  wifi@0 {
		  compatible = "pci17cb,1101";
		  reg = <0x10000 0x0 0x0 0x0 0x0>;

Based on
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci.txt?id=v6.12#n46
(which is written for Root Ports and Switch Ports, but presumably
applies to endpoints like wifi as well), "reg" contains the device's
bus/device/function:

  - reg:
     Identifies the PCI-PCI bridge. As defined in the IEEE Std 1275-1994
     document, it is a five-cell address encoded as (phys.hi phys.mid
     phys.lo size.hi size.lo). phys.hi should contain the device's BDF as
     0b00000000 bbbbbbbb dddddfff 00000000. The other cells should be zero.

So 0x10000 would decode to 01:00.0, which matches the <1 1> bus-range.

I don't know the reason for requiring the BDF there, but the venerable
https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf, sec
4.1.1, says "reg" is mandatory for PCI Child Nodes, and the first
entry must be the config space address (bus/device/function).

I suppose maybe the BDF is needed to associate the properties with the
correct device, and if the OS were to reprogram the bridge secondary
bus number, it would have to remember the original value to preserve
this association.  I don't think Linux *does* remember that, but it
also generally leaves the bridge bus numbers alone.

Bjorn