| Message ID | 1379842421-13957-1-git-send-email-Yuantian.Tang@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | York Sun |
| Headers | show |
On 09/22/2013 02:33 AM, Tang Yuantian wrote: > From: Tang Yuantian <yuantian.tang@freescale.com> > > The offset of register address within GPIO module is just > CONFIG_SYS_MPC85xx_GPIO_ADDR, no reason to add 0xc00. > > Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com> > --- > arch/powerpc/include/asm/mpc85xx_gpio.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/mpc85xx_gpio.h b/arch/powerpc/include/asm/mpc85xx_gpio.h > index 3d11884..87bb4a0 100644 > --- a/arch/powerpc/include/asm/mpc85xx_gpio.h > +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h > @@ -20,7 +20,7 @@ > static inline void mpc85xx_gpio_set(unsigned int mask, > unsigned int dir, unsigned int val) > { > - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); > + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); > > /* First mask off the unwanted parts of "dir" and "val" */ > dir &= mask; > @@ -56,7 +56,7 @@ static inline void mpc85xx_gpio_set_high(unsigned int gpios) > > static inline unsigned int mpc85xx_gpio_get(unsigned int mask) > { > - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); > + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); > > /* Read the requested values */ > return in_be32(&gpio->gpdat) & mask; > Yuantian, Please go through the base address again. I think some SoCs do use 0xc00 offset from 0xF000, for eample P1020, P1023, MPC8572. I only spot checked several. York
> > diff --git a/arch/powerpc/include/asm/mpc85xx_gpio.h > > b/arch/powerpc/include/asm/mpc85xx_gpio.h > > index 3d11884..87bb4a0 100644 > > --- a/arch/powerpc/include/asm/mpc85xx_gpio.h > > +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h > > @@ -20,7 +20,7 @@ > > static inline void mpc85xx_gpio_set(unsigned int mask, > > unsigned int dir, unsigned int val) { > > - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); > > + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); > > > > /* First mask off the unwanted parts of "dir" and "val" */ > > dir &= mask; > > @@ -56,7 +56,7 @@ static inline void mpc85xx_gpio_set_high(unsigned > > int gpios) > > > > static inline unsigned int mpc85xx_gpio_get(unsigned int mask) { > > - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); > > + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); > > > > /* Read the requested values */ > > return in_be32(&gpio->gpdat) & mask; > > > > Yuantian, > > Please go through the base address again. I think some SoCs do use 0xc00 > offset from 0xF000, for eample P1020, P1023, MPC8572. I only spot checked > several. > Hi York, I double checked the offset address of GPIO, I found that the offset addresses of GPIO on the boards you mentioned above are all changed to 0x0, not 0xc00 according to the newest RM. I do found that the offset address is 0xc00 in some old RMs. You can find the newest RM here: For MPC8572: http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223503931/226628079/226445024/MPC8572ERM_Rev3_DRAFT1.pdf?nodeid=226438746&vernum=-2 For p1023: http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223506436/223521755/223743960/P1023RM_Mark-up.pdf?nodeid=229647544&vernum=-2 for 1020: http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223506436/223522385/224515312/P1020RM_Rev6_Mark-up.pdf?nodeid=228476444&vernum=-2 If the offset addresses on these boards were 0xc00, the driver is still wrong, because in that case The GPIO address should be: CONFIG_SYS_IMMR + 0xc00, not CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00. (CONFIG_SYS_MPC85xx_GPIO_ADDR == CONFIG_SYS_IMMR + CONFIG_SYS_MPC85xx_GPIO_OFFSET). So, please apply this patch, I need the GPIO driver to operate GPIO. Regards, Yuantian > York
On 10/07/2013 08:28 PM, Tang Yuantian-B29983 wrote: >>> diff --git a/arch/powerpc/include/asm/mpc85xx_gpio.h >>> b/arch/powerpc/include/asm/mpc85xx_gpio.h >>> index 3d11884..87bb4a0 100644 >>> --- a/arch/powerpc/include/asm/mpc85xx_gpio.h >>> +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h >>> @@ -20,7 +20,7 @@ >>> static inline void mpc85xx_gpio_set(unsigned int mask, >>> unsigned int dir, unsigned int val) { >>> - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); >>> + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); >>> >>> /* First mask off the unwanted parts of "dir" and "val" */ >>> dir &= mask; >>> @@ -56,7 +56,7 @@ static inline void mpc85xx_gpio_set_high(unsigned >>> int gpios) >>> >>> static inline unsigned int mpc85xx_gpio_get(unsigned int mask) { >>> - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); >>> + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); >>> >>> /* Read the requested values */ >>> return in_be32(&gpio->gpdat) & mask; >>> >> >> Yuantian, >> >> Please go through the base address again. I think some SoCs do use 0xc00 >> offset from 0xF000, for eample P1020, P1023, MPC8572. I only spot checked >> several. >> > > Hi York, > I double checked the offset address of GPIO, I found that the offset addresses of > GPIO on the boards you mentioned above are all changed to 0x0, not 0xc00 according > to the newest RM. > I do found that the offset address is 0xc00 in some old RMs. > You can find the newest RM here: > For MPC8572: http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223503931/226628079/226445024/MPC8572ERM_Rev3_DRAFT1.pdf?nodeid=226438746&vernum=-2 > For p1023: > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223506436/223521755/223743960/P1023RM_Mark-up.pdf?nodeid=229647544&vernum=-2 > for 1020: > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223506436/223522385/224515312/P1020RM_Rev6_Mark-up.pdf?nodeid=228476444&vernum=-2 > > If the offset addresses on these boards were 0xc00, the driver is still wrong, because in that case > The GPIO address should be: CONFIG_SYS_IMMR + 0xc00, not CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00. > (CONFIG_SYS_MPC85xx_GPIO_ADDR == CONFIG_SYS_IMMR + CONFIG_SYS_MPC85xx_GPIO_OFFSET). > > So, please apply this patch, I need the GPIO driver to operate GPIO. > Looks you are correct. I find the changed offset on some published documents as well (please use public link for discussion in the future). York
On 10/07/2013 08:28 PM, Tang Yuantian-B29983 wrote: >>> diff --git a/arch/powerpc/include/asm/mpc85xx_gpio.h >>> b/arch/powerpc/include/asm/mpc85xx_gpio.h >>> index 3d11884..87bb4a0 100644 >>> --- a/arch/powerpc/include/asm/mpc85xx_gpio.h >>> +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h >>> @@ -20,7 +20,7 @@ >>> static inline void mpc85xx_gpio_set(unsigned int mask, >>> unsigned int dir, unsigned int val) { >>> - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); >>> + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); >>> >>> /* First mask off the unwanted parts of "dir" and "val" */ >>> dir &= mask; >>> @@ -56,7 +56,7 @@ static inline void mpc85xx_gpio_set_high(unsigned >>> int gpios) >>> >>> static inline unsigned int mpc85xx_gpio_get(unsigned int mask) { >>> - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); >>> + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); >>> >>> /* Read the requested values */ >>> return in_be32(&gpio->gpdat) & mask; >>> >> >> Yuantian, >> >> Please go through the base address again. I think some SoCs do use 0xc00 >> offset from 0xF000, for eample P1020, P1023, MPC8572. I only spot checked >> several. >> > > Hi York, > I double checked the offset address of GPIO, I found that the offset addresses of > GPIO on the boards you mentioned above are all changed to 0x0, not 0xc00 according > to the newest RM. > I do found that the offset address is 0xc00 in some old RMs. > You can find the newest RM here: > For MPC8572: http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223503931/226628079/226445024/MPC8572ERM_Rev3_DRAFT1.pdf?nodeid=226438746&vernum=-2 > For p1023: > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223506436/223521755/223743960/P1023RM_Mark-up.pdf?nodeid=229647544&vernum=-2 > for 1020: > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/200815/108253488/223469393/223506436/223522385/224515312/P1020RM_Rev6_Mark-up.pdf?nodeid=228476444&vernum=-2 > > If the offset addresses on these boards were 0xc00, the driver is still wrong, because in that case > The GPIO address should be: CONFIG_SYS_IMMR + 0xc00, not CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00. > (CONFIG_SYS_MPC85xx_GPIO_ADDR == CONFIG_SYS_IMMR + CONFIG_SYS_MPC85xx_GPIO_OFFSET). > > So, please apply this patch, I need the GPIO driver to operate GPIO. > > Please update the commit message to list all SoCs you have confirmed the offset. And don't say "no reason to add 0xc00". The reason was clear when the code was written. Reference manuals said so. York
> > > > Hi York, > > I double checked the offset address of GPIO, I found that the offset > > addresses of GPIO on the boards you mentioned above are all changed to > > 0x0, not 0xc00 according to the newest RM. > > I do found that the offset address is 0xc00 in some old RMs. > > You can find the newest RM here: > > For MPC8572: > > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/ > > 200815/108253488/223469393/223503931/226628079/226445024/MPC8572ERM_Re > > v3_DRAFT1.pdf?nodeid=226438746&vernum=-2 > > For p1023: > > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/ > > 200815/108253488/223469393/223506436/223521755/223743960/P1023RM_Mark- > > up.pdf?nodeid=229647544&vernum=-2 > > for 1020: > > http://compass.freescale.net/livelink/livelink/fetch/2001/3448/223475/ > > 200815/108253488/223469393/223506436/223522385/224515312/P1020RM_Rev6_ > > Mark-up.pdf?nodeid=228476444&vernum=-2 > > > > If the offset addresses on these boards were 0xc00, the driver is > > still wrong, because in that case The GPIO address should be: > CONFIG_SYS_IMMR + 0xc00, not CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00. > > (CONFIG_SYS_MPC85xx_GPIO_ADDR == CONFIG_SYS_IMMR + > CONFIG_SYS_MPC85xx_GPIO_OFFSET). > > > > So, please apply this patch, I need the GPIO driver to operate GPIO. > > > > > > > Please update the commit message to list all SoCs you have confirmed the > offset. And don't say "no reason to add 0xc00". The reason was clear when > the code was written. Reference manuals said so. > > York OK, I will resend this patch once the offset is confirmed by the RM owner. Thanks, Yuantian
diff --git a/arch/powerpc/include/asm/mpc85xx_gpio.h b/arch/powerpc/include/asm/mpc85xx_gpio.h index 3d11884..87bb4a0 100644 --- a/arch/powerpc/include/asm/mpc85xx_gpio.h +++ b/arch/powerpc/include/asm/mpc85xx_gpio.h @@ -20,7 +20,7 @@ static inline void mpc85xx_gpio_set(unsigned int mask, unsigned int dir, unsigned int val) { - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); /* First mask off the unwanted parts of "dir" and "val" */ dir &= mask; @@ -56,7 +56,7 @@ static inline void mpc85xx_gpio_set_high(unsigned int gpios) static inline unsigned int mpc85xx_gpio_get(unsigned int mask) { - ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00); + ccsr_gpio_t *gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR); /* Read the requested values */ return in_be32(&gpio->gpdat) & mask;