diff mbox series

[v3,2/7] regulator: qca6390: add support for QCA639x powerup sequence

Message ID 20210621223141.1638189-3-dmitry.baryshkov@linaro.org
State New
Headers show
Series Add support for Qualcomm QCA639x chips family | expand

Commit Message

Dmitry Baryshkov June 21, 2021, 10:31 p.m. UTC
Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
being controlled through the UART and WiFi being present on PCIe
bus. Both blocks share common power sources. Add device driver handling
power sequencing of QCA6390/1.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/regulator/Kconfig        |  13 +++
 drivers/regulator/Makefile       |   1 +
 drivers/regulator/qcom-qca639x.c | 157 +++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 drivers/regulator/qcom-qca639x.c

Comments

Mark Brown June 22, 2021, 11:28 a.m. UTC | #1
On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:

> Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part

> being controlled through the UART and WiFi being present on PCIe

> bus. Both blocks share common power sources. Add device driver handling

> power sequencing of QCA6390/1.


Are you sure this is a regulator and not a MFD?  It appears to be a
consumer driver that turns on and off a bunch of regulators en masse
which for some reason exposes that on/off control as a single supply.
This looks like it'd be much more appropriate to implement as a MFD or
possibly power domain with the subdevices using runtime PM, it's clearly
not a regulator.

> +// SPDX-License-Identifier: GPL-2.0-only

> +/*

> + * Copyright (c) 2021, Linaro Limited

> + */


Please make the entire comment a C++ one so things look more
intentional.

> +static int qca6390_enable(struct regulator_dev *rdev)

