[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

As since sama5d3, to reset the chip, we don't need to shutdown the ddr
controller.

So add a new compatible string and new restart function for sama5d3 and
later chips. As we don't use sama5d3 ddr controller, so remove it as
well.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---

 drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index 36dc52f..8944b63 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
 	return NOTIFY_DONE;
 }
 
+static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
+			void *cmd)
+{
+	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
+				at91_rstc_base);
+	return NOTIFY_DONE;
+}
+
 static void __init at91_reset_status(struct platform_device *pdev)
 {
 	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
@@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
 static const struct of_device_id at91_ramc_of_match[] = {
 	{ .compatible = "atmel,at91sam9260-sdramc", },
 	{ .compatible = "atmel,at91sam9g45-ddramc", },
-	{ .compatible = "atmel,sama5d3-ddramc", },
 	{ /* sentinel */ }
 };
 
 static const struct of_device_id at91_reset_of_match[] = {
 	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
 	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
+	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
 	{ /* sentinel */ }
 };
 
@@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	for_each_matching_node(np, at91_ramc_of_match) {
-		at91_ramc_base[idx] = of_iomap(np, 0);
-		if (!at91_ramc_base[idx]) {
-			dev_err(&pdev->dev, "Could not map ram controller address\n");
-			return -ENODEV;
+	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
+	at91_restart_nb.notifier_call = match->data;
+
+	if (match->data != sama5d3_restart) {
+		/* we need to shutdown the ddr controller, so get ramc base */
+		for_each_matching_node(np, at91_ramc_of_match) {
+			at91_ramc_base[idx] = of_iomap(np, 0);
+			if (!at91_ramc_base[idx]) {
+				dev_err(&pdev->dev, "Could not map ram controller address\n");
+				return -ENODEV;
+			}
+			idx++;
 		}
-		idx++;
 	}
 
-	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
-	at91_restart_nb.notifier_call = match->data;
 	return register_restart_handler(&at91_restart_nb);
 }
 
-- 
1.9.1

[PATCH 2/2] ARM: at91: sama5/dt: update rstc to correct compatible string

[Josh Wu]

They'll use "atmel,sama5d3-rstc" for reset function.

Cc: devicetree at vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 arch/arm/boot/dts/sama5d3.dtsi | 2 +-
 arch/arm/boot/dts/sama5d4.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index 9e2444b..280255b 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -1259,7 +1259,7 @@
 			};
 
 			rstc at fffffe00 {
-				compatible = "atmel,at91sam9g45-rstc";
+				compatible = "atmel,sama5d3-rstc";
 				reg = <0xfffffe00 0x10>;
 			};
 
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 3ee22ee..481196c 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -1277,7 +1277,7 @@
 			};
 
 			rstc at fc068600 {
-				compatible = "atmel,at91sam9g45-rstc";
+				compatible = "atmel,sama5d3-rstc";
 				reg = <0xfc068600 0x10>;
 			};
 
-- 
1.9.1

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Maxime Ripard]

Hi,

