[U-Boot] [PATCH 3/7] mpc85xx: Add inline GPIO acessor functions

[Moffett, Kyle D]

On Feb 21, 2011, at 16:14, Wolfgang Denk wrote:
> In message <1298311199-18775-4-git-send-email-Kyle.D.Moffett at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6686 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110221/2a7fba91/attachment.bin