[PATCH v2 0/9] Various patches for SAMA5D2 backup mode

[PATCH v2 0/9] 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.

Changes in v2:
* drop the IIO patch duplicating existing code
* determine the number of programmable clocks to save dynamically
* declare a required local variable in the tty/serial patch

Romain Izard (9):
  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
  pwm: atmel-tcb: Support backup mode
  atmel_flexcom: Support backup mode
  tty/serial: atmel: Prevent a warning on suspend

 drivers/clk/at91/clk-programmable.c |  2 ++
 drivers/clk/at91/pmc.c              | 55 +++++++++++++++++++++++++------
 drivers/clk/at91/pmc.h              |  2 ++
 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   | 14 ++++++++
 drivers/usb/host/ehci-atmel.c       |  3 +-
 8 files changed, 182 insertions(+), 37 deletions(-)

-- 
2.11.0

[PATCH v2 1/9] 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

Re: [PATCH v2 1/9] clk: at91: pmc: Wait for clocks when resuming

[Ludovic Desroches]

On Fri, Sep 15, 2017 at 04:04:03PM +0200, 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>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

I faced the same issue because of the use of regmap_read_poll_timeout
when timekeeping is not ready.


Ludovic

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

Re: [PATCH v2 1/9] clk: at91: pmc: Wait for clocks when resuming

[Nicolas Ferre]

On 15/09/2017 at 16:04, 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>

And here is my:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.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 = {
> 


-- 
Nicolas Ferre

[PATCH v2 2/9] 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>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.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 v2 3/9] clk: at91: pmc: Support backup for programmable clocks

[Romain Izard]

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

When an AT91 programmable clock is declared in the device tree, register
it into the Power Management Controller driver. On entering suspend mode,
the driver saves and restores the Programmable Clock registers to support
the backup mode for these clocks.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
Changes in v2:
* register PCKs on clock startup

 drivers/clk/at91/clk-programmable.c |  2 ++
 drivers/clk/at91/pmc.c              | 27 +++++++++++++++++++++++++++
 drivers/clk/at91/pmc.h              |  2 ++
 3 files changed, 31 insertions(+)

diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c
index 85a449cf61e3..0e6aab1252fc 100644
--- a/drivers/clk/at91/clk-programmable.c
+++ b/drivers/clk/at91/clk-programmable.c
@@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap,
 	if (ret) {
 		kfree(prog);
 		hw = ERR_PTR(ret);
+	} else {
+		pmc_register_pck(id);
 	}
 
 	return hw;
diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 07dc2861ad3f..3910b7537152 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -22,6 +22,7 @@
 #include "pmc.h"
 
 #define PMC_MAX_IDS 128
+#define PMC_MAX_PCKS 8
 
 int of_at91_get_clk_range(struct device_node *np, const char *propname,
 			  struct clk_range *range)
@@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range);
 static struct regmap *pmcreg;
 
 static u8 registered_ids[PMC_MAX_IDS];
+static u8 registered_pcks[PMC_MAX_PCKS];
 
 static struct
 {
@@ -66,8 +68,10 @@ static struct
 	u32 pcr[PMC_MAX_IDS];
 	u32 audio_pll0;
 	u32 audio_pll1;
+	u32 pckr[PMC_MAX_PCKS];
 } pmc_cache;
 
+/* Clock ID 0 is invalid */
 void pmc_register_id(u8 id)
 {
 	int i;
@@ -82,6 +86,21 @@ void pmc_register_id(u8 id)
 	}
 }
 
+/* Programmable Clock 0 is valid */
+void pmc_register_pck(u8 pck)
+{
+	int i;
+
+	for (i = 0; i < PMC_MAX_PCKS; i++) {
+		if (registered_pcks[i] == 0) {
+			registered_pcks[i] = pck + 1;
+			break;
+		}
+		if (registered_pcks[i] == (pck + 1))
+			break;
+	}
+}
+
 static int pmc_suspend(void)
 {
 	int i;
@@ -103,6 +122,10 @@ static int pmc_suspend(void)
 		regmap_read(pmcreg, AT91_PMC_PCR,
 			    &pmc_cache.pcr[registered_ids[i]]);
 	}
