linux-kernel - RE: [PATCH v9 3/3] soc: fsl: add RCPM driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB8PR04MB682655D179242325ED6EC1FDF16B0@DB8PR04MB6826.eurprd04.prod.outlook.com>
Date:   Wed, 23 Oct 2019 09:42:45 +0000
From:   Ran Wang <ran.wang_1@....com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
CC:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Rob Herring <robh+dt@...nel.org>, Leo Li <leoyang.li@....com>,
        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>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: RE: [PATCH v9 3/3] soc: fsl: add RCPM driver

Hi Rafael,

On Wednesday, October 23, 2019 17:12, Rafael J. Wysocki wrote:
> 
> On Wed, Oct 23, 2019 at 10:24 AM Ran Wang <ran.wang_1@....com> wrote:
> >
> > The NXP's QorIQ Processors based on ARM Core have RCPM module (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>
> > ---
> > Change in v9:
> >         - Add kerneldoc for rcpm_pm_prepare().
> >         - Use pr_debug() to replace dev_info(), to print message when decide
> >           skip current wakeup object, this is mainly for debugging (in order
> >           to detect potential improper implementation on device tree which
> >           might cause this skip).
> >         - Refactor looping implementation in rcpm_pm_prepare(), add more
> >           comments to help clarify.
> >
> > Change in v8:
> >         - Adjust related API usage to meet wakeup.c's update in patch 1/3.
> >         - Add sanity checking for the case of ws->dev or ws->dev->parent
> >           is null.
> >
<snip>
> > +
> > +               /*  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'm still quite unsure why it is useful to print this message instead of printing one
> when the wakeup source does match (there may be many wakeup source
> objects you don't care about in principle), but whatever.

OK, my previous idea was that user might likely mis-understand related description in
bindings when adding node and property "fsl,rcpm-wakeup". Add this print might
help him/her to get alerted that RCPM driver doesn't successfully parsing info which
they didn't expect. Currently on LS1088ARDB board, I can only see one wakeup source
the RCPM driver doesn’t need to care.

> > +                       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);
> > +                       }
> > +               }
> > +       }
> > +
> > +       wakeup_sources_read_unlock(idx);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rcpm_pm_ops = {
> > +       .prepare =  rcpm_pm_prepare,
> > +};
> 
> For the above:
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

Thanks for your time.

Regards,
Ran