[PATCH v1 00/10] Various patches for SAMA5D2 backup mode

[PATCH v1 00/10] Various patches for SAMA5D2 backup mode

[Romain Izard]

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

[PATCH v1 00/10] Various patches for SAMA5D2 backup mode

[Romain Izard]

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

[PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 775af473fe11..5c2b26de303e 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -107,10 +107,20 @@ static int pmc_suspend(void)
 	return 0;
 }
 
+static bool pmc_ready(unsigned int mask)
+{
+	unsigned int status;
+
+	regmap_read(pmcreg, AT91_PMC_SR, &status);
+
+	return ((status & mask) == mask) ? 1 : 0;
+}
+
 static void pmc_resume(void)
 {
-	int i, ret = 0;
+	int i;
 	u32 tmp;
+	u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
 
 	regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
 	if (pmc_cache.mckr != tmp)
@@ -134,13 +144,11 @@ static void pmc_resume(void)
 			     AT91_PMC_PCR_CMD);
 	}
 
-	if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
-		ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
-					       !(tmp & AT91_PMC_LOCKU),
-					       10, 5000);
-		if (ret)
-			pr_crit("USB PLL didn't lock when resuming\n");
-	}
+	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
+		mask |= AT91_PMC_LOCKU;
+
+	while (!pmc_ready(mask))
+		cpu_relax();
 }
 
 static struct syscore_ops pmc_syscore_ops = {
-- 
2.11.0

[PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 775af473fe11..5c2b26de303e 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -107,10 +107,20 @@ static int pmc_suspend(void)
 	return 0;
 }
 
+static bool pmc_ready(unsigned int mask)
+{
+	unsigned int status;
+
+	regmap_read(pmcreg, AT91_PMC_SR, &status);
+
+	return ((status & mask) == mask) ? 1 : 0;
+}
+
 static void pmc_resume(void)
 {
-	int i, ret = 0;
+	int i;
 	u32 tmp;
+	u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
 
 	regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
 	if (pmc_cache.mckr != tmp)
@@ -134,13 +144,11 @@ static void pmc_resume(void)
 			     AT91_PMC_PCR_CMD);
 	}
 
-	if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
-		ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
-					       !(tmp & AT91_PMC_LOCKU),
-					       10, 5000);
-		if (ret)
-			pr_crit("USB PLL didn't lock when resuming\n");
-	}
+	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
+		mask |= AT91_PMC_LOCKU;
+
+	while (!pmc_ready(mask))
+		cpu_relax();
 }
 
 static struct syscore_ops pmc_syscore_ops = {
-- 
2.11.0

[PATCH v1 02/10] clk: at91: pmc: Save SCSR during suspend

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 5c2b26de303e..07dc2861ad3f 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -86,7 +86,7 @@ static int pmc_suspend(void)
 {
 	int i;
 
-	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);
-- 
2.11.0

[PATCH v1 02/10] clk: at91: pmc: Save SCSR during suspend

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 5c2b26de303e..07dc2861ad3f 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -86,7 +86,7 @@ static int pmc_suspend(void)
 {
 	int i;
 
-	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);
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v1 02/10] clk: at91: pmc: Save SCSR during suspend

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 5c2b26de303e..07dc2861ad3f 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -86,7 +86,7 @@ static int pmc_suspend(void)
 {
 	int i;
 
-	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);
-- 
2.11.0

[PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Romain Izard]

From: Romain Izard <romain.izard@mobile-devices.fr>

Save and restore the System Clock and Programmable Clock register for
the backup use case.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 07dc2861ad3f..5421b03553ec 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -66,6 +66,7 @@ static struct
 	u32 pcr[PMC_MAX_IDS];
 	u32 audio_pll0;
 	u32 audio_pll1;
+	u32 pckr[3];
 } 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++)
+		regmap_read(pmcreg, AT91_PMC_PCKR(i), &pmc_cache.pckr[i]);
 
 	return 0;
 }
@@ -143,6 +146,8 @@ static void pmc_resume(void)
 			     pmc_cache.pcr[registered_ids[i]] |
 			     AT91_PMC_PCR_CMD);
 	}
+	for (i = 0; i < 3; i++)
+		regmap_write(pmcreg, AT91_PMC_PCKR(i), pmc_cache.pckr[i]);
 
 	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
 		mask |= AT91_PMC_LOCKU;
-- 
2.11.0

[PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Romain Izard]

From: Romain Izard <romain.izard@mobile-devices.fr>

Save and restore the System Clock and Programmable Clock register for
the backup use case.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/clk/at91/pmc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 07dc2861ad3f..5421b03553ec 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -66,6 +66,7 @@ static struct
 	u32 pcr[PMC_MAX_IDS];
 	u32 audio_pll0;
 	u32 audio_pll1;
+	u32 pckr[3];
 } 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++)
+		regmap_read(pmcreg, AT91_PMC_PCKR(i), &pmc_cache.pckr[i]);
 
 	return 0;
 }
@@ -143,6 +146,8 @@ static void pmc_resume(void)
 			     pmc_cache.pcr[registered_ids[i]] |
 			     AT91_PMC_PCR_CMD);
 	}
+	for (i = 0; i < 3; i++)
+		regmap_write(pmcreg, AT91_PMC_PCKR(i), pmc_cache.pckr[i]);
 
 	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
 		mask |= AT91_PMC_LOCKU;
-- 
2.11.0

[PATCH v1 04/10] mtd: nand: atmel: Avoid ECC errors when leaving backup mode

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/mtd/nand/atmel/pmecc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c
index 8c210a5776bc..8d1208f38025 100644
--- a/drivers/mtd/nand/atmel/pmecc.c
+++ b/drivers/mtd/nand/atmel/pmecc.c
@@ -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;
@@ -797,10 +800,6 @@ EXPORT_SYMBOL_GPL(atmel_pmecc_enable);
 
 void atmel_pmecc_disable(struct atmel_pmecc_user *user)
 {
-	struct atmel_pmecc *pmecc = user->pmecc;
-
-	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
-	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
 	mutex_unlock(&user->pmecc->lock);
 }
 EXPORT_SYMBOL_GPL(atmel_pmecc_disable);
@@ -856,10 +855,6 @@ static struct atmel_pmecc *atmel_pmecc_create(struct platform_device *pdev,
 	/* Disable all interrupts before registering the PMECC handler. */
 	writel(0xffffffff, pmecc->regs.base + ATMEL_PMECC_IDR);
 
-	/* Reset the ECC engine */
-	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
-	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
-
 	return pmecc;
 }
 
-- 
2.11.0

[PATCH v1 04/10] mtd: nand: atmel: Avoid ECC errors when leaving backup mode

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/mtd/nand/atmel/pmecc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c
index 8c210a5776bc..8d1208f38025 100644
--- a/drivers/mtd/nand/atmel/pmecc.c
+++ b/drivers/mtd/nand/atmel/pmecc.c
@@ -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;
@@ -797,10 +800,6 @@ EXPORT_SYMBOL_GPL(atmel_pmecc_enable);
 
 void atmel_pmecc_disable(struct atmel_pmecc_user *user)
 {
-	struct atmel_pmecc *pmecc = user->pmecc;
-
-	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
-	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
 	mutex_unlock(&user->pmecc->lock);
 }
 EXPORT_SYMBOL_GPL(atmel_pmecc_disable);
@@ -856,10 +855,6 @@ static struct atmel_pmecc *atmel_pmecc_create(struct platform_device *pdev,
 	/* Disable all interrupts before registering the PMECC handler. */
 	writel(0xffffffff, pmecc->regs.base + ATMEL_PMECC_IDR);
 
-	/* Reset the ECC engine */
-	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
-	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
-
 	return pmecc;
 }
 
-- 
2.11.0

[PATCH v1 05/10] mtd: nand: atmel: Report PMECC failures as errors

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/mtd/nand/atmel/pmecc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c
index 8d1208f38025..2a23f1ff945f 100644
--- a/drivers/mtd/nand/atmel/pmecc.c
+++ b/drivers/mtd/nand/atmel/pmecc.c
@@ -687,6 +687,8 @@ static int atmel_pmecc_err_location(struct atmel_pmecc_user *user)
 	 * Number of roots does not match the degree of smu
 	 * unable to correct error.
 	 */
+	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;
-- 
2.11.0

[PATCH v1 05/10] mtd: nand: atmel: Report PMECC failures as errors

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/mtd/nand/atmel/pmecc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c
index 8d1208f38025..2a23f1ff945f 100644
--- a/drivers/mtd/nand/atmel/pmecc.c
+++ b/drivers/mtd/nand/atmel/pmecc.c
@@ -687,6 +687,8 @@ static int atmel_pmecc_err_location(struct atmel_pmecc_user *user)
 	 * Number of roots does not match the degree of smu
 	 * unable to correct error.
 	 */
+	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;
-- 
2.11.0

[PATCH v1 06/10] ehci-atmel: Power down during suspend is normal

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/usb/host/ehci-atmel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index 7440722bfbf0..2a8b9bdc0e57 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -205,7 +205,8 @@ static int __maybe_unused ehci_atmel_drv_resume(struct device *dev)
 	struct atmel_ehci_priv *atmel_ehci = hcd_to_atmel_ehci_priv(hcd);
 
 	atmel_start_clock(atmel_ehci);
-	return ehci_resume(hcd, false);
+	ehci_resume(hcd, false);
+	return 0;
 }
 
 #ifdef CONFIG_OF
-- 
2.11.0

[PATCH v1 06/10] ehci-atmel: Power down during suspend is normal

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/usb/host/ehci-atmel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index 7440722bfbf0..2a8b9bdc0e57 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -205,7 +205,8 @@ static int __maybe_unused ehci_atmel_drv_resume(struct device *dev)
 	struct atmel_ehci_priv *atmel_ehci = hcd_to_atmel_ehci_priv(hcd);
 
 	atmel_start_clock(atmel_ehci);
-	return ehci_resume(hcd, false);
+	ehci_resume(hcd, false);
+	return 0;
 }
 
 #ifdef CONFIG_OF
-- 
2.11.0

[PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Romain Izard]

Support the backup mode for platform suspend, by restoring the hardware
registers on resume.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index e10dca3ed74b..f9718c863363 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -200,6 +200,7 @@ struct at91_adc_state {
 	u32				conversion_value;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
+	unsigned int			current_rate;
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
 	 * sysfs.
@@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
 	mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
 	at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
 
+	st->current_rate = freq;
+
 	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
 		freq, startup, prescal);
 }
@@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	    val > st->soc_info.max_sample_rate)
 		return -EINVAL;
 
+	mutex_lock(&st->lock);
 	at91_adc_setup_samp_freq(st, val);
+	mutex_unlock(&st->lock);
 
 	return 0;
 }
@@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
+{
+	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
+	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
+	/*
+	 * Transfer field must be set to 2 according to the datasheet and
+	 * allows different analog settings for each channel.
+	 */
+	at91_adc_writel(st, AT91_SAMA5D2_MR,
+			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
+
+	at91_adc_setup_samp_freq(st, freq);
+
+}
+
 static int at91_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 		goto vref_disable;
 	}
 
