[PATCH 0/4] power: reset: at91-poweroff: cleanups

[PATCH 0/4] power: reset: at91-poweroff: cleanups

[Claudiu.Beznea]

Hi,

This series includes cleanups for at91-poweroff.c similar to the one did for
SAMA5D2 Xplained SHDWC on series at [1].

Changes were tested on SAMA5D3 Xplained, SAMA5D4 Xplained and AT91SAM9G35-EK
boards.

Thank you,
Claudiu Beznea

[1] https://www.spinics.net/lists/arm-kernel/msg673559.html

Claudiu Beznea (4):
  power: reset: at91-poweroff: use one poweroff function for
    at91-poweroff
  power: reset: at91-poweroff: move shdwc related data to one structure
  power: reset: at91-poweroff: check shdwc data structure at the
    beginning of probe
  power: reset: at91-poweroff: remove at91_ramc_of_match

 drivers/power/reset/at91-poweroff.c | 101 +++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 42 deletions(-)

-- 
2.7.4

[PATCH 1/4] power: reset: at91-poweroff: use one poweroff function for at91-poweroff

[Claudiu.Beznea]

Use only one poweroff function and adapt it to work for both scenarios
(with LPDDR or not). The assignement of pm_power_off was moved at the
end of probe after all initializations are OK. This patch adapt the idea
from commit 4e018c1e9b05 ("power: reset: at91-poweroff: use only one
poweroff function").

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/power/reset/at91-poweroff.c | 49 ++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index fb2fc8f741a1..82533f4c72fc 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -76,11 +76,6 @@ static void __init at91_wakeup_status(struct platform_device *pdev)
 
 static void at91_poweroff(void)
 {
-	writel(AT91_SHDW_KEY | AT91_SHDW_SHDW, at91_shdwc_base + AT91_SHDW_CR);
-}
-
-static void at91_lpddr_poweroff(void)
-{
 	asm volatile(
 		/* Align to cache lines */
 		".balign 32\n\t"
@@ -89,9 +84,11 @@ static void at91_lpddr_poweroff(void)
 		"	ldr	r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
 
 		/* Power down SDRAM0 */
+		"	tst	%0, #0\n\t"
+		"	beq	1f\n\t"
 		"	str	%1, [%0, #" __stringify(AT91_DDRSDRC_LPR) "]\n\t"
 		/* Shutdown CPU */
-		"	str	%3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
+		"1:	str	%3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
 
 		"	b	.\n\t"
 		:
@@ -177,34 +174,42 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		at91_poweroff_dt_set_wakeup_mode(pdev);
 
-	pm_power_off = at91_poweroff;
-
 	np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc");
-	if (!np)
-		return 0;
+	if (np) {
+		mpddrc_base = of_iomap(np, 0);
+		of_node_put(np);
 
-	mpddrc_base = of_iomap(np, 0);
-	of_node_put(np);
+		if (!mpddrc_base) {
+			ret = -ENOMEM;
+			goto clk_disable;
+		}
 
-	if (!mpddrc_base)
-		return 0;
+		ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) &
+				 AT91_DDRSDRC_MD;
+		if (ddr_type != AT91_DDRSDRC_MD_LPDDR2 &&
+		    ddr_type != AT91_DDRSDRC_MD_LPDDR3) {
+			iounmap(mpddrc_base);
+			mpddrc_base = NULL;
+		}
+	}
 
-	ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD;
-	if ((ddr_type == AT91_DDRSDRC_MD_LPDDR2) ||
-	    (ddr_type == AT91_DDRSDRC_MD_LPDDR3))
-		pm_power_off = at91_lpddr_poweroff;
-	else
-		iounmap(mpddrc_base);
+	pm_power_off = at91_poweroff;
 
 	return 0;
+
+clk_disable:
+	clk_disable_unprepare(sclk);
+	return ret;
 }
 
 static int __exit at91_poweroff_remove(struct platform_device *pdev)
 {
-	if (pm_power_off == at91_poweroff ||
-	    pm_power_off == at91_lpddr_poweroff)
+	if (pm_power_off == at91_poweroff)
 		pm_power_off = NULL;
 
+	if (mpddrc_base)
+		iounmap(mpddrc_base);
+
 	clk_disable_unprepare(sclk);
 
 	return 0;
-- 
2.7.4

[PATCH 2/4] power: reset: at91-poweroff: move shdwc related data to one structure

[Claudiu.Beznea]

Move SHDWC realted data to only one structure to have them grouped.
Inspired from commit 9be74f0d39c1 ("power: reset: at91-poweroff: make
mpddrc_base part of struct shdwc").

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/power/reset/at91-poweroff.c | 60 +++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index 82533f4c72fc..48661e04a3de 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -51,14 +51,19 @@ static const char *shdwc_wakeup_modes[] = {
 	[AT91_SHDW_WKMODE0_ANYLEVEL]	= "any",
 };
 
-static void __iomem *at91_shdwc_base;
-static struct clk *sclk;
-static void __iomem *mpddrc_base;
+struct shdwc {
+	struct clk *sclk;
+	void __iomem *shdwc_base;
+	void __iomem *mpddrc_base;
+};
+
+static struct shdwc *at91_shdwc;
 
 static void __init at91_wakeup_status(struct platform_device *pdev)
 {
+	struct shdwc *shdwc = platform_get_drvdata(pdev);
 	const char *reason;
-	u32 reg = readl(at91_shdwc_base + AT91_SHDW_SR);
+	u32 reg = readl(shdwc->shdwc_base + AT91_SHDW_SR);
 
 	/* Simple power-on, just bail out */
 	if (!reg)
@@ -92,9 +97,9 @@ static void at91_poweroff(void)
 
 		"	b	.\n\t"
 		:
-		: "r" (mpddrc_base),
+		: "r" (at91_shdwc->mpddrc_base),
 		  "r" cpu_to_le32(AT91_DDRSDRC_LPDDR2_PWOFF),
-		  "r" (at91_shdwc_base),
+		  "r" (at91_shdwc->shdwc_base),
 		  "r" cpu_to_le32(AT91_SHDW_KEY | AT91_SHDW_SHDW)
 		: "r6");
 }
@@ -118,6 +123,7 @@ static int at91_poweroff_get_wakeup_mode(struct device_node *np)
 
 static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev)
 {
+	struct shdwc *shdwc = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
 	int wakeup_mode;
 	u32 mode = 0, tmp;
@@ -144,7 +150,7 @@ static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev)
 	if (of_property_read_bool(np, "atmel,wakeup-rtt-timer"))
 			mode |= AT91_SHDW_RTTWKEN;
 
-	writel(wakeup_mode | mode, at91_shdwc_base + AT91_SHDW_MR);
+	writel(wakeup_mode | mode, shdwc->shdwc_base + AT91_SHDW_MR);
 }
 
 static int __init at91_poweroff_probe(struct platform_device *pdev)
