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

[Markus Brunner]

On Friday 28 March 2008, you wrote:
> 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?

no typo, I ment ppc440ep

>
> > 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.

This could explain a lot. I'm already waiting for response from amcc - I let 
you know. If this is an error in the 440ep manual, then there are some in the 
405ep manual, too.

>
> > 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.

This might as well be a bug in the amcc documentation.

> > 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?
>
See below.


> > 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.

Let the compiler fail with an error if an address wasn't defined explicitly 
for this CPU. But I'm going to write a separate email dedicated to this 
subject, so it gets enough attraction.

>
> > 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.

If the values for the registers follow a simple rule as described in the 
detailed register description and not as randomly as in the alternate 
function table I agree with you and it should get fixed.

>
> > 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. 

ppc405ep doesn't have GPIO0_RR2, GPIO0_ISR2H and GPIO0_ISR2L, but the rest 
conforms with the datasheet, doesn't it?. 
I think you got me wrong here.
Here is a example. The numbers in brackets are the offsets for the OSR.
The datasheet of 405ep says the address of the high register (0x08) is 4 bytes 
lower than the low register (0x0C) and the high register is responsible for 
the pins 0-15.
So if gpio_config, with the "broken" code tries to set something for gpio pin 
0, it will try to access a low register (0x0C), because on the other cpus the 
low register is responsible for the pins 0-15. But on 405EP the high register 
is responsible for pins 0-15. So this is wrong.
If gpio_config tries to set pin 16, it will try to set the low register (0x0C) 
plus the offset of 4, which is the tsrh (0x10) register on 405EP.

If gpio_set_chip_configuration tries to set something for gpio pin 0, it will 
try to access the low register, which is defined as 0x08, which is the high 
register of 405ep. Here both errors cancel out each other. The flipped 
address fixes the flipped function.
If gpio_set_chip_configuration tries to set something for gpio pin 16, it will 
try to access the low register which is the high register (0x08) and add an 
offset of 4, so we end up in the low register (0x0C) and again both errors 
cancel out each other.
This seems to be nice on the one side, but we still have wrong register 
addresses defined, which is extremely ugly. 
We could add some defines like OSRL_0_15 and GPIO_OFFS_16_31 to use the same 
code on all processors. This also makes clear, that the meaning of the 
registers isn't always the same.

> 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.

See above.

>
> > 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.

See above.

>
> 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
> =====================================================================