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

[Haavard Skinnemoen]

Olav Morken wrote:
> 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 don't think that would be much of an improvement. It would make the
code much larger, and the pin definitions themselves would still need
some magic number...and you'll need to know which GPIO port they belong
to in order to actually use them.

> >> 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.

Exactly. At some point, you need code which encapsulates the
definitions in the data sheet, and that's the whole purpose of these
functions.

> > 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...

I think you need to actually use the parameters passed to that function
in order to set up the correct address, data and chip select lines.

Now, the alternative mappings for some of the pins are tricky...but you
could perhaps add a few more flags to cover those possibilities...

Once you do that, the whole function becomes a matter of shifting bits
into position, and I very much doubt a bunch of defines can make that
sort of code any clearer.

Think of this function as the executable equivalent of a pin definition.

> >> 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.

I suppose this might be a good idea. But I haven't heard of any
problems with other boards because of this...and wasn't there a generic
PHY layer in the works anyway?

> > 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.

On other chips, it also covers the NOR flash you're booting from. So I
suppose we should look into this...maybe we need some sort of "notifier
chain" thing to give other drivers a chance to reset their
peripherals...

Haavard