[PATCH v5] atmel_flexcom: Support backup mode

The controller used by a flexcom module is configured at boot, and left
alone after this. As the configuration will be lost after backup mode,
restore the state of the flexcom driver on resume.

Signed-off-by: Romain Izard <redacted>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changes in v5:
* extract from the patch series, and send as a standalone patch

 drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 064bde9cff5a..ef1235c4a179 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c

@@ -39,34 +39,44 @@
 #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
 				 FLEX_MR_OPMODE_MASK)
 
+struct atmel_flexcom {
+	void __iomem *base;
+	u32 opmode;
+	struct clk *clk;
+};
 
 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct clk *clk;
 	struct resource *res;
-	void __iomem *base;
-	u32 opmode;
+	struct atmel_flexcom *afc;
 	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

On Thu, 19 Oct 2017, Romain Izard wrote:

Nit: I'd prefer if you call this 'ddata'.

But the concept and implementation is fine, so if you're going to
change it please do so and apply my:

Acked-by: Lee Jones <redacted>

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

On Mon, 23 Oct 2017, Lee Jones wrote:

On Thu, 19 Oct 2017, Romain Izard wrote:

quoted

The controller used by a flexcom module is configured at boot, and left
alone after this. As the configuration will be lost after backup mode,
restore the state of the flexcom driver on resume.

Signed-off-by: Romain Izard <redacted>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changes in v5:
* extract from the patch series, and send as a standalone patch

 drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 064bde9cff5a..ef1235c4a179 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c

@@ -39,34 +39,44 @@
 #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
 				 FLEX_MR_OPMODE_MASK)
 
+struct atmel_flexcom {
+	void __iomem *base;
+	u32 opmode;
+	struct clk *clk;
+};
 
 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct clk *clk;
 	struct resource *res;
-	void __iomem *base;
-	u32 opmode;
+	struct atmel_flexcom *afc;

Nit: I'd prefer if you call this 'ddata'.

But the concept and implementation is fine, so if you're going to
change it please do so and apply my:

Acked-by: Lee Jones <redacted>

Also, 'back-up mode' isn't really a thing is it?

How about "Reinstall state on resume" or similar?

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

2017-10-23 18:07 GMT+02:00 Lee Jones [off-list ref]:

On Mon, 23 Oct 2017, Lee Jones wrote:

quoted

On Thu, 19 Oct 2017, Romain Izard wrote:

quoted

The controller used by a flexcom module is configured at boot, and left
alone after this. As the configuration will be lost after backup mode,
restore the state of the flexcom driver on resume.

Signed-off-by: Romain Izard <redacted>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changes in v5:
* extract from the patch series, and send as a standalone patch

 drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 064bde9cff5a..ef1235c4a179 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c

@@ -39,34 +39,44 @@
 #define FLEX_MR_OPMODE(opmode)     (((opmode) << FLEX_MR_OPMODE_OFFSET) &  \
                             FLEX_MR_OPMODE_MASK)

+struct atmel_flexcom {
+   void __iomem *base;
+   u32 opmode;
+   struct clk *clk;
+};

 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
    struct device_node *np = pdev->dev.of_node;
-   struct clk *clk;
    struct resource *res;
-   void __iomem *base;
-   u32 opmode;
+   struct atmel_flexcom *afc;

Nit: I'd prefer if you call this 'ddata'.

But the concept and implementation is fine, so if you're going to
change it please do so and apply my:

Acked-by: Lee Jones <redacted>

Also, 'back-up mode' isn't really a thing is it?

How about "Reinstall state on resume" or similar?

The expression comes from the SAMA5D2's datasheet.

Other Atmel chips use a different single suspend mode with Linux, where
the SoC remains completely powered with a slow clock. The registers are
preserved in this mode, so there is no need for a specific suspend and
resume code.

The SoC can also be powered down, but the CPU is reset and only a small
part is powered with a backup battery to maintain a valid RTC and a
small internal SRAM.

In the SAMA5D2, the mode with only the backup power supply has been
extended to isolate the memory I/O lines, making it possible to keep the
external SDRAM memory in self-refresh. This mode has a lower consumption
compared to the slow clock mode, but it has a higher wakeup latency, and
needs specific software support in the bootloader and the kernel.

As a result, the "backup mode" expression is used to contrast with the
"slow clock" expression when describing the different suspend modes
supported by the chip.

But if you think that it is necessary, I can reword the commit.

Best regards,
-- 
Romain Izard

On 23/10/2017 at 19:03, Romain Izard wrote:

2017-10-23 18:07 GMT+02:00 Lee Jones [off-list ref]:

quoted

On Mon, 23 Oct 2017, Lee Jones wrote:

quoted

On Thu, 19 Oct 2017, Romain Izard wrote:

quoted

The controller used by a flexcom module is configured at boot, and left
alone after this. As the configuration will be lost after backup mode,
restore the state of the flexcom driver on resume.