@@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
 	u32 ddr_type;
 	int ret;
 
+	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
+	if (!at91_shdwc)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, at91_shdwc);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	at91_shdwc_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(at91_shdwc_base))
-		return PTR_ERR(at91_shdwc_base);
+	at91_shdwc->shdwc_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(at91_shdwc->shdwc_base))
+		return PTR_ERR(at91_shdwc->shdwc_base);
 
-	sclk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(sclk))
-		return PTR_ERR(sclk);
+	at91_shdwc->sclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(at91_shdwc->sclk))
+		return PTR_ERR(at91_shdwc->sclk);
 
-	ret = clk_prepare_enable(sclk);
+	ret = clk_prepare_enable(at91_shdwc->sclk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not enable slow clock\n");
 		return ret;
@@ -176,20 +188,20 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
 
 	np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc");
 	if (np) {
-		mpddrc_base = of_iomap(np, 0);
+		at91_shdwc->mpddrc_base = of_iomap(np, 0);
 		of_node_put(np);
 
-		if (!mpddrc_base) {
+		if (!at91_shdwc->mpddrc_base) {
 			ret = -ENOMEM;
 			goto clk_disable;
 		}
 
-		ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) &
+		ddr_type = readl(at91_shdwc->mpddrc_base + AT91_DDRSDRC_MDR) &
 				 AT91_DDRSDRC_MD;
 		if (ddr_type != AT91_DDRSDRC_MD_LPDDR2 &&
 		    ddr_type != AT91_DDRSDRC_MD_LPDDR3) {
-			iounmap(mpddrc_base);
-			mpddrc_base = NULL;
+			iounmap(at91_shdwc->mpddrc_base);
+			at91_shdwc->mpddrc_base = NULL;
 		}
 	}
 
@@ -198,19 +210,21 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
 	return 0;
 
 clk_disable:
-	clk_disable_unprepare(sclk);
+	clk_disable_unprepare(at91_shdwc->sclk);
 	return ret;
 }
 
 static int __exit at91_poweroff_remove(struct platform_device *pdev)
 {
+	struct shdwc *shdwc = platform_get_drvdata(pdev);
+
 	if (pm_power_off == at91_poweroff)
 		pm_power_off = NULL;
 
-	if (mpddrc_base)
-		iounmap(mpddrc_base);
+	if (shdwc->mpddrc_base)
+		iounmap(shdwc->mpddrc_base);
 
-	clk_disable_unprepare(sclk);
+	clk_disable_unprepare(shdwc->sclk);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 3/4] power: reset: at91-poweroff: check shdwc data structure at the beginning of probe