-	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
-	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
-	/*
-	 * Transfer field must be set to 2 according to the datasheet and
-	 * allows different analog settings for each channel.
-	 */
-	at91_adc_writel(st, AT91_SAMA5D2_MR,
-			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
-
-	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
+	at91_adc_init_hw(st, st->soc_info.min_sample_rate);
 
 	ret = clk_prepare_enable(st->per_clk);
 	if (ret)
@@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
 
+#ifdef CONFIG_PM_SLEEP
+static int at91_adc_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	clk_disable_unprepare(st->per_clk);
+
+	regulator_disable(st->vref);
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+static int at91_adc_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int err;
+
+	err = regulator_enable(st->reg);
+	if (err)
+		return err;
+
+	err = regulator_enable(st->vref);
+	if (err)
+		return err;
+
+	at91_adc_init_hw(st, st->current_rate);
+
+	err = clk_prepare_enable(st->per_clk);
+	return err;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
+
 static struct platform_driver at91_adc_driver = {
 	.probe = at91_adc_probe,
 	.remove = at91_adc_remove,
 	.driver = {
 		.name = "at91-sama5d2_adc",
 		.of_match_table = at91_adc_dt_match,
+		.pm = &at91_adc_pm_ops,
 	},
 };
 module_platform_driver(at91_adc_driver)
-- 
2.11.0

[PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Romain Izard]

Support the backup mode for platform suspend, by restoring the hardware
registers on resume.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index e10dca3ed74b..f9718c863363 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -200,6 +200,7 @@ struct at91_adc_state {
 	u32				conversion_value;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
+	unsigned int			current_rate;
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
 	 * sysfs.
@@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
 	mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
 	at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
 
+	st->current_rate = freq;
+
 	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
 		freq, startup, prescal);
 }
@@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	    val > st->soc_info.max_sample_rate)
 		return -EINVAL;
 
+	mutex_lock(&st->lock);
 	at91_adc_setup_samp_freq(st, val);
+	mutex_unlock(&st->lock);
 
 	return 0;
 }
@@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
+{
+	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
+	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
+	/*
+	 * Transfer field must be set to 2 according to the datasheet and
+	 * allows different analog settings for each channel.
+	 */
+	at91_adc_writel(st, AT91_SAMA5D2_MR,
+			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
+
+	at91_adc_setup_samp_freq(st, freq);
+
+}
+
 static int at91_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 		goto vref_disable;
 	}
 
-	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
-	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
-	/*
-	 * Transfer field must be set to 2 according to the datasheet and
-	 * allows different analog settings for each channel.
-	 */
-	at91_adc_writel(st, AT91_SAMA5D2_MR,
-			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
-
-	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
+	at91_adc_init_hw(st, st->soc_info.min_sample_rate);
 
 	ret = clk_prepare_enable(st->per_clk);
 	if (ret)
@@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
 
+#ifdef CONFIG_PM_SLEEP
+static int at91_adc_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	clk_disable_unprepare(st->per_clk);
+
+	regulator_disable(st->vref);
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+static int at91_adc_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int err;
+
+	err = regulator_enable(st->reg);
+	if (err)
+		return err;
+
+	err = regulator_enable(st->vref);
+	if (err)
+		return err;
+
+	at91_adc_init_hw(st, st->current_rate);
+
+	err = clk_prepare_enable(st->per_clk);
+	return err;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
+
 static struct platform_driver at91_adc_driver = {
 	.probe = at91_adc_probe,
 	.remove = at91_adc_remove,
 	.driver = {
 		.name = "at91-sama5d2_adc",
 		.of_match_table = at91_adc_dt_match,
+		.pm = &at91_adc_pm_ops,
 	},
 };
 module_platform_driver(at91_adc_driver)
-- 
2.11.0

[PATCH v1 08/10] pwm: atmel-tcb: Support backup mode

[Romain Izard]

Save and restore registers for the PWM on suspend and resume, which
makes hibernation and backup modes possible.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/pwm/pwm-atmel-tcb.c | 63 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 75db585a2a94..acd3ce8ecf3f 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -37,11 +37,20 @@ struct atmel_tcb_pwm_device {
 	unsigned period;		/* PWM period expressed in clk cycles */
 };
 
+struct atmel_tcb_channel {
+	u32 enabled;
+	u32 cmr;
+	u32 ra;
+	u32 rb;
+	u32 rc;
+};
+
 struct atmel_tcb_pwm_chip {
 	struct pwm_chip chip;
 	spinlock_t lock;
 	struct atmel_tc *tc;
 	struct atmel_tcb_pwm_device *pwms[NPWM];
+	struct atmel_tcb_channel bkup[NPWM / 2];
 };
 
 static inline struct atmel_tcb_pwm_chip *to_tcb_chip(struct pwm_chip *chip)
@@ -175,12 +184,15 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	 * Use software trigger to apply the new setting.
 	 * If both PWM devices in this group are disabled we stop the clock.
 	 */
-	if (!(cmr & (ATMEL_TC_ACPC | ATMEL_TC_BCPC)))
+	if (!(cmr & (ATMEL_TC_ACPC | ATMEL_TC_BCPC))) {
 		__raw_writel(ATMEL_TC_SWTRG | ATMEL_TC_CLKDIS,
 			     regs + ATMEL_TC_REG(group, CCR));
-	else
+		tcbpwmc->bkup[group].enabled = 1;
+	} else {
 		__raw_writel(ATMEL_TC_SWTRG, regs +
 			     ATMEL_TC_REG(group, CCR));
+		tcbpwmc->bkup[group].enabled = 0;
+	}
 
 	spin_unlock(&tcbpwmc->lock);
 }
@@ -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);
 	return 0;
 }
@@ -445,10 +458,56 @@ static const struct of_device_id atmel_tcb_pwm_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, atmel_tcb_pwm_dt_ids);
 
+#ifdef CONFIG_PM_SLEEP
+static int atmel_tcb_pwm_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct atmel_tcb_pwm_chip *tcbpwm = platform_get_drvdata(pdev);
+	void __iomem *base = tcbpwm->tc->regs;
+	int i;
+
+	for (i = 0; i < (NPWM / 2); i++) {
+		struct atmel_tcb_channel *chan = &tcbpwm->bkup[i];
+
+		chan->cmr = readl(base + ATMEL_TC_REG(i, CMR));
+		chan->ra = readl(base + ATMEL_TC_REG(i, RA));
+		chan->rb = readl(base + ATMEL_TC_REG(i, RB));
+		chan->rc = readl(base + ATMEL_TC_REG(i, RC));
+	}
+	return 0;
+}
+
+static int atmel_tcb_pwm_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct atmel_tcb_pwm_chip *tcbpwm = platform_get_drvdata(pdev);
+	void __iomem *base = tcbpwm->tc->regs;
+	int i;
+
+	for (i = 0; i < (NPWM / 2); i++) {
+		struct atmel_tcb_channel *chan = &tcbpwm->bkup[i];
+
+		writel(chan->cmr, base + ATMEL_TC_REG(i, CMR));
+		writel(chan->ra, base + ATMEL_TC_REG(i, RA));
+		writel(chan->rb, base + ATMEL_TC_REG(i, RB));
+		writel(chan->rc, base + ATMEL_TC_REG(i, RC));
+		if (chan->enabled) {
+			writel(ATMEL_TC_CLKEN | ATMEL_TC_SWTRG,
+				base + ATMEL_TC_REG(i, CCR));
+		}
+	}
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(atmel_tcb_pwm_pm_ops, atmel_tcb_pwm_suspend,
+			 atmel_tcb_pwm_resume);
+
 static struct platform_driver atmel_tcb_pwm_driver = {
 	.driver = {
 		.name = "atmel-tcb-pwm",
 		.of_match_table = atmel_tcb_pwm_dt_ids,
+		.pm = &atmel_tcb_pwm_pm_ops,
 	},
 	.probe = atmel_tcb_pwm_probe,
 	.remove = atmel_tcb_pwm_remove,
-- 
2.11.0

[PATCH v1 08/10] pwm: atmel-tcb: Support backup mode

[Romain Izard]

Save and restore registers for the PWM on suspend and resume, which
makes hibernation and backup modes possible.

Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pwm/pwm-atmel-tcb.c | 63 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 75db585a2a94..acd3ce8ecf3f 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -37,11 +37,20 @@ struct atmel_tcb_pwm_device {
 	unsigned period;		/* PWM period expressed in clk cycles */
 };
 
+struct atmel_tcb_channel {
+	u32 enabled;
+	u32 cmr;
+	u32 ra;
+	u32 rb;
+	u32 rc;
+};
+
 struct atmel_tcb_pwm_chip {
 	struct pwm_chip chip;
 	spinlock_t lock;
 	struct atmel_tc *tc;
 	struct atmel_tcb_pwm_device *pwms[NPWM];
+	struct atmel_tcb_channel bkup[NPWM / 2];
 };
 
 static inline struct atmel_tcb_pwm_chip *to_tcb_chip(struct pwm_chip *chip)
@@ -175,12 +184,15 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	 * Use software trigger to apply the new setting.
 	 * If both PWM devices in this group are disabled we stop the clock.
 	 */
-	if (!(cmr & (ATMEL_TC_ACPC | ATMEL_TC_BCPC)))
+	if (!(cmr & (ATMEL_TC_ACPC | ATMEL_TC_BCPC))) {
 		__raw_writel(ATMEL_TC_SWTRG | ATMEL_TC_CLKDIS,
 			     regs + ATMEL_TC_REG(group, CCR));
-	else
+		tcbpwmc->bkup[group].enabled = 1;
+	} else {
 		__raw_writel(ATMEL_TC_SWTRG, regs +
 			     ATMEL_TC_REG(group, CCR));
+		tcbpwmc->bkup[group].enabled = 0;
+	}
 
 	spin_unlock(&tcbpwmc->lock);
 }
@@ -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);
 	return 0;
 }
@@ -445,10 +458,56 @@ static const struct of_device_id atmel_tcb_pwm_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, atmel_tcb_pwm_dt_ids);
 
