[U-Boot-Users] ppc4xx: gpio setup broken for ppc405ep

[Stefan Roese <sr@denx.de>]

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: office at denx.de
=====================================================================