diff mbox series

[v5,14/18] PCI/pwrctl: add a power control driver for WCN7850

Message ID 20240216203215.40870-15-brgl@bgdev.pl
State New
Headers show
Series power: sequencing: implement the subsystem and add first users | expand

Commit Message

Bartosz Golaszewski Feb. 16, 2024, 8:32 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a PCI power control driver that's capable of correctly powering up
the ath12k module on WCN7850 using the PCI pwrctl functionality.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 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

Comments

Mark Brown Feb. 19, 2024, 5:49 p.m. UTC | #1
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.
Bartosz Golaszewski Feb. 20, 2024, 11:22 a.m. UTC | #2
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
Mark Brown Feb. 20, 2024, 12:47 p.m. UTC | #3
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.
Konrad Dybcio Feb. 20, 2024, 9:21 p.m. UTC | #4
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
Mark Brown Feb. 20, 2024, 11:44 p.m. UTC | #5
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.
Bartosz Golaszewski Feb. 22, 2024, 9:22 a.m. UTC | #6
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
Mark Brown Feb. 22, 2024, 12:21 p.m. UTC | #7
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?
Bartosz Golaszewski Feb. 22, 2024, 12:26 p.m. UTC | #8
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
Mark Brown Feb. 22, 2024, 1:15 p.m. UTC | #9
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.
Bartosz Golaszewski Feb. 22, 2024, 1:26 p.m. UTC | #10
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 mbox series

Patch

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");