+#ifdef CONFIG_PM_SLEEP
+static int atmel_tcb_pwm_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct atmel_tcb_pwm_chip *tcbpwm = platform_get_drvdata(pdev);
+	void __iomem *base = tcbpwm->tc->regs;
+	int i;
+
+	for (i = 0; i < (NPWM / 2); i++) {
+		struct atmel_tcb_channel *chan = &tcbpwm->bkup[i];
+
+		chan->cmr = readl(base + ATMEL_TC_REG(i, CMR));
+		chan->ra = readl(base + ATMEL_TC_REG(i, RA));
+		chan->rb = readl(base + ATMEL_TC_REG(i, RB));
+		chan->rc = readl(base + ATMEL_TC_REG(i, RC));
+	}
+	return 0;
+}
+
+static int atmel_tcb_pwm_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct atmel_tcb_pwm_chip *tcbpwm = platform_get_drvdata(pdev);
+	void __iomem *base = tcbpwm->tc->regs;
+	int i;
+
+	for (i = 0; i < (NPWM / 2); i++) {
+		struct atmel_tcb_channel *chan = &tcbpwm->bkup[i];
+
+		writel(chan->cmr, base + ATMEL_TC_REG(i, CMR));
+		writel(chan->ra, base + ATMEL_TC_REG(i, RA));
+		writel(chan->rb, base + ATMEL_TC_REG(i, RB));
+		writel(chan->rc, base + ATMEL_TC_REG(i, RC));
+		if (chan->enabled) {
+			writel(ATMEL_TC_CLKEN | ATMEL_TC_SWTRG,
+				base + ATMEL_TC_REG(i, CCR));
+		}
+	}
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(atmel_tcb_pwm_pm_ops, atmel_tcb_pwm_suspend,
+			 atmel_tcb_pwm_resume);
+
 static struct platform_driver atmel_tcb_pwm_driver = {
 	.driver = {
 		.name = "atmel-tcb-pwm",
 		.of_match_table = atmel_tcb_pwm_dt_ids,
+		.pm = &atmel_tcb_pwm_pm_ops,
 	},
 	.probe = atmel_tcb_pwm_probe,
 	.remove = atmel_tcb_pwm_remove,
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 08/10] pwm: atmel-tcb: Support backup mode

[Romain Izard]

Save and restore registers for the PWM on suspend and resume, which
makes hibernation and backup modes possible.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/pwm/pwm-atmel-tcb.c | 63 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 75db585a2a94..acd3ce8ecf3f 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -37,11 +37,20 @@ struct atmel_tcb_pwm_device {
 	unsigned period;		/* PWM period expressed in clk cycles */
 };
 
+struct atmel_tcb_channel {
+	u32 enabled;
+	u32 cmr;
+	u32 ra;
+	u32 rb;
+	u32 rc;
+};
+
 struct atmel_tcb_pwm_chip {
 	struct pwm_chip chip;
 	spinlock_t lock;
 	struct atmel_tc *tc;
 	struct atmel_tcb_pwm_device *pwms[NPWM];
+	struct atmel_tcb_channel bkup[NPWM / 2];
 };
 
 static inline struct atmel_tcb_pwm_chip *to_tcb_chip(struct pwm_chip *chip)
@@ -175,12 +184,15 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	 * Use software trigger to apply the new setting.
 	 * If both PWM devices in this group are disabled we stop the clock.
 	 */
-	if (!(cmr & (ATMEL_TC_ACPC | ATMEL_TC_BCPC)))
+	if (!(cmr & (ATMEL_TC_ACPC | ATMEL_TC_BCPC))) {
 		__raw_writel(ATMEL_TC_SWTRG | ATMEL_TC_CLKDIS,
 			     regs + ATMEL_TC_REG(group, CCR));
-	else
+		tcbpwmc->bkup[group].enabled = 1;
+	} else {
 		__raw_writel(ATMEL_TC_SWTRG, regs +
 			     ATMEL_TC_REG(group, CCR));
+		tcbpwmc->bkup[group].enabled = 0;
+	}
 
 	spin_unlock(&tcbpwmc->lock);
 }
@@ -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);
 	return 0;
 }
@@ -445,10 +458,56 @@ static const struct of_device_id atmel_tcb_pwm_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, atmel_tcb_pwm_dt_ids);
 
+#ifdef CONFIG_PM_SLEEP
+static int atmel_tcb_pwm_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct atmel_tcb_pwm_chip *tcbpwm = platform_get_drvdata(pdev);
+	void __iomem *base = tcbpwm->tc->regs;
+	int i;
+
+	for (i = 0; i < (NPWM / 2); i++) {
+		struct atmel_tcb_channel *chan = &tcbpwm->bkup[i];
+
+		chan->cmr = readl(base + ATMEL_TC_REG(i, CMR));
+		chan->ra = readl(base + ATMEL_TC_REG(i, RA));
+		chan->rb = readl(base + ATMEL_TC_REG(i, RB));
+		chan->rc = readl(base + ATMEL_TC_REG(i, RC));
+	}
+	return 0;
+}
+
+static int atmel_tcb_pwm_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct atmel_tcb_pwm_chip *tcbpwm = platform_get_drvdata(pdev);
+	void __iomem *base = tcbpwm->tc->regs;
+	int i;
+
+	for (i = 0; i < (NPWM / 2); i++) {
+		struct atmel_tcb_channel *chan = &tcbpwm->bkup[i];
+
+		writel(chan->cmr, base + ATMEL_TC_REG(i, CMR));
+		writel(chan->ra, base + ATMEL_TC_REG(i, RA));
+		writel(chan->rb, base + ATMEL_TC_REG(i, RB));
+		writel(chan->rc, base + ATMEL_TC_REG(i, RC));
+		if (chan->enabled) {
+			writel(ATMEL_TC_CLKEN | ATMEL_TC_SWTRG,
+				base + ATMEL_TC_REG(i, CCR));
+		}
+	}
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(atmel_tcb_pwm_pm_ops, atmel_tcb_pwm_suspend,
+			 atmel_tcb_pwm_resume);
+
 static struct platform_driver atmel_tcb_pwm_driver = {
 	.driver = {
 		.name = "atmel-tcb-pwm",
 		.of_match_table = atmel_tcb_pwm_dt_ids,
+		.pm = &atmel_tcb_pwm_pm_ops,
 	},
 	.probe = atmel_tcb_pwm_probe,
 	.remove = atmel_tcb_pwm_remove,
-- 
2.11.0

[PATCH v1 09/10] atmel_flexcom: Support backup mode

[Romain Izard]

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>
---
 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;
 	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,
 	},
 };
-- 
2.11.0

[PATCH v1 09/10] atmel_flexcom: Support backup mode

[Romain Izard]

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>
---
 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;
 	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,
 	},
 };
-- 
2.11.0