+	for (i = 0; registered_pcks[i]; i++) {
+		u8 num = registered_pcks[i] - 1;
+		regmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]);
+	}
 
 	return 0;
 }
@@ -143,6 +166,10 @@ static void pmc_resume(void)
 			     pmc_cache.pcr[registered_ids[i]] |
 			     AT91_PMC_PCR_CMD);
 	}
+	for (i = 0; registered_pcks[i]; i++) {
+		u8 num = registered_pcks[i] - 1;
+		regmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]);
+	}
 
 	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
 		mask |= AT91_PMC_LOCKU;
diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
index 858e8ef7e8db..d22b1fa9ecdc 100644
--- a/drivers/clk/at91/pmc.h
+++ b/drivers/clk/at91/pmc.h
@@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,
 
 #ifdef CONFIG_PM
 void pmc_register_id(u8 id);
+void pmc_register_pck(u8 pck);
 #else
 static inline void pmc_register_id(u8 id) {}
+static inline void pmc_register_pck(u8 pck) {}
 #endif
 
 #endif /* __PMC_H_ */
-- 
2.11.0

Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable clocks

[Nicolas Ferre]

On 15/09/2017 at 16:04, Romain Izard wrote:
> From: Romain Izard <romain.izard@mobile-devices.fr>
> 
> When an AT91 programmable clock is declared in the device tree, register
> it into the Power Management Controller driver. On entering suspend mode,
> the driver saves and restores the Programmable Clock registers to support
> the backup mode for these clocks.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>

Romain,

Some nitpicking and one comment. But on the overall patch, here is my:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

See below:

> ---
> Changes in v2:
> * register PCKs on clock startup
> 
>  drivers/clk/at91/clk-programmable.c |  2 ++
>  drivers/clk/at91/pmc.c              | 27 +++++++++++++++++++++++++++
>  drivers/clk/at91/pmc.h              |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c
> index 85a449cf61e3..0e6aab1252fc 100644
> --- a/drivers/clk/at91/clk-programmable.c
> +++ b/drivers/clk/at91/clk-programmable.c
> @@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap,
>  	if (ret) {
>  		kfree(prog);
>  		hw = ERR_PTR(ret);

Nit: "else" not needed.

> +	} else {
> +		pmc_register_pck(id);
>  	}
>  
>  	return hw;
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 07dc2861ad3f..3910b7537152 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -22,6 +22,7 @@
>  #include "pmc.h"
>  
>  #define PMC_MAX_IDS 128
> +#define PMC_MAX_PCKS 8
>  
>  int of_at91_get_clk_range(struct device_node *np, const char *propname,
>  			  struct clk_range *range)
> @@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range);
>  static struct regmap *pmcreg;
>  
>  static u8 registered_ids[PMC_MAX_IDS];
> +static u8 registered_pcks[PMC_MAX_PCKS];
>  
>  static struct
>  {
> @@ -66,8 +68,10 @@ static struct
>  	u32 pcr[PMC_MAX_IDS];
>  	u32 audio_pll0;
>  	u32 audio_pll1;
> +	u32 pckr[PMC_MAX_PCKS];
>  } pmc_cache;
>  
> +/* Clock ID 0 is invalid */

(read: so we can use the 0 value as an indicator that this place in the
table hasn't been filled, so unused)

>  void pmc_register_id(u8 id)
>  {
>  	int i;
> @@ -82,6 +86,21 @@ void pmc_register_id(u8 id)
>  	}
>  }
>  
> +/* Programmable Clock 0 is valid */

I understand the rationale behind these ^^ two comments, but I would
like that it's more explicit. Saying that you will store the pck id as
(id + 1) and that you would have to invert this operation while using
the stored id.
Maybe add a comment about this transformation to the struct definition
as well...


