Re: [RFC 4/9] power: pwrseq: add a driver for the QCA6390 PMU module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2 Feb 2024 at 06:54, Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:27PM +0100, Bartosz Golaszewski wrote:
> > diff --git a/drivers/power/sequencing/pwrseq-qca6390.c b/drivers/power/sequencing/pwrseq-qca6390.c
> [..]
> > +static int pwrseq_qca6390_power_on(struct pwrseq_device *pwrseq)
> > +{
> > +     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
> > +     int ret;
> > +
> > +     ret = regulator_bulk_enable(ctx->pdata->num_vregs, ctx->regs);
> > +     if (ret)
> > +             return ret;
> > +
> > +     gpiod_set_value_cansleep(ctx->bt_gpio, 1);
> > +     gpiod_set_value_cansleep(ctx->wlan_gpio, 1);
>
> So it's no longer possible to power these independently?

I'd second this, there must be a way to power them on and off
separately. In the end, this provides a good way to restart the BT
core if it gets sick.

>
> > +
> > +     if (ctx->pdata->pwup_delay_msec)
> > +             msleep(ctx->pdata->pwup_delay_msec);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pwrseq_qca6390_power_off(struct pwrseq_device *pwrseq)
> > +{
> > +     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
> > +
> > +     gpiod_set_value_cansleep(ctx->bt_gpio, 0);
> > +     gpiod_set_value_cansleep(ctx->wlan_gpio, 0);
> > +
>
> The answer that was provided recently was that the WiFi and BT modules
> absolutely must be modelled together, because there must be a 100ms
> delay between bt_gpio going low and wlan_gpio going high.

For the reference, it was for the QCA6490 (not QCA6390, next
revision), which maps to WCN6855.


>
> If you're not going to address that concern, then I fail to see the
> reason for adding the power sequence framework - just let the BT and
> PCI power control (WiFi) do their thing independently.
>
> > +     return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
> > +}
> > +
> > +static int pwrseq_qca6390_match(struct pwrseq_device *pwrseq,
> > +                             struct device *dev)
> > +{
> > +     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
> > +     struct device_node *dev_node = dev->of_node;
> > +
> > +     /*
> > +      * The PMU supplies power to the Bluetooth and WLAN modules. both
> > +      * consume the PMU AON output so check the presence of the
> > +      * 'vddaon-supply' property and whether it leads us to the right
> > +      * device.
> > +      */
> > +     if (!of_property_present(dev_node, "vddaon-supply"))
> > +             return 0;
> > +
> > +     struct device_node *reg_node __free(of_node) =
> > +                     of_parse_phandle(dev_node, "vddaon-supply", 0);
> > +     if (!reg_node)
> > +             return 0;
> > +
> > +     /*
> > +      * `reg_node` is the PMU AON regulator, its parent is the `regulators`
> > +      * node and finally its grandparent is the PMU device node that we're
> > +      * looking for.
> > +      */
> > +     if (!reg_node->parent || !reg_node->parent->parent ||
> > +         reg_node->parent->parent != ctx->of_node)
> > +             return 0;
>
> Your DeviceTree example gives a sense that a set of supplies feeds the
> PMU, which then supplies power to the BT and WiFi nodes through some
> entity that can switch power on and off, and adjust the voltage level.
>
> Then comes this function, which indicates that the DeviceTree model was
> just for show.
>
> > +
> > +     return 1;
> > +}
> > +
> > +static int pwrseq_qca6390_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct pwrseq_qca6390_ctx *ctx;
> > +     struct pwrseq_config config;
> > +     int ret, i;
> > +
> > +     ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +     if (!ctx)
> > +             return -ENOMEM;
> > +
> > +     ctx->of_node = dev->of_node;
> > +
> > +     ctx->pdata = of_device_get_match_data(dev);
> > +     if (!ctx->pdata)
> > +             return dev_err_probe(dev, -ENODEV,
> > +                                  "Failed to obtain platform data\n");
> > +
> > +     if (ctx->pdata->vregs) {
> > +             ctx->regs = devm_kcalloc(dev, ctx->pdata->num_vregs,
> > +                                      sizeof(*ctx->regs), GFP_KERNEL);
> > +             if (!ctx->regs)
> > +                     return -ENOMEM;
> > +
> > +             for (i = 0; i < ctx->pdata->num_vregs; i++)
> > +                     ctx->regs[i].supply = ctx->pdata->vregs[i].name;
> > +
> > +             ret = devm_regulator_bulk_get(dev, ctx->pdata->num_vregs,
> > +                                           ctx->regs);
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "Failed to get all regulators\n");
> > +
> > +             for (i = 0; i < ctx->pdata->num_vregs; i++) {
> > +                     if (!ctx->pdata->vregs[1].load_uA)
> > +                             continue;
> > +
> > +                     ret = regulator_set_load(ctx->regs[i].consumer,
> > +                                              ctx->pdata->vregs[i].load_uA);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "Failed to set vreg load\n");
> > +             }
> > +     }
> > +
> > +     ctx->bt_gpio = devm_gpiod_get_optional(dev, "bt-enable", GPIOD_OUT_LOW);
>
> Why are these optional? Does it make sense to have a qca6390 without
> both of these gpios connected?
>
> Regards,
> Bjorn
>
> > +     if (IS_ERR(ctx->bt_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(ctx->bt_gpio),
> > +                                  "Failed to get the Bluetooth enable GPIO\n");
> > +
> > +     ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
> > +                                              GPIOD_OUT_LOW);
> > +     if (IS_ERR(ctx->wlan_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
> > +                                  "Failed to get the WLAN enable GPIO\n");
> > +
> > +     memset(&config, 0, sizeof(config));
> > +
> > +     config.parent = dev;
> > +     config.owner = THIS_MODULE;
> > +     config.drvdata = ctx;
> > +     config.match = pwrseq_qca6390_match;
> > +     config.power_on = pwrseq_qca6390_power_on;
> > +     config.power_off = pwrseq_qca6390_power_off;
> > +
> > +     ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> > +     if (IS_ERR(ctx->pwrseq))
> > +             return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> > +                                  "Failed to register the power sequencer\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id pwrseq_qca6390_of_match[] = {
> > +     {
> > +             .compatible = "qcom,qca6390-pmu",
> > +             .data = &pwrseq_qca6390_of_data,
> > +     },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, pwrseq_qca6390_of_match);
> > +
> > +static struct platform_driver pwrseq_qca6390_driver = {
> > +     .driver = {
> > +             .name = "pwrseq-qca6390",
> > +             .of_match_table = pwrseq_qca6390_of_match,
> > +     },
> > +     .probe = pwrseq_qca6390_probe,
> > +};
> > +module_platform_driver(pwrseq_qca6390_driver);
> > +
> > +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.40.1
> >
>


--
With best wishes
Dmitry




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux