RE: [PATCH v1] soc: fsl: rcpm: Add ACPI support

[Ran Wang]

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.alam@xxxxxxxxxxxxxxxx>
> >>> Signed-off-by: kuldip dwivedi <kuldip.dwivedi@xxxxxxxxxxxxxxxx>
> >>
> >> 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.wang_1@xxxxxxx>
> >>>
> >>> +#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,
> >>>
> >