> +void pmc_register_pck(u8 pck)
> +{
> +	int i;
> +
> +	for (i = 0; i < PMC_MAX_PCKS; i++) {
> +		if (registered_pcks[i] == 0) {
> +			registered_pcks[i] = pck + 1;
> +			break;
> +		}
> +		if (registered_pcks[i] == (pck + 1))
> +			break;
> +	}
> +}
> +
>  static int pmc_suspend(void)
>  {
>  	int i;
> @@ -103,6 +122,10 @@ static int pmc_suspend(void)
>  		regmap_read(pmcreg, AT91_PMC_PCR,
>  			    &pmc_cache.pcr[registered_ids[i]]);
>  	}
> +	for (i = 0; registered_pcks[i]; i++) {
> +		u8 num = registered_pcks[i] - 1;

Nit: declaration are better made at the beginning of the function. This
lead to a checkpatch warning:
"WARNING: Missing a blank line after declarations"

> +		regmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]);
> +	}
>  
>  	return 0;
>  }
> @@ -143,6 +166,10 @@ static void pmc_resume(void)
>  			     pmc_cache.pcr[registered_ids[i]] |
>  			     AT91_PMC_PCR_CMD);
>  	}
> +	for (i = 0; registered_pcks[i]; i++) {
> +		u8 num = registered_pcks[i] - 1;

Ditto

> +		regmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]);
> +	}
>  
>  	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>  		mask |= AT91_PMC_LOCKU;
> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> index 858e8ef7e8db..d22b1fa9ecdc 100644
> --- a/drivers/clk/at91/pmc.h
> +++ b/drivers/clk/at91/pmc.h
> @@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,
>  
>  #ifdef CONFIG_PM
>  void pmc_register_id(u8 id);
> +void pmc_register_pck(u8 pck);
>  #else
>  static inline void pmc_register_id(u8 id) {}
> +static inline void pmc_register_pck(u8 pck) {}
>  #endif
>  
>  #endif /* __PMC_H_ */
> 


-- 
Nicolas Ferre

Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable clocks

[Romain Izard]

2017-09-22 12:31 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> On 15/09/2017 at 16:04, Romain Izard wrote:
>> From: Romain Izard <romain.izard@mobile-devices.fr>
>>
>> When an AT91 programmable clock is declared in the device tree, register
>> it into the Power Management Controller driver. On entering suspend mode,
>> the driver saves and restores the Programmable Clock registers to support
>> the backup mode for these clocks.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>
> Romain,
>
> Some nitpicking and one comment. But on the overall patch, here is my:
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> See below:
>
>> ---
>> Changes in v2:
>> * register PCKs on clock startup
>>
>>  drivers/clk/at91/clk-programmable.c |  2 ++
>>  drivers/clk/at91/pmc.c              | 27 +++++++++++++++++++++++++++
>>  drivers/clk/at91/pmc.h              |  2 ++
>>  3 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c
>> index 85a449cf61e3..0e6aab1252fc 100644
>> --- a/drivers/clk/at91/clk-programmable.c
>> +++ b/drivers/clk/at91/clk-programmable.c
>> @@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap,
>>       if (ret) {
>>               kfree(prog);
>>               hw = ERR_PTR(ret);
>
> Nit: "else" not needed.
>
This is a shared idiom in all the atmel clock drivers, so I prefer to keep
it this way.

>> +     } else {
>> +             pmc_register_pck(id);
>>       }
>>
>>       return hw;
>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> index 07dc2861ad3f..3910b7537152 100644
>> --- a/drivers/clk/at91/pmc.c
>> +++ b/drivers/clk/at91/pmc.c
>> @@ -22,6 +22,7 @@
>>  #include "pmc.h"
>>
>>  #define PMC_MAX_IDS 128
>> +#define PMC_MAX_PCKS 8
>>
>>  int of_at91_get_clk_range(struct device_node *np, const char *propname,
>>                         struct clk_range *range)
>> @@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range);
>>  static struct regmap *pmcreg;
>>
>>  static u8 registered_ids[PMC_MAX_IDS];
>> +static u8 registered_pcks[PMC_MAX_PCKS];
>>
>>  static struct
>>  {
>> @@ -66,8 +68,10 @@ static struct
>>       u32 pcr[PMC_MAX_IDS];
>>       u32 audio_pll0;
>>       u32 audio_pll1;
>> +     u32 pckr[PMC_MAX_PCKS];
>>  } pmc_cache;
>>
>> +/* Clock ID 0 is invalid */
>
> (read: so we can use the 0 value as an indicator that this place in the
> table hasn't been filled, so unused)
>
>>  void pmc_register_id(u8 id)
>>  {
>>       int i;
>> @@ -82,6 +86,21 @@ void pmc_register_id(u8 id)
>>       }
>>  }
>>
>> +/* Programmable Clock 0 is valid */
>
> I understand the rationale behind these ^^ two comments, but I would
> like that it's more explicit. Saying that you will store the pck id as
> (id + 1) and that you would have to invert this operation while using
> the stored id.
> Maybe add a comment about this transformation to the struct definition
> as well...
>

