| Message ID | 20240528-pwrseq-v8-16-d354d52b763c@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | power: sequencing: implement the subsystem and add first users | expand |
On Tue, May 28, 2024 at 09:03:24PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a PCI power control driver that's capable of correctly powering up > devices using the power sequencing subsystem. The first users of this > driver are the ath11k module on QCA6390 and ath12k on WCN7850. Can you add a little detail here about what benefit we will see from this driver? E.g., something that doesn't work correctly now, but will work with this driver? > +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = { > + { > + /* ATH11K in QCA6390 package. */ > + .compatible = "pci17cb,1101", > + .data = "wlan", > + }, > + { > + /* ATH12K in WCN7850 package. */ > + .compatible = "pci17cb,1107", > + .data = "wlan", > + }, IIUC, "pci17cb,1101" and "pci17cb,1107" exist partly so we can check that a DTS conforms to the schema, e.g., a "pci17cb,1101" node contains all the required regulators. For that use, we obviously need a very specific "compatible" string. Is there any opportunity to add a more generic "compatible" string in addition to those so this list doesn't have to be updated for every PMU? The .data here is "wlan" in both cases, and for this purpose, we don't care whether it's "pci17cb,1101" or "pci17cb,1107".
On Wed, 5 Jun 2024 at 02:23, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, May 28, 2024 at 09:03:24PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add a PCI power control driver that's capable of correctly powering up > > devices using the power sequencing subsystem. The first users of this > > driver are the ath11k module on QCA6390 and ath12k on WCN7850. > > Can you add a little detail here about what benefit we will see from > this driver? E.g., something that doesn't work correctly now, but > will work with this driver? > > > +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = { > > + { > > + /* ATH11K in QCA6390 package. */ > > + .compatible = "pci17cb,1101", > > + .data = "wlan", > > + }, > > + { > > + /* ATH12K in WCN7850 package. */ > > + .compatible = "pci17cb,1107", > > + .data = "wlan", > > + }, > > IIUC, "pci17cb,1101" and "pci17cb,1107" exist partly so we can check > that a DTS conforms to the schema, e.g., a "pci17cb,1101" node > contains all the required regulators. For that use, we obviously need > a very specific "compatible" string. > > Is there any opportunity to add a more generic "compatible" string in > addition to those so this list doesn't have to be updated for every > PMU? The .data here is "wlan" in both cases, and for this purpose, we > don't care whether it's "pci17cb,1101" or "pci17cb,1107". These two devices have different set of regulators and different requirements to power them on.
On Wed, Jun 05, 2024 at 02:34:52AM +0300, Dmitry Baryshkov wrote: > On Wed, 5 Jun 2024 at 02:23, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Tue, May 28, 2024 at 09:03:24PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Add a PCI power control driver that's capable of correctly powering up > > > devices using the power sequencing subsystem. The first users of this > > > driver are the ath11k module on QCA6390 and ath12k on WCN7850. > > > +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = { > > > + { > > > + /* ATH11K in QCA6390 package. */ > > > + .compatible = "pci17cb,1101", > > > + .data = "wlan", > > > + }, > > > + { > > > + /* ATH12K in WCN7850 package. */ > > > + .compatible = "pci17cb,1107", > > > + .data = "wlan", > > > + }, > > > > IIUC, "pci17cb,1101" and "pci17cb,1107" exist partly so we can check > > that a DTS conforms to the schema, e.g., a "pci17cb,1101" node > > contains all the required regulators. For that use, we obviously need > > a very specific "compatible" string. > > > > Is there any opportunity to add a more generic "compatible" string in > > addition to those so this list doesn't have to be updated for every > > PMU? The .data here is "wlan" in both cases, and for this purpose, we > > don't care whether it's "pci17cb,1101" or "pci17cb,1107". > > These two devices have different set of regulators and different > requirements to power them on. Right, but I don't think pci_pwrctl_pwrseq_probe() knows about those different sets. It basically looks like: pci_pwrctl_pwrseq_probe(struct platform_device *pdev) { struct pci_pwrctl_pwrseq_data *data; struct device *dev = &pdev->dev; data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev)); pwrseq_power_on(data->pwrseq); data->ctx.dev = dev; devm_pci_pwrctl_device_set_ready(dev, &data->ctx); } I think of_device_get_match_data(dev) will return "wlan" for both "pci17cb,1101" and "pci17cb,1107", so devm_pwrseq_get(), pwrseq_power_on(), and devm_pci_pwrctl_device_set_ready() don't see the distinction between them. Of course, they also get "dev", so they can find the device-specifc stuff that way, but I think that's on the drivers/power/sequencing/ side, not in this pci-pwrctl-pwrseq driver itself. So what if there were a more generic "compatible" string, e.g., if the DT contained something like this: wifi@0 { compatible = "pci17cb,1101", "wlan-pwrseq"; ... } and pci_pwrctl_pwrseq_of_match[] had this: { .compatible = "wlan-pwrseq", .data = "wlan", } Wouldn't this pci-pwrctl-pwrseq driver work the same? I'm not a DT whiz, so likely I'm missing something, but it would be nice if we didn't have to update this very generic-looking driver to add every device that needs it. Bjorn
On Wed, Jun 5, 2024 at 4:13 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Jun 05, 2024 at 02:34:52AM +0300, Dmitry Baryshkov wrote: > > On Wed, 5 Jun 2024 at 02:23, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > On Tue, May 28, 2024 at 09:03:24PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Add a PCI power control driver that's capable of correctly powering up > > > > devices using the power sequencing subsystem. The first users of this > > > > driver are the ath11k module on QCA6390 and ath12k on WCN7850. > > > > > +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = { > > > > + { > > > > + /* ATH11K in QCA6390 package. */ > > > > + .compatible = "pci17cb,1101", > > > > + .data = "wlan", > > > > + }, > > > > + { > > > > + /* ATH12K in WCN7850 package. */ > > > > + .compatible = "pci17cb,1107", > > > > + .data = "wlan", > > > > + }, > > > > > > IIUC, "pci17cb,1101" and "pci17cb,1107" exist partly so we can check > > > that a DTS conforms to the schema, e.g., a "pci17cb,1101" node > > > contains all the required regulators. For that use, we obviously need > > > a very specific "compatible" string. > > > > > > Is there any opportunity to add a more generic "compatible" string in > > > addition to those so this list doesn't have to be updated for every > > > PMU? The .data here is "wlan" in both cases, and for this purpose, we > > > don't care whether it's "pci17cb,1101" or "pci17cb,1107". > > > > These two devices have different set of regulators and different > > requirements to power them on. > > Right, but I don't think pci_pwrctl_pwrseq_probe() knows about those > different sets. It basically looks like: > > pci_pwrctl_pwrseq_probe(struct platform_device *pdev) > { > struct pci_pwrctl_pwrseq_data *data; > struct device *dev = &pdev->dev; > > data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev)); > pwrseq_power_on(data->pwrseq); > data->ctx.dev = dev; > devm_pci_pwrctl_device_set_ready(dev, &data->ctx); > } > > I think of_device_get_match_data(dev) will return "wlan" for both > "pci17cb,1101" and "pci17cb,1107", so devm_pwrseq_get(), > pwrseq_power_on(), and devm_pci_pwrctl_device_set_ready() don't see > the distinction between them. > These are only the first two users of this generic driver. We may end up adding more that will use different targets or even extend the match data with additional fields. > Of course, they also get "dev", so they can find the device-specifc > stuff that way, but I think that's on the drivers/power/sequencing/ > side, not in this pci-pwrctl-pwrseq driver itself. > > So what if there were a more generic "compatible" string, e.g., if the > DT contained something like this: > > wifi@0 { > compatible = "pci17cb,1101", "wlan-pwrseq"; What even is "pwrseq" in the context of the hardware description? DT maintainers would like to have a word with you. :) > ... > } > > and pci_pwrctl_pwrseq_of_match[] had this: > > { .compatible = "wlan-pwrseq", .data = "wlan", } > > Wouldn't this pci-pwrctl-pwrseq driver work the same? I'm not a DT > whiz, so likely I'm missing something, but it would be nice if we > didn't have to update this very generic-looking driver to add every > device that needs it. > Device-tree describes hardware, not the implementation. You can see elsewhere in this series that we have the PMU described as a PMIC on the device tree but we never register with the regulator subsystem nor do we create actual regulators in C. The HW description does not have to match the C implementation 1:1 but has to be accurate. There's not such HW component as "wlan-pwrseq". If you want a good example of such generic fallback - it'll be the C45 ethernet PHYs as they actually exist: there's a HW definition of what a broader C45 PHY is, even though it can be extended in concrete HW designs. I'd leave this as is. Bart
On Wed, Jun 5, 2024 at 7:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > > wifi@0 { > > > compatible = "pci17cb,1101", "wlan-pwrseq"; > > > > What even is "pwrseq" in the context of the hardware description? DT > > maintainers would like to have a word with you. :) > > There are "compatible" strings like "simple-bus", "simple-mfd", and > "syscon" that allow drivers to bind and provide generic functionality > when they don't need to know the exact hardware. > There's a difference however: a "simple bus" is a thing. A "simple multifunction device" is also an actual thing. A "pwrseq" or "power-sequencer" is not a thing, it's a functionality. And we don't describe it in device-tree. Rob has said before that he regrets having merged the mmc pwrseq bindings back in the day and that he wouldn't do it again now because it describes what HW does and not what it is. In this case the PMU is simply a PMIC and the bindings I'm proposing describe it as such. But what you're proposing is even worse: this is the ath1x module of the larger chipset (power sequencee rather than sequencer) so naming it "wlan-pwrseq" makes absolutely no sense at all. It's a PCI device whose ID is 0x17cb1101 and the device tree describes it as such. > > > and pci_pwrctl_pwrseq_of_match[] had this: > > > > > > { .compatible = "wlan-pwrseq", .data = "wlan", } > > > > > > Wouldn't this pci-pwrctl-pwrseq driver work the same? I'm not a DT > > > whiz, so likely I'm missing something, but it would be nice if we > > > didn't have to update this very generic-looking driver to add every > > > device that needs it. > > Do you have any other ideas to reduce the churn in this file? It just > seems weird to have to add an ID to this file without adding any > actual code or data related to it. > Is it really that much churn though? You'd save 4 lines of code? I think this is premature optimization, we'll see about unifying it when we have several models supported, right now with two, I'd just leave it as is and not seek perfection. > We should probably also add a pattern to MAINTAINERS so > get_maintainers.pl on this file will show you as a maintainer. > Makes sense. Bartosz > Bjorn
On Tue, May 28, 2024 at 09:03:24PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a PCI power control driver that's capable of correctly powering up > devices using the power sequencing subsystem. The first users of this > driver are the ath11k module on QCA6390 and ath12k on WCN7850. > > Tested-by: Amit Pundir <amit.pundir@linaro.org> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> With s/add/Add/ in subject, Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pwrctl/Kconfig | 9 ++++ > drivers/pci/pwrctl/Makefile | 2 + > drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 89 ++++++++++++++++++++++++++++++++++ > 3 files changed, 100 insertions(+) > > diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig > index 96195395af69..f1b824955d4b 100644 > --- a/drivers/pci/pwrctl/Kconfig > +++ b/drivers/pci/pwrctl/Kconfig > @@ -5,4 +5,13 @@ 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 || ATH12K) && ARCH_QCOM) > + help > + Enable support for the PCI power control driver for device > + drivers using the Power Sequencing subsystem. > + > endmenu > diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile > index 52ae0640ef7b..d308aae4800c 100644 > --- a/drivers/pci/pwrctl/Makefile > +++ b/drivers/pci/pwrctl/Makefile > @@ -2,3 +2,5 @@ > > obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o > pci-pwrctl-core-y := core.o > + > +obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o > diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > new file mode 100644 > index 000000000000..c7a113a76c0c > --- /dev/null > +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2024 Linaro Ltd. > + */ > + > +#include <linux/device.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/pci-pwrctl.h> > +#include <linux/platform_device.h> > +#include <linux/pwrseq/consumer.h> > +#include <linux/slab.h> > +#include <linux/types.h> > + > +struct pci_pwrctl_pwrseq_data { > + struct pci_pwrctl ctx; > + struct pwrseq_desc *pwrseq; > +}; > + > +static void devm_pci_pwrctl_pwrseq_power_off(void *data) > +{ > + struct pwrseq_desc *pwrseq = data; > + > + pwrseq_power_off(pwrseq); > +} > + > +static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev) > +{ > + struct pci_pwrctl_pwrseq_data *data; > + struct device *dev = &pdev->dev; > + int ret; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev)); > + if (IS_ERR(data->pwrseq)) > + return dev_err_probe(dev, PTR_ERR(data->pwrseq), > + "Failed to get the power sequencer\n"); > + > + ret = pwrseq_power_on(data->pwrseq); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to power-on the device\n"); > + > + ret = devm_add_action_or_reset(dev, devm_pci_pwrctl_pwrseq_power_off, > + data->pwrseq); > + if (ret) > + return ret; > + > + data->ctx.dev = dev; > + > + ret = devm_pci_pwrctl_device_set_ready(dev, &data->ctx); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to register the pwrctl wrapper\n"); > + > + return 0; > +} > + > +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = { > + { > + /* ATH11K in QCA6390 package. */ > + .compatible = "pci17cb,1101", > + .data = "wlan", > + }, > + { > + /* ATH12K in WCN7850 package. */ > + .compatible = "pci17cb,1107", > + .data = "wlan", > + }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, pci_pwrctl_pwrseq_of_match); > + > +static struct platform_driver pci_pwrctl_pwrseq_driver = { > + .driver = { > + .name = "pci-pwrctl-pwrseq", > + .of_match_table = pci_pwrctl_pwrseq_of_match, > + }, > + .probe = pci_pwrctl_pwrseq_probe, > +}; > +module_platform_driver(pci_pwrctl_pwrseq_driver); > + > +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); > +MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices"); > +MODULE_LICENSE("GPL"); > > -- > 2.43.0 >
diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig index 96195395af69..f1b824955d4b 100644 --- a/drivers/pci/pwrctl/Kconfig +++ b/drivers/pci/pwrctl/Kconfig @@ -5,4 +5,13 @@ 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 || ATH12K) && ARCH_QCOM) + help + Enable support for the PCI power control driver for device + drivers using the Power Sequencing subsystem. + endmenu diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile index 52ae0640ef7b..d308aae4800c 100644 --- a/drivers/pci/pwrctl/Makefile +++ b/drivers/pci/pwrctl/Makefile @@ -2,3 +2,5 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o pci-pwrctl-core-y := core.o + +obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c new file mode 100644 index 000000000000..c7a113a76c0c --- /dev/null +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Linaro Ltd. + */ + +#include <linux/device.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/pci-pwrctl.h> +#include <linux/platform_device.h> +#include <linux/pwrseq/consumer.h> +#include <linux/slab.h> +#include <linux/types.h> + +struct pci_pwrctl_pwrseq_data { + struct pci_pwrctl ctx; + struct pwrseq_desc *pwrseq; +}; + +static void devm_pci_pwrctl_pwrseq_power_off(void *data) +{ + struct pwrseq_desc *pwrseq = data; + + pwrseq_power_off(pwrseq); +} + +static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev) +{ + struct pci_pwrctl_pwrseq_data *data; + struct device *dev = &pdev->dev; + int ret; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev)); + if (IS_ERR(data->pwrseq)) + return dev_err_probe(dev, PTR_ERR(data->pwrseq), + "Failed to get the power sequencer\n"); + + ret = pwrseq_power_on(data->pwrseq); + if (ret) + return dev_err_probe(dev, ret, + "Failed to power-on the device\n"); + + ret = devm_add_action_or_reset(dev, devm_pci_pwrctl_pwrseq_power_off, + data->pwrseq); + if (ret) + return ret; + + data->ctx.dev = dev; + + ret = devm_pci_pwrctl_device_set_ready(dev, &data->ctx); + if (ret) + return dev_err_probe(dev, ret, + "Failed to register the pwrctl wrapper\n"); + + return 0; +} + +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = { + { + /* ATH11K in QCA6390 package. */ + .compatible = "pci17cb,1101", + .data = "wlan", + }, + { + /* ATH12K in WCN7850 package. */ + .compatible = "pci17cb,1107", + .data = "wlan", + }, + { } +}; +MODULE_DEVICE_TABLE(of, pci_pwrctl_pwrseq_of_match); + +static struct platform_driver pci_pwrctl_pwrseq_driver = { + .driver = { + .name = "pci-pwrctl-pwrseq", + .of_match_table = pci_pwrctl_pwrseq_of_match, + }, + .probe = pci_pwrctl_pwrseq_probe, +}; +module_platform_driver(pci_pwrctl_pwrseq_driver); + +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); +MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices"); +MODULE_LICENSE("GPL");