On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
> controller.
> 
> So add a new compatible string and new restart function for sama5d3 and
> later chips. As we don't use sama5d3 ddr controller, so remove it as
> well.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> 
>  drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index 36dc52f..8944b63 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>  	return NOTIFY_DONE;
>  }
>  
> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
> +			void *cmd)
> +{
> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
> +				at91_rstc_base);
> +	return NOTIFY_DONE;
> +}
> +
>  static void __init at91_reset_status(struct platform_device *pdev)
>  {
>  	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>  static const struct of_device_id at91_ramc_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-sdramc", },
>  	{ .compatible = "atmel,at91sam9g45-ddramc", },
> -	{ .compatible = "atmel,sama5d3-ddramc", },
>  	{ /* sentinel */ }
>  };
>  
>  static const struct of_device_id at91_reset_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>  	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>  	{ /* sentinel */ }
>  };
>  
> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	for_each_matching_node(np, at91_ramc_of_match) {
> -		at91_ramc_base[idx] = of_iomap(np, 0);
> -		if (!at91_ramc_base[idx]) {
> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
> -			return -ENODEV;
> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> +	at91_restart_nb.notifier_call = match->data;
> +
> +	if (match->data != sama5d3_restart) {

Using of_device_is_compatible seems more appropriate.

Also, why are you changing the order of this loop and the notifier
registration?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150709/56e3802f/attachment.sig>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Nicolas Ferre]

Le 09/07/2015 14:03, Maxime Ripard a ?crit :
> Hi,
> 
> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
>> controller.
>>
>> So add a new compatible string and new restart function for sama5d3 and
>> later chips. As we don't use sama5d3 ddr controller, so remove it as
>> well.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>
>>  drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
>> index 36dc52f..8944b63 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>>  	return NOTIFY_DONE;
>>  }
>>  
>> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
>> +			void *cmd)
>> +{
>> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
>> +				at91_rstc_base);
>> +	return NOTIFY_DONE;
>> +}
>> +
>>  static void __init at91_reset_status(struct platform_device *pdev)
>>  {
>>  	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
>> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>>  static const struct of_device_id at91_ramc_of_match[] = {
>>  	{ .compatible = "atmel,at91sam9260-sdramc", },
>>  	{ .compatible = "atmel,at91sam9g45-ddramc", },
>> -	{ .compatible = "atmel,sama5d3-ddramc", },
>>  	{ /* sentinel */ }
>>  };
>>  
>>  static const struct of_device_id at91_reset_of_match[] = {
>>  	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>>  	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
>> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>>  	{ /* sentinel */ }
>>  };
>>  
>> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>>  		return -ENODEV;
>>  	}
>>  
>> -	for_each_matching_node(np, at91_ramc_of_match) {
>> -		at91_ramc_base[idx] = of_iomap(np, 0);
>> -		if (!at91_ramc_base[idx]) {
>> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
>> -			return -ENODEV;
>> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>> +	at91_restart_nb.notifier_call = match->data;
>> +
>> +	if (match->data != sama5d3_restart) {
> 
> Using of_device_is_compatible seems more appropriate.
> 
> Also, why are you changing the order of this loop and the notifier
> registration?

Well, it's because the sama5d3 onwards controllers don't need ramc
controller.
So, reverting the order seems needed. Doesn't it address your question,
or did I lost track?

Bye,
-- 
Nicolas Ferre

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Guenter Roeck]

On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
> controller.
> 
> So add a new compatible string and new restart function for sama5d3 and
> later chips. As we don't use sama5d3 ddr controller, so remove it as
> well.
> 
That sounds like it should be two separate patches, or am I missing something ?

Guenter

> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> 
>  drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index 36dc52f..8944b63 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>  	return NOTIFY_DONE;
>  }
>  
> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
> +			void *cmd)
> +{
> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
> +				at91_rstc_base);
> +	return NOTIFY_DONE;
> +}
> +
>  static void __init at91_reset_status(struct platform_device *pdev)
>  {
>  	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>  static const struct of_device_id at91_ramc_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-sdramc", },
>  	{ .compatible = "atmel,at91sam9g45-ddramc", },
> -	{ .compatible = "atmel,sama5d3-ddramc", },
>  	{ /* sentinel */ }
>  };
>  
>  static const struct of_device_id at91_reset_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>  	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>  	{ /* sentinel */ }
>  };
>  
> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	for_each_matching_node(np, at91_ramc_of_match) {
> -		at91_ramc_base[idx] = of_iomap(np, 0);
> -		if (!at91_ramc_base[idx]) {
> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
> -			return -ENODEV;
> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> +	at91_restart_nb.notifier_call = match->data;
> +
> +	if (match->data != sama5d3_restart) {
> +		/* we need to shutdown the ddr controller, so get ramc base */
> +		for_each_matching_node(np, at91_ramc_of_match) {
> +			at91_ramc_base[idx] = of_iomap(np, 0);
> +			if (!at91_ramc_base[idx]) {
> +				dev_err(&pdev->dev, "Could not map ram controller address\n");
> +				return -ENODEV;
> +			}
> +			idx++;
>  		}
> -		idx++;
>  	}
>  
> -	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> -	at91_restart_nb.notifier_call = match->data;
>  	return register_restart_handler(&at91_restart_nb);
>  }
>  
> -- 
> 1.9.1
> 

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

Hi, Guenter

On 7/10/2015 1:37 AM, Guenter Roeck wrote:
> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
>> controller.
>>
>> So add a new compatible string and new restart function for sama5d3 and
>> later chips. As we don't use sama5d3 ddr controller, so remove it as
>> well.
>>
> That sounds like it should be two separate patches, or am I missing something ?

I think using one patch makes more sense. Maybe the commit log is not 
clear enough. How about put it this way:

This patch introduces a new compatible string: "atmel,sama5d3-rstc" for 
the reset driver of sama5d3 and later chips.
As in sama5d3 or later chips, we don't have to shutdown the DDR 
controller before reset. Shutdown the DDR controller before reset is a 
workaround to avoid DDR signal driving the bus, but since sama5d3 and 
later chips there is no such a conflict.
That means:
   1. the sama5d3 reset function only need to write the rstc register 
and return.
   2. for sama5d3, we can remove the code related with DDR controller as 
we don't use it at all.

Best Regards,
Josh Wu
>
> Guenter
>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>
>>   drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
>> index 36dc52f..8944b63 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>>   	return NOTIFY_DONE;
>>   }
>>   
>> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
>> +			void *cmd)
>> +{
>> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
>> +				at91_rstc_base);
>> +	return NOTIFY_DONE;
>> +}
>> +
>>   static void __init at91_reset_status(struct platform_device *pdev)
>>   {
>>   	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
>> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>>   static const struct of_device_id at91_ramc_of_match[] = {
>>   	{ .compatible = "atmel,at91sam9260-sdramc", },
>>   	{ .compatible = "atmel,at91sam9g45-ddramc", },
>> -	{ .compatible = "atmel,sama5d3-ddramc", },
>>   	{ /* sentinel */ }
>>   };
>>   
>>   static const struct of_device_id at91_reset_of_match[] = {
>>   	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>>   	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
>> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>>   	{ /* sentinel */ }
>>   };
>>   
>> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>>   		return -ENODEV;
>>   	}
>>   
>> -	for_each_matching_node(np, at91_ramc_of_match) {
>> -		at91_ramc_base[idx] = of_iomap(np, 0);
>> -		if (!at91_ramc_base[idx]) {
>> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
>> -			return -ENODEV;
>> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>> +	at91_restart_nb.notifier_call = match->data;
>> +
>> +	if (match->data != sama5d3_restart) {
>> +		/* we need to shutdown the ddr controller, so get ramc base */
>> +		for_each_matching_node(np, at91_ramc_of_match) {
>> +			at91_ramc_base[idx] = of_iomap(np, 0);
>> +			if (!at91_ramc_base[idx]) {
>> +				dev_err(&pdev->dev, "Could not map ram controller address\n");
>> +				return -ENODEV;
>> +			}
>> +			idx++;
>>   		}
>> -		idx++;
>>   	}
>>   
>> -	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>> -	at91_restart_nb.notifier_call = match->data;
>>   	return register_restart_handler(&at91_restart_nb);
>>   }
>>   
>> -- 
>> 1.9.1
>>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

Hi, Maxime

On 7/9/2015 8:03 PM, Maxime Ripard wrote:
> Hi,
>
> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
>> controller.
>>
>> So add a new compatible string and new restart function for sama5d3 and
>> later chips. As we don't use sama5d3 ddr controller, so remove it as
>> well.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>
>>   drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
>> index 36dc52f..8944b63 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>>   	return NOTIFY_DONE;
>>   }
>>   
>> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
>> +			void *cmd)
>> +{
>> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
>> +				at91_rstc_base);
>> +	return NOTIFY_DONE;
>> +}
>> +
>>   static void __init at91_reset_status(struct platform_device *pdev)
>>   {
>>   	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
>> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>>   static const struct of_device_id at91_ramc_of_match[] = {
>>   	{ .compatible = "atmel,at91sam9260-sdramc", },
>>   	{ .compatible = "atmel,at91sam9g45-ddramc", },
>> -	{ .compatible = "atmel,sama5d3-ddramc", },
>>   	{ /* sentinel */ }
>>   };
>>   
>>   static const struct of_device_id at91_reset_of_match[] = {
>>   	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>>   	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
>> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>>   	{ /* sentinel */ }
>>   };
>>   
>> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>>   		return -ENODEV;
>>   	}
>>   
>> -	for_each_matching_node(np, at91_ramc_of_match) {
>> -		at91_ramc_base[idx] = of_iomap(np, 0);
>> -		if (!at91_ramc_base[idx]) {
>> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
>> -			return -ENODEV;
>> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>> +	at91_restart_nb.notifier_call = match->data;
>> +
>> +	if (match->data != sama5d3_restart) {
> Using of_device_is_compatible seems more appropriate.
>
> Also, why are you changing the order of this loop and the notifier
> registration?

I moved this order because I use the match->data to compare whether is 
sama5d3_restart. So I need to move this function (of_match_node) up.

Best Regards,
Josh Wu

>
> Maxime
>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Guenter Roeck]

On Fri, Jul 10, 2015 at 09:59:53AM +0800, Josh Wu wrote:
> Hi, Guenter
> 
> On 7/10/2015 1:37 AM, Guenter Roeck wrote:
> >On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
> >>As since sama5d3, to reset the chip, we don't need to shutdown the ddr
> >>controller.
> >>
> >>So add a new compatible string and new restart function for sama5d3 and
> >>later chips. As we don't use sama5d3 ddr controller, so remove it as
> >>well.
> >>
> >That sounds like it should be two separate patches, or am I missing something ?
> 
> I think using one patch makes more sense. Maybe the commit log is not clear
> enough. How about put it this way:
> 
> This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the
> reset driver of sama5d3 and later chips.
> As in sama5d3 or later chips, we don't have to shutdown the DDR controller
> before reset. Shutdown the DDR controller before reset is a workaround to
> avoid DDR signal driving the bus, but since sama5d3 and later chips there is
> no such a conflict.
> That means:
>   1. the sama5d3 reset function only need to write the rstc register and
> return.
>   2. for sama5d3, we can remove the code related with DDR controller as we
> don't use it at all.
> 
Sorry, I don't get it. Doesn't that mean there are two distinct logical
changes, which would ask for two separate patches ?

Guenter

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

Hi, Guenter

On 7/10/2015 11:14 AM, Guenter Roeck wrote:
> On Fri, Jul 10, 2015 at 09:59:53AM +0800, Josh Wu wrote:
>> Hi, Guenter
>>
>> On 7/10/2015 1:37 AM, Guenter Roeck wrote:
>>> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
>>>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
>>>> controller.
>>>>
>>>> So add a new compatible string and new restart function for sama5d3 and
>>>> later chips. As we don't use sama5d3 ddr controller, so remove it as
>>>> well.
>>>>
>>> That sounds like it should be two separate patches, or am I missing something ?
>> I think using one patch makes more sense. Maybe the commit log is not clear
>> enough. How about put it this way:
>>
>> This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the
>> reset driver of sama5d3 and later chips.
>> As in sama5d3 or later chips, we don't have to shutdown the DDR controller
>> before reset. Shutdown the DDR controller before reset is a workaround to
>> avoid DDR signal driving the bus, but since sama5d3 and later chips there is
>> no such a conflict.
>> That means:
>>    1. the sama5d3 reset function only need to write the rstc register and
>> return.
>>    2. for sama5d3, we can remove the code related with DDR controller as we
>> don't use it at all.
>>
> Sorry, I don't get it. Doesn't that mean there are two distinct logical
> changes, which would ask for two separate patches ?

The rewritten reset function for sama5d3 has no need to access the ddr 
controller, so this patch rewrite the reset function and remove ddr 
access for sama5d3.
Those two changes are related and so make it as one patch is reasonable.

Best Regards,
Josh Wu
>
> Guenter

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Alexandre Belloni]

Hi Guenter,

On 09/07/2015 at 20:14:38 -0700, Guenter Roeck wrote :
> > This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the
> > reset driver of sama5d3 and later chips.
> > As in sama5d3 or later chips, we don't have to shutdown the DDR controller
> > before reset. Shutdown the DDR controller before reset is a workaround to
> > avoid DDR signal driving the bus, but since sama5d3 and later chips there is
> > no such a conflict.
> > That means:
> >   1. the sama5d3 reset function only need to write the rstc register and
> > return.
> >   2. for sama5d3, we can remove the code related with DDR controller as we
> > don't use it at all.
> > 
> Sorry, I don't get it. Doesn't that mean there are two distinct logical
> changes, which would ask for two separate patches ?

I would agree with Josh and I think that only one patch is needed. There
is only one change, the removal of the workaround for sama5d3 and later.

As the workaround is using a table of compatibles to remap the ram
controller and the one for sama5d3 is not used because it is not needed,
I think it makes sense to remove it in that same patch. The logical
change here is the removal of the workaround.

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

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Alexandre Belloni]

