[U-Boot] [RESEND] [PATCH 0/2] kirkwood: Generalise dns325 support, include dns320

[Jamie Lentin]

Hi Prafulla, thankyou for taking the time to review.

On Tue, 31 Jul 2012, Prafulla Wadaskar wrote:

>> -----Original Message-----
>> From: Jamie Lentin [mailto:jm at lentin.co.uk]
>> Sent: 31 July 2012 14:01
>> To: Prafulla Wadaskar
>> Cc: u-boot at lists.denx.de; albert.u.boot at aribaud.net;
>> uboot at lukaperkov.net
>> Subject: RE: [RESEND] [PATCH 0/2] kirkwood: Generalise dns325 support,
>> include dns320
>>
>> On Mon, 30 Jul 2012, Prafulla Wadaskar wrote:
>>
>>> Dear Jamie Lentin
>>>
>>>> -----Original Message-----
>>>> From: Jamie Lentin [mailto:jm at lentin.co.uk]
>>>> Sent: 31 July 2012 03:26
>>>> To: u-boot at lists.denx.de
>>>> Cc: Prafulla Wadaskar; albert.u.boot at aribaud.net;
>>>> uboot at lukaperkov.net; Jamie Lentin
>>>> Subject: [RESEND] [PATCH 0/2] kirkwood: Generalise dns325 support,
>>>> include dns320
>>>>
>>>> I submitted this a while ago[0], it would be nice to see it
>> included
>>>> if possible. It generalises the DNS325 support so that it can be
>> used
>>>> for both the DNS320 and DNS325.
>>>>
>>>> Luka Perkov stated "I have no more questions regarding this patch",
>>>> not
>>>> sure if this counts as an ACK. The patch here is ~same, just
>> rebased.
>>>>
>>>> Somewhat related, I have tried using tools/kwboot to boot both
>> NASes.
>>>> The DNS325 boots fine, however the DNS320 reports:-
>>>>
>>>> Sending boot message. Please reboot the target...|
>>>> Sending boot image...
>>>>   0 % [+++++++++++++++++xmodem: Bad message
>>>>
>>>> UART-boot works with kwuartboot, although only when it is killed
>> and
>>>> restarted. If I work out anything interesting will follow it up in
>>>> a separate thread.
>>>>
>>>> Any feedback appreciated!
>>>>
>>>> [0] http://thread.gmane.org/gmane.comp.boot-loaders.u-
>>>> boot/130234/focus=130575
>>>>
>>>> Jamie Lentin (2):
>>>>   kirkwood: Rename dns325 to dnskw
>>>>   kirkwood: Add support for the D-Link DNS-320
>>>>
>>>>  MAINTAINERS                                        |    4 +
>>>>  board/d-link/{dns325 => dnskw}/Makefile            |    2 +-
>>>>  board/d-link/{dns325/dns325.c => dnskw/dnskw.c}    |   18 +-
>>>>  board/d-link/{dns325/dns325.h => dnskw/dnskw.h}    |   30 ++-
>>>>  board/d-link/dnskw/kwbimage.dns320.cfg             |  207
>>>> ++++++++++++++++++++
>>>>  .../kwbimage.cfg => dnskw/kwbimage.dns325.cfg}     |    0
>>>>  boards.cfg                                         |    3 +-
>>>>  include/configs/{dns325.h => dnskw.h}              |   21 +-
>>>>  8 files changed, 261 insertions(+), 24 deletions(-)
>>>>  rename board/d-link/{dns325 => dnskw}/Makefile (98%)
>>>>  rename board/d-link/{dns325/dns325.c => dnskw/dnskw.c} (90%)
>>>>  rename board/d-link/{dns325/dns325.h => dnskw/dnskw.h} (65%)
>>>>  create mode 100644 board/d-link/dnskw/kwbimage.dns320.cfg
>>>
>>> Finally you will two kwbimage files for two boards being supported.
>> BTW:
>>> I would like to ask : what is a difference between these two files?
>> If
>>> it is very small, it can be handled in early_board_init().
>>>
>>> In that case you can keep the earlier name kwbimage.cfg as it is and
>> one file can be avoided.
>>
>> The DNS-320 has half the RAM of the DNS-325 at different timings, so
>> the
>> difference is non-trivial. I'm not sure there would be a logical way
>> to
>> divide it up into a common setup and changes in early_board_init(). I
>> could have a go if you prefer though---if there's prior art somewhere
>> else
>> in u-boot please let me know and I'll have a look.
>
> Hi Jamie
>
> You can create a common kwbimage.cfg that suits for both the 
> boards (with larger RAM size and comfortable timings for both the 
> boards)
>
> These configuration will be pushed on the Kirkwood registers before 
> u-boot image kick start. Then in early_board_init() you can tuned these 
> register for appropriate boards (for ex. reduce size, or tune timings 
> etc.)
>
> Please try to implement this if this sounds good.

I've had a look into this, and still not convinced it's the way to go.

Designing a kwbimage.cfg that suits both is somewhat outside my sphere of 
knowledge (both of the hardware in question and generally). e.g. can I 
assert both M_ODT[0] (as used in DNS-325) and M_ODT[1] (DNS-320) during 
write to CS0 safely, not knowing where the other pin is wired to? Will 
changing the address multiplex at runtime result in memory corruption?

Regardless, the end result would increase board-specifics. There would be 
the same information as in kwbimage.dns32[0|5].cfg now, but hidden away in 
device-specfic code and C constants instead of in a config file with a 
known format and location. This seems like a step backwards to me, and 
would certainly give headaches to anyone trying to move away from 
kwbimage.cfg to, say, devicetree.

I've implemented the other comments on the patchset, and can send these if 
you're happy with the above.

Thanks,

> Regards...
> Prafulla . . .
>

-- 
Jamie Lentin