[U-Boot] [PATCH v3 13/15] arm, da850evm: Add an SPL for SPI boot

[Tom Rini]

On 12/05/2011 03:20 AM, Christian Riesch wrote:
> Hi Tom,
> 
> On Sat, Dec 3, 2011 at 6:49 AM, Christian Riesch
> <christian.riesch at omicron.at> wrote:
>> Hi Tom,
>> Thanks for your comments.
>>
>>
>> On Friday, December 2, 2011, Tom Rini <trini at ti.com> wrote:
>>> On 12/02/2011 09:12 AM, Christian Riesch wrote:
>>>
>>> [snip]
>>>>  include/configs/da850evm.h            |   87
>>>> +++++++++++++++++++++++++++++++++
>>> [snip]
>>>> +#define CONFIG_SYS_DA850_DDR2_SDTIMR (0 |    \
>>>> +     (14 << DV_DDR_SDTMR1_RFC_SHIFT) |       \
>>>> +     (2 << DV_DDR_SDTMR1_RP_SHIFT) |         \
>>>> +     (2 << DV_DDR_SDTMR1_RCD_SHIFT) |        \
>>>> +     (1 << DV_DDR_SDTMR1_WR_SHIFT) |         \
>>>> +     (5 << DV_DDR_SDTMR1_RAS_SHIFT) |        \
>>>> +     (8 << DV_DDR_SDTMR1_RC_SHIFT) |         \
>>>> +     (1 << DV_DDR_SDTMR1_RRD_SHIFT) |        \
>>>> +     (0 << DV_DDR_SDTMR1_WTR_SHIFT))
>>>
>>> '0 | ..' and '0 << ...' don't help readability over just value saying it
>>> (same with shifting 0).  Also, unless the manual these come from uses
>>> decimal here, hex is preferred.  Thanks!
>>>
>>
>> '0 | ...'.   I agree, I'll remove this.
>>
>> '0 << ...'
>> Aaaahhh... Yes, that's pretty useless here, the WTR bits are reserved bits
>> :-/
>>
> 
> Uh, sorry, they are not reserved. But a zero value here means that we
> have one clock cycle (because it's number of clock cycles minus one).
> So I'd like to keep the line because it will help others to see that
> WTR is set to one clock cycle.

I'd say it's not obvious that a value of zero means one clock cycle, so
drop the define and just comment what the full value contains.

-- 
Tom