diff mbox series

[v2] drivers: soc: xilinx: Call InitFinalize from late_initcall_sync instead of probe

Message ID 1599693000-25641-1-git-send-email-amit.sunil.dhamne@xilinx.com (mailing list archive)
State New, archived
Headers show
Series [v2] drivers: soc: xilinx: Call InitFinalize from late_initcall_sync instead of probe | expand

Commit Message

Amit Sunil Dhamne Sept. 9, 2020, 11:10 p.m. UTC 
From: Rajan Vaja <rajan.vaja@xilinx.com>

Initially all devices are in power up state. Firmware expect that
processor should call InitFinalize API once it have requested devices
which are required so that it can turn off all unused devices and
save power. From Linux, PM driver calls InitFinalize to inform the
firmware that it can power down the unused devices. Upon
InitFinalize() call firmware power downs all unused devices.

There are chances that PM driver is probed along with or before other
device drivers. So in that case some of the devices may not be
requested from firmware which is done by genpd driver. Due to that
firmware will consider those devices as unused and firmware will power
down those devices. Later when any device driver is probed, genpd
driver will ask firmware to power up that device using request node
API. So for those devices, power transition will be like on->off->on
which creates unnecessary power glitch to those devices.

To avoid such unnecessary power transitions and as ideal behavior
InitFinalize should be called after all drivers are probed. So call
InitFinalize from late_initcall_sync.

Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
---
Changes in v2:
 - Check for compatible string for zynqmp or versal platform before
   calling init finalize.
---
 drivers/soc/xilinx/zynqmp_power.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Comments

Michal Simek Sept. 10, 2020, 6:49 a.m. UTC | #1 
Hi Arnd,