[PATCH v1 10/10] tty/serial: atmel: Prevent a warning on suspend

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7551cab438ff..195c0d1b594e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -171,6 +171,7 @@ struct atmel_uart_port {
 	bool			has_hw_timer;
 	struct timer_list	uart_timer;
 
+	bool			tx_stopped;
 	bool			suspended;
 	unsigned int		pending;
 	unsigned int		pending_status;
@@ -380,6 +381,10 @@ static int atmel_config_rs485(struct uart_port *port,
  */
 static u_int atmel_tx_empty(struct uart_port *port)
 {
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
+	if (atmel_port->tx_stopped)
+		return TIOCSER_TEMT;
 	return (atmel_uart_readl(port, ATMEL_US_CSR) & ATMEL_US_TXEMPTY) ?
 		TIOCSER_TEMT :
 		0;
@@ -485,6 +490,7 @@ static void atmel_stop_tx(struct uart_port *port)
 	 * is fully transmitted.
 	 */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
+	atmel_port->tx_stopped = true;
 
 	/* Disable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
@@ -492,6 +498,7 @@ static void atmel_stop_tx(struct uart_port *port)
 	if ((port->rs485.flags & SER_RS485_ENABLED) &&
 	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
 		atmel_start_rx(port);
+
 }
 
 /*
@@ -521,6 +528,7 @@ static void atmel_start_tx(struct uart_port *port)
 
 	/* re-enable the transmitter */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+	atmel_port->tx_stopped = false;
 }
 
 /*
@@ -1866,6 +1874,7 @@ static int atmel_startup(struct uart_port *port)
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	/* enable xmit & rcvr */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	setup_timer(&atmel_port->uart_timer,
 			atmel_uart_timer_callback,
@@ -2122,6 +2131,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* disable receiver and transmitter */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
+	atmel_port->tx_stopped = true;
 
 	/* mode */
 	if (port->rs485.flags & SER_RS485_ENABLED) {
@@ -2207,6 +2217,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 	atmel_uart_writel(port, ATMEL_US_BRGR, quot);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	/* restore interrupts */
 	atmel_uart_writel(port, ATMEL_US_IER, imr);
@@ -2450,6 +2461,7 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
 
 	/* Make sure that tx path is actually able to send characters */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+	atmel_port->tx_stopped = false;
 
 	uart_console_write(port, s, count, atmel_console_putchar);
 
@@ -2528,6 +2540,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
 	atmel_uart_writel(port, ATMEL_US_IDR, -1);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
-- 
2.11.0

[PATCH v1 10/10] tty/serial: atmel: Prevent a warning on suspend

[Romain Izard]

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 <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7551cab438ff..195c0d1b594e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -171,6 +171,7 @@ struct atmel_uart_port {
 	bool			has_hw_timer;
 	struct timer_list	uart_timer;
 
+	bool			tx_stopped;
 	bool			suspended;
 	unsigned int		pending;
 	unsigned int		pending_status;
@@ -380,6 +381,10 @@ static int atmel_config_rs485(struct uart_port *port,
  */
 static u_int atmel_tx_empty(struct uart_port *port)
 {
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
+	if (atmel_port->tx_stopped)
+		return TIOCSER_TEMT;
 	return (atmel_uart_readl(port, ATMEL_US_CSR) & ATMEL_US_TXEMPTY) ?
 		TIOCSER_TEMT :
 		0;
@@ -485,6 +490,7 @@ static void atmel_stop_tx(struct uart_port *port)
 	 * is fully transmitted.
 	 */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
+	atmel_port->tx_stopped = true;
 
 	/* Disable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
@@ -492,6 +498,7 @@ static void atmel_stop_tx(struct uart_port *port)
 	if ((port->rs485.flags & SER_RS485_ENABLED) &&
 	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
 		atmel_start_rx(port);
+
 }
 
 /*
@@ -521,6 +528,7 @@ static void atmel_start_tx(struct uart_port *port)
 
 	/* re-enable the transmitter */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+	atmel_port->tx_stopped = false;
 }
 
 /*
@@ -1866,6 +1874,7 @@ static int atmel_startup(struct uart_port *port)
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	/* enable xmit & rcvr */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	setup_timer(&atmel_port->uart_timer,
 			atmel_uart_timer_callback,
@@ -2122,6 +2131,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* disable receiver and transmitter */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
+	atmel_port->tx_stopped = true;
 
 	/* mode */
 	if (port->rs485.flags & SER_RS485_ENABLED) {
@@ -2207,6 +2217,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 	atmel_uart_writel(port, ATMEL_US_BRGR, quot);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	/* restore interrupts */
 	atmel_uart_writel(port, ATMEL_US_IER, imr);
@@ -2450,6 +2461,7 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
 
 	/* Make sure that tx path is actually able to send characters */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+	atmel_port->tx_stopped = false;
 
 	uart_console_write(port, s, count, atmel_console_putchar);
 
@@ -2528,6 +2540,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
 	atmel_uart_writel(port, ATMEL_US_IDR, -1);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
-- 
2.11.0

[PATCH v1 10/10] tty/serial: atmel: Prevent a warning on suspend

[Romain Izard]

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 <romain.izard.pro@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7551cab438ff..195c0d1b594e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -171,6 +171,7 @@ struct atmel_uart_port {
 	bool			has_hw_timer;
 	struct timer_list	uart_timer;
 
+	bool			tx_stopped;
 	bool			suspended;
 	unsigned int		pending;
 	unsigned int		pending_status;
@@ -380,6 +381,10 @@ static int atmel_config_rs485(struct uart_port *port,
  */
 static u_int atmel_tx_empty(struct uart_port *port)
 {
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
+	if (atmel_port->tx_stopped)
+		return TIOCSER_TEMT;
 	return (atmel_uart_readl(port, ATMEL_US_CSR) & ATMEL_US_TXEMPTY) ?
 		TIOCSER_TEMT :
 		0;
@@ -485,6 +490,7 @@ static void atmel_stop_tx(struct uart_port *port)
 	 * is fully transmitted.
 	 */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
+	atmel_port->tx_stopped = true;
 
 	/* Disable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
@@ -492,6 +498,7 @@ static void atmel_stop_tx(struct uart_port *port)
 	if ((port->rs485.flags & SER_RS485_ENABLED) &&
 	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
 		atmel_start_rx(port);
+
 }
 
 /*
@@ -521,6 +528,7 @@ static void atmel_start_tx(struct uart_port *port)
 
 	/* re-enable the transmitter */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+	atmel_port->tx_stopped = false;
 }
 
 /*
@@ -1866,6 +1874,7 @@ static int atmel_startup(struct uart_port *port)
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	/* enable xmit & rcvr */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	setup_timer(&atmel_port->uart_timer,
 			atmel_uart_timer_callback,
@@ -2122,6 +2131,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* disable receiver and transmitter */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
+	atmel_port->tx_stopped = true;
 
 	/* mode */
 	if (port->rs485.flags & SER_RS485_ENABLED) {
@@ -2207,6 +2217,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 	atmel_uart_writel(port, ATMEL_US_BRGR, quot);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	/* restore interrupts */
 	atmel_uart_writel(port, ATMEL_US_IER, imr);
@@ -2450,6 +2461,7 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
 
 	/* Make sure that tx path is actually able to send characters */
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+	atmel_port->tx_stopped = false;
 
 	uart_console_write(port, s, count, atmel_console_putchar);
 
@@ -2528,6 +2540,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
 	atmel_uart_writel(port, ATMEL_US_IDR, -1);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
 	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
+	atmel_port->tx_stopped = false;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
-- 
2.11.0

Re: [PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Nicolas Ferre]

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 <romain.izard.pro@gmail.com>

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,

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
>  1 file changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index e10dca3ed74b..f9718c863363 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -200,6 +200,7 @@ struct at91_adc_state {
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	unsigned int			current_rate;
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
>  	 * sysfs.
> @@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>  	mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
>  	at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
>  
> +	st->current_rate = freq;
> +
>  	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>  		freq, startup, prescal);
>  }
> @@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	    val > st->soc_info.max_sample_rate)
>  		return -EINVAL;
>  
> +	mutex_lock(&st->lock);
>  	at91_adc_setup_samp_freq(st, val);
> +	mutex_unlock(&st->lock);
>  
>  	return 0;
>  }
> @@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> +	/*
> +	 * Transfer field must be set to 2 according to the datasheet and
> +	 * allows different analog settings for each channel.
> +	 */
> +	at91_adc_writel(st, AT91_SAMA5D2_MR,
> +			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
> +
> +	at91_adc_setup_samp_freq(st, freq);
> +
> +}
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto vref_disable;
>  	}
>  
> -	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> -	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> -	/*
> -	 * Transfer field must be set to 2 according to the datasheet and
> -	 * allows different analog settings for each channel.
> -	 */
> -	at91_adc_writel(st, AT91_SAMA5D2_MR,
> -			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
> -
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_init_hw(st, st->soc_info.min_sample_rate);
>  
>  	ret = clk_prepare_enable(st->per_clk);
>  	if (ret)
> @@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int at91_adc_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	clk_disable_unprepare(st->per_clk);
> +
> +	regulator_disable(st->vref);
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int err;
> +
> +	err = regulator_enable(st->reg);
> +	if (err)
> +		return err;
> +
> +	err = regulator_enable(st->vref);
> +	if (err)
> +		return err;
> +
> +	at91_adc_init_hw(st, st->current_rate);
> +
> +	err = clk_prepare_enable(st->per_clk);
> +	return err;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
> +
>  static struct platform_driver at91_adc_driver = {
>  	.probe = at91_adc_probe,
>  	.remove = at91_adc_remove,
>  	.driver = {
>  		.name = "at91-sama5d2_adc",
>  		.of_match_table = at91_adc_dt_match,
> +		.pm = &at91_adc_pm_ops,
>  	},
>  };
>  module_platform_driver(at91_adc_driver)
> 


-- 
Nicolas Ferre

Re: [PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Nicolas Ferre]

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 <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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,

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
>  1 file changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index e10dca3ed74b..f9718c863363 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -200,6 +200,7 @@ struct at91_adc_state {
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	unsigned int			current_rate;
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
>  	 * sysfs.
> @@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>  	mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
>  	at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
>  
> +	st->current_rate = freq;
> +
>  	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>  		freq, startup, prescal);
>  }
> @@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	    val > st->soc_info.max_sample_rate)
>  		return -EINVAL;
>  
> +	mutex_lock(&st->lock);
>  	at91_adc_setup_samp_freq(st, val);
> +	mutex_unlock(&st->lock);
>  
>  	return 0;
>  }
> @@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> +	/*
> +	 * Transfer field must be set to 2 according to the datasheet and
> +	 * allows different analog settings for each channel.
> +	 */
> +	at91_adc_writel(st, AT91_SAMA5D2_MR,
> +			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
> +
> +	at91_adc_setup_samp_freq(st, freq);
> +
> +}
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto vref_disable;
>  	}
>  
> -	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> -	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> -	/*
> -	 * Transfer field must be set to 2 according to the datasheet and
> -	 * allows different analog settings for each channel.
> -	 */
> -	at91_adc_writel(st, AT91_SAMA5D2_MR,
> -			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
> -
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_init_hw(st, st->soc_info.min_sample_rate);
>  
>  	ret = clk_prepare_enable(st->per_clk);
>  	if (ret)
> @@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int at91_adc_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	clk_disable_unprepare(st->per_clk);
> +
> +	regulator_disable(st->vref);
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int err;
> +
> +	err = regulator_enable(st->reg);
> +	if (err)
> +		return err;
> +
> +	err = regulator_enable(st->vref);
> +	if (err)
> +		return err;
> +
> +	at91_adc_init_hw(st, st->current_rate);
> +
> +	err = clk_prepare_enable(st->per_clk);
> +	return err;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
> +
>  static struct platform_driver at91_adc_driver = {
>  	.probe = at91_adc_probe,
>  	.remove = at91_adc_remove,
>  	.driver = {
>  		.name = "at91-sama5d2_adc",
>  		.of_match_table = at91_adc_dt_match,
> +		.pm = &at91_adc_pm_ops,
>  	},
>  };
>  module_platform_driver(at91_adc_driver)
> 


-- 
Nicolas Ferre

[PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Nicolas Ferre]

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 <romain.izard.pro@gmail.com>

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,

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
>  1 file changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index e10dca3ed74b..f9718c863363 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -200,6 +200,7 @@ struct at91_adc_state {
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	unsigned int			current_rate;
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
>  	 * sysfs.
> @@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>  	mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
>  	at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
>  
> +	st->current_rate = freq;
> +
>  	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>  		freq, startup, prescal);
>  }
> @@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	    val > st->soc_info.max_sample_rate)
>  		return -EINVAL;
>  
> +	mutex_lock(&st->lock);
>  	at91_adc_setup_samp_freq(st, val);
> +	mutex_unlock(&st->lock);
>  
>  	return 0;
>  }
> @@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> +	/*
> +	 * Transfer field must be set to 2 according to the datasheet and
> +	 * allows different analog settings for each channel.
> +	 */
> +	at91_adc_writel(st, AT91_SAMA5D2_MR,
> +			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
> +
> +	at91_adc_setup_samp_freq(st, freq);
> +
> +}
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto vref_disable;
>  	}
>  
> -	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> -	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> -	/*
> -	 * Transfer field must be set to 2 according to the datasheet and
> -	 * allows different analog settings for each channel.
> -	 */
> -	at91_adc_writel(st, AT91_SAMA5D2_MR,
> -			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
> -
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_init_hw(st, st->soc_info.min_sample_rate);
>  
>  	ret = clk_prepare_enable(st->per_clk);
>  	if (ret)
> @@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int at91_adc_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	clk_disable_unprepare(st->per_clk);
> +
> +	regulator_disable(st->vref);
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int err;
> +
> +	err = regulator_enable(st->reg);
> +	if (err)
> +		return err;
> +
> +	err = regulator_enable(st->vref);
> +	if (err)
> +		return err;
> +
> +	at91_adc_init_hw(st, st->current_rate);
> +
> +	err = clk_prepare_enable(st->per_clk);
> +	return err;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
> +
>  static struct platform_driver at91_adc_driver = {
>  	.probe = at91_adc_probe,
>  	.remove = at91_adc_remove,
>  	.driver = {
>  		.name = "at91-sama5d2_adc",
>  		.of_match_table = at91_adc_dt_match,
> +		.pm = &at91_adc_pm_ops,
>  	},
>  };
>  module_platform_driver(at91_adc_driver)
> 


-- 
Nicolas Ferre

Re: [PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Romain Izard]

2017-09-08 18:03 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> 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 <romain.izard.pro@gmail.com>
>
> 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,
>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index e10dca3ed74b..f9718c863363 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -200,6 +200,7 @@ struct at91_adc_state {
>>       u32                             conversion_value;
>>       struct at91_adc_soc_info        soc_info;
>>       wait_queue_head_t               wq_data_available;
>> +     unsigned int                    current_rate;
>>       /*
>>        * lock to prevent concurrent 'single conversion' requests through
>>        * sysfs.
>> @@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>>       mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
>>       at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
>>
>> +     st->current_rate = freq;
>> +
>>       dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>>               freq, startup, prescal);
>>  }
>> @@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>           val > st->soc_info.max_sample_rate)
>>               return -EINVAL;
>>
>> +     mutex_lock(&st->lock);
>>       at91_adc_setup_samp_freq(st, val);
>> +     mutex_unlock(&st->lock);
>>
>>       return 0;
>>  }
>> @@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
>>       .driver_module = THIS_MODULE,
>>  };
>>
>> +static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
>> +{
>> +     at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>> +     at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>> +     /*
>> +      * Transfer field must be set to 2 according to the datasheet and
>> +      * allows different analog settings for each channel.
>> +      */
>> +     at91_adc_writel(st, AT91_SAMA5D2_MR,
>> +                     AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>> +
>> +     at91_adc_setup_samp_freq(st, freq);
>> +
>> +}
>> +
>>  static int at91_adc_probe(struct platform_device *pdev)
>>  {
>>       struct iio_dev *indio_dev;
>> @@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>>               goto vref_disable;
>>       }
>>
>> -     at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>> -     at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>> -     /*
>> -      * Transfer field must be set to 2 according to the datasheet and
>> -      * allows different analog settings for each channel.
>> -      */
>> -     at91_adc_writel(st, AT91_SAMA5D2_MR,
>> -                     AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>> -
>> -     at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>> +     at91_adc_init_hw(st, st->soc_info.min_sample_rate);
>>
>>       ret = clk_prepare_enable(st->per_clk);
>>       if (ret)
>> @@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int at91_adc_suspend(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +     clk_disable_unprepare(st->per_clk);
>> +
>> +     regulator_disable(st->vref);
>> +     regulator_disable(st->reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static int at91_adc_resume(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int err;
>> +
>> +     err = regulator_enable(st->reg);
>> +     if (err)
>> +             return err;
>> +
>> +     err = regulator_enable(st->vref);
>> +     if (err)
>> +             return err;
>> +
>> +     at91_adc_init_hw(st, st->current_rate);
>> +
>> +     err = clk_prepare_enable(st->per_clk);
>> +     return err;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
>> +
>>  static struct platform_driver at91_adc_driver = {
>>       .probe = at91_adc_probe,
>>       .remove = at91_adc_remove,
>>       .driver = {
>>               .name = "at91-sama5d2_adc",
>>               .of_match_table = at91_adc_dt_match,
>> +             .pm = &at91_adc_pm_ops,
>>       },
>>  };
>>  module_platform_driver(at91_adc_driver)
>>

Please ignore this patch. The existing merged patch is better.

Best regards,
-- 
Romain Izard

Re: [PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Romain Izard]

2017-09-08 18:03 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> 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 <romain.izard.pro@gmail.com>
>
> 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,
>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index e10dca3ed74b..f9718c863363 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -200,6 +200,7 @@ struct at91_adc_state {
>>       u32                             conversion_value;
>>       struct at91_adc_soc_info        soc_info;
>>       wait_queue_head_t               wq_data_available;
>> +     unsigned int                    current_rate;
>>       /*
>>        * lock to prevent concurrent 'single conversion' requests through
>>        * sysfs.
>> @@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>>       mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
>>       at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
>>
>> +     st->current_rate = freq;
>> +
>>       dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>>               freq, startup, prescal);
>>  }
>> @@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>           val > st->soc_info.max_sample_rate)
>>               return -EINVAL;
>>
>> +     mutex_lock(&st->lock);
>>       at91_adc_setup_samp_freq(st, val);
>> +     mutex_unlock(&st->lock);
>>
>>       return 0;
>>  }
>> @@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
>>       .driver_module = THIS_MODULE,
>>  };
>>
>> +static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
>> +{
>> +     at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>> +     at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>> +     /*
>> +      * Transfer field must be set to 2 according to the datasheet and
>> +      * allows different analog settings for each channel.
>> +      */
>> +     at91_adc_writel(st, AT91_SAMA5D2_MR,
>> +                     AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>> +
>> +     at91_adc_setup_samp_freq(st, freq);
>> +
>> +}
>> +
>>  static int at91_adc_probe(struct platform_device *pdev)
>>  {
>>       struct iio_dev *indio_dev;
>> @@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>>               goto vref_disable;
>>       }
>>
>> -     at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>> -     at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>> -     /*
>> -      * Transfer field must be set to 2 according to the datasheet and
>> -      * allows different analog settings for each channel.
>> -      */
>> -     at91_adc_writel(st, AT91_SAMA5D2_MR,
>> -                     AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>> -
>> -     at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>> +     at91_adc_init_hw(st, st->soc_info.min_sample_rate);
>>
>>       ret = clk_prepare_enable(st->per_clk);
>>       if (ret)
>> @@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int at91_adc_suspend(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +     clk_disable_unprepare(st->per_clk);
>> +
>> +     regulator_disable(st->vref);
>> +     regulator_disable(st->reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static int at91_adc_resume(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int err;
>> +
>> +     err = regulator_enable(st->reg);
>> +     if (err)
>> +             return err;
>> +
>> +     err = regulator_enable(st->vref);
>> +     if (err)
>> +             return err;
>> +
>> +     at91_adc_init_hw(st, st->current_rate);
>> +
>> +     err = clk_prepare_enable(st->per_clk);
>> +     return err;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
>> +
>>  static struct platform_driver at91_adc_driver = {
>>       .probe = at91_adc_probe,
>>       .remove = at91_adc_remove,
>>       .driver = {
>>               .name = "at91-sama5d2_adc",
>>               .of_match_table = at91_adc_dt_match,
>> +             .pm = &at91_adc_pm_ops,
>>       },
>>  };
>>  module_platform_driver(at91_adc_driver)
>>

Please ignore this patch. The existing merged patch is better.

Best regards,
-- 
Romain Izard

[PATCH v1 07/10] iio:adc:at91-sama5d2: Support backup mode

[Romain Izard]

2017-09-08 18:03 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> 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 <romain.izard.pro@gmail.com>
>
> 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,
>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 71 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index e10dca3ed74b..f9718c863363 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -200,6 +200,7 @@ struct at91_adc_state {
>>       u32                             conversion_value;
>>       struct at91_adc_soc_info        soc_info;
>>       wait_queue_head_t               wq_data_available;
>> +     unsigned int                    current_rate;
>>       /*
>>        * lock to prevent concurrent 'single conversion' requests through
>>        * sysfs.
>> @@ -269,6 +270,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>>       mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
>>       at91_adc_writel(st, AT91_SAMA5D2_MR, mr);
>>
>> +     st->current_rate = freq;
>> +
>>       dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>>               freq, startup, prescal);
>>  }
>> @@ -375,7 +378,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>           val > st->soc_info.max_sample_rate)
>>               return -EINVAL;
>>
>> +     mutex_lock(&st->lock);
>>       at91_adc_setup_samp_freq(st, val);
>> +     mutex_unlock(&st->lock);
>>
>>       return 0;
>>  }
>> @@ -386,6 +391,21 @@ static const struct iio_info at91_adc_info = {
>>       .driver_module = THIS_MODULE,
>>  };
>>
>> +static void at91_adc_init_hw(struct at91_adc_state *st, unsigned int freq)
>> +{
>> +     at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>> +     at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>> +     /*
>> +      * Transfer field must be set to 2 according to the datasheet and
>> +      * allows different analog settings for each channel.
>> +      */
>> +     at91_adc_writel(st, AT91_SAMA5D2_MR,
>> +                     AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>> +
>> +     at91_adc_setup_samp_freq(st, freq);
>> +
>> +}
>> +
>>  static int at91_adc_probe(struct platform_device *pdev)
>>  {
>>       struct iio_dev *indio_dev;
>> @@ -482,16 +502,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>>               goto vref_disable;
>>       }
>>
>> -     at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>> -     at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>> -     /*
>> -      * Transfer field must be set to 2 according to the datasheet and
>> -      * allows different analog settings for each channel.
>> -      */
>> -     at91_adc_writel(st, AT91_SAMA5D2_MR,
>> -                     AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>> -
>> -     at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>> +     at91_adc_init_hw(st, st->soc_info.min_sample_rate);
>>
>>       ret = clk_prepare_enable(st->per_clk);
>>       if (ret)
>> @@ -541,12 +552,52 @@ static const struct of_device_id at91_adc_dt_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int at91_adc_suspend(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +     clk_disable_unprepare(st->per_clk);
>> +
>> +     regulator_disable(st->vref);
>> +     regulator_disable(st->reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static int at91_adc_resume(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int err;
>> +
>> +     err = regulator_enable(st->reg);
>> +     if (err)
>> +             return err;
>> +
>> +     err = regulator_enable(st->vref);
>> +     if (err)
>> +             return err;
>> +
>> +     at91_adc_init_hw(st, st->current_rate);
>> +
>> +     err = clk_prepare_enable(st->per_clk);
>> +     return err;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
>> +
>>  static struct platform_driver at91_adc_driver = {
>>       .probe = at91_adc_probe,
>>       .remove = at91_adc_remove,
>>       .driver = {
>>               .name = "at91-sama5d2_adc",
>>               .of_match_table = at91_adc_dt_match,
>> +             .pm = &at91_adc_pm_ops,
>>       },
>>  };
>>  module_platform_driver(at91_adc_driver)
>>

Please ignore this patch. The existing merged patch is better.

Best regards,
-- 
Romain Izard

Re: [PATCH v1 10/10] tty/serial: atmel: Prevent a warning on suspend

[Romain Izard]

2017-09-08 17:36 GMT+02:00 Romain Izard <romain.izard.pro@gmail.com>:
> 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 <romain.izard.pro@gmail.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7551cab438ff..195c0d1b594e 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -171,6 +171,7 @@ struct atmel_uart_port {
>         bool                    has_hw_timer;
>         struct timer_list       uart_timer;
>
> +       bool                    tx_stopped;
>         bool                    suspended;
>         unsigned int            pending;
>         unsigned int            pending_status;
> @@ -380,6 +381,10 @@ static int atmel_config_rs485(struct uart_port *port,
>   */
>  static u_int atmel_tx_empty(struct uart_port *port)
>  {
> +       struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> +
> +       if (atmel_port->tx_stopped)
> +               return TIOCSER_TEMT;
>         return (atmel_uart_readl(port, ATMEL_US_CSR) & ATMEL_US_TXEMPTY) ?
>                 TIOCSER_TEMT :
>                 0;
> @@ -485,6 +490,7 @@ static void atmel_stop_tx(struct uart_port *port)
>          * is fully transmitted.
>          */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
> +       atmel_port->tx_stopped = true;
>
>         /* Disable interrupts */
>         atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> @@ -492,6 +498,7 @@ static void atmel_stop_tx(struct uart_port *port)
>         if ((port->rs485.flags & SER_RS485_ENABLED) &&
>             !(port->rs485.flags & SER_RS485_RX_DURING_TX))
>                 atmel_start_rx(port);
> +
>  }
>
>  /*
> @@ -521,6 +528,7 @@ static void atmel_start_tx(struct uart_port *port)
>
>         /* re-enable the transmitter */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +       atmel_port->tx_stopped = false;
>  }
>
>  /*
> @@ -1866,6 +1874,7 @@ static int atmel_startup(struct uart_port *port)
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         /* enable xmit & rcvr */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         setup_timer(&atmel_port->uart_timer,
>                         atmel_uart_timer_callback,
> @@ -2122,6 +2131,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>
>         /* disable receiver and transmitter */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
> +       atmel_port->tx_stopped = true;
>
>         /* mode */
>         if (port->rs485.flags & SER_RS485_ENABLED) {
> @@ -2207,6 +2217,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>         atmel_uart_writel(port, ATMEL_US_BRGR, quot);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         /* restore interrupts */
>         atmel_uart_writel(port, ATMEL_US_IER, imr);
> @@ -2450,6 +2461,7 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
>
>         /* Make sure that tx path is actually able to send characters */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +       atmel_port->tx_stopped = false;
>
>         uart_console_write(port, s, count, atmel_console_putchar);
>
> @@ -2528,6 +2540,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
>         atmel_uart_writel(port, ATMEL_US_IDR, -1);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         if (options)
>                 uart_parse_options(options, &baud, &parity, &bits, &flow);
> --
> 2.11.0
>

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

Re: [PATCH v1 10/10] tty/serial: atmel: Prevent a warning on suspend

[Romain Izard]

2017-09-08 17:36 GMT+02:00 Romain Izard <romain.izard.pro@gmail.com>:
> 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 <romain.izard.pro@gmail.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7551cab438ff..195c0d1b594e 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -171,6 +171,7 @@ struct atmel_uart_port {
>         bool                    has_hw_timer;
>         struct timer_list       uart_timer;
>
> +       bool                    tx_stopped;
>         bool                    suspended;
>         unsigned int            pending;
>         unsigned int            pending_status;
> @@ -380,6 +381,10 @@ static int atmel_config_rs485(struct uart_port *port,
>   */
>  static u_int atmel_tx_empty(struct uart_port *port)
>  {
> +       struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> +
> +       if (atmel_port->tx_stopped)
> +               return TIOCSER_TEMT;
>         return (atmel_uart_readl(port, ATMEL_US_CSR) & ATMEL_US_TXEMPTY) ?
>                 TIOCSER_TEMT :
>                 0;
> @@ -485,6 +490,7 @@ static void atmel_stop_tx(struct uart_port *port)
>          * is fully transmitted.
>          */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
> +       atmel_port->tx_stopped = true;
>
>         /* Disable interrupts */
>         atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> @@ -492,6 +498,7 @@ static void atmel_stop_tx(struct uart_port *port)
>         if ((port->rs485.flags & SER_RS485_ENABLED) &&
>             !(port->rs485.flags & SER_RS485_RX_DURING_TX))
>                 atmel_start_rx(port);
> +
>  }
>
>  /*
> @@ -521,6 +528,7 @@ static void atmel_start_tx(struct uart_port *port)
>
>         /* re-enable the transmitter */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +       atmel_port->tx_stopped = false;
>  }
>
>  /*
> @@ -1866,6 +1874,7 @@ static int atmel_startup(struct uart_port *port)
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         /* enable xmit & rcvr */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         setup_timer(&atmel_port->uart_timer,
>                         atmel_uart_timer_callback,
> @@ -2122,6 +2131,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>
>         /* disable receiver and transmitter */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
> +       atmel_port->tx_stopped = true;
>
>         /* mode */
>         if (port->rs485.flags & SER_RS485_ENABLED) {
> @@ -2207,6 +2217,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>         atmel_uart_writel(port, ATMEL_US_BRGR, quot);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         /* restore interrupts */
>         atmel_uart_writel(port, ATMEL_US_IER, imr);
> @@ -2450,6 +2461,7 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
>
>         /* Make sure that tx path is actually able to send characters */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +       atmel_port->tx_stopped = false;
>
>         uart_console_write(port, s, count, atmel_console_putchar);
>
> @@ -2528,6 +2540,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
>         atmel_uart_writel(port, ATMEL_US_IDR, -1);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         if (options)
>                 uart_parse_options(options, &baud, &parity, &bits, &flow);
> --
> 2.11.0
>

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

[PATCH v1 10/10] tty/serial: atmel: Prevent a warning on suspend

[Romain Izard]

2017-09-08 17:36 GMT+02:00 Romain Izard <romain.izard.pro@gmail.com>:
> 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 <romain.izard.pro@gmail.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7551cab438ff..195c0d1b594e 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -171,6 +171,7 @@ struct atmel_uart_port {
>         bool                    has_hw_timer;
>         struct timer_list       uart_timer;
>
> +       bool                    tx_stopped;
>         bool                    suspended;
>         unsigned int            pending;
>         unsigned int            pending_status;
> @@ -380,6 +381,10 @@ static int atmel_config_rs485(struct uart_port *port,
>   */
>  static u_int atmel_tx_empty(struct uart_port *port)
>  {
> +       struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> +
> +       if (atmel_port->tx_stopped)
> +               return TIOCSER_TEMT;
>         return (atmel_uart_readl(port, ATMEL_US_CSR) & ATMEL_US_TXEMPTY) ?
>                 TIOCSER_TEMT :
>                 0;
> @@ -485,6 +490,7 @@ static void atmel_stop_tx(struct uart_port *port)
>          * is fully transmitted.
>          */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
> +       atmel_port->tx_stopped = true;
>
>         /* Disable interrupts */
>         atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> @@ -492,6 +498,7 @@ static void atmel_stop_tx(struct uart_port *port)
>         if ((port->rs485.flags & SER_RS485_ENABLED) &&
>             !(port->rs485.flags & SER_RS485_RX_DURING_TX))
>                 atmel_start_rx(port);
> +
>  }
>
>  /*
> @@ -521,6 +528,7 @@ static void atmel_start_tx(struct uart_port *port)
>
>         /* re-enable the transmitter */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +       atmel_port->tx_stopped = false;
>  }
>
>  /*
> @@ -1866,6 +1874,7 @@ static int atmel_startup(struct uart_port *port)
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         /* enable xmit & rcvr */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         setup_timer(&atmel_port->uart_timer,
>                         atmel_uart_timer_callback,
> @@ -2122,6 +2131,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>
>         /* disable receiver and transmitter */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
> +       atmel_port->tx_stopped = true;
>
>         /* mode */
>         if (port->rs485.flags & SER_RS485_ENABLED) {
> @@ -2207,6 +2217,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>         atmel_uart_writel(port, ATMEL_US_BRGR, quot);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         /* restore interrupts */
>         atmel_uart_writel(port, ATMEL_US_IER, imr);
> @@ -2450,6 +2461,7 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
>
>         /* Make sure that tx path is actually able to send characters */
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +       atmel_port->tx_stopped = false;
>
>         uart_console_write(port, s, count, atmel_console_putchar);
>
> @@ -2528,6 +2540,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
>         atmel_uart_writel(port, ATMEL_US_IDR, -1);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>         atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> +       atmel_port->tx_stopped = false;
>
>         if (options)
>                 uart_parse_options(options, &baud, &parity, &bits, &flow);
> --
> 2.11.0
>

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

Re: [PATCH v1 02/10] clk: at91: pmc: Save SCSR during suspend

[Nicolas Ferre]

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 <romain.izard.pro@gmail.com>

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,

> ---
>  drivers/clk/at91/pmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 5c2b26de303e..07dc2861ad3f 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -86,7 +86,7 @@ static int pmc_suspend(void)
>  {
>  	int i;
>  
> -	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);
> 


-- 
Nicolas Ferre

Re: [PATCH v1 02/10] clk: at91: pmc: Save SCSR during suspend

[Nicolas Ferre]

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 <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Yes, it looks like a typo:
Acked-by: Nicolas Ferre <nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>

I didn't experienced the issue with LCD nor USB though.

Regards,

> ---
>  drivers/clk/at91/pmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 5c2b26de303e..07dc2861ad3f 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -86,7 +86,7 @@ static int pmc_suspend(void)
>  {
>  	int i;
>  
> -	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);
> 


-- 
Nicolas Ferre

[PATCH v1 02/10] clk: at91: pmc: Save SCSR during suspend

[Nicolas Ferre]

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 <romain.izard.pro@gmail.com>

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,

> ---
>  drivers/clk/at91/pmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 5c2b26de303e..07dc2861ad3f 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -86,7 +86,7 @@ static int pmc_suspend(void)
>  {
>  	int i;
>  
> -	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);
> 


-- 
Nicolas Ferre

Re: [PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 08/09/2017 at 17:35, Romain Izard wrote:
> 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 <romain.izard.pro@gmail.com>
> ---
>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 775af473fe11..5c2b26de303e 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>  	return 0;
>  }
>  
> +static bool pmc_ready(unsigned int mask)
> +{
> +	unsigned int status;
> +
> +	regmap_read(pmcreg, AT91_PMC_SR, &status);
> +
> +	return ((status & mask) == mask) ? 1 : 0;
> +}
> +
>  static void pmc_resume(void)
>  {
> -	int i, ret = 0;
> +	int i;
>  	u32 tmp;
> +	u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>  
>  	regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>  	if (pmc_cache.mckr != tmp)
> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>  			     AT91_PMC_PCR_CMD);
>  	}
>  
> -	if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
> -		ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
> -					       !(tmp & AT91_PMC_LOCKU),
> -					       10, 5000);
> -		if (ret)
> -			pr_crit("USB PLL didn't lock when resuming\n");
> -	}
> +	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
> +		mask |= AT91_PMC_LOCKU;
> +
> +	while (!pmc_ready(mask))
> +		cpu_relax();

Okay, but I would prefer to keep the timeout property in it. So we may
need to re-implement a timeout way-out here.

Regards,

>  }
>  
>  static struct syscore_ops pmc_syscore_ops = {
> 


-- 
Nicolas Ferre

Re: [PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 08/09/2017 at 17:35, Romain Izard wrote:
> 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 <romain.izard.pro@gmail.com>
> ---
>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 775af473fe11..5c2b26de303e 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>  	return 0;
>  }
>  
> +static bool pmc_ready(unsigned int mask)
> +{
> +	unsigned int status;
> +
> +	regmap_read(pmcreg, AT91_PMC_SR, &status);
> +
> +	return ((status & mask) == mask) ? 1 : 0;
> +}
> +
>  static void pmc_resume(void)
>  {
> -	int i, ret = 0;
> +	int i;
>  	u32 tmp;
> +	u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>  
>  	regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>  	if (pmc_cache.mckr != tmp)
> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>  			     AT91_PMC_PCR_CMD);
>  	}
>  
> -	if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
> -		ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
> -					       !(tmp & AT91_PMC_LOCKU),
> -					       10, 5000);
> -		if (ret)
> -			pr_crit("USB PLL didn't lock when resuming\n");
> -	}
> +	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
> +		mask |= AT91_PMC_LOCKU;
> +
> +	while (!pmc_ready(mask))
> +		cpu_relax();

Okay, but I would prefer to keep the timeout property in it. So we may
need to re-implement a timeout way-out here.

Regards,

>  }
>  
>  static struct syscore_ops pmc_syscore_ops = {
> 


-- 
Nicolas Ferre

[PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 08/09/2017 at 17:35, Romain Izard wrote:
> 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 <romain.izard.pro@gmail.com>
> ---
>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 775af473fe11..5c2b26de303e 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>  	return 0;
>  }
>  
> +static bool pmc_ready(unsigned int mask)
> +{
> +	unsigned int status;
> +
> +	regmap_read(pmcreg, AT91_PMC_SR, &status);
> +
> +	return ((status & mask) == mask) ? 1 : 0;
> +}
> +
>  static void pmc_resume(void)
>  {
> -	int i, ret = 0;
> +	int i;
>  	u32 tmp;
> +	u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>  
>  	regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>  	if (pmc_cache.mckr != tmp)
> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>  			     AT91_PMC_PCR_CMD);
>  	}
>  
> -	if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
> -		ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
> -					       !(tmp & AT91_PMC_LOCKU),
> -					       10, 5000);
> -		if (ret)
> -			pr_crit("USB PLL didn't lock when resuming\n");
> -	}
> +	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
> +		mask |= AT91_PMC_LOCKU;
> +
> +	while (!pmc_ready(mask))
> +		cpu_relax();

Okay, but I would prefer to keep the timeout property in it. So we may
need to re-implement a timeout way-out here.

Regards,

>  }
>  
>  static struct syscore_ops pmc_syscore_ops = {
> 


-- 
Nicolas Ferre

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 08/09/2017 at 17:35, Romain Izard wrote:
> From: Romain Izard <romain.izard@mobile-devices.fr>
> 
> Save and restore the System Clock and Programmable Clock register for
> the backup use case.

"System Clock" seems to be handled in another patch.

> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  drivers/clk/at91/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 07dc2861ad3f..5421b03553ec 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -66,6 +66,7 @@ static struct
>  	u32 pcr[PMC_MAX_IDS];
>  	u32 audio_pll0;
>  	u32 audio_pll1;
> +	u32 pckr[3];

Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...

>  } 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,


> +		regmap_read(pmcreg, AT91_PMC_PCKR(i), &pmc_cache.pckr[i]);
>  
>  	return 0;
>  }
> @@ -143,6 +146,8 @@ static void pmc_resume(void)
>  			     pmc_cache.pcr[registered_ids[i]] |
>  			     AT91_PMC_PCR_CMD);
>  	}
> +	for (i = 0; i < 3; i++)
> +		regmap_write(pmcreg, AT91_PMC_PCKR(i), pmc_cache.pckr[i]);
>  
>  	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>  		mask |= AT91_PMC_LOCKU;
> 


-- 
Nicolas Ferre

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 08/09/2017 at 17:35, Romain Izard wrote:
> From: Romain Izard <romain.izard@mobile-devices.fr>
> 
> Save and restore the System Clock and Programmable Clock register for
> the backup use case.

"System Clock" seems to be handled in another patch.

> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  drivers/clk/at91/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 07dc2861ad3f..5421b03553ec 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -66,6 +66,7 @@ static struct
>  	u32 pcr[PMC_MAX_IDS];
>  	u32 audio_pll0;
>  	u32 audio_pll1;
> +	u32 pckr[3];

Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...

>  } 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,


> +		regmap_read(pmcreg, AT91_PMC_PCKR(i), &pmc_cache.pckr[i]);
>  
>  	return 0;
>  }
> @@ -143,6 +146,8 @@ static void pmc_resume(void)
>  			     pmc_cache.pcr[registered_ids[i]] |
>  			     AT91_PMC_PCR_CMD);
>  	}
> +	for (i = 0; i < 3; i++)
> +		regmap_write(pmcreg, AT91_PMC_PCKR(i), pmc_cache.pckr[i]);
>  
>  	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>  		mask |= AT91_PMC_LOCKU;
> 


-- 
Nicolas Ferre

[PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 08/09/2017 at 17:35, Romain Izard wrote:
> From: Romain Izard <romain.izard@mobile-devices.fr>
> 
> Save and restore the System Clock and Programmable Clock register for
> the backup use case.

"System Clock" seems to be handled in another patch.

> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  drivers/clk/at91/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 07dc2861ad3f..5421b03553ec 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -66,6 +66,7 @@ static struct
>  	u32 pcr[PMC_MAX_IDS];
>  	u32 audio_pll0;
>  	u32 audio_pll1;
> +	u32 pckr[3];

Some products have different numbers of PCK (only 2 on at91sam9x5 for
instance)...

>  } 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,


> +		regmap_read(pmcreg, AT91_PMC_PCKR(i), &pmc_cache.pckr[i]);
>  
>  	return 0;
>  }
> @@ -143,6 +146,8 @@ static void pmc_resume(void)
>  			     pmc_cache.pcr[registered_ids[i]] |
>  			     AT91_PMC_PCR_CMD);
>  	}
> +	for (i = 0; i < 3; i++)
> +		regmap_write(pmcreg, AT91_PMC_PCKR(i), pmc_cache.pckr[i]);
>  
>  	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>  		mask |= AT91_PMC_LOCKU;
> 


-- 
Nicolas Ferre

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Alexandre Belloni]

On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
> On 08/09/2017 at 17:35, Romain Izard wrote:
> > From: Romain Izard <romain.izard@mobile-devices.fr>
> > 
> > Save and restore the System Clock and Programmable Clock register for
> > the backup use case.
> 
> "System Clock" seems to be handled in another patch.
> 
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > ---
> >  drivers/clk/at91/pmc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> > index 07dc2861ad3f..5421b03553ec 100644
> > --- a/drivers/clk/at91/pmc.c
> > +++ b/drivers/clk/at91/pmc.c
> > @@ -66,6 +66,7 @@ static struct
> >  	u32 pcr[PMC_MAX_IDS];
> >  	u32 audio_pll0;
> >  	u32 audio_pll1;
> > +	u32 pckr[3];
> 
> 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

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Alexandre Belloni]

On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
> On 08/09/2017 at 17:35, Romain Izard wrote:
> > From: Romain Izard <romain.izard-Tny/h2m1dROW3RwJEphJ61AUjnlXr6A1@public.gmane.org>
> > 
> > Save and restore the System Clock and Programmable Clock register for
> > the backup use case.
> 
> "System Clock" seems to be handled in another patch.
> 
> > Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/clk/at91/pmc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> > index 07dc2861ad3f..5421b03553ec 100644
> > --- a/drivers/clk/at91/pmc.c
> > +++ b/drivers/clk/at91/pmc.c
> > @@ -66,6 +66,7 @@ static struct
> >  	u32 pcr[PMC_MAX_IDS];
> >  	u32 audio_pll0;
> >  	u32 audio_pll1;
> > +	u32 pckr[3];
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Alexandre Belloni]

On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
> On 08/09/2017 at 17:35, Romain Izard wrote:
> > From: Romain Izard <romain.izard@mobile-devices.fr>
> > 
> > Save and restore the System Clock and Programmable Clock register for
> > the backup use case.
> 
> "System Clock" seems to be handled in another patch.
> 
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > ---
> >  drivers/clk/at91/pmc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> > index 07dc2861ad3f..5421b03553ec 100644
> > --- a/drivers/clk/at91/pmc.c
> > +++ b/drivers/clk/at91/pmc.c
> > @@ -66,6 +66,7 @@ static struct
> >  	u32 pcr[PMC_MAX_IDS];
> >  	u32 audio_pll0;
> >  	u32 audio_pll1;
> > +	u32 pckr[3];
> 
> 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

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[romain izard]

2017-09-13 19:03 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
>> On 08/09/2017 at 17:35, Romain Izard wrote:
>> > From: Romain Izard <romain.izard@mobile-devices.fr>
>> >
>> > Save and restore the System Clock and Programmable Clock register for
>> > the backup use case.
>>
>> "System Clock" seems to be handled in another patch.
>>
>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> > ---
>> >  drivers/clk/at91/pmc.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> > index 07dc2861ad3f..5421b03553ec 100644
>> > --- a/drivers/clk/at91/pmc.c
>> > +++ b/drivers/clk/at91/pmc.c
>> > @@ -66,6 +66,7 @@ static struct
>> >     u32 pcr[PMC_MAX_IDS];
>> >     u32 audio_pll0;
>> >     u32 audio_pll1;
>> > +   u32 pckr[3];
>>
>> 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

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[romain izard]

2017-09-13 19:03 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
>> On 08/09/2017 at 17:35, Romain Izard wrote:
>> > From: Romain Izard <romain.izard@mobile-devices.fr>
>> >
>> > Save and restore the System Clock and Programmable Clock register for
>> > the backup use case.
>>
>> "System Clock" seems to be handled in another patch.
>>
>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> > ---
>> >  drivers/clk/at91/pmc.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> > index 07dc2861ad3f..5421b03553ec 100644
>> > --- a/drivers/clk/at91/pmc.c
>> > +++ b/drivers/clk/at91/pmc.c
>> > @@ -66,6 +66,7 @@ static struct
>> >     u32 pcr[PMC_MAX_IDS];
>> >     u32 audio_pll0;
>> >     u32 audio_pll1;
>> > +   u32 pckr[3];
>>
>> 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

[PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[romain izard]

2017-09-13 19:03 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
>> On 08/09/2017 at 17:35, Romain Izard wrote:
>> > From: Romain Izard <romain.izard@mobile-devices.fr>
>> >
>> > Save and restore the System Clock and Programmable Clock register for
>> > the backup use case.
>>
>> "System Clock" seems to be handled in another patch.
>>
>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> > ---
>> >  drivers/clk/at91/pmc.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> > index 07dc2861ad3f..5421b03553ec 100644
>> > --- a/drivers/clk/at91/pmc.c
>> > +++ b/drivers/clk/at91/pmc.c
>> > @@ -66,6 +66,7 @@ static struct
>> >     u32 pcr[PMC_MAX_IDS];
>> >     u32 audio_pll0;
>> >     u32 audio_pll1;
>> > +   u32 pckr[3];
>>
>> 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

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 14/09/2017 at 09:41, romain izard wrote:
> 2017-09-13 19:03 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
>> On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
>>> On 08/09/2017 at 17:35, Romain Izard wrote:
>>>> From: Romain Izard <romain.izard@mobile-devices.fr>
>>>>
>>>> Save and restore the System Clock and Programmable Clock register for
>>>> the backup use case.
>>>
>>> "System Clock" seems to be handled in another patch.
>>>
>>>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>>>> ---
>>>>  drivers/clk/at91/pmc.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>>>> index 07dc2861ad3f..5421b03553ec 100644
>>>> --- a/drivers/clk/at91/pmc.c
>>>> +++ b/drivers/clk/at91/pmc.c
>>>> @@ -66,6 +66,7 @@ static struct
>>>>     u32 pcr[PMC_MAX_IDS];
>>>>     u32 audio_pll0;
>>>>     u32 audio_pll1;
>>>> +   u32 pckr[3];
>>>
>>> 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

Re: [PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 14/09/2017 at 09:41, romain izard wrote:
> 2017-09-13 19:03 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
>> On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
>>> On 08/09/2017 at 17:35, Romain Izard wrote:
>>>> From: Romain Izard <romain.izard@mobile-devices.fr>
>>>>
>>>> Save and restore the System Clock and Programmable Clock register for
>>>> the backup use case.
>>>
>>> "System Clock" seems to be handled in another patch.
>>>
>>>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>>>> ---
>>>>  drivers/clk/at91/pmc.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>>>> index 07dc2861ad3f..5421b03553ec 100644
>>>> --- a/drivers/clk/at91/pmc.c
>>>> +++ b/drivers/clk/at91/pmc.c
>>>> @@ -66,6 +66,7 @@ static struct
>>>>     u32 pcr[PMC_MAX_IDS];
>>>>     u32 audio_pll0;
>>>>     u32 audio_pll1;
>>>> +   u32 pckr[3];
>>>
>>> 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v1 03/10] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 14/09/2017 at 09:41, romain izard wrote:
> 2017-09-13 19:03 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
>> On 13/09/2017 at 14:29:35 +0200, Nicolas Ferre wrote:
>>> On 08/09/2017 at 17:35, Romain Izard wrote:
>>>> From: Romain Izard <romain.izard@mobile-devices.fr>
>>>>
>>>> Save and restore the System Clock and Programmable Clock register for
>>>> the backup use case.
>>>
>>> "System Clock" seems to be handled in another patch.
>>>
>>>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>>>> ---
>>>>  drivers/clk/at91/pmc.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>>>> index 07dc2861ad3f..5421b03553ec 100644
>>>> --- a/drivers/clk/at91/pmc.c
>>>> +++ b/drivers/clk/at91/pmc.c
>>>> @@ -66,6 +66,7 @@ static struct
>>>>     u32 pcr[PMC_MAX_IDS];
>>>>     u32 audio_pll0;
>>>>     u32 audio_pll1;
>>>> +   u32 pckr[3];
>>>
>>> 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

Re: [PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Romain Izard]

2017-09-13 14:15 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> On 08/09/2017 at 17:35, Romain Izard wrote:
>> 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 <romain.izard.pro@gmail.com>
>> ---
>>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> index 775af473fe11..5c2b26de303e 100644
>> --- a/drivers/clk/at91/pmc.c
>> +++ b/drivers/clk/at91/pmc.c
>> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>>       return 0;
>>  }
>>
>> +static bool pmc_ready(unsigned int mask)
>> +{
>> +     unsigned int status;
>> +
>> +     regmap_read(pmcreg, AT91_PMC_SR, &status);
>> +
>> +     return ((status & mask) == mask) ? 1 : 0;
>> +}
>> +
>>  static void pmc_resume(void)
>>  {
>> -     int i, ret = 0;
>> +     int i;
>>       u32 tmp;
>> +     u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>>
>>       regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>>       if (pmc_cache.mckr != tmp)
>> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>>                            AT91_PMC_PCR_CMD);
>>       }
>>
>> -     if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
>> -             ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
>> -                                            !(tmp & AT91_PMC_LOCKU),
>> -                                            10, 5000);
>> -             if (ret)
>> -                     pr_crit("USB PLL didn't lock when resuming\n");
>> -     }
>> +     if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>> +             mask |= AT91_PMC_LOCKU;
>> +
>> +     while (!pmc_ready(mask))
>> +             cpu_relax();
>
> 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

Re: [PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Romain Izard]

2017-09-13 14:15 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> On 08/09/2017 at 17:35, Romain Izard wrote:
>> 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 <romain.izard.pro@gmail.com>
>> ---
>>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> index 775af473fe11..5c2b26de303e 100644
>> --- a/drivers/clk/at91/pmc.c
>> +++ b/drivers/clk/at91/pmc.c
>> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>>       return 0;
>>  }
>>
>> +static bool pmc_ready(unsigned int mask)
>> +{
>> +     unsigned int status;
>> +
>> +     regmap_read(pmcreg, AT91_PMC_SR, &status);
>> +
>> +     return ((status & mask) == mask) ? 1 : 0;
>> +}
>> +
>>  static void pmc_resume(void)
>>  {
>> -     int i, ret = 0;
>> +     int i;
>>       u32 tmp;
>> +     u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>>
>>       regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>>       if (pmc_cache.mckr != tmp)
>> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>>                            AT91_PMC_PCR_CMD);
>>       }
>>
>> -     if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
>> -             ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
>> -                                            !(tmp & AT91_PMC_LOCKU),
>> -                                            10, 5000);
>> -             if (ret)
>> -                     pr_crit("USB PLL didn't lock when resuming\n");
>> -     }
>> +     if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>> +             mask |= AT91_PMC_LOCKU;
>> +
>> +     while (!pmc_ready(mask))
>> +             cpu_relax();
>
> 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

[PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Romain Izard]

2017-09-13 14:15 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> On 08/09/2017 at 17:35, Romain Izard wrote:
>> 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 <romain.izard.pro@gmail.com>
>> ---
>>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> index 775af473fe11..5c2b26de303e 100644
>> --- a/drivers/clk/at91/pmc.c
>> +++ b/drivers/clk/at91/pmc.c
>> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>>       return 0;
>>  }
>>
>> +static bool pmc_ready(unsigned int mask)
>> +{
>> +     unsigned int status;
>> +
>> +     regmap_read(pmcreg, AT91_PMC_SR, &status);
>> +
>> +     return ((status & mask) == mask) ? 1 : 0;
>> +}
>> +
>>  static void pmc_resume(void)
>>  {
>> -     int i, ret = 0;
>> +     int i;
>>       u32 tmp;
>> +     u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>>
>>       regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>>       if (pmc_cache.mckr != tmp)
>> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>>                            AT91_PMC_PCR_CMD);
>>       }
>>
>> -     if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
>> -             ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
>> -                                            !(tmp & AT91_PMC_LOCKU),
>> -                                            10, 5000);
>> -             if (ret)
>> -                     pr_crit("USB PLL didn't lock when resuming\n");
>> -     }
>> +     if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>> +             mask |= AT91_PMC_LOCKU;
>> +
>> +     while (!pmc_ready(mask))
>> +             cpu_relax();
>
> 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

