Hi Ard, On Wednesday, September 16, 2020 2:11 PM, Ard Biesheuvel wrote: > Subject: Re: [PATCH v1] soc: fsl: rcpm: Add ACPI support > > On 9/16/20 3:32 AM, Ran Wang wrote: > > Hi Ard, > > > > On Tuesday, September 15, 2020 7:10 PM, Ard Biesheuvel wrote: > >> Subject: Re: [PATCH v1] soc: fsl: rcpm: Add ACPI support > >> > >> On 9/15/20 1:06 PM, kuldip dwivedi wrote: > >>> Add ACPI support in fsl RCPM driver. This is required to support > >>> ACPI > >>> S3 state. S3 is the ACPI sleep state that is known as "sleep" or > >>> "suspend to RAM". > >>> It essentially turns off most power of the system but keeps memory > >>> powered. > >>> > >>> Signed-off-by: tanveer <tanveer.a...@puresoftware.com> > >>> Signed-off-by: kuldip dwivedi <kuldip.dwiv...@puresoftware.com> > >> > >> Why does the OS need to program this device? Can't this be done by > >> firmware? > > > > This device is use to tell HW which IP (such as USB, SDHC, SATA, etc) > > should not be clock gated during system enter low power state (to > > allow that IP work as a wakeup source). And user does this configuration in > device tree. > > The point of ACPI is *not* to describe a DT topology using a table format that > is not suited for it. The point of ACPI is to describe a machine that is more > abstracted from the hardware than is typically possible with DT, where the > abstractions are implemented by AML code that is provided by the firmware, > but executed in the context of the OS. > > So the idea is *not* finding the shortest possible path to get your existing > DT > driver code running on a system that boots via ACPI. > Instead, you should carefully think about the abstract ACPI machine that you > will expose to the OS, and hide everything else in firmware. > > In this particular case, it seems like your USB, SDHC and SATA device objects > may need power state dependent AML methods that program this block > directly.
Actually the scenario is a little bit complicated for RCPM function: it need to query kernel wakeup source framework (see for_each_wakeup_source(ws)) to fetch all potential candidates then do the programming accordingly. If we implement this logic in AML methods, I have no idea how to collect those information stored in wakeup source data of kernel. Regards, Ran > > > > So implement > > this RCPM driver to do it in kernel rather than firmware. > > > > Regards, > > Ran > > > >>> --- > >>> > >>> Notes: > >>> 1. Add ACPI match table > >>> 2. NXP team members are added for confirming HID changes > >>> 3. There is only one node in ACPI so no need to check for > >>> current device explicitly > >>> 4. These changes are tested on LX2160A and LS1046A platforms > >>> > >>> drivers/soc/fsl/rcpm.c | 22 +++++++++++++++++++--- > >>> 1 file changed, 19 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/soc/fsl/rcpm.c b/drivers/soc/fsl/rcpm.c index > >>> a093dbe6d2cb..e75a436fb159 100644 > >>> --- a/drivers/soc/fsl/rcpm.c > >>> +++ b/drivers/soc/fsl/rcpm.c > >>> @@ -2,10 +2,12 @@ > >>> // > >>> // rcpm.c - Freescale QorIQ RCPM driver > >>> // > >>> -// Copyright 2019 NXP > >>> +// Copyright 2019-2020 NXP > >>> +// Copyright 2020 Puresoftware Ltd. > >>> // > >>> // Author: Ran Wang <ran.wan...@nxp.com> > >>> > >>> +#include <linux/acpi.h> > >>> #include <linux/init.h> > >>> #include <linux/module.h> > >>> #include <linux/platform_device.h> @@ -57,8 +59,13 @@ static int > >>> rcpm_pm_prepare(struct device *dev) > >>> rcpm->wakeup_cells + 1); > >>> > >>> /* Wakeup source should refer to current rcpm device */ > >>> - if (ret || (np->phandle != value[0])) > >>> - continue; > >>> + if (is_acpi_node(dev->fwnode)) { > >>> + if (ret) > >>> + continue; > >>> + } else { > >>> + if (ret || (np->phandle != value[0])) > >>> + continue; > >>> + } > >>> > >>> /* Property "#fsl,rcpm-wakeup-cells" of rcpm node > >>> defines the > >>> * number of IPPDEXPCR register cells, and > >>> "fsl,rcpm-wakeup" > >>> @@ -139,10 +146,19 @@ static const struct of_device_id > >>> rcpm_of_match[] > >> = { > >>> }; > >>> MODULE_DEVICE_TABLE(of, rcpm_of_match); > >>> > >>> +#ifdef CONFIG_ACPI > >>> +static const struct acpi_device_id rcpm_acpi_match[] = { > >>> + { "NXP0015", }, > >>> + { } > >>> +}; > >>> +MODULE_DEVICE_TABLE(acpi, rcpm_acpi_match); #endif > >>> + > >>> static struct platform_driver rcpm_driver = { > >>> .driver = { > >>> .name = "rcpm", > >>> .of_match_table = rcpm_of_match, > >>> + .acpi_match_table = ACPI_PTR(rcpm_acpi_match), > >>> .pm = &rcpm_pm_ops, > >>> }, > >>> .probe = rcpm_probe, > >>> > >