| Message ID | 20210621223141.1638189-3-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Qualcomm QCA639x chips family | expand |
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.
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] I've attempted implementing that as a genpd (in previous iterations), but it results in worse design. PCIe controllers are not expected to handle power domains for EP devices, especially in cases when the PD must come up before the controller does link training and bus probe. I've tried following Rob's suggestions on implementing things clearly, but doing so results in too big restructure just for a single device. > > +// 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. Ack. > > > +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? > > > + /* Wait for 1ms before toggling enable pins. */ > > + usleep_range(1000, 2000); > > There's core support for delays after power on, better to use it. Ack. > > > + data->enable_counter++; > > You shouldn't assume that enable and disable calls are matched. Ack. -- With best wishes Dmitry
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.
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
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.
+ 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 >
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.
- 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
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. -- With best wishes Dmitry
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
On Wed 14 Jul 11:47 CDT 2021, Rob Herring 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. > But what do you want to see? We have a single piece of hardware that needs a specific power sequence, which is interacted with using both UART and PCIe. > 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. > Do you mean "device" as in the IC or device as in struct device? Because we do have one physical IC, that has a need for a specific power on sequence, but we have two struct device, on two different busses in Linux interacting with this thing. > 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. > Okay, so DT tells us that there's actually a WiFi thing on the PCIe bus, even though we can't find it, so we probe something...then what? Regards, Bjorn
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
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
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.
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");
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