Message-ID: <CAE-0n51sr_D7e66WiSETiSuE3nZaAJwS5KfuYzdVWsLbL=q2zA@mail.gmail.com>
Date: Tue, 14 Jan 2025 16:24:06 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Rob Herring <robh@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev, 
	devicetree@...r.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, linux-arm-msm@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Arnd Bergmann <arnd@...db.de>, 
	Conor Dooley <conor+dt@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Uwe Kleine-König <u.kleine-koenig@...libre.com>
Subject: Re: [RFC PATCH 5/6] bus: qcom-sc7180: Attach pm domain to watchdog device

Quoting Rob Herring (2025-01-10 06:09:34)
> > diff --git a/drivers/bus/qcom/qcom-sc7180.c b/drivers/bus/qcom/qcom-sc7180.c
> > index a615cf5a2129..7dfe6b32efef 100644
> > --- a/drivers/bus/qcom/qcom-sc7180.c
> > +++ b/drivers/bus/qcom/qcom-sc7180.c
> > @@ -3,18 +3,140 @@
> >   * SoC bus driver for Qualcomm SC7180 SoCs
> >   */
> >
> > +#include <linux/cleanup.h>
> > +#include <linux/clk.h>
> >  #include <linux/device.h>
> > +#include <linux/dev_printk.h>
[...]
> > +
> > +static int qcom_soc_domain_power_on(struct generic_pm_domain *domain)
> > +{
> > +     struct qcom_soc_pm_domain *soc_domain;
> > +
> > +     pr_info("Powering on device\n");
> > +     soc_domain = gpd_to_qcom_soc_pm_domain(domain);
> > +
> > +     return clk_prepare_enable(soc_domain->clk);
> > +}
> > +
> > +static int qcom_soc_domain_power_off(struct generic_pm_domain *domain)
> > +{
> > +     struct qcom_soc_pm_domain *soc_domain;
> > +
> > +     pr_info("Powering off device\n");
> > +     soc_domain = gpd_to_qcom_soc_pm_domain(domain);
> > +
> > +     clk_disable_unprepare(soc_domain->clk);
>
> How's this going to scale when there are multiple clocks and it's not
> just turn on/off all the clocks in any order? Or when there's ordering
> requirements between different resources.

We'll need different power_on/power_off functions when the ordering
requirements are different. It would be similar to how we have different
clk_ops for different types of clks.

>
> I'm pretty sure I've seen attempts to order clock entries in DT based on
> the order they want to enable them.

Yes, I've seen that too. The order in DT shouldn't matter. The SoC PM
driver will know what order of operations to take, including between
different resources like resets, interconnects, power-domains, etc.
That's the general idea of this driver, it will coordinate all the power
for devices in the soc node, because it's written for that particular
SoC. For example, if there are ordering requirements it can get clks by
name and "do the right thing".

Another goal I have is to power off devices that aren't bound to a
driver, and/or never will be. If we can figure out the runtime PM state
of devices before adding them to the platform bus it would allow us to
power those devices off at runtime or during system suspend if userspace
isn't actively trying to power down devices. Maybe to do that we'll have
to be notified by subsystem frameworks when a provider is registered and
then once all the providers are registered, get the PM resources like
clks and interconnects, etc., figure out if they're on/off, set the
runtime PM state of the device to match, and finally add the device to
the bus. Then we can extend the driver PM core to allow userspace to
turn off devices that aren't bound to a driver, because we've moved the
SoC PM glue out of each driver into this one driver that both adds the
devices to the system and manages the device power.

If a node is status = "disabled" I'd like to still get all the PM
resources for that device and either put them into one overall SoC PM
domain, or in an "unused device" domain that we can have userspace tell
the kernel it's safe to power down those devices that were left in a
(semi-)powered state by the bootloader. Obviously I haven't gotten to
this point yet, but it's another TODO item. We could also populate those
devices but never let them be bound to a driver because they're marked
disabled in DT. Then we don't have to do anything different from devices
that are ok to use, but we waste kernel memory. Either way, the PM
resources for disabled devices need to be dealt with.

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int qcom_soc_add_clk_domain(struct platform_device *socdev,
> > +                                struct platform_device *pdev)
> > +{
> > +     struct qcom_soc_pm_domain *domain;
> > +     struct generic_pm_domain *pd;
> > +     int ret;
> > +
> > +     domain = devm_kzalloc(&socdev->dev, sizeof(*domain), GFP_KERNEL);
> > +     if (!domain)
> > +             return -ENOMEM;
> > +
> > +     pd = &domain->pd;
> > +     pd->name = "wdog";
> > +     ret = pm_genpd_init(pd, NULL, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* TODO: Wrap this in a generic_pm_domain function similar to power_on() */
> > +     pd->domain.activate = qcom_soc_domain_activate;
> > +     pd->power_on = qcom_soc_domain_power_on;
> > +     pd->power_off = qcom_soc_domain_power_off;
> > +
> > +     dev_info(&socdev->dev, "adding pm domain for %s\n", dev_name(&pdev->dev));
> > +     dev_pm_domain_set(&pdev->dev, &pd->domain);
> > +
> > +     return 0;
> > +}
> >
> >  static int qcom_soc_sc7180_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct device_node *np = dev->of_node;
> > +     struct platform_device *sdev;
> > +     int ret;
> > +
> > +     sdev = qcom_soc_alloc_device(pdev, "qcom,apss-wdt-sc7180");
>
> We're going to have to have an explicit call for every child node?

Probably! Or we can populate the "complicated" ones that require
something different and then populate the rest of the nodes that didn't
get populated explicitly with some sort of loop over non-populated nodes
that attaches a simple pm domain that does generic on/off sequencing. I
haven't gotten that far to know if populating everything explicitly will
be a problem.

Powered by blists - more mailing lists