[RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

During last year's Linux Plumbers we had several discussions centered
around the need to power-on PCI devices before they can be detected on
the bus.

The consensus during the conference was that we need to introduce a
class of "PCI slot drivers" that would handle the power-sequencing.

After some additional brain-storming with Manivannan and the realization
that the DT maintainers won't like adding any "fake" nodes not
representing actual devices, we decided to reuse the existing
infrastructure provided by the PCIe port drivers.

The general idea is to instantiate platform devices for child nodes of
the PCIe port DT node. For those nodes for which a power-sequencing
driver exists, we bind it and let it probe. The driver then triggers a
rescan of the PCI bus with the aim of detecting the now powered-on
device. The device will consume the same DT node as the platform,
power-sequencing device. We use device links to make the latter become
the parent of the former.

The main advantage of this approach is not modifying the existing DT in
any way and especially not adding any "fake" platform devices.

Bartosz Golaszewski (9):
  arm64: dts: qcom: sm8250: describe the PCIe port
  arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390
  PCI/portdrv: create platform devices for child OF nodes
  PCI: hold the rescan mutex when scanning for the first time
  PCI/pwrseq: add pwrseq core code
  dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros
  dt-bindings: wireless: ath11k: describe QCA6390
  PCI/pwrseq: add a pwrseq driver for QCA6390
  arm64: defconfig: enable the PCIe power sequencing for QCA6390

 .../net/wireless/qcom,ath11k-pci.yaml         |  14 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   1 +
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  24 +++
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
 arch/arm64/configs/defconfig                  |   2 +
 drivers/pci/pcie/Kconfig                      |   2 +
 drivers/pci/pcie/Makefile                     |   2 +
 drivers/pci/pcie/portdrv.c                    |   3 +-
 drivers/pci/pcie/pwrseq/Kconfig               |  19 ++
 drivers/pci/pcie/pwrseq/Makefile              |   4 +
 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
 drivers/pci/pcie/pwrseq/pwrseq.c              |  83 ++++++++
 drivers/pci/probe.c                           |   2 +
 include/linux/pcie-pwrseq.h                   |  24 +++
 14 files changed, 386 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/pcie/pwrseq/Kconfig
 create mode 100644 drivers/pci/pcie/pwrseq/Makefile
 create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
 create mode 100644 drivers/pci/pcie/pwrseq/pwrseq.c
 create mode 100644 include/linux/pcie-pwrseq.h

-- 
2.40.1

[RFC 1/9] arm64: dts: qcom: sm8250: describe the PCIe port

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Improve the description of the PCIe topology by defining the port node
at the SoC level.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 760501c1301a..fef9c314ce55 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2197,6 +2197,16 @@ pcie0: pcie@1c00000 {
 			dma-coherent;
 
 			status = "disabled";
+
+			pcieport0: pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+
+				bus-range = <0x01 0xff>;
+			};
 		};
 
 		pcie0_phy: phy@1c06000 {
-- 
2.40.1

[RFC 2/9] arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Describe the ath11k WLAN on-board the QCA6390 module. Include the
relevant regulators and the enable GPIO.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index cd0db4f31d4a..721f86af952b 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -734,6 +734,22 @@ &pcie0_phy {
 	vdda-pll-supply = <&vreg_l9a_1p2>;
 };
 
+&pcieport0 {
+	wifi@0 {
+		compatible = "pci17cb,1101";
+		reg = <0x10000 0x0 0x0 0x0 0x0>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wlan_en_state>;
+
+		enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
+
+		vddpmu-supply = <&vreg_s2f_0p95>;
+		vddpcie1-supply = <&vreg_s8c_1p3>;
+		vddpcie2-supply = <&vreg_s5a_1p9>;
+	};
+};
+
 &pcie1 {
 	status = "okay";
 };
@@ -1303,6 +1319,14 @@ sdc2_card_det_n: sd-card-det-n-state {
 		function = "gpio";
 		bias-pull-up;
 	};
+
+	wlan_en_state: wlan-default-state {
+		pins = "gpio20";
+		function = "gpio";
+		drive-strength = <16>;
+		output-low;
+		bias-pull-up;
+	};
 };
 
 &uart6 {
-- 
2.40.1

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390

[Dmitry Baryshkov]

On Thu, 4 Jan 2024 at 15:03, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Describe the ath11k WLAN on-board the QCA6390 module. Include the
> relevant regulators and the enable GPIO.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> index cd0db4f31d4a..721f86af952b 100644
> --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> @@ -734,6 +734,22 @@ &pcie0_phy {
>         vdda-pll-supply = <&vreg_l9a_1p2>;
>  };
>
> +&pcieport0 {
> +       wifi@0 {
> +               compatible = "pci17cb,1101";
> +               reg = <0x10000 0x0 0x0 0x0 0x0>;
> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&wlan_en_state>;
> +
> +               enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
> +
> +               vddpmu-supply = <&vreg_s2f_0p95>;
> +               vddpcie1-supply = <&vreg_s8c_1p3>;
> +               vddpcie2-supply = <&vreg_s5a_1p9>;

If I remember correctly, qca6390 has at least 8 power supplies AON,
PMU, 3xRFA, 2xPCIe and VDDIO.

Moreover, these bindings do not solve another problem: the PMU is
shared between WiFi and BT parts. For the next gen (WCN6855) this
becomes even more important, see the code in msm-5.10 which makes sure
that there is a proper time between one of the units going down and
another one being powered on.

> +       };
> +};
> +
>  &pcie1 {
>         status = "okay";
>  };
> @@ -1303,6 +1319,14 @@ sdc2_card_det_n: sd-card-det-n-state {
>                 function = "gpio";
>                 bias-pull-up;
>         };
> +
> +       wlan_en_state: wlan-default-state {
> +               pins = "gpio20";
> +               function = "gpio";
> +               drive-strength = <16>;
> +               output-low;
> +               bias-pull-up;
> +       };
>  };
>
>  &uart6 {
> --
> 2.40.1
>
>


-- 
With best wishes
Dmitry

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390

[Bartosz Golaszewski]

On Thu, Jan 4, 2024 at 2:44 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 4 Jan 2024 at 15:03, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Describe the ath11k WLAN on-board the QCA6390 module. Include the
> > relevant regulators and the enable GPIO.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > index cd0db4f31d4a..721f86af952b 100644
> > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > @@ -734,6 +734,22 @@ &pcie0_phy {
> >         vdda-pll-supply = <&vreg_l9a_1p2>;
> >  };
> >
> > +&pcieport0 {
> > +       wifi@0 {
> > +               compatible = "pci17cb,1101";
> > +               reg = <0x10000 0x0 0x0 0x0 0x0>;
> > +
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&wlan_en_state>;
> > +
> > +               enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
> > +
> > +               vddpmu-supply = <&vreg_s2f_0p95>;
> > +               vddpcie1-supply = <&vreg_s8c_1p3>;
> > +               vddpcie2-supply = <&vreg_s5a_1p9>;
>
> If I remember correctly, qca6390 has at least 8 power supplies AON,
> PMU, 3xRFA, 2xPCIe and VDDIO.
>
> Moreover, these bindings do not solve another problem: the PMU is
> shared between WiFi and BT parts. For the next gen (WCN6855) this
> becomes even more important, see the code in msm-5.10 which makes sure
> that there is a proper time between one of the units going down and
> another one being powered on.
>

Unified, shared power sequencing for this type of loosely coupled MFD
devices is something I have in mind and that will be orthogonal to
this. It will come on top of the PCIe power sequencing driver. I
wanted to address a simpler issue first.

Bartosz

> > +       };
> > +};
> > +
> >  &pcie1 {
> >         status = "okay";
> >  };
> > @@ -1303,6 +1319,14 @@ sdc2_card_det_n: sd-card-det-n-state {
> >                 function = "gpio";
> >                 bias-pull-up;
> >         };
> > +
> > +       wlan_en_state: wlan-default-state {
> > +               pins = "gpio20";
> > +               function = "gpio";
> > +               drive-strength = <16>;
> > +               output-low;
> > +               bias-pull-up;
> > +       };
> >  };
> >
> >  &uart6 {
> > --
> > 2.40.1
> >
> >
>
>
> --
> With best wishes
> Dmitry

[RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to introduce PCIe power-sequencing, we need to create platform
devices for child nodes of the port driver node. They will get matched
against the pwrseq drivers (if one exists) and then the actuak PCIe
device will reuse the node once it's detected on the bus.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/pcie/portdrv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 14a4b89a3b83..401fb731009d 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -13,6 +13,7 @@
 #include <linux/pci.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/of_platform.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/string.h>
@@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		pm_runtime_allow(&dev->dev);
 	}
 
-	return 0;
+	return devm_of_platform_populate(&dev->dev);
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
-- 
2.40.1

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Jeff Johnson]

On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> In order to introduce PCIe power-sequencing, we need to create platform
> devices for child nodes of the port driver node. They will get matched
> against the pwrseq drivers (if one exists) and then the actuak PCIe

nit if you re-spin: s/actuak /actual /

> device will reuse the node once it's detected on the bus.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/pcie/portdrv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 14a4b89a3b83..401fb731009d 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -13,6 +13,7 @@
>  #include <linux/pci.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> +#include <linux/of_platform.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/string.h>
> @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		pm_runtime_allow(&dev->dev);
>  	}
>  
> -	return 0;
> +	return devm_of_platform_populate(&dev->dev);
>  }
>  
>  static void pcie_portdrv_remove(struct pci_dev *dev)

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> In order to introduce PCIe power-sequencing, we need to create platform
> devices for child nodes of the port driver node. They will get matched
> against the pwrseq drivers (if one exists) and then the actuak PCIe
> device will reuse the node once it's detected on the bus.
[...]
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		pm_runtime_allow(&dev->dev);
>  	}
>  
> -	return 0;
> +	return devm_of_platform_populate(&dev->dev);
>  }

I think this belongs in of_pci_make_dev_node(), portdrv seems totally
the wrong place.  Note that you're currently calling this for RCECs
(Root Complex Event Collectors) as well, which is likely not what
you want.

devm functions can't be used in the PCI core, so symmetrically call
of_platform_unpopulate() from of_pci_remove_node().

Thanks,

Lukas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

On Tue, Jan 9, 2024 at 3:43 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > In order to introduce PCIe power-sequencing, we need to create platform
> > devices for child nodes of the port driver node. They will get matched
> > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > device will reuse the node once it's detected on the bus.
> [...]
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >               pm_runtime_allow(&dev->dev);
> >       }
> >
> > -     return 0;
> > +     return devm_of_platform_populate(&dev->dev);
> >  }
>
> I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> the wrong place.  Note that you're currently calling this for RCECs
> (Root Complex Event Collectors) as well, which is likely not what
> you want.
>

of_pci_make_dev_node() is only called when the relevant PCI device is
instantiated which doesn't happen until it's powered-up and scanned -
precisely the problem I'm trying to address.

Calling this for whomever isn't really a problem though, is it? We
will create a platform device alright - if it's defined on the DT -
and at worst it won't match against any driver. It seems harmless IMO.

> devm functions can't be used in the PCI core, so symmetrically call
> of_platform_unpopulate() from of_pci_remove_node().

I don't doubt what you're saying is true (I've seen worse things) but
this is the probe() callback of a driver using the driver model. Why
wouldn't devres work?

Bart

>
> Thanks,
>
> Lukas
>

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > In order to introduce PCIe power-sequencing, we need to create platform
> > > devices for child nodes of the port driver node. They will get matched
> > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > device will reuse the node once it's detected on the bus.
> > [...]
> > > --- a/drivers/pci/pcie/portdrv.c
> > > +++ b/drivers/pci/pcie/portdrv.c
> > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > >               pm_runtime_allow(&dev->dev);
> > >       }
> > >
> > > -     return 0;
> > > +     return devm_of_platform_populate(&dev->dev);
> > >  }
> >
> > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > the wrong place.  Note that you're currently calling this for RCECs
> > (Root Complex Event Collectors) as well, which is likely not what
> > you want.
> >
> 
> of_pci_make_dev_node() is only called when the relevant PCI device is
> instantiated which doesn't happen until it's powered-up and scanned -
> precisely the problem I'm trying to address.

