From mboxrd@z Thu Jan  1 00:00:00 1970
From: Albert ARIBAUD <albert.aribaud@free.fr>
Date: Thu, 01 Jul 2010 00:35:55 +0200
Subject: [U-Boot] [PATCH] orion5x: edminiv2: add libata support
In-Reply-To: <20100630213939.1AEBE1524EC@gemini.denx.de>
References: <1277933418-682-1-git-send-email-albert.aribaud@free.fr>
	<20100630213939.1AEBE1524EC@gemini.denx.de>
Message-ID: <4C2BC6CB.1070802@free.fr>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Wolfgang,

Le 30/06/2010 23:39, Wolfgang Denk a ?crit :
> Dear Albert Aribaud,
>
> In message<1277933418-682-1-git-send-email-albert.aribaud@free.fr>  you wrote:
>>
>> Signed-off-by: Albert Aribaud<albert.aribaud@free.fr>
>> ---
>> This patch:
>> - adds support in libata for the orion5x MVSATAHC controller;
>> - enables orion5x MVSTAHC port 1 on the edmini board;
>> - adds IDE and EXT2 commands to the edminiv2 command set.
>
> What exactly do you mean by "libata"?   Do we have something like this
> in U-Boot?

My mistake: I meant ide. I'd started this work looking into the libata.c 
block driver but realized later that it wasn't what I needed, but the 
term stuck in my mind. I'll fix references to mention IDE instead.

>> +/* ED Mini V2 uses SATA PORT1. Initialize this port and disable
>> + * disable low power on it
>> + */
>> + /* mask for isolating IPM and DET fields in SControl register */
>
> Incorrect multiline comment style.

Will this be ok?

	/*
	 * ED Mini V2 [...]
	 * disable [...]
	 */

	/* mask for [...] */

>> -#if defined(__PPC__) || defined(CONFIG_PXA_PCMCIA) || defined(CONFIG_SH)
>> +#if defined(__PPC__) || defined(CONFIG_PXA_PCMCIA) || defined(CONFIG_SH) \
>> +	|| defined(__ARM__)
>
> Please do not add to this mess of tests, but introduce a single
> feature-specific #define instead.

Will do. Would CONFIG_IDE_USE_INSW_OUTSW do? Basically, that's what the 
test is for: either use outsw()/insw() or use outw()/inw() within a loop.

Note that this change does not relate per se to my patch and requires 
addition of this #define to a lot of config header files. Wouldn't it be 
better if I submitted it as an independent atomic patch?

>> +/* Debugging features */
>> +
>> +/* #define the following if u-boot will boot from RAM */
>> +/* #undef it if u-boot will boot from FLASH */
>> +#undef CONFIG_SKIP_LOWLEVEL_INIT	/* disable board lowlevel_init */
>> +
>
> This seems to be an unrelated change (like someother, smaller ones).
> Please make sure to split your patches into independent parts.

I'll put them in two separate patches, one for fixing typos and one for 
adding the CONFIG_SKIP_LOWLEVEL_INIT comments and line.

> Best regards,
>
> Wolfgang Denk

Thanks for the feedback.

Amicalement,
-- 
Albert.