[U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

[Andreas Bießmann]

Hi Markus,

On 01.08.12 21:28, Markus Hubig wrote:
> Hello Andreas,
> 
> thanks for your responce. I will provide an updated patch based on
> your comments!
> 
> On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
>> ---8<---
>> andreas at andreas-mbp % ./tools/checkpatch.pl
>> U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch
>> WARNING: Whitespace before semicolon
>> #214: FILE: board/taskit/stamp9g20/stamp9g20.c:123:
>> +		;
>>
>> total: 0 errors, 1 warnings, 484 lines checked
>>
>> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
>> MULTISTATEMENT_MACRO_USE_DO_WHILE
>>
>> U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch has style
>> problems, please review.
>>
>> If any of these errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>> --->8---
>>
>> I know this part is copied from other at91 boards but I wonder how we
>> should handle these while-loops without content.
> 
> Oh I didn't recognice there is a version of checkpatch provided with
> u-boot. I used the one from kernel.org which didn't put a warning out
> for this one ...
> 
>> On 30.07.12 20:01, Markus Hubig wrote:
>>> This adds support for the AT91SAM9G20 boards by taskit GmbH.
>>> Both boards, Stamp9G20 and PortuxG20, are integrated in one
>>> file. PortuxG20 is basically a SBC built around the Stamp9G20.
>>>
>>> Signed-off-by: Markus Hubig <mhubig at imko.de>
>>> Cc: Andreas Bießmann <andreas.devel at googlemail.com>
>>> ---
>>>  board/taskit/stamp9g20/Makefile    |   52 ++++++++
>>>  board/taskit/stamp9g20/stamp9g20.c |  199 +++++++++++++++++++++++++++++++
>>>  boards.cfg                         |    2 +
>>>  include/configs/stamp9g20.h        |  225 ++++++++++++++++++++++++++++++++++++
>>
>> MAINTAINER entry is missing
> 
> Fixed!
> 
>>> +#ifdef CONFIG_MACB
>>> +static void stamp9G20_macb_hw_init(void)
>>> +{
>>> +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>>> +	struct at91_port *pioa = (struct at91_port *)ATMEL_BASE_PIOA;
>>> +	struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
>>> +	unsigned long erstl;
>>> +
>>> +	/* Enable MACB Chip, this is the enable PIN on Stamp Adaptor*/
>> This comment is a bit misleading. Isn't MACB the SoC MAC component? Why
>> should we switch an external element to enable an internal part? Can you
>> please rewrite the comment to be more precise?
> 
> Hmm yes you're right. That pin enables the PHY Chip which of course is
> an external component ...
> 
>>> +	at91_set_gpio_output(AT91_PIN_PA26, 0);
>>> +
>>> +	/* Enable EMAC clock */
>>> +	writel(1 << ATMEL_ID_EMAC0, &pmc->pcer);
>>
>> Is it possible to move this 'enable clock' into at91_macb_hw_init()?
>> Does your PHY address setting still work then?
>> If yes, can you please send a separate patch first adding the 'enable
>> clock' into the correct at91_macb_hw_init()?
> 
> Yes it works. I put it into arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
> and will send a separate patch.
> 
>>> +	/* Need to reset PHY -> 500ms reset */
>>> +	writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
>>> +		AT91_RSTC_MR_URSTEN, &rstc->mr);
>>
>> Hmm ... is it OK to generate the user reset here? I know this is the
>> same in at least at91sam9263ek, can you please check if we should
>> instead delete that bit in MR?
> 
> MR? Sorry I don't get this one. Please explain a bit ...

I talked about URSTEN bit in RSTC_MR (Reset Controller Mode Register;
p99 in at91sam9g20 datasheet). The URSTEN bit set to 1 means disable low
level detection on NRST pin. Which in fact disables external reset with
the reset key. One have to check if this is true or maybe I'm wrong here.

>>> +
>>> +	writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, &rstc->cr);
>>> +
>>> +	/* Wait for end hardware reset */
>>> +	while (!(readl(&rstc->sr) & AT91_RSTC_SR_NRSTL))
>>> +		;
>>
>> Endless loop if bit is not toggling, can you please add some watchdog
>> reset (if you still use wdt in your board) and some timeout?
>> This will also solve the warning detected by checkpatch.
> 
> Maybe something like this?
> 

> | /* Wait for end of hardware reset */
> | while (!(readl(&rstc->sr) & AT91_RSTC_SR_NRSTL))
> | {
> | 	/* avoid shutdown by watchdog */
> | 	hw_watchdog_reset();

	WATCHDOG_RESET();

> | 	
> | 	mdelay(10);
> | 	timeout--;
> | 	
> | 	/* timeout for not getting stuck in an endless loop */
> | 	if (timeout <= 0) {
> | 		debug("ERROR: Timeout waiting for PHY reset!\n");

Error messages should not use debug macro.

> | 		break;
> | 	};
> | };

For timeout stuff you could also use get_timer(0) to get current
timestamp and compare against another timestamp.

<snip>

> 
> PS.: I will send a new patch with all your requested changes already
> included, not a couple of small ones based in this one? Right!?

Yes, just a new version (v2), it is still arrived, will have a look then.

Best regards

Andreas Bießmann