On Thursday 27 March 2008, M B wrote:
I found some bugs for the gpio setup for ppc405ep and was about to fix them. After i fixed them (for 405ep) I realised that it's rather impossible to have a function like gpio_set_chip_configuration to setup the gpios for all ppc4xx, without turning it into ifdef hell.
I definitely don't hope so. Till now it works quite well. And currently most 440 variants and 405EX are supported, IIRC.
Even worse you can't even have one which only takes care of the ppc440ep.
ppc405ep?
Maybe I misunderstood the datasheet of the ppc440ep, so please correct me if I'm wrong. According to Table 29-6 on page 687 which lists the registers for the alternate1 function the tsrl bits for gpio 12 have to be 01, but for gpio 13 they have to be 00. Both are inputs and both are alt1, so I don't see how to find a rule to decide what value has to be set.
I'm pretty sure that this is a documentation fault. Please contact AMCC support on this.
It's no big deal to have such a function for the ppc405ep and some others, but it should be obvious to see for what processors this function was tested and should fail with #error else.
The Bugs I've found:
Now, that's a list...
- The addresses of the select registers (input/output/3state), which
are defined in gpio.h of the ppc405ep are wrong. The address of the low register is 4 bytes higher than the high register.
I think you spotted an error in U-Boot here. Seems like this is defined incorrectly for some 405 variants. Please take a look at the 405EX defines. They are correct.
Furthermore GPIOs 0-16 are managed by the high register. Because the meaning and the address of the registers have changed, gpios 0-16 can be configured, which makes spotting this bug more difficult. 2. config_gpio assumes the address of the high register to be 4 bytes above the low register, which isn't the case for 405ep.
Please see above.
- gpio_set_chip_configuration also assumes the address of the high
register to be 4 bytes above the low register. 3.1 Furthermore the 3state select registers will get set, which should always be 00 for the 405ep. 3.2 The TCR register bits will only get set for gpio_out && gpio_sel, but they must be set for all outputs on 405ep.
Not sure here. I'll have to look at this when I have more time.
- The taihu board (405ep) uses gpio_set_chip_configuration.
Right. And it seems to work. Or did I miss something here?
This bugs probably could have been avoided if the addresses of the registers would have been defined with #else #error, at least we would have a scapegoat now, who defined the wrong addresses. So why not add this rule to the coding style?
Not sure what you mean with this.
Because no 405ep board uses config_gpio and only taihu uses gpio_set_chip_configuration, I think it would be best to undef this error prone functions and registers for 405ep.
I don't think so.
The registers defined in ppc405.h are correct,
No, I don't think they are correct. These are the defines for 405EP (and 405GP btw) in ppc405.h (I know this file is hell):
#define GPIO_BASE 0xEF600700 #define GPIO0_OR (GPIO_BASE+0x0) #define GPIO0_TCR (GPIO_BASE+0x4) #define GPIO0_OSRH (GPIO_BASE+0x8) #define GPIO0_OSRL (GPIO_BASE+0xC) #define GPIO0_TSRH (GPIO_BASE+0x10) #define GPIO0_TSRL (GPIO_BASE+0x14) #define GPIO0_ODR (GPIO_BASE+0x18) #define GPIO0_IR (GPIO_BASE+0x1C) #define GPIO0_RR1 (GPIO_BASE+0x20) #define GPIO0_RR2 (GPIO_BASE+0x24) #define GPIO0_ISR1H (GPIO_BASE+0x30) #define GPIO0_ISR1L (GPIO_BASE+0x34) #define GPIO0_ISR2H (GPIO_BASE+0x38) #define GPIO0_ISR2L (GPIO_BASE+0x3C)
And as you pointed out above, this is incorrect. High and low is swapped here. But these defines are not used in gpio_set_chip_configuration(). So it should not matter for this function. But they *are* used in gpio_config(). And that's the reason why this functions can't be used correctly on 405EX/GR currently.
so most boards are o.k. anyway. I would have a patch for the registers and config_gpio, so if you don't agree with my solution I can submit it.
I suggest to fix gpio_config() to use the same GPIO register access as done in gpio_set_chip_configuration(). Then this function should be usable for those PPC variants too.
Please let me know if you think I missed something. Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================