No, of_pci_make_dev_node() is called *before* device_attach(),
i.e. before portdrv has even probed.  So it seems this should
work perfectly well for your use case.


> > devm functions can't be used in the PCI core, so symmetrically call
> > of_platform_unpopulate() from of_pci_remove_node().
> 
> I don't doubt what you're saying is true (I've seen worse things) but
> this is the probe() callback of a driver using the driver model. Why
> wouldn't devres work?

The long term plan is to move the functionality in portdrv to
the PCI core.  Because devm functions can't be used in the PCI
core, adding new ones to portdrv will *add* a new roadblock to
migrating portdrv to the PCI core.  In other words, it makes
future maintenance more difficult.

Generally, only PCIe port services which share the same interrupt
(hotplug, PME, bandwith notification, flit error counter, ...)
need to live in portdrv.  Arbitrary other stuff should not be
shoehorned into portdrv.

Thanks,

Lukas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

On Wed, Jan 10, 2024 at 2:28 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > > In order to introduce PCIe power-sequencing, we need to create platform
> > > > devices for child nodes of the port driver node. They will get matched
> > > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > > device will reuse the node once it's detected on the bus.
> > > [...]
> > > > --- a/drivers/pci/pcie/portdrv.c
> > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > > >               pm_runtime_allow(&dev->dev);
> > > >       }
> > > >
> > > > -     return 0;
> > > > +     return devm_of_platform_populate(&dev->dev);
> > > >  }
> > >
> > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > > the wrong place.  Note that you're currently calling this for RCECs
> > > (Root Complex Event Collectors) as well, which is likely not what
> > > you want.
> > >
> >
> > of_pci_make_dev_node() is only called when the relevant PCI device is
> > instantiated which doesn't happen until it's powered-up and scanned -
> > precisely the problem I'm trying to address.
>
> No, of_pci_make_dev_node() is called *before* device_attach(),
> i.e. before portdrv has even probed.  So it seems this should
> work perfectly well for your use case.
>

Seems like the following must be true but isn't in my case (from
pci_bus_add_device()):

    if (pci_is_bridge(dev))
        of_pci_make_dev_node(dev);

Shouldn't it evaluate to true for ports?

Bartosz

[snip]

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> Seems like the following must be true but isn't in my case (from
> pci_bus_add_device()):
> 
>     if (pci_is_bridge(dev))
>         of_pci_make_dev_node(dev);
> 
> Shouldn't it evaluate to true for ports?

It should.

What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
> On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
>> Seems like the following must be true but isn't in my case (from
>> pci_bus_add_device()):
>>
>>     if (pci_is_bridge(dev))
>>         of_pci_make_dev_node(dev);
>>
>> Shouldn't it evaluate to true for ports?
>
> It should.
>
> What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
>

I cut out the hexdump part, let me know if you really need it. Output follows.

Bart

--

# lspci -vvvvxxxx -s  0000:00:00.0
0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
(prog-if 00 [Normal decode])
	Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR+ FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 176
	IOMMU group: 8
	Region 0: Memory at 60300000 (32-bit, non-prefetchable) [size=4K]
	Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
	I/O behind bridge: f000-0fff [disabled] [16-bit]
	Memory behind bridge: 60400000-604fffff [size=1M] [32-bit]
	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
[disabled] [64-bit]
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+
		Address: 00000000a1c3f000  Data: 0000
		Masking: fffffffe  Pending: 00000000
	Capabilities: [70] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE+
		DevCtl:	CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 8GT/s, Width x1, ASPM L0s L1, Exit Latency
L0s <1us, L1 <64us
			ClockPM- Surprise+ LLActRep+ BwNot+ ASPMOptComp+
		LnkCtl:	ASPM Disabled; RCB 128 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 5GT/s, Width x1
			TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt+
		SltCap:	AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug- Surprise+
			Slot #0, PowerLimit 0W; Interlock+ NoCompl-
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Off, PwrInd Off, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
			Changed: MRL- PresDet- LinkState-
		RootCap: CRSVisible+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible+
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ NROPrPrP+ LTR+
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS- LN System CLS Not Supported, TPHComp+ ExtTPHComp- ARIFwd-
			 AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+
10BitTagReq- OBFF Disabled, ARIFwd-
			 AtomicOpsCtl: ReqEn- EgressBlck-
		LnkCap2: Supported Link Speeds: 2.5-8GT/s, Crosslink- Retimer- 2Retimers- DRS-
		LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
			 Compliance Preset/De-emphasis: -6dB de-emphasis, 0dB preshoot
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-
EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [100 v2] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+
MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
		RootCmd: CERptEn+ NFERptEn+ FERptEn+
		RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd-
			 FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0
		ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000
	Capabilities: [148 v1] Secondary PCI Express
		LnkCtl3: LnkEquIntrruptEn- PerformEqu-
		LaneErrStat: 0
	Capabilities: [168 v1] Transaction Processing Hints
		No steering table available
	Capabilities: [1fc v1] L1 PM Substates
		L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
			  PortCommonModeRestoreTime=70us PortTPowerOnTime=0us
		L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
			   T_CommonMode=70us LTR1.2_Threshold=76800ns
		L1SubCtl2: T_PwrOn=0us
	Kernel driver in use: pcieport

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> > > Seems like the following must be true but isn't in my case (from
> > > pci_bus_add_device()):
> > >
> > >     if (pci_is_bridge(dev))
> > >         of_pci_make_dev_node(dev);
> > >
> > > Shouldn't it evaluate to true for ports?
> >
> > It should.
> >
> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
> 
> I cut out the hexdump part, let me know if you really need it.

I really need it.

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said:
> On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
>> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
>> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
>> > > Seems like the following must be true but isn't in my case (from
>> > > pci_bus_add_device()):
>> > >
>> > >     if (pci_is_bridge(dev))
>> > >         of_pci_make_dev_node(dev);
>> > >
>> > > Shouldn't it evaluate to true for ports?
>> >
>> > It should.
>> >
>> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
>>
>> I cut out the hexdump part, let me know if you really need it.
>
> I really need it.
>

Ok, one more:

# lspci -vvvvxxxx -s 0000:00:00
0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
(prog-if 00 [Normal decode])
	Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR+ FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 188
	IOMMU group: 7
	Region 0: Memory at 60300000 (32-bit, non-prefetchable) [size=4K]
	Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
	I/O behind bridge: f000-0fff [disabled] [16-bit]
	Memory behind bridge: 60400000-604fffff [size=1M] [32-bit]
	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
[disabled] [64-bit]
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+
		Address: 00000000a1c00000  Data: 0000
		Masking: fffffffe  Pending: 00000000
	Capabilities: [70] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE+
		DevCtl:	CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 8GT/s, Width x1, ASPM L0s L1, Exit Latency
L0s <1us, L1 <64us
			ClockPM- Surprise+ LLActRep+ BwNot+ ASPMOptComp+
		LnkCtl:	ASPM Disabled; RCB 128 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 5GT/s, Width x1
			TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt+
		SltCap:	AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug- Surprise+
			Slot #0, PowerLimit 0W; Interlock+ NoCompl-
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Off, PwrInd Off, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
			Changed: MRL- PresDet- LinkState-
		RootCap: CRSVisible+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible+
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ NROPrPrP+ LTR+
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS- LN System CLS Not Supported, TPHComp+ ExtTPHComp- ARIFwd-
			 AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+
10BitTagReq- OBFF Disabled, ARIFwd-
			 AtomicOpsCtl: ReqEn- EgressBlck-
		LnkCap2: Supported Link Speeds: 2.5-8GT/s, Crosslink- Retimer- 2Retimers- DRS-
		LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
			 Compliance Preset/De-emphasis: -6dB de-emphasis, 0dB preshoot
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-
EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [100 v2] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+
MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
		RootCmd: CERptEn+ NFERptEn+ FERptEn+
		RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd-
			 FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0
		ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000
	Capabilities: [148 v1] Secondary PCI Express
		LnkCtl3: LnkEquIntrruptEn- PerformEqu-
		LaneErrStat: 0
	Capabilities: [168 v1] Transaction Processing Hints
		No steering table available
	Capabilities: [1fc v1] L1 PM Substates
		L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
			  PortCommonModeRestoreTime=70us PortTPowerOnTime=0us
		L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
			   T_CommonMode=70us LTR1.2_Threshold=76800ns
		L1SubCtl2: T_PwrOn=0us
	Kernel driver in use: pcieport
00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00
10: 00 00 30 60 00 00 00 00 00 01 ff 00 f0 00 00 00
20: f0 ff 00 00 f1 ff 01 00 00 00 00 00 00 00 00 00
30: 00 00 00 00 40 00 00 00 00 00 00 00 bb 01 02 00
40: 01 50 03 c8 08 00 00 00 00 00 00 00 00 00 00 00
50: 05 70 8b 01 00 00 c0 a1 00 00 00 00 00 00 00 00
60: fe ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00
70: 10 00 42 01 00 80 00 00 1f 28 10 00 13 4c 7b 00
80: 48 00 12 f0 3f 00 02 00 c0 03 00 00 18 00 01 00
90: 00 00 00 00 1f 1c 00 00 00 04 00 00 0e 00 00 00
a0: 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
100: 01 00 82 14 00 00 00 00 00 00 40 00 30 20 46 00
110: 00 00 00 00 00 e0 00 00 a0 00 00 00 00 00 00 00
120: 00 00 00 00 00 00 00 00 00 00 00 00 07 00 00 00
130: 00 00 00 000 00 00 00 00 00 00 00 00 00 00 00
140: 00 00 00 00 00 00 00 00 19 00 81 16 00 00 00 00
150: 00 00 00 00 7f 6f 00 00 00 00 00 00 00 00 00 00
160: 00 00 00 00 00 00 00 00 17 00 c1 1f 01 00 00 00
170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1f0: 00 00 00 00 18 00 c1 1f 00 00 00 00 1e 00 01 00
200: 1f 46 00 00 00 46 4b 40 00 00 00 00 00 00 00 00
210: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
220: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
260: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
270: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5f0: 00 ��0 00 00 00 00 00 00 00 00 00 00 00 00 00
600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
700: 3b 00 b1 00 ff ff ff ff 04 00 80 00 00 90 90 1b
710: 20 01 01 00 00 00 00 00 00 40 01 00 40 01 00 00
720: 00 00 00 00 01 00 00 00 11 74 40 03 10 00 00 08
730: 60 00 01 00 07 a0 01 00 ff ff 0f 00 00 00 00 00
740: 0f 00 00 00 00 00 00 00 60 00 21 45 07 a0 21 05
750: 00 00 20 05 00 00 00 00 00 00 00 00 00 00 00 00
760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
800: 00 00 00 00 00 00 00 00 00 00 00 00 90 01 00 00
810: 00 00 00 00 00 00 00 00 00 00 00 00 7f 00 00 00
820: 00 00 c0 a1 00 00 00 00 ff ff ff ff fe ff ff ff
830: 00 00 00 00 ff ff ff ff 00 3c 02 fe 00 00 00 00
840: ff ff ff ff ff ff ff ff 00 00 00 00 ff ff ff ff
850: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff
860: 00 00 00 00 ff ff ff ff ff ff ff ff 00 00 00 00
870: ff ff ff ff ff ff ff ff 00 00 00 00 ff ff ff ff
880: ff ff ff ff 00 00 00 00 00 00 00 00 01 00 00 00
890: 01 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00
8a0: 00 00 00 00 00 00 00 00 60 00 00 04 a0 55 01 00
8b0: 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00
8c0: 00 00 00 00 44 00 00 00 00 00 00 00 01 00 00 ff
8d0: 00 9c 00 00 32 00 00 00 00 00 00 00 00 00 00 00
8e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
8f0: 00 00 00 00 00 00 00 00 2a 30 30 35 39 32 61 65
900: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00
910: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
920: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
930: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
940: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
950: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
960: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
970: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
980: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
990: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
aa0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ab0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ac0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ad0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ae0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
af0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b40: 14 00 00 00 d2 00 00 00 00 00 00 00 00 00 00 00
b50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ba0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
be0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bf0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ca0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ce0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cf0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
da0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
db0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
dc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
dd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
de0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
df0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ea0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
eb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ec0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ed0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ee0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ef0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f20: 90 01 00 00 00 00 00 00 01 14 01 00 00 00 00 00
f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00� 00
f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Bartosz

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Thu, Jan 11, 2024 at 05:09:09AM -0600, Bartosz Golaszewski wrote:
> On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said:
> > On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
> >> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
> >> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> >> > > Seems like the following must be true but isn't in my case (from
> >> > > pci_bus_add_device()):
> >> > >
> >> > >     if (pci_is_bridge(dev))
> >> > >         of_pci_make_dev_node(dev);
> >> > >
> >> > > Shouldn't it evaluate to true for ports?
> >> >
> >> > It should.
> >> >
> >> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
> 
> # lspci -vvvvxxxx -s 0000:00:00
> 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
> (prog-if 00 [Normal decode])
> 	Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
[...]
> 00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00
                                                ^^
