While the core of the backup mode for SAMA5D2 has been integrated in
v4.13, it is far from complete. Individual controllers in the chip have
drivers that do not support the reset of the registers during suspend,
and they need to be adapted to handle it.
The first patch uses the clock wakeup code from the prototype backup
mode instead of the version integrated in the mainline, as the mainline
version is not stable. During a test loop with two-second backup
suspend, the mainline version will hang in less than one day, whereas
the prototype version has been running the same test for more than a
week without hanging.
Romain Izard (10):
clk: at91: pmc: Wait for clocks when resuming
clk: at91: pmc: Save SCSR during suspend
clk: at91: pmc: Support backup for programmable clocks
mtd: nand: atmel: Avoid ECC errors when leaving backup mode
mtd: nand: atmel: Report PMECC failures as errors
ehci-atmel: Power down during suspend is normal
iio:adc:at91-sama5d2: Support backup mode
pwm: atmel-tcb: Support backup mode
atmel_flexcom: Support backup mode
tty/serial: atmel: Prevent a warning on suspend
drivers/clk/at91/pmc.c | 33 ++++++++++++------
drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++--------
drivers/mtd/nand/atmel/pmecc.c | 15 ++++----
drivers/pwm/pwm-atmel-tcb.c | 63 +++++++++++++++++++++++++++++++--
drivers/tty/serial/atmel_serial.c | 13 +++++++
drivers/usb/host/ehci-atmel.c | 3 +-
7 files changed, 216 insertions(+), 47 deletions(-)
--
2.11.0
Wait for the syncronization of all clocks when resuming, not only the
UPLL clock. Do not use regmap_read_poll_timeout, as it will call BUG()
when interrupts are masked, which is the case in here.
Signed-off-by: Romain Izard <redacted>
---
drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
During backup mode, the contents of all registers will be cleared as the
SoC will be completely powered down. For a product that boots on NAND
Flash memory, the bootloader will obviously use the related controller
to read the Flash and correct any detected error in the memory, before
handling back control to the kernel's resuming entry point.
In normal devices, it is up to the driver's suspend/resume code to
restore the registers in a valid state. But the PMECC is not a regular
device in the driver model when used with the legacy device tree binding
for the Atmel NAND controller, and suspend/resume code is not called.
As in my case the bootloader leaves the PMECC controller in a programmed
state, and the controller is only reset at boot or after a NAND access,
the first NAND Flash access with the Atmel controller will report
uncorrectable ECC errors.
To avoid this, systematically reset the PMECC controller before using
it.
Signed-off-by: Romain Izard <redacted>
---
drivers/mtd/nand/atmel/pmecc.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
@@ -777,6 +777,9 @@ int atmel_pmecc_enable(struct atmel_pmecc_user *user, int op)mutex_lock(&user->pmecc->lock);+writel(PMECC_CTRL_RST,pmecc->regs.base+ATMEL_PMECC_CTRL);+writel(PMECC_CTRL_DISABLE,pmecc->regs.base+ATMEL_PMECC_CTRL);+cfg=user->cache.cfg;if(op==NAND_ECC_WRITE)cfg|=PMECC_CFG_WRITE_OP;
Save and restore registers for the PWM on suspend and resume, which
makes hibernation and backup modes possible.
Signed-off-by: Romain Izard <redacted>
---
drivers/pwm/pwm-atmel-tcb.c | 63 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 61 insertions(+), 2 deletions(-)
@@ -263,6 +275,7 @@ static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)/* Use software trigger to apply the new setting */__raw_writel(ATMEL_TC_CLKEN|ATMEL_TC_SWTRG,regs+ATMEL_TC_REG(group,CCR));+tcbpwmc->bkup[group].enabled=1;spin_unlock(&tcbpwmc->lock);return0;}
Support the backup mode for platform suspend, by restoring the hardware
registers on resume.
Signed-off-by: Romain Izard <redacted>
---
drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
1 file changed, 61 insertions(+), 10 deletions(-)
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 <redacted>
---
drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 15 deletions(-)
The atmel serial port driver reported the following warning on suspend:
atmel_usart f8020000.serial: ttyS1: Unable to drain transmitter
As the ATMEL_US_TXEMPTY status bit in ATMEL_US_CSR is always cleared
when the transmitter is disabled, we need to know the transmitter's
state to return the real fifo state. And as ATMEL_US_CR is write-only,
it is necessary to save the state of the transmitter in a local
variable, and update the variable when TXEN and TXDIS is written in
ATMEL_US_CR.
After those changes, atmel_tx_empty can return "empty" on suspend, the
warning in uart_suspend_port disappears, and suspending is 20ms shorter
for each enabled Atmel serial port.
Signed-off-by: Romain Izard <redacted>
---
drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
When an Atmel SoC is suspended with the backup mode, the USB bus will be
powered down. As this is expected, do not return an error to the driver
core when ehci_resume detects it.
Signed-off-by: Romain Izard <redacted>
---
drivers/usb/host/ehci-atmel.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
It is not normal for the PMECC to fail when trying to fix ECC errors.
Report these cases as errors.
Signed-off-by: Romain Izard <redacted>
---
drivers/mtd/nand/atmel/pmecc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -687,6 +687,8 @@ static int atmel_pmecc_err_location(struct atmel_pmecc_user *user)*Numberofrootsdoesnotmatchthedegreeofsmu*unabletocorrecterror.*/+dev_err(pmecc->dev,+"PMECC: Impossible to calculate error location.\n");return-EBADMSG;}
@@ -729,7 +731,7 @@ int atmel_pmecc_correct_sector(struct atmel_pmecc_user *user, int sector,ptr=ecc+byte-sectorsize;area="ECC";}else{-dev_dbg(pmecc->dev,+dev_err(pmecc->dev,"Invalid errpos value (%d, max is %d)\n",errpos,(sectorsize+eccbytes)*8);return-EINVAL;
From: Romain Izard <redacted>
Save and restore the System Clock and Programmable Clock register for
the backup use case.
Signed-off-by: Romain Izard <redacted>
---
drivers/clk/at91/pmc.c | 5 +++++
1 file changed, 5 insertions(+)
The contents of the System Clock Status Register (SCSR) needs to be
restored into the System Clock Enable Register (SCER).
As the bootloader will restore some clocks by itself, the issue can be
missed as only the USB controller, the LCD controller, the Image Sensor
controller and the programmable clocks will be impacted.
Fix the obvious typo in the suspend/resume code, as the IMR register
does not need to be saved twice.
Signed-off-by: Romain Izard <redacted>
---
drivers/clk/at91/pmc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -86,7 +86,7 @@ static int pmc_suspend(void){inti;-regmap_read(pmcreg,AT91_PMC_IMR,&pmc_cache.scsr);+regmap_read(pmcreg,AT91_PMC_SCSR,&pmc_cache.scsr);regmap_read(pmcreg,AT91_PMC_PCSR,&pmc_cache.pcsr0);regmap_read(pmcreg,AT91_CKGR_UCKR,&pmc_cache.uckr);regmap_read(pmcreg,AT91_CKGR_MOR,&pmc_cache.mor);
@@ -129,7 +129,7 @@ static void pmc_resume(void)if(pmc_cache.pllar!=tmp)pr_warn("PLLAR was not configured properly by the firmware\n");-regmap_write(pmcreg,AT91_PMC_IMR,pmc_cache.scsr);+regmap_write(pmcreg,AT91_PMC_SCER,pmc_cache.scsr);regmap_write(pmcreg,AT91_PMC_PCER,pmc_cache.pcsr0);regmap_write(pmcreg,AT91_CKGR_UCKR,pmc_cache.uckr);regmap_write(pmcreg,AT91_CKGR_MOR,pmc_cache.mor);
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2017-09-08 16:02:32
On 08/09/2017 at 17:36, Romain Izard wrote:
Support the backup mode for platform suspend, by restoring the hardware
registers on resume.
Signed-off-by: Romain Izard <redacted>
Romain,
Thanks for your series: definitively some of your patches need to be
integrated (I've merged some of them in our current linux-4.9-at91 branch.
However, It seems that some of your additions have already been
submitted and/or accepted by maintainers.
For instance an equivalent of this one seems already in Linus' tree:
500a2eefd6b16ba141a8fb777ea6962d2eb65e3b ("iio: adc: at91-sama5d2_adc:
add support for suspend/resume functionality").
Please tell us if it fits what your observed on this driver (or others).
Regards,
2017-09-08 18:03 GMT+02:00 Nicolas Ferre [off-list ref]:
On 08/09/2017 at 17:36, Romain Izard wrote:
quoted
Support the backup mode for platform suspend, by restoring the hardware
registers on resume.
Signed-off-by: Romain Izard <redacted>
Romain,
Thanks for your series: definitively some of your patches need to be
integrated (I've merged some of them in our current linux-4.9-at91 branch.
However, It seems that some of your additions have already been
submitted and/or accepted by maintainers.
For instance an equivalent of this one seems already in Linus' tree:
500a2eefd6b16ba141a8fb777ea6962d2eb65e3b ("iio: adc: at91-sama5d2_adc:
add support for suspend/resume functionality").
Please tell us if it fits what your observed on this driver (or others).
Regards,
The atmel serial port driver reported the following warning on suspend:
atmel_usart f8020000.serial: ttyS1: Unable to drain transmitter
As the ATMEL_US_TXEMPTY status bit in ATMEL_US_CSR is always cleared
when the transmitter is disabled, we need to know the transmitter's
state to return the real fifo state. And as ATMEL_US_CR is write-only,
it is necessary to save the state of the transmitter in a local
variable, and update the variable when TXEN and TXDIS is written in
ATMEL_US_CR.
After those changes, atmel_tx_empty can return "empty" on suspend, the
warning in uart_suspend_port disappears, and suspending is 20ms shorter
for each enabled Atmel serial port.
Signed-off-by: Romain Izard <redacted>
---
drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
Unfortunately this patch was broken when I reported it from my branch to
the v4.13, as it does not build because of the missing declaration of
'atmel_port' in 'atmel_console_setup'.
I'll send a corrected version for v2.
--
Romain Izard
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2017-09-13 12:09:50
On 08/09/2017 at 17:35, Romain Izard wrote:
The contents of the System Clock Status Register (SCSR) needs to be
restored into the System Clock Enable Register (SCER).
As the bootloader will restore some clocks by itself, the issue can be
missed as only the USB controller, the LCD controller, the Image Sensor
controller and the programmable clocks will be impacted.
Fix the obvious typo in the suspend/resume code, as the IMR register
does not need to be saved twice.
Signed-off-by: Romain Izard <redacted>
Yes, it looks like a typo:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
I didn't experienced the issue with LCD nor USB though.
Regards,
@@ -86,7 +86,7 @@ static int pmc_suspend(void){inti;-regmap_read(pmcreg,AT91_PMC_IMR,&pmc_cache.scsr);+regmap_read(pmcreg,AT91_PMC_SCSR,&pmc_cache.scsr);regmap_read(pmcreg,AT91_PMC_PCSR,&pmc_cache.pcsr0);regmap_read(pmcreg,AT91_CKGR_UCKR,&pmc_cache.uckr);regmap_read(pmcreg,AT91_CKGR_MOR,&pmc_cache.mor);
@@ -129,7 +129,7 @@ static void pmc_resume(void)if(pmc_cache.pllar!=tmp)pr_warn("PLLAR was not configured properly by the firmware\n");-regmap_write(pmcreg,AT91_PMC_IMR,pmc_cache.scsr);+regmap_write(pmcreg,AT91_PMC_SCER,pmc_cache.scsr);regmap_write(pmcreg,AT91_PMC_PCER,pmc_cache.pcsr0);regmap_write(pmcreg,AT91_CKGR_UCKR,pmc_cache.uckr);regmap_write(pmcreg,AT91_CKGR_MOR,pmc_cache.mor);
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2017-09-13 12:15:12
On 08/09/2017 at 17:35, Romain Izard wrote:
quoted hunk
Wait for the syncronization of all clocks when resuming, not only the
UPLL clock. Do not use regmap_read_poll_timeout, as it will call BUG()
when interrupts are masked, which is the case in here.
Signed-off-by: Romain Izard <redacted>
---
drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...
quoted hunk
} pmc_cache;
void pmc_register_id(u8 id)
@@ -103,6 +104,8 @@ static int pmc_suspend(void) regmap_read(pmcreg, AT91_PMC_PCR, &pmc_cache.pcr[registered_ids[i]]); }+ for (i = 0; i < 3; i++)
And it might be a good practice to have this constant value in a #define.
We have "#define PROG_ID_MAX 7" defined in
drivers/clk/at91/clk-programmable.c.
Regards,
Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...
My opinion is that it will be time to change that when multiple SoCs
will need to save their registers.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...
My opinion is that it will be time to change that when multiple SoCs will
need to save their registers.
For the next version, I'll add a #define. But as this code requires a
device tree node with the compatible string "atmel,sama5d2-pmc", I believe
that we can ignore other chips for now.
--
Romain Izard
Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...
My opinion is that it will be time to change that when multiple SoCs will
need to save their registers.
For the next version, I'll add a #define. But as this code requires a
device tree node with the compatible string "atmel,sama5d2-pmc", I believe
that we can ignore other chips for now.
Fair enough, let's go for this.
Bye,
--
Nicolas Ferre
2017-09-13 14:15 GMT+02:00 Nicolas Ferre [off-list ref]:
On 08/09/2017 at 17:35, Romain Izard wrote:
quoted
Wait for the syncronization of all clocks when resuming, not only the
UPLL clock. Do not use regmap_read_poll_timeout, as it will call BUG()
when interrupts are masked, which is the case in here.
Signed-off-by: Romain Izard <redacted>
---
drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
Okay, but I would prefer to keep the timeout property in it. So we may
need to re-implement a timeout way-out here.
We need to have a reference clock to measure the timeout delay. If we use
the kernel's timekeeping, it relies on the clocks that we are configuring in
this code. Moreover, my experience with the mainline code is that when
something goes wrong, nothing will work. No oops or panic will be reported,
the device will just stop working.
In my case, I had obvious failures (it just stopped working unless I removed
USB wakeup or activated the console during suspend) but also very rare
failures, that occurred in the bootloader. Those issues were detected when
testing repeated suspend cycles for a night: the memory controller would
never enter the self-refresh mode during the resume sequence.
This led me to question the bootloader's code first, and I set up 4 boards
with the backup prototype code on v4.9 to verify that it was stable on
suspend. I've reached 1.5 million sleep cycles over 3 weeks without
failure, so this hinted towards the difference between the prototype and the
backup code provided for v4.12 (which contained the patch that got in
v4.13). Once I integrated this patch, I've run the v4.12 code for 2 weeks
without issue as well.
In the end, I don't want to touch this code if I do not have to, as checking
that it does not regress is really cumbersome.
--
Romain Izard
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2017-09-22 12:11:11
On 14/09/2017 at 18:15, Romain Izard wrote:
2017-09-13 14:15 GMT+02:00 Nicolas Ferre [off-list ref]:
quoted
On 08/09/2017 at 17:35, Romain Izard wrote:
quoted
Wait for the syncronization of all clocks when resuming, not only the
UPLL clock. Do not use regmap_read_poll_timeout, as it will call BUG()
when interrupts are masked, which is the case in here.
Signed-off-by: Romain Izard <redacted>
---
drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
Okay, but I would prefer to keep the timeout property in it. So we may
need to re-implement a timeout way-out here.
We need to have a reference clock to measure the timeout delay. If we use
the kernel's timekeeping, it relies on the clocks that we are configuring in
this code. Moreover, my experience with the mainline code is that when
something goes wrong, nothing will work. No oops or panic will be reported,
the device will just stop working.
In my case, I had obvious failures (it just stopped working unless I removed
USB wakeup or activated the console during suspend) but also very rare
failures, that occurred in the bootloader. Those issues were detected when
testing repeated suspend cycles for a night: the memory controller would
never enter the self-refresh mode during the resume sequence.
This led me to question the bootloader's code first, and I set up 4 boards
with the backup prototype code on v4.9 to verify that it was stable on
suspend. I've reached 1.5 million sleep cycles over 3 weeks without
failure, so this hinted towards the difference between the prototype and the
backup code provided for v4.12 (which contained the patch that got in
v4.13). Once I integrated this patch, I've run the v4.12 code for 2 weeks
without issue as well.
In the end, I don't want to touch this code if I do not have to, as checking
that it does not regress is really cumbersome.
The timeout was more for PLL like the one use for USB. I didn't want to
block only for USB PLL failure (which is kind of hypothetical, I admit).
Anyway, I understand your arguments and taking into account the
extensive tests that you've run, I agree with your approach. I'm adding
my Ack to the v2.
Thanks for having take the time to describe your debugging session: it's
valuable information for everybody.
Best regards,
--
Nicolas Ferre