Signed-off-by: Romain Izard <redacted>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changes in v5:
* extract from the patch series, and send as a standalone patch

 drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 064bde9cff5a..ef1235c4a179 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c

@@ -39,34 +39,44 @@
 #define FLEX_MR_OPMODE(opmode)     (((opmode) << FLEX_MR_OPMODE_OFFSET) &  \
                             FLEX_MR_OPMODE_MASK)

+struct atmel_flexcom {
+   void __iomem *base;
+   u32 opmode;
+   struct clk *clk;
+};

 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
    struct device_node *np = pdev->dev.of_node;
-   struct clk *clk;
    struct resource *res;
-   void __iomem *base;
-   u32 opmode;
+   struct atmel_flexcom *afc;

Nit: I'd prefer if you call this 'ddata'.

But the concept and implementation is fine, so if you're going to
change it please do so and apply my:

Acked-by: Lee Jones <redacted>

Also, 'back-up mode' isn't really a thing is it?

How about "Reinstall state on resume" or similar?

The expression comes from the SAMA5D2's datasheet.

Other Atmel chips use a different single suspend mode with Linux, where
the SoC remains completely powered with a slow clock. The registers are
preserved in this mode, so there is no need for a specific suspend and
resume code.

The SoC can also be powered down, but the CPU is reset and only a small
part is powered with a backup battery to maintain a valid RTC and a
small internal SRAM.

In the SAMA5D2, the mode with only the backup power supply has been
extended to isolate the memory I/O lines, making it possible to keep the
external SDRAM memory in self-refresh. This mode has a lower consumption
compared to the slow clock mode, but it has a higher wakeup latency, and
needs specific software support in the bootloader and the kernel.

As a result, the "backup mode" expression is used to contrast with the
"slow clock" expression when describing the different suspend modes
supported by the chip.

aka: Ultra Low Power modes (ULP0 and ULP1).

But if you think that it is necessary, I can reword the commit.

Thanks for the whole explanation Romain.

Yes we have such a wording in our documents and we used some kind of
"Backup mode", "Backup+Self-Refresh (B+SR or B+S-R)", "Backup and DDR in
Self-refresh" or "suspend-to-mem" wording for our patches. I take
advantage of this discussion to list all them here, for the record ;-)

So, I have the feeling that together with the commit message itself, we
can go with this wording.

Best regards,
-- 
Nicolas Ferre

On Mon, 23 Oct 2017, Romain Izard wrote:

2017-10-23 18:07 GMT+02:00 Lee Jones [off-list ref]:

quoted

On Mon, 23 Oct 2017, Lee Jones wrote:

quoted

On Thu, 19 Oct 2017, Romain Izard wrote:

quoted

The controller used by a flexcom module is configured at boot, and left
alone after this. As the configuration will be lost after backup mode,
restore the state of the flexcom driver on resume.

Signed-off-by: Romain Izard <redacted>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changes in v5:
* extract from the patch series, and send as a standalone patch

 drivers/mfd/atmel-flexcom.c | 65 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 064bde9cff5a..ef1235c4a179 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c

@@ -39,34 +39,44 @@
 #define FLEX_MR_OPMODE(opmode)     (((opmode) << FLEX_MR_OPMODE_OFFSET) &  \
                             FLEX_MR_OPMODE_MASK)

+struct atmel_flexcom {
+   void __iomem *base;
+   u32 opmode;
+   struct clk *clk;
+};

 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
    struct device_node *np = pdev->dev.of_node;
-   struct clk *clk;
    struct resource *res;
-   void __iomem *base;
-   u32 opmode;
+   struct atmel_flexcom *afc;

Nit: I'd prefer if you call this 'ddata'.

But the concept and implementation is fine, so if you're going to
change it please do so and apply my:

Acked-by: Lee Jones <redacted>

Also, 'back-up mode' isn't really a thing is it?

How about "Reinstall state on resume" or similar?

The expression comes from the SAMA5D2's datasheet.

Other Atmel chips use a different single suspend mode with Linux, where
the SoC remains completely powered with a slow clock. The registers are
preserved in this mode, so there is no need for a specific suspend and
resume code.

The SoC can also be powered down, but the CPU is reset and only a small
part is powered with a backup battery to maintain a valid RTC and a
small internal SRAM.

In the SAMA5D2, the mode with only the backup power supply has been
extended to isolate the memory I/O lines, making it possible to keep the
external SDRAM memory in self-refresh. This mode has a lower consumption
compared to the slow clock mode, but it has a higher wakeup latency, and
needs specific software support in the bootloader and the kernel.

As a result, the "backup mode" expression is used to contrast with the
"slow clock" expression when describing the different suspend modes
supported by the chip.

But if you think that it is necessary, I can reword the commit.

No, no need.  Thanks for the excellent explanation.

It might be worth you providing a succinct description of the
datasheet's meaning of "back-up mode" though.

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