[Claudiu.Beznea]

Check at91_shdwc before continuing with probe since we want only one instance of
this driver. Inspired from commit 9f1e44774be5 ("power: reset: at91-poweroff:
do not procede if at91_shdwc is allocated").

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/power/reset/at91-poweroff.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index 48661e04a3de..e75d8f0f0526 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -160,6 +160,9 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
 	u32 ddr_type;
 	int ret;
 
+	if (at91_shdwc)
+		return -EBUSY;
+
 	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
 	if (!at91_shdwc)
 		return -ENOMEM;
-- 
2.7.4

[PATCH 4/4] power: reset: at91-poweroff: remove at91_ramc_of_match

[Claudiu.Beznea]

Remove at91_ramc_of_match[] since it is not used anywhere in this code.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/power/reset/at91-poweroff.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index e75d8f0f0526..43b4cccb7d0c 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -232,11 +232,6 @@ static int __exit at91_poweroff_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id at91_ramc_of_match[] = {
-	{ .compatible = "atmel,sama5d3-ddramc", },
-	{ /* sentinel */ }
-};
-
 static const struct of_device_id at91_poweroff_of_match[] = {
 	{ .compatible = "atmel,at91sam9260-shdwc", },
 	{ .compatible = "atmel,at91sam9rl-shdwc", },
-- 
2.7.4

Re: [PATCH 2/4] power: reset: at91-poweroff: move shdwc related data to one structure

[Alexandre Belloni]

Hi Claudiu,

On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote:
>  static int __init at91_poweroff_probe(struct platform_device *pdev)
> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>  	u32 ddr_type;
>  	int ret;
>  
> +	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
> +	if (!at91_shdwc)
> +		return -ENOMEM;
> +

Is there any real benefit that will offset the time lost for that
allocation at boot time?

I understand you are then testing at91_shdwc to know whether the driver
already probed once. But, the driver will never probe twice as there is
only one shutdown controller on the SoC and anyway, If it was to probe
twice, it will still work as expected.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] power: reset: at91-poweroff: move shdwc related data to one structure

[Claudiu.Beznea]

Hi Alexandre,

On 06.11.2018 23:09, Alexandre Belloni wrote:
> Hi Claudiu,
> 
> On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote:
>>  static int __init at91_poweroff_probe(struct platform_device *pdev)
>> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>>  	u32 ddr_type;
>>  	int ret;
>>  
>> +	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
>> +	if (!at91_shdwc)
>> +		return -ENOMEM;
>> +
> 
> Is there any real benefit that will offset the time lost for that
> allocation at boot time?

No, I haven't run benchmarks on this. I only wanted to have them grouped in
one structure. Please let me know if you have some tests in mind.

> 
> I understand you are then testing at91_shdwc to know whether the driver
> already probed once. But, the driver will never probe twice as there is
> only one shutdown controller on the SoC and anyway, If it was to probe
> twice, it will still work as expected.

I had in mind the scenario where the driver would be compiled as module. I
know insmod already does this checking. I'm ok to remove this checking. I
will do it in next version. With this I will also remove devm_kzalloc() of
at91_shdwc.

Thank you,
Claudiu Beznea

> 
> 

Re: [PATCH 2/4] power: reset: at91-poweroff: move shdwc related data to one structure

[Alexandre Belloni]

On 07/11/2018 14:54:17+0000, Claudiu.Beznea@microchip.com wrote:
> Hi Alexandre,
> 
> On 06.11.2018 23:09, Alexandre Belloni wrote:
> > Hi Claudiu,
> > 
> > On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote:
> >>  static int __init at91_poweroff_probe(struct platform_device *pdev)
> >> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
> >>  	u32 ddr_type;
> >>  	int ret;
> >>  
> >> +	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
> >> +	if (!at91_shdwc)
> >> +		return -ENOMEM;
> >> +
> > 
> > Is there any real benefit that will offset the time lost for that
> > allocation at boot time?
> 
> No, I haven't run benchmarks on this. I only wanted to have them grouped in
> one structure. Please let me know if you have some tests in mind.
> 

Well, it is probably not much but small things adds up. Havinf it as a
global structure is probably good enough.

> > 
> > I understand you are then testing at91_shdwc to know whether the driver
> > already probed once. But, the driver will never probe twice as there is
> > only one shutdown controller on the SoC and anyway, If it was to probe
> > twice, it will still work as expected.
> 
> I had in mind the scenario where the driver would be compiled as module. I
> know insmod already does this checking. I'm ok to remove this checking. I
> will do it in next version. With this I will also remove devm_kzalloc() of
> at91_shdwc.
> 

Thanks,

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/4] power: reset: at91-poweroff: use one poweroff function for at91-poweroff

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