On 10. 09. 20 1:10, Amit Sunil Dhamne wrote:
> From: Rajan Vaja <rajan.vaja@xilinx.com>
> 
> Initially all devices are in power up state. Firmware expect that
> processor should call InitFinalize API once it have requested devices
> which are required so that it can turn off all unused devices and
> save power. From Linux, PM driver calls InitFinalize to inform the
> firmware that it can power down the unused devices. Upon
> InitFinalize() call firmware power downs all unused devices.
> 
> There are chances that PM driver is probed along with or before other
> device drivers. So in that case some of the devices may not be
> requested from firmware which is done by genpd driver. Due to that
> firmware will consider those devices as unused and firmware will power
> down those devices. Later when any device driver is probed, genpd
> driver will ask firmware to power up that device using request node
> API. So for those devices, power transition will be like on->off->on
> which creates unnecessary power glitch to those devices.
> 
> To avoid such unnecessary power transitions and as ideal behavior
> InitFinalize should be called after all drivers are probed. So call
> InitFinalize from late_initcall_sync.
> 
> Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
> ---
> Changes in v2:
>  - Check for compatible string for zynqmp or versal platform before
>    calling init finalize.
> ---
>  drivers/soc/xilinx/zynqmp_power.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/xilinx/zynqmp_power.c b/drivers/soc/xilinx/zynqmp_power.c
> index 31ff49f..22d2d2e 100644
> --- a/drivers/soc/xilinx/zynqmp_power.c
> +++ b/drivers/soc/xilinx/zynqmp_power.c
> @@ -178,7 +178,6 @@ static int zynqmp_pm_probe(struct platform_device *pdev)
>  	u32 pm_api_version;
>  	struct mbox_client *client;
>  
> -	zynqmp_pm_init_finalize();
>  	zynqmp_pm_get_api_version(&pm_api_version);
>  
>  	/* Check PM API version number */
> @@ -246,6 +245,23 @@ static int zynqmp_pm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __init do_init_finalize(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> +	if (!np) {
> +		np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
> +		if (!np)
> +			return 0;
> +	}
> +	of_node_put(np);
> +
> +	return zynqmp_pm_init_finalize();
> +}
> +
> +late_initcall_sync(do_init_finalize);
> +
>  static const struct of_device_id pm_of_match[] = {
>  	{ .compatible = "xlnx,zynqmp-power", },
>  	{ /* end of table */ },
> 

Arnd: are you fine with this way how to check that it runs on zynqmp or
versal?

Thanks,
Michal
Arnd Bergmann Sept. 10, 2020, 8:35 a.m. UTC | #2 
On Thu, Sep 10, 2020 at 8:50 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > @@ -246,6 +245,23 @@ static int zynqmp_pm_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static int __init do_init_finalize(void)
> > +{
> > +     struct device_node *np;
> > +
> > +     np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> > +     if (!np) {
> > +             np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
> > +             if (!np)
> > +                     return 0;
> > +     }
> > +     of_node_put(np);
> > +
> > +     return zynqmp_pm_init_finalize();
> > +}
> > +
> > +late_initcall_sync(do_init_finalize);
> > +
> >  static const struct of_device_id pm_of_match[] = {
> >       { .compatible = "xlnx,zynqmp-power", },
> >       { /* end of table */ },
> >
>
> Arnd: are you fine with this way how to check that it runs on zynqmp or
> versal?

I might be missing something, but this sounds like the wrong way to do it.
There is already a platform driver probed in the presence of the
"xlnx,zynqmp-power" node in the same file. Wouldn't it be better to
either check for the same node instead of an arbitrarily different set
of SoC names, or to make the platform driver itself get registered
form the late initcall?

       Arnd
Michal Simek Sept. 11, 2020, 9:22 a.m. UTC | #3 
On 10. 09. 20 10:35, Arnd Bergmann wrote:
> On Thu, Sep 10, 2020 at 8:50 AM Michal Simek <michal.simek@xilinx.com> wrote:
>>> @@ -246,6 +245,23 @@ static int zynqmp_pm_remove(struct platform_device *pdev)
>>>       return 0;
>>>  }
>>>
>>> +static int __init do_init_finalize(void)
>>> +{
>>> +     struct device_node *np;
>>> +
>>> +     np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
>>> +     if (!np) {
>>> +             np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
>>> +             if (!np)
>>> +                     return 0;
>>> +     }
>>> +     of_node_put(np);
>>> +
>>> +     return zynqmp_pm_init_finalize();
>>> +}
>>> +
>>> +late_initcall_sync(do_init_finalize);
>>> +
>>>  static const struct of_device_id pm_of_match[] = {
>>>       { .compatible = "xlnx,zynqmp-power", },
>>>       { /* end of table */ },
>>>
>>
>> Arnd: are you fine with this way how to check that it runs on zynqmp or
>> versal?
> 
> I might be missing something, but this sounds like the wrong way to do it.
> There is already a platform driver probed in the presence of the
> "xlnx,zynqmp-power" node in the same file. Wouldn't it be better to
> either check for the same node instead of an arbitrarily different set
> of SoC names, or to make the platform driver itself get registered
> form the late initcall?

Rajan/Amit: On the top of my head I expect that you can't call it as
late initcall because you need the first part earlier.

Does it make sense to check same node?
There is and should be only one instance of this driver.
Isn't it easier just to setup one static variable instead of calling dt
functions which will take time and result will be the same.

Thanks,
Michal
Rajan Vaja Sept. 11, 2020, 9:33 a.m. UTC | #4 
Hi Michal,

> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Friday, September 11, 2020 2:52 PM
> To: Arnd Bergmann <arnd@arndb.de>; Michal Simek <michals@xilinx.com>
> Cc: Amit Sunil Dhamne <amitsuni@xilinx.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Ingo Molnar <mingo@kernel.org>; gregkh
> <gregkh@linuxfoundation.org>; Matt Fleming <matt@codeblueprint.co.uk>;
> Sudeep Holla <sudeep.holla@arm.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; Kees Cook <keescook@chromium.org>; Dmitry
> Torokhov <dmitry.torokhov@gmail.com>; Rajan Vaja <RAJANV@xilinx.com>;
> Linux ARM <linux-arm-kernel@lists.infradead.org>; linux-
> kernel@vger.kernel.org; Tejas Patel <TEJASP@xilinx.com>; Jolly Shah
> <JOLLYS@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>
> Subject: Re: [PATCH v2] drivers: soc: xilinx: Call InitFinalize from
> late_initcall_sync instead of probe
> 
> 
> 
> On 10. 09. 20 10:35, Arnd Bergmann wrote:
> > On Thu, Sep 10, 2020 at 8:50 AM Michal Simek <michal.simek@xilinx.com>
> wrote:
> >>> @@ -246,6 +245,23 @@ static int zynqmp_pm_remove(struct
> platform_device *pdev)
> >>>       return 0;
> >>>  }
> >>>
> >>> +static int __init do_init_finalize(void) {
> >>> +     struct device_node *np;
> >>> +
> >>> +     np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> >>> +     if (!np) {
> >>> +             np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
> >>> +             if (!np)
> >>> +                     return 0;
> >>> +     }
> >>> +     of_node_put(np);
> >>> +
> >>> +     return zynqmp_pm_init_finalize(); }
> >>> +
> >>> +late_initcall_sync(do_init_finalize);
> >>> +
> >>>  static const struct of_device_id pm_of_match[] = {
> >>>       { .compatible = "xlnx,zynqmp-power", },
> >>>       { /* end of table */ },
> >>>
> >>
> >> Arnd: are you fine with this way how to check that it runs on zynqmp
> >> or versal?
> >
> > I might be missing something, but this sounds like the wrong way to do it.
> > There is already a platform driver probed in the presence of the
> > "xlnx,zynqmp-power" node in the same file. Wouldn't it be better to
> > either check for the same node instead of an arbitrarily different set
> > of SoC names, or to make the platform driver itself get registered
> > form the late initcall?
> 
> Rajan/Amit: On the top of my head I expect that you can't call it as late initcall
> because you need the first part earlier.
> 
> Does it make sense to check same node?
> There is and should be only one instance of this driver.
> Isn't it easier just to setup one static variable instead of calling dt functions
> which will take time and result will be the same.
[Rajan Vaja] Yes Michal, that make sense. We will make changes and will submit new version.

Thanks,
Rajan
> 
> Thanks,
> Michal
diff mbox series

Patch

diff --git a/drivers/soc/xilinx/zynqmp_power.c b/drivers/soc/xilinx/zynqmp_power.c
index 31ff49f..22d2d2e 100644
--- a/drivers/soc/xilinx/zynqmp_power.c
+++ b/drivers/soc/xilinx/zynqmp_power.c
@@ -178,7 +178,6 @@  static int zynqmp_pm_probe(struct platform_device *pdev)
        u32 pm_api_version;
        struct mbox_client *client;

-       zynqmp_pm_init_finalize();
        zynqmp_pm_get_api_version(&pm_api_version);

        /* Check PM API version number */
@@ -246,6 +245,23 @@  static int zynqmp_pm_remove(struct platform_device *pdev)
        return 0;
 }

+static int __init do_init_finalize(void)
+{
+       struct device_node *np;
+
+       np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
+       if (!np) {
+               np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
+               if (!np)
+                       return 0;
+       }
+       of_node_put(np);
+
+       return zynqmp_pm_init_finalize();
+}
+
+late_initcall_sync(do_init_finalize);
+
 static const struct of_device_id pm_of_match[] = {
        { .compatible = "xlnx,zynqmp-power", },
        { /* end of table */ },