The Header Type in config space is 0x1, i.e. PCI_HEADER_TYPE_BRIDGE.

So pci_is_bridge(dev) does return true (unlike what you write above)
and control flow enters of_pci_make_dev_node().

But perhaps of_pci_make_dev_node() returns immediately because:

	/*
	 * If there is already a device tree node linked to this device,
	 * return immediately.
	 */
	if (pci_device_to_OF_node(pdev))
		return;

...and lspci does list a devicetree node for that Root Port.

In any case, of_pci_make_dev_node() is the right place to add
the call to of_platform_populate().  Just make sure it's called
even if there is already a DT node for the Root Port itself.

Thanks,

Lukas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

On Thu, Jan 11, 2024 at 4:02 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Jan 11, 2024 at 05:09:09AM -0600, Bartosz Golaszewski wrote:
> > On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said:
> > > On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
> > >> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
> > >> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> > >> > > Seems like the following must be true but isn't in my case (from
> > >> > > pci_bus_add_device()):
> > >> > >
> > >> > >     if (pci_is_bridge(dev))
> > >> > >         of_pci_make_dev_node(dev);
> > >> > >
> > >> > > Shouldn't it evaluate to true for ports?
> > >> >
> > >> > It should.
> > >> >
> > >> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
> >
> > # lspci -vvvvxxxx -s 0000:00:00
> > 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
> > (prog-if 00 [Normal decode])
> >       Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
> [...]
> > 00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00
>                                                 ^^
> The Header Type in config space is 0x1, i.e. PCI_HEADER_TYPE_BRIDGE.
>
> So pci_is_bridge(dev) does return true (unlike what you write above)
> and control flow enters of_pci_make_dev_node().
>
> But perhaps of_pci_make_dev_node() returns immediately because:
>

No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
which requires OF_UNITTEST (!).

We definitely don't need to enable dynamic OF nodes. We don't want to
modify the DT, we want to create devices for existing nodes. Also:
with the approach in this RFC we maintain a clear hierarchy of devices
with the port device being the parent of the power sequencing device
which becomes the parent of the actual PCIe device (the port stays the
parent of this device too).

Bartosz

>         /*
>          * If there is already a device tree node linked to this device,
>          * return immediately.
>          */
>         if (pci_device_to_OF_node(pdev))
>                 return;
>
> ...and lspci does list a devicetree node for that Root Port.
>
> In any case, of_pci_make_dev_node() is the right place to add
> the call to of_platform_populate().  Just make sure it's called
> even if there is already a DT node for the Root Port itself.
>
> Thanks,
>
> Lukas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Geert Uytterhoeven]

Hi Bartosz,

On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> which requires OF_UNITTEST (!).

Huh? Config PCI_DYNAMIC_OF_NODES does select OF_DYNAMIC.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Bartosz Golaszewski]

On Thu, Jan 11, 2024 at 10:44 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> > which requires OF_UNITTEST (!).
>
> Huh? Config PCI_DYNAMIC_OF_NODES does select OF_DYNAMIC.
>

Indeed, I got something wrong.

But in any case: we *don't* need dynamic OF nodes as we don't create
new ones. We use the ones that already exist. This is logically a
wrong place to add this.

Lukas, Terry: am I getting this right - is the port driver supposed to
go away at some point? Because I'm not sure I understand what the
problem is here. To me it seems that when we create a real device for
the PCIe port, then it's only normal to populate its child devices
from the port driver.

Bartosz

> Gr{oetje,eeting}s,
>
>                         Geert
>
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Fri, Jan 12, 2024 at 10:43:04AM +0100, Bartosz Golaszewski wrote:
> Lukas, Terry: am I getting this right - is the port driver supposed to
> go away at some point?

Yes, that's the plan.

> Because I'm not sure I understand what the
> problem is here. To me it seems that when we create a real device for
> the PCIe port, then it's only normal to populate its child devices
> from the port driver.

portdrv is not creating a real device for the PCIe port.
It *binds* to that device.  The device is created much earlier.

NAK for adding this to portdrv.

Thanks,

Lukas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Thu, Jan 11, 2024 at 05:16:45PM +0100, Bartosz Golaszewski wrote:
> On Thu, Jan 11, 2024 at 4:02???PM Lukas Wunner <lukas@wunner.de> wrote:
> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> > >     if (pci_is_bridge(dev))
> > >         of_pci_make_dev_node(dev);
> > 
> > But perhaps of_pci_make_dev_node() returns immediately because:
> 
> No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> which requires OF_UNITTEST (!).
> 
> We definitely don't need to enable dynamic OF nodes. We don't want to
> modify the DT, we want to create devices for existing nodes.

Consider refactoring of_pci_make_dev_node() to suit your needs or
add a separate function call inside the "if (pci_is_bridge(dev))"
clause which populates the child OF nodes.

Thanks,

Lukas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Rob Herring]

On Fri, Jan 12, 2024 at 3:43 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Jan 11, 2024 at 05:16:45PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Jan 11, 2024 at 4:02???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> > > >     if (pci_is_bridge(dev))
> > > >         of_pci_make_dev_node(dev);
> > >
> > > But perhaps of_pci_make_dev_node() returns immediately because:
> >
> > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> > which requires OF_UNITTEST (!).
> >
> > We definitely don't need to enable dynamic OF nodes. We don't want to
> > modify the DT, we want to create devices for existing nodes.
>
> Consider refactoring of_pci_make_dev_node() to suit your needs or
> add a separate function call inside the "if (pci_is_bridge(dev))"
> clause which populates the child OF nodes.

The latter because of_pci_make_dev_node() has absolutely nothing to do
with the issue this series solves. The uses are pretty much mutually
exclusive. If we have a DT node with power related properties, there
is no need to create that node because it already exists.

Rob

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Dan Williams]

[ add Terry ]


Lukas Wunner wrote:
> On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > > In order to introduce PCIe power-sequencing, we need to create platform
> > > > devices for child nodes of the port driver node. They will get matched
> > > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > > device will reuse the node once it's detected on the bus.
> > > [...]
> > > > --- a/drivers/pci/pcie/portdrv.c
> > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > > >               pm_runtime_allow(&dev->dev);
> > > >       }
> > > >
> > > > -     return 0;
> > > > +     return devm_of_platform_populate(&dev->dev);
> > > >  }
> > >
> > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > > the wrong place.  Note that you're currently calling this for RCECs
> > > (Root Complex Event Collectors) as well, which is likely not what
> > > you want.
> > >
> > 
> > of_pci_make_dev_node() is only called when the relevant PCI device is
> > instantiated which doesn't happen until it's powered-up and scanned -
> > precisely the problem I'm trying to address.
> 
> No, of_pci_make_dev_node() is called *before* device_attach(),
> i.e. before portdrv has even probed.  So it seems this should
> work perfectly well for your use case.
> 
> 
> > > devm functions can't be used in the PCI core, so symmetrically call
> > > of_platform_unpopulate() from of_pci_remove_node().
> > 
> > I don't doubt what you're saying is true (I've seen worse things) but
> > this is the probe() callback of a driver using the driver model. Why
> > wouldn't devres work?
> 
> The long term plan is to move the functionality in portdrv to
> the PCI core.  Because devm functions can't be used in the PCI
> core, adding new ones to portdrv will *add* a new roadblock to
> migrating portdrv to the PCI core.  In other words, it makes
> future maintenance more difficult.
> 
> Generally, only PCIe port services which share the same interrupt
> (hotplug, PME, bandwith notification, flit error counter, ...)
> need to live in portdrv.  Arbitrary other stuff should not be
> shoehorned into portdrv.

I came here to say the same thing. It is already the case that portdrv
is not a good model to build new functionality upon [1], and PCI core
enlightenment should be considered first.

The portdrv model is impeding Terry's CXL Port error handling effort, so
I am on the lookout for portdrv growing new entanglements to unwind
later.

[1]: http://lore.kernel.org/r/20221025232535.GA579167@bhelgaas

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Manivannan Sadhasivam]