I will improve the comments for the next revision.

>
>> +void pmc_register_pck(u8 pck)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < PMC_MAX_PCKS; i++) {
>> +             if (registered_pcks[i] == 0) {
>> +                     registered_pcks[i] = pck + 1;
>> +                     break;
>> +             }
>> +             if (registered_pcks[i] == (pck + 1))
>> +                     break;
>> +     }
>> +}
>> +
>>  static int pmc_suspend(void)
>>  {
>>       int i;
>> @@ -103,6 +122,10 @@ static int pmc_suspend(void)
>>               regmap_read(pmcreg, AT91_PMC_PCR,
>>                           &pmc_cache.pcr[registered_ids[i]]);
>>       }
>> +     for (i = 0; registered_pcks[i]; i++) {
>> +             u8 num = registered_pcks[i] - 1;
>
> Nit: declaration are better made at the beginning of the function. This
> lead to a checkpatch warning:
> "WARNING: Missing a blank line after declarations"
>

I'll fix this as well.

>> +             regmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]);
>> +     }
>>
>>       return 0;
>>  }
>> @@ -143,6 +166,10 @@ static void pmc_resume(void)
>>                            pmc_cache.pcr[registered_ids[i]] |
>>                            AT91_PMC_PCR_CMD);
>>       }
>> +     for (i = 0; registered_pcks[i]; i++) {
>> +             u8 num = registered_pcks[i] - 1;
>
> Ditto
>
>> +             regmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]);
>> +     }
>>
>>       if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>>               mask |= AT91_PMC_LOCKU;
>> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
>> index 858e8ef7e8db..d22b1fa9ecdc 100644
>> --- a/drivers/clk/at91/pmc.h
>> +++ b/drivers/clk/at91/pmc.h
>> @@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,
>>
>>  #ifdef CONFIG_PM
>>  void pmc_register_id(u8 id);
>> +void pmc_register_pck(u8 pck);
>>  #else
>>  static inline void pmc_register_id(u8 id) {}
>> +static inline void pmc_register_pck(u8 pck) {}
>>  #endif
>>
>>  #endif /* __PMC_H_ */
>>

-- 
Romain Izard

[PATCH v2 4/9] 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

Re: [PATCH v2 4/9] mtd: nand: atmel: Avoid ECC errors when leaving backup mode

[Boris Brezillon]

Hi Romain,

On Fri, 15 Sep 2017 16:04:06 +0200
Romain Izard <romain.izard.pro@gmail.com> wrote:

> 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);

So know you leave the ECC engine enabled even when it's not in use? Not
sure what kind of implication this has on power-consumption, but I
think I'd prefer to keep the write RST+DISABLE sequence in the disable
path.

How about creating a atmel_pmecc_reset() function that you'd call from
the nand-controller resume hook. Something like:

void atmel_pmecc_reset(struct atmel_pmecc *pmecc)
{
	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
}

