[U-Boot] [PATCH v2 8/9] AVR32: CPU support for AT32UC3A0xxx CPUs

[Olav Morken]

On Mon, Jan 26, 2009 at 21:03, Wolfgang Denk <wd at denx.de> wrote:
> Dear =?ISO-8859-1?Q?Gunnar_Rang=F8y?=,
>
> In message <e1b3d0a60901260726i422cfe85s7c5a35745eedc60f at mail.gmail.com> you wrote:
[....]
>> >
>> > It would be nice if you used readable names instead of all these
>> > magic hardcoded constants.
>>
>> Each bit in the masks represents a single pin on the microcontroller.
>> The readable names would therefore become something like:
>> #define EBI_PINS_PORT0 (0x0003C000 | 0x1E000000)
>> #define EBI_PINS_PORT1 (0x00000010 | 0x00007C00 | ...)
>
> Um, no. You didn't get it. You use magic numbers again.
>
> That should be some
>
> #define FOO_PIN_XXX     0x0003C000
> #define FOO_PIN_YYY     0x1E000000
> #define BAR_PIN_XXX     0x00000010
> #DEFINE BAR_PIN_YYY     0x00007C00
>
> in one place and
>
> #define EBI_PINS_PORT0 (FOO_PIN_XXX | FOO_PIN_YYY)
> #define EBI_PINS_PORT1 ((BAR_PIN_XXX| BAR_PIN_YYY)
>
> elsewhere, using readable names for the defines.

Sorry, but the bitwise or's in the code are a bit misleading. The only
function of them is to make it more readable in the presence of the
datasheet for the microcontroller. Each single bit in the bitmask is
one pin on the microcontroller. In this case, it would become something
like:
#define EBI_PIN_NCS0    (1<<14)
#define EBI_PIN_ADDR20  (1<<15)
#define EBI_PIN_ADDR21  (1<<16)
#define EBI_PIN_ADDR22  (1<<17)
(....)
#define EBI_PINS_PORT0 (EBI_PIN_NCS0 | EBI_PIN_ADDR20 | EBI_PIN_ADDR21 \
                        | EBI_PIN_ADDR22 | (....))

>> I'm not certain that that would make it much more readable. The
>> bitmasks are more or less based on reading the datasheet and counting
>> which pins are used.
>
> This is exactly what should NOT be necessary to read and understand
> the code.

[....]

But in this case, this is code which should never be changed without
looking at the datasheet, and probably schematics for the board in
question.

> This is simply not acceptable.
>
>> Maybe adding a comment before the code would be just as useful?
>
> No, a comment can't fix this, especially as you just have to  wait  a
> few months to see the comment and the code getting out of sync.
>
> Please fix the code.

How about something like this:
/*
 * These bitmasks represents the pins used by the EBI on this board.
 * Please refer to the datasheet for the UC3A-series microcontrollers
 * for a description of the individual pins.
 */
#define EBI_PINS_PORT0 0x1E03C000
#define EBI_PINS_PORT1 0xE0037C10
(....)

And in the portmux-configuration-function:
portmux_select_peripheral(PORTMUX_PORT(0), EBI_PINS_PORT0,
			  PORTMUX_FUNC_C, 0);
(....)

When looking at this code I can see that we need a way for each
individual board to change the bitmasks, since the same pin in some
cases can be routed out to several places (depending on the board).
Maybe a CONFIG_UC3A0xxx_EBI_PINS_PORT0 option or something like that
could be added...

>> > Instead of using offsets everywhere I would appreciate if the code
>> > could be changed to use C structs instead (like we do in PPC land).
>>
>> It was done like this because it was done in the existing AVR32 code
>> which we based our work on. Since we used parts of that code, it made
>> most sense to use the same coding style.
>>
>> We can change it if you think it is necessary.
>
> Yes, please change it. And patches to fix the existing code that doe
> sthis are welcome, too.

OK, will do this. I do however think that we will leave the code for
other chips and boards as it is, since changing it would require very
much time, and we don't have any way to test the changes in any case.

Do you have any specific files we could use as a style-guide for such
structs?

>> As most of the needed functionality is embedded in the microcontroller,
>> there are very few external peripherals used by U-Boot. Apart from
>> external memory, and oscillator, and level-shifters for the serial-port,
>> there is only the ethernet PHY, and that one shouldn't need a reset.
>
> Famous last words. What if exactrly the PHY is stuck and needs a
> reset?

The only reset we can do on the PHY is a software reset, by sending a
reset command over the (R)MII bus, and I don't believe that the generic
chip code is the place to do that. If it should be done, I believe it
should be done by the macb-driver after the reset. This would allow it
to recover even if the microcontroller wasn't reset by the
reset-command, but for example by a watchdog timer.

> Hmmm... "apart from external memory" ... does  externam  memory  also
> include  NOR  flash?  Eventually  the NOR flash you are booting from?
> Assume the NOR flash is in query mode when you reset the board -  how
> does it get reset, then?

External memory in this case would be SRAM or SDRAM.


Best regards,
Olav Morken