| Message ID | 20171019164443.487-1-romain.izard.pro@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 19 Oct 2017, Romain Izard wrote: > The controller used by a flexcom module is configured at boot, and left > alone after this. As the configuration will be lost after backup mode, > restore the state of the flexcom driver on resume. > > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> > --- > Changes in v5: > * extract from the patch series, and send as a standalone patch > > drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 50 insertions(+), 15 deletions(-) > > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c > index 064bde9cff5a..ef1235c4a179 100644 > --- a/drivers/mfd/atmel-flexcom.c > +++ b/drivers/mfd/atmel-flexcom.c > @@ -39,34 +39,44 @@ > #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ > FLEX_MR_OPMODE_MASK) > > +struct atmel_flexcom { > + void __iomem *base; > + u32 opmode; > + struct clk *clk; > +}; > > static int atmel_flexcom_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > - struct clk *clk; > struct resource *res; > - void __iomem *base; > - u32 opmode; > + struct atmel_flexcom *afc; Nit: I'd prefer if you call this 'ddata'. But the concept and implementation is fine, so if you're going to change it please do so and apply my: Acked-by: Lee Jones <lee.jones@linaro.org>
On Mon, 23 Oct 2017, Lee Jones wrote: > On Thu, 19 Oct 2017, Romain Izard wrote: > > > The controller used by a flexcom module is configured at boot, and left > > alone after this. As the configuration will be lost after backup mode, > > restore the state of the flexcom driver on resume. > > > > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > > Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> > > --- > > Changes in v5: > > * extract from the patch series, and send as a standalone patch > > > > drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 50 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c > > index 064bde9cff5a..ef1235c4a179 100644 > > --- a/drivers/mfd/atmel-flexcom.c > > +++ b/drivers/mfd/atmel-flexcom.c > > @@ -39,34 +39,44 @@ > > #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ > > FLEX_MR_OPMODE_MASK) > > > > +struct atmel_flexcom { > > + void __iomem *base; > > + u32 opmode; > > + struct clk *clk; > > +}; > > > > static int atmel_flexcom_probe(struct platform_device *pdev) > > { > > struct device_node *np = pdev->dev.of_node; > > - struct clk *clk; > > struct resource *res; > > - void __iomem *base; > > - u32 opmode; > > + struct atmel_flexcom *afc; > > Nit: I'd prefer if you call this 'ddata'. > > But the concept and implementation is fine, so if you're going to > change it please do so and apply my: > > Acked-by: Lee Jones <lee.jones@linaro.org> Also, 'back-up mode' isn't really a thing is it? How about "Reinstall state on resume" or similar?
2017-10-23 18:07 GMT+02:00 Lee Jones <lee.jones@linaro.org>: > On Mon, 23 Oct 2017, Lee Jones wrote: >> On Thu, 19 Oct 2017, Romain Izard wrote: >> >> > The controller used by a flexcom module is configured at boot, and left >> > alone after this. As the configuration will be lost after backup mode, >> > restore the state of the flexcom driver on resume. >> > >> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> >> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> >> > Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> >> > --- >> > Changes in v5: >> > * extract from the patch series, and send as a standalone patch >> > >> > drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++----------- >> > 1 file changed, 50 insertions(+), 15 deletions(-) >> > >> > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c >> > index 064bde9cff5a..ef1235c4a179 100644 >> > --- a/drivers/mfd/atmel-flexcom.c >> > +++ b/drivers/mfd/atmel-flexcom.c >> > @@ -39,34 +39,44 @@ >> > #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ >> > FLEX_MR_OPMODE_MASK) >> > >> > +struct atmel_flexcom { >> > + void __iomem *base; >> > + u32 opmode; >> > + struct clk *clk; >> > +}; >> > >> > static int atmel_flexcom_probe(struct platform_device *pdev) >> > { >> > struct device_node *np = pdev->dev.of_node; >> > - struct clk *clk; >> > struct resource *res; >> > - void __iomem *base; >> > - u32 opmode; >> > + struct atmel_flexcom *afc; >> >> Nit: I'd prefer if you call this 'ddata'. >> >> But the concept and implementation is fine, so if you're going to >> change it please do so and apply my: >> >> Acked-by: Lee Jones <lee.jones@linaro.org> > > Also, 'back-up mode' isn't really a thing is it? > > How about "Reinstall state on resume" or similar? > The expression comes from the SAMA5D2's datasheet. Other Atmel chips use a different single suspend mode with Linux, where the SoC remains completely powered with a slow clock. The registers are preserved in this mode, so there is no need for a specific suspend and resume code. The SoC can also be powered down, but the CPU is reset and only a small part is powered with a backup battery to maintain a valid RTC and a small internal SRAM. In the SAMA5D2, the mode with only the backup power supply has been extended to isolate the memory I/O lines, making it possible to keep the external SDRAM memory in self-refresh. This mode has a lower consumption compared to the slow clock mode, but it has a higher wakeup latency, and needs specific software support in the bootloader and the kernel. As a result, the "backup mode" expression is used to contrast with the "slow clock" expression when describing the different suspend modes supported by the chip. But if you think that it is necessary, I can reword the commit. Best regards,
On 23/10/2017 at 19:03, Romain Izard wrote: > 2017-10-23 18:07 GMT+02:00 Lee Jones <lee.jones@linaro.org>: >> On Mon, 23 Oct 2017, Lee Jones wrote: >>> On Thu, 19 Oct 2017, Romain Izard wrote: >>> >>>> The controller used by a flexcom module is configured at boot, and left >>>> alone after this. As the configuration will be lost after backup mode, >>>> restore the state of the flexcom driver on resume. >>>> >>>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> >>>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> >>>> Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> >>>> --- >>>> Changes in v5: >>>> * extract from the patch series, and send as a standalone patch >>>> >>>> drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++----------- >>>> 1 file changed, 50 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c >>>> index 064bde9cff5a..ef1235c4a179 100644 >>>> --- a/drivers/mfd/atmel-flexcom.c >>>> +++ b/drivers/mfd/atmel-flexcom.c >>>> @@ -39,34 +39,44 @@ >>>> #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ >>>> FLEX_MR_OPMODE_MASK) >>>> >>>> +struct atmel_flexcom { >>>> + void __iomem *base; >>>> + u32 opmode; >>>> + struct clk *clk; >>>> +}; >>>> >>>> static int atmel_flexcom_probe(struct platform_device *pdev) >>>> { >>>> struct device_node *np = pdev->dev.of_node; >>>> - struct clk *clk; >>>> struct resource *res; >>>> - void __iomem *base; >>>> - u32 opmode; >>>> + struct atmel_flexcom *afc; >>> >>> Nit: I'd prefer if you call this 'ddata'. >>> >>> But the concept and implementation is fine, so if you're going to >>> change it please do so and apply my: >>> >>> Acked-by: Lee Jones <lee.jones@linaro.org> >> >> Also, 'back-up mode' isn't really a thing is it? >> >> How about "Reinstall state on resume" or similar? >> > > The expression comes from the SAMA5D2's datasheet. > > Other Atmel chips use a different single suspend mode with Linux, where > the SoC remains completely powered with a slow clock. The registers are > preserved in this mode, so there is no need for a specific suspend and > resume code. > > The SoC can also be powered down, but the CPU is reset and only a small > part is powered with a backup battery to maintain a valid RTC and a > small internal SRAM. > > In the SAMA5D2, the mode with only the backup power supply has been > extended to isolate the memory I/O lines, making it possible to keep the > external SDRAM memory in self-refresh. This mode has a lower consumption > compared to the slow clock mode, but it has a higher wakeup latency, and > needs specific software support in the bootloader and the kernel. > > As a result, the "backup mode" expression is used to contrast with the > "slow clock" expression when describing the different suspend modes > supported by the chip. aka: Ultra Low Power modes (ULP0 and ULP1). > But if you think that it is necessary, I can reword the commit. Thanks for the whole explanation Romain. Yes we have such a wording in our documents and we used some kind of "Backup mode", "Backup+Self-Refresh (B+SR or B+S-R)", "Backup and DDR in Self-refresh" or "suspend-to-mem" wording for our patches. I take advantage of this discussion to list all them here, for the record ;-) So, I have the feeling that together with the commit message itself, we can go with this wording. Best regards,
On Mon, 23 Oct 2017, Romain Izard wrote: > 2017-10-23 18:07 GMT+02:00 Lee Jones <lee.jones@linaro.org>: > > On Mon, 23 Oct 2017, Lee Jones wrote: > >> On Thu, 19 Oct 2017, Romain Izard wrote: > >> > >> > The controller used by a flexcom module is configured at boot, and left > >> > alone after this. As the configuration will be lost after backup mode, > >> > restore the state of the flexcom driver on resume. > >> > > >> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > >> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > >> > Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> > >> > --- > >> > Changes in v5: > >> > * extract from the patch series, and send as a standalone patch > >> > > >> > drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++----------- > >> > 1 file changed, 50 insertions(+), 15 deletions(-) > >> > > >> > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c > >> > index 064bde9cff5a..ef1235c4a179 100644 > >> > --- a/drivers/mfd/atmel-flexcom.c > >> > +++ b/drivers/mfd/atmel-flexcom.c > >> > @@ -39,34 +39,44 @@ > >> > #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ > >> > FLEX_MR_OPMODE_MASK) > >> > > >> > +struct atmel_flexcom { > >> > + void __iomem *base; > >> > + u32 opmode; > >> > + struct clk *clk; > >> > +}; > >> > > >> > static int atmel_flexcom_probe(struct platform_device *pdev) > >> > { > >> > struct device_node *np = pdev->dev.of_node; > >> > - struct clk *clk; > >> > struct resource *res; > >> > - void __iomem *base; > >> > - u32 opmode; > >> > + struct atmel_flexcom *afc; > >> > >> Nit: I'd prefer if you call this 'ddata'. > >> > >> But the concept and implementation is fine, so if you're going to > >> change it please do so and apply my: > >> > >> Acked-by: Lee Jones <lee.jones@linaro.org> > > > > Also, 'back-up mode' isn't really a thing is it? > > > > How about "Reinstall state on resume" or similar? > > > > The expression comes from the SAMA5D2's datasheet. > > Other Atmel chips use a different single suspend mode with Linux, where > the SoC remains completely powered with a slow clock. The registers are > preserved in this mode, so there is no need for a specific suspend and > resume code. > > The SoC can also be powered down, but the CPU is reset and only a small > part is powered with a backup battery to maintain a valid RTC and a > small internal SRAM. > > In the SAMA5D2, the mode with only the backup power supply has been > extended to isolate the memory I/O lines, making it possible to keep the > external SDRAM memory in self-refresh. This mode has a lower consumption > compared to the slow clock mode, but it has a higher wakeup latency, and > needs specific software support in the bootloader and the kernel. > > As a result, the "backup mode" expression is used to contrast with the > "slow clock" expression when describing the different suspend modes > supported by the chip. > > But if you think that it is necessary, I can reword the commit. No, no need. Thanks for the excellent explanation. It might be worth you providing a succinct description of the datasheet's meaning of "back-up mode" though.
diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c index 064bde9cff5a..ef1235c4a179 100644 --- a/drivers/mfd/atmel-flexcom.c +++ b/drivers/mfd/atmel-flexcom.c @@ -39,34 +39,44 @@ #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ FLEX_MR_OPMODE_MASK) +struct atmel_flexcom { + void __iomem *base; + u32 opmode; + struct clk *clk; +}; static int atmel_flexcom_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; - struct clk *clk; struct resource *res; - void __iomem *base; - u32 opmode; + struct atmel_flexcom *afc; int err; + u32 val; + + afc = devm_kzalloc(&pdev->dev, sizeof(*afc), GFP_KERNEL); + if (!afc) + return -ENOMEM; - err = of_property_read_u32(np, "atmel,flexcom-mode", &opmode); + platform_set_drvdata(pdev, afc); + + err = of_property_read_u32(np, "atmel,flexcom-mode", &afc->opmode); if (err) return err; - if (opmode < ATMEL_FLEXCOM_MODE_USART || - opmode > ATMEL_FLEXCOM_MODE_TWI) + if (afc->opmode < ATMEL_FLEXCOM_MODE_USART || + afc->opmode > ATMEL_FLEXCOM_MODE_TWI) return -EINVAL; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(base)) - return PTR_ERR(base); + afc->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(afc->base)) + return PTR_ERR(afc->base); - clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(clk)) - return PTR_ERR(clk); + afc->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(afc->clk)) + return PTR_ERR(afc->clk); - err = clk_prepare_enable(clk); + err = clk_prepare_enable(afc->clk); if (err) return err; @@ -76,9 +86,10 @@ static int atmel_flexcom_probe(struct platform_device *pdev) * inaccessible and are read as zero. Also the external I/O lines of the * Flexcom are muxed to reach the selected device. */ - writel(FLEX_MR_OPMODE(opmode), base + FLEX_MR); + val = FLEX_MR_OPMODE(afc->opmode); + writel(val, afc->base + FLEX_MR); - clk_disable_unprepare(clk); + clk_disable_unprepare(afc->clk); return devm_of_platform_populate(&pdev->dev); } @@ -89,10 +100,34 @@ static const struct of_device_id atmel_flexcom_of_match[] = { }; MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match); +#ifdef CONFIG_PM_SLEEP +static int atmel_flexcom_resume(struct device *dev) +{ + struct atmel_flexcom *afc = dev_get_drvdata(dev); + int err; + u32 val; + + err = clk_prepare_enable(afc->clk); + if (err) + return err; + + val = FLEX_MR_OPMODE(afc->opmode), + writel(val, afc->base + FLEX_MR); + + clk_disable_unprepare(afc->clk); + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(atmel_flexcom_pm_ops, NULL, + atmel_flexcom_resume); + static struct platform_driver atmel_flexcom_driver = { .probe = atmel_flexcom_probe, .driver = { .name = "atmel_flexcom", + .pm = &atmel_flexcom_pm_ops, .of_match_table = atmel_flexcom_of_match, }, };