Message-ID: <9349e633a64a2275857b2c08731e40721e5af82b.camel@nxp.com>
Date:   Fri, 12 Aug 2022 16:13:41 +0800
From:   Liu Ying <victor.liu@....com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Saravana Kannan <saravanak@...gle.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v3 2/3] drivers: bus: simple-pm-bus: Use clocks

On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote:
> Hi Liu,

Hi Geert,

> 
> On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@....com> wrote:
> > Simple Power-Managed bus controller may need functional clock(s)
> > to be enabled before child devices connected to the bus can be
> > accessed.  Get the clock(s) as a bulk and enable/disable the
> > clock(s) when the bus is being power managed.
> > 
> > One example is that Freescale i.MX8qxp pixel link MSI bus
> > controller
> > needs MSI clock and AHB clock to be enabled before accessing child
> > devices.
> > 
> > Signed-off-by: Liu Ying <victor.liu@....com>
> 
> Thanks for your patch!

Thanks for the review.

> 
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -8,11 +8,17 @@
> >   * for more details.
> >   */
> > 
> > +#include <linux/clk.h>
> >  #include <linux/module.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > 
> > +struct simple_pm_bus {
> > +       struct clk_bulk_data *clks;
> > +       int num_clks;
> > +};
> > +
> >  static const struct of_device_id simple_pm_bus_child_matches[] = {
> >         { .compatible = "simple-mfd", },
> >         {}
> > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct
> > platform_device *pdev)
> >         const struct of_dev_auxdata *lookup =
> > dev_get_platdata(dev);
> >         struct device_node *np = dev->of_node;
> >         const struct of_device_id *match;
> > +       struct simple_pm_bus *bus;
> > 
> >         /*
> >          * Allow user to use driver_override to bind this driver to
> > a
> > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct
> > platform_device *pdev)
> >                         return -ENODEV;
> >         }
> > 
> > +       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> > +       if (!bus)
> > +               return -ENOMEM;
> > +
> > +       bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus-
> > > clks);
> > 
> > +       if (bus->num_clks < 0)
> > +               return dev_err_probe(&pdev->dev, bus->num_clks,
> > "failed to get clocks\n");
> > +
> > +       dev_set_drvdata(&pdev->dev, bus);
> > +
> >         dev_dbg(&pdev->dev, "%s\n", __func__);
> > 
> >         pm_runtime_enable(&pdev->dev);
> 
> While I agree this patch has merits on its own[*], I am wondering
> if you really need it for your use case.
> 
> In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel
> link MSI bus binding", I see your bus has both "clocks" and
> "power-domains" properties.  Perhaps your PM Domain can be a clock
> domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing
> generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock
> handling over to Runtime PM?

It looks like most(if not all) PM domains can be clock domains with
GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks
set. So, technically, my PM domain(scu-pd.c) can be a clock domain with
all those set and a special check for "simple-pm-bus" in the
{at,de}tach_dev callbacks.  But, I'm not sure if it is appropriate to
do that. How do we determine clocks should be managed by PM domains or
device drivers? Technically, both would work...

> 
> [*] The simple-pm-bus DT bindings state:
> 
>       clocks: true
>         # Functional clocks
>         # Required if power-domains is absent, optional otherwise
> 
>       power-domains:
>         # Required if clocks is absent, optional otherwise
>         minItems: 1
> 
> While "power-domains" (+ "clocks" in case of a clock domain) is
> handled through Runtime PM, the current driver indeed does not handle
> "clocks" in the absence of the "power-domains" property.  It looks

Right.

> like all existing users that use "clocks" rely on the PM Domain being
> a clock domain.

"renesas,bsc" seems to be one of the users.

> 
> With your patch, the clocks might be controlled twice: once
> explicitly,
> through clk_bulk_*(), and a second time implicitly, through Runtime
> PM.
> While this works fine to do clock enable counters, it looks
> suboptimal
> to me.  This could be avoided by making the new explicit clock code
> depend on the absence of the "power-domains" property, but that would
> break users that have both a PM Domain which is not a clock domain,
> and clocks.  So we may have no other option.

Hmm, I'm not sure if there are really users that have both a PM domain
which is not a clock domain and clocks, given that a PM domain can sort
of always be a clock domain by setting that GENPD flag and those
callbacks.  So, what do you suggest? Keep the patch as-is? Or, maybe,
make my PM domain additionally be a clock domain?

Regards,
Liu Ying

Powered by blists - more mailing lists