Re: [PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 14/09/2017 at 18:15, Romain Izard wrote:
> 2017-09-13 14:15 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
>> On 08/09/2017 at 17:35, Romain Izard wrote:
>>> 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 <romain.izard.pro@gmail.com>
>>> ---
>>>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>>> index 775af473fe11..5c2b26de303e 100644
>>> --- a/drivers/clk/at91/pmc.c
>>> +++ b/drivers/clk/at91/pmc.c
>>> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>>>       return 0;
>>>  }
>>>
>>> +static bool pmc_ready(unsigned int mask)
>>> +{
>>> +     unsigned int status;
>>> +
>>> +     regmap_read(pmcreg, AT91_PMC_SR, &status);
>>> +
>>> +     return ((status & mask) == mask) ? 1 : 0;
>>> +}
>>> +
>>>  static void pmc_resume(void)
>>>  {
>>> -     int i, ret = 0;
>>> +     int i;
>>>       u32 tmp;
>>> +     u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>>>
>>>       regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>>>       if (pmc_cache.mckr != tmp)
>>> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>>>                            AT91_PMC_PCR_CMD);
>>>       }
>>>
>>> -     if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
>>> -             ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
>>> -                                            !(tmp & AT91_PMC_LOCKU),
>>> -                                            10, 5000);
>>> -             if (ret)
>>> -                     pr_crit("USB PLL didn't lock when resuming\n");
>>> -     }
>>> +     if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>>> +             mask |= AT91_PMC_LOCKU;
>>> +
>>> +     while (!pmc_ready(mask))
>>> +             cpu_relax();
>>
>> 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

