Re: [PATCH] gpio: mxc: add power management support
Hi Anson, On Mon, Jul 16, 2018 at 3:14 AM, Anson Huang wrote: > OK, since i.MX7D has same GPIO type as i.MX35, to make it simple, I just added > a flag to indicate whether it supports save/restore, only i.MX7D enables In imx7s.dtsi the gpio nodes have: compatible = "fsl,imx7d-gpio", "fsl,imx35-gpio"; If you add fsl,imx7d-gpio entry in the gpio driver, then the match will be done against "fsl,imx7d-gpio" since it is more specific. Then in the mx8 dts you can add: compatible = "fsl,imx8m-gpio", "fsl,imx7d-gpio"; and it will also match the more generic "fsl,imx7d-gpio" compatible. > the flag now, since other i.MX8 SoCs' dts is NOT upstreamed yet, so I did NOT > add > support for i.MX8 SoCs, please help review V2 patch, thanks. I did not see the v2. Did you put me on Cc?
RE: [PATCH] gpio: mxc: add power management support
Hi, Fabio Anson Huang Best Regards! > -Original Message- > From: Fabio Estevam [mailto:feste...@gmail.com] > Sent: Sunday, July 15, 2018 12:13 AM > To: Anson Huang > Cc: Linus Walleij ; open list:GPIO SUBSYSTEM > ; linux-kernel ; > dl-linux-imx > Subject: Re: [PATCH] gpio: mxc: add power management support > > Hi Anson, > > On Fri, Jul 13, 2018 at 10:53 PM, Anson Huang > wrote: > > > Here are the details, i.MX7D LPSR mode and i.MX8QM/8QXP etc.' > > suspend/resume may cause GPIO bank lose power, so need to > > save/restore, for other i.MX SoCs, although no need to do > > save/restore, but doing it is NOT harmful, so do you think we should add SoC > type check here? > > I think it would be safer to restrict the save/restore operations to mx7/mx8. > > You can add a fsl,imx7d-gpio compatible entry in the driver. > > Thanks OK, since i.MX7D has same GPIO type as i.MX35, to make it simple, I just added a flag to indicate whether it supports save/restore, only i.MX7D enables the flag now, since other i.MX8 SoCs' dts is NOT upstreamed yet, so I did NOT add support for i.MX8 SoCs, please help review V2 patch, thanks. Anson.
Re: [PATCH] gpio: mxc: add power management support
Hi Anson, On Fri, Jul 13, 2018 at 10:53 PM, Anson Huang wrote: > Here are the details, i.MX7D LPSR mode and i.MX8QM/8QXP etc.' suspend/resume > may cause GPIO bank lose power, so need to save/restore, for other i.MX SoCs, > although no need to do save/restore, but doing it is NOT harmful, so do you > think > we should add SoC type check here? I think it would be safer to restrict the save/restore operations to mx7/mx8. You can add a fsl,imx7d-gpio compatible entry in the driver. Thanks
RE: [PATCH] gpio: mxc: add power management support
Hi, Fabio
Anson Huang
Best Regards!
> -Original Message-
> From: Fabio Estevam [mailto:feste...@gmail.com]
> Sent: Saturday, July 14, 2018 2:34 AM
> To: Anson Huang
> Cc: Linus Walleij ; open list:GPIO SUBSYSTEM
> ; linux-kernel ;
> dl-linux-imx
> Subject: Re: [PATCH] gpio: mxc: add power management support
>
> Hi Anson,
>
> On Tue, Jul 10, 2018 at 4:48 AM, Anson Huang
> wrote:
> > GPIO registers could lose context on some i.MX SoCs, like on i.MX7D,
> > when enter LPSR mode, the whole SoC
>
> After further reviewing this patchI have a question: here you say that i.MX7D
> needs to save some registers.
>
> > will be powered off except LPSR domain, GPIO banks will lose context
> > in this case, need to restore the context after resume from LPSR mode.
> >
> > This patch adds GPIO save/restore for those necessary registers, and
> > put the save/restore operations in noirq suspend/resume phase, since
> > GPIO is fundamental module which could be used by other peripherals'
> > resume phase.
> >
> > Signed-off-by: Anson Huang
> > ---
> > drivers/gpio/gpio-mxc.c | 68
> > +
> > 1 file changed, 68 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index
> > 2f28299..0fc52d8 100644
> > --- a/drivers/gpio/gpio-mxc.c
> > +++ b/drivers/gpio/gpio-mxc.c
> > @@ -45,6 +45,15 @@ struct mxc_gpio_hwdata {
> > unsigned fall_edge;
> > };
> >
> > +struct mxc_gpio_reg_saved {
> > + u32 icr1;
> > + u32 icr2;
> > + u32 imr;
> > + u32 gdir;
> > + u32 edge_sel;
> > + u32 dr;
> > +};
> > +
> > struct mxc_gpio_port {
> > struct list_head node;
> > void __iomem *base;
> > @@ -55,6 +64,7 @@ struct mxc_gpio_port {
> > struct gpio_chip gc;
> > struct device *dev;
> > u32 both_edges;
> > + struct mxc_gpio_reg_saved gpio_saved_reg;
> > };
> >
> > static struct mxc_gpio_hwdata imx1_imx21_gpio_hwdata = { @@ -497,6
> > +507,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> >
> > list_add_tail(&port->node, &mxc_gpio_ports);
> >
> > + platform_set_drvdata(pdev, port);
> > +
> > return 0;
> >
> > out_irqdomain_remove:
> > @@ -507,11 +519,67 @@ static int mxc_gpio_probe(struct platform_device
> *pdev)
> > return err;
> > }
> >
> > +static void mxc_gpio_save_regs(struct mxc_gpio_port *port) {
> > + if (mxc_gpio_hwtype == IMX21_GPIO)
> > + return;
>
> but here you only block IMX21_GPIO.
>
> This means that mx31/mx35/mx51/mx53/mx6 will execute this code too now.
> Is this always safe?
>
> Shouldn't it this save/restore be executed only on mx7d?
>
> Please clarify.
Here are the details, i.MX7D LPSR mode and i.MX8QM/8QXP etc.' suspend/resume
may cause GPIO bank lose power, so need to save/restore, for other i.MX SoCs,
although no need to do save/restore, but doing it is NOT harmful, so do you
think
we should add SoC type check here?
Anson.
Re: [PATCH] gpio: mxc: add power management support
Hi Anson,
On Tue, Jul 10, 2018 at 4:48 AM, Anson Huang wrote:
> GPIO registers could lose context on some i.MX SoCs,
> like on i.MX7D, when enter LPSR mode, the whole SoC
After further reviewing this patchI have a question: here you say that
i.MX7D needs to save some registers.
> will be powered off except LPSR domain, GPIO banks
> will lose context in this case, need to restore
> the context after resume from LPSR mode.
>
> This patch adds GPIO save/restore for those necessary
> registers, and put the save/restore operations in noirq
> suspend/resume phase, since GPIO is fundamental module
> which could be used by other peripherals' resume phase.
>
> Signed-off-by: Anson Huang
> ---
> drivers/gpio/gpio-mxc.c | 68
> +
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 2f28299..0fc52d8 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -45,6 +45,15 @@ struct mxc_gpio_hwdata {
> unsigned fall_edge;
> };
>
> +struct mxc_gpio_reg_saved {
> + u32 icr1;
> + u32 icr2;
> + u32 imr;
> + u32 gdir;
> + u32 edge_sel;
> + u32 dr;
> +};
> +
> struct mxc_gpio_port {
> struct list_head node;
> void __iomem *base;
> @@ -55,6 +64,7 @@ struct mxc_gpio_port {
> struct gpio_chip gc;
> struct device *dev;
> u32 both_edges;
> + struct mxc_gpio_reg_saved gpio_saved_reg;
> };
>
> static struct mxc_gpio_hwdata imx1_imx21_gpio_hwdata = {
> @@ -497,6 +507,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>
> list_add_tail(&port->node, &mxc_gpio_ports);
>
> + platform_set_drvdata(pdev, port);
> +
> return 0;
>
> out_irqdomain_remove:
> @@ -507,11 +519,67 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> return err;
> }
>
> +static void mxc_gpio_save_regs(struct mxc_gpio_port *port)
> +{
> + if (mxc_gpio_hwtype == IMX21_GPIO)
> + return;
but here you only block IMX21_GPIO.
This means that mx31/mx35/mx51/mx53/mx6 will execute this code too
now. Is this always safe?
Shouldn't it this save/restore be executed only on mx7d?
Please clarify.
Re: [PATCH] gpio: mxc: add power management support
On Fri, Jul 13, 2018 at 4:11 AM, Linus Walleij wrote: > On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > >> GPIO registers could lose context on some i.MX SoCs, >> like on i.MX7D, when enter LPSR mode, the whole SoC >> will be powered off except LPSR domain, GPIO banks >> will lose context in this case, need to restore >> the context after resume from LPSR mode. >> >> This patch adds GPIO save/restore for those necessary >> registers, and put the save/restore operations in noirq >> suspend/resume phase, since GPIO is fundamental module >> which could be used by other peripherals' resume phase. >> >> Signed-off-by: Anson Huang > > Hoping for some review on this before applying... > Fabio? Bartosz? Now I could find the patch on my Gmail Inbox :-) It looks good to me: Reviewed-by: Fabio Estevam
Re: [PATCH] gpio: mxc: add power management support
2018-07-13 9:11 GMT+02:00 Linus Walleij : > On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > >> GPIO registers could lose context on some i.MX SoCs, >> like on i.MX7D, when enter LPSR mode, the whole SoC >> will be powered off except LPSR domain, GPIO banks >> will lose context in this case, need to restore >> the context after resume from LPSR mode. >> >> This patch adds GPIO save/restore for those necessary >> registers, and put the save/restore operations in noirq >> suspend/resume phase, since GPIO is fundamental module >> which could be used by other peripherals' resume phase. >> >> Signed-off-by: Anson Huang > > Hoping for some review on this before applying... > Fabio? Bartosz? > > Yours, > Linus Walleij I'm not familiar with these SoCs but the code looks good and clean to me. Reviewed-by: Bartosz Golaszewski
Re: [PATCH] gpio: mxc: add power management support
On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > GPIO registers could lose context on some i.MX SoCs, > like on i.MX7D, when enter LPSR mode, the whole SoC > will be powered off except LPSR domain, GPIO banks > will lose context in this case, need to restore > the context after resume from LPSR mode. > > This patch adds GPIO save/restore for those necessary > registers, and put the save/restore operations in noirq > suspend/resume phase, since GPIO is fundamental module > which could be used by other peripherals' resume phase. > > Signed-off-by: Anson Huang Hoping for some review on this before applying... Fabio? Bartosz? Yours, Linus Walleij
