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@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxx>
Thanks for your time.
Regards,
Ran
