| Message ID | 20240201155532.49707-10-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: sequencing: implement the subsystem and add first users | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/CheckPatch | warning | WARNING: please write a help paragraph that fully describes the config symbol #139: FILE: drivers/pci/pwrctl/Kconfig:8: +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. + WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #159: new file mode 100644 WARNING: DT compatible string "pci17cb,1101" appears un-documented -- check ./Documentation/devicetree/bindings/ #229: FILE: drivers/pci/pwrctl/pci-pwrctl-pwrseq.c:66: + .compatible = "pci17cb,1101", WARNING: DT compatible string vendor "pci17cb" appears un-documented -- check ./Documentation/devicetree/bindings/vendor-prefixes.yaml #229: FILE: drivers/pci/pwrctl/pci-pwrctl-pwrseq.c:66: + .compatible = "pci17cb,1101", total: 0 errors, 4 warnings, 100 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13541343.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
On Thu, Feb 01, 2024 at 04:55:32PM +0100, 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. For now we support the > ath11k module on QCA6390. > For a PCI device which doesn't share resources with something on another bus, the whole power sequencing would be implemented in a driver like this - without the involvement of the power sequence framework. I think it would be nice to see this series introduce a simple pci_pwrctl driver, and then (in the same series) introduce the power sequence framework and your PMU driver. One case where such model would be appropriate is the XHCI controller (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on the PCI controller, but it should have been vdd33-supply, vdd10-supply, avdd33-supply on the PCI device. That would provide an example for how a simple PCI power control driver can/should look like, and we can discuss the PCI pieces separate from the introduction of the new power sequence framework (which is unrelated to PCI). Regards, Bjorn > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/pci/pwrctl/Kconfig | 9 +++ > drivers/pci/pwrctl/Makefile | 1 + > drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 83 ++++++++++++++++++++++++++ > 3 files changed, 93 insertions(+) > create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > > diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig > index e2dc5e5d2af1..bca72dc08e79 100644 > --- a/drivers/pci/pwrctl/Kconfig > +++ b/drivers/pci/pwrctl/Kconfig > @@ -5,4 +5,13 @@ menu "PCI Power control drivers" > config PCI_PWRCTL > bool > > +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. > + > endmenu > diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile > index 4381cfbf3f21..919c0f704ee9 100644 > --- a/drivers/pci/pwrctl/Makefile > +++ b/drivers/pci/pwrctl/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_PCI_PWRCTL) += 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..510598c4edc4 > --- /dev/null > +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > @@ -0,0 +1,83 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2023-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); > + 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_enable(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", > + }, > + { } > +}; > +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.40.1 >
On Fri, Feb 2, 2024 at 5:03 AM Bjorn Andersson <andersson@kernel.org> wrote: > > On Thu, Feb 01, 2024 at 04:55:32PM +0100, 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. For now we support the > > ath11k module on QCA6390. > > > > For a PCI device which doesn't share resources with something on another > bus, the whole power sequencing would be implemented in a driver like > this - without the involvement of the power sequence framework. > Yes, this is what I did in the previous incarnation of this code[1]. (I know, I should have linked it here. My bad, I will do it next time). > I think it would be nice to see this series introduce a simple > pci_pwrctl driver, and then (in the same series) introduce the power > sequence framework and your PMU driver. > I disagree. I was initially annoyed by Dmitry asking me to do a lot more work than anticipated but he's right after all. WLAN and BT consuming what is really the PMU's inputs is simply not the actual representation. That's why we should make it a pwrseq user IMO. > One case where such model would be appropriate is the XHCI controller > (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on > the PCI controller, but it should have been vdd33-supply, vdd10-supply, > avdd33-supply on the PCI device. Sounds like a good second user then! > > That would provide an example for how a simple PCI power control driver > can/should look like, and we can discuss the PCI pieces separate from > the introduction of the new power sequence framework (which is unrelated > to PCI). I agree it's unrelated and it could possibly go upstream separately but the particular use-case on RB5 (and other Qcom platforms) requires both the PCI and generic power sequencing to be addressed. Bart [snip] [1] https://lore.kernel.org/netdev/20240117160748.37682-7-brgl@bgdev.pl/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1
On Fri, Feb 02, 2024 at 02:05:59PM +0100, Bartosz Golaszewski wrote: > On Fri, Feb 2, 2024 at 5:03 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Thu, Feb 01, 2024 at 04:55:32PM +0100, 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. For now we support the > > > ath11k module on QCA6390. > > > > > > > For a PCI device which doesn't share resources with something on another > > bus, the whole power sequencing would be implemented in a driver like > > this - without the involvement of the power sequence framework. > > > > Yes, this is what I did in the previous incarnation of this code[1]. > > (I know, I should have linked it here. My bad, I will do it next time). > > > I think it would be nice to see this series introduce a simple > > pci_pwrctl driver, and then (in the same series) introduce the power > > sequence framework and your PMU driver. > > > > I disagree. I was initially annoyed by Dmitry asking me to do a lot > more work than anticipated but he's right after all. WLAN and BT > consuming what is really the PMU's inputs is simply not the actual > representation. That's why we should make it a pwrseq user IMO. > If the PMU registers the "internal" output regulators, then PCI device would consume the PCI outputs of the PMU, the BT device would consume the BT outputs of the PMU. The PMU requests inputs enabled and drives BT_EN and WLAN_EN according to which subset of these output regulators are enabled. Pretty much exactly as "regulator-fixes" isn't a pwrseq device. Regards, Bjorn > > One case where such model would be appropriate is the XHCI controller > > (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on > > the PCI controller, but it should have been vdd33-supply, vdd10-supply, > > avdd33-supply on the PCI device. > > Sounds like a good second user then! > > > > > That would provide an example for how a simple PCI power control driver > > can/should look like, and we can discuss the PCI pieces separate from > > the introduction of the new power sequence framework (which is unrelated > > to PCI). > > I agree it's unrelated and it could possibly go upstream separately > but the particular use-case on RB5 (and other Qcom platforms) requires > both the PCI and generic power sequencing to be addressed. > > Bart > > [snip] > > [1] https://lore.kernel.org/netdev/20240117160748.37682-7-brgl@bgdev.pl/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1
diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig index e2dc5e5d2af1..bca72dc08e79 100644 --- a/drivers/pci/pwrctl/Kconfig +++ b/drivers/pci/pwrctl/Kconfig @@ -5,4 +5,13 @@ menu "PCI Power control drivers" config PCI_PWRCTL bool +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. + endmenu diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile index 4381cfbf3f21..919c0f704ee9 100644 --- a/drivers/pci/pwrctl/Makefile +++ b/drivers/pci/pwrctl/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PCI_PWRCTL) += 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..510598c4edc4 --- /dev/null +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2023-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); + 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_enable(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", + }, + { } +}; +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");