Re: [PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 14/09/2017 at 18:15, Romain Izard wrote:
> 2017-09-13 14:15 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
>> On 08/09/2017 at 17:35, Romain Izard wrote:
>>> 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 <romain.izard.pro@gmail.com>
>>> ---
>>>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>>> index 775af473fe11..5c2b26de303e 100644
>>> --- a/drivers/clk/at91/pmc.c
>>> +++ b/drivers/clk/at91/pmc.c
>>> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>>>       return 0;
>>>  }
>>>
>>> +static bool pmc_ready(unsigned int mask)
>>> +{
>>> +     unsigned int status;
>>> +
>>> +     regmap_read(pmcreg, AT91_PMC_SR, &status);
>>> +
>>> +     return ((status & mask) == mask) ? 1 : 0;
>>> +}
>>> +
>>>  static void pmc_resume(void)
>>>  {
>>> -     int i, ret = 0;
>>> +     int i;
>>>       u32 tmp;
>>> +     u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>>>
>>>       regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>>>       if (pmc_cache.mckr != tmp)
>>> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>>>                            AT91_PMC_PCR_CMD);
>>>       }
>>>
>>> -     if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
>>> -             ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
>>> -                                            !(tmp & AT91_PMC_LOCKU),
>>> -                                            10, 5000);
>>> -             if (ret)
>>> -                     pr_crit("USB PLL didn't lock when resuming\n");
>>> -     }
>>> +     if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>>> +             mask |= AT91_PMC_LOCKU;
>>> +
>>> +     while (!pmc_ready(mask))
>>> +             cpu_relax();
>>
>> 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