Hi,

On 09/07/2015 at 18:15:46 +0800, Josh Wu wrote :
> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
> controller.
> 
> So add a new compatible string and new restart function for sama5d3 and
> later chips. As we don't use sama5d3 ddr controller, so remove it as
> well.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> 
>  drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index 36dc52f..8944b63 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>  	return NOTIFY_DONE;
>  }
>  
> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
> +			void *cmd)

Please align that line properly.

> +{
> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
> +				at91_rstc_base);

That one too.

> +	return NOTIFY_DONE;
> +}
> +
>  static void __init at91_reset_status(struct platform_device *pdev)
>  {
>  	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>  static const struct of_device_id at91_ramc_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-sdramc", },
>  	{ .compatible = "atmel,at91sam9g45-ddramc", },
> -	{ .compatible = "atmel,sama5d3-ddramc", },
>  	{ /* sentinel */ }
>  };
>  
>  static const struct of_device_id at91_reset_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>  	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>  	{ /* sentinel */ }
>  };
>  
> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	for_each_matching_node(np, at91_ramc_of_match) {
> -		at91_ramc_base[idx] = of_iomap(np, 0);
> -		if (!at91_ramc_base[idx]) {
> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
> -			return -ENODEV;
> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> +	at91_restart_nb.notifier_call = match->data;
> +
> +	if (match->data != sama5d3_restart) {

This doesn't scale well. I would create a structure with a pointer to
the restart function and a boolean or a bitfield to store whether the
workaround is needed. Use that structure in your match data. Then, you
won't need to reorder anything.

> +		/* we need to shutdown the ddr controller, so get ramc base */
> +		for_each_matching_node(np, at91_ramc_of_match) {
> +			at91_ramc_base[idx] = of_iomap(np, 0);
> +			if (!at91_ramc_base[idx]) {
> +				dev_err(&pdev->dev, "Could not map ram controller address\n");
> +				return -ENODEV;
> +			}
> +			idx++;
>  		}
> -		idx++;
>  	}
>  
> -	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> -	at91_restart_nb.notifier_call = match->data;
>  	return register_restart_handler(&at91_restart_nb);
>  }

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

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Maxime Ripard]

