| Message ID | 20240117160748.37682-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCI: introduce the concept of power sequencing of PCIe devices | expand |
On 17/01/2024 17:07, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > The responses to the RFC were rather positive so here's a proper series. > > 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 DT maintainers won't like adding any "fake" nodes not representing > actual devices, we decided to reuse existing PCI infrastructure. > > 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. > > Changes since RFC: > - move the pwrseq functionality out of the port driver and into PCI core > - add support for WCN7850 to the first pwrseq driver (and update bindings) > - describe the WLAN modules in sm8550-qrd and sm8650-qrd > - rework Kconfig options, drop the defconfig changes from the series as > they are no longer needed > - drop the dt-binding changes for PCI vendor codes > - extend the DT bindings for ath11k_pci with strict property checking > - various minor tweaks and fixes > > Bartosz Golaszewski (7): > arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390 > PCI: create platform devices for child OF nodes of the port node > PCI: hold the rescan mutex when scanning for the first time > PCI/pwrseq: add pwrseq core code > dt-bindings: wireless: ath11k: describe QCA6390 > dt-bindings: wireless: ath11k: describe WCN7850 > PCI/pwrseq: add a pwrseq driver for QCA6390 > > Neil Armstrong (2): > arm64: dts: qcom: sm8550-qrd: add Wifi nodes > arm64: dts: qcom: sm8650-qrd: add Wifi nodes > > .../net/wireless/qcom,ath11k-pci.yaml | 89 ++++++ > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 29 ++ > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 + > arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 37 +++ > arch/arm64/boot/dts/qcom/sm8550.dtsi | 10 + > arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 ++ > arch/arm64/boot/dts/qcom/sm8650.dtsi | 10 + > drivers/pci/Kconfig | 1 + > drivers/pci/Makefile | 1 + > drivers/pci/bus.c | 9 +- > drivers/pci/probe.c | 2 + > drivers/pci/pwrseq/Kconfig | 16 ++ > drivers/pci/pwrseq/Makefile | 4 + > drivers/pci/pwrseq/pci-pwrseq-qca6390.c | 267 ++++++++++++++++++ > drivers/pci/pwrseq/pwrseq.c | 82 ++++++ > drivers/pci/remove.c | 3 +- > include/linux/pci-pwrseq.h | 24 ++ > 17 files changed, 621 insertions(+), 2 deletions(-) > create mode 100644 drivers/pci/pwrseq/Kconfig > create mode 100644 drivers/pci/pwrseq/Makefile > create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c > create mode 100644 drivers/pci/pwrseq/pwrseq.c > create mode 100644 include/linux/pci-pwrseq.h > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD Thanks, Neil
On Wed, 17 Jan 2024 at 18:08, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > The responses to the RFC were rather positive so here's a proper series. > > 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 DT maintainers won't like adding any "fake" nodes not representing > actual devices, we decided to reuse existing PCI infrastructure. > > 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'd still like to see how this can be extended to handle BT power up, having a single entity driving both of the BT and WiFI. The device tree changes behave in exactly the opposite way: they define regulators for the WiFi device, while the WiFi is not being powered by these regulators. Both WiFi and BT are powered by the PMU, which in turn consumes all specified regulators. > > Changes since RFC: > - move the pwrseq functionality out of the port driver and into PCI core > - add support for WCN7850 to the first pwrseq driver (and update bindings) > - describe the WLAN modules in sm8550-qrd and sm8650-qrd > - rework Kconfig options, drop the defconfig changes from the series as > they are no longer needed > - drop the dt-binding changes for PCI vendor codes > - extend the DT bindings for ath11k_pci with strict property checking > - various minor tweaks and fixes > > Bartosz Golaszewski (7): > arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390 > PCI: create platform devices for child OF nodes of the port node > PCI: hold the rescan mutex when scanning for the first time > PCI/pwrseq: add pwrseq core code > dt-bindings: wireless: ath11k: describe QCA6390 > dt-bindings: wireless: ath11k: describe WCN7850 > PCI/pwrseq: add a pwrseq driver for QCA6390 > > Neil Armstrong (2): > arm64: dts: qcom: sm8550-qrd: add Wifi nodes > arm64: dts: qcom: sm8650-qrd: add Wifi nodes > > .../net/wireless/qcom,ath11k-pci.yaml | 89 ++++++ > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 29 ++ > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 + > arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 37 +++ > arch/arm64/boot/dts/qcom/sm8550.dtsi | 10 + > arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 ++ > arch/arm64/boot/dts/qcom/sm8650.dtsi | 10 + > drivers/pci/Kconfig | 1 + > drivers/pci/Makefile | 1 + > drivers/pci/bus.c | 9 +- > drivers/pci/probe.c | 2 + > drivers/pci/pwrseq/Kconfig | 16 ++ > drivers/pci/pwrseq/Makefile | 4 + > drivers/pci/pwrseq/pci-pwrseq-qca6390.c | 267 ++++++++++++++++++ > drivers/pci/pwrseq/pwrseq.c | 82 ++++++ > drivers/pci/remove.c | 3 +- > include/linux/pci-pwrseq.h | 24 ++ > 17 files changed, 621 insertions(+), 2 deletions(-) > create mode 100644 drivers/pci/pwrseq/Kconfig > create mode 100644 drivers/pci/pwrseq/Makefile > create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c > create mode 100644 drivers/pci/pwrseq/pwrseq.c > create mode 100644 include/linux/pci-pwrseq.h > > -- > 2.40.1 > >
On Wed, Jan 17, 2024 at 10:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > The responses to the RFC were rather positive so here's a proper series. Thanks for tackling this. > 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 DT maintainers won't like adding any "fake" nodes not representing > actual devices, we decided to reuse existing PCI infrastructure. Thank you. :) > 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. Suspend/resume has been brought up already, but I disagree we can worry about that later unless there is and always will be no power sequencing during suspend/resume for all devices ever. Given the supplies aren't standard, it wouldn't surprise me if standard PCI power management isn't either. The primary issue I see with this design is we will end up with 2 drivers doing the same power sequencing: the platform driver for initial power on and the device's PCI driver for suspend/resume. Rob
On Thu, 18 Jan 2024 15:29:01 +0100, Rob Herring <robh+dt@kernel.org> said: > On Wed, Jan 17, 2024 at 10:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > [snip] > >> 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. > > Suspend/resume has been brought up already, but I disagree we can > worry about that later unless there is and always will be no power > sequencing during suspend/resume for all devices ever. Given the > supplies aren't standard, it wouldn't surprise me if standard PCI > power management isn't either. The primary issue I see with this > design is we will end up with 2 drivers doing the same power > sequencing: the platform driver for initial power on and the device's > PCI driver for suspend/resume. > > Rob > I admit that I don't have any HW where I could test it but I my thinking was that with the following relationships between the devices: ┌─────────────────────┐ │ │ │ PCI Port device │ │ │ └───┬───────────┬─────┘ │ │ │ │ │ │ ┌─────────────────────▼─────┐ │ │ │ │ │ QCA6390 pwrseq device │ │ │ │ │ └─────────────────────┬─────┘ │ │ │ │ │ │ │ ┌─────▼───────────▼───┐ │ │ │ ath11k_pci device │ │ │ └─────────────────────┘ the PM subsystem would handle the dependencies automatically and correctly setup the sequence for suspend and resume. Also: the PCI ath11k driver does not deal with the kind of resources that the power sequencing platform driver handles: regulators, GPIOs and clocks. I agree, it would be useful to have a working case of handling suspend/resume with this code though. Bartosz
On Thu, Jan 18, 2024 at 08:29:01AM -0600, Rob Herring wrote: > On Wed, Jan 17, 2024 at 10:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > The responses to the RFC were rather positive so here's a proper series. > > Thanks for tackling this. > > > 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 DT maintainers won't like adding any "fake" nodes not representing > > actual devices, we decided to reuse existing PCI infrastructure. > > Thank you. :) > > > 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. > > Suspend/resume has been brought up already, but I disagree we can > worry about that later unless there is and always will be no power > sequencing during suspend/resume for all devices ever. Given the > supplies aren't standard, it wouldn't surprise me if standard PCI > power management isn't either. The primary issue I see with this > design is we will end up with 2 drivers doing the same power > sequencing: the platform driver for initial power on and the device's > PCI driver for suspend/resume. > There are actually 3 drivers need to do their own power management operations: 1. PCIe device driver - Handle the PM of the device itself (shutdown, low power) 2. PCIe pwrseq driver (this one) - Control resources of the PCIe devices 3. PCIe controller driver - Control resources of PCIe controller and Link And all of them has different responsibilities, so I do not see an issue with that. But what is really important is that all 3 has to work in sync and that's quite involved. That's why I thought of dealing with that later. Moreover, even if driver (2) doesn't do any PM operations now, it won't break anything on the currently supported platforms (Qcom). It will be a problem once people start adding pwrseq drivers for platforms whose controller drivers are already handling the job which is now offloaded by this driver. - Mani > Rob >
On Wed, 17 Jan 2024 at 20:16, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 17 Jan 2024 at 18:08, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > The responses to the RFC were rather positive so here's a proper series. > > > > 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 DT maintainers won't like adding any "fake" nodes not representing > > actual devices, we decided to reuse existing PCI infrastructure. > > > > 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'd still like to see how this can be extended to handle BT power up, > having a single entity driving both of the BT and WiFI. > > The device tree changes behave in exactly the opposite way: they > define regulators for the WiFi device, while the WiFi is not being > powered by these regulators. Both WiFi and BT are powered by the PMU, > which in turn consumes all specified regulators. Some additional justification, why I think that this should be modelled as a single instance instead of two different items. This is from msm-5.10 kernel: ===== CUT HERE ===== /** * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status * @plat_priv: Platform private data structure pointer * * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and * WLAN_EN_GPIO on. This is done to avoid power up issues. * * Return: Status of pinctrl select operation. 0 - Success. */ static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv) ===== CUT HERE ===== Also see the bt_configure_gpios() function in the same kernel. > > > > > Changes since RFC: > > - move the pwrseq functionality out of the port driver and into PCI core > > - add support for WCN7850 to the first pwrseq driver (and update bindings) > > - describe the WLAN modules in sm8550-qrd and sm8650-qrd > > - rework Kconfig options, drop the defconfig changes from the series as > > they are no longer needed > > - drop the dt-binding changes for PCI vendor codes > > - extend the DT bindings for ath11k_pci with strict property checking > > - various minor tweaks and fixes > > > > Bartosz Golaszewski (7): > > arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390 > > PCI: create platform devices for child OF nodes of the port node > > PCI: hold the rescan mutex when scanning for the first time > > PCI/pwrseq: add pwrseq core code > > dt-bindings: wireless: ath11k: describe QCA6390 > > dt-bindings: wireless: ath11k: describe WCN7850 > > PCI/pwrseq: add a pwrseq driver for QCA6390 > > > > Neil Armstrong (2): > > arm64: dts: qcom: sm8550-qrd: add Wifi nodes > > arm64: dts: qcom: sm8650-qrd: add Wifi nodes > > > > .../net/wireless/qcom,ath11k-pci.yaml | 89 ++++++ > > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 29 ++ > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 + > > arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 37 +++ > > arch/arm64/boot/dts/qcom/sm8550.dtsi | 10 + > > arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 ++ > > arch/arm64/boot/dts/qcom/sm8650.dtsi | 10 + > > drivers/pci/Kconfig | 1 + > > drivers/pci/Makefile | 1 + > > drivers/pci/bus.c | 9 +- > > drivers/pci/probe.c | 2 + > > drivers/pci/pwrseq/Kconfig | 16 ++ > > drivers/pci/pwrseq/Makefile | 4 + > > drivers/pci/pwrseq/pci-pwrseq-qca6390.c | 267 ++++++++++++++++++ > > drivers/pci/pwrseq/pwrseq.c | 82 ++++++ > > drivers/pci/remove.c | 3 +- > > include/linux/pci-pwrseq.h | 24 ++ > > 17 files changed, 621 insertions(+), 2 deletions(-) > > create mode 100644 drivers/pci/pwrseq/Kconfig > > create mode 100644 drivers/pci/pwrseq/Makefile > > create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c > > create mode 100644 drivers/pci/pwrseq/pwrseq.c > > create mode 100644 include/linux/pci-pwrseq.h > > > > -- > > 2.40.1 > > > > > > > -- > With best wishes > Dmitry
On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > [snip] > > > > I'd still like to see how this can be extended to handle BT power up, > > having a single entity driving both of the BT and WiFI. > > > > The device tree changes behave in exactly the opposite way: they > > define regulators for the WiFi device, while the WiFi is not being > > powered by these regulators. Both WiFi and BT are powered by the PMU, > > which in turn consumes all specified regulators. > > Some additional justification, why I think that this should be > modelled as a single instance instead of two different items. > > This is from msm-5.10 kernel: > > > ===== CUT HERE ===== > /** > * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status > * @plat_priv: Platform private data structure pointer > * > * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and > * WLAN_EN_GPIO on. This is done to avoid power up issues. > * > * Return: Status of pinctrl select operation. 0 - Success. > */ > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv) > ===== CUT HERE ===== > > > Also see the bt_configure_gpios() function in the same kernel. > You are talking about a different problem. Unfortunately we're using similar naming here but I don't have a better alternative in mind. We have two separate issues: one is powering-up a PCI device so that it can be detected and the second is dealing with a device that has multiple modules in it which share a power sequence. The two are independent and this series isn't trying to solve the latter. But I am aware of this and so I actually have an idea for a generalized power sequencing framework. Let's call it pwrseq as opposed to pci_pwrseq. Krzysztof is telling me that there cannot be any power sequencing information contained in DT. Also: modelling the PMU in DT would just over complicate stuff for now reason. We'd end up having the PMU node consuming the regulators but it too would need to expose regulators for WLAN and BT or be otherwise referenced by their nodes. So I'm thinking that the DT representation should remain as it is: with separate WLAN and BT nodes consuming resources relevant to their functionality (BT does not need to enable PCIe regulators). Now how to handle the QCA6490 model you brought up? How about pwrseq drivers that would handle the sequence based on compatibles? We'd add a new subsystem at drivers/pwrseq/. Inside there would be: drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an API to "sub-drivers" (in this case: BT serdev driver and the qca6490 power sequencing driver). Now the latter goes: struct pwrseq_desc *pwrseq = pwrseq_get(dev); And the pwrseq subsystem matches the device's compatible against the correct, *shared* sequence. The BT driver can do the same at any time. The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be responsible for dealing with them. In sub-drivers we now do: ret = pwrseq_power_on(pwrseq); or ret = pwrseq_power_off(pwrseq); in the sub-device drivers and no longer interact with each regulator on our own. The pwrseq subsystem is now in charge of adding delays etc. That's only an idea and I haven't done any real work yet but I'm throwing it out there for discussion. Bartosz [snip]
On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > [snip] > > > > > > > I'd still like to see how this can be extended to handle BT power up, > > > having a single entity driving both of the BT and WiFI. > > > > > > The device tree changes behave in exactly the opposite way: they > > > define regulators for the WiFi device, while the WiFi is not being > > > powered by these regulators. Both WiFi and BT are powered by the PMU, > > > which in turn consumes all specified regulators. > > > > Some additional justification, why I think that this should be > > modelled as a single instance instead of two different items. > > > > This is from msm-5.10 kernel: > > > > > > ===== CUT HERE ===== > > /** > > * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status > > * @plat_priv: Platform private data structure pointer > > * > > * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and > > * WLAN_EN_GPIO on. This is done to avoid power up issues. > > * > > * Return: Status of pinctrl select operation. 0 - Success. > > */ > > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv) > > ===== CUT HERE ===== > > > > > > Also see the bt_configure_gpios() function in the same kernel. > > > > You are talking about a different problem. Unfortunately we're using > similar naming here but I don't have a better alternative in mind. > > We have two separate issues: one is powering-up a PCI device so that > it can be detected and the second is dealing with a device that has > multiple modules in it which share a power sequence. The two are > independent and this series isn't trying to solve the latter. I see it from a different angle: a power up of the WiFi+BT chips. This includes devices like wcn3990 (which have platform + serial parts) and qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts). From my point of view, the PCIe-only part was nice for an RFC, but for v1 I have expected to see a final solution that we can reuse for wcn3990. > > But I am aware of this and so I actually have an idea for a > generalized power sequencing framework. Let's call it pwrseq as > opposed to pci_pwrseq. > > Krzysztof is telling me that there cannot be any power sequencing > information contained in DT. Also: modelling the PMU in DT would just > over complicate stuff for now reason. We'd end up having the PMU node > consuming the regulators but it too would need to expose regulators > for WLAN and BT or be otherwise referenced by their nodes. Yes. And it is a correct representation of the device. The WiFi and BT parts are powered up by the outputs from PMU. We happen to have three different pieces (WiFi, BT and PMU) squashed on a single physical device. > > So I'm thinking that the DT representation should remain as it is: > with separate WLAN and BT nodes consuming resources relevant to their > functionality (BT does not need to enable PCIe regulators). Is it so? The QCA6390 docs clearly say that all regulators should be enabled before asserting BT_EN / WLAN_EN. See the powerup timing diagram and the t2 note to that diagram. > Now how to > handle the QCA6490 model you brought up? How about pwrseq drivers that > would handle the sequence based on compatibles? The QCA6490 is also known as WCN6855. So this problem applies to Qualcomm sm8350 / sm8450 platforms. And strictly speaking I don't see any significant difference between QCA6390 and WCN6855. The regulators might be different, but the implementation should be the same. > > We'd add a new subsystem at drivers/pwrseq/. Inside there would be: > drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an > API to "sub-drivers" (in this case: BT serdev driver and the qca6490 > power sequencing driver). Now the latter goes: > > struct pwrseq_desc *pwrseq = pwrseq_get(dev); > > And the pwrseq subsystem matches the device's compatible against the > correct, *shared* sequence. The BT driver can do the same at any time. > The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be > responsible for dealing with them. > > In sub-drivers we now do: > > ret = pwrseq_power_on(pwrseq); > > or > > ret = pwrseq_power_off(pwrseq); > > in the sub-device drivers and no longer interact with each regulator > on our own. The pwrseq subsystem is now in charge of adding delays > etc. > > That's only an idea and I haven't done any real work yet but I'm > throwing it out there for discussion. I've been there and I had implemented it in the same way, but rather having the pwrseq as a primary device in DT and parsing end-devices only as a fallback / compatibility case.
On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> said: > On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> >> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov >> <dmitry.baryshkov@linaro.org> wrote: >> > >> >> [snip] >> >> > > >> > > I'd still like to see how this can be extended to handle BT power up, >> > > having a single entity driving both of the BT and WiFI. >> > > >> > > The device tree changes behave in exactly the opposite way: they >> > > define regulators for the WiFi device, while the WiFi is not being >> > > powered by these regulators. Both WiFi and BT are powered by the PMU, >> > > which in turn consumes all specified regulators. >> > >> > Some additional justification, why I think that this should be >> > modelled as a single instance instead of two different items. >> > >> > This is from msm-5.10 kernel: >> > >> > >> > ===== CUT HERE ===== >> > /** >> > * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status >> > * @plat_priv: Platform private data structure pointer >> > * >> > * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and >> > * WLAN_EN_GPIO on. This is done to avoid power up issues. >> > * >> > * Return: Status of pinctrl select operation. 0 - Success. >> > */ >> > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv) >> > ===== CUT HERE ===== >> > >> > >> > Also see the bt_configure_gpios() function in the same kernel. >> > >> >> You are talking about a different problem. Unfortunately we're using >> similar naming here but I don't have a better alternative in mind. >> >> We have two separate issues: one is powering-up a PCI device so that >> it can be detected and the second is dealing with a device that has >> multiple modules in it which share a power sequence. The two are >> independent and this series isn't trying to solve the latter. > > I see it from a different angle: a power up of the WiFi+BT chips. This > includes devices like wcn3990 (which have platform + serial parts) and > qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts). > > From my point of view, the PCIe-only part was nice for an RFC, but for > v1 I have expected to see a final solution that we can reuse for > wcn3990. > The submodules are represented as independent devices on the DT and I don't think this will change. It's not even possible as they operate on different buses so it's not like we can MFD it with a top-level platform device and two sub-nodes of which one is PCI and another serdev. With that in mind, I'm insisting that there are two separate issues and a generic power sequencing can be built on top of the PCI-specific pwrseq added here. >> >> But I am aware of this and so I actually have an idea for a >> generalized power sequencing framework. Let's call it pwrseq as >> opposed to pci_pwrseq. >> >> Krzysztof is telling me that there cannot be any power sequencing >> information contained in DT. Also: modelling the PMU in DT would just >> over complicate stuff for now reason. We'd end up having the PMU node >> consuming the regulators but it too would need to expose regulators >> for WLAN and BT or be otherwise referenced by their nodes. > > Yes. And it is a correct representation of the device. The WiFi and BT > parts are powered up by the outputs from PMU. We happen to have three > different pieces (WiFi, BT and PMU) squashed on a single physical > device. > Alright, so let's imagine we do model the PMU on the device tree. It would look something like this: qca6390_pmu: pmic@0 { compatible = "qcom,qca6390-pmu"; bt-gpios = <...>; wlan-gpios = <...>; vdd-supply = <&vreg...>; ... regulators-0 { vreg_x: foo { ... }; ... }; }; Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we cannot go: wlan { pwrseq = &qca6390_pmu; }; But it's enough to: wlan { vdd-supply = <&vreg_x>; }; But the pwrseq driver for "qcom,qca6390-pmu" could map BT and WLAN compatibles to the correct power sequence and then the relevant drivers could enable it using pwrseq_power_on(). But that comes back to what I'm doing here: the PCI part for ath11k still needs the platform driver that will trigger the power sequence and that could be the PCI pwrseq driver for which the framework is introduced in this series. As I said: the two are largely orthogonal. >> >> So I'm thinking that the DT representation should remain as it is: >> with separate WLAN and BT nodes consuming resources relevant to their >> functionality (BT does not need to enable PCIe regulators). > > Is it so? The QCA6390 docs clearly say that all regulators should be > enabled before asserting BT_EN / WLAN_EN. See the powerup timing > diagram and the t2 note to that diagram. > Fair enough. >> Now how to >> handle the QCA6490 model you brought up? How about pwrseq drivers that >> would handle the sequence based on compatibles? > > The QCA6490 is also known as WCN6855. So this problem applies to > Qualcomm sm8350 / sm8450 platforms. > > And strictly speaking I don't see any significant difference between > QCA6390 and WCN6855. The regulators might be different, but the > implementation should be the same. > >> >> We'd add a new subsystem at drivers/pwrseq/. Inside there would be: >> drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an >> API to "sub-drivers" (in this case: BT serdev driver and the qca6490 >> power sequencing driver). Now the latter goes: >> >> struct pwrseq_desc *pwrseq = pwrseq_get(dev); >> >> And the pwrseq subsystem matches the device's compatible against the >> correct, *shared* sequence. The BT driver can do the same at any time. >> The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be >> responsible for dealing with them. >> >> In sub-drivers we now do: >> >> ret = pwrseq_power_on(pwrseq); >> >> or >> >> ret = pwrseq_power_off(pwrseq); >> >> in the sub-device drivers and no longer interact with each regulator >> on our own. The pwrseq subsystem is now in charge of adding delays >> etc. >> >> That's only an idea and I haven't done any real work yet but I'm >> throwing it out there for discussion. > > I've been there and I had implemented it in the same way, but rather > having the pwrseq as a primary device in DT and parsing end-devices > only as a fallback / compatibility case. > Would you mind posting an example DT code here? I'm not sure if I understand what "primary device" means in this context. Bartosz > > > -- > With best wishes > Dmitry >
On Fri, 19 Jan 2024 at 15:35, <brgl@bgdev.pl> wrote: > > On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> said: > > On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> > >> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov > >> <dmitry.baryshkov@linaro.org> wrote: > >> > > >> > >> [snip] > >> > >> > > > >> > > I'd still like to see how this can be extended to handle BT power up, > >> > > having a single entity driving both of the BT and WiFI. > >> > > > >> > > The device tree changes behave in exactly the opposite way: they > >> > > define regulators for the WiFi device, while the WiFi is not being > >> > > powered by these regulators. Both WiFi and BT are powered by the PMU, > >> > > which in turn consumes all specified regulators. > >> > > >> > Some additional justification, why I think that this should be > >> > modelled as a single instance instead of two different items. > >> > > >> > This is from msm-5.10 kernel: > >> > > >> > > >> > ===== CUT HERE ===== > >> > /** > >> > * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status > >> > * @plat_priv: Platform private data structure pointer > >> > * > >> > * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and > >> > * WLAN_EN_GPIO on. This is done to avoid power up issues. > >> > * > >> > * Return: Status of pinctrl select operation. 0 - Success. > >> > */ > >> > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv) > >> > ===== CUT HERE ===== > >> > > >> > > >> > Also see the bt_configure_gpios() function in the same kernel. > >> > > >> > >> You are talking about a different problem. Unfortunately we're using > >> similar naming here but I don't have a better alternative in mind. > >> > >> We have two separate issues: one is powering-up a PCI device so that > >> it can be detected and the second is dealing with a device that has > >> multiple modules in it which share a power sequence. The two are > >> independent and this series isn't trying to solve the latter. > > > > I see it from a different angle: a power up of the WiFi+BT chips. This > > includes devices like wcn3990 (which have platform + serial parts) and > > qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts). > > > > From my point of view, the PCIe-only part was nice for an RFC, but for > > v1 I have expected to see a final solution that we can reuse for > > wcn3990. > > > > The submodules are represented as independent devices on the DT and I don't > think this will change. It's not even possible as they operate on different > buses so it's not like we can MFD it with a top-level platform device and two > sub-nodes of which one is PCI and another serdev. With that in mind, I'm > insisting that there are two separate issues and a generic power sequencing > can be built on top of the PCI-specific pwrseq added here. > > >> > >> But I am aware of this and so I actually have an idea for a > >> generalized power sequencing framework. Let's call it pwrseq as > >> opposed to pci_pwrseq. > >> > >> Krzysztof is telling me that there cannot be any power sequencing > >> information contained in DT. Also: modelling the PMU in DT would just > >> over complicate stuff for now reason. We'd end up having the PMU node > >> consuming the regulators but it too would need to expose regulators > >> for WLAN and BT or be otherwise referenced by their nodes. > > > > Yes. And it is a correct representation of the device. The WiFi and BT > > parts are powered up by the outputs from PMU. We happen to have three > > different pieces (WiFi, BT and PMU) squashed on a single physical > > device. > > > > Alright, so let's imagine we do model the PMU on the device tree. It would > look something like this: > > qca6390_pmu: pmic@0 { > compatible = "qcom,qca6390-pmu"; > > bt-gpios = <...>; > wlan-gpios = <...>; > > vdd-supply = <&vreg...>; > ... > > regulators-0 { > vreg_x: foo { > ... > }; > > ... > }; > }; > > Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we > cannot go: > > wlan { > pwrseq = &qca6390_pmu; > }; > > But it's enough to: > > wlan { > vdd-supply = <&vreg_x>; > }; I'm not sure this will fly. This means expecting that regulator framework is reentrant, which I think is not the case. > But the pwrseq driver for "qcom,qca6390-pmu" could map BT and WLAN compatibles > to the correct power sequence and then the relevant drivers could enable it > using pwrseq_power_on(). > > But that comes back to what I'm doing here: the PCI part for ath11k still > needs the platform driver that will trigger the power sequence and that could > be the PCI pwrseq driver for which the framework is introduced in this series. > > As I said: the two are largely orthogonal. I'm fine with that as long as it stays as an RFC. We need to fix both issues before committing qca6390 power up support. > > >> > >> So I'm thinking that the DT representation should remain as it is: > >> with separate WLAN and BT nodes consuming resources relevant to their > >> functionality (BT does not need to enable PCIe regulators). > > > > Is it so? The QCA6390 docs clearly say that all regulators should be > > enabled before asserting BT_EN / WLAN_EN. See the powerup timing > > diagram and the t2 note to that diagram. > > > > Fair enough. > > >> Now how to > >> handle the QCA6490 model you brought up? How about pwrseq drivers that > >> would handle the sequence based on compatibles? > > > > The QCA6490 is also known as WCN6855. So this problem applies to > > Qualcomm sm8350 / sm8450 platforms. > > > > And strictly speaking I don't see any significant difference between > > QCA6390 and WCN6855. The regulators might be different, but the > > implementation should be the same. > > > >> > >> We'd add a new subsystem at drivers/pwrseq/. Inside there would be: > >> drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an > >> API to "sub-drivers" (in this case: BT serdev driver and the qca6490 > >> power sequencing driver). Now the latter goes: > >> > >> struct pwrseq_desc *pwrseq = pwrseq_get(dev); > >> > >> And the pwrseq subsystem matches the device's compatible against the > >> correct, *shared* sequence. The BT driver can do the same at any time. > >> The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be > >> responsible for dealing with them. > >> > >> In sub-drivers we now do: > >> > >> ret = pwrseq_power_on(pwrseq); > >> > >> or > >> > >> ret = pwrseq_power_off(pwrseq); > >> > >> in the sub-device drivers and no longer interact with each regulator > >> on our own. The pwrseq subsystem is now in charge of adding delays > >> etc. > >> > >> That's only an idea and I haven't done any real work yet but I'm > >> throwing it out there for discussion. > > > > I've been there and I had implemented it in the same way, but rather > > having the pwrseq as a primary device in DT and parsing end-devices > > only as a fallback / compatibility case. > > > > Would you mind posting an example DT code here? I'm not sure if I understand > what "primary device" means in this context. qca_pwrseq: qca-pwrseq { compatible = "qcom,qca6390-pwrseq"; #pwrseq-cells = <1>; vddaon-supply = <&vreg_s6a_0p95>; vddpmu-supply = <&vreg_s2f_0p95>; vddrfa1-supply = <&vreg_s2f_0p95>; vddrfa2-supply = <&vreg_s8c_1p3>; vddrfa3-supply = <&vreg_s5a_1p9>; vddpcie1-supply = <&vreg_s8c_1p3>; vddpcie2-supply = <&vreg_s5a_1p9>; vddio-supply = <&vreg_s4a_1p8>; bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; wifi-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; swctrl-gpios = <&tlmm 124 GPIO_ACTIVE_HIGH>; }; &uart6 { status = "okay"; bluetooth { compatible = "qcom,qca6390-bt"; clocks = <&sleep_clk>; bt-pwrseq = <&qca_pwrseq 1>; }; }; See https://lore.kernel.org/linux-arm-msm/20211006035407.1147909-13-dmitry.baryshkov@linaro.org/
On Fri, Jan 19, 2024 at 3:07 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > [snip] > > > > > > > Alright, so let's imagine we do model the PMU on the device tree. It would > > look something like this: > > > > qca6390_pmu: pmic@0 { > > compatible = "qcom,qca6390-pmu"; > > > > bt-gpios = <...>; > > wlan-gpios = <...>; > > > > vdd-supply = <&vreg...>; > > ... > > > > regulators-0 { > > vreg_x: foo { > > ... > > }; > > > > ... > > }; > > }; > > > > Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we > > cannot go: > > > > wlan { > > pwrseq = &qca6390_pmu; > > }; > > > > But it's enough to: > > > > wlan { > > vdd-supply = <&vreg_x>; > > }; > > I'm not sure this will fly. This means expecting that regulator > framework is reentrant, which I think is not the case. > Oh maybe I didn't make myself clear. That's the DT representation of HW. With pwrseq, the BT or ATH11K drivers wouldn't use the regulator framework. They would use the pwrseq framework and it could use the phandle of the regulator to get into the correct pwrseq device without making Rob and Krzysztof angry. Bart [snip]
On Fri, Jan 19, 2024 at 3:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Fri, Jan 19, 2024 at 3:07 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > [snip] > > > > > > > > > > > Alright, so let's imagine we do model the PMU on the device tree. It would > > > look something like this: > > > > > > qca6390_pmu: pmic@0 { > > > compatible = "qcom,qca6390-pmu"; > > > > > > bt-gpios = <...>; > > > wlan-gpios = <...>; > > > > > > vdd-supply = <&vreg...>; > > > ... > > > > > > regulators-0 { > > > vreg_x: foo { > > > ... > > > }; > > > > > > ... > > > }; > > > }; > > > > > > Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we > > > cannot go: > > > > > > wlan { > > > pwrseq = &qca6390_pmu; > > > }; > > > > > > But it's enough to: > > > > > > wlan { > > > vdd-supply = <&vreg_x>; > > > }; > > > > I'm not sure this will fly. This means expecting that regulator > > framework is reentrant, which I think is not the case. > > > > Oh maybe I didn't make myself clear. That's the DT representation of > HW. With pwrseq, the BT or ATH11K drivers wouldn't use the regulator > framework. They would use the pwrseq framework and it could use the > phandle of the regulator to get into the correct pwrseq device without > making Rob and Krzysztof angry. > > Bart > > [snip] I'm working on a PoC of the generic pwrseq framework but without the explicit pwrseq modelling in DT. Should have an RFC ready early next week. Bart
On Fri, Jan 19, 2024 at 12:52:00PM +0100, Bartosz Golaszewski wrote: > We have two separate issues: one is powering-up a PCI device so that > it can be detected Just wondering, I note in really_probe() we configure the pin controller, active the pm_domain etc before probing a driver. Would it make sense for the issue you mention above to similarly amend pci_scan_device() to enable whatever clocks or regulators are described in the devicetree as providers for the given PCI device? Thanks, Lukas
On Fri, Jan 19, 2024 at 10:34 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Fri, Jan 19, 2024 at 12:52:00PM +0100, Bartosz Golaszewski wrote: > > We have two separate issues: one is powering-up a PCI device so that > > it can be detected > > Just wondering, I note in really_probe() we configure the pin controller, > active the pm_domain etc before probing a driver. > > Would it make sense for the issue you mention above to similarly > amend pci_scan_device() to enable whatever clocks or regulators > are described in the devicetree as providers for the given PCI device? If you mean via a callback to some device specific code, then yes, I think that's exactly what should be done here. That's roughly what MDIO does. If firmware says there is a device present, then probe it anyways even if not detected. I don't think that will work for PCI because it accesses a lot of registers before probe. We'd need some sort of pre-probe hook instead called after reading vendor and device ID, but before anything else. If you mean PCI core just enable whatever clocks and regulators (and GPIOs), then no, because what is the correct order and timing for each of those? You don't know because that is device specific information just as how to program a device is. Rob
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> The responses to the RFC were rather positive so here's a proper series. 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 DT maintainers won't like adding any "fake" nodes not representing actual devices, we decided to reuse existing PCI infrastructure. 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. Changes since RFC: - move the pwrseq functionality out of the port driver and into PCI core - add support for WCN7850 to the first pwrseq driver (and update bindings) - describe the WLAN modules in sm8550-qrd and sm8650-qrd - rework Kconfig options, drop the defconfig changes from the series as they are no longer needed - drop the dt-binding changes for PCI vendor codes - extend the DT bindings for ath11k_pci with strict property checking - various minor tweaks and fixes Bartosz Golaszewski (7): arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390 PCI: create platform devices for child OF nodes of the port node PCI: hold the rescan mutex when scanning for the first time PCI/pwrseq: add pwrseq core code dt-bindings: wireless: ath11k: describe QCA6390 dt-bindings: wireless: ath11k: describe WCN7850 PCI/pwrseq: add a pwrseq driver for QCA6390 Neil Armstrong (2): arm64: dts: qcom: sm8550-qrd: add Wifi nodes arm64: dts: qcom: sm8650-qrd: add Wifi nodes .../net/wireless/qcom,ath11k-pci.yaml | 89 ++++++ arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 29 ++ arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 + arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 37 +++ arch/arm64/boot/dts/qcom/sm8550.dtsi | 10 + arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 ++ arch/arm64/boot/dts/qcom/sm8650.dtsi | 10 + drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/probe.c | 2 + drivers/pci/pwrseq/Kconfig | 16 ++ drivers/pci/pwrseq/Makefile | 4 + drivers/pci/pwrseq/pci-pwrseq-qca6390.c | 267 ++++++++++++++++++ drivers/pci/pwrseq/pwrseq.c | 82 ++++++ drivers/pci/remove.c | 3 +- include/linux/pci-pwrseq.h | 24 ++ 17 files changed, 621 insertions(+), 2 deletions(-) create mode 100644 drivers/pci/pwrseq/Kconfig create mode 100644 drivers/pci/pwrseq/Makefile create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c create mode 100644 drivers/pci/pwrseq/pwrseq.c create mode 100644 include/linux/pci-pwrseq.h