power: sequencing: implement the subsystem and add first users
Subsystems (14)
v5 cover power: sequencing: implement the subsystem and add first users
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:
https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):
https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)
https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a __free() helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.
Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
For QCA6390 on RB5 we add the PMU node as a platform device. It consumes regulators and GPIOs from the host and exposed regulators consumer in turn by the BT and WLAN modules. This represents the internal structure of the package.
Patches 10/18-14/18:
These contain the bulk of the PCI changes for this series. We introduce a simple framework for powering up PCI devices before detecting them on the bus and the first user of this library in the form of the WCN7850 PCI power control driver.
The general approach is as follows: PCI devices that need special treatment before they can be powered up, scanned and bound to their PCI drivers must be described on the device-tree as child nodes of the PCI port node. These devices will be instantiated on the platform bus. They will in fact be generic platform devices with the compatible of the form used for PCI devices already upstream ("pci<vendor ID>,<device ID">). We add a new directory under drivers/pci/pwrctl/ that contains PCI pwrctl drivers. These drivers are platform drivers that will now be matched against the devices instantiated from port children just like any other platform pairs.
Both the power control platform device *AND* the associated PCI device reuse the same OF node and have access to the same properties. The goal of the platform driver is to request and bring up any required resources and let the pwrctl framework know that it's now OK to rescan the bus and detect the devices. When the device is bound, we are notified about it by the PCI bus notifier event and can establish a device link between the power control device and the PCI device so that any future extension for power-management will already be able to work with the correct hierachy.
The reusing of the OF node is the reason for the small changes to the PCI OF core: as the bootloader can possibly leave the relevant regulators on before booting linux, the PCI device can be detected before its platform abstraction is probed. In this case, we find that device first and mark its OF node as reused. The pwrctl framework handles the opposite case (when the PCI device is detected only after the platform driver successfully enabled it).
Patches 15/18-16/18:
These add a relatively simple power sequencing subsystem and the first driver using it: the pwrseq module for the QCA6390 PMU.
For the record: Bjorn suggested a different solution: a regulator driver that would - based on which regulators are enabled by a consumer - enable relevant resources (drive the enable GPIOs) while respecting the HW-specific delays. This would however require significant and yet unprecised changed to the regulator subsystem as well as be an abuse of the regulator provider API akin to using the reset framework for power sequencing as proposed before.
Instead I'm proposing to add a subsystem that allows different devices to use a shared power sequence split into consumer-specific as well as common "units".
A power sequence provider driver registers a set of units with pwrseq core. Each unit can be enabled and disabled and contains an optional list of other units which must be enabled before it itself can be. A unit represents a discreet chunk of the power sequence.
It also registers a list of targets: a target is an abstraction wrapping a unit which allows consumers to tell pwrseq which unit they want to reach. Real-life example is the driver we're adding here: there's a set of common regulators, two PCIe-specific ones and two enable GPIOs: one for Bluetooth and one for WLAN.
The Bluetooth driver requests a descriptor to the power sequencer and names the target it wants to reach:
pwrseq = devm_pwrseq_get(dev, "bluetooth");
The pwrseq core then knows that when the driver calls:
pwrseq_power_on(pwrseq);
It must enable the "bluetooth-enable" unit but it depends on the "regulators-common" unit so this one is enabled first. The provider driver is also in charge of assuring an appropriate delay between enabling the BT and WLAN enable GPIOs. The WLAN-specific resources are handled by the "wlan-enable" unit and so are not enabled until the WLAN driver requests the "wlan" target to be powered on.
Another thing worth discussing is the way we associate the consumer with the relevant power sequencer. DT maintainers have expressed a discontent with the existing mmc pwrseq bindings and have NAKed an earlier initiative to introduce global pwrseq bindings to the kernel[1].
In this approach, we model the existing regulators and GPIOs in DT but the pwrseq subsystem requires each provider to provide a .match() callback. Whenever a consumer requests a power sequencer handle, we iterate over the list of pwrseq drivers and call .match() for each. It's up to the driver to verify in a platform-specific way whether it deals with its consumer and let the core pwrseq code know.
The advantage of this over reusing the regulator or reset subsystem is that it's more generalized and can handle resources of all kinds as well as deal with any kind of power-on sequences: for instance, Qualcomm has a PCI switch they want a driver for but this switch requires enabling some resources first (PCI pwrctl) and then configuring the device over I2C (which can be handled by the pwrseq provider).
Patch 17/18:
This patch makes the Qualcomm Bluetooth driver get and use the power sequencer for QCA6390.
Patch 18/18:
While tiny, this patch is possibly the highlight of the entire series. It uses the two abstraction layers we introduced before to create an elegant power sequencing PCI power control driver and supports the ath11k module on QCA6390.
With this series we can now enable BT and WLAN on several new Qualcomm boards upstream.
I tested the series on RB5 while Neil tested it on sm8650-qrd and sm8550-qrd.
Best Regards,
Bartosz Golaszewski
It's hard to list the changes between versions here as the scope changed significantly between each iteration and some versions were not even full series but rather RFCs for parts of the solution. For this reason, I'll only start listing changes starting from v6.
[*] This is what the docs say. In practice it seems that this delay can be ignored. However the subsequent model - QCA6490 - *does* require users to respect it, so the problem remains valid.
[1] https://lore.kernel.org/netdev/[email protected]/
Bartosz Golaszewski (15):
arm64: defconfig: enable ath12k as a module
dt-bindings: regulator: describe the PMU module of the QCA6390 package
dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
dt-bindings: new: wireless: qcom,ath11k: describe the ath11k on
QCA6390
dt-bindings: new: wireless: describe the ath12k PCI module
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: reuse the OF node for power controlled devices PCI/pwrctl: create platform devices for child OF nodes of the port
node
PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a power control driver for WCN7850
power: sequencing: implement the pwrseq core
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390 PCI/pwrctl: add a PCI power control driver for power sequenced devices
Jonathan Cameron (1):
of: Add cleanup.h based auto release via __free(device_node) markings.
Neil Armstrong (2):
arm64: dts: qcom: sm8550-qrd: add the Wifi node
arm64: dts: qcom: sm8650-qrd: add the Wifi node
.../net/bluetooth/qualcomm-bluetooth.yaml | 17 + .../net/wireless/qcom,ath11k-pci.yaml | 28 + .../net/wireless/qcom,ath12k-pci.yaml | 103 ++ .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 +++ MAINTAINERS | 8 + arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 123 +- 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 + arch/arm64/configs/defconfig | 1 + drivers/bluetooth/hci_qca.c | 31 + drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/of.c | 14 +- drivers/pci/probe.c | 2 + drivers/pci/pwrctl/Kconfig | 25 + drivers/pci/pwrctl/Makefile | 7 + drivers/pci/pwrctl/core.c | 136 +++ drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 84 ++ drivers/pci/pwrctl/pci-pwrctl-wcn7850.c | 202 ++++ drivers/pci/remove.c | 2 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 28 + drivers/power/sequencing/Makefile | 6 + drivers/power/sequencing/core.c | 1065 +++++++++++++++++ drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++ include/linux/of.h | 2 + include/linux/pci-pwrctl.h | 51 + include/linux/pwrseq/consumer.h | 56 + include/linux/pwrseq/provider.h | 75 ++ 34 files changed, 2678 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml create mode 100644 drivers/pci/pwrctl/Kconfig create mode 100644 drivers/pci/pwrctl/Makefile create mode 100644 drivers/pci/pwrctl/core.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-wcn7850.c create mode 100644 drivers/power/sequencing/Kconfig create mode 100644 drivers/power/sequencing/Makefile create mode 100644 drivers/power/sequencing/core.c create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c create mode 100644 include/linux/pci-pwrctl.h create mode 100644 include/linux/pwrseq/consumer.h create mode 100644 include/linux/pwrseq/provider.h
Signature · 1 line
2.40.1
PATCH v5 1/18 of: Add cleanup.h based auto release via __free(device_node) markings.
From: Jonathan Cameron <[email protected]>
The recent addition of scope based cleanup support to the kernel provides a convenient tool to reduce the chances of leaking reference counts where of_node_put() should have been called in an error path.
This enables
struct device_node *child __free(device_node) = NULL;
for_each_child_of_node(np, child) {
if (test)
return test;
}with no need for a manual call of of_node_put().
A following patch will reduce the scope of the child variable to the for loop, to avoid an issues with ordering of autocleanup, and make it obvious when this assigned a non NULL value.
In this simple example the gains are small but there are some very complex error handling cases buried in these loops that will be greatly simplified by enabling early returns with out the need for this manual of_node_put() call.
Note that there are coccinelle checks in scripts/coccinelle/iterators/for_each_child.cocci to detect a failure to call of_node_put(). This new approach does not cause false positives. Longer term we may want to add scripting to check this new approach is done correctly with no double of_node_put() calls being introduced due to the auto cleanup. It may also be useful to script finding places this new approach is useful.
--- include/linux/of.h | 2 ++ 1 file changed, 2 insertions(+)
include/linux/of.h +2 -0
@@ -13,6 +13,7 @@*/ #include <linux/types.h> #include <linux/bitops.h> +#include <linux/cleanup.h> #include <linux/errno.h> #include <linux/kobject.h> #include <linux/mod_devicetable.h>
@@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node) }static inline void of_node_put (struct device_node *node) { }#endif /* !CONFIG_OF_DYNAMIC */ +DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T)) /* Pointer for first entry in chain of all nodes. */extern struct device_node *of_root;
Signature · 1 line
2.40.1
This is an automated email and please do not reply to this email.
Dear Submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.
----- Output -----
error: patch failed: arch/arm64/boot/dts/qcom/sm8650-qrd.dts:1139
error: arch/arm64/boot/dts/qcom/sm8650-qrd.dts: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Please resolve the issue and submit the patches again.
---
Regards,
Linux Bluetooth
PATCH v5 2/18 arm64: defconfig: enable ath12k as a module
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Build the ath12k driver as a module for arm64 default config.
--- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+)
arch/arm64/configs/defconfig +1 -0
@@ -409,6 +409,7 @@ CONFIG_WCN36XX=m CONFIG_ATH11K=m CONFIG_ATH11K_AHB=m CONFIG_ATH11K_PCI=m +CONFIG_ATH12K=m CONFIG_BRCMFMAC=m CONFIG_MWIFIEX=m CONFIG_MWIFIEX_SDIO=m
Signature · 1 line
2.40.1
Re: [PATCH v5 02/18] arm64: defconfig: enable ath12k as a module
Quoted 3 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Build the ath12k driver as a module for arm64 default config.
This we see from the diff. Please tell us "why", e.g. "Board foo with Qualcomm baz uses it."
Also this should not be in these series. It only makes the power-sequencing patchset bigger, without any real reason.
Best regards,
Krzysztof
Re: [PATCH v5 02/18] arm64: defconfig: enable ath12k as a module
Quoted 9 lines · Krzysztof Kozlowski
Quoted 3 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Build the ath12k driver as a module for arm64 default config.
This we see from the diff. Please tell us "why", e.g. "Board foo with Qualcomm baz uses it."
Quoted 7 lines
Also this should not be in these series. It only makes the power-sequencing patchset bigger, without any real reason.
Best regards,
Krzysztof
Got it, I will resend it separately.
Bart
PATCH v5 3/18 dt-bindings: regulator: describe the PMU module of the QCA6390 package
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
--- .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml
Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml new file +166 -0
@@ -0,0 +1,166 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/regulator/qcom,qca6390-pmu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies, Inc. QCA6390 PMU Regulators + +maintainers: + - Bartosz Golaszewski <[email protected]> + +description: + The QCA6390 package contains discreet modules for WLAN and Bluetooth. They + are powered by the Power Management Unit (PMU) that takes inputs from the + host and provides LDO outputs. This document describes this module. + +properties: + compatible: + const: qcom,qca6390-pmu + + vddaon-supply: + description: VDD_AON supply regulator handle + + vddpmu-supply: + description: VDD_PMU supply regulator handle + + vddrfa0p95-supply: + description: VDD_RFA_0P95 supply regulator handle + + vddrfa1p3-supply: + description: VDD_RFA_1P3 supply regulator handle + + vddrfa1p9-supply: + description: VDD_RFA_1P9 supply regulator handle + + vddpcie1p3-supply: + description: VDD_PCIE_1P3 supply regulator handle<S-Del> + + vddpcie1p9-supply: + description: VDD_PCIE_1P9 supply regulator handle + + vddio-supply: + description: VDD_IO supply regulator handle + + wlan-enable-gpios: + maxItems: 1 + description: + GPIO line enabling the ATH11K WLAN module supplied by the PMU + + bt-enable-gpios: + maxItems: 1 + description: + GPIO line enabling the ATH11K Bluetooth module supplied by the PMU + + regulators: + type: object + description: + LDO outputs of the PMU + + patternProperties: + "^ldo[0-9]$": + $ref: regulator.yaml# + type: object + unevaluatedProperties: false + + additionalProperties: false + +required: + - compatible + - vddaon-supply + - vddpmu-supply + - vddrfa0p95-supply + - vddrfa1p3-supply + - vddrfa1p9-supply + - vddpcie1p3-supply + - vddpcie1p9-supply + - vddio-supply + - wlan-enable-gpios + - bt-enable-gpios + - regulators + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + pmu { + compatible = "qcom,qca6390-pmu"; + + pinctrl-names = "default"; + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; + + vddaon-supply = <&vreg_s6a_0p95>; + vddpmu-supply = <&vreg_s2f_0p95>; + vddrfa0p95-supply = <&vreg_s2f_0p95>; + vddrfa1p3-supply = <&vreg_s8c_1p3>; + vddrfa1p9-supply = <&vreg_s5a_1p9>; + vddpcie1p3-supply = <&vreg_s8c_1p3>; + vddpcie1p9-supply = <&vreg_s5a_1p9>; + vddio-supply = <&vreg_s4a_1p8>; + + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; + + regulators { + vreg_pmu_rfa_cmn: ldo0 { + regulator-name = "vreg_pmu_rfa_cmn"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_wlcx_0p8: ldo2 { + regulator-name = "vreg_pmu_wlcx_0p8"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_wlmx_0p85: ldo3 { + regulator-name = "vreg_pmu_wlmx_0p85"; + regulator-min-microvolt = <810000>; + regulator-max-microvolt = <890000>; + }; + + vreg_pmu_btcmx_0p85: ldo4 { + regulator-name = "vreg_pmu_btcmx_0p85"; + regulator-min-microvolt = <810000>; + regulator-max-microvolt = <890000>; + }; + + vreg_pmu_rfa_0p8: ldo5 { + regulator-name = "vreg_pmu_rfa_0p8"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_rfa_1p2: ldo6 { + regulator-name = "vreg_pmu_rfa_1p2"; + regulator-min-microvolt = <1187000>; + regulator-max-microvolt = <1313000>; + }; + + vreg_pmu_rfa_1p7: ldo7 { + regulator-name = "vreg_pmu_rfa_1p7"; + regulator-min-microvolt = <1710000>; + regulator-max-microvolt = <1890000>; + }; + + vreg_pmu_pcie_0p9: ldo8 { + regulator-name = "vreg_pmu_pcie_0p9"; + regulator-min-microvolt = <870000>; + regulator-max-microvolt = <970000>; + }; + + vreg_pmu_pcie_1p8: ldo9 { + regulator-name = "vreg_pmu_pcie_1p8"; + regulator-min-microvolt = <1710000>; + regulator-max-microvolt = <1890000>; + }; + }; + };
Signature · 1 line
2.40.1
Mark Brown
maintainerRe: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Re: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
Quoted 8 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
Quoted 5 lines
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Mark,
This is quite vague, could you elaborate? I have no idea what is wrong with this patch.
Bartosz
Mark Brown
maintainerRe: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
Quoted 10 lines · Bartosz Golaszewski
Quoted 5 lines · Mark Brown
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
This is quite vague, could you elaborate? I have no idea what is wrong with this patch.
The subject line does not look like the subject line for a regulator patch rendering it almost invisible in my inbox. As you will see if you follow the above suggestion and look at other commits to the same area you should see that subject lines should start regulator:.
Re: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
In reply to Bartosz GolaszewskiQuoted 15 lines · Bartosz Golaszewski
Quoted 8 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
Quoted 5 lines
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Quoted 4 lines
Mark,
This is quite vague, could you elaborate? I have no idea what is wrong with this patch.
Use subject prefixes matching the subsystem. You can get them for example with git log --oneline -- DIRECTORY_OR_FILE on the directory your patch is touching.
Best regards,
Krzysztof
Re: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
Quoted 22 lines · Krzysztof Kozlowski
Quoted 15 lines · Bartosz Golaszewski
Quoted 8 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
Quoted 5 lines
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Quoted 4 lines
Mark,
This is quite vague, could you elaborate? I have no idea what is wrong with this patch.
Quoted 7 lines
Use subject prefixes matching the subsystem. You can get them for example with
git log --oneline -- DIRECTORY_OR_FILEon the directory your patch is touching.Best regards,
Krzysztof
Yes, I always do. And for Documentation/devicetree/bindings/regulator/ the subjects are split 50:50 between "dt-bindings: regulator: ..." and "regulator: dt-bindings: ...". For Documentation/devicetree/bindings/ it's overwhelmingly "dt-bindings: <subsystem>: ...". It's the first time someone wants me to send a DT bindings patch without "dt-bindings" coming first in the subject.
I mean: I can do it alright but it's not stated anywhere explicitly.
Bartosz
Re: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
Quoted 28 lines · Bartosz Golaszewski
Quoted 26 lines · Krzysztof Kozlowski
Quoted 15 lines · Bartosz Golaszewski
Quoted 8 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
Quoted 5 lines
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Quoted 4 lines
Mark,
This is quite vague, could you elaborate? I have no idea what is wrong with this patch.
Use subject prefixes matching the subsystem. You can get them for example with
git log --oneline -- DIRECTORY_OR_FILEon the directory your patch is touching.
Quoted 6 lines
Quoted 3 lines
Best regards,
KrzysztofYes, I always do. And for
Documentation/devicetree/bindings/regulator/the subjects are split 50:50 between "dt-bindings: regulator: ..." and
No, there are only ~54 "dt + regulator" ones and around 400 starting with "regulator" (Mark removes first prefix if it is not regulator:). So 50 to 400 is not 50:50.
Quoted 6 lines
"regulator: dt-bindings: ...". For
Documentation/devicetree/bindings/it's overwhelmingly "dt-bindings: <subsystem>: ...". It's the first time someone wants me to send a DT bindings patch without "dt-bindings" coming first in the subject.I mean: I can do it alright but it's not stated anywhere explicitly.
Well, practice was kind of known and obvious, but it is also stated:
Best regards,
Krzysztof
Re: [PATCH v5 03/18] dt-bindings: regulator: describe the PMU module of the QCA6390 package
In reply to Bartosz GolaszewskiQuoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The QCA6390 package contains discreet modules for WLAN and Bluetooth. They are powered by the Power Management Unit (PMU) that takes inputs from the host and provides LDO outputs. This document describes this module.
Quoted 6 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml
This matches our internal discussions and how we see the QCA6390 power handling based on datasheet.
Best regards,
Krzysztof
PATCH v5 4/18 dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
QCA6390 has a compatible listed in the bindings but is missing the regulators description. Add the missing supply property and list the required ones in the allOf section.
--- .../net/bluetooth/qualcomm-bluetooth.yaml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml +17 -0
@@ -62,6 +62,9 @@ properties: vdddig-supply: description: VDD_DIG supply regulator handle + vddbtcmx-supply: + description: VDD_BT_CMX supply regulator handle + vddbtcxmx-supply: description: VDD_BT_CXMX supply regulator handle
@@ -180,6 +183,20 @@ allOf: - vddrfa0p8-supply - vddrfa1p2-supply - vddrfa1p9-supply + - if: + properties: + compatible: + contains: + enum: + - qcom,qca6390-bt + then: + required: + - vddrfacmn-supply + - vddaon-supply + - vddbtcmx-supply + - vddrfa0p8-supply + - vddrfa1p2-supply + - vddrfa1p7-supply examples: - |
Signature · 1 line
2.40.1
Re: [PATCH v5 04/18] dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
QCA6390 has a compatible listed in the bindings but is missing the regulators description. Add the missing supply property and list the required ones in the allOf section.
Signed-off-by Bartosz Golaszewski <[email protected]>---
Best regards,
Krzysztof
PATCH v5 5/18 dt-bindings: new: wireless: qcom,ath11k: describe the ath11k on QCA6390
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a PCI compatible for the ATH11K module on QCA6390 and describe the power inputs from the PMU that it consumes.
--- .../net/wireless/qcom,ath11k-pci.yaml | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+)
Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml +28 -0
@@ -16,6 +16,7 @@ description: | properties: compatible: enum: + - pci17cb,1101 # QCA6390 - pci17cb,1103 # WCN6855 reg:
@@ -27,6 +28,33 @@ properties: string to uniquely identify variant of the calibration data for designs with colliding bus and device ids + vddrfacmn-supply: + description: VDD_RFA_CMN supply regulator handle + + vddaon-supply: + description: VDD_AON supply regulator handle + + vddwlcx-supply: + description: VDD_WL_CX supply regulator handle + + vddwlmx-supply: + description: VDD_WL_MX supply regulator handle + + vddrfa0p8-supply: + description: VDD_RFA_0P8 supply regulator handle + + vddrfa1p2-supply: + description: VDD_RFA_1P2 supply regulator handle + + vddrfa1p7-supply: + description: VDD_RFA_1P7 supply regulator handle + + vddpcie0p9-supply: + description: VDD_PCIE_0P9 supply regulator handle + + vddpcie1p8-supply: + description: VDD_PCIE_1P8 supply regulator handle + required: - compatible - reg
Signature · 1 line
2.40.1
PATCH v5 6/18 dt-bindings: new: wireless: describe the ath12k PCI module
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add device-tree bindings for the ATH12K module found in the WCN7850 package.
--- .../net/wireless/qcom,ath12k-pci.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +103 -0
@@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2024 Linaro Limited +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/wireless/qcom,ath12k-pci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies ath12k wireless devices (PCIe) + +maintainers: + - Bartosz Golaszewski <[email protected]> + +description: | + Qualcomm Technologies IEEE 802.11ax PCIe devices + +properties: + compatible: + enum: + - pci17cb,1107 # WCN7850 + + reg: + maxItems: 1 + + enable-gpios: + maxItems: 1 + description: GPIO line enabling the WLAN module + + vdd-supply: + description: VDD supply regulator handle + + vddio-supply: + description: VDD_IO supply regulator handle + + vddio1p2-supply: + description: VDD_IO_1P2 supply regulator handle + + vddaon-supply: + description: VDD_AON supply regulator handle + + vdddig-supply: + description: VDD_DIG supply regulator handle + + vddrfa1p2-supply: + description: VDD_RFA_1P2 supply regulator handle + + vddrfa1p8-supply: + description: VDD_PCIE_1P8 supply regulator handle + + clocks: + maxItems: 1 + description: + Reference clock phandle + +required: + - compatible + - reg + - vdd-supply + - vddio-supply + - vddaon-supply + - vdddig-supply + - vddrfa1p2-supply + - vddrfa1p8-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,rpmh.h> + #include <dt-bindings/gpio/gpio.h> + pcie { + #address-cells = <3>; + #size-cells = <2>; + + pcie@0 { + device_type = "pci"; + reg = <0x0 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; + + bus-range = <0x01 0xff>; + + wifi@0 { + compatible = "pci17cb,1107"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + pinctrl-names = "default"; + pinctrl-0 = <&wlan_en>; + + enable-gpios = <&tlmm 16 GPIO_ACTIVE_HIGH>; + + vdd-supply = <&vreg_s4i_0p85>; + vddio-supply = <&vreg_l15b_1p8>; + vddio1p2-supply = <&vreg_l3c_1p2>; + vddaon-supply = <&vreg_s2c_0p8>; + vdddig-supply = <&vreg_s3c_0p9>; + vddrfa1p2-supply = <&vreg_s1c_1p2>; + vddrfa1p8-supply = <&vreg_s6c_1p8>; + + clocks = <&rpmhcc RPMH_RF_CLK1>; + }; + }; + };
Signature · 1 line
2.40.1
Re: [PATCH v5 06/18] dt-bindings: new: wireless: describe the ath12k PCI module
Quoted 18 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Add device-tree bindings for the ATH12K module found in the WCN7850 package.
Signed-off-by Bartosz Golaszewski <[email protected]>--- .../net/wireless/qcom,ath12k-pci.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yamlDocumentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +1
-0@@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml line 11
Quoted 10 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +10
-0+# Copyright (c) 2024 Linaro Limited +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/wireless/qcom,ath12k-pci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies ath12k wireless devices (PCIe) + +maintainers: + - Bartosz Golaszewski <[email protected]>
Jeff and me are the ath12k driver maintainers so shouldn't we listed here as well?
Jeff, this reminds me that we should add you to qcom,ath10k.yaml, qcom,ath11k-pci.yaml and qcom,ath11k.yaml as maintainer.
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v5 06/18] dt-bindings: new: wireless: describe the ath12k PCI module
Quoted 26 lines · Kalle Valo
Quoted 23 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Add device-tree bindings for the ATH12K module found in the WCN7850 package.
Signed-off-by Bartosz Golaszewski <[email protected]>--- .../net/wireless/qcom,ath12k-pci.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yamlDocumentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +6
-0@@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2024 Linaro Limited +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/wireless/qcom,ath12k-pci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
Quoted 9 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml line 11
Quoted 5 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +5
-0+ +title: Qualcomm Technologies ath12k wireless devices (PCIe) + +maintainers: + - Bartosz Golaszewski <[email protected]>Jeff and me are the ath12k driver maintainers so shouldn't we listed here as well?
Sure will do. I also noticed the subject is wrong, should have been "net" not "new".
Also, Jeff is not showing up for ath12k bindings in get_maintainer.pl. You could consider adding an N: ath12k entry to MAINTAINERS.
Bartosz
Quoted 7 lines
Jeff, this reminds me that we should add you to qcom,ath10k.yaml, qcom,ath11k-pci.yaml and qcom,ath11k.yaml as maintainer.
--
https://patchwork.kernel.org/project/linux-wireless/list/https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v5 06/18] dt-bindings: new: wireless: describe the ath12k PCI module
Quoted 37 lines · Bartosz Golaszewski
Quoted 26 lines · Kalle Valo
Quoted 23 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Add device-tree bindings for the ATH12K module found in the WCN7850 package.
Signed-off-by Bartosz Golaszewski <[email protected]>--- .../net/wireless/qcom,ath12k-pci.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yamlDocumentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +6
-0@@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2024 Linaro Limited +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/wireless/qcom,ath12k-pci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#Quoted 9 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml line 11
Quoted 5 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +5
-0+ +title: Qualcomm Technologies ath12k wireless devices (PCIe) + +maintainers: + - Bartosz Golaszewski <[email protected]>Jeff and me are the ath12k driver maintainers so shouldn't we listed here as well?
Quoted 10 lines
Sure will do. I also noticed the subject is wrong, should have been "net" not "new".
Also, Jeff is not showing up for ath12k bindings in get_maintainer.pl. You could consider adding an N: ath12k entry to
MAINTAINERS.Bartosz
Quoted 2 lines
Jeff, this reminds me that we should add you to qcom,ath10k.yaml, qcom,ath11k-pci.yaml and qcom,ath11k.yaml as maintainer.
OK, I'll update those files.
And looking at MAINTAINERS, in ath10 we have an explicit path:
F: Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
And in ath11k we have an explicit path:
F: Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
However the ath11k entry is missing:Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
So it looks like I have a few patches:
- update my entries in MAINTAINERS to use my @kernel.org e-mail
- add myself to the existing *ath1*k*.yaml files as a maintainer
- update MAINTAINERS to replace F: *.yaml with N: ath1*k to match any related files, including the YAML files
/jeff
Re: [PATCH v5 06/18] dt-bindings: new: wireless: describe the ath12k PCI module
In reply to Bartosz GolaszewskiDocumentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml
Quoted 16 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Add device-tree bindings for the ATH12K module found in the WCN7850 package.
Signed-off-by Bartosz Golaszewski <[email protected]>--- .../net/wireless/qcom,ath12k-pci.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yamlDocumentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +0
-0
This means there can be ath12k over AHB or some other bus, which would be quite different and not fit together in one schema file. It's that the case?
Quoted 4 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +3
-0@@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2024 Linaro Limited +%YAML 1.2
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +10
-0+--- +$id: http://devicetree.org/schemas/net/wireless/qcom,ath12k-pci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies ath12k wireless devices (PCIe) + +maintainers: + - Bartosz Golaszewski <[email protected]> + +description: |
Do not need '|' unless you need to preserve formatting.
Quoted 29 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +29
-0+ Qualcomm Technologies IEEE 802.11ax PCIe devices + +properties: + compatible: + enum: + - pci17cb,1107 # WCN7850 + + reg: + maxItems: 1 + + enable-gpios: + maxItems: 1 + description: GPIO line enabling the WLAN module + + vdd-supply: + description: VDD supply regulator handle + + vddio-supply: + description: VDD_IO supply regulator handle + + vddio1p2-supply: + description: VDD_IO_1P2 supply regulator handle + + vddaon-supply: + description: VDD_AON supply regulator handle + + vdddig-supply: + description: VDD_DIG supply regulator handle +
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml line 52
Quoted 10 lines
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml new file +10
-0+ vddrfa1p2-supply: + description: VDD_RFA_1P2 supply regulator handle + + vddrfa1p8-supply: + description: VDD_PCIE_1P8 supply regulator handle + + clocks: + maxItems: 1 + description: + Reference clock phandle
These three could be just:
items:
- description: Reference clock
(phandle is redundant in this context)
Best regards,
Krzysztof
Rob Herring
maintainerRe: [PATCH v5 06/18] dt-bindings: new: wireless: describe the ath12k PCI module
In reply to Bartosz GolaszewskiQuoted 1 line · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
s/new/net/ in the subject.
Quoted 9 lines
Add device-tree bindings for the ATH12K module found in the WCN7850 package.
Signed-off-by Bartosz Golaszewski <[email protected]>--- .../net/wireless/qcom,ath12k-pci.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml
PATCH v5 7/18 arm64: dts: qcom: sm8550-qrd: add the Wifi node
In reply to Bartosz GolaszewskiFrom: Neil Armstrong <[email protected]>
Describe the ath12k WLAN on-board the WCN7850 module present on the board.
[Bartosz:
- move the pcieport0 node into the .dtsi
- make regulator naming consistent with existing DT code
- add commit message]
--- arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 37 +++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sm8550.dtsi | 10 +++++++ 2 files changed, 47 insertions(+)
arch/arm64/boot/dts/qcom/sm8550-qrd.dts +37 -0
@@ -824,6 +824,25 @@ &pcie0 { status = "okay"; }; +&pcieport0 { + wifi@0 { + compatible = "pci17cb,1107"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + pinctrl-names = "default"; + pinctrl-0 = <&wlan_en>, <&pmk8550_sleep_clk>; + + enable-gpios = <&tlmm 80 GPIO_ACTIVE_HIGH>; + + vdd-supply = <&vreg_s5g_0p85>; + vddio-supply = <&vreg_l15b_1p8>; + vddaon-supply = <&vreg_s2g_0p85>; + vdddig-supply = <&vreg_s4e_0p95>; + vddrfa1p2-supply = <&vreg_s4g_1p25>; + vddrfa1p8-supply = <&vreg_s6g_1p86>; + }; +}; + &pcie0_phy { vdda-phy-supply = <&vreg_l1e_0p88>; vdda-pll-supply = <&vreg_l3e_1p2>;
@@ -911,6 +930,17 @@ &pcie_1_phy_aux_clk { clock-frequency = <1000>; }; +&pmk8550_gpios { + pmk8550_sleep_clk: sleep-clk-state { + pins = "gpio3"; + function = "func1"; + input-disable; + output-enable; + bias-disable; + power-source = <0>; + }; +}; + &qupv3_id_0 { status = "okay"; };
@@ -1084,6 +1114,13 @@ wcd_default: wcd-reset-n-active-state { bias-disable; output-low; }; + + wlan_en: wlan-en-state { + pins = "gpio80"; + function = "gpio"; + drive-strength = <8>; + bias-pull-down; + }; }; &uart7 {
arch/arm64/boot/dts/qcom/sm8550.dtsi +10 -0
@@ -1767,6 +1767,16 @@ pcie0: pcie@1c00000 { phy-names = "pciephy"; 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 {
Signature · 1 line
2.40.1
PATCH v5 8/18 arm64: dts: qcom: sm8650-qrd: add the Wifi node
In reply to Bartosz GolaszewskiFrom: Neil Armstrong <[email protected]>
Describe the ath12k WLAN on-board the WCN7850 module present on the board.
[Bartosz:
- move the pcieport0 node into the .dtsi
- make regulator naming consistent with existing DT code
- add commit message]
--- arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 +++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sm8650.dtsi | 10 +++++++++ 2 files changed, 39 insertions(+)
arch/arm64/boot/dts/qcom/sm8650-qrd.dts +29 -0
@@ -845,6 +845,28 @@ &pcie0 { status = "okay"; }; +&pcieport0 { + wifi@0 { + compatible = "pci17cb,1107"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + pinctrl-names = "default"; + pinctrl-0 = <&wlan_en>; + + enable-gpios = <&tlmm 16 GPIO_ACTIVE_HIGH>; + + vdd-supply = <&vreg_s4i_0p85>; + vddio-supply = <&vreg_l15b_1p8>; + vddio1p2-supply = <&vreg_l3c_1p2>; + vddaon-supply = <&vreg_s2c_0p8>; + vdddig-supply = <&vreg_s3c_0p9>; + vddrfa1p2-supply = <&vreg_s1c_1p2>; + vddrfa1p8-supply = <&vreg_s6c_1p8>; + + clocks = <&rpmhcc RPMH_RF_CLK1>; + }; +}; + &pcie0_phy { vdda-phy-supply = <&vreg_l1i_0p88>; vdda-pll-supply = <&vreg_l3i_1p2>;
@@ -1139,6 +1161,13 @@ wcd_default: wcd-reset-n-active-state { bias-disable; output-low; }; + + wlan_en: wlan-en-state { + pins = "gpio16"; + function = "gpio"; + drive-strength = <8>; + bias-pull-down; + }; }; &uart14 {
arch/arm64/boot/dts/qcom/sm8650.dtsi +10 -0
@@ -2293,6 +2293,16 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, 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 {
Signature · 1 line
2.40.1
Re: [PATCH v5 08/18] arm64: dts: qcom: sm8650-qrd: add the Wifi node
Quoted 5 lines · Bartosz Golaszewski
From: Neil Armstrong <[email protected]>
Describe the ath12k WLAN on-board the WCN7850 module present on the board.
WCN7850 is the same combo WiFi + BT chip. Is there any reason for describing its parts separately rather than using the same PMU approach?
Quoted 83 lines
Signed-off-by Neil Armstrong <[email protected]>[Bartosz:
- move the pcieport0 node into the .dtsi
- make regulator naming consistent with existing DT code
- add commit message]Signed-off-by Bartosz Golaszewski <[email protected]>--- arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 +++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sm8650.dtsi | 10 +++++++++ 2 files changed, 39 insertions(+)arch/arm64/boot/dts/qcom/sm8650-qrd.dts +29
-0@@ -845,6 +845,28 @@ &pcie0 { status = "okay"; }; +&pcieport0 { + wifi@0 { + compatible = "pci17cb,1107"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + pinctrl-names = "default"; + pinctrl-0 = <&wlan_en>; + + enable-gpios = <&tlmm 16 GPIO_ACTIVE_HIGH>; + + vdd-supply = <&vreg_s4i_0p85>; + vddio-supply = <&vreg_l15b_1p8>; + vddio1p2-supply = <&vreg_l3c_1p2>; + vddaon-supply = <&vreg_s2c_0p8>; + vdddig-supply = <&vreg_s3c_0p9>; + vddrfa1p2-supply = <&vreg_s1c_1p2>; + vddrfa1p8-supply = <&vreg_s6c_1p8>; + + clocks = <&rpmhcc RPMH_RF_CLK1>; + }; +}; + &pcie0_phy { vdda-phy-supply = <&vreg_l1i_0p88>; vdda-pll-supply = <&vreg_l3i_1p2>;@@ -1139,6 +1161,13 @@ wcd_default: wcd-reset-n-active-state { bias-disable; output-low; }; + + wlan_en: wlan-en-state { + pins = "gpio16"; + function = "gpio"; + drive-strength = <8>; + bias-pull-down; + }; }; &uart14 {arch/arm64/boot/dts/qcom/sm8650.dtsi +10
-1@@ -2293,6 +2293,16 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, 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
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH v5 08/18] arm64: dts: qcom: sm8650-qrd: add the Wifi node
Quoted 12 lines · Dmitry Baryshkov
Quoted 5 lines · Bartosz Golaszewski
From: Neil Armstrong <[email protected]>
Describe the ath12k WLAN on-board the WCN7850 module present on the board.
WCN7850 is the same combo WiFi + BT chip. Is there any reason for describing its parts separately rather than using the same PMU approach?
Yes, I explained the rationale in the cover letter in detail in the section describing the DTS changes for the series.
Bart
[snip]
PATCH v5 9/18 arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a node for the PMU module of the QCA6391 present on the RB5 board. Assign its LDO power outputs to the existing Bluetooth module. Add a node for the PCIe port to sm8250.dtsi and define the WLAN node on it in the board's .dts and also make it consume the power outputs of the PMU.
--- arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 123 +++++++++++++++++++++-- arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++ 2 files changed, 122 insertions(+), 11 deletions(-)
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +112 -11
@@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { regulator-always-on; }; + qca6390-pmu { + compatible = "qcom,qca6390-pmu"; + + pinctrl-names = "default"; + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; + + vddaon-supply = <&vreg_s6a_0p95>; + vddpmu-supply = <&vreg_s2f_0p95>; + vddrfa0p95-supply = <&vreg_s2f_0p95>; + vddrfa1p3-supply = <&vreg_s8c_1p3>; + vddrfa1p9-supply = <&vreg_s5a_1p9>; + vddpcie1p3-supply = <&vreg_s8c_1p3>; + vddpcie1p9-supply = <&vreg_s5a_1p9>; + vddio-supply = <&vreg_s4a_1p8>; + + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; + + regulators { + vreg_pmu_rfa_cmn: ldo0 { + regulator-name = "vreg_pmu_rfa_cmn"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_wlcx_0p8: ldo2 { + regulator-name = "vreg_pmu_wlcx_0p8"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_wlmx_0p85: ldo3 { + regulator-name = "vreg_pmu_wlmx_0p85"; + regulator-min-microvolt = <810000>; + regulator-max-microvolt = <890000>; + }; + + vreg_pmu_btcmx_0p85: ldo4 { + regulator-name = "vreg_pmu_btcmx_0p85"; + regulator-min-microvolt = <810000>; + regulator-max-microvolt = <890000>; + }; + + vreg_pmu_rfa_0p8: ldo5 { + regulator-name = "vreg_pmu_rfa_0p8"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_rfa_1p2: ldo6 { + regulator-name = "vreg_pmu_rfa_1p2"; + regulator-min-microvolt = <1187000>; + regulator-max-microvolt = <1313000>; + }; + + vreg_pmu_rfa_1p7: ldo7 { + regulator-name = "vreg_pmu_rfa_1p7"; + regulator-min-microvolt = <1710000>; + regulator-max-microvolt = <1890000>; + }; + + vreg_pmu_pcie_0p9: ldo8 { + regulator-name = "vreg_pmu_pcie_0p9"; + regulator-min-microvolt = <870000>; + regulator-max-microvolt = <970000>; + }; + + vreg_pmu_pcie_1p8: ldo9 { + regulator-name = "vreg_pmu_pcie_1p8"; + regulator-min-microvolt = <1710000>; + regulator-max-microvolt = <1890000>; + }; + }; + }; + thermal-zones { conn-thermal { polling-delay-passive = <0>;
@@ -734,6 +815,23 @@ &pcie0_phy { vdda-pll-supply = <&vreg_l9a_1p2>; }; +&pcieport0 { + wifi@0 { + compatible = "pci17cb,1101"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; + vddaon-supply = <&vreg_pmu_aon_0p59>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p7-supply = <&vreg_pmu_rfa_1p7>; + vddpcie0p9-supply = <&vreg_pmu_pcie_0p9>; + vddpcie1p8-supply = <&vreg_pmu_pcie_1p8>; + }; +}; + &pcie1 { status = "okay"; };
@@ -1303,6 +1401,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 {
@@ -1311,17 +1417,12 @@ &uart6 { bluetooth { compatible = "qcom,qca6390-bt"; - pinctrl-names = "default"; - pinctrl-0 = <&bt_en_state>; - - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; - - vddio-supply = <&vreg_s4a_1p8>; - vddpmu-supply = <&vreg_s2f_0p95>; - vddaon-supply = <&vreg_s6a_0p95>; - vddrfa0p9-supply = <&vreg_s2f_0p95>; - vddrfa1p3-supply = <&vreg_s8c_1p3>; - vddrfa1p9-supply = <&vreg_s5a_1p9>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; + vddaon-supply = <&vreg_pmu_aon_0p59>; + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p7-supply = <&vreg_pmu_rfa_1p7>; }; };
arch/arm64/boot/dts/qcom/sm8250.dtsi +10 -0
@@ -2203,6 +2203,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 {
Signature · 1 line
2.40.1
Mark Brown
maintainerRe: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 140
Quoted 5 lines · Bartosz Golaszewski
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };
That's a *very* wide voltage range for a supply that's got a name ending in _0_p59 which sounds a lot like it should be fixed at 0.59V. Similarly for a bunch of the other supplies, and I'm not seeing any evidence that the consumers do any voltage changes here? There doesn't appear to be any logic here, I'm not convinced these are validated or safe constraints.
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 9 lines · Mark Brown
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 140
Quoted 5 lines · Bartosz Golaszewski
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };
Quoted 6 lines
That's a *very* wide voltage range for a supply that's got a name ending in
_0_p59which sounds a lot like it should be fixed at 0.59V. Similarly for a bunch of the other supplies, and I'm not seeing any evidence that the consumers do any voltage changes here? There doesn't appear to be any logic here, I'm not convinced these are validated or safe constraints.
No, the users don't request any regulators (or rather: software representations thereof) because - as per the cover letter - no regulators are created by the PMU driver. This is what is physically on the board - as the schematics and the datasheet define it. I took the values from the docs verbatim. In C, we create a power sequencing provider which doesn't use the regulator framework at all.
Bartosz
Mark Brown
maintainerRe: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 16 lines · Bartosz Golaszewski
Quoted 8 lines · Mark Brown
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 140
Quoted 5 lines · Bartosz Golaszewski
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };Quoted 6 lines
That's a *very* wide voltage range for a supply that's got a name ending in
_0_p59which sounds a lot like it should be fixed at 0.59V. Similarly for a bunch of the other supplies, and I'm not seeing any evidence that the consumers do any voltage changes here? There doesn't appear to be any logic here, I'm not convinced these are validated or safe constraints.
Quoted 4 lines
No, the users don't request any regulators (or rather: software representations thereof) because - as per the cover letter - no regulators are created by the PMU driver. This is what is physically on the board - as the schematics and the datasheet define it. I took
The above makes no sense. How can constraints be "what is physically on the board", particularly variable constrants when there isn't even a consumer? What values are you taking from which documentation?
The cover letter and binding both claimed (buried after large amounts of changelog) that these PMUs were exposing regulators to consumers and the DTS puports to do exactly that...
Quoted 2 lines
the values from the docs verbatim. In C, we create a power sequencing provider which doesn't use the regulator framework at all.
For something that doesn't use the regulator framework at all what appears to be a provider in patch 16 ("power: pwrseq: add a driver for the QCA6390 PMU module") seems to have a lot of regualtor API calls?
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 5 lines · Mark Brown
Quoted 2 lines · Bartosz Golaszewski
Quoted 1 line · Mark Brown
On Fri, Feb 16, 2024 at 09:32:06PM +0100, Bartosz Golaszewski wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 140
Quoted 5 lines
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };That's a *very* wide voltage range for a supply that's got a name ending
Because it's an error, it should have been 640000. Thanks for spotting it.
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
in
_0_p59which sounds a lot like it should be fixed at 0.59V. Similarly for a bunch of the other supplies, and I'm not seeing any evidence that the consumers do any voltage changes here? There doesn't appear to be any logic here, I'm not convinced these are validated or safe constraints.
Quoted 9 lines
Quoted 4 lines
No, the users don't request any regulators (or rather: software representations thereof) because - as per the cover letter - no regulators are created by the PMU driver. This is what is physically on the board - as the schematics and the datasheet define it. I took
The above makes no sense. How can constraints be "what is physically on the board", particularly variable constrants when there isn't even a consumer? What values are you taking from which documentation?
The operating conditions for PMU outputs. I took them from a confidential datasheet. There's a table for input constraints and possible output values.
And what do you mean by there not being any consumers? The WLAN and BT *are* the consumers.
Quoted 4 lines
The cover letter and binding both claimed (buried after large amounts of changelog) that these PMUs were exposing regulators to consumers and the DTS puports to do exactly that...
Yes, but I'm not sure what the question is.
Quoted 6 lines
Quoted 2 lines
the values from the docs verbatim. In C, we create a power sequencing provider which doesn't use the regulator framework at all.
For something that doesn't use the regulator framework at all what appears to be a provider in patch 16 ("power: pwrseq: add a driver for the QCA6390 PMU module") seems to have a lot of regualtor API calls?
This driver is a power sequencing *provider* but also a regulator *consumer*. It gets regulators from the host and exposes a power sequencer to *its* consumers (WLAN and BT). On DT it exposes regulators (LDO outputs of the PMU) but we don't instantiate them in C.
Bart
Mark Brown
maintainerRe: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 8 lines · Bartosz Golaszewski
Quoted 6 lines · Mark Brown
Quoted 4 lines · Bartosz Golaszewski
No, the users don't request any regulators (or rather: software representations thereof) because - as per the cover letter - no regulators are created by the PMU driver. This is what is physically on the board - as the schematics and the datasheet define it. I took
Quoted 7 lines
Quoted 3 lines
The above makes no sense. How can constraints be "what is physically on the board", particularly variable constrants when there isn't even a consumer? What values are you taking from which documentation?
The operating conditions for PMU outputs. I took them from a confidential datasheet. There's a table for input constraints and possible output values.
That sounds like you're just putting the maximum range of voltages that the PMU can output in there. This is a fundamental misunderstanding of what the constraints are for, the constraints exist to specify what is safe on a specific board which will in essentially all cases be much more restricted. The regulator driver should describe whatever the PMU can support by itself, the constraints whatever is actually safe and functional on the specific board.
Quoted 2 lines
And what do you mean by there not being any consumers? The WLAN and BT *are* the consumers.
There are no drivers that bind to the regulators and vary the voltages at runtime.
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
the values from the docs verbatim. In C, we create a power sequencing provider which doesn't use the regulator framework at all.
For something that doesn't use the regulator framework at all what appears to be a provider in patch 16 ("power: pwrseq: add a driver for the QCA6390 PMU module") seems to have a lot of regualtor API calls?
Quoted 5 lines
This driver is a power sequencing *provider* but also a regulator *consumer*. It gets regulators from the host and exposes a power sequencer to *its* consumers (WLAN and BT). On DT it exposes regulators (LDO outputs of the PMU) but we don't instantiate them in C.
Right, which sounds a lot like being a user of the regualtor framework.
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 18 lines · Mark Brown
Quoted 8 lines · Bartosz Golaszewski
Quoted 6 lines · Mark Brown
Quoted 4 lines · Bartosz Golaszewski
No, the users don't request any regulators (or rather: software representations thereof) because - as per the cover letter - no regulators are created by the PMU driver. This is what is physically on the board - as the schematics and the datasheet define it. I took
Quoted 7 lines
Quoted 3 lines
The above makes no sense. How can constraints be "what is physically on the board", particularly variable constrants when there isn't even a consumer? What values are you taking from which documentation?
The operating conditions for PMU outputs. I took them from a confidential datasheet. There's a table for input constraints and possible output values.
Quoted 8 lines
That sounds like you're just putting the maximum range of voltages that the PMU can output in there. This is a fundamental misunderstanding of what the constraints are for, the constraints exist to specify what is safe on a specific board which will in essentially all cases be much more restricted. The regulator driver should describe whatever the PMU can support by itself, the constraints whatever is actually safe and functional on the specific board.
Ok, got it. Yeah I misunderstood that, but I think it's maybe the second or third time I'm adding a regulators node myself to DT. I'll change that.
Quoted 6 lines
Quoted 2 lines
And what do you mean by there not being any consumers? The WLAN and BT *are* the consumers.
There are no drivers that bind to the regulators and vary the voltages at runtime.
Even with the above misunderstanding clarified: so what? DT is the representation of hardware. There's nothing that obligates us to model DT sources in drivers 1:1.
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
the values from the docs verbatim. In C, we create a power sequencing provider which doesn't use the regulator framework at all.
For something that doesn't use the regulator framework at all what appears to be a provider in patch 16 ("power: pwrseq: add a driver for the QCA6390 PMU module") seems to have a lot of regualtor API calls?
Quoted 7 lines
Quoted 5 lines
This driver is a power sequencing *provider* but also a regulator *consumer*. It gets regulators from the host and exposes a power sequencer to *its* consumers (WLAN and BT). On DT it exposes regulators (LDO outputs of the PMU) but we don't instantiate them in C.
Right, which sounds a lot like being a user of the regualtor framework.
Ok, I meant "user" as a regulator provider but maybe I wasn't clear enough.
Bart
Mark Brown
maintainerRe: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 6 lines · Bartosz Golaszewski
Quoted 4 lines · Mark Brown
Quoted 2 lines · Bartosz Golaszewski
And what do you mean by there not being any consumers? The WLAN and BT *are* the consumers.
Quoted 6 lines
Quoted 2 lines
There are no drivers that bind to the regulators and vary the voltages at runtime.
Even with the above misunderstanding clarified: so what? DT is the representation of hardware. There's nothing that obligates us to model DT sources in drivers 1:1.
It is generally a bad sign if there is a voltage range specified on a regulator that's not got any indication that the voltage is going to be actively managed, especially in situations like with several of the supplies the DT was specifying where there are clear indications that the supply is intended to be fixed voltage (or cases where every single supply has a voltage range which would be highly unusual). Looking at the consumers might provide an explanation for such unusual and likely incorrect constraints, and the lack of any consumers in conjunction with other warning signs reenforces those warning signs.
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 15 lines · Mark Brown
Quoted 6 lines · Bartosz Golaszewski
Quoted 4 lines · Mark Brown
Quoted 2 lines · Bartosz Golaszewski
And what do you mean by there not being any consumers? The WLAN and BT *are* the consumers.
Quoted 6 lines
Quoted 2 lines
There are no drivers that bind to the regulators and vary the voltages at runtime.
Even with the above misunderstanding clarified: so what? DT is the representation of hardware. There's nothing that obligates us to model DT sources in drivers 1:1.
Quoted 9 lines
It is generally a bad sign if there is a voltage range specified on a regulator that's not got any indication that the voltage is going to be actively managed, especially in situations like with several of the supplies the DT was specifying where there are clear indications that the supply is intended to be fixed voltage (or cases where every single supply has a voltage range which would be highly unusual). Looking at the consumers might provide an explanation for such unusual and likely incorrect constraints, and the lack of any consumers in conjunction with other warning signs reenforces those warning signs.
What do you recommend? No values at all in these regulators as it's the PMU which will manage those on its own once powered up by the host PMIC?
Bartosz
Mark Brown
maintainerRe: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 12 lines · Bartosz Golaszewski
Quoted 9 lines · Mark Brown
It is generally a bad sign if there is a voltage range specified on a regulator that's not got any indication that the voltage is going to be actively managed, especially in situations like with several of the supplies the DT was specifying where there are clear indications that the supply is intended to be fixed voltage (or cases where every single supply has a voltage range which would be highly unusual). Looking at the consumers might provide an explanation for such unusual and likely incorrect constraints, and the lack of any consumers in conjunction with other warning signs reenforces those warning signs.
Quoted 3 lines
What do you recommend? No values at all in these regulators as it's the PMU which will manage those on its own once powered up by the host PMIC?
Unless something is actively going to change the voltages at runtime or Linux needs to set a specific voltage (in which case minimum and maximum should be identical) there should be nothing specified.
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
In reply to Bartosz GolaszewskiQuoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Mark Brown
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
That's a *very* wide voltage range for a supply that's got a name ending
Because it's an error, it should have been 640000. Thanks for spotting it.
According to the datasheet, VDD08_PMU_AON_O goes up to 0.85V then down to 0.59V, which is the working voltage.
VDD08_PMU_RFA_CMN is normally at 0.8V, but goes to 0.4V during sleep.
Quoted 45 lines
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
in
_0_p59which sounds a lot like it should be fixed at 0.59V. Similarly for a bunch of the other supplies, and I'm not seeing any evidence that the consumers do any voltage changes here? There doesn't appear to be any logic here, I'm not convinced these are validated or safe constraints.Quoted 9 lines
Quoted 4 lines
No, the users don't request any regulators (or rather: software representations thereof) because - as per the cover letter - no regulators are created by the PMU driver. This is what is physically on the board - as the schematics and the datasheet define it. I took
The above makes no sense. How can constraints be "what is physically on the board", particularly variable constrants when there isn't even a consumer? What values are you taking from which documentation?
The operating conditions for PMU outputs. I took them from a confidential datasheet. There's a table for input constraints and possible output values.
And what do you mean by there not being any consumers? The WLAN and BT *are* the consumers.
Quoted 4 lines
The cover letter and binding both claimed (buried after large amounts of changelog) that these PMUs were exposing regulators to consumers and the DTS puports to do exactly that...
Yes, but I'm not sure what the question is.
Quoted 6 lines
Quoted 2 lines
the values from the docs verbatim. In C, we create a power sequencing provider which doesn't use the regulator framework at all.
For something that doesn't use the regulator framework at all what appears to be a provider in patch 16 ("power: pwrseq: add a driver for the QCA6390 PMU module") seems to have a lot of regualtor API calls?
This driver is a power sequencing *provider* but also a regulator *consumer*. It gets regulators from the host and exposes a power sequencer to *its* consumers (WLAN and BT). On DT it exposes regulators (LDO outputs of the PMU) but we don't instantiate them in C.
Bart
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 15 lines · Dmitry Baryshkov
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Mark Brown
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
That's a *very* wide voltage range for a supply that's got a name ending
Because it's an error, it should have been 640000. Thanks for spotting it.
According to the datasheet,
VDD08_PMU_AON_Ogoes up to 0.85V then down to 0.59V, which is the working voltage.
Hmm indeed this is what figure 3.4 says but table 3-2 says the maximum is 0.64V.
Quoted 2 lines
VDD08_PMU_RFA_CMNis normally at 0.8V, but goes to 0.4V during sleep.
Again figure 3.4 and table 3-2 disagree unless I'm missing something.
Bart
[snip]
Re: [PATCH v5 09/18] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
Quoted 26 lines · Bartosz Golaszewski
Quoted 15 lines · Dmitry Baryshkov
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Mark Brown
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
(quoted diff) +5
-0+ vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + };Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
That's a *very* wide voltage range for a supply that's got a name ending
Because it's an error, it should have been 640000. Thanks for spotting it.
According to the datasheet,
VDD08_PMU_AON_Ogoes up to 0.85V then down to 0.59V, which is the working voltage.
Quoted 6 lines
Hmm indeed this is what figure 3.4 says but table 3-2 says the maximum is 0.64V.
Quoted 2 lines
VDD08_PMU_RFA_CMNis normally at 0.8V, but goes to 0.4V during sleep.Again figure 3.4 and table 3-2 disagree unless I'm missing something.
I suspect that the table you have mentioned provides normal working conditions for the PMU, while power-up and sleep might be outside of 'normal' conditions.
I suppose that these outputs are underspecified in the datasheet. I think we can omit the values here.
Signature · 2 lines
With best wishes Dmitry
PATCH v5 10/18 PCI: hold the rescan mutex when scanning for the first time
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
With the introduction of PCI device power control drivers that will be able to trigger the port rescan when probing, we need to hold the rescan mutex during the initial pci_host_probe() too or the two could get in each other's way.
--- drivers/pci/probe.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/pci/probe.c +2 -0
@@ -3066,7 +3066,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;
Signature · 1 line
2.40.1
PATCH v5 11/18 PCI/pwrctl: reuse the OF node for power controlled devices
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
With PCI power control we deal with two struct device objects bound to two different drivers but consuming the same OF node. We must not bind the pinctrl twice. To that end: before setting the OF node of the newly instantiated PCI device, check if a platform device consuming the same OF node doesn't already exist on the platform bus and - if so - mark the PCI device as reusing the OF node.
--- drivers/pci/of.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/pci/of.c +10 -4
@@ -6,6 +6,7 @@*/ #define pr_fmt (fmt) "PCI: OF: " fmt +#include <linux/cleanup.h> #include <linux/irqdomain.h> #include <linux/kernel.h> #include <linux/pci.h>
@@ -13,6 +14,7 @@#include <linux/of_irq.h> #include <linux/of_address.h> #include <linux/of_pci.h> +#include <linux/platform_device.h> #include "pci.h" #ifdef CONFIG_PCI
@@ -25,16 +27,20 @@*/ int pci_set_of_node (struct pci_dev *dev) { -struct device_node *node; -if (!dev->bus->dev.of_node)return 0 ; - node =of_pci_find_child_device (dev->bus->dev.of_node, dev->devfn); + struct device_node *node __free(device_node) = +of_pci_find_child_device (dev->bus->dev.of_node, dev->devfn);if (!node)return 0 ; -device_set_node (&dev->dev,of_fwnode_handle (node)); + struct device *pdev __free(put_device) = +bus_find_device_by_of_node (&platform_bus_type, node); +if (pdev) + dev->bus->dev.of_node_reused = true; + +device_set_node (&dev->dev,of_fwnode_handle (no_free_ptr (node)));return 0 ; }
Signature · 1 line
2.40.1
PATCH v5 12/18 PCI/pwrctl: create platform devices for child OF nodes of the port node
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
In preparation for introducing PCI device power control - a set of library functions that will allow powering-up of PCI devices before they're detected on the PCI bus - we need to populate the devices defined on the device-tree.
We are reusing the platform bus as it provides us with all the infrastructure we need to match the pwrctl drivers against the compatibles from OF nodes.
These platform devices will be probed by the driver core and bound to the PCI pwrctl drivers we'll introduce later.
--- drivers/pci/bus.c | 9 ++++++++- drivers/pci/remove.c | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-)
drivers/pci/bus.c +8 -1
@@ -12,6 +12,7 @@#include <linux/errno.h> #include <linux/ioport.h> #include <linux/of.h> +#include <linux/of_platform.h> #include <linux/proc_fs.h> #include <linux/slab.h>
@@ -342,8 +343,14 @@ void pci_bus_add_device(struct pci_dev *dev)*/ pcibios_bus_add_device (dev);pci_fixup_device (pci_fixup_final, dev); -if (pci_is_bridge (dev)) +if (pci_is_bridge (dev)) {of_pci_make_dev_node (dev); + retval =of_platform_populate (dev->dev.of_node,NULL ,NULL , + &dev->dev); +if (retval) +pci_err (dev,"failed to populate child OF nodes (%d)\n" , + retval); + }pci_create_sysfs_dev_files (dev);pci_proc_attach_device (dev);pci_bridge_d3_update (dev);
drivers/pci/remove.c +2 -0
@@ -1,6 +1,7 @@// SPDX-License-Identifier: GPL-2.0 #include <linux/pci.h> #include <linux/module.h> +#include <linux/of_platform.h> #include "pci.h" static void pci_free_resources (struct pci_dev *dev)
@@ -22,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)device_release_driver (&dev->dev);pci_proc_detach_device (dev);pci_remove_sysfs_dev_files (dev); +of_platform_depopulate (&dev->dev);of_pci_remove_node (dev);pci_dev_assign_added (dev, false);
Signature · 1 line
2.40.1
PATCH v5 13/18 PCI/pwrctl: add PCI power control core code
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Some PCI devices must be powered-on before they can be detected on the bus. Introduce a simple framework reusing the existing PCI OF infrastructure.
The way this works is: a DT node representing a PCI device connected to the port can be matched against its power control platform driver. If the match succeeds, the driver is responsible for powering-up the device and calling pcie_pwrctl_device_set_ready() 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 control device.
--- drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/pwrctl/Kconfig | 8 +++ drivers/pci/pwrctl/Makefile | 4 ++ drivers/pci/pwrctl/core.c | 136 ++++++++++++++++++++++++++++++++++++ include/linux/pci-pwrctl.h | 51 ++++++++++++++ 6 files changed, 201 insertions(+) create mode 100644 drivers/pci/pwrctl/Kconfig create mode 100644 drivers/pci/pwrctl/Makefile create mode 100644 drivers/pci/pwrctl/core.c create mode 100644 include/linux/pci-pwrctl.h
drivers/pci/Kconfig +1 -0
@@ -296,5 +296,6 @@ source "drivers/pci/hotplug/Kconfig" source "drivers/pci/controller/Kconfig" source "drivers/pci/endpoint/Kconfig" source "drivers/pci/switch/Kconfig" +source "drivers/pci/pwrctl/Kconfig" endif
drivers/pci/Makefile +1 -0
@@ -9,6 +9,7 @@ obj-$(CONFIG_PCI) += access.o bus.o probe.o host-bridge.o \ obj-$(CONFIG_PCI) += msi/ obj-$(CONFIG_PCI) += pcie/ +obj-$(CONFIG_PCI) += pwrctl/ ifdef CONFIG_PCI obj-$(CONFIG_PROC_FS) += proc.o
drivers/pci/pwrctl/Kconfig new file +8 -0
@@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-only + +menu "PCI Power control drivers" + +config PCI_PWRCTL + tristate + +endmenu
drivers/pci/pwrctl/Makefile new file +4 -0
@@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0-only + +obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o +pci-pwrctl-core-y := core.o
drivers/pci/pwrctl/core.c new file +136 -0
136 lines not on this page · raw message
include/linux/pci-pwrctl.h new file +51 -0
51 lines not on this page · raw message
Signature · 1 line
2.40.1
PATCH v5 14/18 PCI/pwrctl: add a power control driver for WCN7850
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a PCI power control driver that's capable of correctly powering up the ath12k module on WCN7850 using the PCI pwrctl functionality.
--- drivers/pci/pwrctl/Kconfig | 8 + drivers/pci/pwrctl/Makefile | 2 + drivers/pci/pwrctl/pci-pwrctl-wcn7850.c | 202 ++++++++++++++++++++++++ 3 files changed, 212 insertions(+) create mode 100644 drivers/pci/pwrctl/pci-pwrctl-wcn7850.c
drivers/pci/pwrctl/Kconfig +8 -0
@@ -5,4 +5,12 @@ menu "PCI Power control drivers" config PCI_PWRCTL tristate +config PCI_PWRCTL_WCN7850 + tristate "PCI Power Control driver for WCN7850" + select PCI_PWRCTL + default m if (ATH12K && ARCH_QCOM) + help + Enable support for the PCI power control driver for the ath12k + module of the WCN7850 WLAN/BT chip. + endmenu
drivers/pci/pwrctl/Makefile +2 -0
@@ -2,3 +2,5 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o pci-pwrctl-core-y := core.o + +obj-$(CONFIG_PCI_PWRCTL_WCN7850) += pci-pwrctl-wcn7850.o
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +202 -0
202 lines not on this page · raw message
Signature · 1 line
2.40.1
kernel test robot
build botRe: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
kernel test robot noticed the following build warnings:
Hi Bartosz,
kernel test robot noticed the following build warnings:
[auto build test WARNING on next-20240216]
[cannot apply to pci/next pci/for-linus robh/for-next bluetooth/master linus/master v6.8-rc4 v6.8-rc3 v6.8-rc2 v6.8-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/of-Add-cleanup-h-based-auto-release-via-__free-device_node-markings/20240217-044313 base: next-20240216 patch link: https://lore.kernel.org/r/20240216203215.40870-15-brgl%40bgdev.pl patch subject: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850 config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240218/[email protected]/config) compiler: aarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240218/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/pci/pwrctl/core.c:121: warning: Function parameter or struct member 'dev' not described in 'devm_pci_pwrctl_device_set_ready'
vim +121 drivers/pci/pwrctl/core.c
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 109
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 110 /**
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 111 * devm_pci_pwrctl_device_set_ready - Managed variant of
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 112 * pci_pwrctl_device_set_ready().
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 113 *
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 114 * @pwrctl: PCI power control data
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 115 *
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 116 * Returns:
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 117 * 0 on success, negative error number on error.
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 118 */
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 119 int devm_pci_pwrctl_device_set_ready(struct device *dev,
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 120 struct pci_pwrctl *pwrctl)
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 @121 {
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 122 int ret;
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 123
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 124 ret = pci_pwrctl_device_set_ready(pwrctl);
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 125 if (ret)
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 126 return ret;
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 127
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 128 return devm_add_action_or_reset(dev,
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 129 devm_pci_pwrctl_device_unset_ready,
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 130 pwrctl);
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 131 }
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 132 EXPORT_SYMBOL_GPL(devm_pci_pwrctl_device_set_ready);
3f368e2e7dc2e8 Bartosz Golaszewski 2024-02-16 133 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Mark Brown
maintainerRe: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
In reply to Bartosz Golaszewskidrivers/pci/pwrctl/pci-pwrctl-wcn7850.c line 42
Quoted 5 lines · Bartosz Golaszewski
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +5
-0+static struct pci_pwrctl_wcn7850_vreg pci_pwrctl_wcn7850_vregs[] = { + { + .name ="vdd" , + .load_uA =16000 , + },
I know a bunch of the QC stuff includes these load numbers but are they actually doing anything constructive? It keeps coming up that they're causing a bunch of work and it's not clear that they have any great effect on modern systems.
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c line 85
Quoted 11 lines
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +11
-0+static int pci_pwrctl_wcn7850_power_on (struct pci_pwrctl_wcn7850_ctx *ctx) +{ +int ret; + + ret =regulator_bulk_enable (ctx->pdata->num_vregs, ctx->regs); +if (ret) +return ret; + + ret =clk_prepare_enable (ctx->clk); +if (ret) +return ret;
This won't disable the regulators on error.
Re: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 9 lines · Mark Brown
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c line 42
Quoted 5 lines · Bartosz Golaszewski
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +5
-0+static struct pci_pwrctl_wcn7850_vreg pci_pwrctl_wcn7850_vregs[] = { + { + .name ="vdd" , + .load_uA =16000 , + },
Quoted 5 lines
I know a bunch of the QC stuff includes these load numbers but are they actually doing anything constructive? It keeps coming up that they're causing a bunch of work and it's not clear that they have any great effect on modern systems.
Yes, we have what is called a high-power mode and a low-power mode in regulators and these values are used to determine which one to use.
Quoted 5 lines
Quoted 5 lines
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +5
-0+static int pci_pwrctl_wcn7850_power_on (struct pci_pwrctl_wcn7850_ctx *ctx) +{ +int ret; + + ret =regulator_bulk_enable (ctx->pdata->num_vregs, ctx->regs);
Quoted 8 lines
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c line 85
Quoted 6 lines
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +6
-0+if (ret) +return ret; + + ret =clk_prepare_enable (ctx->clk); +if (ret) +return ret;This won't disable the regulators on error.
Indeed. Thanks for catching this.
Bart
Mark Brown
maintainerRe: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 9 lines · Bartosz Golaszewski
Quoted 7 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +5
-0+static struct pci_pwrctl_wcn7850_vreg pci_pwrctl_wcn7850_vregs[] = { + { + .name ="vdd" , + .load_uA =16000 , + },
Quoted 7 lines
Quoted 4 lines
I know a bunch of the QC stuff includes these load numbers but are they actually doing anything constructive? It keeps coming up that they're causing a bunch of work and it's not clear that they have any great effect on modern systems.
Yes, we have what is called a high-power mode and a low-power mode in regulators and these values are used to determine which one to use.
Are you *sure* this actually happens (and that the regulators don't figure it out by themselves), especially given that the consumers are just specifying the load once rather than varying it dynamically at runtime which is supposed to be the use case for this API? This API is intended to be used dynamically, if the regulator always needs to be in a particular mode just configure that statically.
Re: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 18 lines · Mark Brown
Quoted 9 lines · Bartosz Golaszewski
Quoted 7 lines · Mark Brown
Quoted 5 lines · Bartosz Golaszewski
drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file +5
-0+static struct pci_pwrctl_wcn7850_vreg pci_pwrctl_wcn7850_vregs[] = { + { + .name ="vdd" , + .load_uA =16000 , + },Quoted 7 lines
Quoted 4 lines
I know a bunch of the QC stuff includes these load numbers but are they actually doing anything constructive? It keeps coming up that they're causing a bunch of work and it's not clear that they have any great effect on modern systems.
Yes, we have what is called a high-power mode and a low-power mode in regulators and these values are used to determine which one to use.
Quoted 6 lines
Are you *sure* this actually happens (and that the regulators don't figure it out by themselves), especially given that the consumers are just specifying the load once rather than varying it dynamically at runtime which is supposed to be the use case for this API? This API is intended to be used dynamically, if the regulator always needs to be in a particular mode just configure that statically.
*AFAIU*
The regulators aggregate the requested current (there may be multiple consumers) and then it's decided if it's high enough to jump into HPM.
Konrad
Mark Brown
maintainerRe: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 9 lines · Konrad Dybcio
Quoted 6 lines · Mark Brown
Are you *sure* this actually happens (and that the regulators don't figure it out by themselves), especially given that the consumers are just specifying the load once rather than varying it dynamically at runtime which is supposed to be the use case for this API? This API is intended to be used dynamically, if the regulator always needs to be in a particular mode just configure that statically.
Quoted 5 lines
*AFAIU*
The regulators aggregate the requested current (there may be multiple consumers) and then it's decided if it's high enough to jump into HPM.
Yes, that's the theory - I just question if it actually does something useful in practice. Between regulators getting more and more able to figure out mode switching autonomously based on load monitoring and them getting more efficient it's become very unclear if this actually accomplishes anything, the only usage is the Qualcomm stuff and that's all really unsophisticated and has an air of something that's being cut'n'pasted forwards rather than delivering practical results. There is some value at ultra low loads, but that's more for suspend modes than for actual use.
Re: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 17 lines · Mark Brown
Quoted 9 lines · Konrad Dybcio
Quoted 6 lines · Mark Brown
Are you *sure* this actually happens (and that the regulators don't figure it out by themselves), especially given that the consumers are just specifying the load once rather than varying it dynamically at runtime which is supposed to be the use case for this API? This API is intended to be used dynamically, if the regulator always needs to be in a particular mode just configure that statically.
Quoted 5 lines
*AFAIU*
The regulators aggregate the requested current (there may be multiple consumers) and then it's decided if it's high enough to jump into HPM.
Quoted 9 lines
Yes, that's the theory - I just question if it actually does something useful in practice. Between regulators getting more and more able to figure out mode switching autonomously based on load monitoring and them getting more efficient it's become very unclear if this actually accomplishes anything, the only usage is the Qualcomm stuff and that's all really unsophisticated and has an air of something that's being cut'n'pasted forwards rather than delivering practical results. There is some value at ultra low loads, but that's more for suspend modes than for actual use.
Removing it would be out of scope for this series and I don't really want to introduce any undefined behavior when doing a big development like that. I'll think about it separately.
Bart
Mark Brown
maintainerRe: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 12 lines · Bartosz Golaszewski
Quoted 9 lines · Mark Brown
Yes, that's the theory - I just question if it actually does something useful in practice. Between regulators getting more and more able to figure out mode switching autonomously based on load monitoring and them getting more efficient it's become very unclear if this actually accomplishes anything, the only usage is the Qualcomm stuff and that's all really unsophisticated and has an air of something that's being cut'n'pasted forwards rather than delivering practical results. There is some value at ultra low loads, but that's more for suspend modes than for actual use.
Quoted 3 lines
Removing it would be out of scope for this series and I don't really want to introduce any undefined behavior when doing a big development like that. I'll think about it separately.
This is new code?
Re: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 14 lines · Mark Brown
Quoted 11 lines · Bartosz Golaszewski
Quoted 9 lines · Mark Brown
Yes, that's the theory - I just question if it actually does something useful in practice. Between regulators getting more and more able to figure out mode switching autonomously based on load monitoring and them getting more efficient it's become very unclear if this actually accomplishes anything, the only usage is the Qualcomm stuff and that's all really unsophisticated and has an air of something that's being cut'n'pasted forwards rather than delivering practical results. There is some value at ultra low loads, but that's more for suspend modes than for actual use.
Quoted 5 lines
Quoted 3 lines
Removing it would be out of scope for this series and I don't really want to introduce any undefined behavior when doing a big development like that. I'll think about it separately.
This is new code?
It's a new driver but Qualcomm standard has been to provide the load values. If it's really unnecessary then maybe let's consider it separately and possibly rework globally?
Bart
Mark Brown
maintainerRe: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 7 lines · Bartosz Golaszewski
Quoted 5 lines · Mark Brown
Quoted 3 lines · Bartosz Golaszewski
Removing it would be out of scope for this series and I don't really want to introduce any undefined behavior when doing a big development like that. I'll think about it separately.
Quoted 5 lines
Quoted 1 line
This is new code?
It's a new driver but Qualcomm standard has been to provide the load values. If it's really unnecessary then maybe let's consider it separately and possibly rework globally?
That doesn't seem a great reason to add more instances of this - it's more instances that need to be removed later, and somewhere else people can cut'n'paste from to introduce new usage.
Re: [PATCH v5 14/18] PCI/pwrctl: add a power control driver for WCN7850
Quoted 11 lines · Mark Brown
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Mark Brown
Quoted 3 lines · Bartosz Golaszewski
Removing it would be out of scope for this series and I don't really want to introduce any undefined behavior when doing a big development like that. I'll think about it separately.
This is new code?
Quoted 7 lines
Quoted 3 lines
It's a new driver but Qualcomm standard has been to provide the load values. If it's really unnecessary then maybe let's consider it separately and possibly rework globally?
That doesn't seem a great reason to add more instances of this - it's more instances that need to be removed later, and somewhere else people can cut'n'paste from to introduce new usage.
Ok, whatever, I'll drop these until they're needed.
Bartosz
PATCH v5 15/18 power: sequencing: implement the pwrseq core
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Implement the power sequencing subsystem allowing devices to share complex powering-up and down procedures. It's split into the consumer and provider parts but does not implement any new DT bindings so that the actual power sequencing is never revealed in the DT representation.
--- MAINTAINERS | 8 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 12 + drivers/power/sequencing/Makefile | 4 + drivers/power/sequencing/core.c | 1065 +++++++++++++++++++++++++++++ include/linux/pwrseq/consumer.h | 56 ++ include/linux/pwrseq/provider.h | 75 ++ 8 files changed, 1222 insertions(+) create mode 100644 drivers/power/sequencing/Kconfig create mode 100644 drivers/power/sequencing/Makefile create mode 100644 drivers/power/sequencing/core.c create mode 100644 include/linux/pwrseq/consumer.h create mode 100644 include/linux/pwrseq/provider.h
MAINTAINERS +8 -0
@@ -17647,6 +17647,14 @@ F: include/linux/pm_* F: include/linux/powercap.h F: kernel/configs/nopm.config +POWER SEQUENCING +M: Bartosz Golaszewski <[email protected]> +L: [email protected] +S: Maintained +T: git git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git +F: drivers/power/sequencing/ +F: include/linux/pwrseq/ + POWER STATE COORDINATION INTERFACE (PSCI) M: Mark Rutland <[email protected]> M: Lorenzo Pieralisi <[email protected]>
drivers/power/Kconfig +1 -0
@@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only source "drivers/power/reset/Kconfig" +source "drivers/power/sequencing/Kconfig" source "drivers/power/supply/Kconfig"
drivers/power/Makefile +1 -0
@@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_POWER_RESET) += reset/ +obj-$(CONFIG_POWER_SEQUENCING) += sequencing/ obj-$(CONFIG_POWER_SUPPLY) += supply/
drivers/power/sequencing/Kconfig new file +12 -0
@@ -0,0 +1,12 @@ +# SPDX-License-Identifier: GPL-2.0-only + +menuconfig POWER_SEQUENCING + tristate "Power Sequencing support" + help + Say Y here to enable the Power Sequencing subsystem. + + This subsystem is designed to control power to devices that share + complex resources and/or require specific power sequences to be run + during power-up. + + If unsure, say no.
drivers/power/sequencing/Makefile new file +4 -0
@@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 + +obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o +pwrseq-core-y := core.o
drivers/power/sequencing/core.c new file +1065 -0
1,065 lines not on this page · raw message
include/linux/pwrseq/consumer.h new file +56 -0
56 lines not on this page · raw message
include/linux/pwrseq/provider.h new file +75 -0
75 lines not on this page · raw message
Signature · 1 line
2.40.1
kernel test robot
build botRe: [PATCH v5 15/18] power: sequencing: implement the pwrseq core
kernel test robot noticed the following build warnings:
Hi Bartosz,
kernel test robot noticed the following build warnings:
[auto build test WARNING on next-20240216]
[cannot apply to pci/next pci/for-linus robh/for-next bluetooth/master linus/master v6.8-rc4 v6.8-rc3 v6.8-rc2 v6.8-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/of-Add-cleanup-h-based-auto-release-via-__free-device_node-markings/20240217-044313 base: next-20240216 patch link: https://lore.kernel.org/r/20240216203215.40870-16-brgl%40bgdev.pl patch subject: [PATCH v5 15/18] power: sequencing: implement the pwrseq core config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240219/[email protected]/config) compiler: aarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240219/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/power/sequencing/core.c:573: warning: expecting prototype for pwrseq_device_get_data(). Prototype was for pwrseq_device_get_drvdata() instead
vim +573 drivers/power/sequencing/core.c
563
564 /**
565 * pwrseq_device_get_data() - Get the driver private data associated with this
566 * sequencer.
567 * @pwrseq: Power sequencer object.
568 *
569 * Returns:
570 * Address of the private driver data.
571 */
572 void *pwrseq_device_get_drvdata(struct pwrseq_device *pwrseq)
> 573 { 574 return dev_get_drvdata(&pwrseq->dev);
575 }
576 EXPORT_SYMBOL_GPL(pwrseq_device_get_drvdata);
577
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
PATCH v5 16/18 power: pwrseq: add a driver for the QCA6390 PMU module
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
This adds the power sequencing driver for the QCA6390's PMU module. It uses the pwrseq subsystem and knows how to match the sequencer to the consumer device by verifying the relevant properties and DT layout.
--- drivers/power/sequencing/Kconfig | 16 + drivers/power/sequencing/Makefile | 2 + drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++++++++++++++++++ 3 files changed, 371 insertions(+) create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c
drivers/power/sequencing/Kconfig +16 -0
@@ -10,3 +10,19 @@ menuconfig POWER_SEQUENCING during power-up. If unsure, say no. + +if POWER_SEQUENCING + +config POWER_SEQUENCING_QCA6390 + tristate "QCA6390 PMU driver" + default m if ARCH_QCOM + help + Say U here to enable the power sequencing driver for Qualcomm + QCA6390. + + The QCA6390 package contains the BT and WLAN modules whose power + is controlled by the PMU module. As the former two share the power-up + sequence which is executed by the PMU, this driver is needed for + correct power control. + +endif
drivers/power/sequencing/Makefile +2 -0
@@ -2,3 +2,5 @@ obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o pwrseq-core-y := core.o + +obj-$(CONFIG_POWER_SEQUENCING_QCA6390) += pwrseq-qca6390.o
drivers/power/sequencing/pwrseq-qca6390.c new file +353 -0
353 lines not on this page · raw message
Signature · 1 line
2.40.1
Re: [PATCH v5 16/18] power: pwrseq: add a driver for the QCA6390 PMU module
Quoted 124 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
This adds the power sequencing driver for the QCA6390's PMU module. It uses the pwrseq subsystem and knows how to match the sequencer to the consumer device by verifying the relevant properties and DT layout.
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/power/sequencing/Kconfig | 16 + drivers/power/sequencing/Makefile | 2 + drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++++++++++++++++++ 3 files changed, 371 insertions(+) create mode 100644 drivers/power/sequencing/pwrseq-qca6390.cdrivers/power/sequencing/Kconfig +16
-0@@ -10,3 +10,19 @@ menuconfig POWER_SEQUENCING during power-up. If unsure, say no. + +if POWER_SEQUENCING + +config POWER_SEQUENCING_QCA6390 + tristate "QCA6390 PMU driver" + default m if ARCH_QCOM + help + Say U here to enable the power sequencing driver for Qualcomm + QCA6390. + + The QCA6390 package contains the BT and WLAN modules whose power + is controlled by the PMU module. As the former two share the power-up + sequence which is executed by the PMU, this driver is needed for + correct power control. + +endifdrivers/power/sequencing/Makefile +2
-0@@ -2,3 +2,5 @@ obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o pwrseq-core-y := core.o + +obj-$(CONFIG_POWER_SEQUENCING_QCA6390) += pwrseq-qca6390.odrivers/power/sequencing/pwrseq-qca6390.c new file +69
-0@@ -0,0 +1,353 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (C) 2024 Linaro Ltd. +*/ + +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/jiffies.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/pwrseq/provider.h> +#include <linux/string.h> +#include <linux/types.h> + +struct pwrseq_qca6390_vreg { +const char *name; +unsigned int load_uA; +}; + +struct pwrseq_qca6390_pdata { +const struct pwrseq_qca6390_vreg *vregs_common; +size_t num_vregs_common; +const struct pwrseq_qca6390_vreg *vregs_wlan; +size_t num_vregs_wlan; +unsigned int pwup_delay_msec; +}; + +struct pwrseq_qca6390_ctx { +struct pwrseq_device *pwrseq; +struct device_node *of_node; +const struct pwrseq_qca6390_pdata *pdata; +struct regulator_bulk_data *regs_common; +struct regulator_bulk_data *regs_wlan; +struct gpio_desc *bt_gpio; +struct gpio_desc *wlan_gpio; +unsigned long last_gpio_enable; +}; + +static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs_common[] = { + { + .name ="vddio" , + .load_uA =20000 , + }, + { + .name ="vddaon" , + .load_uA =100000 , + }, + { + .name ="vddpmu" , + .load_uA =1250000 , + }, + { + .name ="vddrfa0p95" , + .load_uA =200000 , + }, + { + .name ="vddrfa1p3" , + .load_uA =400000 , + }, + { + .name ="vddrfa1p9" , + .load_uA =400000 , + }, +}; +
drivers/power/sequencing/pwrseq-qca6390.c line 79
Quoted 10 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +10
-0+static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs_wlan[] = { + { + .name ="vddpcie1p3" , + .load_uA =35000 , + }, + { + .name ="vddpcie1p9" , + .load_uA =15000 , + }, +};
I thought that we had discussed this already. According to the docs, all PMU supplies should be powered on when the chip is being switched on, no matter whether it is for the WiFi or for the BT.
Quoted 58 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +58
-0+ +static void pwrseq_qca6390_ensure_gpio_delay (struct pwrseq_qca6390_ctx *ctx) +{ +unsigned long diff_jiffies = jiffies - ctx->last_gpio_enable; +unsigned int diff_msecs =jiffies_to_msecs (diff_jiffies); + +if (diff_msecs <100 ) +msleep (100 - diff_msecs); +} + +static const struct pwrseq_qca6390_pdata pwrseq_qca6390_of_data = { + .vregs_common = pwrseq_qca6390_vregs_common, + .num_vregs_common =ARRAY_SIZE (pwrseq_qca6390_vregs_common), + .vregs_wlan = pwrseq_qca6390_vregs_wlan, + .num_vregs_wlan =ARRAY_SIZE (pwrseq_qca6390_vregs_wlan), + .pwup_delay_msec =16 , +}; + +static int pwrseq_qca6390_vregs_enable (struct pwrseq_device *pwrseq) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); + +return regulator_bulk_enable (ctx->pdata->num_vregs_common, + ctx->regs_common); +} + +static int pwrseq_qca6390_vregs_disable (struct pwrseq_device *pwrseq) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); + +return regulator_bulk_disable (ctx->pdata->num_vregs_common, + ctx->regs_common); +} + +static const struct pwrseq_unit_data pwrseq_qca6390_vregs_unit_data = { + .name ="regulators-enable" , + .enable = pwrseq_qca6390_vregs_enable, + .disable = pwrseq_qca6390_vregs_disable, +}; + +static const struct pwrseq_unit_data *pwrseq_qca6390_unit_deps[] = { + &pwrseq_qca6390_vregs_unit_data, +NULL +}; + +static int pwrseq_qca6390_bt_enable (struct pwrseq_device *pwrseq) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); + +pwrseq_qca6390_ensure_gpio_delay (ctx); +gpiod_set_value_cansleep (ctx->bt_gpio,1 ); + ctx->last_gpio_enable = jiffies; + +return 0 ; +} + +static int pwrseq_qca6390_bt_disable (struct pwrseq_device *pwrseq) +{
drivers/power/sequencing/pwrseq-qca6390.c line 147
Quoted 10 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +10
-0+struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); + +gpiod_set_value_cansleep (ctx->bt_gpio,0 ); + +return 0 ; +} + +static const struct pwrseq_unit_data pwrseq_qca6390_bt_unit_data = { + .name ="bluetooth-enable" , + .deps = pwrseq_qca6390_unit_deps,
Can we call corresponding regulator_bulk functions from bt and wlan enable/disable? This will save us from building the tree-like structures (and possible loops inside that tree).
Quoted 209 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +206
-1+ .enable = pwrseq_qca6390_bt_enable, + .disable = pwrseq_qca6390_bt_disable, +}; + +static int pwrseq_qca6390_wlan_enable (struct pwrseq_device *pwrseq) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); +int ret; + + ret =regulator_bulk_enable (ctx->pdata->num_vregs_wlan, ctx->regs_wlan); +if (ret) +return ret; + +pwrseq_qca6390_ensure_gpio_delay (ctx); +gpiod_set_value_cansleep (ctx->wlan_gpio,1 ); + ctx->last_gpio_enable = jiffies; + +return 0 ; +} + +static int pwrseq_qca6390_wlan_disable (struct pwrseq_device *pwrseq) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); + +gpiod_set_value_cansleep (ctx->wlan_gpio,0 ); + +return regulator_bulk_disable (ctx->pdata->num_vregs_wlan, + ctx->regs_wlan); +} + +static const struct pwrseq_unit_data pwrseq_qca6390_wlan_unit_data = { + .name ="wlan-enable" , + .deps = pwrseq_qca6390_unit_deps, + .enable = pwrseq_qca6390_wlan_enable, + .disable = pwrseq_qca6390_wlan_disable, +}; + +static int pwrseq_qca6390_pwup_delay (struct pwrseq_device *pwrseq) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); + +if (ctx->pdata->pwup_delay_msec) +msleep (ctx->pdata->pwup_delay_msec); + +return 0 ; +} + +static const struct pwrseq_target_data pwrseq_qca6390_bt_target_data = { + .name ="bluetooth" , + .unit = &pwrseq_qca6390_bt_unit_data, + .post_enable = pwrseq_qca6390_pwup_delay, +}; + +static const struct pwrseq_target_data pwrseq_qca6390_wlan_target_data = { + .name ="wlan" , + .unit = &pwrseq_qca6390_wlan_unit_data, + .post_enable = pwrseq_qca6390_pwup_delay, +}; + +static const struct pwrseq_target_data *pwrseq_qca6390_targets[] = { + &pwrseq_qca6390_bt_target_data, + &pwrseq_qca6390_wlan_target_data, +NULL +}; + +static int pwrseq_qca6390_match (struct pwrseq_device *pwrseq, +struct device *dev) +{ +struct pwrseq_qca6390_ctx *ctx =pwrseq_device_get_drvdata (pwrseq); +struct device_node *dev_node = dev->of_node; + +/* +* The PMU supplies power to the Bluetooth and WLAN modules. both +* consume the PMU AON output so check the presence of the +* 'vddaon-supply' property and whether it leads us to the right +* device. +*/ +if (!of_property_present (dev_node,"vddaon-supply" )) +return 0 ; + + struct device_node *reg_node __free(device_node) = +of_parse_phandle (dev_node,"vddaon-supply" ,0 ); +if (!reg_node) +return 0 ; + +/* +* `reg_node` is the PMU AON regulator, its parent is the `regulators` +* node and finally its grandparent is the PMU device node that we're +* looking for. +*/ +if (!reg_node->parent || !reg_node->parent->parent || + reg_node->parent->parent != ctx->of_node) +return 0 ; + +return 1 ; +} + +static struct regulator_bulk_data * +pwrseq_qca6390_get_regs (struct device *dev,size_t num_regs, +const struct pwrseq_qca6390_vreg *pdata) +{ +struct regulator_bulk_data *regs; +int ret, i; + + regs =devm_kcalloc (dev, num_regs,sizeof (*regs),GFP_KERNEL ); +if (!regs) +return ERR_PTR (-ENOMEM ); + +for (i =0 ; i < num_regs; i++) + regs[i].supply = pdata[i].name; + + ret =devm_regulator_bulk_get (dev, num_regs, regs); +if (ret <0 ) +return ERR_PTR (ret); + +for (i =0 ; i < num_regs; i++) { +if (!pdata[i].load_uA) +continue ; + + ret =regulator_set_load (regs[i].consumer, pdata[i].load_uA); +if (ret) +return ERR_PTR (ret); + } + +return regs; +} + +static int pwrseq_qca6390_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct pwrseq_qca6390_ctx *ctx; +struct pwrseq_config config; + + ctx =devm_kzalloc (dev,sizeof (*ctx),GFP_KERNEL ); +if (!ctx) +return -ENOMEM ; + + ctx->of_node = dev->of_node; + + ctx->pdata =of_device_get_match_data (dev); +if (!ctx->pdata) +return dev_err_probe (dev, -ENODEV , +"Failed to obtain platform data\n" ); + + ctx->regs_common =pwrseq_qca6390_get_regs (dev, + ctx->pdata->num_vregs_common, + ctx->pdata->vregs_common); +if (IS_ERR (ctx->regs_common)) +return dev_err_probe (dev,PTR_ERR (ctx->regs_common), +"Failed to get all regulators\n" ); + + ctx->regs_wlan =pwrseq_qca6390_get_regs (dev, + ctx->pdata->num_vregs_wlan, + ctx->pdata->vregs_wlan); +if (IS_ERR (ctx->regs_wlan)) +return dev_err_probe (dev,PTR_ERR (ctx->regs_wlan), +"Failed to get all regulators\n" ); + + ctx->bt_gpio =devm_gpiod_get_optional (dev,"bt-enable" ,GPIOD_OUT_LOW ); +if (IS_ERR (ctx->bt_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->bt_gpio), +"Failed to get the Bluetooth enable GPIO\n" ); + + ctx->wlan_gpio =devm_gpiod_get_optional (dev,"wlan-enable" , +GPIOD_OUT_LOW ); +if (IS_ERR (ctx->wlan_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->wlan_gpio), +"Failed to get the WLAN enable GPIO\n" ); + +memset (&config,0 ,sizeof (config)); + + config.parent = dev; + config.owner =THIS_MODULE ; + config.drvdata = ctx; + config.match = pwrseq_qca6390_match; + config.targets = pwrseq_qca6390_targets; + + ctx->pwrseq =devm_pwrseq_device_register (dev, &config); +if (IS_ERR (ctx->pwrseq)) +return dev_err_probe (dev,PTR_ERR (ctx->pwrseq), +"Failed to register the power sequencer\n" ); + +return 0 ; +} + +static const struct of_device_id pwrseq_qca6390_of_match[] = { + { + .compatible ="qcom,qca6390-pmu" , + .data = &pwrseq_qca6390_of_data, + }, + { } +}; +MODULE_DEVICE_TABLE (of, pwrseq_qca6390_of_match); + +static struct platform_driver pwrseq_qca6390_driver = { + .driver = { + .name ="pwrseq-qca6390" , + .of_match_table = pwrseq_qca6390_of_match, + }, + .probe = pwrseq_qca6390_probe, +}; +module_platform_driver (pwrseq_qca6390_driver); + +MODULE_AUTHOR ("Bartosz Golaszewski <[email protected]>" ); +MODULE_DESCRIPTION ("QCA6390 PMU power sequencing driver" ); +MODULE_LICENSE ("GPL" ); --2.40.1
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH v5 16/18] power: pwrseq: add a driver for the QCA6390 PMU module
Quoted 11 lines · Dmitry Baryshkov
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
This adds the power sequencing driver for the QCA6390's PMU module. It uses the pwrseq subsystem and knows how to match the sequencer to the consumer device by verifying the relevant properties and DT layout.
Signed-off-by Bartosz Golaszewski <[email protected]>---
[snip]
Quoted 34 lines
Quoted 34 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +34
-0+ +static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs_common[] = { + { + .name ="vddio" , + .load_uA =20000 , + }, + { + .name ="vddaon" , + .load_uA =100000 , + }, + { + .name ="vddpmu" , + .load_uA =1250000 , + }, + { + .name ="vddrfa0p95" , + .load_uA =200000 , + }, + { + .name ="vddrfa1p3" , + .load_uA =400000 , + }, + { + .name ="vddrfa1p9" , + .load_uA =400000 , + }, +}; + +static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs_wlan[] = { + { + .name ="vddpcie1p3" , + .load_uA =35000 , + }, + {
Quoted 9 lines
drivers/power/sequencing/pwrseq-qca6390.c line 79
Quoted 4 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +4
-0+ .name = "vddpcie1p9", + .load_uA = 15000, + }, +};I thought that we had discussed this already. According to the docs, all PMU supplies should be powered on when the chip is being switched on, no matter whether it is for the WiFi or for the BT.
I know, I mostly did it to make Bjorn happy because he was adamant we don't need the PCIe regulators for BT and when I checked, it does work in practice so I thought: whatever. But indeed, the docs say otherwise. Noted for v6.
[snip]
Quoted 9 lines
drivers/power/sequencing/pwrseq-qca6390.c line 147
Quoted 4 lines
drivers/power/sequencing/pwrseq-qca6390.c new file +4
-0+ +static const struct pwrseq_unit_data pwrseq_qca6390_bt_unit_data = { + .name ="bluetooth-enable" , + .deps = pwrseq_qca6390_unit_deps,Can we call corresponding
regulator_bulkfunctions from bt and wlan enable/disable? This will save us from building the tree-like structures (and possible loops inside that tree).
Can we? Sure, but the dependency graph (yeah, we should enforce it to be acyclic) is what makes this code future-proof and allows it to avoid repeating calls in different targets.
[snip]
Bart
PATCH v5 17/18 Bluetooth: qca: use the power sequencer for QCA6390
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Use the pwrseq subsystem's consumer API to run the power-up sequence for the Bluetooth module of the QCA6390 package.
--- drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
drivers/bluetooth/hci_qca.c +31 -0
@@ -29,6 +29,7 @@#include <linux/of.h> #include <linux/acpi.h> #include <linux/platform_device.h> +#include <linux/pwrseq/consumer.h> #include <linux/regulator/consumer.h> #include <linux/serdev.h> #include <linux/mutex.h>
@@ -215,6 +216,7 @@ struct qca_power {struct regulator_bulk_data *vreg_bulk;int num_vregs;bool vregs_on; +struct pwrseq_desc *pwrseq; };struct qca_serdev {
@@ -1792,6 +1794,11 @@ static int qca_power_on(struct hci_dev *hdev) ret =qca_regulator_init (hu);break ; +case QCA_QCA6390 : + qcadev =serdev_device_get_drvdata (hu->serdev); + ret =pwrseq_power_on (qcadev->bt_power->pwrseq); +break ; +default : qcadev =serdev_device_get_drvdata (hu->serdev);if (qcadev->bt_en) {
@@ -2170,6 +2177,10 @@ static void qca_power_shutdown(struct hci_uart *hu) }break ; +case QCA_QCA6390 : +pwrseq_power_off (qcadev->bt_power->pwrseq); +break ; +default :gpiod_set_value_cansleep (qcadev->bt_en,0 ); }
@@ -2308,12 +2319,25 @@ static int qca_serdev_probe(struct serdev_device *serdev)case QCA_WCN6750 :case QCA_WCN6855 :case QCA_WCN7850 : +case QCA_QCA6390 : qcadev->bt_power =devm_kzalloc (&serdev->dev,sizeof (struct qca_power ),GFP_KERNEL );if (!qcadev->bt_power)return -ENOMEM ; +break ; +default : +break ; + } +switch (qcadev->btsoc_type) { +case QCA_WCN3988 : +case QCA_WCN3990 : +case QCA_WCN3991 : +case QCA_WCN3998 : +case QCA_WCN6750 : +case QCA_WCN6855 : +case QCA_WCN7850 : qcadev->bt_power->dev = &serdev->dev; err =qca_init_regulators (qcadev->bt_power, data->vregs, data->num_vregs);
@@ -2354,6 +2378,13 @@ static int qca_serdev_probe(struct serdev_device *serdev) }break ; +case QCA_QCA6390 : + qcadev->bt_power->pwrseq =devm_pwrseq_get (&serdev->dev, +"bluetooth" ); +if (IS_ERR (qcadev->bt_power->pwrseq)) +return PTR_ERR (qcadev->bt_power->pwrseq); + fallthrough; +default : qcadev->bt_en =devm_gpiod_get_optional (&serdev->dev,"enable" ,GPIOD_OUT_LOW );
Signature · 1 line
2.40.1
PATCH v5 18/18 PCI/pwrctl: add a PCI power control driver for power sequenced devices
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a PCI power control driver that's capable of correctly powering up devices using the power sequencing subsystem. The first user of this driver is the ath11k module on QCA6390.
--- drivers/pci/pwrctl/Kconfig | 9 +++ drivers/pci/pwrctl/Makefile | 1 + drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 84 ++++++++++++++++++++++++++ 3 files changed, 94 insertions(+) create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
drivers/pci/pwrctl/Kconfig +9 -0
@@ -5,6 +5,15 @@ menu "PCI Power control drivers" config PCI_PWRCTL tristate +config PCI_PWRCTL_PWRSEQ + tristate "PCI Power Control driver using the Power Sequencing subsystem" + select POWER_SEQUENCING + select PCI_PWRCTL + default m if (ATH11K_PCI && ARCH_QCOM) + help + Enable support for the PCI power control driver for device + drivers using the Power Sequencing subsystem. + config PCI_PWRCTL_WCN7850 tristate "PCI Power Control driver for WCN7850" select PCI_PWRCTL
drivers/pci/pwrctl/Makefile +1 -0
@@ -3,4 +3,5 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o pci-pwrctl-core-y := core.o +obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o obj-$(CONFIG_PCI_PWRCTL_WCN7850) += pci-pwrctl-wcn7850.o
drivers/pci/pwrctl/pci-pwrctl-pwrseq.c new file +84 -0
84 lines not on this page · raw message
Signature · 1 line
2.40.1
Quoted 95 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
Quoted 4 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 68 lines
For QCA6390 on RB5 we add the PMU node as a platform device. It consumes regulators and GPIOs from the host and exposed regulators consumer in turn by the BT and WLAN modules. This represents the internal structure of the package.
Patches 10/18-14/18:
These contain the bulk of the PCI changes for this series. We introduce a simple framework for powering up PCI devices before detecting them on the bus and the first user of this library in the form of the WCN7850 PCI power control driver.
The general approach is as follows: PCI devices that need special treatment before they can be powered up, scanned and bound to their PCI drivers must be described on the device-tree as child nodes of the PCI port node. These devices will be instantiated on the platform bus. They will in fact be generic platform devices with the compatible of the form used for PCI devices already upstream ("pci<vendor ID>,<device ID">). We add a new directory under
drivers/pci/pwrctl/that contains PCI pwrctl drivers. These drivers are platform drivers that will now be matched against the devices instantiated from port children just like any other platform pairs.Both the power control platform device *AND* the associated PCI device reuse the same OF node and have access to the same properties. The goal of the platform driver is to request and bring up any required resources and let the pwrctl framework know that it's now OK to rescan the bus and detect the devices. When the device is bound, we are notified about it by the PCI bus notifier event and can establish a device link between the power control device and the PCI device so that any future extension for power-management will already be able to work with the correct hierachy.
The reusing of the OF node is the reason for the small changes to the PCI OF core: as the bootloader can possibly leave the relevant regulators on before booting linux, the PCI device can be detected before its platform abstraction is probed. In this case, we find that device first and mark its OF node as reused. The pwrctl framework handles the opposite case (when the PCI device is detected only after the platform driver successfully enabled it).
Patches 15/18-16/18:
These add a relatively simple power sequencing subsystem and the first driver using it: the pwrseq module for the QCA6390 PMU.
For the record: Bjorn suggested a different solution: a regulator driver that would - based on which regulators are enabled by a consumer - enable relevant resources (drive the enable GPIOs) while respecting the HW-specific delays. This would however require significant and yet unprecised changed to the regulator subsystem as well as be an abuse of the regulator provider API akin to using the reset framework for power sequencing as proposed before.
Instead I'm proposing to add a subsystem that allows different devices to use a shared power sequence split into consumer-specific as well as common "units".
A power sequence provider driver registers a set of units with pwrseq core. Each unit can be enabled and disabled and contains an optional list of other units which must be enabled before it itself can be. A unit represents a discreet chunk of the power sequence.
It also registers a list of targets: a target is an abstraction wrapping a unit which allows consumers to tell pwrseq which unit they want to reach. Real-life example is the driver we're adding here: there's a set of common regulators, two PCIe-specific ones and two enable GPIOs: one for Bluetooth and one for WLAN.
Quoted 4 lines
The Bluetooth driver requests a descriptor to the power sequencer and names the target it wants to reach:
pwrseq = devm_pwrseq_get(dev, "bluetooth");
Is this target tied to the device or not? If not, this might become a limitation, if somebody installs two WiFi/BT modules to a single device.
Quoted 16 lines
The pwrseq core then knows that when the driver calls:
pwrseq_power_on(pwrseq);It must enable the "bluetooth-enable" unit but it depends on the "regulators-common" unit so this one is enabled first. The provider driver is also in charge of assuring an appropriate delay between enabling the BT and WLAN enable GPIOs. The WLAN-specific resources are handled by the "wlan-enable" unit and so are not enabled until the WLAN driver requests the "wlan" target to be powered on.
Another thing worth discussing is the way we associate the consumer with the relevant power sequencer. DT maintainers have expressed a discontent with the existing mmc pwrseq bindings and have NAKed an earlier initiative to introduce global pwrseq bindings to the kernel[1].
Quoted 6 lines
In this approach, we model the existing regulators and GPIOs in DT but the pwrseq subsystem requires each provider to provide a .match() callback. Whenever a consumer requests a power sequencer handle, we iterate over the list of pwrseq drivers and call .match() for each. It's up to the driver to verify in a platform-specific way whether it deals with its consumer and let the core pwrseq code know.
This looks really nice, it will allow us to migrate the BT driver to always use pwrseq instead of regulators without touching the DT.
Quoted 119 lines
The advantage of this over reusing the regulator or reset subsystem is that it's more generalized and can handle resources of all kinds as well as deal with any kind of power-on sequences: for instance, Qualcomm has a PCI switch they want a driver for but this switch requires enabling some resources first (PCI pwrctl) and then configuring the device over I2C (which can be handled by the pwrseq provider).
Patch 17/18:
This patch makes the Qualcomm Bluetooth driver get and use the power sequencer for QCA6390.
Patch 18/18:
While tiny, this patch is possibly the highlight of the entire series. It uses the two abstraction layers we introduced before to create an elegant power sequencing PCI power control driver and supports the ath11k module on QCA6390.
With this series we can now enable BT and WLAN on several new Qualcomm boards upstream.
I tested the series on RB5 while Neil tested it on sm8650-qrd and sm8550-qrd.
Best Regards,
Bartosz GolaszewskiIt's hard to list the changes between versions here as the scope changed significantly between each iteration and some versions were not even full series but rather RFCs for parts of the solution. For this reason, I'll only start listing changes starting from v6.
[*] This is what the docs say. In practice it seems that this delay can be ignored. However the subsequent model - QCA6490 - *does* require users to respect it, so the problem remains valid.
[1] https://lore.kernel.org/netdev/[email protected]/
Bartosz Golaszewski (15):
arm64: defconfig: enable ath12k as a module
dt-bindings: regulator: describe the PMU module of the QCA6390 package
dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
dt-bindings: new: wireless: qcom,ath11k: describe the ath11k onQCA6390dt-bindings: new: wireless: describe the ath12k PCI module
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: reuse the OF node for power controlled devices PCI/pwrctl: create platform devices for child OF nodes of the port
node
PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a power control driver for WCN7850
power: sequencing: implement the pwrseq core
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390 PCI/pwrctl: add a PCI power control driver for power sequenced devicesJonathan Cameron (1):
of: Addcleanup.hbased auto release via__free(device_node)markings.Neil Armstrong (2):
arm64: dts: qcom: sm8550-qrd: add the Wifi node
arm64: dts: qcom: sm8650-qrd: add the Wifi node.../net/bluetooth/qualcomm-bluetooth.yaml | 17 + .../net/wireless/qcom,ath11k-pci.yaml | 28 + .../net/wireless/qcom,ath12k-pci.yaml | 103 ++ .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 +++ MAINTAINERS | 8 + arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 123 +- 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 + arch/arm64/configs/defconfig | 1 + drivers/bluetooth/hci_qca.c | 31 + drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/of.c | 14 +- drivers/pci/probe.c | 2 + drivers/pci/pwrctl/Kconfig | 25 + drivers/pci/pwrctl/Makefile | 7 + drivers/pci/pwrctl/core.c | 136 +++ drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 84 ++ drivers/pci/pwrctl/pci-pwrctl-wcn7850.c | 202 ++++ drivers/pci/remove.c | 2 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 28 + drivers/power/sequencing/Makefile | 6 + drivers/power/sequencing/core.c | 1065 +++++++++++++++++ drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++ include/linux/of.h | 2 + include/linux/pci-pwrctl.h | 51 + include/linux/pwrseq/consumer.h | 56 + include/linux/pwrseq/provider.h | 75 ++ 34 files changed, 2678 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml create mode 100644 drivers/pci/pwrctl/Kconfig create mode 100644 drivers/pci/pwrctl/Makefile create mode 100644 drivers/pci/pwrctl/core.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-wcn7850.c create mode 100644 drivers/power/sequencing/Kconfig create mode 100644 drivers/power/sequencing/Makefile create mode 100644 drivers/power/sequencing/core.c create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c create mode 100644 include/linux/pci-pwrctl.h create mode 100644 include/linux/pwrseq/consumer.h create mode 100644 include/linux/pwrseq/provider.h--
2.40.1
Signature · 2 lines
With best wishes Dmitry
Quoted 101 lines · Dmitry Baryshkov
Quoted 95 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
Quoted 4 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
Quoted 4 lines
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
Quoted 225 lines
Quoted 69 lines
For QCA6390 on RB5 we add the PMU node as a platform device. It consumes regulators and GPIOs from the host and exposed regulators consumer in turn by the BT and WLAN modules. This represents the internal structure of the package.
Patches 10/18-14/18:
These contain the bulk of the PCI changes for this series. We introduce a simple framework for powering up PCI devices before detecting them on the bus and the first user of this library in the form of the WCN7850 PCI power control driver.
The general approach is as follows: PCI devices that need special treatment before they can be powered up, scanned and bound to their PCI drivers must be described on the device-tree as child nodes of the PCI port node. These devices will be instantiated on the platform bus. They will in fact be generic platform devices with the compatible of the form used for PCI devices already upstream ("pci<vendor ID>,<device ID">). We add a new directory under
drivers/pci/pwrctl/that contains PCI pwrctl drivers. These drivers are platform drivers that will now be matched against the devices instantiated from port children just like any other platform pairs.Both the power control platform device *AND* the associated PCI device reuse the same OF node and have access to the same properties. The goal of the platform driver is to request and bring up any required resources and let the pwrctl framework know that it's now OK to rescan the bus and detect the devices. When the device is bound, we are notified about it by the PCI bus notifier event and can establish a device link between the power control device and the PCI device so that any future extension for power-management will already be able to work with the correct hierachy.
The reusing of the OF node is the reason for the small changes to the PCI OF core: as the bootloader can possibly leave the relevant regulators on before booting linux, the PCI device can be detected before its platform abstraction is probed. In this case, we find that device first and mark its OF node as reused. The pwrctl framework handles the opposite case (when the PCI device is detected only after the platform driver successfully enabled it).
Patches 15/18-16/18:
These add a relatively simple power sequencing subsystem and the first driver using it: the pwrseq module for the QCA6390 PMU.
For the record: Bjorn suggested a different solution: a regulator driver that would - based on which regulators are enabled by a consumer - enable relevant resources (drive the enable GPIOs) while respecting the HW-specific delays. This would however require significant and yet unprecised changed to the regulator subsystem as well as be an abuse of the regulator provider API akin to using the reset framework for power sequencing as proposed before.
Instead I'm proposing to add a subsystem that allows different devices to use a shared power sequence split into consumer-specific as well as common "units".
A power sequence provider driver registers a set of units with pwrseq core. Each unit can be enabled and disabled and contains an optional list of other units which must be enabled before it itself can be. A unit represents a discreet chunk of the power sequence.
It also registers a list of targets: a target is an abstraction wrapping a unit which allows consumers to tell pwrseq which unit they want to reach. Real-life example is the driver we're adding here: there's a set of common regulators, two PCIe-specific ones and two enable GPIOs: one for Bluetooth and one for WLAN.
Quoted 4 lines
The Bluetooth driver requests a descriptor to the power sequencer and names the target it wants to reach:
pwrseq = devm_pwrseq_get(dev, "bluetooth");Is this target tied to the device or not? If not, this might become a limitation, if somebody installs two WiFi/BT modules to a single device.
Quoted 16 lines
The pwrseq core then knows that when the driver calls:
pwrseq_power_on(pwrseq);It must enable the "bluetooth-enable" unit but it depends on the "regulators-common" unit so this one is enabled first. The provider driver is also in charge of assuring an appropriate delay between enabling the BT and WLAN enable GPIOs. The WLAN-specific resources are handled by the "wlan-enable" unit and so are not enabled until the WLAN driver requests the "wlan" target to be powered on.
Another thing worth discussing is the way we associate the consumer with the relevant power sequencer. DT maintainers have expressed a discontent with the existing mmc pwrseq bindings and have NAKed an earlier initiative to introduce global pwrseq bindings to the kernel[1].
Quoted 6 lines
In this approach, we model the existing regulators and GPIOs in DT but the pwrseq subsystem requires each provider to provide a .match() callback. Whenever a consumer requests a power sequencer handle, we iterate over the list of pwrseq drivers and call .match() for each. It's up to the driver to verify in a platform-specific way whether it deals with its consumer and let the core pwrseq code know.
This looks really nice, it will allow us to migrate the BT driver to always use pwrseq instead of regulators without touching the DT.
Quoted 119 lines
The advantage of this over reusing the regulator or reset subsystem is that it's more generalized and can handle resources of all kinds as well as deal with any kind of power-on sequences: for instance, Qualcomm has a PCI switch they want a driver for but this switch requires enabling some resources first (PCI pwrctl) and then configuring the device over I2C (which can be handled by the pwrseq provider).
Patch 17/18:
This patch makes the Qualcomm Bluetooth driver get and use the power sequencer for QCA6390.
Patch 18/18:
While tiny, this patch is possibly the highlight of the entire series. It uses the two abstraction layers we introduced before to create an elegant power sequencing PCI power control driver and supports the ath11k module on QCA6390.
With this series we can now enable BT and WLAN on several new Qualcomm boards upstream.
I tested the series on RB5 while Neil tested it on sm8650-qrd and sm8550-qrd.
Best Regards,
Bartosz GolaszewskiIt's hard to list the changes between versions here as the scope changed significantly between each iteration and some versions were not even full series but rather RFCs for parts of the solution. For this reason, I'll only start listing changes starting from v6.
[*] This is what the docs say. In practice it seems that this delay can be ignored. However the subsequent model - QCA6490 - *does* require users to respect it, so the problem remains valid.
[1] https://lore.kernel.org/netdev/[email protected]/
Bartosz Golaszewski (15):
arm64: defconfig: enable ath12k as a module
dt-bindings: regulator: describe the PMU module of the QCA6390 package
dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
dt-bindings: new: wireless: qcom,ath11k: describe the ath11k onQCA6390dt-bindings: new: wireless: describe the ath12k PCI module
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: reuse the OF node for power controlled devices PCI/pwrctl: create platform devices for child OF nodes of the port
node
PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a power control driver for WCN7850
power: sequencing: implement the pwrseq core
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390 PCI/pwrctl: add a PCI power control driver for power sequenced devicesJonathan Cameron (1):
of: Addcleanup.hbased auto release via__free(device_node)markings.Neil Armstrong (2):
arm64: dts: qcom: sm8550-qrd: add the Wifi node
arm64: dts: qcom: sm8650-qrd: add the Wifi node.../net/bluetooth/qualcomm-bluetooth.yaml | 17 + .../net/wireless/qcom,ath11k-pci.yaml | 28 + .../net/wireless/qcom,ath12k-pci.yaml | 103 ++ .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 +++ MAINTAINERS | 8 + arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 123 +- 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 + arch/arm64/configs/defconfig | 1 + drivers/bluetooth/hci_qca.c | 31 + drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/of.c | 14 +- drivers/pci/probe.c | 2 + drivers/pci/pwrctl/Kconfig | 25 + drivers/pci/pwrctl/Makefile | 7 + drivers/pci/pwrctl/core.c | 136 +++ drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 84 ++ drivers/pci/pwrctl/pci-pwrctl-wcn7850.c | 202 ++++ drivers/pci/remove.c | 2 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 28 + drivers/power/sequencing/Makefile | 6 + drivers/power/sequencing/core.c | 1065 +++++++++++++++++ drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++ include/linux/of.h | 2 + include/linux/pci-pwrctl.h | 51 + include/linux/pwrseq/consumer.h | 56 + include/linux/pwrseq/provider.h | 75 ++ 34 files changed, 2678 insertions(+), 16 deletions(-)create mode 100644
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yamlcreate mode 100644Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yamlcreate mode 100644drivers/pci/pwrctl/Kconfig
create mode 100644drivers/pci/pwrctl/Makefile
create mode 100644drivers/pci/pwrctl/core.c
create mode 100644drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
create mode 100644drivers/pci/pwrctl/pci-pwrctl-wcn7850.c
create mode 100644drivers/power/sequencing/Kconfig
create mode 100644drivers/power/sequencing/Makefile
create mode 100644drivers/power/sequencing/core.c
create mode 100644drivers/power/sequencing/pwrseq-qca6390.c
create mode 100644include/linux/pci-pwrctl.h
create mode 100644include/linux/pwrseq/consumer.h
create mode 100644include/linux/pwrseq/provider.h--
2.40.1
Quoted 103 lines · Neil Armstrong
Quoted 101 lines · Dmitry Baryshkov
Quoted 99 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
Quoted 8 lines
Quoted 4 lines
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
Signature · 2 lines
With best wishes Dmitry
Quoted 109 lines · Dmitry Baryshkov
Quoted 108 lines · Neil Armstrong
Quoted 105 lines · Dmitry Baryshkov
Quoted 95 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
Quoted 4 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the WLAN_EN and BT_EN ordering and the only requirement is to have all input regulators up before pulling up WLAN_EN and/or BT_EN.
This makes the PMU transparent and BT and WLAN can be described as independent.
Neil
Quoted 120 lines · On Mon, 19 Feb 2024 at 11:42, <[email protected]> wrote:
Quoted 109 lines · Dmitry Baryshkov
Quoted 108 lines · Neil Armstrong
Quoted 105 lines · Dmitry Baryshkov
Quoted 95 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
Quoted 4 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
Quoted 5 lines
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So the device tree should have the same style as the previous generations.
Signature · 2 lines
With best wishes Dmitry
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.
Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So the device tree should have the same style as the previous generations.
My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Bartosz
Quoted 38 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So the device tree should have the same style as the previous generations.
Quoted 7 lines
My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
Signature · 2 lines
With best wishes Dmitry
Quoted 44 lines · Dmitry Baryshkov
Quoted 43 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
Quoted 6 lines
Quoted 2 lines
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
Neil
Quoted 7 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- The BT_EN / WLAN_EN pins are not connected to the PMU
So, not representing the PMU in the device tree is a simplification.
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
Signature · 2 lines
With best wishes Dmitry
Quoted 64 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
Quoted 8 lines
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMUSo, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
Bart
Quoted 12 lines
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
--
With best wishes
Dmitry
Quoted 73 lines · Bartosz Golaszewski
Quoted 70 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMU
Quoted 6 lines
Quoted 2 lines
So, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
We have bindings that are not reflecting the hardware. So yes, we should gradually update them once the powerseq is merged.
Quoted 2 lines
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
So do WCN6855 and QCA6391 BlueTooth parts.
Quoted 15 lines
Bart
Quoted 12 lines
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
--
With best wishes
Dmitry
Signature · 2 lines
With best wishes Dmitry
Quoted 82 lines · Dmitry Baryshkov
Quoted 73 lines · Bartosz Golaszewski
Quoted 70 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMUQuoted 6 lines
Quoted 2 lines
So, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
Quoted 8 lines
We have bindings that are not reflecting the hardware. So yes, we should gradually update them once the powerseq is merged.
Quoted 2 lines
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
So do WCN6855 and QCA6391 BlueTooth parts.
That is not true for the latter, this series is adding regulators for it.
Bart
Quoted 21 lines
Quoted 15 lines
Bart
Quoted 12 lines
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
--
With best wishes
Dmitry--
With best wishes
Dmitry
Quoted 88 lines · Bartosz Golaszewski
Quoted 85 lines · Dmitry Baryshkov
Quoted 73 lines · Bartosz Golaszewski
Quoted 70 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not
necessary. The BT and WLAN devices on the device-tree are represented as
consuming the inputs (relevant to the functionality of each) of the PMU
directly.We are describing the hardware. From the hardware point of view, there is a PMU. I think at some point we would really like to describe all Qualcomm/Atheros WiFI+BT units using this PMU approach, including the older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).
Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMUQuoted 6 lines
Quoted 2 lines
So, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
We have bindings that are not reflecting the hardware. So yes, we should gradually update them once the powerseq is merged.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
So do WCN6855 and QCA6391 BlueTooth parts.
That is not true for the latter, this series is adding regulators for it.
But the bindings exist already, so you still have to extend it, deprecating regulator-less bindings.
Bartosz, I really don't understand what is the issue there. There is a PMU. As such it should be represented in the DT and it can be handled by the same driver as you are adding for QCA6390.
Quoted 24 lines
Bart
Quoted 21 lines
Quoted 15 lines
Bart
Quoted 12 lines
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
--
With best wishes
Dmitry--
With best wishes
Dmitry
Signature · 2 lines
With best wishes Dmitry
Quoted 96 lines · Dmitry Baryshkov
Quoted 94 lines · Bartosz Golaszewski
Quoted 90 lines · Dmitry Baryshkov
Quoted 73 lines · Bartosz Golaszewski
Quoted 70 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not
necessary. The BT and WLAN devices on the device-tree are represented as
consuming the inputs (relevant to the functionality of each) of the PMU
directly.We are describing the hardware. From the hardware point of view, there
is a PMU. I think at some point we would really like to describe all
Qualcomm/Atheros WiFI+BT units using this PMU approach, including the
older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for WCN7850 since BT+WiFi are now designed to be fully independent and PMU is transparent.
I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMUQuoted 6 lines
Quoted 2 lines
So, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
We have bindings that are not reflecting the hardware. So yes, we should gradually update them once the powerseq is merged.
Quoted 2 lines
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
So do WCN6855 and QCA6391 BlueTooth parts.
Quoted 9 lines
Quoted 1 line
That is not true for the latter, this series is adding regulators for it.
But the bindings exist already, so you still have to extend it, deprecating regulator-less bindings.
Bartosz, I really don't understand what is the issue there. There is a PMU. As such it should be represented in the DT and it can be handled by the same driver as you are adding for QCA6390.
The issue is that we'll pull in the pwrseq subsystem for WCN7850 which clearly does not require it in practice.
I'd like to hear Krzysztof, Conor or Rob chime in here and make the decision on how to proceed.
Bart
Quoted 30 lines
Quoted 24 lines
Bart
Quoted 21 lines
Quoted 15 lines
Bart
Quoted 12 lines
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
--
With best wishes
Dmitry--
With best wishes
Dmitry--
With best wishes
Dmitry
Quoted 108 lines · Bartosz Golaszewski
Quoted 96 lines · Dmitry Baryshkov
Quoted 94 lines · Bartosz Golaszewski
Quoted 90 lines · Dmitry Baryshkov
Quoted 73 lines · Bartosz Golaszewski
Quoted 70 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not
necessary. The BT and WLAN devices on the device-tree are represented as
consuming the inputs (relevant to the functionality of each) of the PMU
directly.We are describing the hardware. From the hardware point of view, there
is a PMU. I think at some point we would really like to describe all
Qualcomm/Atheros WiFI+BT units using this PMU approach, including the
older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for
WCN7850 since BT+WiFi are now designed to be fully independent and PMU is
transparent.I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMUQuoted 6 lines
Quoted 2 lines
So, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
We have bindings that are not reflecting the hardware. So yes, we should gradually update them once the powerseq is merged.
Quoted 2 lines
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
So do WCN6855 and QCA6391 BlueTooth parts.
Quoted 9 lines
Quoted 1 line
That is not true for the latter, this series is adding regulators for it.
But the bindings exist already, so you still have to extend it, deprecating regulator-less bindings.
Bartosz, I really don't understand what is the issue there. There is a PMU. As such it should be represented in the DT and it can be handled by the same driver as you are adding for QCA6390.
Quoted 5 lines
The issue is that we'll pull in the pwrseq subsystem for WCN7850 which clearly does not require it in practice.
I'd like to hear Krzysztof, Conor or Rob chime in here and make the decision on how to proceed.
For WCN7850 we'll be describing the PMU which is basically discrete, so we could also add dummy fixed regulators, and it would be the same.
Neil
Quoted 33 lines
Bart
Quoted 30 lines
Quoted 24 lines
Bart
Quoted 21 lines
Quoted 15 lines
Bart
Quoted 12 lines
Quoted 8 lines
Neil
Quoted 5 lines
Moreover, I think we definitely want to move BT driver to use only the pwrseq power up method. Doing it in the other way results in the code duplication and possible issues because of the regulator / pwrseq taking different code paths.
--
With best wishes
Dmitry--
With best wishes
Dmitry--
With best wishes
Dmitry
Quoted 108 lines · Bartosz Golaszewski
Quoted 96 lines · Dmitry Baryshkov
Quoted 94 lines · Bartosz Golaszewski
Quoted 90 lines · Dmitry Baryshkov
Quoted 73 lines · Bartosz Golaszewski
Quoted 70 lines · Dmitry Baryshkov
Quoted 49 lines · On Mon, 19 Feb 2024 at 19:18, <[email protected]> wrote:
Quoted 47 lines · Dmitry Baryshkov
Quoted 45 lines · Bartosz Golaszewski
Quoted 1 line · Dmitry Baryshkov
[snip]
Quoted 24 lines
Quoted 24 lines
Quoted 11 lines
Quoted 11 lines
For WCN7850 we hide the existence of the PMU as modeling it is simply not
necessary. The BT and WLAN devices on the device-tree are represented as
consuming the inputs (relevant to the functionality of each) of the PMU
directly.We are describing the hardware. From the hardware point of view, there
is a PMU. I think at some point we would really like to describe all
Qualcomm/Atheros WiFI+BT units using this PMU approach, including the
older ath10k units present on RB3 (WCN3990) and db820c (QCA6174).Quoted 8 lines
Quoted 3 lines
While I agree with older WiFi+BT units, I don't think it's needed for
WCN7850 since BT+WiFi are now designed to be fully independent and PMU is
transparent.I don't see any significant difference between WCN6750/WCN6855 and WCN7850 from the PMU / power up point of view. Could you please point me to the difference?
The WCN7850 datasheet clearly states there's not contraint on the
WLAN_ENandBT_ENordering and the only requirement is to have all input regulators up before pulling upWLAN_ENand/orBT_EN.Quoted 6 lines
Quoted 1 line
This makes the PMU transparent and BT and WLAN can be described as independent.
From the hardware perspective, there is a PMU. It has several LDOs. So
the device tree should have the same style as the previous generations.My thinking was this: yes, there is a PMU but describing it has no benefit (unlike QCA6x90). If we do describe, then we'll end up having to use pwrseq here despite it not being needed because now we won't be able to just get regulators from WLAN/BT drivers directly.
So I also vote for keeping it this way. Let's go into the package detail only if it's required.
Quoted 7 lines
Quoted 3 lines
The WiFi / BT parts are not powered up by the board regulators. They are powered up by the PSU. So we are not describing it in the accurate way.
I disagree, the WCN7850 can also be used as a discrete PCIe M.2 card, and in this situation the PCIe part is powered with the M.2 slot and the BT side is powered separately as we currently do it now.
QCA6390 can also be used as a discrete M.2 card.
Quoted 2 lines
So yes there's a PMU, but it's not an always visible hardware part, from the SoC PoV, only the separate PCIe and BT subsystems are visible/controllable/powerable.
From the hardware point:
- There is a PMU
- The PMU is connected to the board supplies
- Both WiFi and BT parts are connected to the PMU
- TheBT_EN/WLAN_ENpins are not connected to the PMUQuoted 6 lines
Quoted 2 lines
So, not representing the PMU in the device tree is a simplification.
What about the existing WLAN and BT users of similar packages? We would have to deprecate a lot of existing bindings. I don't think it's worth it.
We have bindings that are not reflecting the hardware. So yes, we should gradually update them once the powerseq is merged.
Quoted 2 lines
The WCN7850 is already described in bindings as consuming what is PMUs inputs and not its outputs.
So do WCN6855 and QCA6391 BlueTooth parts.
Quoted 9 lines
Quoted 1 line
That is not true for the latter, this series is adding regulators for it.
But the bindings exist already, so you still have to extend it, deprecating regulator-less bindings.
Bartosz, I really don't understand what is the issue there. There is a PMU. As such it should be represented in the DT and it can be handled by the same driver as you are adding for QCA6390.
Quoted 5 lines
The issue is that we'll pull in the pwrseq subsystem for WCN7850 which clearly does not require it in practice.
I'd like to hear Krzysztof, Conor or Rob chime in here and make the decision on how to proceed.
There's like 12 emails here, so please don't just point "MR X, please read everything to find the question I want to ask", but just ask the question with short intro. We all (and I bet you as well) are way too busy to read long threads...
If I got it correctly, you ask if some other, existing QCA/WCN chips should be changed to this PMU approach?
If yes, then:
1. It depends whether they have the PMU, so some sort of analysis of datasheet should be done.
2. You could but you don't have to. Bindings were done, they represent the hardware more-or-less, maybe less, but still good enough.
3. It does not have to impact actual behavior of Linux. You don't have to bind entire pwrseq driver to that QCA/WCN compatible. Anyway Linux behavior is here a bit separate question - it can change, it can stay the same, up to you.
Best regards,
Krzysztof
Quoted 295 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
For QCA6390 on RB5 we add the PMU node as a platform device. It consumes regulators and GPIOs from the host and exposed regulators consumer in turn by the BT and WLAN modules. This represents the internal structure of the package.
Patches 10/18-14/18:
These contain the bulk of the PCI changes for this series. We introduce a simple framework for powering up PCI devices before detecting them on the bus and the first user of this library in the form of the WCN7850 PCI power control driver.
The general approach is as follows: PCI devices that need special treatment before they can be powered up, scanned and bound to their PCI drivers must be described on the device-tree as child nodes of the PCI port node. These devices will be instantiated on the platform bus. They will in fact be generic platform devices with the compatible of the form used for PCI devices already upstream ("pci<vendor ID>,<device ID">). We add a new directory under
drivers/pci/pwrctl/that contains PCI pwrctl drivers. These drivers are platform drivers that will now be matched against the devices instantiated from port children just like any other platform pairs.Both the power control platform device *AND* the associated PCI device reuse the same OF node and have access to the same properties. The goal of the platform driver is to request and bring up any required resources and let the pwrctl framework know that it's now OK to rescan the bus and detect the devices. When the device is bound, we are notified about it by the PCI bus notifier event and can establish a device link between the power control device and the PCI device so that any future extension for power-management will already be able to work with the correct hierachy.
The reusing of the OF node is the reason for the small changes to the PCI OF core: as the bootloader can possibly leave the relevant regulators on before booting linux, the PCI device can be detected before its platform abstraction is probed. In this case, we find that device first and mark its OF node as reused. The pwrctl framework handles the opposite case (when the PCI device is detected only after the platform driver successfully enabled it).
Patches 15/18-16/18:
These add a relatively simple power sequencing subsystem and the first driver using it: the pwrseq module for the QCA6390 PMU.
For the record: Bjorn suggested a different solution: a regulator driver that would - based on which regulators are enabled by a consumer - enable relevant resources (drive the enable GPIOs) while respecting the HW-specific delays. This would however require significant and yet unprecised changed to the regulator subsystem as well as be an abuse of the regulator provider API akin to using the reset framework for power sequencing as proposed before.
Instead I'm proposing to add a subsystem that allows different devices to use a shared power sequence split into consumer-specific as well as common "units".
A power sequence provider driver registers a set of units with pwrseq core. Each unit can be enabled and disabled and contains an optional list of other units which must be enabled before it itself can be. A unit represents a discreet chunk of the power sequence.
It also registers a list of targets: a target is an abstraction wrapping a unit which allows consumers to tell pwrseq which unit they want to reach. Real-life example is the driver we're adding here: there's a set of common regulators, two PCIe-specific ones and two enable GPIOs: one for Bluetooth and one for WLAN.
The Bluetooth driver requests a descriptor to the power sequencer and names the target it wants to reach:
pwrseq = devm_pwrseq_get(dev, "bluetooth");The pwrseq core then knows that when the driver calls:
pwrseq_power_on(pwrseq);It must enable the "bluetooth-enable" unit but it depends on the "regulators-common" unit so this one is enabled first. The provider driver is also in charge of assuring an appropriate delay between enabling the BT and WLAN enable GPIOs. The WLAN-specific resources are handled by the "wlan-enable" unit and so are not enabled until the WLAN driver requests the "wlan" target to be powered on.
Another thing worth discussing is the way we associate the consumer with the relevant power sequencer. DT maintainers have expressed a discontent with the existing mmc pwrseq bindings and have NAKed an earlier initiative to introduce global pwrseq bindings to the kernel[1].
In this approach, we model the existing regulators and GPIOs in DT but the pwrseq subsystem requires each provider to provide a .match() callback. Whenever a consumer requests a power sequencer handle, we iterate over the list of pwrseq drivers and call .match() for each. It's up to the driver to verify in a platform-specific way whether it deals with its consumer and let the core pwrseq code know.
The advantage of this over reusing the regulator or reset subsystem is that it's more generalized and can handle resources of all kinds as well as deal with any kind of power-on sequences: for instance, Qualcomm has a PCI switch they want a driver for but this switch requires enabling some resources first (PCI pwrctl) and then configuring the device over I2C (which can be handled by the pwrseq provider).
Patch 17/18:
This patch makes the Qualcomm Bluetooth driver get and use the power sequencer for QCA6390.
Patch 18/18:
While tiny, this patch is possibly the highlight of the entire series. It uses the two abstraction layers we introduced before to create an elegant power sequencing PCI power control driver and supports the ath11k module on QCA6390.
With this series we can now enable BT and WLAN on several new Qualcomm boards upstream.
I tested the series on RB5 while Neil tested it on sm8650-qrd and sm8550-qrd.
Best Regards,
Bartosz GolaszewskiIt's hard to list the changes between versions here as the scope changed significantly between each iteration and some versions were not even full series but rather RFCs for parts of the solution. For this reason, I'll only start listing changes starting from v6.
[*] This is what the docs say. In practice it seems that this delay can be ignored. However the subsequent model - QCA6490 - *does* require users to respect it, so the problem remains valid.
[1] https://lore.kernel.org/netdev/[email protected]/
Bartosz Golaszewski (15):
arm64: defconfig: enable ath12k as a module
dt-bindings: regulator: describe the PMU module of the QCA6390 package
dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
dt-bindings: new: wireless: qcom,ath11k: describe the ath11k onQCA6390dt-bindings: new: wireless: describe the ath12k PCI module
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: reuse the OF node for power controlled devices PCI/pwrctl: create platform devices for child OF nodes of the port
node
PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a power control driver for WCN7850
power: sequencing: implement the pwrseq core
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390 PCI/pwrctl: add a PCI power control driver for power sequenced devicesJonathan Cameron (1):
of: Addcleanup.hbased auto release via__free(device_node)markings.Neil Armstrong (2):
arm64: dts: qcom: sm8550-qrd: add the Wifi node
arm64: dts: qcom: sm8650-qrd: add the Wifi node.../net/bluetooth/qualcomm-bluetooth.yaml | 17 + .../net/wireless/qcom,ath11k-pci.yaml | 28 + .../net/wireless/qcom,ath12k-pci.yaml | 103 ++ .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 +++ MAINTAINERS | 8 + arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 123 +- 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 + arch/arm64/configs/defconfig | 1 + drivers/bluetooth/hci_qca.c | 31 + drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/of.c | 14 +- drivers/pci/probe.c | 2 + drivers/pci/pwrctl/Kconfig | 25 + drivers/pci/pwrctl/Makefile | 7 + drivers/pci/pwrctl/core.c | 136 +++ drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 84 ++ drivers/pci/pwrctl/pci-pwrctl-wcn7850.c | 202 ++++ drivers/pci/remove.c | 2 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 28 + drivers/power/sequencing/Makefile | 6 + drivers/power/sequencing/core.c | 1065 +++++++++++++++++ drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++ include/linux/of.h | 2 + include/linux/pci-pwrctl.h | 51 + include/linux/pwrseq/consumer.h | 56 + include/linux/pwrseq/provider.h | 75 ++ 34 files changed, 2678 insertions(+), 16 deletions(-)
Quoted 15 lines
create mode 100644
Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yamlcreate mode 100644Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yamlcreate mode 100644drivers/pci/pwrctl/Kconfig
create mode 100644drivers/pci/pwrctl/Makefile
create mode 100644drivers/pci/pwrctl/core.c
create mode 100644drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
create mode 100644drivers/pci/pwrctl/pci-pwrctl-wcn7850.c
create mode 100644drivers/power/sequencing/Kconfig
create mode 100644drivers/power/sequencing/Makefile
create mode 100644drivers/power/sequencing/core.c
create mode 100644drivers/power/sequencing/pwrseq-qca6390.c
create mode 100644include/linux/pci-pwrctl.h
create mode 100644include/linux/pwrseq/consumer.h
create mode 100644include/linux/pwrseq/provider.h
Thanks,
Neil
Quoted 215 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
First, I'd like to apologize for the somewhat chaotic previous iterations of this series and improper versioning which was rightfully pointed out to me. I figured that the scope changed so much that it didn't make sense to consider previous submissions part of the same series as the original RFC but others thought otherwise so this one becomes v5 and I'll keep the versioning going forward.
This is the summary of the work so far:
v1: Original RFC:
https://lore.kernel.org/lkml/[email protected]/T/
v2: First real patch series (should have been PATCH v2) adding what I
referred to back then as PCI power sequencing:https://lore.kernel.org/linux-arm-kernel/2024021413-grumbling-unlivable-c145@gregkh/T/
v3: RFC for the DT representation of the PMU supplying the WLAN and BT
modules inside the QCA6391 package (was largely separate from the series but probably should have been called PATCH or RFC v3):https://lore.kernel.org/all/CAMRc=Mc+GNoi57eTQg71DXkQKjdaoAmCpB=h2ndEpGnmdhVV-Q@mail.gmail.com/T/
v4: Second attempt at the full series with changed scope (introduction of
the pwrseq subsystem, should have been RFC v4)https://lore.kernel.org/lkml/[email protected]/T/
===
With that out of the way, I'd like to get down to explaining the two problems I'm trying to solve.
Problem statement #1: Dynamic bus chicken-and-egg problem.
Certain on-board PCI devices need to be powered up before they are can be detected but their PCI drivers won't get bound until the device is powered-up so enabling the relevant resources in the PCI device driver itself is impossible.
Problem statement #2: Sharing inter-dependent resources between devices.
Certain devices that use separate drivers (often on different busses) share resources (regulators, clocks, etc.). Typically these resources are reference-counted but in some cases there are additional interactions between them to consider, for example specific power-up sequence timings.
===
The reason for tackling both of these problems in a single series is the fact the the platform I'm working on - Qualcomm RB5 - deals with both and both need to be addressed in order to enable WLAN and Bluetooth support upstream.
The on-board WLAN/BT package - QCA6391 - has a Power Management Unit that takes inputs from the host and exposes LDO outputs consumed by the BT and WLAN modules which can be powered-up and down independently. However a delay of 100ms must be respected between enabling the BT- and WLAN-enable GPIOs[*].
===
This series is logically split into several sections. I'll go patch-by-patch and explain each step.
Patch 1/18:
This is a commit taken from the list by Jonathan Cameron that adds a
__free()helper for OF nodes. Not strictly related to the series but until said commit ends in next, I need to carry it with this series.Patch 2/18:
This enables the ath12k PCI module in arm64 defconfig as Qualcomm sm8650 and sm8550 reference platforms use it in the WCN7850 module.
Patches 3/18-6/18:
These contain all relevant DT bindings changes. We add new documents for the QCA6390 PMU and ATH12K devices as well as extend the bindings for the Qualcomm Bluetooth and ATH11K modules with regulators used by them in QCA6390.
Patches 7/18-9/18:
These contain changes to device-tree sources for the three platforms we work with in this series. As the WCN7850 module doesn't require any specific timings introducing dependencies between the Bluetooth and WLAN modules, while the QCA6390 does, we take two different approaches to how me model them in DT.
For WCN7850 we hide the existence of the PMU as modeling it is simply not necessary. The BT and WLAN devices on the device-tree are represented as consuming the inputs (relevant to the functionality of each) of the PMU directly.
For QCA6390 on RB5 we add the PMU node as a platform device. It consumes regulators and GPIOs from the host and exposed regulators consumer in turn by the BT and WLAN modules. This represents the internal structure of the package.
Patches 10/18-14/18:
These contain the bulk of the PCI changes for this series. We introduce a simple framework for powering up PCI devices before detecting them on the bus and the first user of this library in the form of the WCN7850 PCI power control driver.
The general approach is as follows: PCI devices that need special treatment before they can be powered up, scanned and bound to their PCI drivers must be described on the device-tree as child nodes of the PCI port node. These devices will be instantiated on the platform bus. They will in fact be generic platform devices with the compatible of the form used for PCI devices already upstream ("pci<vendor ID>,<device ID">). We add a new directory under
drivers/pci/pwrctl/that contains PCI pwrctl drivers. These drivers are platform drivers that will now be matched against the devices instantiated from port children just like any other platform pairs.Both the power control platform device *AND* the associated PCI device reuse the same OF node and have access to the same properties. The goal of the platform driver is to request and bring up any required resources and let the pwrctl framework know that it's now OK to rescan the bus and detect the devices. When the device is bound, we are notified about it by the PCI bus notifier event and can establish a device link between the power control device and the PCI device so that any future extension for power-management will already be able to work with the correct hierachy.
The reusing of the OF node is the reason for the small changes to the PCI OF core: as the bootloader can possibly leave the relevant regulators on before booting linux, the PCI device can be detected before its platform abstraction is probed. In this case, we find that device first and mark its OF node as reused. The pwrctl framework handles the opposite case (when the PCI device is detected only after the platform driver successfully enabled it).
Patches 15/18-16/18:
These add a relatively simple power sequencing subsystem and the first driver using it: the pwrseq module for the QCA6390 PMU.
For the record: Bjorn suggested a different solution: a regulator driver that would - based on which regulators are enabled by a consumer - enable relevant resources (drive the enable GPIOs) while respecting the HW-specific delays. This would however require significant and yet unprecised changed to the regulator subsystem as well as be an abuse of the regulator provider API akin to using the reset framework for power sequencing as proposed before.
Instead I'm proposing to add a subsystem that allows different devices to use a shared power sequence split into consumer-specific as well as common "units".
A power sequence provider driver registers a set of units with pwrseq core. Each unit can be enabled and disabled and contains an optional list of other units which must be enabled before it itself can be. A unit represents a discreet chunk of the power sequence.
It also registers a list of targets: a target is an abstraction wrapping a unit which allows consumers to tell pwrseq which unit they want to reach. Real-life example is the driver we're adding here: there's a set of common regulators, two PCIe-specific ones and two enable GPIOs: one for Bluetooth and one for WLAN.
The Bluetooth driver requests a descriptor to the power sequencer and names the target it wants to reach:
pwrseq = devm_pwrseq_get(dev, "bluetooth");The pwrseq core then knows that when the driver calls:
pwrseq_power_on(pwrseq);It must enable the "bluetooth-enable" unit but it depends on the "regulators-common" unit so this one is enabled first. The provider driver is also in charge of assuring an appropriate delay between enabling the BT and WLAN enable GPIOs. The WLAN-specific resources are handled by the "wlan-enable" unit and so are not enabled until the WLAN driver requests the "wlan" target to be powered on.
Another thing worth discussing is the way we associate the consumer with the relevant power sequencer. DT maintainers have expressed a discontent with the existing mmc pwrseq bindings and have NAKed an earlier initiative to introduce global pwrseq bindings to the kernel[1].
In this approach, we model the existing regulators and GPIOs in DT but the pwrseq subsystem requires each provider to provide a .match() callback. Whenever a consumer requests a power sequencer handle, we iterate over the list of pwrseq drivers and call .match() for each. It's up to the driver to verify in a platform-specific way whether it deals with its consumer and let the core pwrseq code know.
The advantage of this over reusing the regulator or reset subsystem is that it's more generalized and can handle resources of all kinds as well as deal with any kind of power-on sequences: for instance, Qualcomm has a PCI switch they want a driver for but this switch requires enabling some resources first (PCI pwrctl) and then configuring the device over I2C (which can be handled by the pwrseq provider).
Patch 17/18:
This patch makes the Qualcomm Bluetooth driver get and use the power sequencer for QCA6390.
Patch 18/18:
While tiny, this patch is possibly the highlight of the entire series. It uses the two abstraction layers we introduced before to create an elegant power sequencing PCI power control driver and supports the ath11k module on QCA6390.
Quoted 5 lines
With this series we can now enable BT and WLAN on several new Qualcomm boards upstream.
I tested the series on RB5 while Neil tested it on sm8650-qrd and sm8550-qrd.
Quoted 95 lines
Best Regards,
Bartosz GolaszewskiIt's hard to list the changes between versions here as the scope changed significantly between each iteration and some versions were not even full series but rather RFCs for parts of the solution. For this reason, I'll only start listing changes starting from v6.
[*] This is what the docs say. In practice it seems that this delay can be ignored. However the subsequent model - QCA6490 - *does* require users to respect it, so the problem remains valid.
[1] https://lore.kernel.org/netdev/[email protected]/
Bartosz Golaszewski (15):
arm64: defconfig: enable ath12k as a module
dt-bindings: regulator: describe the PMU module of the QCA6390 package
dt-bindings: net: bluetooth: qualcomm: describe regulators for QCA6390
dt-bindings: new: wireless: qcom,ath11k: describe the ath11k onQCA6390dt-bindings: new: wireless: describe the ath12k PCI module
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: reuse the OF node for power controlled devices PCI/pwrctl: create platform devices for child OF nodes of the port
node
PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a power control driver for WCN7850
power: sequencing: implement the pwrseq core
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390 PCI/pwrctl: add a PCI power control driver for power sequenced devicesJonathan Cameron (1):
of: Addcleanup.hbased auto release via__free(device_node)markings.Neil Armstrong (2):
arm64: dts: qcom: sm8550-qrd: add the Wifi node
arm64: dts: qcom: sm8650-qrd: add the Wifi node.../net/bluetooth/qualcomm-bluetooth.yaml | 17 + .../net/wireless/qcom,ath11k-pci.yaml | 28 + .../net/wireless/qcom,ath12k-pci.yaml | 103 ++ .../bindings/regulator/qcom,qca6390-pmu.yaml | 166 +++ MAINTAINERS | 8 + arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 123 +- 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 + arch/arm64/configs/defconfig | 1 + drivers/bluetooth/hci_qca.c | 31 + drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/of.c | 14 +- drivers/pci/probe.c | 2 + drivers/pci/pwrctl/Kconfig | 25 + drivers/pci/pwrctl/Makefile | 7 + drivers/pci/pwrctl/core.c | 136 +++ drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 84 ++ drivers/pci/pwrctl/pci-pwrctl-wcn7850.c | 202 ++++ drivers/pci/remove.c | 2 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 28 + drivers/power/sequencing/Makefile | 6 + drivers/power/sequencing/core.c | 1065 +++++++++++++++++ drivers/power/sequencing/pwrseq-qca6390.c | 353 ++++++ include/linux/of.h | 2 + include/linux/pci-pwrctl.h | 51 + include/linux/pwrseq/consumer.h | 56 + include/linux/pwrseq/provider.h | 75 ++ 34 files changed, 2678 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-pci.yaml create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml create mode 100644 drivers/pci/pwrctl/Kconfig create mode 100644 drivers/pci/pwrctl/Makefile create mode 100644 drivers/pci/pwrctl/core.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-wcn7850.c create mode 100644 drivers/power/sequencing/Kconfig create mode 100644 drivers/power/sequencing/Makefile create mode 100644 drivers/power/sequencing/core.c create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c create mode 100644 include/linux/pci-pwrctl.h create mode 100644 include/linux/pwrseq/consumer.h create mode 100644 include/linux/pwrseq/provider.h--
2.40.1