On Fri, Jul 10, 2015 at 11:06:52AM +0800, Josh Wu wrote:
> Hi, Maxime
> 
> On 7/9/2015 8:03 PM, Maxime Ripard wrote:
> >Hi,
> >
> >On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
> >>As since sama5d3, to reset the chip, we don't need to shutdown the ddr
> >>controller.
> >>
> >>So add a new compatible string and new restart function for sama5d3 and
> >>later chips. As we don't use sama5d3 ddr controller, so remove it as
> >>well.
> >>
> >>Signed-off-by: Josh Wu <josh.wu@atmel.com>
> >>Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> >>---
> >>
> >>  drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
> >>  1 file changed, 21 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> >>index 36dc52f..8944b63 100644
> >>--- a/drivers/power/reset/at91-reset.c
> >>+++ b/drivers/power/reset/at91-reset.c
> >>@@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
> >>  	return NOTIFY_DONE;
> >>  }
> >>+static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
> >>+			void *cmd)
> >>+{
> >>+	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
> >>+				at91_rstc_base);
> >>+	return NOTIFY_DONE;
> >>+}
> >>+
> >>  static void __init at91_reset_status(struct platform_device *pdev)
> >>  {
> >>  	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
> >>@@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
> >>  static const struct of_device_id at91_ramc_of_match[] = {
> >>  	{ .compatible = "atmel,at91sam9260-sdramc", },
> >>  	{ .compatible = "atmel,at91sam9g45-ddramc", },
> >>-	{ .compatible = "atmel,sama5d3-ddramc", },
> >>  	{ /* sentinel */ }
> >>  };
> >>  static const struct of_device_id at91_reset_of_match[] = {
> >>  	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
> >>  	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> >>+	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
> >>  	{ /* sentinel */ }
> >>  };
> >>@@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
> >>  		return -ENODEV;
> >>  	}
> >>-	for_each_matching_node(np, at91_ramc_of_match) {
> >>-		at91_ramc_base[idx] = of_iomap(np, 0);
> >>-		if (!at91_ramc_base[idx]) {
> >>-			dev_err(&pdev->dev, "Could not map ram controller address\n");
> >>-			return -ENODEV;
> >>+	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> >>+	at91_restart_nb.notifier_call = match->data;
> >>+
> >>+	if (match->data != sama5d3_restart) {
> >Using of_device_is_compatible seems more appropriate.
> >
> >Also, why are you changing the order of this loop and the notifier
> >registration?
> 
> I moved this order because I use the match->data to compare whether is
> sama5d3_restart. So I need to move this function (of_match_node) up.

Ah right, my bad.

Still, testing against the kernel pointer is not that great.

It would be great to use something explicit instead, like
of_device_is_compatible.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150710/2456d567/attachment.sig>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Maxime Ripard]

On Fri, Jul 10, 2015 at 08:03:50AM +0200, Alexandre Belloni wrote:
> >  static const struct of_device_id at91_reset_of_match[] = {
> >  	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
> >  	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> > +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
> >  	{ /* sentinel */ }
> >  };
> >  
> > @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  	}
> >  
> > -	for_each_matching_node(np, at91_ramc_of_match) {
> > -		at91_ramc_base[idx] = of_iomap(np, 0);
> > -		if (!at91_ramc_base[idx]) {
> > -			dev_err(&pdev->dev, "Could not map ram controller address\n");
> > -			return -ENODEV;
> > +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> > +	at91_restart_nb.notifier_call = match->data;
> > +
> > +	if (match->data != sama5d3_restart) {
> 
> This doesn't scale well. I would create a structure with a pointer to
> the restart function and a boolean or a bitfield to store whether the
> workaround is needed. Use that structure in your match data. Then, you
> won't need to reorder anything.

Maybe it simply doesn't need to scale (yet).

You have a single exception here. Maybe you will have only this one in
the future, maybe you won't, but for now, that solution looks a bit
overkill.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150710/b2174e07/attachment.sig>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

Hi, Alexandre

On 7/10/2015 2:03 PM, Alexandre Belloni wrote:
> Hi,
>
> On 09/07/2015 at 18:15:46 +0800, Josh Wu wrote :
>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
>> controller.
>>
>> So add a new compatible string and new restart function for sama5d3 and
>> later chips. As we don't use sama5d3 ddr controller, so remove it as
>> well.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>
>>   drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
>> index 36dc52f..8944b63 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>>   	return NOTIFY_DONE;
>>   }
>>   
>> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
>> +			void *cmd)
> Please align that line properly.

Ok.

>
>> +{
>> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
>> +				at91_rstc_base);
> That one too.

I'll align them in v2.

>
>> +	return NOTIFY_DONE;
>> +}
>> +
>>   static void __init at91_reset_status(struct platform_device *pdev)
>>   {
>>   	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
>> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>>   static const struct of_device_id at91_ramc_of_match[] = {
>>   	{ .compatible = "atmel,at91sam9260-sdramc", },
>>   	{ .compatible = "atmel,at91sam9g45-ddramc", },
>> -	{ .compatible = "atmel,sama5d3-ddramc", },
>>   	{ /* sentinel */ }
>>   };
>>   
>>   static const struct of_device_id at91_reset_of_match[] = {
>>   	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>>   	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
>> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>>   	{ /* sentinel */ }
>>   };
>>   
>> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>>   		return -ENODEV;
>>   	}
>>   
>> -	for_each_matching_node(np, at91_ramc_of_match) {
>> -		at91_ramc_base[idx] = of_iomap(np, 0);
>> -		if (!at91_ramc_base[idx]) {
>> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
>> -			return -ENODEV;
>> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>> +	at91_restart_nb.notifier_call = match->data;
>> +
>> +	if (match->data != sama5d3_restart) {
> This doesn't scale well. I would create a structure with a pointer to
> the restart function and a boolean or a bitfield to store whether the
> workaround is needed. Use that structure in your match data. Then, you
> won't need to reorder anything.

I would agree with Maxime. Currently all latest chip reset function is 
compatible with the atmel,sama5d3-rstc.
So check compatible string is enough for now.
But of cause if we have other incompatible reset in future with new 
chip, the structure like you said is needed.

Thanks.
Best Regards,
Josh Wu
>
>> +		/* we need to shutdown the ddr controller, so get ramc base */
>> +		for_each_matching_node(np, at91_ramc_of_match) {
>> +			at91_ramc_base[idx] = of_iomap(np, 0);
>> +			if (!at91_ramc_base[idx]) {
>> +				dev_err(&pdev->dev, "Could not map ram controller address\n");
>> +				return -ENODEV;
>> +			}
>> +			idx++;
>>   		}
>> -		idx++;
>>   	}
>>   
>> -	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>> -	at91_restart_nb.notifier_call = match->data;
>>   	return register_restart_handler(&at91_restart_nb);
>>   }

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

On 7/10/2015 2:54 PM, Maxime Ripard wrote:
> On Fri, Jul 10, 2015 at 11:06:52AM +0800, Josh Wu wrote:
>> Hi, Maxime
>>
>> On 7/9/2015 8:03 PM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote:
>>>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr
>>>> controller.
>>>>
>>>> So add a new compatible string and new restart function for sama5d3 and
>>>> later chips. As we don't use sama5d3 ddr controller, so remove it as
>>>> well.
>>>>
>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>> ---
>>>>
>>>>   drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++---------
>>>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
>>>> index 36dc52f..8944b63 100644
>>>> --- a/drivers/power/reset/at91-reset.c
>>>> +++ b/drivers/power/reset/at91-reset.c
>>>> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode,
>>>>   	return NOTIFY_DONE;
>>>>   }
>>>> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
>>>> +			void *cmd)
>>>> +{
>>>> +	writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST),
>>>> +				at91_rstc_base);
>>>> +	return NOTIFY_DONE;
>>>> +}
>>>> +
>>>>   static void __init at91_reset_status(struct platform_device *pdev)
>>>>   {
>>>>   	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
>>>> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev)
>>>>   static const struct of_device_id at91_ramc_of_match[] = {
>>>>   	{ .compatible = "atmel,at91sam9260-sdramc", },
>>>>   	{ .compatible = "atmel,at91sam9g45-ddramc", },
>>>> -	{ .compatible = "atmel,sama5d3-ddramc", },
>>>>   	{ /* sentinel */ }
>>>>   };
>>>>   static const struct of_device_id at91_reset_of_match[] = {
>>>>   	{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
>>>>   	{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
>>>> +	{ .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
>>>>   	{ /* sentinel */ }
>>>>   };
>>>> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev)
>>>>   		return -ENODEV;
>>>>   	}
>>>> -	for_each_matching_node(np, at91_ramc_of_match) {
>>>> -		at91_ramc_base[idx] = of_iomap(np, 0);
>>>> -		if (!at91_ramc_base[idx]) {
>>>> -			dev_err(&pdev->dev, "Could not map ram controller address\n");
>>>> -			return -ENODEV;
>>>> +	match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
>>>> +	at91_restart_nb.notifier_call = match->data;
>>>> +
>>>> +	if (match->data != sama5d3_restart) {
>>> Using of_device_is_compatible seems more appropriate.
>>>
>>> Also, why are you changing the order of this loop and the notifier
>>> registration?
>> I moved this order because I use the match->data to compare whether is
>> sama5d3_restart. So I need to move this function (of_match_node) up.
> Ah right, my bad.
>
> Still, testing against the kernel pointer is not that great.
>
> It would be great to use something explicit instead, like
> of_device_is_compatible.

I agree. I will use of_device_is_compatible() in v2. And that can avoid 
the order change in the loop as well. Thanks.

Best Regards,
Josh Wu

>
> Maxime
>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Alexandre Belloni]

Hi,

On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
> I would agree with Maxime. Currently all latest chip reset function is
> compatible with the atmel,sama5d3-rstc.
> So check compatible string is enough for now.
> But of cause if we have other incompatible reset in future with new chip,
> the structure like you said is needed.

We managed to avoid using of_machine_is_compatible() in all the at91
drivers. I'd like to keep it that way. It was painful enough to remove
all those cpu_is_at91xxx calls.
Also, using it is trying to match strings and will result in longer boot
times.

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

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Maxime Ripard]

On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
> > I would agree with Maxime. Currently all latest chip reset function is
> > compatible with the atmel,sama5d3-rstc.
> > So check compatible string is enough for now.
> > But of cause if we have other incompatible reset in future with new chip,
> > the structure like you said is needed.
> 
> We managed to avoid using of_machine_is_compatible() in all the at91
> drivers. I'd like to keep it that way. It was painful enough to remove
> all those cpu_is_at91xxx calls.

That's your call...

> Also, using it is trying to match strings and will result in longer boot
> times.

Have you looked at the implementation of of_match_device? If that's
really a concern to you, you should actually avoid it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150710/7998893a/attachment.sig>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Alexandre Belloni]

On 10/07/2015 at 14:31:48 +0200, Maxime Ripard wrote :
> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
> > Hi,
> > 
> > On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
> > > I would agree with Maxime. Currently all latest chip reset function is
> > > compatible with the atmel,sama5d3-rstc.
> > > So check compatible string is enough for now.
> > > But of cause if we have other incompatible reset in future with new chip,
> > > the structure like you said is needed.
> > 
> > We managed to avoid using of_machine_is_compatible() in all the at91
> > drivers. I'd like to keep it that way. It was painful enough to remove
> > all those cpu_is_at91xxx calls.
> 
> That's your call...
> 
> > Also, using it is trying to match strings and will result in longer boot
> > times.
> 
> Have you looked at the implementation of of_match_device? If that's
> really a concern to you, you should actually avoid it.
> 

Indeed, I misread. of_device_is_compatible is acceptable,
of_machine_is_compatible is not :)


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

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Nicolas Ferre]