This way you can re-use the same function and call it from the probe
and disable path as well.

Regards,

Boris

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

[PATCH v2 5/9] 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

Re: [PATCH v2 5/9] mtd: nand: atmel: Report PMECC failures as errors

[Boris Brezillon]

Hi Romain,

On Fri, 15 Sep 2017 16:04:07 +0200
Romain Izard <romain.izard.pro@gmail.com> wrote:

> It is not normal for the PMECC to fail when trying to fix ECC errors.
> Report these cases as errors.

I'm not sure we want to have ECC error messages at this level. ECC
errors are rather unusual but not impossible, and sometimes it's even
not a real error (I'm thinking of bitflips in erased pages for
example, which are not necessarily detected/fixed in hardware).

If we decide to print error messages when unfixable bitflips are
detected, it should be done in the nand-controller driver (somewhere
along those lines [1]).

Regards,

Boris

[1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/atmel/nand-controller.c#L827

> 
> 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;

Re: [PATCH v2 5/9] mtd: nand: atmel: Report PMECC failures as errors

[Romain Izard]

2017-09-18 12:00 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Romain,
>
> On Fri, 15 Sep 2017 16:04:07 +0200
> Romain Izard <romain.izard.pro@gmail.com> wrote:
>
>> It is not normal for the PMECC to fail when trying to fix ECC errors.
>> Report these cases as errors.
>
> I'm not sure we want to have ECC error messages at this level. ECC
> errors are rather unusual but not impossible, and sometimes it's even
> not a real error (I'm thinking of bitflips in erased pages for
> example, which are not necessarily detected/fixed in hardware).
>
> If we decide to print error messages when unfixable bitflips are
> detected, it should be done in the nand-controller driver (somewhere
> along those lines [1]).
>
> Regards,
>
> Boris
>
> [1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/atmel/nand-controller.c#L827
>
>>
>> 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;
>

Ok, I will drop this patch.

[PATCH v2 6/9] 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

Re: [PATCH v2 6/9] ehci-atmel: Power down during suspend is normal

[Nicolas Ferre]

On 15/09/2017 at 16:04, Romain Izard wrote:
> 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;

Ok, I agree with that as the underlying function takes care about the
controller, in any case (even for !B+S-R case). So we don't have any
added value to propagate this information.

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

>  }
>  
>  #ifdef CONFIG_OF
> 


-- 
Nicolas Ferre

[PATCH v2 7/9] 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

Re: [PATCH v2 7/9] pwm: atmel-tcb: Support backup mode

[Nicolas Ferre]

On 15/09/2017 at 16:04, Romain Izard wrote:
> 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>

Seems good to me:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.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,
> 


-- 
Nicolas Ferre

[PATCH v2 8/9] 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

Re: [PATCH v2 8/9] atmel_flexcom: Support backup mode

[Nicolas Ferre]

On 15/09/2017 at 16:04, Romain Izard wrote:
> The controller used by a flexcom module is configured at boot, and left
> alone after this. As the configuration will be lost after backup mode,
> restore the state of the flexcom driver on resume.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>

Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
On sama5d2 Xplained board (i2c0 from flexcom 4).
and obviously:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks Romain!

Regards,

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


-- 
Nicolas Ferre

Re: [PATCH v2 8/9] atmel_flexcom: Support backup mode

[Lee Jones]

On Tue, 19 Sep 2017, Nicolas Ferre wrote:

> On 15/09/2017 at 16:04, Romain Izard wrote:
> > The controller used by a flexcom module is configured at boot, and left
> > alone after this. As the configuration will be lost after backup mode,
> > restore the state of the flexcom driver on resume.
> > 
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> 
> Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> On sama5d2 Xplained board (i2c0 from flexcom 4).
> and obviously:
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Thanks Romain!
> 
> Regards,
> 
> > ---
> >  drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 50 insertions(+), 15 deletions(-)

This is the first time I've seen this patch.  Why's that?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 8/9] atmel_flexcom: Support backup mode