Hi,

On Mon, Nov 05, 2018 at 11:14:23AM +0000, Claudiu.Beznea@microchip.com wrote:
> Use only one poweroff function and adapt it to work for both scenarios
> (with LPDDR or not). The assignement of pm_power_off was moved at the
> end of probe after all initializations are OK. This patch adapt the idea
> from commit 4e018c1e9b05 ("power: reset: at91-poweroff: use only one
> poweroff function").
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Thanks, queued.

-- Sebastian

>  drivers/power/reset/at91-poweroff.c | 49 ++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
> index fb2fc8f741a1..82533f4c72fc 100644
> --- a/drivers/power/reset/at91-poweroff.c
> +++ b/drivers/power/reset/at91-poweroff.c
> @@ -76,11 +76,6 @@ static void __init at91_wakeup_status(struct platform_device *pdev)
>  
>  static void at91_poweroff(void)
>  {
> -	writel(AT91_SHDW_KEY | AT91_SHDW_SHDW, at91_shdwc_base + AT91_SHDW_CR);
> -}
> -
> -static void at91_lpddr_poweroff(void)
> -{
>  	asm volatile(
>  		/* Align to cache lines */
>  		".balign 32\n\t"
> @@ -89,9 +84,11 @@ static void at91_lpddr_poweroff(void)
>  		"	ldr	r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
>  
>  		/* Power down SDRAM0 */
> +		"	tst	%0, #0\n\t"
> +		"	beq	1f\n\t"
>  		"	str	%1, [%0, #" __stringify(AT91_DDRSDRC_LPR) "]\n\t"
>  		/* Shutdown CPU */
> -		"	str	%3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
> +		"1:	str	%3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
>  
>  		"	b	.\n\t"
>  		:
> @@ -177,34 +174,42 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>  	if (pdev->dev.of_node)
>  		at91_poweroff_dt_set_wakeup_mode(pdev);
>  
> -	pm_power_off = at91_poweroff;
> -
>  	np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc");
> -	if (!np)
> -		return 0;
> +	if (np) {
> +		mpddrc_base = of_iomap(np, 0);
> +		of_node_put(np);
>  
> -	mpddrc_base = of_iomap(np, 0);
> -	of_node_put(np);
> +		if (!mpddrc_base) {
> +			ret = -ENOMEM;
> +			goto clk_disable;
> +		}
>  
> -	if (!mpddrc_base)
> -		return 0;
> +		ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) &
> +				 AT91_DDRSDRC_MD;
> +		if (ddr_type != AT91_DDRSDRC_MD_LPDDR2 &&
> +		    ddr_type != AT91_DDRSDRC_MD_LPDDR3) {
> +			iounmap(mpddrc_base);
> +			mpddrc_base = NULL;
> +		}
> +	}
>  
> -	ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD;
> -	if ((ddr_type == AT91_DDRSDRC_MD_LPDDR2) ||
> -	    (ddr_type == AT91_DDRSDRC_MD_LPDDR3))
> -		pm_power_off = at91_lpddr_poweroff;
> -	else
> -		iounmap(mpddrc_base);
> +	pm_power_off = at91_poweroff;
>  
>  	return 0;
> +
> +clk_disable:
> +	clk_disable_unprepare(sclk);
> +	return ret;
>  }
>  
>  static int __exit at91_poweroff_remove(struct platform_device *pdev)
>  {
> -	if (pm_power_off == at91_poweroff ||
> -	    pm_power_off == at91_lpddr_poweroff)
> +	if (pm_power_off == at91_poweroff)
>  		pm_power_off = NULL;
>  
> +	if (mpddrc_base)
> +		iounmap(mpddrc_base);
> +
>  	clk_disable_unprepare(sclk);
>  
>  	return 0;
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] power: reset: at91-poweroff: remove at91_ramc_of_match

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

Hi,

On Mon, Nov 05, 2018 at 11:14:31AM +0000, Claudiu.Beznea@microchip.com wrote:
> Remove at91_ramc_of_match[] since it is not used anywhere in this code.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/reset/at91-poweroff.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
> index e75d8f0f0526..43b4cccb7d0c 100644
> --- a/drivers/power/reset/at91-poweroff.c
> +++ b/drivers/power/reset/at91-poweroff.c
> @@ -232,11 +232,6 @@ static int __exit at91_poweroff_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id at91_ramc_of_match[] = {
> -	{ .compatible = "atmel,sama5d3-ddramc", },
> -	{ /* sentinel */ }
> -};
> -
>  static const struct of_device_id at91_poweroff_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-shdwc", },
>  	{ .compatible = "atmel,at91sam9rl-shdwc", },
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] power: reset: at91-poweroff: check shdwc data structure at the beginning of probe

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

