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... > 1. 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. > 3. 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. > 4. 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: [EMAIL PROTECTED] ===================================================================== ------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace _______________________________________________ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
