From: Bartosz Golaszewski <[email protected]>
During last year's Linux Plumbers we had several discussions centered
around the need to power-on PCI devices before they can be detected on
the bus.
The consensus during the conference was that we need to introduce a
class of "PCI slot drivers" that would handle the power-sequencing.
After some additional brain-storming with Manivannan and the realization
that the DT maintainers won't like adding any "fake" nodes not
representing actual devices, we decided to reuse the existing
infrastructure provided by the PCIe port drivers.
The general idea is to instantiate platform devices for child nodes of
the PCIe port DT node. For those nodes for which a power-sequencing
driver exists, we bind it and let it probe. The driver then triggers a
rescan of the PCI bus with the aim of detecting the now powered-on
device. The device will consume the same DT node as the platform,
power-sequencing device. We use device links to make the latter become
the parent of the former.
The main advantage of this approach is not modifying the existing DT in
any way and especially not adding any "fake" platform devices.
Bartosz Golaszewski (9):
arm64: dts: qcom: sm8250: describe the PCIe port
arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390
PCI/portdrv: create platform devices for child OF nodes
PCI: hold the rescan mutex when scanning for the first time
PCI/pwrseq: add pwrseq core code
dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros
dt-bindings: wireless: ath11k: describe QCA6390
PCI/pwrseq: add a pwrseq driver for QCA6390
arm64: defconfig: enable the PCIe power sequencing for QCA6390
.../net/wireless/qcom,ath11k-pci.yaml | 14 ++
.../devicetree/bindings/vendor-prefixes.yaml | 1 +
arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 24 +++
arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 +
arch/arm64/configs/defconfig | 2 +
drivers/pci/pcie/Kconfig | 2 +
drivers/pci/pcie/Makefile | 2 +
drivers/pci/pcie/portdrv.c | 3 +-
drivers/pci/pcie/pwrseq/Kconfig | 19 ++
drivers/pci/pcie/pwrseq/Makefile | 4 +
drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
drivers/pci/pcie/pwrseq/pwrseq.c | 83 ++++++++
drivers/pci/probe.c | 2 +
include/linux/pcie-pwrseq.h | 24 +++
14 files changed, 386 insertions(+), 1 deletion(-)
create mode 100644 drivers/pci/pcie/pwrseq/Kconfig
create mode 100644 drivers/pci/pcie/pwrseq/Makefile
create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
create mode 100644 drivers/pci/pcie/pwrseq/pwrseq.c
create mode 100644 include/linux/pcie-pwrseq.h
--
2.40.1
From: Bartosz Golaszewski <[email protected]>
Document the PCI vendor prefix for Qualcomm Atheros so that we can
define the QCA PCI devices on device tree.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 2dc098b39234..297d6037cd12 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1128,6 +1128,7 @@ patternProperties:
"^purism,.*":
description: Purism, SPC
"^qca,.*":
+ "^pci17cb,.*":
description: Qualcomm Atheros, Inc.
"^qcom,.*":
description: Qualcomm Technologies, Inc
--
2.40.1
From: Bartosz Golaszewski <[email protected]>
Add a PCIe power sequencing driver that's capable of correctly powering
up the ath11k module on QCA6390 using the PCIe pwrseq functionality.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/pci/pcie/pwrseq/Kconfig | 11 +
drivers/pci/pcie/pwrseq/Makefile | 1 +
drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
index 010e31f432c9..f9fe555b8506 100644
--- a/drivers/pci/pcie/pwrseq/Kconfig
+++ b/drivers/pci/pcie/pwrseq/Kconfig
@@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
help
Say yes here to enable support for PCIe power sequencing
drivers.
+
+if PCIE_PWRSEQ
+
+config PCIE_PWRSEQ_QCA6390
+ tristate "PCIe Power Sequencing driver for QCA6390"
+ depends on ARCH_QCOM || COMPILE_TEST
+ help
+ Enable support for the PCIe power sequencing driver for the
+ ath11k module of the QCA6390 WLAN/BT chip.
+
+endif
diff --git a/drivers/pci/pcie/pwrseq/Makefile b/drivers/pci/pcie/pwrseq/Makefile
index da99566594f6..da3e02063404 100644
--- a/drivers/pci/pcie/pwrseq/Makefile
+++ b/drivers/pci/pcie/pwrseq/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_PCIE_PWRSEQ) += pwrseq.o
+obj-$(CONFIG_PCIE_PWRSEQ_QCA6390) += pcie-pwrseq-qca6390.o
diff --git a/drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c b/drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
new file mode 100644
index 000000000000..e9fddbb642fe
--- /dev/null
+++ b/drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Linaro Ltd.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pcie-pwrseq.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct pcie_pwrseq_qca6390_vreg {
+ const char *name;
+ unsigned int load_uA;
+};
+
+struct pcie_pwrseq_qca6390_pdata {
+ struct pcie_pwrseq_qca6390_vreg *vregs;
+ size_t num_vregs;
+ unsigned int delay_msec;
+};
+
+struct pcie_pwrseq_qca6390_ctx {
+ struct pcie_pwrseq pwrseq;
+ const struct pcie_pwrseq_qca6390_pdata *pdata;
+ struct regulator_bulk_data *regs;
+ struct gpio_descs *en_gpios;
+ unsigned long *en_gpios_values;
+};
+
+static struct pcie_pwrseq_qca6390_vreg pcie_pwrseq_qca6390_vregs[] = {
+ {
+ .name = "vddpmu",
+ .load_uA = 1250000,
+ },
+ {
+ .name = "vddpcie1",
+ .load_uA = 35000,
+ },
+ {
+ .name = "vddpcie2",
+ .load_uA = 15000,
+ },
+};
+
+static struct pcie_pwrseq_qca6390_pdata pcie_pwrseq_qca6390_of_data = {
+ .vregs = pcie_pwrseq_qca6390_vregs,
+ .num_vregs = ARRAY_SIZE(pcie_pwrseq_qca6390_vregs),
+ .delay_msec = 16,
+};
+
+static int pcie_pwrseq_qca6390_power_on(struct pcie_pwrseq_qca6390_ctx *ctx)
+{
+ int ret;
+
+ ret = regulator_bulk_enable(ctx->pdata->num_vregs, ctx->regs);
+ if (ret)
+ return ret;
+
+ bitmap_fill(ctx->en_gpios_values, ctx->en_gpios->ndescs);
+
+ ret = gpiod_set_array_value_cansleep(ctx->en_gpios->ndescs,
+ ctx->en_gpios->desc,
+ ctx->en_gpios->info,
+ ctx->en_gpios_values);
+ if (ret) {
+ regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+ return ret;
+ }
+
+ if (ctx->pdata->delay_msec)
+ msleep(ctx->pdata->delay_msec);
+
+ return 0;
+}
+
+static int pcie_pwrseq_qca6390_power_off(struct pcie_pwrseq_qca6390_ctx *ctx)
+{
+ int ret;
+
+ bitmap_zero(ctx->en_gpios_values, ctx->en_gpios->ndescs);
+
+ ret = gpiod_set_array_value_cansleep(ctx->en_gpios->ndescs,
+ ctx->en_gpios->desc,
+ ctx->en_gpios->info,
+ ctx->en_gpios_values);
+ if (ret)
+ return ret;
+
+ return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+}
+
+static void devm_pcie_pwrseq_qca6390_power_off(void *data)
+{
+ struct pcie_pwrseq_qca6390_ctx *ctx = data;
+
+ pcie_pwrseq_qca6390_power_off(ctx);
+}
+
+static int pcie_pwrseq_qca6309_probe(struct platform_device *pdev)
+{
+ struct pcie_pwrseq_qca6390_ctx *ctx;
+ struct device *dev = &pdev->dev;
+ int ret, i;
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->pdata = of_device_get_match_data(dev);
+ if (!ctx->pdata)
+ return dev_err_probe(dev, -ENODEV,
+ "Failed to obtain platform data\n");
+
+ if (ctx->pdata->vregs) {
+ ctx->regs = devm_kcalloc(dev, ctx->pdata->num_vregs,
+ sizeof(*ctx->regs), GFP_KERNEL);
+ if (!ctx->regs)
+ return -ENOMEM;
+
+ for (i = 0; i < ctx->pdata->num_vregs; i++)
+ ctx->regs[i].supply = ctx->pdata->vregs[i].name;
+
+ ret = devm_regulator_bulk_get(dev, ctx->pdata->num_vregs,
+ ctx->regs);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to get all regulators\n");
+
+ for (i = 0; i < ctx->pdata->num_vregs; i++) {
+ ret = regulator_set_load(ctx->regs[i].consumer,
+ ctx->pdata->vregs[i].load_uA);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to set vreg load\n");
+ }
+ }
+
+ ctx->en_gpios = devm_gpiod_get_array_optional(dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->en_gpios))
+ return dev_err_probe(dev, PTR_ERR(ctx->en_gpios),
+ "Failed to get enable GPIOs\n");
+
+ ctx->en_gpios_values = devm_bitmap_zalloc(dev, ctx->en_gpios->ndescs,
+ GFP_KERNEL);
+ if (!ctx->en_gpios_values)
+ return -ENOMEM;
+
+ ret = pcie_pwrseq_qca6390_power_on(ctx);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to power on the device\n");
+
+ ret = devm_add_action_or_reset(dev, devm_pcie_pwrseq_qca6390_power_off,
+ ctx);
+ if (ret)
+ return ret;
+
+ ctx->pwrseq.dev = dev;
+
+ ret = devm_pcie_pwrseq_device_enable(dev, &ctx->pwrseq);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to register the pwrseq wrapper\n");
+
+ return 0;
+}
+
+static const struct of_device_id pcie_pwrseq_qca6309_of_match[] = {
+ {
+ .compatible = "pci17cb,1101",
+ .data = &pcie_pwrseq_qca6390_of_data,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, pcie_pwrseq_qca6309_of_match);
+
+static struct platform_driver pcie_pwrseq_qca6309_driver = {
+ .driver = {
+ .name = "pcie-pwrseq-qca6390",
+ .of_match_table = pcie_pwrseq_qca6309_of_match,
+ },
+ .probe = pcie_pwrseq_qca6309_probe,
+};
+module_platform_driver(pcie_pwrseq_qca6309_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("PCIe Power Sequencing module for QCA6390");
+MODULE_LICENSE("GPL");
--
2.40.1
Hi,
On 04/01/2024 14:01, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
>
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
>
> After some additional brain-storming with Manivannan and the realization
> that the DT maintainers won't like adding any "fake" nodes not
> representing actual devices, we decided to reuse the existing
> infrastructure provided by the PCIe port drivers.
>
> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. For those nodes for which a power-sequencing
> driver exists, we bind it and let it probe. The driver then triggers a
> rescan of the PCI bus with the aim of detecting the now powered-on
> device. The device will consume the same DT node as the platform,
> power-sequencing device. We use device links to make the latter become
> the parent of the former.
>
> The main advantage of this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.
I've successfully tested this serie for the WCN7850 Wifi/BT combo onboard chip
present on the SM8550-QRD and SM8650-QRD boards and it works just fine.
Here's a branch with the wcn7850 vreg table added to the pwrseq driver,
and the DT changes:
https://git.codelinaro.org/neil.armstrong/linux/-/commits/topic/sm8x50/wcn7850-wifi-pwrseq/?ref_type=heads
Thanks,
Neil
>
> Bartosz Golaszewski (9):
> arm64: dts: qcom: sm8250: describe the PCIe port
> arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390
> PCI/portdrv: create platform devices for child OF nodes
> PCI: hold the rescan mutex when scanning for the first time
> PCI/pwrseq: add pwrseq core code
> dt-bindings: vendor-prefixes: add a PCI prefix for Qualcomm Atheros
> dt-bindings: wireless: ath11k: describe QCA6390
> PCI/pwrseq: add a pwrseq driver for QCA6390
> arm64: defconfig: enable the PCIe power sequencing for QCA6390
>
> .../net/wireless/qcom,ath11k-pci.yaml | 14 ++
> .../devicetree/bindings/vendor-prefixes.yaml | 1 +
> arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 24 +++
> arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 +
> arch/arm64/configs/defconfig | 2 +
> drivers/pci/pcie/Kconfig | 2 +
> drivers/pci/pcie/Makefile | 2 +
> drivers/pci/pcie/portdrv.c | 3 +-
> drivers/pci/pcie/pwrseq/Kconfig | 19 ++
> drivers/pci/pcie/pwrseq/Makefile | 4 +
> drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c | 197 ++++++++++++++++++
> drivers/pci/pcie/pwrseq/pwrseq.c | 83 ++++++++
> drivers/pci/probe.c | 2 +
> include/linux/pcie-pwrseq.h | 24 +++
> 14 files changed, 386 insertions(+), 1 deletion(-)
> create mode 100644 drivers/pci/pcie/pwrseq/Kconfig
> create mode 100644 drivers/pci/pcie/pwrseq/Makefile
> create mode 100644 drivers/pci/pcie/pwrseq/pcie-pwrseq-qca6390.c
> create mode 100644 drivers/pci/pcie/pwrseq/pwrseq.c
> create mode 100644 include/linux/pcie-pwrseq.h
>
On 09/01/2024 03:56, Rob Herring wrote:
> On Mon, Jan 8, 2024 at 12:22 PM Bartosz Golaszewski <[email protected]> wrote:
>>
>> On Mon, Jan 8, 2024 at 8:10 PM Rob Herring <[email protected]> wrote:
>>>
>>> On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <[email protected]>
>>>>
>>>> Document the PCI vendor prefix for Qualcomm Atheros so that we can
>>>> define the QCA PCI devices on device tree.
>>>
>>> Why? vendor-prefixes.yaml is only applied to property names. 'qca'
>>> should be the prefix for those.
>>>
>>> Rob
>>
>> I didn't have any better idea. PCI devices on DT are defined by their
>> "pci<vendor ID>,<model ID>" compatible, not regular human-readable
>> strings and this makes checkpatch.pl complain.
>>
>> I'm open to suggestions.
>
> The checkpatch.pl check predates schemas and we could consider just
> dropping it. The only thing it provides is checking a patch rather
> than the tree (which the schema do). It's pretty hacky because it just
> greps the tree for a compatible string which is not entirely accurate.
> Also, we can extract an exact list of compatibles with
> "dt-extract-compatibles" which would make a better check, but I'm not
> sure making dtschema a dependency on checkpatch would be good.
>
> The other option is just ignore the warning. PCI compatibles are fairly rare.
Yep, the same warnings are for EEPROM and USB VID/PID compatibles, and
we live with these, so I don't think PCI should be treated differently.
Best regards,
Krzysztof
Jeff Johnson <[email protected]> writes:
> On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
>> index 010e31f432c9..f9fe555b8506 100644
>> --- a/drivers/pci/pcie/pwrseq/Kconfig
>> +++ b/drivers/pci/pcie/pwrseq/Kconfig
>> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>> help
>> Say yes here to enable support for PCIe power sequencing
>> drivers.
>> +
>> +if PCIE_PWRSEQ
>> +
>> +config PCIE_PWRSEQ_QCA6390
>> + tristate "PCIe Power Sequencing driver for QCA6390"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + help
>> + Enable support for the PCIe power sequencing driver for the
>> + ath11k module of the QCA6390 WLAN/BT chip.
>> +
>> +endif
>
> As I mentioned in the 5/9 patch I'm concerned that the current
> definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
> the fact that QCA6390 may need additional configuration since the menu
> item will only show up if you have already enabled PCIE_PWRSEQ.
> Yes I see that these are set in the defconfig in 9/9 but I'm concerned
> about the more generic case.
>
> I'm wondering if there should be a separate config QCA6390 within ath11k
> which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
Or is it possible to provide an optional dependency in Kconfig (I guess
not)? Or what about mentioning about PCIE_PWRSEQ_QCA6390 in ATH11K_PCI
help text?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, Jan 9, 2024 at 10:17 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 09/01/2024 03:56, Rob Herring wrote:
> > On Mon, Jan 8, 2024 at 12:22 PM Bartosz Golaszewski <brgl@bgdevpl> wrote:
> >>
> >> On Mon, Jan 8, 2024 at 8:10 PM Rob Herring <[email protected]> wrote:
> >>>
> >>> On Thu, Jan 04, 2024 at 02:01:20PM +0100, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski <[email protected]>
> >>>>
> >>>> Document the PCI vendor prefix for Qualcomm Atheros so that we can
> >>>> define the QCA PCI devices on device tree.
> >>>
> >>> Why? vendor-prefixes.yaml is only applied to property names. 'qca'
> >>> should be the prefix for those.
> >>>
> >>> Rob
> >>
> >> I didn't have any better idea. PCI devices on DT are defined by their
> >> "pci<vendor ID>,<model ID>" compatible, not regular human-readable
> >> strings and this makes checkpatch.pl complain.
> >>
> >> I'm open to suggestions.
> >
> > The checkpatch.pl check predates schemas and we could consider just
> > dropping it. The only thing it provides is checking a patch rather
> > than the tree (which the schema do). It's pretty hacky because it just
> > greps the tree for a compatible string which is not entirely accurate.
> > Also, we can extract an exact list of compatibles with
> > "dt-extract-compatibles" which would make a better check, but I'm not
> > sure making dtschema a dependency on checkpatch would be good.
> >
> > The other option is just ignore the warning. PCI compatibles are fairly rare.
>
> Yep, the same warnings are for EEPROM and USB VID/PID compatibles, and
> we live with these, so I don't think PCI should be treated differently.
>
Got it, I will drop this patch.
Bart
On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <[email protected]> wrote:
>
> Jeff Johnson <[email protected]> writes:
>
> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
> >> index 010e31f432c9..f9fe555b8506 100644
> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
> >> help
> >> Say yes here to enable support for PCIe power sequencing
> >> drivers.
> >> +
> >> +if PCIE_PWRSEQ
> >> +
> >> +config PCIE_PWRSEQ_QCA6390
> >> + tristate "PCIe Power Sequencing driver for QCA6390"
> >> + depends on ARCH_QCOM || COMPILE_TEST
> >> + help
> >> + Enable support for the PCIe power sequencing driver for the
> >> + ath11k module of the QCA6390 WLAN/BT chip.
> >> +
> >> +endif
> >
> > As I mentioned in the 5/9 patch I'm concerned that the current
> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
> > the fact that QCA6390 may need additional configuration since the menu
> > item will only show up if you have already enabled PCIE_PWRSEQ.
> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
> > about the more generic case.
> >
> > I'm wondering if there should be a separate config QCA6390 within ath11k
> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
>
> Or is it possible to provide an optional dependency in Kconfig (I guess
imply PCIE_PWRSEQ
imply PCIE_PWRSEQ_QCA6390
?
> not)? Or what about mentioning about PCIE_PWRSEQ_QCA6390 in ATH11K_PCI
> help text?
On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
> Chen-Yu Tsai <[email protected]> writes:
>> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <[email protected]> wrote:
>>> Jeff Johnson <[email protected]> writes:
>>>
>>> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
>>> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
>>> >> index 010e31f432c9..f9fe555b8506 100644
>>> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
>>> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
>>> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
>>> >> help
>>> >> Say yes here to enable support for PCIe power sequencing
>>> >> drivers.
>>> >> +
>>> >> +if PCIE_PWRSEQ
>>> >> +
>>> >> +config PCIE_PWRSEQ_QCA6390
>>> >> + tristate "PCIe Power Sequencing driver for QCA6390"
>>> >> + depends on ARCH_QCOM || COMPILE_TEST
>>> >> + help
>>> >> + Enable support for the PCIe power sequencing driver for the
>>> >> + ath11k module of the QCA6390 WLAN/BT chip.
>>> >> +
>>> >> +endif
>>> >
>>> > As I mentioned in the 5/9 patch I'm concerned that the current
>>> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
>>> > the fact that QCA6390 may need additional configuration since the menu
>>> > item will only show up if you have already enabled PCIE_PWRSEQ.
>>> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
>>> > about the more generic case.
>>> >
>>> > I'm wondering if there should be a separate config QCA6390 within ath11k
>>> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
>>>
>>> Or is it possible to provide an optional dependency in Kconfig (I guess
>>
>> imply PCIE_PWRSEQ
>> imply PCIE_PWRSEQ_QCA6390
>> ?
>
> Nice, I had forgotten imply altogether. Would 'imply
> PCIE_PWRSEQ_QCA6390' in ath11k Kconfig be enough to address Jeff's
> concern?
Please don't use imply (ever), it doesn't normally do
what you want. In this case, the only effect the
'imply' has is to change the default of the PCIE_PWRSEQ_QCA6390
option when a defconfig contains QCA6390.
If this is indeed what you want, it's still better to do the
equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
config PCIE_PWRSEQ_QCA6390
tristate "PCIe Power Sequencing driver for QCA6390"
default ATH11K && ARCH_QCOM
Arnd
On Tue, Jan 9, 2024 at 6:15 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
> > Chen-Yu Tsai <[email protected]> writes:
> >> On Tue, Jan 9, 2024 at 5:18 PM Kalle Valo <[email protected]> wrote:
> >>> Jeff Johnson <[email protected]> writes:
> >>>
> >>> > On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> >>> >> diff --git a/drivers/pci/pcie/pwrseq/Kconfig b/drivers/pci/pcie/pwrseq/Kconfig
> >>> >> index 010e31f432c9..f9fe555b8506 100644
> >>> >> --- a/drivers/pci/pcie/pwrseq/Kconfig
> >>> >> +++ b/drivers/pci/pcie/pwrseq/Kconfig
> >>> >> @@ -6,3 +6,14 @@ menuconfig PCIE_PWRSEQ
> >>> >> help
> >>> >> Say yes here to enable support for PCIe power sequencing
> >>> >> drivers.
> >>> >> +
> >>> >> +if PCIE_PWRSEQ
> >>> >> +
> >>> >> +config PCIE_PWRSEQ_QCA6390
> >>> >> + tristate "PCIe Power Sequencing driver for QCA6390"
> >>> >> + depends on ARCH_QCOM || COMPILE_TEST
> >>> >> + help
> >>> >> + Enable support for the PCIe power sequencing driver for the
> >>> >> + ath11k module of the QCA6390 WLAN/BT chip.
> >>> >> +
> >>> >> +endif
> >>> >
> >>> > As I mentioned in the 5/9 patch I'm concerned that the current
> >>> > definition of PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390 will effectively hide
> >>> > the fact that QCA6390 may need additional configuration since the menu
> >>> > item will only show up if you have already enabled PCIE_PWRSEQ.
> >>> > Yes I see that these are set in the defconfig in 9/9 but I'm concerned
> >>> > about the more generic case.
> >>> >
> >>> > I'm wondering if there should be a separate config QCA6390 within ath11k
> >>> > which would then select PCIE_PWRSEQ and PCIE_PWRSEQ_QCA6390
> >>>
> >>> Or is it possible to provide an optional dependency in Kconfig (I guess
> >>
> >> imply PCIE_PWRSEQ
> >> imply PCIE_PWRSEQ_QCA6390
> >> ?
> >
> > Nice, I had forgotten imply altogether. Would 'imply
> > PCIE_PWRSEQ_QCA6390' in ath11k Kconfig be enough to address Jeff's
> > concern?
>
> Please don't use imply (ever), it doesn't normally do
> what you want. In this case, the only effect the
> 'imply' has is to change the default of the PCIE_PWRSEQ_QCA6390
> option when a defconfig contains QCA6390.
>
> If this is indeed what you want, it's still better to do the
> equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
>
> config PCIE_PWRSEQ_QCA6390
> tristate "PCIe Power Sequencing driver for QCA6390"
> default ATH11K && ARCH_QCOM
PCIE_PWRSEQ_QCA6390 is also guarded by PCIE_PWRSEQ though. That would
require the default statement to be duplicated to the PCIE_PWRSEQ option
as well.
Presumably we'd get a few more power sequencing drivers, and the list of
default statements for PCIE_PWRSEQ would grow.
If that's acceptable then Arnd's proposal plus duplicating it to
PCIE_PWRSEQ should work as described.
ChenYu
On Tue, Jan 9, 2024, at 17:43, Kalle Valo wrote:
> "Arnd Bergmann" <[email protected]> writes:
>> On Tue, Jan 9, 2024, at 11:09, Kalle Valo wrote:
>>
>> If this is indeed what you want, it's still better to do the
>> equivalent expression in PCIE_PWRSEQ_QCA6390 rather than ATH11K:
>>
>> config PCIE_PWRSEQ_QCA6390
>> tristate "PCIe Power Sequencing driver for QCA6390"
>> default ATH11K && ARCH_QCOM
>
> Sounds good to me but should it be 'default ATH11K_PCI && ARCH_QCOM'? My
> understanding is that we don't need PWRSEQ for ATH11K_AHB devices.
Right, that is better.
Arnd