Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

11 messages, 4 authors, 2021-02-03 · open the first message on its own page

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Dmitry Baryshkov <hidden>
Date: 2021-01-29 03:46:26

On 28/01/2021 22:26, Rob Herring wrote:
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and 
changing pcie port driver to apply power supply before bus probing 
happens, I'd also ask for the comments from PCI maintainers. Will that 
solution be acceptable to you?

quoted
Signed-off-by: Dmitry Baryshkov <redacted>
---
  drivers/pci/controller/dwc/pcie-qcom.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ab21aa01c95d..eb73c8540d4d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -20,6 +20,7 @@
  #include <linux/of_device.h>
  #include <linux/of_gpio.h>
  #include <linux/pci.h>
+#include <linux/pm_domain.h>
  #include <linux/pm_runtime.h>
  #include <linux/platform_device.h>
  #include <linux/phy/phy.h>
@@ -1568,6 +1569,26 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);

+static void qcom_fixup_power(struct pci_dev *dev)
+{
+       int ret;
+       struct pcie_port *pp = dev->bus->sysdata;
+       struct dw_pcie *pci;
+
+       if (!pci_is_root_bus(dev->bus))
+               return;
+
+       ret = dev_pm_domain_attach(&dev->dev, true);
+       if (ret < 0 || !dev->dev.pm_domain)
+               return;
+
+       pci = to_dw_pcie_from_pp(pp);
+       dev_info(&dev->dev, "Bus powered up, waiting for link to come up\n");
+
+       dw_pcie_wait_for_link(pci);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x010b, qcom_fixup_power);
+
  static struct platform_driver qcom_pcie_driver = {
         .probe = qcom_pcie_probe,
         .driver = {
--
2.29.2

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-01-29 21:51:47

On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?

Nit: when changing any file, please take a look at the commit history
and make yours match, e.g.,

  pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

does not look like:

  PCI: qcom: Add support for configuring BDF to SID mapping for SM8250
  PCI: qcom: Add SM8250 SoC support
  PCI: qcom: Make sure PCIe is reset before init for rev 2.1.0
  PCI: qcom: Replace define with standard value
  PCI: qcom: Support pci speed set for ipq806x
  PCI: qcom: Add ipq8064 rev2 variant

Also, if you capitalize it as "QCA6390" in the commit log, do it the
same in the subject.
quoted
quoted
Signed-off-by: Dmitry Baryshkov <redacted>
---
  drivers/pci/controller/dwc/pcie-qcom.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ab21aa01c95d..eb73c8540d4d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -20,6 +20,7 @@
  #include <linux/of_device.h>
  #include <linux/of_gpio.h>
  #include <linux/pci.h>
+#include <linux/pm_domain.h>
  #include <linux/pm_runtime.h>
  #include <linux/platform_device.h>
  #include <linux/phy/phy.h>
@@ -1568,6 +1569,26 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);

+static void qcom_fixup_power(struct pci_dev *dev)
+{
+       int ret;
+       struct pcie_port *pp = dev->bus->sysdata;
+       struct dw_pcie *pci;
+
+       if (!pci_is_root_bus(dev->bus))
+               return;
+
+       ret = dev_pm_domain_attach(&dev->dev, true);
+       if (ret < 0 || !dev->dev.pm_domain)
+               return;
+
+       pci = to_dw_pcie_from_pp(pp);
+       dev_info(&dev->dev, "Bus powered up, waiting for link to come up\n");
+
+       dw_pcie_wait_for_link(pci);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x010b, qcom_fixup_power);
+
  static struct platform_driver qcom_pcie_driver = {
         .probe = qcom_pcie_probe,
         .driver = {
--
2.29.2

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Dmitry Baryshkov <hidden>
Date: 2021-01-29 22:20:25

On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.

I'm not sure about link-up interrupt. I've just lightly tested using
PCIe HP on this port, getting no interrupts from it.
If I manually rescan the bus after enabling the qca6390 device (e.g.
via sysfs), it gets enabled, but then I see PCIe link errors (most
probably because the PCIe link was not retrained after the device
comes up).
Nit: when changing any file, please take a look at the commit history
and make yours match, e.g.,

  pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

does not look like:
Ack.


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-01-29 22:40:11

On Sat, Jan 30, 2021 at 01:19:10AM +0300, Dmitry Baryshkov wrote:
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
OK, just elaborate on this a bit in the commit log so the topology
makes sense to someone with no prior knowledge of QCA6390.

Bjorn

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Rob Herring <robh+dt@kernel.org>
Date: 2021-01-29 23:40:17

On Thu, Jan 28, 2021 at 9:45 PM Dmitry Baryshkov
[off-list ref] wrote:
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing
happens, I'd also ask for the comments from PCI maintainers. Will that
solution be acceptable to you?
Oh good, something exists. :)

FYI, there's another similar case needing this that just popped up[1].

Rob

[1] https://lore.kernel.org/linux-pci/20210129173057.30288c9d@coco.lan/

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Bjorn Andersson <hidden>
Date: 2021-01-30 04:36:45

On Fri 29 Jan 16:19 CST 2021, Dmitry Baryshkov wrote:
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
So what you're saying is that if either the PCI controller or bluetooth
driver probes these regulators will be turned on, indefinitely?

If so, why do we need a driver to turn them on, rather than just mark
them as always-on?

What's the timing requirement wrt regulators vs WL_EN/BT_EN?

Regards,
Bjorn A
I'm not sure about link-up interrupt. I've just lightly tested using
PCIe HP on this port, getting no interrupts from it.
If I manually rescan the bus after enabling the qca6390 device (e.g.
via sysfs), it gets enabled, but then I see PCIe link errors (most
probably because the PCIe link was not retrained after the device
comes up).
quoted
Nit: when changing any file, please take a look at the commit history
and make yours match, e.g.,

  pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

does not look like:
Ack.


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Dmitry Baryshkov <hidden>
Date: 2021-01-30 16:18:58

On Sat, 30 Jan 2021 at 06:53, Bjorn Andersson
[off-list ref] wrote:
On Fri 29 Jan 16:19 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
So what you're saying is that if either the PCI controller or bluetooth
driver probes these regulators will be turned on, indefinitely?

If so, why do we need a driver to turn them on, rather than just mark
them as always-on?

What's the timing requirement wrt regulators vs WL_EN/BT_EN?
According to the documentation I have, they must be enabled right
after enabling powering the chip and they must stay enabled all the
time.




-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Bjorn Andersson <hidden>
Date: 2021-02-02 19:51:17

On Sat 30 Jan 10:14 CST 2021, Dmitry Baryshkov wrote:
On Sat, 30 Jan 2021 at 06:53, Bjorn Andersson
[off-list ref] wrote:
quoted
On Fri 29 Jan 16:19 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
So what you're saying is that if either the PCI controller or bluetooth
driver probes these regulators will be turned on, indefinitely?

If so, why do we need a driver to turn them on, rather than just mark
them as always-on?

What's the timing requirement wrt regulators vs WL_EN/BT_EN?
According to the documentation I have, they must be enabled right
after enabling powering the chip and they must stay enabled all the
time.
So presumably just marking these things always-on and flipping the GPIO
statically won't be good enough due to the lack of control over the
timing.

This really do look like a simplified case of what we see with the
PCIe attached modems, where similar requirements are provided, but also
the ability to perform a device specific reset sequence in case the
hardware has locked up. I'm slightly worried about the ability of
extending your power-domain model to handle the restart operation
though.

Regards,
Bjorn

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Rob Herring <robh+dt@kernel.org>
Date: 2021-02-02 21:38:30

On Tue, Feb 2, 2021 at 1:48 PM Bjorn Andersson
[off-list ref] wrote:
On Sat 30 Jan 10:14 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 06:53, Bjorn Andersson
[off-list ref] wrote:
quoted
On Fri 29 Jan 16:19 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
So what you're saying is that if either the PCI controller or bluetooth
driver probes these regulators will be turned on, indefinitely?

If so, why do we need a driver to turn them on, rather than just mark
them as always-on?

What's the timing requirement wrt regulators vs WL_EN/BT_EN?
According to the documentation I have, they must be enabled right
after enabling powering the chip and they must stay enabled all the
time.
So presumably just marking these things always-on and flipping the GPIO
statically won't be good enough due to the lack of control over the
timing.

This really do look like a simplified case of what we see with the
PCIe attached modems, where similar requirements are provided, but also
the ability to perform a device specific reset sequence in case the
hardware has locked up. I'm slightly worried about the ability of
extending your power-domain model to handle the restart operation
though.
I think this is an abuse of 'power-domains'. Just define the
regulators in both WiFi and BT nodes and have each driver enable them.
They're refcounted. If that's still not enough control over the power
sequencing, then create a 3rd entity to do it, but that doesn't need
to leak into DT. You already have all the information you need.

Rob

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Bjorn Andersson <hidden>
Date: 2021-02-02 21:51:47

On Tue 02 Feb 15:37 CST 2021, Rob Herring wrote:
On Tue, Feb 2, 2021 at 1:48 PM Bjorn Andersson
[off-list ref] wrote:
quoted
On Sat 30 Jan 10:14 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 06:53, Bjorn Andersson
[off-list ref] wrote:
quoted
On Fri 29 Jan 16:19 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
So what you're saying is that if either the PCI controller or bluetooth
driver probes these regulators will be turned on, indefinitely?

If so, why do we need a driver to turn them on, rather than just mark
them as always-on?

What's the timing requirement wrt regulators vs WL_EN/BT_EN?
According to the documentation I have, they must be enabled right
after enabling powering the chip and they must stay enabled all the
time.
So presumably just marking these things always-on and flipping the GPIO
statically won't be good enough due to the lack of control over the
timing.

This really do look like a simplified case of what we see with the
PCIe attached modems, where similar requirements are provided, but also
the ability to perform a device specific reset sequence in case the
hardware has locked up. I'm slightly worried about the ability of
extending your power-domain model to handle the restart operation
though.
I think this is an abuse of 'power-domains'. Just define the
regulators in both WiFi and BT nodes and have each driver enable them.
They're refcounted. If that's still not enough control over the power
sequencing, then create a 3rd entity to do it, but that doesn't need
to leak into DT. You already have all the information you need.
As Dmitry explained he still need to pull the two GPIOs high after
enabling the regulators, but before scanning the PCI or serdev buses.

I was thinking something along the lines you suggest, but I've not been
able to come up with something that will guarantee the ordering of the
events.

Regards,
Bjorn

Re: [PATCH v2 3/5] pcie-qcom: provide a way to power up qca6390 chip on RB5 platform

From: Dmitry Baryshkov <hidden>
Date: 2021-02-03 04:15:33

On Wed, 3 Feb 2021 at 00:37, Rob Herring [off-list ref] wrote:
On Tue, Feb 2, 2021 at 1:48 PM Bjorn Andersson
[off-list ref] wrote:
quoted
On Sat 30 Jan 10:14 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 06:53, Bjorn Andersson
[off-list ref] wrote:
quoted
On Fri 29 Jan 16:19 CST 2021, Dmitry Baryshkov wrote:
quoted
On Sat, 30 Jan 2021 at 00:50, Bjorn Helgaas [off-list ref] wrote:
quoted
On Fri, Jan 29, 2021 at 06:45:21AM +0300, Dmitry Baryshkov wrote:
quoted
On 28/01/2021 22:26, Rob Herring wrote:
quoted
On Thu, Jan 28, 2021 at 11:52 AM Dmitry Baryshkov
[off-list ref] wrote:
quoted
Some Qualcomm platforms require to power up an external device before
probing the PCI bus. E.g. on RB5 platform the QCA6390 WiFi/BT chip needs
to be powered up before PCIe0 bus is probed. Add a quirk to the
respective PCIe root bridge to attach to the power domain if one is
required, so that the QCA chip is started before scanning the PCIe bus.
This is solving a generic problem in a specific driver. It needs to be
solved for any PCI host and any device.
Ack. I see your point here.

As this would require porting code from powerpc/spark of-pci code and
changing pcie port driver to apply power supply before bus probing happens,
I'd also ask for the comments from PCI maintainers. Will that solution be
acceptable to you?
I can't say without seeing the code.  I don't know enough about this
scenario to envision how it might look.

I guess the QCA6390 is a PCIe device?  Why does it need to be powered
up before probing?  Shouldn't we get a link-up interrupt when it is
powered up so we could probe it then?
Not quite. QCA6390 is a multifunction device, with PCIe and serial
parts. It has internal power regulators which once enabled will
powerup the PCIe, serial and radio parts. There is no need to manage
regulators. Once enabled they will automatically handle device
suspend/resume, etc.
So what you're saying is that if either the PCI controller or bluetooth
driver probes these regulators will be turned on, indefinitely?

If so, why do we need a driver to turn them on, rather than just mark
them as always-on?

What's the timing requirement wrt regulators vs WL_EN/BT_EN?
According to the documentation I have, they must be enabled right
after enabling powering the chip and they must stay enabled all the
time.
So presumably just marking these things always-on and flipping the GPIO
statically won't be good enough due to the lack of control over the
timing.

This really do look like a simplified case of what we see with the
PCIe attached modems, where similar requirements are provided, but also
the ability to perform a device specific reset sequence in case the
hardware has locked up. I'm slightly worried about the ability of
extending your power-domain model to handle the restart operation
though.
I think this is an abuse of 'power-domains'. Just define the
regulators in both WiFi and BT nodes and have each driver enable them.
I think it is too late to enable regulators in the WiFi driver. Even
if I modify the/pci code to register devices basing on the OF nodes
(like we do for PPC and Sparc), necessary link training/hotplug
handling should happen outside of the WiFi driver.
They're refcounted. If that's still not enough control over the power
sequencing, then create a 3rd entity to do it, but that doesn't need
to leak into DT. You already have all the information you need.
From my point of view the proposed design (with three nodes) exactly
represents the hardware: the power-handling part, the WiFi part and
the BT part. If you don't like the power domains, would regulators be
better from your point of view? The "power" device providing a
regulator to be used by the child nodes. For the BT part the regulator
is fine, while for the WiFi...

The major problem with regulators in this case is that they are
typically enabled by the device driver itself, rather than by parent
device/bus code. And in the WiFi driver case the WiFi chip should be
already up and running before probing the ath11k driver.

Maybe it would still be better to take a step back and just introduce
'vcc-child-supply' entry to the pcie-qcom device tree node or to the
PCIe bridge node? This would also cover cases of PCIe mezzanine boards
when the on-mezzanine devices are visible through the PCIe bus, but
the mezzanine is powered by a separate voltage regulator. It would not
be possible to describe child devices in the device tree node, but
rather it would be possible to describe that there might be devices
behind the PCIe bridge, they must be powered on using a referenced
regulator. This starts to sound like a kind of PCI hotplug.

This would result in the following nodes:

pcie0: pci@1c0000 {
   compatible = "qcom,pcie-sm8250";
   [....]
   bridge@0,0 {
        compatible = "pci17cb,010b", "linux,regulator-hotplug";
        [....]
        vcc-children-supply = <&qca6390>;
        /* known WiFi card */
   };
};

pce1: pci@1c08000 {
   compatible = "qcom,pcie-sm8250";
   [....]
   bridge@1,0 {
        compatible = "pci17cb,010b", "linux,regulator-hotplug";
        [....]
        vcc-children-supply = <&vdc_gpio13>;
        /* unpredictable devices on the mezzanine card */
   };
};

-- 
With best wishes
Dmitry
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help