Hi,

On Mon, Nov 05, 2018 at 11:14:28AM +0000, Claudiu.Beznea@microchip.com wrote:
> Check at91_shdwc before continuing with probe since we want only one instance of
> this driver. Inspired from commit 9f1e44774be5 ("power: reset: at91-poweroff:
> do not procede if at91_shdwc is allocated").
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---

This depends on patch 2, so I skipped this one.

-- Sebastian

>  drivers/power/reset/at91-poweroff.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
> index 48661e04a3de..e75d8f0f0526 100644
> --- a/drivers/power/reset/at91-poweroff.c
> +++ b/drivers/power/reset/at91-poweroff.c
> @@ -160,6 +160,9 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>  	u32 ddr_type;
>  	int ret;
>  
> +	if (at91_shdwc)
> +		return -EBUSY;
> +
>  	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
>  	if (!at91_shdwc)
>  		return -ENOMEM;
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] power: reset: at91-poweroff: move shdwc related data to one structure

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

Hi,

On Wed, Nov 07, 2018 at 06:23:40PM +0100, Alexandre Belloni wrote:
> On 07/11/2018 14:54:17+0000, Claudiu.Beznea@microchip.com wrote:
> > Hi Alexandre,
> > 
> > On 06.11.2018 23:09, Alexandre Belloni wrote:
> > > Hi Claudiu,
> > > 
> > > On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote:
> > >>  static int __init at91_poweroff_probe(struct platform_device *pdev)
> > >> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
> > >>  	u32 ddr_type;
> > >>  	int ret;
> > >>  
> > >> +	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
> > >> +	if (!at91_shdwc)
> > >> +		return -ENOMEM;
> > >> +
> > > 
> > > Is there any real benefit that will offset the time lost for that
> > > allocation at boot time?
> > 
> > No, I haven't run benchmarks on this. I only wanted to have them grouped in
> > one structure. Please let me know if you have some tests in mind.
> > 
> 
> Well, it is probably not much but small things adds up. Having it as a
> global structure is probably good enough.

