On Feb 21, 2011, at 16:14, Wolfgang Denk wrote: > In message <1298311199-18775-4-git-send-email-kyle.d.moff...@boeing.com> you > wrote: >> To ease the implementation of other MPC85xx board ports, several common >> GPIO helpers are added to <asm/mpc85xx_gpio.h>. > > In which way is this specific to 85xx? Why not make available on a > wider base?
This particular set of registers is a particular part of the MPC85xx-series SOC.
The other boards based on mpc85xx seem to just do stuff like this:
volatile ccsr_gpio_t *pgpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR);
[...]
in_be32(&pgpio->gpdat);
[...]
out_be32(&pgpio->gpdat, newdat);
[...]
I thought that was kind of ugly and abstracted it out into static inline
functions in my board-specific header. Last time this was posted for review it
was pointed out that they are generally applicable to all MPC85xx chips ans so
I moved them to a more-generic header for other MPC85xx boards to use.
>> To assist other board ports, a small set of wrappers are used which
>> provides a standard gpio_request() interface around the MPC85xx-specific
>> functions. This can be enabled with CONFIG_MPC85XX_GENERIC_GPIO
>
> How similar is this interface with other "generic" GPIO interfaces we
> have here in U-Boot (and in Linux) ?
The interface emulates the "generic" GPIO API in Linux because Peter Tyser
indicated he would use that generic interface for his other MPC85xx board ports
if it was available. I don't actually use that particular API though; it isn't
really suitable to my board-support code and I'd rather delete it than try to
extend it further.
>> +static inline void mpc85xx_gpio_set(unsigned int mask,
>> + unsigned int dir, unsigned int val)
>> +{
>> + volatile ccsr_gpio_t *gpio;
>> +
>> + /* First mask off the unwanted parts of "dir" and "val" */
>> + dir &= mask;
>> + val &= mask;
>> +
>> + /* Now read in the values we're supposed to preserve */
>> + gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00);
>> + dir |= (in_be32(&gpio->gpdir) & ~mask);
>> + val |= (in_be32(&gpio->gpdat) & ~mask);
>> +
>> + /* Now write out the new values, writing the direction first */
>> + out_be32(&gpio->gpdir, dir);
>
> This way work, but quite often is wrong. If you set the direction
> first for an output, you start driving the old value, before you set
> the correct one. This results in a short pulse that may cause
> negative side effects.
>
> Don't!
Whoops, I did get that completely backwards! I was very carefully trying to
ensure a clean transition and managed to swap it :-(.
Will fix, thanks for noticing!
>> + asm("sync; isync":::"memory");
>
> Why would that be needed? out_be32() is supposed to provide sufficient
> memory barriers.
Again, I was just cribbing from some of the other board ports. If in_be32()
and out_be32() are defined to provide enough barriers then I'll remove it.
>> +static inline unsigned int mpc85xx_gpio_get(unsigned int mask)
>> +{
>> + volatile ccsr_gpio_t *gpio;
>
> Why would this volatile be needed here? [Please fix globally.]
As above this was copied from other MPC85xx boards, will fix.
>> +#ifdef CONFIG_MPC85XX_GENERIC_GPIO
>> +static inline int gpio_request(unsigned gpio, const char *label)
>> +{
>> + /* Do nothing */
>> + (void)gpio;
>> + (void)label;
>
> NAK. Please don't do that. Fix globally.
>
>> +static inline int gpio_direction_input(unsigned gpio)
>> +{
>> + mpc85xx_gpio_set_in(1U << gpio);
>> + return 0;
>> +}
>
> Why is this function not void when it cannot return any usefult return
> code anyway?
>
>> +static inline int gpio_direction_output(unsigned gpio, int value)
>> +{
>> + mpc85xx_gpio_set_low(1U << gpio);
>> + return 0;
>> +}
>
> Ditto.
This is the "Linux-compatible" gpio layer that Peter Tyser was asking for. I
sort-of-copied most of these functions from arch/nios2/include/asm/gpio.h,
which just does the "return 0;" in several functions.
Those 2 later functions are expected to be able to return an error (for I2C
chips and such). As I said above, if these wrappers are unacceptable then I
will just delete them; the only thing I use are the mpc85xx_gpio_*() functions.
Thanks for the review!
Cheers,
Kyle Moffett
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
