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, Feb 2, 2024 at 8:48 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> 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.
>

Makes sense, I'll think about it. I'm thinking about adding a flags
argument for this kind of switching.

> >
> > > +
> > > +     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.
>

The docs for QCA6390 also mention the 100ms delay but it doesn't seem
to be necessary. But yes, this was done after Dmitry raised concerns
about the QCA6490.

Bart

>
> >
> > 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]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux