| Message ID | 20240216203215.40870-15-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: sequencing: implement the subsystem and add first users | expand |
On Fri, Feb 16, 2024 at 09:32:11PM +0100, Bartosz Golaszewski wrote: > +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. > +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.
On Mon, Feb 19, 2024 at 6:50 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Feb 16, 2024 at 09:32:11PM +0100, Bartosz Golaszewski wrote: > > > +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. > 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. > > +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. Indeed. Thanks for catching this. Bart
On Tue, Feb 20, 2024 at 12:22:42PM +0100, Bartosz Golaszewski wrote: > On Mon, Feb 19, 2024 at 6:50 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Feb 16, 2024 at 09:32:11PM +0100, Bartosz Golaszewski wrote: > > > +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. > 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.
On 20.02.2024 13:47, Mark Brown wrote: > On Tue, Feb 20, 2024 at 12:22:42PM +0100, Bartosz Golaszewski wrote: >> On Mon, Feb 19, 2024 at 6:50 PM Mark Brown <broonie@kernel.org> wrote: >>> On Fri, Feb 16, 2024 at 09:32:11PM +0100, Bartosz Golaszewski wrote: > >>>> +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. > >> 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. *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
On Tue, Feb 20, 2024 at 10:21:04PM +0100, Konrad Dybcio wrote: > On 20.02.2024 13:47, Mark Brown wrote: > > 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. 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.
On Wed, Feb 21, 2024 at 12:44 AM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 20, 2024 at 10:21:04PM +0100, Konrad Dybcio wrote: > > On 20.02.2024 13:47, Mark Brown wrote: > > > > 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. > > 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
On Thu, Feb 22, 2024 at 10:22:50AM +0100, Bartosz Golaszewski wrote: > On Wed, Feb 21, 2024 at 12:44 AM Mark Brown <broonie@kernel.org> wrote: > > 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. This is new code?
On Thu, Feb 22, 2024 at 1:21 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Feb 22, 2024 at 10:22:50AM +0100, Bartosz Golaszewski wrote: > > On Wed, Feb 21, 2024 at 12:44 AM Mark Brown <broonie@kernel.org> wrote: > > > > 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. > > 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
On Thu, Feb 22, 2024 at 01:26:59PM +0100, Bartosz Golaszewski wrote: > On Thu, Feb 22, 2024 at 1:21 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Feb 22, 2024 at 10:22:50AM +0100, Bartosz Golaszewski wrote: > > > 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? 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.
On Thu, Feb 22, 2024 at 2:15 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Feb 22, 2024 at 01:26:59PM +0100, Bartosz Golaszewski wrote: > > On Thu, Feb 22, 2024 at 1:21 PM Mark Brown <broonie@kernel.org> wrote: > > > On Thu, Feb 22, 2024 at 10:22:50AM +0100, Bartosz Golaszewski wrote: > > > > > 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? > > 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
diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig index 96195395af69..b91170ebfb49 100644 --- a/drivers/pci/pwrctl/Kconfig +++ b/drivers/pci/pwrctl/Kconfig @@ -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 diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile index 52ae0640ef7b..de20c3af1b78 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_WCN7850) += pci-pwrctl-wcn7850.o diff --git a/drivers/pci/pwrctl/pci-pwrctl-wcn7850.c b/drivers/pci/pwrctl/pci-pwrctl-wcn7850.c new file mode 100644 index 000000000000..e2b2c53bff29 --- /dev/null +++ b/drivers/pci/pwrctl/pci-pwrctl-wcn7850.c @@ -0,0 +1,202 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Linaro Ltd. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/consumer.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/regulator/consumer.h> +#include <linux/slab.h> +#include <linux/types.h> + +struct pci_pwrctl_wcn7850_vreg { + const char *name; + unsigned int load_uA; +}; + +struct pci_pwrctl_wcn7850_pdata { + struct pci_pwrctl_wcn7850_vreg *vregs; + size_t num_vregs; + unsigned int delay_msec; +}; + +struct pci_pwrctl_wcn7850_ctx { + struct pci_pwrctl pwrctl; + const struct pci_pwrctl_wcn7850_pdata *pdata; + struct regulator_bulk_data *regs; + struct gpio_desc *en_gpio; + struct clk *clk; +}; + +static struct pci_pwrctl_wcn7850_vreg pci_pwrctl_wcn7850_vregs[] = { + { + .name = "vdd", + .load_uA = 16000, + }, + { + .name = "vddio", + .load_uA = 5000, + }, + { + .name = "vddio1p2", + .load_uA = 16000, + }, + { + .name = "vddaon", + .load_uA = 26000, + }, + { + .name = "vdddig", + .load_uA = 126000, + }, + { + .name = "vddrfa1p2", + .load_uA = 257000, + }, + { + .name = "vddrfa1p8", + .load_uA = 302000, + }, +}; + +static struct pci_pwrctl_wcn7850_pdata pci_pwrctl_wcn7850_of_data = { + .vregs = pci_pwrctl_wcn7850_vregs, + .num_vregs = ARRAY_SIZE(pci_pwrctl_wcn7850_vregs), + .delay_msec = 50, +}; + +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; + + gpiod_set_value_cansleep(ctx->en_gpio, 1); + + if (ctx->pdata->delay_msec) + msleep(ctx->pdata->delay_msec); + + return 0; +} + +static int pci_pwrctl_wcn7850_power_off(struct pci_pwrctl_wcn7850_ctx *ctx) +{ + gpiod_set_value_cansleep(ctx->en_gpio, 0); + clk_disable_unprepare(ctx->clk); + + return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs); +} + +static void devm_pci_pwrctl_wcn7850_power_off(void *data) +{ + struct pci_pwrctl_wcn7850_ctx *ctx = data; + + pci_pwrctl_wcn7850_power_off(ctx); +} + +static int pci_pwrctl_wcn7850_probe(struct platform_device *pdev) +{ + struct pci_pwrctl_wcn7850_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++) { + if (!ctx->pdata->vregs[1].load_uA) + continue; + + 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->clk = devm_clk_get_optional(dev, NULL); + if (IS_ERR(ctx->clk)) + return dev_err_probe(dev, PTR_ERR(ctx->clk), + "Failed to get clock\n"); + + ctx->en_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); + if (IS_ERR(ctx->en_gpio)) + return dev_err_probe(dev, PTR_ERR(ctx->en_gpio), + "Failed to get enable the GPIO\n"); + + ret = pci_pwrctl_wcn7850_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_pci_pwrctl_wcn7850_power_off, + ctx); + if (ret) + return ret; + + ctx->pwrctl.dev = dev; + + ret = devm_pci_pwrctl_device_set_ready(dev, &ctx->pwrctl); + 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_wcn7850_of_match[] = { + { + .compatible = "pci17cb,1107", + .data = &pci_pwrctl_wcn7850_of_data, + }, + { } +}; +MODULE_DEVICE_TABLE(of, pci_pwrctl_wcn7850_of_match); + +static struct platform_driver pci_pwrctl_wcn7850_driver = { + .driver = { + .name = "pci-pwrctl-wcn7850", + .of_match_table = pci_pwrctl_wcn7850_of_match, + }, + .probe = pci_pwrctl_wcn7850_probe, +}; +module_platform_driver(pci_pwrctl_wcn7850_driver); + +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); +MODULE_DESCRIPTION("PCI Power Sequencing module for WCN7850"); +MODULE_LICENSE("GPL");