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>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
 		};
 
 		pcie0_phy: phy@1c06000 {
@@ -2318,6 +2328,16 @@ pcie1: pcie@1c08000 {
 			dma-coherent;
 
 			status = "disabled";
+
+			pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				bus-range = <0x01 0xff>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
 		};
 
 		pcie1_phy: phy@1c0e000 {
@@ -2433,6 +2453,16 @@ pcie2: pcie@1c10000 {
 			dma-coherent;
 
 			status = "disabled";
+
+			pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				bus-range = <0x01 0xff>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
 		};
 
 		pcie2_phy: phy@1c16000 {

-- 
2.25.1

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.

If that's the case, I don't think we need to include "bus-range" in DT
for them, do we?

Bjorn

Hi Bjorn,

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.

- Mani

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

On Sun, Jan 05, 2025 at 03:46:12PM +0530, Manivannan Sadhasivam wrote:
> Hi Bjorn,
> 
> 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)

The mistake is using bus number 1 instead of 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.

Sorry I'm late to the party, but found this from another thread 
today[1]. More details there.

You should not have 'bus-range' at all in your DT and the bus for every 
BDF address should be 0. You only need 'bus-range' if your h/w is 
broken.

Rob

[1] https://lore.kernel.org/all/CAL_Jsq+z+5_=YXiyCW1sbKDe0cjGNG7Qk=uRQ3efAFTd1J2ayQ@mail.gmail.com/

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

[+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

On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> [+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.
> 

Device drivers need to parse the properties defined in the device DT node. And
the only way to identify the node is by using its 'reg' property which has the
BDF identifier. This is common to other busses where the device address is
encoded in the 'reg' property.

- Mani

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

On Wed, Jan 15, 2025 at 04:24:31PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> > 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.
> 
> Device drivers need to parse the properties defined in the device DT
> node. And the only way to identify the node is by using its 'reg'
> property which has the BDF identifier. This is common to other
> busses where the device address is encoded in the 'reg' property.

Does this assume there is some firmware to configure these bridges
before Linux boots?  If bridges are completely unconfigured after
power-on, their secondary and subordinate bus numbers will be zero, so
a bus-range property for the bridge can only be an assumption about
what Linux will do.

Bjorn

On Wed, Jan 15, 2025 at 11:42:10AM -0600, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2025 at 04:24:31PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> > > 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.
> > 
> > Device drivers need to parse the properties defined in the device DT
> > node. And the only way to identify the node is by using its 'reg'
> > property which has the BDF identifier. This is common to other
> > busses where the device address is encoded in the 'reg' property.
> 
> Does this assume there is some firmware to configure these bridges
> before Linux boots?

No.

>  If bridges are completely unconfigured after
> power-on, their secondary and subordinate bus numbers will be zero, so
> a bus-range property for the bridge can only be an assumption about
> what Linux will do.
> 

Secondary bus number for sure is not an assumption as it depends on the hardware
topology which linux would know from DT. But subordinate number could be
considered as an assumption.

FWIW, DT is not tied to any OS. So linux assumption doesn't really matter even
though it is the most commonly used reference.

- Mani

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

On Wed, Jan 15, 2025 at 11:29:18PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jan 15, 2025 at 11:42:10AM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 15, 2025 at 04:24:31PM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> > > > 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.
> > > 
> > > Device drivers need to parse the properties defined in the device DT
> > > node. And the only way to identify the node is by using its 'reg'
> > > property which has the BDF identifier. This is common to other
> > > busses where the device address is encoded in the 'reg' property.
> > 
> > Does this assume there is some firmware to configure these bridges
> > before Linux boots?
> 
> No.
> 
> >  If bridges are completely unconfigured after
> > power-on, their secondary and subordinate bus numbers will be zero, so
> > a bus-range property for the bridge can only be an assumption about
> > what Linux will do.
> 
> Secondary bus number for sure is not an assumption as it depends on
> the hardware topology which linux would know from DT. But
> subordinate number could be considered as an assumption.

If there's no firmware and the secondary bus number is 0 when Linux
enumerates the bridge, does Linux know how to get the bus-range from
DT and program the bridge's secondary bus?

And does Linux know how to update the subordinate bus number in the
case where several Root Ports specify 0xff in bus-range?

Bjorn

On Wed, Jan 15, 2025 at 12:13:40PM -0600, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2025 at 11:29:18PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jan 15, 2025 at 11:42:10AM -0600, Bjorn Helgaas wrote:
> > > On Wed, Jan 15, 2025 at 04:24:31PM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> > > > > 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.
> > > > 
> > > > Device drivers need to parse the properties defined in the device DT
> > > > node. And the only way to identify the node is by using its 'reg'
> > > > property which has the BDF identifier. This is common to other
> > > > busses where the device address is encoded in the 'reg' property.
> > > 
> > > Does this assume there is some firmware to configure these bridges
> > > before Linux boots?
> > 
> > No.
> > 
> > >  If bridges are completely unconfigured after
> > > power-on, their secondary and subordinate bus numbers will be zero, so
> > > a bus-range property for the bridge can only be an assumption about
> > > what Linux will do.
> > 
> > Secondary bus number for sure is not an assumption as it depends on
> > the hardware topology which linux would know from DT. But
> > subordinate number could be considered as an assumption.
> 
> If there's no firmware and the secondary bus number is 0 when Linux
> enumerates the bridge, does Linux know how to get the bus-range from
> DT and program the bridge's secondary bus?
> 

Linux doesn't seem to make use of the secondary bus number from DT node of a
bridge, but there is no guarantee that other OSes making use of DT won't do.

> And does Linux know how to update the subordinate bus number in the
> case where several Root Ports specify 0xff in bus-range?
> 

Same answer as above.

- Mani

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

On Sun, Jan 19, 2025 at 08:55:34PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jan 15, 2025 at 12:13:40PM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 15, 2025 at 11:29:18PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jan 15, 2025 at 11:42:10AM -0600, Bjorn Helgaas wrote:
> > > > On Wed, Jan 15, 2025 at 04:24:31PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> > > > > > 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.
> > > > > 
> > > > > Device drivers need to parse the properties defined in the device DT
> > > > > node. And the only way to identify the node is by using its 'reg'
> > > > > property which has the BDF identifier. This is common to other
> > > > > busses where the device address is encoded in the 'reg' property.
> > > > 
> > > > Does this assume there is some firmware to configure these bridges
> > > > before Linux boots?
> > > 
> > > No.
> > > 
> > > >  If bridges are completely unconfigured after
> > > > power-on, their secondary and subordinate bus numbers will be zero, so
> > > > a bus-range property for the bridge can only be an assumption about
> > > > what Linux will do.
> > > 
> > > Secondary bus number for sure is not an assumption as it depends on
> > > the hardware topology which linux would know from DT. But
> > > subordinate number could be considered as an assumption.
> > 
> > If there's no firmware and the secondary bus number is 0 when Linux
> > enumerates the bridge, does Linux know how to get the bus-range from
> > DT and program the bridge's secondary bus?
> 
> Linux doesn't seem to make use of the secondary bus number from DT node of a
> bridge, but there is no guarantee that other OSes making use of DT won't do.
> 
> > And does Linux know how to update the subordinate bus number in the
> > case where several Root Ports specify 0xff in bus-range?
> 
> Same answer as above.

Let me back up; I don't think we're understanding each other.  This
DT:

  pcie@0 {
    bus-range = <0x01 0xff>;

    &pcieport0 {
      wifi@0 {
	reg = <0x10000 0x0 0x0 0x0 0x0>;

says that wifi@0 is at 01:00.0, which is only true if the pcie@0
secondary bus number is 01.  The power-up default is 00, so it's only
01 if either firmware or Linux has programmed it that way.

I claim this DT assumes the pcie@0 secondary bus number is programmed
either by firmware or Linux.  This makes me a bit nervous because
AFAIK there's nothing that guarantees Linux would choose bus 01.

Maybe Linux just needs to get smarter and program it based on the
start of the DT bus-range?

On Tue, Jan 21, 2025 at 05:11:31PM -0600, Bjorn Helgaas wrote:
> On Sun, Jan 19, 2025 at 08:55:34PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jan 15, 2025 at 12:13:40PM -0600, Bjorn Helgaas wrote:
> > > On Wed, Jan 15, 2025 at 11:29:18PM +0530, Manivannan Sadhasivam wrote:
> > > > On Wed, Jan 15, 2025 at 11:42:10AM -0600, Bjorn Helgaas wrote:
> > > > > On Wed, Jan 15, 2025 at 04:24:31PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Mon, Jan 06, 2025 at 05:07:05PM -0600, Bjorn Helgaas wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > Device drivers need to parse the properties defined in the device DT
> > > > > > node. And the only way to identify the node is by using its 'reg'
> > > > > > property which has the BDF identifier. This is common to other
> > > > > > busses where the device address is encoded in the 'reg' property.
> > > > > 
> > > > > Does this assume there is some firmware to configure these bridges
> > > > > before Linux boots?
> > > > 
> > > > No.
> > > > 
> > > > >  If bridges are completely unconfigured after
> > > > > power-on, their secondary and subordinate bus numbers will be zero, so
> > > > > a bus-range property for the bridge can only be an assumption about
> > > > > what Linux will do.
> > > > 
> > > > Secondary bus number for sure is not an assumption as it depends on
> > > > the hardware topology which linux would know from DT. But
> > > > subordinate number could be considered as an assumption.
> > > 
> > > If there's no firmware and the secondary bus number is 0 when Linux
> > > enumerates the bridge, does Linux know how to get the bus-range from
> > > DT and program the bridge's secondary bus?
> > 
> > Linux doesn't seem to make use of the secondary bus number from DT node of a
> > bridge, but there is no guarantee that other OSes making use of DT won't do.
> > 
> > > And does Linux know how to update the subordinate bus number in the
> > > case where several Root Ports specify 0xff in bus-range?
> > 
> > Same answer as above.
> 
> Let me back up; I don't think we're understanding each other.  This
> DT:
> 
>   pcie@0 {
>     bus-range = <0x01 0xff>;
> 
>     &pcieport0 {
>       wifi@0 {
> 	reg = <0x10000 0x0 0x0 0x0 0x0>;
> 
> says that wifi@0 is at 01:00.0, which is only true if the pcie@0
> secondary bus number is 01.  The power-up default is 00, so it's only
> 01 if either firmware or Linux has programmed it that way.
> 
> I claim this DT assumes the pcie@0 secondary bus number is programmed
> either by firmware or Linux.  This makes me a bit nervous because
> AFAIK there's nothing that guarantees Linux would choose bus 01.
> 

Why do you think so? PCI devices are scanned in a depth-first manner, so the
bus number should match the DT order. Like, while scanning the bus under pcie@0,
it should use 01 as the secondary bus number as it is going to be the first bus
after the root bus. I don't know how linux or any other OS would end up using a
different bus number.
 
Also, do note that the bus numbering also takes into account of the domain
number which is different for each controller instance.

Please let me know if my understanding is wrong.

- Mani

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

On Tue, Jan 28, 2025 at 07:15:14PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jan 21, 2025 at 05:11:31PM -0600, Bjorn Helgaas wrote:
> ...

> > Let me back up; I don't think we're understanding each other.  This
> > DT:
> > 
> >   pcie@0 {
> >     bus-range = <0x01 0xff>;
> > 
> >     &pcieport0 {
> >       wifi@0 {
> > 	reg = <0x10000 0x0 0x0 0x0 0x0>;
> > 
> > says that wifi@0 is at 01:00.0, which is only true if the pcie@0
> > secondary bus number is 01.  The power-up default is 00, so it's
> > only 01 if either firmware or Linux has programmed it that way.
> > 
> > I claim this DT assumes the pcie@0 secondary bus number is
> > programmed either by firmware or Linux.  This makes me a bit
> > nervous because AFAIK there's nothing that guarantees Linux would
> > choose bus 01.
> 
> Why do you think so? PCI devices are scanned in a depth-first
> manner, so the bus number should match the DT order. Like, while
> scanning the bus under pcie@0, it should use 01 as the secondary bus
> number as it is going to be the first bus after the root bus. I
> don't know how linux or any other OS would end up using a different
> bus number.

In this case of the first bridge on the root bus, it's very likely
that the secondary bus will be 01.

If there are more bridges, it's dangerous to make assumptions about
their secondary bus numbers because those bus numbers depend on what
hierarchies have already been enumerated and any additional space
assigned in anticipation of hotplug.

I don't know of any spec that requires the OS to assign bus numbers in
a certain way, and it feels kind of subtle if this kind of DT
description is only reliable for things below the first bridge on a
root bus.

Bjorn

On Tue, Jan 28, 2025 at 10:16:12AM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 07:15:14PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 21, 2025 at 05:11:31PM -0600, Bjorn Helgaas wrote:
> > ...
> 
> > > Let me back up; I don't think we're understanding each other.  This
> > > DT:
> > > 
> > >   pcie@0 {
> > >     bus-range = <0x01 0xff>;
> > > 
> > >     &pcieport0 {
> > >       wifi@0 {
> > > 	reg = <0x10000 0x0 0x0 0x0 0x0>;
> > > 
> > > says that wifi@0 is at 01:00.0, which is only true if the pcie@0
> > > secondary bus number is 01.  The power-up default is 00, so it's
> > > only 01 if either firmware or Linux has programmed it that way.
> > > 
> > > I claim this DT assumes the pcie@0 secondary bus number is
> > > programmed either by firmware or Linux.  This makes me a bit
> > > nervous because AFAIK there's nothing that guarantees Linux would
> > > choose bus 01.
> > 
> > Why do you think so? PCI devices are scanned in a depth-first
> > manner, so the bus number should match the DT order. Like, while
> > scanning the bus under pcie@0, it should use 01 as the secondary bus
> > number as it is going to be the first bus after the root bus. I
> > don't know how linux or any other OS would end up using a different
> > bus number.
> 
> In this case of the first bridge on the root bus, it's very likely
> that the secondary bus will be 01.
> 
> If there are more bridges, it's dangerous to make assumptions about
> their secondary bus numbers because those bus numbers depend on what
> hierarchies have already been enumerated and any additional space
> assigned in anticipation of hotplug.
> 

The enumeration hierarchy should match the DT order. I'm not sure how there can
be a deviation if the firmware enumerates the bridges in order. Or you are
fearing that the firmware *could* do something crazy?

> I don't know of any spec that requires the OS to assign bus numbers in
> a certain way, and it feels kind of subtle if this kind of DT
> description is only reliable for things below the first bridge on a
> root bus.
> 

This is almost similar to how domain numbers are assigned today. For DT based
systems, 'linux,pci-domain' property is parsed to find the domain number if
available. And this domain number is not guaranteed to match the order in which
the RC nodes are defined in DT.

But I guess no one bothered to parse 'bus-range' property for PCI bridges since
it is not bound to change.

- Mani

--