> +{

> +	struct qca6390_data *data = rdev_get_drvdata(rdev);

> +	int ret;

> +

> +	ret = regulator_bulk_enable(data->num_vregs, data->regulators);

> +	if (ret) {

> +		dev_err(data->dev, "Failed to enable regulators");

> +		return ret;

> +	}


The regulator API is *not* recursive, I am astonished this works.

> +	/* Wait for 1ms before toggling enable pins. */

> +	usleep_range(1000, 2000);


There's core support for delays after power on, better to use it.

> +	data->enable_counter++;


You shouldn't assume that enable and disable calls are matched.
Mark Brown June 22, 2021, 2:38 p.m. UTC | #2
On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote:
> On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@kernel.org> wrote:

> > On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:


> > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part

> > > being controlled through the UART and WiFi being present on PCIe

> > > bus. Both blocks share common power sources. Add device driver handling

> > > power sequencing of QCA6390/1.


> > Are you sure this is a regulator and not a MFD?  It appears to be a

> > consumer driver that turns on and off a bunch of regulators en masse

> > which for some reason exposes that on/off control as a single supply.

> > This looks like it'd be much more appropriate to implement as a MFD or

> > possibly power domain with the subdevices using runtime PM, it's clearly

> > not a regulator.


> First attempt was designed to be an MFD. And Lee clearly stated that

> this is wrong:

> "This is not an MFD, since it utilised neither the MFD API nor

> of_platform_populate() to register child devices." [1]


Well, perhaps it should do one of those things then?  Like I say this is
very clearly not a regulator, it looks like a consumer of some kind.
The regulator API isn't there just to absorb things that need reference
counting, it's there to represent things providing supplies.  This seems
to be very clearly not a supply given that it's grouping together a
bunch of other supplies and switching them on and off together without
providing a clear output supply.

> I've tried following Rob's suggestions on implementing things clearly,

> but doing so results in too big restructure just for a single device.


I don't know what that suggestion was?  If there's only one device that
uses this why is it not implemented as part of that device?

> > > +static int qca6390_enable(struct regulator_dev *rdev)

> > > +{

> > > +     struct qca6390_data *data = rdev_get_drvdata(rdev);

> > > +     int ret;


> > > +     ret = regulator_bulk_enable(data->num_vregs, data->regulators);

> > > +     if (ret) {

> > > +             dev_err(data->dev, "Failed to enable regulators");

> > > +             return ret;

> > > +     }


> > The regulator API is *not* recursive, I am astonished this works.


> It does, even with lockdep enabled. Moreover BT regularly does disable

> and enable this regulator, so both enable and disable paths were well

> tested.

> Should I change this into some internal call to remove API recursiveness?


You should not be implementing this as a regulator at all.
Dmitry Baryshkov June 22, 2021, 4:46 p.m. UTC | #3
On Tue, 22 Jun 2021 at 17:38, Mark Brown <broonie@kernel.org> wrote:
>

> On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote:

> > On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@kernel.org> wrote:

> > > On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:

>

> > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part

> > > > being controlled through the UART and WiFi being present on PCIe

> > > > bus. Both blocks share common power sources. Add device driver handling

> > > > power sequencing of QCA6390/1.

>

> > > Are you sure this is a regulator and not a MFD?  It appears to be a

> > > consumer driver that turns on and off a bunch of regulators en masse

> > > which for some reason exposes that on/off control as a single supply.

> > > This looks like it'd be much more appropriate to implement as a MFD or

> > > possibly power domain with the subdevices using runtime PM, it's clearly

> > > not a regulator.

>

> > First attempt was designed to be an MFD. And Lee clearly stated that

> > this is wrong:

> > "This is not an MFD, since it utilised neither the MFD API nor

> > of_platform_populate() to register child devices." [1]

>

> Well, perhaps it should do one of those things then?


I don't think so. BT part is just a serdev sitting on top of UART,
WiFi is PCIe device (for qca6390). So using MFD API (which primarily
targets platform devices) does not seem logical and feasible.

> Like I say this is

> very clearly not a regulator, it looks like a consumer of some kind.

> The regulator API isn't there just to absorb things that need reference

> counting, it's there to represent things providing supplies.  This seems

> to be very clearly not a supply given that it's grouping together a

> bunch of other supplies and switching them on and off together without

> providing a clear output supply.


Ack.

> > I've tried following Rob's suggestions on implementing things clearly,

> > but doing so results in too big restructure just for a single device.

>

> I don't know what that suggestion was?  If there's only one device that

> uses this why is it not implemented as part of that device?


One device = qca6390 (or 91). Because it is still a device sitting on
a PCI bus which is typically discoverable, while adding power
sequencer to the device driver would demand making the bus fully
descibiable (like PCI bus is on Spark).

> > > > +static int qca6390_enable(struct regulator_dev *rdev)

> > > > +{

> > > > +     struct qca6390_data *data = rdev_get_drvdata(rdev);

> > > > +     int ret;

>

> > > > +     ret = regulator_bulk_enable(data->num_vregs, data->regulators);

> > > > +     if (ret) {

> > > > +             dev_err(data->dev, "Failed to enable regulators");

> > > > +             return ret;

> > > > +     }

>

> > > The regulator API is *not* recursive, I am astonished this works.

>

> > It does, even with lockdep enabled. Moreover BT regularly does disable

> > and enable this regulator, so both enable and disable paths were well

> > tested.

> > Should I change this into some internal call to remove API recursiveness?

>

> You should not be implementing this as a regulator at all.


Ack

-- 
With best wishes
Dmitry
Mark Brown June 22, 2021, 5:08 p.m. UTC | #4
On Tue, Jun 22, 2021 at 07:46:08PM +0300, Dmitry Baryshkov wrote:
> On Tue, 22 Jun 2021 at 17:38, Mark Brown <broonie@kernel.org> wrote:

> > Well, perhaps it should do one of those things then?

> I don't think so. BT part is just a serdev sitting on top of UART,
> WiFi is PCIe device (for qca6390). So using MFD API (which primarily
> targets platform devices) does not seem logical and feasible.

That really does sound like a MFD - AIUI it's a single chip with
multiple interfaces that needs some glue logic to hold it together.  It
doesn't fit well with the current framework that MFD offers but it's
definitely the same sort of one chip in multiple Linux frameworks sort
of thing.  The only other thing I can think might fit is handling it
like a plug in module for a development board (eg, RPi hats) but we've
not been doing so great at getting them supported upstream.
Ulf Hansson July 6, 2021, 7:54 a.m. UTC | #5
+ Peter

On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> being controlled through the UART and WiFi being present on PCIe
> bus. Both blocks share common power sources. Add device driver handling
> power sequencing of QCA6390/1.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Power sequencing of discoverable buses have been discussed several
times before at LKML. The last attempt [1] I am aware of, was in 2017
from Peter Chen. I don't think there is a common solution, yet.

Note that, this isn't specific to the PCIe bus, but rather to all
buses that may have discoverable devices attached and which need some
kind of pre-power-sequence before they can be discovered/probed.  USB,
PCIe, SDIO, etc.

Long time ago, we fixed the problem for SDIO (that also can have WiFi,
UART, bluetooth chips attached), but unfortunately through an MMC
subsystem specific implementation, that can't be re-used in a generic
way.

In any case, I have looped in Peter Chen, maybe he can provide us with
a better update on how things have moved forward, if at all.

Kind regards
Uffe

[1]
https://www.spinics.net/lists/linux-usb/msg158451.html

> ---
>  drivers/regulator/Kconfig        |  13 +++
>  drivers/regulator/Makefile       |   1 +
>  drivers/regulator/qcom-qca639x.c | 157 +++++++++++++++++++++++++++++++
>  3 files changed, 171 insertions(+)
>  create mode 100644 drivers/regulator/qcom-qca639x.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 3e7a38525cb3..7a560cddea7a 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -909,6 +909,19 @@ config REGULATOR_PWM
>           This driver supports PWM controlled voltage regulators. PWM
>           duty cycle can increase or decrease the voltage.
>
> +config REGULATOR_QCOM_QCA639X
> +       tristate "Qualcomm QCA639x WiFi/Bluetooth module support"
> +       help
> +         If you say yes to this option, support will be included for Qualcomm
> +         QCA639x family of WiFi and Bluetooth SoCs. Note, this driver supports
> +         only power control for this SoC, you still have to enable individual
> +         Bluetooth and WiFi drivers. This driver is only necessary on ARM
> +         platforms with this chip. PCIe cards handle power sequencing on their
> +         own.
> +
> +         Say M here if you want to include support for QCA639x chips as a
> +         module. This will build a module called "qcom-qca639x".
> +
>  config REGULATOR_QCOM_RPM
>         tristate "Qualcomm RPM regulator driver"
>         depends on MFD_QCOM_RPM
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 580b015296ea..129c2110b78d 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_REGULATOR_MT6380)        += mt6380-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
>  obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
> +obj-$(CONFIG_REGULATOR_QCOM_QCA639X) += qcom-qca639x.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> diff --git a/drivers/regulator/qcom-qca639x.c b/drivers/regulator/qcom-qca639x.c
> new file mode 100644
> index 000000000000..a2c78c0f8baa
> --- /dev/null
> +++ b/drivers/regulator/qcom-qca639x.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/slab.h>
> +
> +#define MAX_NUM_REGULATORS     8
> +
> +static struct vreg {
> +       const char *name;
> +       unsigned int load_uA;
> +} vregs[MAX_NUM_REGULATORS] = {
> +       /* 2.0 V */
> +       { "vddpcie2", 15000 },
> +       { "vddrfa3", 400000 },
> +
> +       /* 0.95 V */
> +       { "vddaon", 100000 },
> +       { "vddpmu", 1250000 },
> +       { "vddrfa1", 200000 },
> +
> +       /* 1.35 V */
> +       { "vddrfa2", 400000 },
> +       { "vddpcie1", 35000 },
> +
> +       /* 1.8 V */
> +       { "vddio", 20000 },
> +};
> +
> +struct qca6390_data {
> +       struct device *dev;
> +       struct regulator_bulk_data regulators[MAX_NUM_REGULATORS];
> +       size_t num_vregs;
> +
> +       struct regulator_desc desc;
> +       struct regulator_dev *regulator_dev;
> +       unsigned int enable_counter;
> +};
> +
> +#define domain_to_data(domain) container_of(domain, struct qca6390_data, pd)
> +
> +static int qca6390_enable(struct regulator_dev *rdev)
> +{
> +       struct qca6390_data *data = rdev_get_drvdata(rdev);
> +       int ret;
> +
> +       ret = regulator_bulk_enable(data->num_vregs, data->regulators);
> +       if (ret) {
> +               dev_err(data->dev, "Failed to enable regulators");
> +               return ret;
> +       }
> +
> +       /* Wait for 1ms before toggling enable pins. */
> +       usleep_range(1000, 2000);
> +
> +       data->enable_counter++;
> +
> +       return 0;
> +}
> +
> +static int qca6390_disable(struct regulator_dev *rdev)
> +{
> +       struct qca6390_data *data = rdev_get_drvdata(rdev);
> +
> +       regulator_bulk_disable(data->num_vregs, data->regulators);
> +
> +       data->enable_counter--;
> +
> +       return 0;
> +}
> +
> +static int qca6390_is_enabled(struct regulator_dev *rdev)
> +{
> +       struct qca6390_data *data = rdev_get_drvdata(rdev);
> +
> +       return data->enable_counter > 0;
> +}
> +
> +static const struct regulator_ops qca6390_ops = {
> +       .enable = qca6390_enable,
> +       .disable = qca6390_disable,
> +       .is_enabled = qca6390_is_enabled,
> +};
> +
> +static int qca6390_probe(struct platform_device *pdev)
> +{
> +       struct qca6390_data *data;
> +       struct device *dev = &pdev->dev;
> +       struct regulator_config cfg = { };
> +       int i, ret;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->dev = dev;
> +       data->num_vregs = ARRAY_SIZE(vregs);
> +
> +       for (i = 0; i < data->num_vregs; i++)
> +               data->regulators[i].supply = vregs[i].name;
> +
> +       ret = devm_regulator_bulk_get(dev, data->num_vregs, data->regulators);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (i = 0; i < data->num_vregs; i++) {
> +               ret = regulator_set_load(data->regulators[i].consumer, vregs[i].load_uA);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       data->desc.name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +       if (!data->desc.name)
> +               return -ENOMEM;
> +
> +       data->desc.type = REGULATOR_VOLTAGE;
> +       data->desc.owner = THIS_MODULE;
> +       data->desc.ops = &qca6390_ops;
> +
> +       cfg.dev = dev;
> +       cfg.of_node = dev->of_node;
> +       cfg.driver_data = data;
> +       cfg.init_data = of_get_regulator_init_data(dev, dev->of_node, &data->desc);
> +
> +       data->regulator_dev = devm_regulator_register(dev, &data->desc, &cfg);
> +       if (IS_ERR(data->regulator_dev)) {
> +               ret = PTR_ERR(data->regulator_dev);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, data);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id qca6390_of_match[] = {
> +       { .compatible = "qcom,qca6390" },
> +};
> +
> +static struct platform_driver qca6390_driver = {
> +       .probe = qca6390_probe,
> +       .driver = {
> +               .name = "qca6390",
> +               .of_match_table = qca6390_of_match,
> +       },
> +};
> +
> +module_platform_driver(qca6390_driver);
> +MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
> +MODULE_DESCRIPTION("Power control for Qualcomm QCA6390/1 BT/WiFi chip");
> +MODULE_LICENSE("GPL v2");
> --
> 2.30.2
>
Mark Brown July 6, 2021, 11:55 a.m. UTC | #6
On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov

> > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > being controlled through the UART and WiFi being present on PCIe
> > bus. Both blocks share common power sources. Add device driver handling
> > power sequencing of QCA6390/1.

> Power sequencing of discoverable buses have been discussed several
> times before at LKML. The last attempt [1] I am aware of, was in 2017
> from Peter Chen. I don't think there is a common solution, yet.

This feels a bit different to the power sequencing problem - it's not
exposing the individual inputs to the device but rather is a block that
manages everything but needs a bit of a kick to get things going (I'd
guess that with ACPI it'd be triggered via AML).  It's in the same space
but it's not quite the same issue I think, something that can handle
control of the individual resources might still struggle with this.
Ulf Hansson July 8, 2021, 10:09 a.m. UTC | #7
- Peter (the email was bouncing)

On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
>
> > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > being controlled through the UART and WiFi being present on PCIe
> > > bus. Both blocks share common power sources. Add device driver handling
> > > power sequencing of QCA6390/1.
>
> > Power sequencing of discoverable buses have been discussed several
> > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > from Peter Chen. I don't think there is a common solution, yet.
>
> This feels a bit different to the power sequencing problem - it's not
> exposing the individual inputs to the device but rather is a block that
> manages everything but needs a bit of a kick to get things going (I'd
> guess that with ACPI it'd be triggered via AML).  It's in the same space
> but it's not quite the same issue I think, something that can handle
> control of the individual resources might still struggle with this.

Well, to me it looks very similar to those resouses we could manage
with the mmc pwrseq, for SDIO. It's also typically the same kind of
combo-chips that moved from supporting SDIO to PCIe, for improved
performance I guess. More importantly, the same constraint to
pre-power on the device is needed to allow it to be discovered/probed.

Therefore, I think it would be worth having a common solution for
this, rather than a solution per subsystem or even worse, per device.

Unfortunately, it looks like Peter's email is bouncing so we can't get
an update from him.

Kind regards
Uffe
Dmitry Baryshkov July 8, 2021, 11:37 a.m. UTC | #8
Hi,

On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> - Peter (the email was bouncing)

+ Peter's kernel.org address

>
> On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> >
> > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > being controlled through the UART and WiFi being present on PCIe
> > > > bus. Both blocks share common power sources. Add device driver handling
> > > > power sequencing of QCA6390/1.
> >
> > > Power sequencing of discoverable buses have been discussed several
> > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > from Peter Chen. I don't think there is a common solution, yet.
> >
> > This feels a bit different to the power sequencing problem - it's not
> > exposing the individual inputs to the device but rather is a block that
> > manages everything but needs a bit of a kick to get things going (I'd
> > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > but it's not quite the same issue I think, something that can handle
> > control of the individual resources might still struggle with this.
>
> Well, to me it looks very similar to those resouses we could manage
> with the mmc pwrseq, for SDIO. It's also typically the same kind of
> combo-chips that moved from supporting SDIO to PCIe, for improved
> performance I guess. More importantly, the same constraint to
> pre-power on the device is needed to allow it to be discovered/probed.

In our case we'd definitely use pwrseq for PCIe bus and we can also
benefit from using pwrseq for serdev and for platform busses also (for
the same story of WiFi+BT chips).

I can take a look at rewriting pwrseq code to also handle the PCIe
bus. Rewriting it to be a generic lib seems like an easy task,
plugging it into PCIe code would be more fun.

Platform and serdev... Definitely even more fun.

> Therefore, I think it would be worth having a common solution for
> this, rather than a solution per subsystem or even worse, per device.
>
> Unfortunately, it looks like Peter's email is bouncing so we can't get
> an update from him.

Let's see if the kernel.org email will get to him.
Rob Herring (Arm) July 14, 2021, 4:47 p.m. UTC | #9
On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> Hi,
> 
> On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > - Peter (the email was bouncing)
> 
> + Peter's kernel.org address
> 
> >
> > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > >
> > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > >
> > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > power sequencing of QCA6390/1.
> > >
> > > > Power sequencing of discoverable buses have been discussed several
> > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > from Peter Chen. I don't think there is a common solution, yet.
> > >
> > > This feels a bit different to the power sequencing problem - it's not
> > > exposing the individual inputs to the device but rather is a block that
> > > manages everything but needs a bit of a kick to get things going (I'd
> > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > but it's not quite the same issue I think, something that can handle
> > > control of the individual resources might still struggle with this.
> >
> > Well, to me it looks very similar to those resouses we could manage
> > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > combo-chips that moved from supporting SDIO to PCIe, for improved
> > performance I guess. More importantly, the same constraint to
> > pre-power on the device is needed to allow it to be discovered/probed.
> 
> In our case we'd definitely use pwrseq for PCIe bus and we can also
> benefit from using pwrseq for serdev and for platform busses also (for
> the same story of WiFi+BT chips).
> 
> I can take a look at rewriting pwrseq code to also handle the PCIe
> bus. Rewriting it to be a generic lib seems like an easy task,
> plugging it into PCIe code would be more fun.
> 
> Platform and serdev... Definitely even more fun.

I don't want to see pwrseq (the binding) expanded to other buses. If 
that was the answer, we wouldn't be having this discussion. It was a 
mistake for MMC IMO. 

If pwrseq works as a kernel library/api, then I have no issue with that.

> 
> > Therefore, I think it would be worth having a common solution for
> > this, rather than a solution per subsystem or even worse, per device.

Power sequencing requirements are inheritently per device unless we're 
talking about standard connectors. 

This is a solved problem on MDIO. It's quite simple. If there's a DT 
node for a device you haven't discovered, then probe it anyways.

Rob
Bjorn Andersson July 14, 2021, 5:23 p.m. UTC | #10
On Thu 08 Jul 05:09 CDT 2021, Ulf Hansson wrote:

> - Peter (the email was bouncing)
> 
> On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> >
> > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > being controlled through the UART and WiFi being present on PCIe
> > > > bus. Both blocks share common power sources. Add device driver handling
> > > > power sequencing of QCA6390/1.
> >
> > > Power sequencing of discoverable buses have been discussed several
> > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > from Peter Chen. I don't think there is a common solution, yet.
> >
> > This feels a bit different to the power sequencing problem - it's not
> > exposing the individual inputs to the device but rather is a block that
> > manages everything but needs a bit of a kick to get things going (I'd
> > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > but it's not quite the same issue I think, something that can handle
> > control of the individual resources might still struggle with this.
> 
> Well, to me it looks very similar to those resouses we could manage
> with the mmc pwrseq, for SDIO. It's also typically the same kind of
> combo-chips that moved from supporting SDIO to PCIe, for improved
> performance I guess. More importantly, the same constraint to
> pre-power on the device is needed to allow it to be discovered/probed.
> 
> Therefore, I think it would be worth having a common solution for
> this, rather than a solution per subsystem or even worse, per device.
> 

Representing the chip and its power needs, separate from the busses does
seem reasonable. It's pretty much what Dmitry suggested originally, but
his attempts to use either power-domain or regulator references to
ensure ordering has been objected.


Beyond this, there is a similar case (that you and I have talked about
earlier) in supporting the SDX55 PCIe modem found in some devices.
Where in addition to ensuring that the power rails are configured, a
couple of gpios needs to be controlled and there's an incoming gpio line
indicating that the firmware of the device has locked up and the power
needs to be toggled and the device re-enumerated.

> Unfortunately, it looks like Peter's email is bouncing so we can't get
> an update from him.
> 

And for this second part, where we need some additional logic it seems
to go beyond what the power sequence discussions has touched upon so
far.

Regards,
Bjorn
Ulf Hansson Aug. 10, 2021, 11:55 a.m. UTC | #11
On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
>

> On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:

> > Hi,

> >

> > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> > >

> > > - Peter (the email was bouncing)

> >

> > + Peter's kernel.org address

> >

> > >

> > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:

> > > >

> > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:

> > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov

> > > >

> > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part

> > > > > > being controlled through the UART and WiFi being present on PCIe

> > > > > > bus. Both blocks share common power sources. Add device driver handling

> > > > > > power sequencing of QCA6390/1.

> > > >

> > > > > Power sequencing of discoverable buses have been discussed several

> > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017

> > > > > from Peter Chen. I don't think there is a common solution, yet.

> > > >

> > > > This feels a bit different to the power sequencing problem - it's not

> > > > exposing the individual inputs to the device but rather is a block that

> > > > manages everything but needs a bit of a kick to get things going (I'd

> > > > guess that with ACPI it'd be triggered via AML).  It's in the same space

> > > > but it's not quite the same issue I think, something that can handle

> > > > control of the individual resources might still struggle with this.

> > >

> > > Well, to me it looks very similar to those resouses we could manage

> > > with the mmc pwrseq, for SDIO. It's also typically the same kind of

> > > combo-chips that moved from supporting SDIO to PCIe, for improved

> > > performance I guess. More importantly, the same constraint to

> > > pre-power on the device is needed to allow it to be discovered/probed.

> >

> > In our case we'd definitely use pwrseq for PCIe bus and we can also

> > benefit from using pwrseq for serdev and for platform busses also (for

> > the same story of WiFi+BT chips).

> >

> > I can take a look at rewriting pwrseq code to also handle the PCIe

> > bus. Rewriting it to be a generic lib seems like an easy task,

> > plugging it into PCIe code would be more fun.

> >

> > Platform and serdev... Definitely even more fun.

>

> I don't want to see pwrseq (the binding) expanded to other buses. If

> that was the answer, we wouldn't be having this discussion. It was a

> mistake for MMC IMO.


Let's make sure we get your point correctly. I think we have discussed
this in the past, but let's refresh our memories.

If I recall correctly, you are against the mmc pwrseq DT bindings
because we are using a separate pwrseq OF node, that we point to via a
"mmc-pwrseq" property that contains a phandle from the mmc controller
device node. Is that correct?

If we would have encoded the power sequence specific properties, from
within a child node for the mmc controller node, that would have been
okay for you, right?

>

> If pwrseq works as a kernel library/api, then I have no issue with that.


That's what Peter Chen was trying to do. A generic interface, flexible
enough so it can be used for many similar configurations (but not
exactly the same).

Perhaps it was too generic though.

>

> >

> > > Therefore, I think it would be worth having a common solution for

> > > this, rather than a solution per subsystem or even worse, per device.

>

> Power sequencing requirements are inheritently per device unless we're

> talking about standard connectors.


The requirements are certainly per device, but the way to manage them
doesn't have to be.

As you said above, a generic library that subsystems/drivers can call
to power on/off a discoverable device, before trying to probe it would
be a good start.

>

> This is a solved problem on MDIO. It's quite simple. If there's a DT

> node for a device you haven't discovered, then probe it anyways.


A child OF node?

Then what do you think about some common power sequence properties
that we can use in such node?

>

> Rob


Kind regards
Uffe
Bjorn Andersson Aug. 10, 2021, 4:03 p.m. UTC | #12
On Tue 10 Aug 06:55 CDT 2021, Ulf Hansson wrote:

> On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> > > Hi,
> > >
> > > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > - Peter (the email was bouncing)
> > >
> > > + Peter's kernel.org address
> > >
> > > >
> > > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > > > >
> > > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > > > power sequencing of QCA6390/1.
> > > > >
> > > > > > Power sequencing of discoverable buses have been discussed several
> > > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > > > from Peter Chen. I don't think there is a common solution, yet.
> > > > >
> > > > > This feels a bit different to the power sequencing problem - it's not
> > > > > exposing the individual inputs to the device but rather is a block that
> > > > > manages everything but needs a bit of a kick to get things going (I'd
> > > > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > > > but it's not quite the same issue I think, something that can handle
> > > > > control of the individual resources might still struggle with this.
> > > >
> > > > Well, to me it looks very similar to those resouses we could manage
> > > > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > > > combo-chips that moved from supporting SDIO to PCIe, for improved
> > > > performance I guess. More importantly, the same constraint to
> > > > pre-power on the device is needed to allow it to be discovered/probed.
> > >
> > > In our case we'd definitely use pwrseq for PCIe bus and we can also
> > > benefit from using pwrseq for serdev and for platform busses also (for
> > > the same story of WiFi+BT chips).
> > >
> > > I can take a look at rewriting pwrseq code to also handle the PCIe
> > > bus. Rewriting it to be a generic lib seems like an easy task,
> > > plugging it into PCIe code would be more fun.
> > >
> > > Platform and serdev... Definitely even more fun.
> >
> > I don't want to see pwrseq (the binding) expanded to other buses. If
> > that was the answer, we wouldn't be having this discussion. It was a
> > mistake for MMC IMO.
> 
> Let's make sure we get your point correctly. I think we have discussed
> this in the past, but let's refresh our memories.
> 
> If I recall correctly, you are against the mmc pwrseq DT bindings
> because we are using a separate pwrseq OF node, that we point to via a
> "mmc-pwrseq" property that contains a phandle from the mmc controller
> device node. Is that correct?
> 
> If we would have encoded the power sequence specific properties, from
> within a child node for the mmc controller node, that would have been
> okay for you, right?
> 

In Dmitry's case, we have an external chip with that needs to be powered
on per a specific sequence, at which point the WiFi driver on PCIe and
BT driver on serdev will be able to communicate with the device.

The extended case of this is where we have an SDX55 modem soldered onto
the pcb next to the SoC, in which case the power sequencing is even more
complex and additionally there are incoming gpios used to detect things
such as the firmware of the modem has crashed and Linux needs to toggle
power and rescan the PCIe bus.

In both of these cases it seems quite reasonable to represent that
external chip (and it's power needs) as a separate DT node. But we need
a way to link the functional devices to that thing.

Regards,
Bjorn

> >
> > If pwrseq works as a kernel library/api, then I have no issue with that.
> 
> That's what Peter Chen was trying to do. A generic interface, flexible
> enough so it can be used for many similar configurations (but not
> exactly the same).
> 
> Perhaps it was too generic though.
> 
> >
> > >
> > > > Therefore, I think it would be worth having a common solution for
> > > > this, rather than a solution per subsystem or even worse, per device.
> >
> > Power sequencing requirements are inheritently per device unless we're
> > talking about standard connectors.
> 
> The requirements are certainly per device, but the way to manage them
> doesn't have to be.
> 
> As you said above, a generic library that subsystems/drivers can call
> to power on/off a discoverable device, before trying to probe it would
> be a good start.
> 
> >
> > This is a solved problem on MDIO. It's quite simple. If there's a DT
> > node for a device you haven't discovered, then probe it anyways.
> 
> A child OF node?
> 
> Then what do you think about some common power sequence properties
> that we can use in such node?
> 
> >
> > Rob
> 
> Kind regards
> Uffe
Ulf Hansson Aug. 12, 2021, 9:48 a.m. UTC | #13
On Tue, 10 Aug 2021 at 18:03, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Tue 10 Aug 06:55 CDT 2021, Ulf Hansson wrote:
>
> > On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> > > > Hi,
> > > >
> > > > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > - Peter (the email was bouncing)
> > > >
> > > > + Peter's kernel.org address
> > > >
> > > > >
> > > > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > > > > >
> > > > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > > > > power sequencing of QCA6390/1.
> > > > > >
> > > > > > > Power sequencing of discoverable buses have been discussed several
> > > > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > > > > from Peter Chen. I don't think there is a common solution, yet.
> > > > > >
> > > > > > This feels a bit different to the power sequencing problem - it's not
> > > > > > exposing the individual inputs to the device but rather is a block that
> > > > > > manages everything but needs a bit of a kick to get things going (I'd
> > > > > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > > > > but it's not quite the same issue I think, something that can handle
> > > > > > control of the individual resources might still struggle with this.
> > > > >
> > > > > Well, to me it looks very similar to those resouses we could manage
> > > > > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > > > > combo-chips that moved from supporting SDIO to PCIe, for improved
> > > > > performance I guess. More importantly, the same constraint to
> > > > > pre-power on the device is needed to allow it to be discovered/probed.
> > > >
> > > > In our case we'd definitely use pwrseq for PCIe bus and we can also
> > > > benefit from using pwrseq for serdev and for platform busses also (for
> > > > the same story of WiFi+BT chips).
> > > >
> > > > I can take a look at rewriting pwrseq code to also handle the PCIe
> > > > bus. Rewriting it to be a generic lib seems like an easy task,
> > > > plugging it into PCIe code would be more fun.
> > > >
> > > > Platform and serdev... Definitely even more fun.
> > >
> > > I don't want to see pwrseq (the binding) expanded to other buses. If
> > > that was the answer, we wouldn't be having this discussion. It was a
> > > mistake for MMC IMO.
> >
> > Let's make sure we get your point correctly. I think we have discussed
> > this in the past, but let's refresh our memories.
> >
> > If I recall correctly, you are against the mmc pwrseq DT bindings
> > because we are using a separate pwrseq OF node, that we point to via a
> > "mmc-pwrseq" property that contains a phandle from the mmc controller
> > device node. Is that correct?
> >
> > If we would have encoded the power sequence specific properties, from
> > within a child node for the mmc controller node, that would have been
> > okay for you, right?
> >
>
> In Dmitry's case, we have an external chip with that needs to be powered
> on per a specific sequence, at which point the WiFi driver on PCIe and
> BT driver on serdev will be able to communicate with the device.

Thanks for sharing more details.

So, not only do we have a discoverable device that needs to be powered
on in a device specific way before probing, but in fact we have two
consumers of that "combo chip", one (PCIe) for Wifi and one (serdev)
for Bluetooth.

>
> The extended case of this is where we have an SDX55 modem soldered onto
> the pcb next to the SoC, in which case the power sequencing is even more
> complex and additionally there are incoming gpios used to detect things
> such as the firmware of the modem has crashed and Linux needs to toggle
> power and rescan the PCIe bus.

That sounds very similar to what we manage for the SDIO bus already.

We have a mmc pwrseq node to describe what resources that are needed
to power on/off the external chip. The driver for the functional
device (Wifi chip for example) may then call SDIO APIs provided by the
mmc core to power on/off the device, in case some kind of reset would
be needed.

Additionally, we have a child node below the mmc controller node,
allowing us to describe device specific things for the SDIO functional
device, like an out-of-band IRQ line for example.

Overall, this seems to work fine, even if the DT bindings may be questionable.

>
> In both of these cases it seems quite reasonable to represent that
> external chip (and it's power needs) as a separate DT node. But we need
> a way to link the functional devices to that thing.

Don't get me wrong, I am not suggesting we should re-use the
mmc-pwrseq DT bindings - but just trying to share our experience
around them.

In the cases you describe, it certainly sounds like we need some kind
of minimal description in DT for these functional external devices.
For GPIO pins, for example.

How to describe this in DT is one thing, let's see if Rob can help to
point us in some direction of what could make sense.

When it comes to implementing a library/interface to manage these
functional devices, I guess we just have to continue to explore
various options. Perhaps just start simple with another subsystem,
like PCIe and see where this brings us.

>
> Regards,
> Bjorn
>

[...]

Kind regards
Uffe
Dmitry Baryshkov Aug. 12, 2021, 11:51 a.m. UTC | #14
On 12/08/2021 12:48, Ulf Hansson wrote:
> On Tue, 10 Aug 2021 at 18:03, Bjorn Andersson

> <bjorn.andersson@linaro.org> wrote:

>>

>> On Tue 10 Aug 06:55 CDT 2021, Ulf Hansson wrote:

>>

>>> On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:

>>>>

>>>> On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:

>>>>> Hi,

>>>>>

>>>>> On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>>>>>>

>>>>>> - Peter (the email was bouncing)

>>>>>

>>>>> + Peter's kernel.org address

>>>>>

>>>>>>

>>>>>> On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:

>>>>>>>

>>>>>>> On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:

>>>>>>>> On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov

>>>>>>>

>>>>>>>>> Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part

>>>>>>>>> being controlled through the UART and WiFi being present on PCIe

>>>>>>>>> bus. Both blocks share common power sources. Add device driver handling

>>>>>>>>> power sequencing of QCA6390/1.

>>>>>>>

>>>>>>>> Power sequencing of discoverable buses have been discussed several

>>>>>>>> times before at LKML. The last attempt [1] I am aware of, was in 2017

>>>>>>>> from Peter Chen. I don't think there is a common solution, yet.

>>>>>>>

>>>>>>> This feels a bit different to the power sequencing problem - it's not

>>>>>>> exposing the individual inputs to the device but rather is a block that

>>>>>>> manages everything but needs a bit of a kick to get things going (I'd

>>>>>>> guess that with ACPI it'd be triggered via AML).  It's in the same space

>>>>>>> but it's not quite the same issue I think, something that can handle

>>>>>>> control of the individual resources might still struggle with this.

>>>>>>

>>>>>> Well, to me it looks very similar to those resouses we could manage

>>>>>> with the mmc pwrseq, for SDIO. It's also typically the same kind of

>>>>>> combo-chips that moved from supporting SDIO to PCIe, for improved

>>>>>> performance I guess. More importantly, the same constraint to

>>>>>> pre-power on the device is needed to allow it to be discovered/probed.

>>>>>

>>>>> In our case we'd definitely use pwrseq for PCIe bus and we can also

>>>>> benefit from using pwrseq for serdev and for platform busses also (for

>>>>> the same story of WiFi+BT chips).

>>>>>

>>>>> I can take a look at rewriting pwrseq code to also handle the PCIe

>>>>> bus. Rewriting it to be a generic lib seems like an easy task,

>>>>> plugging it into PCIe code would be more fun.

>>>>>

>>>>> Platform and serdev... Definitely even more fun.

>>>>

>>>> I don't want to see pwrseq (the binding) expanded to other buses. If

>>>> that was the answer, we wouldn't be having this discussion. It was a

>>>> mistake for MMC IMO.

>>>

>>> Let's make sure we get your point correctly. I think we have discussed

>>> this in the past, but let's refresh our memories.

>>>

>>> If I recall correctly, you are against the mmc pwrseq DT bindings

>>> because we are using a separate pwrseq OF node, that we point to via a

>>> "mmc-pwrseq" property that contains a phandle from the mmc controller

>>> device node. Is that correct?

>>>

>>> If we would have encoded the power sequence specific properties, from

>>> within a child node for the mmc controller node, that would have been

>>> okay for you, right?

>>>

>>

>> In Dmitry's case, we have an external chip with that needs to be powered

>> on per a specific sequence, at which point the WiFi driver on PCIe and

>> BT driver on serdev will be able to communicate with the device.

> 

> Thanks for sharing more details.

> 

> So, not only do we have a discoverable device that needs to be powered

> on in a device specific way before probing, but in fact we have two

> consumers of that "combo chip", one (PCIe) for Wifi and one (serdev)

> for Bluetooth.

> 

>>

>> The extended case of this is where we have an SDX55 modem soldered onto

>> the pcb next to the SoC, in which case the power sequencing is even more

>> complex and additionally there are incoming gpios used to detect things

>> such as the firmware of the modem has crashed and Linux needs to toggle

>> power and rescan the PCIe bus.

> 

> That sounds very similar to what we manage for the SDIO bus already.

> 

> We have a mmc pwrseq node to describe what resources that are needed

> to power on/off the external chip. The driver for the functional

> device (Wifi chip for example) may then call SDIO APIs provided by the

> mmc core to power on/off the device, in case some kind of reset would

> be needed.

> 

> Additionally, we have a child node below the mmc controller node,

> allowing us to describe device specific things for the SDIO functional

> device, like an out-of-band IRQ line for example.

> 

> Overall, this seems to work fine, even if the DT bindings may be questionable.

> 

>>

>> In both of these cases it seems quite reasonable to represent that

>> external chip (and it's power needs) as a separate DT node. But we need

>> a way to link the functional devices to that thing.

> 

> Don't get me wrong, I am not suggesting we should re-use the

> mmc-pwrseq DT bindings - but just trying to share our experience

> around them.

> 

> In the cases you describe, it certainly sounds like we need some kind

> of minimal description in DT for these functional external devices.

> For GPIO pins, for example.

> 

> How to describe this in DT is one thing, let's see if Rob can help to

> point us in some direction of what could make sense.

> 

> When it comes to implementing a library/interface to manage these

> functional devices, I guess we just have to continue to explore

> various options. Perhaps just start simple with another subsystem,

> like PCIe and see where this brings us.


Thank you for your opinion and suggestions. In fact I'm probably going 
to start working on non-discoverable busses first (by chaning support 
for few other BT+WiFi Qualcomm chips), later shifting the attention to 
the PCIe part. While this may seem like a longer path, I'd like to 
narrow pwrseq subsystem first, before going into PCIe details.


-- 
With best wishes
Dmitry
diff mbox series

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 3e7a38525cb3..7a560cddea7a 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -909,6 +909,19 @@  config REGULATOR_PWM
 	  This driver supports PWM controlled voltage regulators. PWM
 	  duty cycle can increase or decrease the voltage.
 
+config REGULATOR_QCOM_QCA639X
+	tristate "Qualcomm QCA639x WiFi/Bluetooth module support"
+	help
+	  If you say yes to this option, support will be included for Qualcomm
+	  QCA639x family of WiFi and Bluetooth SoCs. Note, this driver supports
+	  only power control for this SoC, you still have to enable individual
+	  Bluetooth and WiFi drivers. This driver is only necessary on ARM
+	  platforms with this chip. PCIe cards handle power sequencing on their
+	  own.
+
+	  Say M here if you want to include support for QCA639x chips as a
+	  module. This will build a module called "qcom-qca639x".
+
 config REGULATOR_QCOM_RPM
 	tristate "Qualcomm RPM regulator driver"
 	depends on MFD_QCOM_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 580b015296ea..129c2110b78d 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -99,6 +99,7 @@  obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
 obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_QCA639X) += qcom-qca639x.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-qca639x.c b/drivers/regulator/qcom-qca639x.c
new file mode 100644
index 000000000000..a2c78c0f8baa
--- /dev/null
+++ b/drivers/regulator/qcom-qca639x.c
@@ -0,0 +1,157 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Linaro Limited
+ */
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+#define MAX_NUM_REGULATORS	8
+
+static struct vreg {
+	const char *name;
+	unsigned int load_uA;
+} vregs[MAX_NUM_REGULATORS] = {
+	/* 2.0 V */
+	{ "vddpcie2", 15000 },
+	{ "vddrfa3", 400000 },
+
+	/* 0.95 V */
+	{ "vddaon", 100000 },
+	{ "vddpmu", 1250000 },
+	{ "vddrfa1", 200000 },
+
+	/* 1.35 V */
+	{ "vddrfa2", 400000 },
+	{ "vddpcie1", 35000 },
+
+	/* 1.8 V */
+	{ "vddio", 20000 },
+};
+
+struct qca6390_data {
+	struct device *dev;
+	struct regulator_bulk_data regulators[MAX_NUM_REGULATORS];
+	size_t num_vregs;
+
+	struct regulator_desc desc;
+	struct regulator_dev *regulator_dev;
+	unsigned int enable_counter;
+};
+
+#define domain_to_data(domain) container_of(domain, struct qca6390_data, pd)
+
+static int qca6390_enable(struct regulator_dev *rdev)
+{
+	struct qca6390_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = regulator_bulk_enable(data->num_vregs, data->regulators);
+	if (ret) {
+		dev_err(data->dev, "Failed to enable regulators");
+		return ret;
+	}
+
+	/* Wait for 1ms before toggling enable pins. */
+	usleep_range(1000, 2000);
+
+	data->enable_counter++;
+
+	return 0;
+}
+
+static int qca6390_disable(struct regulator_dev *rdev)
+{
+	struct qca6390_data *data = rdev_get_drvdata(rdev);
+
+	regulator_bulk_disable(data->num_vregs, data->regulators);
+
+	data->enable_counter--;
+
+	return 0;
+}
+
+static int qca6390_is_enabled(struct regulator_dev *rdev)
+{
+	struct qca6390_data *data = rdev_get_drvdata(rdev);
+
+	return data->enable_counter > 0;
+}
+
+static const struct regulator_ops qca6390_ops = {
+	.enable = qca6390_enable,
+	.disable = qca6390_disable,
+	.is_enabled = qca6390_is_enabled,
+};
+
+static int qca6390_probe(struct platform_device *pdev)
+{
+	struct qca6390_data *data;
+	struct device *dev = &pdev->dev;
+	struct regulator_config cfg = { };
+	int i, ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->dev = dev;
+	data->num_vregs = ARRAY_SIZE(vregs);
+
+	for (i = 0; i < data->num_vregs; i++)
+		data->regulators[i].supply = vregs[i].name;
+
+	ret = devm_regulator_bulk_get(dev, data->num_vregs, data->regulators);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < data->num_vregs; i++) {
+		ret = regulator_set_load(data->regulators[i].consumer, vregs[i].load_uA);
+		if (ret)
+			return ret;
+	}
+
+	data->desc.name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!data->desc.name)
+		return -ENOMEM;
+
+	data->desc.type = REGULATOR_VOLTAGE;
+	data->desc.owner = THIS_MODULE;
+	data->desc.ops = &qca6390_ops;
+
+	cfg.dev = dev;
+	cfg.of_node = dev->of_node;
+	cfg.driver_data = data;
+	cfg.init_data = of_get_regulator_init_data(dev, dev->of_node, &data->desc);
+
+	data->regulator_dev = devm_regulator_register(dev, &data->desc, &cfg);
+	if (IS_ERR(data->regulator_dev)) {
+		ret = PTR_ERR(data->regulator_dev);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static const struct of_device_id qca6390_of_match[] = {
+	{ .compatible = "qcom,qca6390" },
+};
+
+static struct platform_driver qca6390_driver = {
+	.probe = qca6390_probe,
+	.driver = {
+		.name = "qca6390",
+		.of_match_table = qca6390_of_match,
+	},
+};
+
+module_platform_driver(qca6390_driver);
+MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
+MODULE_DESCRIPTION("Power control for Qualcomm QCA6390/1 BT/WiFi chip");
+MODULE_LICENSE("GPL v2");