[Romain Izard]

2017-09-19 17:25 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 19 Sep 2017, Nicolas Ferre wrote:
>
>> On 15/09/2017 at 16:04, Romain Izard wrote:
>> > The controller used by a flexcom module is configured at boot, and left
>> > alone after this. As the configuration will be lost after backup mode,
>> > restore the state of the flexcom driver on resume.
>> >
>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>>
>> Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> On sama5d2 Xplained board (i2c0 from flexcom 4).
>> and obviously:
>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>
>> Thanks Romain!
>>
>> Regards,
>>
>> > ---
>> >  drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
>> >  1 file changed, 50 insertions(+), 15 deletions(-)
>
> This is the first time I've seen this patch.  Why's that?
>

As the patchset covers many subsystems, get_maintainers.pl provided a
very long list of both developpers and mailing lists (28). I thought it
was a good idea to shorten it a little. Bad idea. Sorry.

Best regards,
-- 
Romain Izard

Re: [PATCH v2 8/9] atmel_flexcom: Support backup mode

[Alexandre Belloni]

On 20/09/2017 at 10:30:31 +0200, Romain Izard wrote:
> 2017-09-19 17:25 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> > On Tue, 19 Sep 2017, Nicolas Ferre wrote:
> >
> >> On 15/09/2017 at 16:04, Romain Izard wrote:
> >> > The controller used by a flexcom module is configured at boot, and left
> >> > alone after this. As the configuration will be lost after backup mode,
> >> > restore the state of the flexcom driver on resume.
> >> >
> >> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> >>
> >> Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> >> On sama5d2 Xplained board (i2c0 from flexcom 4).
> >> and obviously:
> >> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> >>
> >> Thanks Romain!
> >>
> >> Regards,
> >>
> >> > ---
> >> >  drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
> >> >  1 file changed, 50 insertions(+), 15 deletions(-)
> >
> > This is the first time I've seen this patch.  Why's that?
> >
> 
> As the patchset covers many subsystems, get_maintainers.pl provided a
> very long list of both developpers and mailing lists (28). I thought it
> was a good idea to shorten it a little. Bad idea. Sorry.
> 

I think the correct way of handling that would have been to send each
patch to the proper subsystem as there are no dependency here.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 9/9] 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7551cab438ff..783af6648be0 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);
 
@@ -2511,6 +2523,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
 {
 	int ret;
 	struct uart_port *port = &atmel_ports[co->index].uart;
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	int baud = 115200;
 	int bits = 8;
 	int parity = 'n';
@@ -2528,6 +2541,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 v2 9/9] tty/serial: atmel: Prevent a warning on suspend

[Nicolas Ferre]

On 15/09/2017 at 16:04, Romain Izard wrote:
> 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>

Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
on sama5d2 Xplained.

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

> ---
>  drivers/tty/serial/atmel_serial.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7551cab438ff..783af6648be0 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);
>  
> @@ -2511,6 +2523,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
>  {
>  	int ret;
>  	struct uart_port *port = &atmel_ports[co->index].uart;
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	int baud = 115200;
>  	int bits = 8;
>  	int parity = 'n';
> @@ -2528,6 +2541,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);
> 


-- 
Nicolas Ferre

Re: [PATCH v2 9/9] tty/serial: atmel: Prevent a warning on suspend

[Richard Genoud]

On 15/09/2017 16:04, Romain Izard wrote:
> 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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7551cab438ff..783af6648be0 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);
> +
>  }
This line feed is not needed.
Otherwise,

Acked-by: Richard Genoud <richard.genoud@gmail.com>

>  
>  /*
> @@ -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);
>  
> @@ -2511,6 +2523,7 @@ static int __init atmel_console_setup(struct console *co, char *options)
>  {
>  	int ret;
>  	struct uart_port *port = &atmel_ports[co->index].uart;
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	int baud = 115200;
>  	int bits = 8;
>  	int parity = 'n';
> @@ -2528,6 +2541,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);
>