I suppose I will get a new patch with this change?

-- Sebastian

> 
> > > 
> > > I understand you are then testing at91_shdwc to know whether the driver
> > > already probed once. But, the driver will never probe twice as there is
> > > only one shutdown controller on the SoC and anyway, If it was to probe
> > > twice, it will still work as expected.
> > 
> > I had in mind the scenario where the driver would be compiled as module. I
> > know insmod already does this checking. I'm ok to remove this checking. I
> > will do it in next version. With this I will also remove devm_kzalloc() of
> > at91_shdwc.
> > 
> 
> Thanks,
> 
> -- 
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] power: reset: at91-poweroff: move shdwc related data to one structure

[Claudiu.Beznea]

Hi Sebastian,

On 06.12.2018 00:40, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Nov 07, 2018 at 06:23:40PM +0100, Alexandre Belloni wrote:
>> On 07/11/2018 14:54:17+0000, Claudiu.Beznea@microchip.com wrote:
>>> Hi Alexandre,
>>>
>>> On 06.11.2018 23:09, Alexandre Belloni wrote:
>>>> Hi Claudiu,
>>>>
>>>> On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote:
>>>>>  static int __init at91_poweroff_probe(struct platform_device *pdev)
>>>>> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>>>>>  	u32 ddr_type;
>>>>>  	int ret;
>>>>>  
>>>>> +	at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL);
>>>>> +	if (!at91_shdwc)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>
>>>> Is there any real benefit that will offset the time lost for that
>>>> allocation at boot time?
>>>
>>> No, I haven't run benchmarks on this. I only wanted to have them grouped in
>>> one structure. Please let me know if you have some tests in mind.
>>>
>>
>> Well, it is probably not much but small things adds up. Having it as a
>> global structure is probably good enough.
> 
> I suppose I will get a new patch with this change?

Yes, I will send a new version for this one.

Thank you,
Claudiu Beznea

> 
> -- Sebastian
> 
>>
>>>>
>>>> I understand you are then testing at91_shdwc to know whether the driver
>>>> already probed once. But, the driver will never probe twice as there is
>>>> only one shutdown controller on the SoC and anyway, If it was to probe
>>>> twice, it will still work as expected.
>>>
>>> I had in mind the scenario where the driver would be compiled as module. I
>>> know insmod already does this checking. I'm ok to remove this checking. I
>>> will do it in next version. With this I will also remove devm_kzalloc() of
>>> at91_shdwc.
>>>
>>
>> Thanks,
>>
>> -- 
>> Alexandre Belloni, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com