Le 10/07/2015 14:31, Maxime Ripard a ?crit :
> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
>> Hi,
>>
>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
>>> I would agree with Maxime. Currently all latest chip reset function is
>>> compatible with the atmel,sama5d3-rstc.
>>> So check compatible string is enough for now.
>>> But of cause if we have other incompatible reset in future with new chip,
>>> the structure like you said is needed.
>>
>> We managed to avoid using of_machine_is_compatible() in all the at91
>> drivers. I'd like to keep it that way. It was painful enough to remove
>> all those cpu_is_at91xxx calls.
> 
> That's your call...
> 
>> Also, using it is trying to match strings and will result in longer boot
>> times.
> 
> Have you looked at the implementation of of_match_device? If that's
> really a concern to you, you should actually avoid it.

I agree: let's keep it simple and use of_match_device().

Bye,
-- 
Nicolas Ferre

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Guenter]

On Fri, Jul 10, 2015 at 07:56:58AM +0200, Alexandre Belloni wrote:
> Hi Guenter,
> 
> On 09/07/2015 at 20:14:38 -0700, Guenter Roeck wrote :
> > > This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the
> > > reset driver of sama5d3 and later chips.
> > > As in sama5d3 or later chips, we don't have to shutdown the DDR controller
> > > before reset. Shutdown the DDR controller before reset is a workaround to
> > > avoid DDR signal driving the bus, but since sama5d3 and later chips there is
> > > no such a conflict.
> > > That means:
> > >   1. the sama5d3 reset function only need to write the rstc register and
> > > return.
> > >   2. for sama5d3, we can remove the code related with DDR controller as we
> > > don't use it at all.
> > > 
> > Sorry, I don't get it. Doesn't that mean there are two distinct logical
> > changes, which would ask for two separate patches ?
> 
> I would agree with Josh and I think that only one patch is needed. There
> is only one change, the removal of the workaround for sama5d3 and later.
> 
> As the workaround is using a table of compatibles to remap the ram
> controller and the one for sama5d3 is not used because it is not needed,
> I think it makes sense to remove it in that same patch. The logical
> change here is the removal of the workaround.
> 
Ok, makes sense.

Thanks,
Guenter

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
> Le 10/07/2015 14:31, Maxime Ripard a ?crit :
>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
>>> Hi,
>>>
>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
>>>> I would agree with Maxime. Currently all latest chip reset function is
>>>> compatible with the atmel,sama5d3-rstc.
>>>> So check compatible string is enough for now.
>>>> But of cause if we have other incompatible reset in future with new chip,
>>>> the structure like you said is needed.
>>> We managed to avoid using of_machine_is_compatible() in all the at91
>>> drivers. I'd like to keep it that way. It was painful enough to remove
>>> all those cpu_is_at91xxx calls.
>> That's your call...
>>
>>> Also, using it is trying to match strings and will result in longer boot
>>> times.
>> Have you looked at the implementation of of_match_device? If that's
>> really a concern to you, you should actually avoid it.
> I agree: let's keep it simple and use of_match_device().

Ok. I will keep it as it is now:  use the (match->data != 
sama5d3_restart) for the condition.

About the of_match_device(), I prefer to keep not changing the code and 
still use of_match_node().
Since of_match_device() is a wrapper for the of_match_node(). And 
dev->of_node and at91_reset_of_match is valid, so we can just use 
of_match_node() directly.

Is it sound okay for us?

Best Regards,
Josh Wu

>
> Bye,

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Maxime Ripard]

