| Message ID | 1298311199-18775-4-git-send-email-Kyle.D.Moffett@boeing.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Kyle Moffett, In message <1298311199-18775-4-git-send-email-Kyle.D.Moffett@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? > 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) ? > +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! > + asm("sync; isync":::"memory"); Why would that be needed? out_be32() is supposed to provide sufficient memory barriers. > +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.] > +#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. Best regards, Wolfgang Denk
On Feb 21, 2011, at 16:14, Wolfgang Denk wrote: > In message <1298311199-18775-4-git-send-email-Kyle.D.Moffett@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
Dear "Moffett, Kyle D", In message <D99CA72A-BD3E-475D-B17F-1253410C6591@boeing.com> you wrote: > > >> +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. It's not unacceptable, but maybe you add a comment to explain why these return int. > --Apple-Mail-2-42328377 > Content-Disposition: attachment; filename"smime.p7s" > Content-Type: application/pkcs7-signature; name"smime.p7s" > Content-Transfer-Encoding: base64 Do you really have to append a 120 line binary encoded signature here? Best regards, Wolfgang Denk
On Feb 21, 2011, at 16:56, Wolfgang Denk wrote: > In message <D99CA72A-BD3E-475D-B17F-1253410C6591@boeing.com> you wrote: >> >>>> +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. > > It's not unacceptable, but maybe you add a comment to explain why > these return int. Ok, will fix, thanks. >> --Apple-Mail-2-42328377 >> Content-Disposition: attachment; filename"smime.p7s" >> Content-Type: application/pkcs7-signature; name"smime.p7s" >> Content-Transfer-Encoding: base64 > > Do you really have to append a 120 line binary encoded signature here? Ah, crap, somehow S/MIME signatures got turned on. It's a tiny checkbox in the corner of the GUI. Sorry! Cheers, Kyle Moffett
diff --git a/arch/powerpc/include/asm/mpc85xx_gpio.h b/arch/powerpc/include/asm/mpc85xx_gpio.h new file mode 100644 index 0000000..d02d933 --- /dev/null +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h @@ -0,0 +1,124 @@ +/* + * Copyright 2010 eXMeritus, A Boeing Company + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <asm/immap_85xx.h> + +/* + * The following internal functions are an MPC85XX-specific GPIO API which + * allows setting and querying multiple GPIOs in a single operation. + * + * All of these look relatively large, but the arguments are almost always + * constants, so they compile down to just a few instructions and a + * memory-mapped IO operation or two. + */ +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); + asm("sync; isync":::"memory"); + out_be32(&gpio->gpdat, val); +} + +static inline void mpc85xx_gpio_set_in(unsigned int gpios) +{ + mpc85xx_gpio_set(gpios, 0x00000000, 0x00000000); +} + +static inline void mpc85xx_gpio_set_low(unsigned int gpios) +{ + mpc85xx_gpio_set(gpios, 0xFFFFFFFF, 0x00000000); +} + +static inline void mpc85xx_gpio_set_high(unsigned int gpios) +{ + mpc85xx_gpio_set(gpios, 0xFFFFFFFF, 0xFFFFFFFF); +} + +static inline unsigned int mpc85xx_gpio_get(unsigned int mask) +{ + volatile ccsr_gpio_t *gpio; + unsigned int ret; + + /* Read the requested values */ + gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); + ret = mask & in_be32(&gpio->gpdat); + + return ret; +} + +/* + * These implement the standard generic GPIO API on top of the + * MPC85xx-specific header. + */ +#ifdef CONFIG_MPC85XX_GENERIC_GPIO +static inline int gpio_request(unsigned gpio, const char *label) +{ + /* Do nothing */ + (void)gpio; + (void)label; +} + +static inline void gpio_free(unsigned gpio) +{ + /* Do nothing */ + (void)gpio; +} + +static inline int gpio_direction_input(unsigned gpio) +{ + mpc85xx_gpio_set_in(1U << gpio); + return 0; +} + +static inline int gpio_direction_output(unsigned gpio, int value) +{ + mpc85xx_gpio_set_low(1U << gpio); + return 0; +} + +static inline int gpio_get_value(unsigned gpio) +{ + return !!mpc85xx_gpio_get(1U << gpio) +} + +static inline void gpio_set_value(unsigned gpio, int value) +{ + if (value) + mpc85xx_gpio_set_high(1U << gpio); + else + mpc85xx_gpio_set_low(1U << gpio); +} + +static inline int gpio_is_valid(int gpio) +{ + return (gpio >= 0) && (gpio < 32); +} +#endif /* CONFIG_MPC85XX_GENERIC_GPIO */
To ease the implementation of other MPC85xx board ports, several common GPIO helpers are added to <asm/mpc85xx_gpio.h>. Since each of these compiles to no more than 4-5 instructions it would be very inefficient to call them out of line, therefore we put them entirely in the header file. The HWW-1U-1A board port which these were written for strongly prefers to set multiple GPIOs as a single batch operation, so the API is designed around that basis. 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 Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com> --- arch/powerpc/include/asm/mpc85xx_gpio.h | 124 +++++++++++++++++++++++++++++++ 1 files changed, 124 insertions(+), 0 deletions(-) create mode 100644 arch/powerpc/include/asm/mpc85xx_gpio.h