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
