Message-ID: <DB8PR04MB6826B681CB2CC116D4ECA6D1F16A0@DB8PR04MB6826.eurprd04.prod.outlook.com>
Date:   Thu, 24 Oct 2019 02:38:03 +0000
From:   Ran Wang <ran.wang_1@....com>
To:     Leo Li <leoyang.li@....com>
CC:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Pavel Machek <pavel@....cz>, Anson Huang <anson.huang@....com>,
        Biwen Li <biwen.li@....com>, Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, lkml <linux-kernel@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: RE: [PATCH v9 3/3] soc: fsl: add RCPM driver

Hi Leo,


On Thursday, October 24, 2019 06:48, Li Yang wrote:
> 
> On Wed, Oct 23, 2019 at 3:24 AM Ran Wang <ran.wang_1@....com> wrote:
> >
> > The NXP's QorIQ Processors based on ARM Core have RCPM module
> 
> Actually not just ARM based QorIQ processors are having RCPM, PowerPC based
> QorIQ SoCs also have RCPM.  Does this driver also work with the PowerPC SoCs?
> Please clarify in the commit message and Kconfig description.
> 
> > (Run Control and Power Management), which performs system level tasks
> > associated with power management such as wakeup source control.
> >
> > This driver depends on PM wakeup source framework which help to
> > collect wake information.
> >
> > Signed-off-by: Ran Wang <ran.wang_1@....com>

<snip>


> > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
> > f9ad8ad..4918856 100644
> > --- a/drivers/soc/fsl/Kconfig
> > +++ b/drivers/soc/fsl/Kconfig
> > @@ -40,4 +40,12 @@ config DPAA2_CONSOLE
> >           /dev/dpaa2_mc_console and /dev/dpaa2_aiop_console,
> >           which can be used to dump the Management Complex and AIOP
> >           firmware logs.
> > +
> > +config FSL_RCPM
> > +       bool "Freescale RCPM support"
> > +       depends on PM_SLEEP
> 
> If this is only for ARM, probably add more dependency here?

OK.
 
> > +       help
> > +         The NXP QorIQ Processors based on ARM Core have RCPM module
> > +         (Run Control and Power Management), which performs all device-level
> > +         tasks associated with power management, such as wakeup source
> control.
> >  endmenu

<snip>

> > +
> > +/**
> > + * rcpm_pm_prepare - performs device-level tasks associated with
> > +power
> > + * management, such as programming related to the wakeup source control.
> > + * @dev: Device to handle.
> > + *
> > + */
> > +static int rcpm_pm_prepare(struct device *dev) {
> > +       int i, ret, idx;
> > +       void __iomem *base;
> > +       struct wakeup_source    *ws;
> > +       struct rcpm             *rcpm;
> > +       struct device_node      *np = dev->of_node;
> > +       u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> > +
> > +       rcpm = dev_get_drvdata(dev);
> > +       if (!rcpm)
> > +               return -EINVAL;
> > +
> > +       base = rcpm->ippdexpcr_base;
> > +       idx = wakeup_sources_read_lock();
> > +
> > +       /* Begin with first registered wakeup source */
> > +       for_each_wakeup_source(ws) {
> > +
> > +               /* skip object which is not attached to device */
> > +               if (!ws->dev || !ws->dev->parent)
> > +                       continue;
> > +
> > +               ret = device_property_read_u32_array(ws->dev->parent,
> > +                               "fsl,rcpm-wakeup", value,
> > +                               rcpm->wakeup_cells + 1);
> > +
> > +               /*  Wakeup source should refer to current rcpm device */
> > +               if (ret || (np->phandle != value[0])) {
> > +                       pr_debug("%s doesn't refer to this rcpm\n",
> > + ws->name);
> 
> I agree with Rafael that this looks a little bit weird.

OK, let me remove this print in next version.

> > +                       continue;
> > +               }
> > +
> > +               /* Property "#fsl,rcpm-wakeup-cells" of rcpm node defines the
> > +                * number of IPPDEXPCR register cells, and "fsl,rcpm-wakeup"
> > +                * of wakeup source IP contains an integer array: <phandle to
> > +                * RCPM node, IPPDEXPCR0 setting, IPPDEXPCR1 setting,
> > +                * IPPDEXPCR2 setting, etc>.
> > +                *
> > +                * So we will go thought them and do programming accordngly.
> > +                */
> > +               for (i = 0; i < rcpm->wakeup_cells; i++) {
> > +                       u32 tmp = value[i + 1];
> > +                       void __iomem *address = base + i * 4;
> > +
> > +                       if (!tmp)
> > +                               continue;
> > +
> > +                       /* We can only OR related bits */
> > +                       if (rcpm->little_endian) {
> > +                               tmp |= ioread32(address);
> > +                               iowrite32(tmp, address);
> > +                       } else {
> > +                               tmp |= ioread32be(address);
> > +                               iowrite32be(tmp, address);
> > +                       }
> 
> Can we do read once at the beginning and write once at the end, instead of
> doing IO read/write for every wakeup source?

Sure, but another loop might need to be added after the one of wakeup source walk
through, to program all IPPDEXPCR registers. Is that OK?

Regards,
Ran

 
> > +               }
> > +       }
> > +
> > +       wakeup_sources_read_unlock(idx);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rcpm_pm_ops = {
> > +       .prepare =  rcpm_pm_prepare,
> > +};
> > +

Powered by blists - more mailing lists