On Wed, Jan 10, 2024 at 12:41:17PM -0800, Dan Williams wrote:
> [ add Terry ]
> 
> 
> Lukas Wunner wrote:
> > On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > > > In order to introduce PCIe power-sequencing, we need to create platform
> > > > > devices for child nodes of the port driver node. They will get matched
> > > > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > > > device will reuse the node once it's detected on the bus.
> > > > [...]
> > > > > --- a/drivers/pci/pcie/portdrv.c
> > > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > > > >               pm_runtime_allow(&dev->dev);
> > > > >       }
> > > > >
> > > > > -     return 0;
> > > > > +     return devm_of_platform_populate(&dev->dev);
> > > > >  }
> > > >
> > > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > > > the wrong place.  Note that you're currently calling this for RCECs
> > > > (Root Complex Event Collectors) as well, which is likely not what
> > > > you want.
> > > >
> > > 
> > > of_pci_make_dev_node() is only called when the relevant PCI device is
> > > instantiated which doesn't happen until it's powered-up and scanned -
> > > precisely the problem I'm trying to address.
> > 
> > No, of_pci_make_dev_node() is called *before* device_attach(),
> > i.e. before portdrv has even probed.  So it seems this should
> > work perfectly well for your use case.
> > 
> > 
> > > > devm functions can't be used in the PCI core, so symmetrically call
> > > > of_platform_unpopulate() from of_pci_remove_node().
> > > 
> > > I don't doubt what you're saying is true (I've seen worse things) but
> > > this is the probe() callback of a driver using the driver model. Why
> > > wouldn't devres work?
> > 
> > The long term plan is to move the functionality in portdrv to
> > the PCI core.  Because devm functions can't be used in the PCI
> > core, adding new ones to portdrv will *add* a new roadblock to
> > migrating portdrv to the PCI core.  In other words, it makes
> > future maintenance more difficult.
> > 
> > Generally, only PCIe port services which share the same interrupt
> > (hotplug, PME, bandwith notification, flit error counter, ...)
> > need to live in portdrv.  Arbitrary other stuff should not be
> > shoehorned into portdrv.
> 
> I came here to say the same thing. It is already the case that portdrv
> is not a good model to build new functionality upon [1], and PCI core
> enlightenment should be considered first.
> 

The primary reason for plugging the power sequencing into portdrv is due to
portdrv binding with all the bridge devices and acting as management driver for
the bridges. This is where exactly the power sequencing part needs to be plugged
in IMO. But if the idea of the portdrv is just to expose services based on
interrupts, then please suggest a better place to plug this power sequencing
part.

- Mani

> The portdrv model is impeding Terry's CXL Port error handling effort, so
> I am on the lookout for portdrv growing new entanglements to unwind
> later.
> 
> [1]: http://lore.kernel.org/r/20221025232535.GA579167@bhelgaas
> 

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

Re: [RFC 3/9] PCI/portdrv: create platform devices for child OF nodes

[Lukas Wunner]

On Thu, Jan 11, 2024 at 06:10:09PM +0530, Manivannan Sadhasivam wrote:
> The primary reason for plugging the power sequencing into portdrv is due to
> portdrv binding with all the bridge devices and acting as management driver
> for the bridges.

As I've said before, portdrv not only binds to bridges but also
Root Complex Event Collectors.  And you most likely don't want to
populate child DT nodes for those.

> This is where exactly the power sequencing part needs to be plugged
> in IMO. But if the idea of the portdrv is just to expose services based on
> interrupts, then please suggest a better place to plug this power sequencing
> part.

Again, I'm suggesting to put this into of_pci_make_dev_node().

Thanks,

Lukas

[RFC 4/9] PCI: hold the rescan mutex when scanning for the first time

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With the introduction of the power sequencing drivers that will be able
to trigger the port rescan, we need to hold the rescan mutex during the
initial pci_host_probe() too or the two could get in each other's way.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b7335be56008..957f7afee7ba 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3122,7 +3122,9 @@ int pci_host_probe(struct pci_host_bridge *bridge)
 	struct pci_bus *bus, *child;
 	int ret;
 
+	pci_lock_rescan_remove();
 	ret = pci_scan_root_bus_bridge(bridge);
+	pci_unlock_rescan_remove();
 	if (ret < 0) {
 		dev_err(bridge->dev.parent, "Scanning root bridge failed");
 		return ret;
-- 
2.40.1

[RFC 5/9] PCI/pwrseq: add pwrseq core code

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Some PCIe devices must be powered-on before they can be detected on the
bus. Introduce a simple framework built around the PCIe port driver
which now instantiates its child devices from device-tree.

The way this works is: a DT node representing a PCIe device connected to
the port can be matched against its power sequencing platform driver. If
the match succeeds, the driver is responsible for powering-up the device
and calling pcie_pwrseq_device_enable() which will trigger a PCI bus
rescan as well as subscribe to PCI bus notifications.

When the device is detected and created, we'll make it consume the same
DT node that the platform device did. When the device is bound, we'll
create a device link between it and the parent power sequencing device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/pcie/Kconfig         |  2 +
 drivers/pci/pcie/Makefile        |  2 +
 drivers/pci/pcie/pwrseq/Kconfig  |  8 +++
 drivers/pci/pcie/pwrseq/Makefile |  3 ++
 drivers/pci/pcie/pwrseq/pwrseq.c | 83 ++++++++++++++++++++++++++++++++
 include/linux/pcie-pwrseq.h      | 24 +++++++++
 6 files changed, 122 insertions(+)
 create mode 100644 drivers/pci/pcie/pwrseq/Kconfig
 create mode 100644 drivers/pci/pcie/pwrseq/Makefile
 create mode 100644 drivers/pci/pcie/pwrseq/pwrseq.c
 create mode 100644 include/linux/pcie-pwrseq.h

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 8999fcebde6a..064843ab6866 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -155,3 +155,5 @@ config PCIE_EDR
 	  the PCI Firmware Specification r3.2.  Enable this if you want to
 	  support hybrid DPC model which uses both firmware and OS to
 	  implement DPC.
+
+source "drivers/pci/pcie/pwrseq/Kconfig"
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 8de4ed5f98f1..ace91384113d 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
 obj-$(CONFIG_PCIE_PTM)		+= ptm.o
 obj-$(CONFIG_PCIE_EDR)		+= edr.o
+
+obj-$(CONFIG_PCIEPORTBUS)	+= pwrseq/
diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
new file mode 100644
index 000000000000..010e31f432c9
--- /dev/null
+++ b/drivers/pci/pcie/pwrseq/Kconfig
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menuconfig PCIE_PWRSEQ
+	bool "PCIe Power Sequencing support"
+	depends on PCIEPORTBUS
+	help
+	  Say yes here to enable support for PCIe power sequencing
+	  drivers.
diff --git a/drivers/pci/pcie/pwrseq/Makefile b/drivers/pci/pcie/pwrseq/Makefile
new file mode 100644
index 000000000000..da99566594f6
--- /dev/null
+++ b/drivers/pci/pcie/pwrseq/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_PCIE_PWRSEQ)		+= pwrseq.o
diff --git a/drivers/pci/pcie/pwrseq/pwrseq.c b/drivers/pci/pcie/pwrseq/pwrseq.c
new file mode 100644
index 000000000000..c4f7958074c9
--- /dev/null
+++ b/drivers/pci/pcie/pwrseq/pwrseq.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pcie-pwrseq.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+static int pcie_pwrseq_notify(struct notifier_block *nb, unsigned long action,
+			      void *data)
+{
+	struct pcie_pwrseq *pwrseq = container_of(nb, struct pcie_pwrseq, nb);
+	struct device *dev = data;
+
+	if (dev_fwnode(dev) != dev_fwnode(pwrseq->dev))
+		return NOTIFY_DONE;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		device_set_of_node_from_dev(dev, pwrseq->dev);
+		break;
+	case BUS_NOTIFY_BOUND_DRIVER:
+		pwrseq->link = device_link_add(dev, pwrseq->dev,
+					       DL_FLAG_AUTOREMOVE_CONSUMER);
+		if (!pwrseq->link)
+			dev_err(pwrseq->dev, "Failed to add device link\n");
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		device_link_del(pwrseq->link);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+int pcie_pwrseq_device_enable(struct pcie_pwrseq *pwrseq)
+{
+	if (!pwrseq->dev)
+		return -ENODEV;
+
+	pwrseq->nb.notifier_call = pcie_pwrseq_notify;
+	bus_register_notifier(&pci_bus_type, &pwrseq->nb);
+
+	pci_lock_rescan_remove();
+	pci_rescan_bus(to_pci_dev(pwrseq->dev->parent)->bus);
+	pci_unlock_rescan_remove();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pcie_pwrseq_device_enable);
+
+void pcie_pwrseq_device_disable(struct pcie_pwrseq *pwrseq)
+{
+	bus_unregister_notifier(&pci_bus_type, &pwrseq->nb);
+}
+EXPORT_SYMBOL_GPL(pcie_pwrseq_device_disable);
+
+static void devm_pcie_pwrseq_device_disable(void *data)
+{
+	struct pcie_pwrseq *pwrseq = data;
+
+	pcie_pwrseq_device_disable(pwrseq);
+}
+
+int devm_pcie_pwrseq_device_enable(struct device *dev,
+				   struct pcie_pwrseq *pwrseq)
+{
+	int ret;
+
+	ret = pcie_pwrseq_device_enable(pwrseq);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_pcie_pwrseq_device_disable,
+					pwrseq);
+}
+EXPORT_SYMBOL_GPL(devm_pcie_pwrseq_device_enable);
diff --git a/include/linux/pcie-pwrseq.h b/include/linux/pcie-pwrseq.h
new file mode 100644
index 000000000000..6abb5796754e
--- /dev/null
+++ b/include/linux/pcie-pwrseq.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#ifndef __PCIE_PWRSEQ_H__
+#define __PCIE_PWRSEQ_H__
+
+#include <linux/notifier.h>
+
+struct device;
+
+struct pcie_pwrseq {
+	struct notifier_block nb;
+	struct device *dev;
+	struct device_link *link;
+};
+
+int pcie_pwrseq_device_enable(struct pcie_pwrseq *pwrseq);
+void pcie_pwrseq_device_disable(struct pcie_pwrseq *pwrseq);
+int devm_pcie_pwrseq_device_enable(struct device *dev,
+				   struct pcie_pwrseq *pwrseq);
+
+#endif /* __PCIE_PWRSEQ_H__ */
-- 
2.40.1

Re: [RFC 5/9] PCI/pwrseq: add pwrseq core code

[Jeff Johnson]

On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
...
> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
> new file mode 100644
> index 000000000000..010e31f432c9
> --- /dev/null
> +++ b/drivers/pci/pcie/pwrseq/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +menuconfig PCIE_PWRSEQ
> +	bool "PCIe Power Sequencing support"
> +	depends on PCIEPORTBUS
> +	help
> +	  Say yes here to enable support for PCIe power sequencing
> +	  drivers.

Do we really need this configuration? I'm concerned about how do we let
the end user know when this needs to be enabled. Speaking specifically
about ath11k, almost all ath11k devices will not require this, but it
then becomes a support issue that if you are using QCA6390 then you need
to enable it.

I see that in [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390
there is an option to enable config PCIE_PWRSEQ_QCA6390 if PCIE_PWRSEQ
is enabled, but you then have the menuconfig dilemma that you won't know
that you need to enable PCIE_PWRSEQ if you are using QCA6390.

Let me speak more to that in my review of that patch.

/jeff

[RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Document the PCI vendor prefix for Qualcomm Atheros so that we can
define the QCA PCI devices on device tree.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 2dc098b39234..297d6037cd12 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1128,6 +1128,7 @@ patternProperties:
   "^purism,.*":
     description: Purism, SPC
   "^qca,.*":
+  "^pci17cb,.*":
     description: Qualcomm Atheros, Inc.
   "^qcom,.*":
     description: Qualcomm Technologies, Inc
-- 
2.40.1

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Rob Herring]

On Thu, 04 Jan 2024 14:01:20 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Document the PCI vendor prefix for Qualcomm Atheros so that we can
> define the QCA PCI devices on device tree.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/vendor-prefixes.yaml:1116:13: [error] empty value in block mapping (empty-values)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/vendor-prefixes.yaml: patternProperties:^qca,.*: None is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/vendor-prefixes.yaml: ignoring, error in schema: patternProperties: ^qca,.*

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240104130123.37115-7-brgl@bgdev.pl

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

Hi,

On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Document the PCI vendor prefix for Qualcomm Atheros so that we can
> define the QCA PCI devices on device tree.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 2dc098b39234..297d6037cd12 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1128,6 +1128,7 @@ patternProperties:
>    "^purism,.*":
>      description: Purism, SPC
>    "^qca,.*":
> +  "^pci17cb,.*":

I don't think it's a good idea to list all the PCI vendor IDs
in vendor-prefixes.yaml. To please the tooling, I suggest to
have a generic entry instead. Something like this (untested):

"^pci[0-9a-f][0-9a-f][0-9a-f][0-9a-f],.*":
  description: PCI SIG Vendor ID

Note, that we we already have a bunch of them:

grep -ho 'pci[0-9a-f][0-9a-f][0-9a-f][0-9a-f],' **/*.dts* | sort | uniq -c
     70 pci0014,
      3 pci10b5,
      1 pci10ee,
      6 pci14e4,
      1 pci16c3,
      2 pci17a0,
      1 pci17cb,
      1 pci1b4b,
     63 pci8086,

Greetings,

-- Sebastian

>      description: Qualcomm Atheros, Inc.
>    "^qcom,.*":
>      description: Qualcomm Technologies, Inc
> -- 
> 2.40.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Rob Herring]

On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Document the PCI vendor prefix for Qualcomm Atheros so that we can
> define the QCA PCI devices on device tree.

Why? vendor-prefixes.yaml is only applied to property names. 'qca' 
should be the prefix for those.

Rob

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Bartosz Golaszewski]

On Mon, Jan 8, 2024 at 8:10 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Document the PCI vendor prefix for Qualcomm Atheros so that we can
> > define the QCA PCI devices on device tree.
>
> Why? vendor-prefixes.yaml is only applied to property names. 'qca'
> should be the prefix for those.
>
> Rob

I didn't have any better idea. PCI devices on DT are defined by their
"pci<vendor ID>,<model ID>" compatible, not regular human-readable
strings and this makes checkpatch.pl complain.

I'm open to suggestions.

Bartosz

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Rob Herring]

On Mon, Jan 8, 2024 at 12:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Mon, Jan 8, 2024 at 8:10 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Document the PCI vendor prefix for Qualcomm Atheros so that we can
> > > define the QCA PCI devices on device tree.
> >
> > Why? vendor-prefixes.yaml is only applied to property names. 'qca'
> > should be the prefix for those.
> >
> > Rob
>
> I didn't have any better idea. PCI devices on DT are defined by their
> "pci<vendor ID>,<model ID>" compatible, not regular human-readable
> strings and this makes checkpatch.pl complain.
>
> I'm open to suggestions.

The checkpatch.pl check predates schemas and we could consider just
dropping it. The only thing it provides is checking a patch rather
than the tree (which the schema do). It's pretty hacky because it just
greps the tree for a compatible string which is not entirely accurate.
Also, we can extract an exact list of compatibles with
"dt-extract-compatibles" which would make a better check, but I'm not
sure making dtschema a dependency on checkpatch would be good.

The other option is just ignore the warning. PCI compatibles are fairly rare.

Rob

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Krzysztof Kozlowski]

On 09/01/2024 03:56, Rob Herring wrote:
> On Mon, Jan 8, 2024 at 12:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>> On Mon, Jan 8, 2024 at 8:10 PM Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> Document the PCI vendor prefix for Qualcomm Atheros so that we can
>>>> define the QCA PCI devices on device tree.
>>>
>>> Why? vendor-prefixes.yaml is only applied to property names. 'qca'
>>> should be the prefix for those.
>>>
>>> Rob
>>
>> I didn't have any better idea. PCI devices on DT are defined by their
>> "pci<vendor ID>,<model ID>" compatible, not regular human-readable
>> strings and this makes checkpatch.pl complain.
>>
>> I'm open to suggestions.
> 
> The checkpatch.pl check predates schemas and we could consider just
> dropping it. The only thing it provides is checking a patch rather
> than the tree (which the schema do). It's pretty hacky because it just
> greps the tree for a compatible string which is not entirely accurate.
> Also, we can extract an exact list of compatibles with
> "dt-extract-compatibles" which would make a better check, but I'm not
> sure making dtschema a dependency on checkpatch would be good.
> 
> The other option is just ignore the warning. PCI compatibles are fairly rare.

Yep, the same warnings are for EEPROM and USB VID/PID compatibles, and
we live with these, so I don't think PCI should be treated differently.

Best regards,
Krzysztof

Re: [RFC 6/9] dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros

[Bartosz Golaszewski]

On Tue, Jan 9, 2024 at 10:17 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/01/2024 03:56, Rob Herring wrote:
> > On Mon, Jan 8, 2024 at 12:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>
> >> On Mon, Jan 8, 2024 at 8:10 PM Rob Herring <robh@kernel.org> wrote:
> >>>
> >>> On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>
> >>>> Document the PCI vendor prefix for Qualcomm Atheros so that we can
> >>>> define the QCA PCI devices on device tree.
> >>>
> >>> Why? vendor-prefixes.yaml is only applied to property names. 'qca'
> >>> should be the prefix for those.
> >>>
> >>> Rob
> >>
> >> I didn't have any better idea. PCI devices on DT are defined by their
> >> "pci<vendor ID>,<model ID>" compatible, not regular human-readable
> >> strings and this makes checkpatch.pl complain.
> >>
> >> I'm open to suggestions.
> >
> > The checkpatch.pl check predates schemas and we could consider just
> > dropping it. The only thing it provides is checking a patch rather
> > than the tree (which the schema do). It's pretty hacky because it just
> > greps the tree for a compatible string which is not entirely accurate.
> > Also, we can extract an exact list of compatibles with
> > "dt-extract-compatibles" which would make a better check, but I'm not
> > sure making dtschema a dependency on checkpatch would be good.
> >
> > The other option is just ignore the warning. PCI compatibles are fairly rare.
>
> Yep, the same warnings are for EEPROM and USB VID/PID compatibles, and
> we live with these, so I don't think PCI should be treated differently.
>

Got it, I will drop this patch.

Bart

[RFC 7/9] dt-bindings: wireless: ath11k: describe QCA6390

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Describe the ath11k variant present on the QCA6390 module.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../bindings/net/wireless/qcom,ath11k-pci.yaml     | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
index 817f02a8b481..f584c25f4276 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
@@ -16,6 +16,7 @@ description: |
 properties:
   compatible:
     enum:
+      - pci17cb,1101  # QCA6390
       - pci17cb,1103  # WCN6855
 
   reg:
@@ -27,6 +28,19 @@ properties:
       string to uniquely identify variant of the calibration data for designs
       with colliding bus and device ids
 
+  enable-gpios:
+    description: GPIO line enabling the ATH11K module when asserted.
+    maxItems: 1
+
+  vddpmu-supply:
+    description: VDD_PMU supply regulator handle
+
+  vddpcie1-supply:
+    description: VDD_PCIE1 supply regulator handle
+
+  vddpcie2-supply:
+    description: VDD_PCIE2 supply regulator handle
+
 required:
   - compatible
   - reg
-- 
2.40.1

Re: [RFC 7/9] dt-bindings: wireless: ath11k: describe QCA6390

[Krzysztof Kozlowski]

On 04/01/2024 14:01, Bartosz Golaszewski wrote:
> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
> index 817f02a8b481..f584c25f4276 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
> @@ -16,6 +16,7 @@ description: |
>  properties:
>    compatible:
>      enum:
> +      - pci17cb,1101  # QCA6390
>        - pci17cb,1103  # WCN6855
>  
>    reg:
> @@ -27,6 +28,19 @@ properties:
>        string to uniquely identify variant of the calibration data for designs
>        with colliding bus and device ids
>  
> +  enable-gpios:
> +    description: GPIO line enabling the ATH11K module when asserted.
> +    maxItems: 1
> +
> +  vddpmu-supply:
> +    description: VDD_PMU supply regulator handle
> +
> +  vddpcie1-supply:
> +    description: VDD_PCIE1 supply regulator handle
> +
> +  vddpcie2-supply:
> +    description: VDD_PCIE2 supply regulator handle

Looks like these are valid only for specific variant, so you should have
allOf:if:then disallowing other properties when not applicable. The same
applies for existing properties which might not exist on qca6390.

Best regards,
Krzysztof

Re: [RFC 7/9] dt-bindings: wireless: ath11k: describe QCA6390

[Kalle Valo]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 04/01/2024 14:01, Bartosz Golaszewski wrote:
>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
>> index 817f02a8b481..f584c25f4276 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
>> @@ -16,6 +16,7 @@ description: |
>>  properties:
>>    compatible:
>>      enum:
>> +      - pci17cb,1101  # QCA6390
>>        - pci17cb,1103  # WCN6855
>>  
>>    reg:
>> @@ -27,6 +28,19 @@ properties:
>>        string to uniquely identify variant of the calibration data for designs
>>        with colliding bus and device ids
>>  
>> +  enable-gpios:
>> +    description: GPIO line enabling the ATH11K module when asserted.
>> +    maxItems: 1
>> +
>> +  vddpmu-supply:
>> +    description: VDD_PMU supply regulator handle
>> +
>> +  vddpcie1-supply:
>> +    description: VDD_PCIE1 supply regulator handle
>> +
>> +  vddpcie2-supply:
>> +    description: VDD_PCIE2 supply regulator handle
>
> Looks like these are valid only for specific variant, so you should have
> allOf:if:then disallowing other properties when not applicable. The same
> applies for existing properties which might not exist on qca6390.

And it would be good to provide more information in the commit log about
the platforms where this is used, otherwise the context might not be
clear. Or maybe document that in the bindings file, not sure.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a PCIe power sequencing driver that's capable of correctly powering
up the ath11k module on QCA6390 using the PCIe pwrseq functionality.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/pcie/pwrseq/Kconfig               |  11 +
 drivers/pci/pcie/pwrseq/Makefile              |   1 +
 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c

diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
index 010e31f432c9..f9fe555b8506 100644
--- a/drivers/pci/pcie/pwrseq/Kconfig
+++ b/drivers/pci/pcie/pwrseq/Kconfig
@@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
 	help
 	  Say yes here to enable support for PCIe power sequencing
 	  drivers.
+
+if PCIE_PWRSEQ
+
+config PCIE_PWRSEQ_QCA6390
+	tristate "PCIe Power Sequencing driver for QCA6390"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  Enable support for the PCIe power sequencing driver for the
+	  ath11k module of the QCA6390 WLAN/BT chip.
+
+endif
diff --git a/drivers/pci/pcie/pwrseq/Makefile b/drivers/pci/pcie/pwrseq/Makefile
index da99566594f6..da3e02063404 100644
--- a/drivers/pci/pcie/pwrseq/Makefile
+++ b/drivers/pci/pcie/pwrseq/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_PCIE_PWRSEQ)		+= pwrseq.o
+obj-$(CONFIG_PCIE_PWRSEQ_QCA6390)	+= pcie-pwrseq-qca6390.o
diff --git a/drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c b/drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
new file mode 100644
index 000000000000..e9fddbb642fe
--- /dev/null
+++ b/drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Linaro Ltd.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pcie-pwrseq.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct pcie_pwrseq_qca6390_vreg {
+	const char *name;
+	unsigned int load_uA;
+};
+
+struct pcie_pwrseq_qca6390_pdata {
+	struct pcie_pwrseq_qca6390_vreg *vregs;
+	size_t num_vregs;
+	unsigned int delay_msec;
+};
+
+struct pcie_pwrseq_qca6390_ctx {
+	struct pcie_pwrseq pwrseq;
+	const struct pcie_pwrseq_qca6390_pdata *pdata;
+	struct regulator_bulk_data *regs;
+	struct gpio_descs *en_gpios;
+	unsigned long *en_gpios_values;
+};
+
+static struct pcie_pwrseq_qca6390_vreg pcie_pwrseq_qca6390_vregs[] = {
+	{
+		.name = "vddpmu",
+		.load_uA = 1250000,
+	},
+	{
+		.name = "vddpcie1",
+		.load_uA = 35000,
+	},
+	{
+		.name = "vddpcie2",
+		.load_uA = 15000,
+	},
+};
+
+static struct pcie_pwrseq_qca6390_pdata pcie_pwrseq_qca6390_of_data = {
+	.vregs = pcie_pwrseq_qca6390_vregs,
+	.num_vregs = ARRAY_SIZE(pcie_pwrseq_qca6390_vregs),
+	.delay_msec = 16,
+};
+
+static int pcie_pwrseq_qca6390_power_on(struct pcie_pwrseq_qca6390_ctx *ctx)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(ctx->pdata->num_vregs, ctx->regs);
+	if (ret)
+		return ret;
+
+	bitmap_fill(ctx->en_gpios_values, ctx->en_gpios->ndescs);
+
+	ret = gpiod_set_array_value_cansleep(ctx->en_gpios->ndescs,
+					     ctx->en_gpios->desc,
+					     ctx->en_gpios->info,
+					     ctx->en_gpios_values);
+	if (ret) {
+		regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+		return ret;
+	}
+
+	if (ctx->pdata->delay_msec)
+		msleep(ctx->pdata->delay_msec);
+
+	return 0;
+}
+
+static int pcie_pwrseq_qca6390_power_off(struct pcie_pwrseq_qca6390_ctx *ctx)
+{
+	int ret;
+
+	bitmap_zero(ctx->en_gpios_values, ctx->en_gpios->ndescs);
+
+	ret = gpiod_set_array_value_cansleep(ctx->en_gpios->ndescs,
+					     ctx->en_gpios->desc,
+					     ctx->en_gpios->info,
+					     ctx->en_gpios_values);
+	if (ret)
+		return ret;
+
+	return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+}
+
+static void devm_pcie_pwrseq_qca6390_power_off(void *data)
+{
+	struct pcie_pwrseq_qca6390_ctx *ctx = data;
+
+	pcie_pwrseq_qca6390_power_off(ctx);
+}
+
+static int pcie_pwrseq_qca6309_probe(struct platform_device *pdev)
+{
+	struct pcie_pwrseq_qca6390_ctx *ctx;
+	struct device *dev = &pdev->dev;
+	int ret, i;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->pdata = of_device_get_match_data(dev);
+	if (!ctx->pdata)
+		return dev_err_probe(dev, -ENODEV,
+				     "Failed to obtain platform data\n");
+
+	if (ctx->pdata->vregs) {
+		ctx->regs = devm_kcalloc(dev, ctx->pdata->num_vregs,
+					 sizeof(*ctx->regs), GFP_KERNEL);
+		if (!ctx->regs)
+			return -ENOMEM;
+
+		for (i = 0; i < ctx->pdata->num_vregs; i++)
+			ctx->regs[i].supply = ctx->pdata->vregs[i].name;
+
+		ret = devm_regulator_bulk_get(dev, ctx->pdata->num_vregs,
+					      ctx->regs);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to get all regulators\n");
+
+		for (i = 0; i < ctx->pdata->num_vregs; i++) {
+			ret = regulator_set_load(ctx->regs[i].consumer,
+						 ctx->pdata->vregs[i].load_uA);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "Failed to set vreg load\n");
+		}
+	}
+
+	ctx->en_gpios = devm_gpiod_get_array_optional(dev, "enable",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->en_gpios))
+		return dev_err_probe(dev, PTR_ERR(ctx->en_gpios),
+				     "Failed to get enable GPIOs\n");
+
+	ctx->en_gpios_values = devm_bitmap_zalloc(dev, ctx->en_gpios->ndescs,
+						  GFP_KERNEL);
+	if (!ctx->en_gpios_values)
+		return -ENOMEM;
+
+	ret = pcie_pwrseq_qca6390_power_on(ctx);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to power on the device\n");
+
+	ret = devm_add_action_or_reset(dev, devm_pcie_pwrseq_qca6390_power_off,
+				       ctx);
+	if (ret)
+		return ret;
+
+	ctx->pwrseq.dev = dev;
+
+	ret = devm_pcie_pwrseq_device_enable(dev, &ctx->pwrseq);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register the pwrseq wrapper\n");
+
+	return 0;
+}
+
+static const struct of_device_id pcie_pwrseq_qca6309_of_match[] = {
+	{
+		.compatible = "pci17cb,1101",
+		.data = &pcie_pwrseq_qca6390_of_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pcie_pwrseq_qca6309_of_match);
+
+static struct platform_driver pcie_pwrseq_qca6309_driver = {
+	.driver = {
+		.name = "pcie-pwrseq-qca6390",
+		.of_match_table = pcie_pwrseq_qca6309_of_match,
+	},
+	.probe = pcie_pwrseq_qca6309_probe,
+};
+module_platform_driver(pcie_pwrseq_qca6309_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>");
+MODULE_DESCRIPTION("PCIe Power Sequencing module for QCA6390");
+MODULE_LICENSE("GPL");
-- 
2.40.1

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Jeff Johnson]

On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
> index 010e31f432c9..f9fe555b8506 100644
> --- a/drivers/pci/pcie/pwrseq/Kconfig
> +++ b/drivers/pci/pcie/pwrseq/Kconfig
> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>  	help
>  	  Say yes here to enable support for PCIe power sequencing
>  	  drivers.
> +
> +if PCIE_PWRSEQ
> +
> +config PCIE_PWRSEQ_QCA6390
> +	tristate "PCIe Power Sequencing driver for QCA6390"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  Enable support for the PCIe power sequencing driver for the
> +	  ath11k module of the QCA6390 WLAN/BT chip.
> +
> +endif

As I mentioned in the 5/9 patch I'm concerned that the current
definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
the fact that QCA6390 may need additional configuration since the menu
item will only show up if you have already enabled PCIE_PWRSEQ.
Yes I see that these are set in the defconfig in 9/9 but I'm concerned
about the more generic case.

I'm wondering if there should be a separate config QCA6390 within ath11k
which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390

/jeff

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
>> index 010e31f432c9..f9fe555b8506 100644
>> --- a/drivers/pci/pcie/pwrseq/Kconfig
>> +++ b/drivers/pci/pcie/pwrseq/Kconfig
>> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>>  	help
>>  	  Say yes here to enable support for PCIe power sequencing
>>  	  drivers.
>> +
>> +if PCIE_PWRSEQ
>> +
>> +config PCIE_PWRSEQ_QCA6390
>> +	tristate "PCIe Power Sequencing driver for QCA6390"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	help
>> +	  Enable support for the PCIe power sequencing driver for the
>> +	  ath11k module of the QCA6390 WLAN/BT chip.
>> +
>> +endif
>
> As I mentioned in the 5/9 patch I'm concerned that the current
> definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
> the fact that QCA6390 may need additional configuration since the menu
> item will only show up if you have already enabled PCIE_PWRSEQ.
> Yes I see that these are set in the defconfig in 9/9 but I'm concerned
> about the more generic case.
>
> I'm wondering if there should be a separate config QCA6390 within ath11k
> which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390

Or is it possible to provide an optional dependency in Kconfig (I guess
not)? Or what about mentioning about PCIE_PWRSEQ_QCA6390 in ATH11K_PCI
help text?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Chen-Yu Tsai]

On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <kvalo@kernel.org> wrote:
>
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
> >> index 010e31f432c9..f9fe555b8506 100644
> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
> >>      help
> >>        Say yes here to enable support for PCIe power sequencing
> >>        drivers.
> >> +
> >> +if PCIE_PWRSEQ
> >> +
> >> +config PCIE_PWRSEQ_QCA6390
> >> +    tristate "PCIe Power Sequencing driver for QCA6390"
> >> +    depends on ARCH_QCOM || COMPILE_TEST
> >> +    help
> >> +      Enable support for the PCIe power sequencing driver for the
> >> +      ath11k module of the QCA6390 WLAN/BT chip.
> >> +
> >> +endif
> >
> > As I mentioned in the 5/9 patch I'm concerned that the current
> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
> > the fact that QCA6390 may need additional configuration since the menu
> > item will only show up if you have already enabled PCIE_PWRSEQ.
> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
> > about the more generic case.
> >
> > I'm wondering if there should be a separate config QCA6390 within ath11k
> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
>
> Or is it possible to provide an optional dependency in Kconfig (I guess

imply PCIE_PWRSEQ
imply PCIE_PWRSEQ_QCA6390
?

> not)? Or what about mentioning about PCIE_PWRSEQ_QCA6390 in ATH11K_PCI
> help text?

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Kalle Valo]

Chen-Yu Tsai <wenst@chromium.org> writes:

> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <kvalo@kernel.org> wrote:
>
>>
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>
>> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
>> >> index 010e31f432c9..f9fe555b8506 100644
>> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
>> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
>> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>> >>      help
>> >>        Say yes here to enable support for PCIe power sequencing
>> >>        drivers.
>> >> +
>> >> +if PCIE_PWRSEQ
>> >> +
>> >> +config PCIE_PWRSEQ_QCA6390
>> >> +    tristate "PCIe Power Sequencing driver for QCA6390"
>> >> +    depends on ARCH_QCOM || COMPILE_TEST
>> >> +    help
>> >> +      Enable support for the PCIe power sequencing driver for the
>> >> +      ath11k module of the QCA6390 WLAN/BT chip.
>> >> +
>> >> +endif
>> >
>> > As I mentioned in the 5/9 patch I'm concerned that the current
>> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
>> > the fact that QCA6390 may need additional configuration since the menu
>> > item will only show up if you have already enabled PCIE_PWRSEQ.
>> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
>> > about the more generic case.
>> >
>> > I'm wondering if there should be a separate config QCA6390 within ath11k
>> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
>>
>> Or is it possible to provide an optional dependency in Kconfig (I guess
>
> imply PCIE_PWRSEQ
> imply PCIE_PWRSEQ_QCA6390
> ?

Nice, I had forgotten imply altogether. Would 'imply
PCIE_PWRSEQ_QCA6390' in ath11k Kconfig be enough to address Jeff's
concern?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Arnd Bergmann]

On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
> Chen-Yu Tsai <wenst@chromium.org> writes:
>> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <kvalo@kernel.org> wrote:
>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>>
>>> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>>> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
>>> >> index 010e31f432c9..f9fe555b8506 100644
>>> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
>>> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
>>> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>>> >>      help
>>> >>        Say yes here to enable support for PCIe power sequencing
>>> >>        drivers.
>>> >> +
>>> >> +if PCIE_PWRSEQ
>>> >> +
>>> >> +config PCIE_PWRSEQ_QCA6390
>>> >> +    tristate "PCIe Power Sequencing driver for QCA6390"
>>> >> +    depends on ARCH_QCOM || COMPILE_TEST
>>> >> +    help
>>> >> +      Enable support for the PCIe power sequencing driver for the
>>> >> +      ath11k module of the QCA6390 WLAN/BT chip.
>>> >> +
>>> >> +endif
>>> >
>>> > As I mentioned in the 5/9 patch I'm concerned that the current
>>> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
>>> > the fact that QCA6390 may need additional configuration since the menu
>>> > item will only show up if you have already enabled PCIE_PWRSEQ.
>>> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
>>> > about the more generic case.
>>> >
>>> > I'm wondering if there should be a separate config QCA6390 within ath11k
>>> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
>>>
>>> Or is it possible to provide an optional dependency in Kconfig (I guess
>>
>> imply PCIE_PWRSEQ
>> imply PCIE_PWRSEQ_QCA6390
>> ?
>
> Nice, I had forgotten imply altogether. Would 'imply
> PCIE_PWRSEQ_QCA6390' in ath11k Kconfig be enough to address Jeff's
> concern?

Please don't use imply (ever), it doesn't normally do
what you want. In this case, the only effect the
'imply' has is to change the default of the PCIE_PWRSEQ_QCA6390
option when a defconfig contains QCA6390.

If this is indeed what you want, it's still better to do the
equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:

config PCIE_PWRSEQ_QCA6390
      tristate "PCIe Power Sequencing driver for QCA6390"
      default ATH11K && ARCH_QCOM

      Arnd

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Chen-Yu Tsai]

On Tue, Jan 9, 2024 at 6:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
> > Chen-Yu Tsai <wenst@chromium.org> writes:
> >> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <kvalo@kernel.org> wrote:
> >>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> >>>
> >>> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> >>> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
> >>> >> index 010e31f432c9..f9fe555b8506 100644
> >>> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
> >>> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
> >>> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
> >>> >>      help
> >>> >>        Say yes here to enable support for PCIe power sequencing
> >>> >>        drivers.
> >>> >> +
> >>> >> +if PCIE_PWRSEQ
> >>> >> +
> >>> >> +config PCIE_PWRSEQ_QCA6390
> >>> >> +    tristate "PCIe Power Sequencing driver for QCA6390"
> >>> >> +    depends on ARCH_QCOM || COMPILE_TEST
> >>> >> +    help
> >>> >> +      Enable support for the PCIe power sequencing driver for the
> >>> >> +      ath11k module of the QCA6390 WLAN/BT chip.
> >>> >> +
> >>> >> +endif
> >>> >
> >>> > As I mentioned in the 5/9 patch I'm concerned that the current
> >>> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
> >>> > the fact that QCA6390 may need additional configuration since the menu
> >>> > item will only show up if you have already enabled PCIE_PWRSEQ.
> >>> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
> >>> > about the more generic case.
> >>> >
> >>> > I'm wondering if there should be a separate config QCA6390 within ath11k
> >>> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
> >>>
> >>> Or is it possible to provide an optional dependency in Kconfig (I guess
> >>
> >> imply PCIE_PWRSEQ
> >> imply PCIE_PWRSEQ_QCA6390
> >> ?
> >
> > Nice, I had forgotten imply altogether. Would 'imply
> > PCIE_PWRSEQ_QCA6390' in ath11k Kconfig be enough to address Jeff's
> > concern?
>
> Please don't use imply (ever), it doesn't normally do
> what you want. In this case, the only effect the
> 'imply' has is to change the default of the PCIE_PWRSEQ_QCA6390
> option when a defconfig contains QCA6390.
>
> If this is indeed what you want, it's still better to do the
> equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
>
> config PCIE_PWRSEQ_QCA6390
>       tristate "PCIe Power Sequencing driver for QCA6390"
>       default ATH11K && ARCH_QCOM

PCIE_PWRSEQ_QCA6390 is also guarded by PCIE_PWRSEQ though. That would
require the default statement to be duplicated to the PCIE_PWRSEQ option
as well.

Presumably we'd get a few more power sequencing drivers, and the list of
default statements for PCIE_PWRSEQ would grow.

If that's acceptable then Arnd's proposal plus duplicating it to
PCIE_PWRSEQ should work as described.

ChenYu

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Arnd Bergmann]

On Tue, Jan 9, 2024, at 11:26, Chen-Yu Tsai wrote:
> On Tue, Jan 9, 2024 at 6:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
>> > Chen-Yu Tsai <wenst@chromium.org> writes:
>> >> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <kvalo@kernel.org> wrote:

>> If this is indeed what you want, it's still better to do the
>> equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
>>
>> config PCIE_PWRSEQ_QCA6390
>>       tristate "PCIe Power Sequencing driver for QCA6390"
>>       default ATH11K && ARCH_QCOM
>
> PCIE_PWRSEQ_QCA6390 is also guarded by PCIE_PWRSEQ though. That would
> require the default statement to be duplicated to the PCIE_PWRSEQ option
> as well.
>
> Presumably we'd get a few more power sequencing drivers, and the list of
> default statements for PCIE_PWRSEQ would grow.
>
> If that's acceptable then Arnd's proposal plus duplicating it to
> PCIE_PWRSEQ should work as described.

Does PCIE_PWRSEQ need to be user-visible? If this is a hidden symbol
that gets selected by PCIE_PWRSEQ_QCA6390 and any future ones, it
would still get enabled.

Another possibility would be to have PCIE_PWRSEQ be default-enabled,
but allow it to be turned off in order to hide the other options
when users are sure they don't need it (e.g. when building a
specialized kernel for a particular board).

     Arnd

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Kalle Valo]

"Arnd Bergmann" <arnd@arndb.de> writes:

> On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
>
>> Chen-Yu Tsai <wenst@chromium.org> writes:
>>> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <kvalo@kernel.org> wrote:
>>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>>>
>>>> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>>>> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
>>>> >> index 010e31f432c9..f9fe555b8506 100644
>>>> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
>>>> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
>>>> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>>>> >>      help
>>>> >>        Say yes here to enable support for PCIe power sequencing
>>>> >>        drivers.
>>>> >> +
>>>> >> +if PCIE_PWRSEQ
>>>> >> +
>>>> >> +config PCIE_PWRSEQ_QCA6390
>>>> >> +    tristate "PCIe Power Sequencing driver for QCA6390"
>>>> >> +    depends on ARCH_QCOM || COMPILE_TEST
>>>> >> +    help
>>>> >> +      Enable support for the PCIe power sequencing driver for the
>>>> >> +      ath11k module of the QCA6390 WLAN/BT chip.
>>>> >> +
>>>> >> +endif
>>>> >
>>>> > As I mentioned in the 5/9 patch I'm concerned that the current
>>>> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
>>>> > the fact that QCA6390 may need additional configuration since the menu
>>>> > item will only show up if you have already enabled PCIE_PWRSEQ.
>>>> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
>>>> > about the more generic case.
>>>> >
>>>> > I'm wondering if there should be a separate config QCA6390 within ath11k
>>>> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
>>>>
>>>> Or is it possible to provide an optional dependency in Kconfig (I guess
>>>
>>> imply PCIE_PWRSEQ
>>> imply PCIE_PWRSEQ_QCA6390
>>> ?
>>
>> Nice, I had forgotten imply altogether. Would 'imply
>> PCIE_PWRSEQ_QCA6390' in ath11k Kconfig be enough to address Jeff's
>> concern?
>
> Please don't use imply (ever), it doesn't normally do
> what you want. In this case, the only effect the
> 'imply' has is to change the default of the PCIE_PWRSEQ_QCA6390
> option when a defconfig contains QCA6390.
>
> If this is indeed what you want, it's still better to do the
> equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
>
> config PCIE_PWRSEQ_QCA6390
>       tristate "PCIe Power Sequencing driver for QCA6390"
>       default ATH11K && ARCH_QCOM

Sounds good to me but should it be 'default ATH11K_PCI && ARCH_QCOM'? My
understanding is that we don't need PWRSEQ for ATH11K_AHB devices.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC 8/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Arnd Bergmann]

On Tue, Jan 9, 2024, at 17:43, Kalle Valo wrote:
> "Arnd Bergmann" <arnd@arndb.de> writes:
>> On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
>>
>> If this is indeed what you want, it's still better to do the
>> equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
>>
>> config PCIE_PWRSEQ_QCA6390
>>       tristate "PCIe Power Sequencing driver for QCA6390"
>>       default ATH11K && ARCH_QCOM
>
> Sounds good to me but should it be 'default ATH11K_PCI && ARCH_QCOM'? My
> understanding is that we don't need PWRSEQ for ATH11K_AHB devices.

Right, that is better.

    Arnd

[RFC 9/9] arm64: defconfig: enable the PCIe power sequencing for QCA6390

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Build the QCA6390 PCIe power sequencing module by default.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 361c31b5d064..7daa863f25e5 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -210,6 +210,8 @@ CONFIG_NFC_S3FWRN5_I2C=m
 CONFIG_PCI=y
 CONFIG_PCIEPORTBUS=y
 CONFIG_PCIEAER=y
+CONFIG_PCIE_PWRSEQ=y
+CONFIG_PCIE_PWRSEQ_QCA6390=m
 CONFIG_PCI_IOV=y
 CONFIG_PCI_PASID=y
 CONFIG_HOTPLUG_PCI=y
-- 
2.40.1

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3280 bytes --]

Hi,

On Thu, Jan 04, 2024 at 02:01:14PM +0100, Bartosz Golaszewski wrote:
> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
> 
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
> 
> After some additional brain-storming with Manivannan and the realization
> that the DT maintainers won't like adding any "fake" nodes not
> representing actual devices, we decided to reuse the existing
> infrastructure provided by the PCIe port drivers.
> 
> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. For those nodes for which a power-sequencing
> driver exists, we bind it and let it probe. The driver then triggers a
> rescan of the PCI bus with the aim of detecting the now powered-on
> device. The device will consume the same DT node as the platform,
> power-sequencing device. We use device links to make the latter become
> the parent of the former.
> 
> The main advantage of this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.

I recently ran into this issue on a Rockchip platform using a PCIe
based AP6275P WLAN device (broadcom based). As far as I can tell your
proposal should also work for that one (obviously using a different
pwrseq driver).

-- Sebastian

> Bartosz Golaszewski (9):
>   arm64: dts: qcom: sm8250: describe the PCIe port
>   arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390
>   PCI/portdrv: create platform devices for child OF nodes
>   PCI: hold the rescan mutex when scanning for the first time
>   PCI/pwrseq: add pwrseq core code
>   dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros
>   dt-bindings: wireless: ath11k: describe QCA6390
>   PCI/pwrseq: add a pwrseq driver for QCA6390
>   arm64: defconfig: enable the PCIe power sequencing for QCA6390
> 
>  .../net/wireless/qcom,ath11k-pci.yaml         |  14 ++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   1 +
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  24 +++
>  arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
>  arch/arm64/configs/defconfig                  |   2 +
>  drivers/pci/pcie/Kconfig                      |   2 +
>  drivers/pci/pcie/Makefile                     |   2 +
>  drivers/pci/pcie/portdrv.c                    |   3 +-
>  drivers/pci/pcie/pwrseq/Kconfig               |  19 ++
>  drivers/pci/pcie/pwrseq/Makefile              |   4 +
>  drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
>  drivers/pci/pcie/pwrseq/pwrseq.c              |  83 ++++++++
>  drivers/pci/probe.c                           |   2 +
>  include/linux/pcie-pwrseq.h                   |  24 +++
>  14 files changed, 386 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pcie/pwrseq/Kconfig
>  create mode 100644 drivers/pci/pcie/pwrseq/Makefile
>  create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
>  create mode 100644 drivers/pci/pcie/pwrseq/pwrseq.c
>  create mode 100644 include/linux/pcie-pwrseq.h
> 
> -- 
> 2.40.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Neil Armstrong]

Hi,

On 04/01/2024 14:01, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
> 
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
> 
> After some additional brain-storming with Manivannan and the realization
> that the DT maintainers won't like adding any "fake" nodes not
> representing actual devices, we decided to reuse the existing
> infrastructure provided by the PCIe port drivers.
> 
> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. For those nodes for which a power-sequencing
> driver exists, we bind it and let it probe. The driver then triggers a
> rescan of the PCI bus with the aim of detecting the now powered-on
> device. The device will consume the same DT node as the platform,
> power-sequencing device. We use device links to make the latter become
> the parent of the former.
> 
> The main advantage of this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.

I've successfully tested this serie for the WCN7850 Wifi/BT combo onboard chip
present on the SM8550-QRD and SM8650-QRD boards and it works just fine.

Here's a branch with the wcn7850 vreg table added to the pwrseq driver,
and the DT changes:
https://git.codelinaro.org/neil.armstrong/linux/-/commits/topic/sm8x50/wcn7850-wifi-pwrseq/?ref_type=heads

Thanks,
Neil

> 
> Bartosz Golaszewski (9):
>    arm64: dts: qcom: sm8250: describe the PCIe port
>    arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390
>    PCI/portdrv: create platform devices for child OF nodes
>    PCI: hold the rescan mutex when scanning for the first time
>    PCI/pwrseq: add pwrseq core code
>    dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros
>    dt-bindings: wireless: ath11k: describe QCA6390
>    PCI/pwrseq: add a pwrseq driver for QCA6390
>    arm64: defconfig: enable the PCIe power sequencing for QCA6390
> 
>   .../net/wireless/qcom,ath11k-pci.yaml         |  14 ++
>   .../devicetree/bindings/vendor-prefixes.yaml  |   1 +
>   arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  24 +++
>   arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
>   arch/arm64/configs/defconfig                  |   2 +
>   drivers/pci/pcie/Kconfig                      |   2 +
>   drivers/pci/pcie/Makefile                     |   2 +
>   drivers/pci/pcie/portdrv.c                    |   3 +-
>   drivers/pci/pcie/pwrseq/Kconfig               |  19 ++
>   drivers/pci/pcie/pwrseq/Makefile              |   4 +
>   drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
>   drivers/pci/pcie/pwrseq/pwrseq.c              |  83 ++++++++
>   drivers/pci/probe.c                           |   2 +
>   include/linux/pcie-pwrseq.h                   |  24 +++
>   14 files changed, 386 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/pci/pcie/pwrseq/Kconfig
>   create mode 100644 drivers/pci/pcie/pwrseq/Makefile
>   create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
>   create mode 100644 drivers/pci/pcie/pwrseq/pwrseq.c
>   create mode 100644 include/linux/pcie-pwrseq.h
> 

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

On Mon, Jan 8, 2024 at 4:24 PM Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Hi,
>
> On 04/01/2024 14:01, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > During last year's Linux Plumbers we had several discussions centered
> > around the need to power-on PCI devices before they can be detected on
> > the bus.
> >
> > The consensus during the conference was that we need to introduce a
> > class of "PCI slot drivers" that would handle the power-sequencing.
> >
> > After some additional brain-storming with Manivannan and the realization
> > that the DT maintainers won't like adding any "fake" nodes not
> > representing actual devices, we decided to reuse the existing
> > infrastructure provided by the PCIe port drivers.
> >
> > The general idea is to instantiate platform devices for child nodes of
> > the PCIe port DT node. For those nodes for which a power-sequencing
> > driver exists, we bind it and let it probe. The driver then triggers a
> > rescan of the PCI bus with the aim of detecting the now powered-on
> > device. The device will consume the same DT node as the platform,
> > power-sequencing device. We use device links to make the latter become
> > the parent of the former.
> >
> > The main advantage of this approach is not modifying the existing DT in
> > any way and especially not adding any "fake" platform devices.
>
> I've successfully tested this serie for the WCN7850 Wifi/BT combo onboard chip
> present on the SM8550-QRD and SM8650-QRD boards and it works just fine.
>
> Here's a branch with the wcn7850 vreg table added to the pwrseq driver,
> and the DT changes:
> https://git.codelinaro.org/neil.armstrong/linux/-/commits/topic/sm8x50/wcn7850-wifi-pwrseq/?ref_type=heads

Thanks, I'll integrate them into v2.

Bart

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Florian Fainelli]

Hello,

On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
> 
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
> 
> After some additional brain-storming with Manivannan and the realization
> that the DT maintainers won't like adding any "fake" nodes not
> representing actual devices, we decided to reuse the existing
> infrastructure provided by the PCIe port drivers.
> 
> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. For those nodes for which a power-sequencing
> driver exists, we bind it and let it probe. The driver then triggers a
> rescan of the PCI bus with the aim of detecting the now powered-on
> device. The device will consume the same DT node as the platform,
> power-sequencing device. We use device links to make the latter become
> the parent of the former.
> 
> The main advantage of this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.

There is prior work in that area which was applied, but eventually reverted:

https://www.spinics.net/lists/linux-pci/msg119136.html

and finally re-applied albeit in a different shape:

https://lore.kernel.org/all/20220716222454.29914-1-jim2101024@gmail.com/

so we might want to think about how to have pcie-brcmstb.c converted 
over your proposed approach. AFAIR there is also pcie-rockchip.c which 
has some rudimentary support for voltage regulators of PCIe end-points.

What does not yet appear in this RFC is support for suspend/resume, 
especially for power states where both the RC and the EP might be losing 
power. There also needs to be some thoughts given to wake-up enabled 
PCIe devices like Wi-Fi which might need to remain powered on to service 
Wake-on-WLAN frames if nothing else.

I sense a potential for a lot of custom power sequencing drivers being 
added and ultimately leading to the decision to create a "generic" one 
which is entirely driven by Device Tree properties...

Thanks for doing this!
-- 
Florian

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Chen-Yu Tsai]

On Tue, Jan 9, 2024 at 12:09 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> Hello,
>
> On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > During last year's Linux Plumbers we had several discussions centered
> > around the need to power-on PCI devices before they can be detected on
> > the bus.
> >
> > The consensus during the conference was that we need to introduce a
> > class of "PCI slot drivers" that would handle the power-sequencing.
> >
> > After some additional brain-storming with Manivannan and the realization
> > that the DT maintainers won't like adding any "fake" nodes not
> > representing actual devices, we decided to reuse the existing
> > infrastructure provided by the PCIe port drivers.
> >
> > The general idea is to instantiate platform devices for child nodes of
> > the PCIe port DT node. For those nodes for which a power-sequencing
> > driver exists, we bind it and let it probe. The driver then triggers a
> > rescan of the PCI bus with the aim of detecting the now powered-on
> > device. The device will consume the same DT node as the platform,
> > power-sequencing device. We use device links to make the latter become
> > the parent of the former.
> >
> > The main advantage of this approach is not modifying the existing DT in
> > any way and especially not adding any "fake" platform devices.
>
> There is prior work in that area which was applied, but eventually reverted:
>
> https://www.spinics.net/lists/linux-pci/msg119136.html
>
> and finally re-applied albeit in a different shape:
>
> https://lore.kernel.org/all/20220716222454.29914-1-jim2101024@gmail.com/
>
> so we might want to think about how to have pcie-brcmstb.c converted
> over your proposed approach. AFAIR there is also pcie-rockchip.c which
> has some rudimentary support for voltage regulators of PCIe end-points.

I think the current in-tree approaches mostly target either PCIe slots,
whether full size or mini-PCIe or M.2, or soldered-on components that
either only have a single power rail, have internal regulators, or have
surrounding circuitry that would be incorporated on a PCIe card.

These all have standardized power rails (+12V, +3.3V, +3.3V aux, etc.).

> What does not yet appear in this RFC is support for suspend/resume,
> especially for power states where both the RC and the EP might be losing
> power. There also needs to be some thoughts given to wake-up enabled
> PCIe devices like Wi-Fi which might need to remain powered on to service
> Wake-on-WLAN frames if nothing else.
>
> I sense a potential for a lot of custom power sequencing drivers being
> added and ultimately leading to the decision to create a "generic" one
> which is entirely driven by Device Tree properties...

We can have one "generic" slot power sequencing driver, which just
enables all the power rails together. I would very much like to see that.

I believe the power sequencing in this series is currently targeting more
tightly coupled designs that use power rails directly from the PMIC, and
thus require more explicit power sequencing.

ChenYu


> Thanks for doing this!
> --
> Florian
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Manivannan Sadhasivam]

On Tue, Jan 09, 2024 at 03:08:32PM +0800, Chen-Yu Tsai wrote:
> On Tue, Jan 9, 2024 at 12:09 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > Hello,
> >
> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > During last year's Linux Plumbers we had several discussions centered
> > > around the need to power-on PCI devices before they can be detected on
> > > the bus.
> > >
> > > The consensus during the conference was that we need to introduce a
> > > class of "PCI slot drivers" that would handle the power-sequencing.
> > >
> > > After some additional brain-storming with Manivannan and the realization
> > > that the DT maintainers won't like adding any "fake" nodes not
> > > representing actual devices, we decided to reuse the existing
> > > infrastructure provided by the PCIe port drivers.
> > >
> > > The general idea is to instantiate platform devices for child nodes of
> > > the PCIe port DT node. For those nodes for which a power-sequencing
> > > driver exists, we bind it and let it probe. The driver then triggers a
> > > rescan of the PCI bus with the aim of detecting the now powered-on
> > > device. The device will consume the same DT node as the platform,
> > > power-sequencing device. We use device links to make the latter become
> > > the parent of the former.
> > >
> > > The main advantage of this approach is not modifying the existing DT in
> > > any way and especially not adding any "fake" platform devices.
> >
> > There is prior work in that area which was applied, but eventually reverted:
> >
> > https://www.spinics.net/lists/linux-pci/msg119136.html
> >
> > and finally re-applied albeit in a different shape:
> >
> > https://lore.kernel.org/all/20220716222454.29914-1-jim2101024@gmail.com/
> >
> > so we might want to think about how to have pcie-brcmstb.c converted
> > over your proposed approach. AFAIR there is also pcie-rockchip.c which
> > has some rudimentary support for voltage regulators of PCIe end-points.
> 
> I think the current in-tree approaches mostly target either PCIe slots,
> whether full size or mini-PCIe or M.2, or soldered-on components that
> either only have a single power rail, have internal regulators, or have
> surrounding circuitry that would be incorporated on a PCIe card.
> 
> These all have standardized power rails (+12V, +3.3V, +3.3V aux, etc.).
> 

Right. But ideally, they should also be converted to use this power sequencing
driver at some point in the future.

> > What does not yet appear in this RFC is support for suspend/resume,
> > especially for power states where both the RC and the EP might be losing
> > power. There also needs to be some thoughts given to wake-up enabled
> > PCIe devices like Wi-Fi which might need to remain powered on to service
> > Wake-on-WLAN frames if nothing else.
> >

I don't think it is necessary to add PM support in this series itself. Even
though PM support is always nice to have or even necessary for controllers
pulling the power plug during suspend, it makes sense to merge this basic
implementation and add features on top.

It should be noted that both the controller drivers and the power sequencing
drivers should be in sync during suspend i.e., if the controller driver decides
to put the device into D3Cold and turning off the controller power supplies,
then this driver should also power off the device. But if the controller driver
decides to keep the device in low power states (ASPM), then this driver
_should_not_ turn off the device power.

Right now, there is no global policy for controller drivers and each one does a
different job. IMO this should also be fixed, but that also requires an
agreement from PM folks.

(Well there is one more entity in this loop, PCIe device drivers... sigh)

> > I sense a potential for a lot of custom power sequencing drivers being
> > added and ultimately leading to the decision to create a "generic" one
> > which is entirely driven by Device Tree properties...
> 
> We can have one "generic" slot power sequencing driver, which just
> enables all the power rails together. I would very much like to see that.
> 

Yeah. And that "generic" driver could be used for simple usecases mentioned
above.

> I believe the power sequencing in this series is currently targeting more
> tightly coupled designs that use power rails directly from the PMIC, and
> thus require more explicit power sequencing.
> 

Precisely!

- Mani

> ChenYu
> 
> 
> > Thanks for doing this!
> > --
> > Florian
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Geert Uytterhoeven]

Hi ChenYu,

CC wsa + renesas-soc

On Tue, Jan 9, 2024 at 8:08 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> On Tue, Jan 9, 2024 at 12:09 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > During last year's Linux Plumbers we had several discussions centered
> > > around the need to power-on PCI devices before they can be detected on
> > > the bus.
> > >
> > > The consensus during the conference was that we need to introduce a
> > > class of "PCI slot drivers" that would handle the power-sequencing.
> > >
> > > After some additional brain-storming with Manivannan and the realization
> > > that the DT maintainers won't like adding any "fake" nodes not
> > > representing actual devices, we decided to reuse the existing
> > > infrastructure provided by the PCIe port drivers.
> > >
> > > The general idea is to instantiate platform devices for child nodes of
> > > the PCIe port DT node. For those nodes for which a power-sequencing
> > > driver exists, we bind it and let it probe. The driver then triggers a
> > > rescan of the PCI bus with the aim of detecting the now powered-on
> > > device. The device will consume the same DT node as the platform,
> > > power-sequencing device. We use device links to make the latter become
> > > the parent of the former.
> > >
> > > The main advantage of this approach is not modifying the existing DT in
> > > any way and especially not adding any "fake" platform devices.
> >
> > There is prior work in that area which was applied, but eventually reverted:
> >
> > https://www.spinics.net/lists/linux-pci/msg119136.html
> >
> > and finally re-applied albeit in a different shape:
> >
> > https://lore.kernel.org/all/20220716222454.29914-1-jim2101024@gmail.com/
> >
> > so we might want to think about how to have pcie-brcmstb.c converted
> > over your proposed approach. AFAIR there is also pcie-rockchip.c which
> > has some rudimentary support for voltage regulators of PCIe end-points.
>
> I think the current in-tree approaches mostly target either PCIe slots,
> whether full size or mini-PCIe or M.2, or soldered-on components that
> either only have a single power rail, have internal regulators, or have
> surrounding circuitry that would be incorporated on a PCIe card.
>
> These all have standardized power rails (+12V, +3.3V, +3.3V aux, etc.).

Indeed. E.g. R-Car PCIe just got support for that in commit
6797e4da2dd1e2c8 ("PCI: rcar-host: Add support for optional regulators")
in pci/next.

> > What does not yet appear in this RFC is support for suspend/resume,
> > especially for power states where both the RC and the EP might be losing
> > power. There also needs to be some thoughts given to wake-up enabled
> > PCIe devices like Wi-Fi which might need to remain powered on to service
> > Wake-on-WLAN frames if nothing else.
> >
> > I sense a potential for a lot of custom power sequencing drivers being
> > added and ultimately leading to the decision to create a "generic" one
> > which is entirely driven by Device Tree properties...
>
> We can have one "generic" slot power sequencing driver, which just
> enables all the power rails together. I would very much like to see that.
>
> I believe the power sequencing in this series is currently targeting more
> tightly coupled designs that use power rails directly from the PMIC, and
> thus require more explicit power sequencing.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Kalle Valo]

Florian Fainelli <f.fainelli@gmail.com> writes:

> Hello,
>
> On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> During last year's Linux Plumbers we had several discussions
>> centered
>> around the need to power-on PCI devices before they can be detected on
>> the bus.
>> The consensus during the conference was that we need to introduce a
>> class of "PCI slot drivers" that would handle the power-sequencing.
>> After some additional brain-storming with Manivannan and the
>> realization
>> that the DT maintainers won't like adding any "fake" nodes not
>> representing actual devices, we decided to reuse the existing
>> infrastructure provided by the PCIe port drivers.
>> The general idea is to instantiate platform devices for child nodes
>> of
>> the PCIe port DT node. For those nodes for which a power-sequencing
>> driver exists, we bind it and let it probe. The driver then triggers a
>> rescan of the PCI bus with the aim of detecting the now powered-on
>> device. The device will consume the same DT node as the platform,
>> power-sequencing device. We use device links to make the latter become
>> the parent of the former.
>> The main advantage of this approach is not modifying the existing DT
>> in
>> any way and especially not adding any "fake" platform devices.
>
> There is prior work in that area which was applied, but eventually reverted:
>
> https://www.spinics.net/lists/linux-pci/msg119136.html
>
> and finally re-applied albeit in a different shape:
>
> https://lore.kernel.org/all/20220716222454.29914-1-jim2101024@gmail.com/
>
> so we might want to think about how to have pcie-brcmstb.c converted
> over your proposed approach. AFAIR there is also pcie-rockchip.c which
> has some rudimentary support for voltage regulators of PCIe
> end-points.
>
> What does not yet appear in this RFC is support for suspend/resume,
> especially for power states where both the RC and the EP might be
> losing power. There also needs to be some thoughts given to wake-up
> enabled PCIe devices like Wi-Fi which might need to remain powered on
> to service Wake-on-WLAN frames if nothing else.

Good point, suspend and resume is very important for ath11k and ath12k.
And although I'm not expecting any issues with hibernation please also
consider that. Currently ath11k hibernation is broken because of MHI and
it has been pain trying to fix that. So best to make sure these cases
work from the beginning.

> I sense a potential for a lot of custom power sequencing drivers being
> added and ultimately leading to the decision to create a "generic" one
> which is entirely driven by Device Tree properties...
>
> Thanks for doing this!

A big thank you from me too! It's great to finally see this solved so
that ath11k and ath12k can be used in wider range of devices.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches