Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1181167 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] power: reset: at91: add sama5d3 reset function Josh Wu <josh.wu@atmel.com> - 2015-07-10 04:10 +0200
    Re: [PATCH 1/2] power: reset: at91: add sama5d3 reset function Guenter Roeck <linux@roeck-us.net> - 2015-07-10 05:20 +0200
      Re: [PATCH 1/2] power: reset: at91: add sama5d3 reset function Josh Wu <josh.wu@atmel.com> - 2015-07-10 06:00 +0200
      Re: [PATCH 1/2] power: reset: at91: add sama5d3 reset function Guenter <linux@roeck-us.net> - 2015-07-10 19:10 +0200

#1181167 — Re: [PATCH 1/2] power: reset: at91: add sama5d3 reset function

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1181184

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1181192

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1181766

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web