[PATCH v1 01/10] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 14/09/2017 at 18:15, Romain Izard wrote:
> 2017-09-13 14:15 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
>> On 08/09/2017 at 17:35, Romain Izard wrote:
>>> 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 <romain.izard.pro@gmail.com>
>>> ---
>>>  drivers/clk/at91/pmc.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>>> index 775af473fe11..5c2b26de303e 100644
>>> --- a/drivers/clk/at91/pmc.c
>>> +++ b/drivers/clk/at91/pmc.c
>>> @@ -107,10 +107,20 @@ static int pmc_suspend(void)
>>>       return 0;
>>>  }
>>>
>>> +static bool pmc_ready(unsigned int mask)
>>> +{
>>> +     unsigned int status;
>>> +
>>> +     regmap_read(pmcreg, AT91_PMC_SR, &status);
>>> +
>>> +     return ((status & mask) == mask) ? 1 : 0;
>>> +}
>>> +
>>>  static void pmc_resume(void)
>>>  {
>>> -     int i, ret = 0;
>>> +     int i;
>>>       u32 tmp;
>>> +     u32 mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;
>>>
>>>       regmap_read(pmcreg, AT91_PMC_MCKR, &tmp);
>>>       if (pmc_cache.mckr != tmp)
>>> @@ -134,13 +144,11 @@ static void pmc_resume(void)
>>>                            AT91_PMC_PCR_CMD);
>>>       }
>>>
>>> -     if (pmc_cache.uckr & AT91_PMC_UPLLEN) {
>>> -             ret = regmap_read_poll_timeout(pmcreg, AT91_PMC_SR, tmp,
>>> -                                            !(tmp & AT91_PMC_LOCKU),
>>> -                                            10, 5000);
>>> -             if (ret)
>>> -                     pr_crit("USB PLL didn't lock when resuming\n");
>>> -     }
>>> +     if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>>> +             mask |= AT91_PMC_LOCKU;
>>> +
>>> +     while (!pmc_ready(mask))
>>> +             cpu_relax();
>>
>> 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