Hi Josh,

On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote:
> On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
> >Le 10/07/2015 14:31, Maxime Ripard a ?crit :
> >>On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
> >>>Hi,
> >>>
> >>>On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
> >>>>I would agree with Maxime. Currently all latest chip reset function is
> >>>>compatible with the atmel,sama5d3-rstc.
> >>>>So check compatible string is enough for now.
> >>>>But of cause if we have other incompatible reset in future with new chip,
> >>>>the structure like you said is needed.
> >>>We managed to avoid using of_machine_is_compatible() in all the at91
> >>>drivers. I'd like to keep it that way. It was painful enough to remove
> >>>all those cpu_is_at91xxx calls.
> >>That's your call...
> >>
> >>>Also, using it is trying to match strings and will result in longer boot
> >>>times.
> >>Have you looked at the implementation of of_match_device? If that's
> >>really a concern to you, you should actually avoid it.
> >I agree: let's keep it simple and use of_match_device().
> 
> Ok. I will keep it as it is now:  use the (match->data != sama5d3_restart)
> for the condition.

I'm not just that's been an option in our discussion so far.

Nicolas said that he was agreeing with me, but at the same time said
the complete opposite of what I was arguing for, so I'm not really
sure what's really on his mind, but the two options that were
discussed were to remove that test, and either:

  - Use of_device_is_compatible to prevent the loop execution

  - define a structure with a flag to say whether you need the ram
    controller quirk or not, and test that flag.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/7417eb15/attachment.sig>

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

Hi, Maxime

On 7/20/2015 3:52 PM, Maxime Ripard wrote:
> Hi Josh,
>
> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote:
>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
>>> Le 10/07/2015 14:31, Maxime Ripard a ?crit :
>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
>>>>>> I would agree with Maxime. Currently all latest chip reset function is
>>>>>> compatible with the atmel,sama5d3-rstc.
>>>>>> So check compatible string is enough for now.
>>>>>> But of cause if we have other incompatible reset in future with new chip,
>>>>>> the structure like you said is needed.
>>>>> We managed to avoid using of_machine_is_compatible() in all the at91
>>>>> drivers. I'd like to keep it that way. It was painful enough to remove
>>>>> all those cpu_is_at91xxx calls.
>>>> That's your call...
>>>>
>>>>> Also, using it is trying to match strings and will result in longer boot
>>>>> times.
>>>> Have you looked at the implementation of of_match_device? If that's
>>>> really a concern to you, you should actually avoid it.
>>> I agree: let's keep it simple and use of_match_device().
>> Ok. I will keep it as it is now:  use the (match->data != sama5d3_restart)
>> for the condition.
> I'm not just that's been an option in our discussion so far.
>
> Nicolas said that he was agreeing with me, but at the same time said
> the complete opposite of what I was arguing for, so I'm not really
> sure what's really on his mind, but the two options that were
> discussed were to remove that test, and either:
>
>    - Use of_device_is_compatible to prevent the loop execution

Thank you for explaining, it is clear to me.

I'll take this above option. As the of_device_is_compatible() almost 
same as of_match_node()/of_match_device(). Except that 
of_device_is_compatible() is more efficient (in this case It calls 
__of_device_is_compatible() directly) than of_match_node/of_match_device.

>
>    - define a structure with a flag to say whether you need the ram
>      controller quirk or not, and test that flag.
>
> Maxime
>

Best Regards,
Josh Wu

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Nicolas Ferre]

Le 20/07/2015 10:35, Josh Wu a ?crit :
> Hi, Maxime
> 
> On 7/20/2015 3:52 PM, Maxime Ripard wrote:
>> Hi Josh,
>>
>> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote:
>>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
>>>> Le 10/07/2015 14:31, Maxime Ripard a ?crit :
>>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
>>>>>>> I would agree with Maxime. Currently all latest chip reset function is
>>>>>>> compatible with the atmel,sama5d3-rstc.
>>>>>>> So check compatible string is enough for now.
>>>>>>> But of cause if we have other incompatible reset in future with new chip,
>>>>>>> the structure like you said is needed.
>>>>>> We managed to avoid using of_machine_is_compatible() in all the at91
>>>>>> drivers. I'd like to keep it that way. It was painful enough to remove
>>>>>> all those cpu_is_at91xxx calls.
>>>>> That's your call...
>>>>>
>>>>>> Also, using it is trying to match strings and will result in longer boot
>>>>>> times.
>>>>> Have you looked at the implementation of of_match_device? If that's
>>>>> really a concern to you, you should actually avoid it.
>>>> I agree: let's keep it simple and use of_match_device().
>>> Ok. I will keep it as it is now:  use the (match->data != sama5d3_restart)
>>> for the condition.
>> I'm not just that's been an option in our discussion so far.
>>
>> Nicolas said that he was agreeing with me, but at the same time said
>> the complete opposite of what I was arguing for, so I'm not really
>> sure what's really on his mind, but the two options that were
>> discussed were to remove that test, and either:
>>
>>    - Use of_device_is_compatible to prevent the loop execution
> 
> Thank you for explaining, it is clear to me.
> 
> I'll take this above option. As the of_device_is_compatible() almost 
> same as of_match_node()/of_match_device(). Except that 
> of_device_is_compatible() is more efficient (in this case It calls 
> __of_device_is_compatible() directly) than of_match_node/of_match_device.

Yes, I was pushing for this solution...


>>    - define a structure with a flag to say whether you need the ram
>>      controller quirk or not, and test that flag.

and not for this one, that's all.

I wrongly added the name of the improper function to use too quickly
picked from your discussion with Alex.

So, all is clear now.

Bye,
-- 
Nicolas Ferre

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

On 7/20/2015 4:35 PM, Josh Wu wrote:
> Hi, Maxime
>
> On 7/20/2015 3:52 PM, Maxime Ripard wrote:
>> Hi Josh,
>>
>> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote:
>>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
>>>> Le 10/07/2015 14:31, Maxime Ripard a ?crit :
>>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
>>>>>>> I would agree with Maxime. Currently all latest chip reset 
>>>>>>> function is
>>>>>>> compatible with the atmel,sama5d3-rstc.
>>>>>>> So check compatible string is enough for now.
>>>>>>> But of cause if we have other incompatible reset in future with 
>>>>>>> new chip,
>>>>>>> the structure like you said is needed.
>>>>>> We managed to avoid using of_machine_is_compatible() in all the at91
>>>>>> drivers. I'd like to keep it that way. It was painful enough to 
>>>>>> remove
>>>>>> all those cpu_is_at91xxx calls.
>>>>> That's your call...
>>>>>
>>>>>> Also, using it is trying to match strings and will result in 
>>>>>> longer boot
>>>>>> times.
>>>>> Have you looked at the implementation of of_match_device? If that's
>>>>> really a concern to you, you should actually avoid it.
>>>> I agree: let's keep it simple and use of_match_device().
>>> Ok. I will keep it as it is now:  use the (match->data != 
>>> sama5d3_restart)
>>> for the condition.
>> I'm not just that's been an option in our discussion so far.
>>
>> Nicolas said that he was agreeing with me, but at the same time said
>> the complete opposite of what I was arguing for, so I'm not really
>> sure what's really on his mind, but the two options that were
>> discussed were to remove that test, and either:
>>
>>    - Use of_device_is_compatible to prevent the loop execution
>
> Thank you for explaining, it is clear to me.
>
> I'll take this above option. As the of_device_is_compatible() almost 
> same as of_match_node()/of_match_device(). Except that 
> of_device_is_compatible() is more efficient (in this case It calls 
> __of_device_is_compatible() directly) than of_match_node/of_match_device.

Sorry, after checking the code a little, I'd say use the of_match_node 
instead of of_device_is_compatible() is better. Since After check the 
of_device_is_compatible() we also need to call of_match_node() again.

So the simplest way is just get the match data by of_match_node() first, 
then check the match->data. like following:

     match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
     if (match->data != sama5d3_restart) {
         /* we need to shutdown the ddr controller, so get ramc base */
         for_each_matching_node(np, at91_ramc_of_match) {
             at91_ramc_base[idx] = of_iomap(np, 0);
             if (!at91_ramc_base[idx]) {
                 dev_err(&pdev->dev, "Could not map ram controller 
address\n");
                 return -ENODEV;
             }
             idx++;
         }
     }

     at91_restart_nb.notifier_call = match->data;

Best Regards,
Josh Wu
>
>>
>>    - define a structure with a flag to say whether you need the ram
>>      controller quirk or not, and test that flag.
>>
>> Maxime
>>
>
> Best Regards,
> Josh Wu

[PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Josh Wu]

On 7/20/2015 4:44 PM, Josh Wu wrote:
> On 7/20/2015 4:35 PM, Josh Wu wrote:
>> Hi, Maxime
>>
>> On 7/20/2015 3:52 PM, Maxime Ripard wrote:
>>> Hi Josh,
>>>
>>> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote:
>>>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
>>>>> Le 10/07/2015 14:31, Maxime Ripard a ?crit :
>>>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
>>>>>>>> I would agree with Maxime. Currently all latest chip reset 
>>>>>>>> function is
>>>>>>>> compatible with the atmel,sama5d3-rstc.
>>>>>>>> So check compatible string is enough for now.
>>>>>>>> But of cause if we have other incompatible reset in future with 
>>>>>>>> new chip,
>>>>>>>> the structure like you said is needed.
>>>>>>> We managed to avoid using of_machine_is_compatible() in all the 
>>>>>>> at91
>>>>>>> drivers. I'd like to keep it that way. It was painful enough to 
>>>>>>> remove
>>>>>>> all those cpu_is_at91xxx calls.
>>>>>> That's your call...
>>>>>>
>>>>>>> Also, using it is trying to match strings and will result in 
>>>>>>> longer boot
>>>>>>> times.
>>>>>> Have you looked at the implementation of of_match_device? If that's
>>>>>> really a concern to you, you should actually avoid it.
>>>>> I agree: let's keep it simple and use of_match_device().
>>>> Ok. I will keep it as it is now:  use the (match->data != 
>>>> sama5d3_restart)
>>>> for the condition.
>>> I'm not just that's been an option in our discussion so far.
>>>
>>> Nicolas said that he was agreeing with me, but at the same time said
>>> the complete opposite of what I was arguing for, so I'm not really
>>> sure what's really on his mind, but the two options that were
>>> discussed were to remove that test, and either:
>>>
>>>    - Use of_device_is_compatible to prevent the loop execution
>>
>> Thank you for explaining, it is clear to me.
>>
>> I'll take this above option. As the of_device_is_compatible() almost 
>> same as of_match_node()/of_match_device(). Except that 
>> of_device_is_compatible() is more efficient (in this case It calls 
>> __of_device_is_compatible() directly) than 
>> of_match_node/of_match_device.
>
> Sorry, after checking the code a little, I'd say use the of_match_node 
> instead of of_device_is_compatible() is better. Since After check the 
> of_device_is_compatible() we also need to call of_match_node() again.

Okay, Please forget above reply. As Maxime said test the pointer is not 
good solution here.
So I'll sent out v2 which use of_device_is_compatible().

Best Regards,
Josh Wu