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

[Ran Wang]

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