Device treee syntax for expanding mpc5200 gpios

Device treee syntax for expanding mpc5200 gpios

[Jon Smirl]

I need to implement some more of the mpc5200's gpio capabilities.

Right now we have:
gpios = <&gpio_wkup 0 0>;
first cell is index into the bank, and second is unused.

What do we need to fully describe a mpc5200 gpio?

1) open drain: 1 enable
2) interrupt: 0 no int, 1 simple, 2 wakeup, 3 both
3) interrupt type: 0 any transition, 1 rising, 2 falling, 3 pulse

A gpio pin would then have four cells?

Or should all of this be encoded into a single cell so that the
existing two cell syntax can be preserved?

--
Jon Smirl
jonsmirl@gmail.com

Re: Device treee syntax for expanding mpc5200 gpios

[David Gibson]

On Fri, Sep 12, 2008 at 04:43:22PM -0400, Jon Smirl wrote:
> I need to implement some more of the mpc5200's gpio capabilities.
> 
> Right now we have:
> gpios = <&gpio_wkup 0 0>;
> first cell is index into the bank, and second is unused.
> 
> What do we need to fully describe a mpc5200 gpio?
> 
> 1) open drain: 1 enable
> 2) interrupt: 0 no int, 1 simple, 2 wakeup, 3 both
> 3) interrupt type: 0 any transition, 1 rising, 2 falling, 3 pulse

Maybe I'm misunderstanding the situation, but 2 and maybe 3 look more
like configuration than something inherent to the hardware setup.
Couldn't different drivers potentially choose different interrupt
modes depending on their needs?  Remember the device tree describes
the hardware, not how it's used.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Device treee syntax for expanding mpc5200 gpios

[Jon Smirl]

On Sun, Sep 14, 2008 at 10:40 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Sep 12, 2008 at 04:43:22PM -0400, Jon Smirl wrote:
>> I need to implement some more of the mpc5200's gpio capabilities.
>>
>> Right now we have:
>> gpios = <&gpio_wkup 0 0>;
>> first cell is index into the bank, and second is unused.
>>
>> What do we need to fully describe a mpc5200 gpio?
>>
>> 1) open drain: 1 enable
>> 2) interrupt: 0 no int, 1 simple, 2 wakeup, 3 both
>> 3) interrupt type: 0 any transition, 1 rising, 2 falling, 3 pulse
>
> Maybe I'm misunderstanding the situation, but 2 and maybe 3 look more
> like configuration than something inherent to the hardware setup.
> Couldn't different drivers potentially choose different interrupt
> modes depending on their needs?  Remember the device tree describes
> the hardware, not how it's used.

This makes sense. Now that I understand more about the Linux interrupt
system I see that there is an API for setting interrupt type - #3.

I had type in there because I was copying from the existing interrupt node:
interrupts = <0x1 0xe 0x0>;
cell 1 = class critical, normal, sdma
cell 2 = number
cell 3 = level

Cell 3 is probably not needed in the existing definitions.

GPIO wake up interrupts come in on two different hardware interrupts
depending if they are normal or wake up.

		gpio_wkup: gpio-wkup@c00 {
			compatible = "fsl,mpc5200b-gpio-wkup","fsl,mpc5200-gpio-wkup";
			reg = <0xc00 0x40>;
			interrupts = <0x1 0x8 0x0 0x0 0x3 0x0>;
			interrupt-parent = <&mpc5200_pic>;
			gpio-controller;
			#gpio-cells = <4>;
		};

There's no existing API in the GPIO system to describe this case. You
have to decide if you want an interrupt to be able to wake the CPU up
from sleep mode. If you want the interrupt to do this, it will trigger
int 3 if the CPU is asleep, otherwise it triggers int 8. Should these
pins have two virqs? or should these two vectors be hidden from the
user of the interrupt?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Device treee syntax for expanding mpc5200 gpios

[Anton Vorontsov]

On Fri, Sep 12, 2008 at 04:43:22PM -0400, Jon Smirl wrote:
> I need to implement some more of the mpc5200's gpio capabilities.
> 
> Right now we have:
> gpios = <&gpio_wkup 0 0>;
> first cell is index into the bank, and second is unused.
> 
> What do we need to fully describe a mpc5200 gpio?
> 
> 1) open drain: 1 enable

This can be described in the gpios = <> property via second cell.

> 2) interrupt: 0 no int, 1 simple, 2 wakeup, 3 both
> 3) interrupt type: 0 any transition, 1 rising, 2 falling, 3 pulse

These we describe via interrupts = <> proprty, so..

> A gpio pin would then have four cells?
> 
> Or should all of this be encoded into a single cell so that the
> existing two cell syntax can be preserved?

It is still possible to preserve two cells syntax and encode
irq-specific information there, but this isn't pretty.

I think that interrupts and their information could be encoded in
the "interrupts" property... something like (QE example):

qe_pio_e: gpio-controller@1460 {
	#gpio-cells = <2>;
	compatible = "fsl,mpc8360-qe-pario-bank",
		     "fsl,mpc8323-qe-pario-bank";
	reg = <0x1460 0x18>;
	interrupts = <0 1 2 3 4 5 6 7 8 9 ...>;
	interrupt-parent = <&qeic>;
	gpio-controller;
};

qeic: interrupt-controller@80 {
	#address-cells = <0>;
	#interrupt-cells = <1>;
	compatible = "fsl,qe-ic";
	interrupt-controller;
	reg = <0x80 0x80>;
	big-endian;
	interrupts = <32 8 33 8>;
	interrupt-parent = <&ipic>;
};

That way it is easy to implement gpio_to_irq() call, and we won't
need to bother with interrupt-specific information in the gpios = <>.

It also correctly describes the hardware, there are really two
distinct hw units: interrupt controller and gpio controller.

Also, I think that gpio_to_irq() call would be a rare case, since
you can always specify interrupts directly, i.e.

device {
	gpios = <&qe_pio_e 0 0>;
	interrupts = <0>;
	interrupt-parent = <&qeic>;
};

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: Device treee syntax for expanding mpc5200 gpios

[Jon Smirl]

On Mon, Sep 15, 2008 at 9:02 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Fri, Sep 12, 2008 at 04:43:22PM -0400, Jon Smirl wrote:
>> I need to implement some more of the mpc5200's gpio capabilities.
>>
>> Right now we have:
>> gpios = <&gpio_wkup 0 0>;
>> first cell is index into the bank, and second is unused.
>>
>> What do we need to fully describe a mpc5200 gpio?
>>
>> 1) open drain: 1 enable
>
> This can be described in the gpios = <> property via second cell.
>
>> 2) interrupt: 0 no int, 1 simple, 2 wakeup, 3 both
>> 3) interrupt type: 0 any transition, 1 rising, 2 falling, 3 pulse
>
> These we describe via interrupts = <> proprty, so..
>
>> A gpio pin would then have four cells?
>>
>> Or should all of this be encoded into a single cell so that the
>> existing two cell syntax can be preserved?
>
> It is still possible to preserve two cells syntax and encode
> irq-specific information there, but this isn't pretty.
>
> I think that interrupts and their information could be encoded in
> the "interrupts" property... something like (QE example):
>
> qe_pio_e: gpio-controller@1460 {
>        #gpio-cells = <2>;
>        compatible = "fsl,mpc8360-qe-pario-bank",
>                     "fsl,mpc8323-qe-pario-bank";
>        reg = <0x1460 0x18>;
>        interrupts = <0 1 2 3 4 5 6 7 8 9 ...>;
>        interrupt-parent = <&qeic>;
>        gpio-controller;
> };
>
> qeic: interrupt-controller@80 {
>        #address-cells = <0>;
>        #interrupt-cells = <1>;
>        compatible = "fsl,qe-ic";
>        interrupt-controller;
>        reg = <0x80 0x80>;
>        big-endian;
>        interrupts = <32 8 33 8>;
>        interrupt-parent = <&ipic>;
> };
>
> That way it is easy to implement gpio_to_irq() call, and we won't
> need to bother with interrupt-specific information in the gpios = <>.

The easiest way to implement gpio_to_irq() is to arrange things so
that the gpio number used internally by linux is the same as the virq
number.  I did this in the code I posted in the other thread,

In the mpc5200 the level sensitivity of each gpio interrupt can be set
independently, but there is a kernel API for doing it.

By doing these two things none of the interrupt info needs to be
encoded into the device tree.


>
> It also correctly describes the hardware, there are really two
> distinct hw units: interrupt controller and gpio controller.
>
> Also, I think that gpio_to_irq() call would be a rare case, since
> you can always specify interrupts directly, i.e.
>
> device {
>        gpios = <&qe_pio_e 0 0>;
>        interrupts = <0>;
>        interrupt-parent = <&qeic>;
> };
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>



-- 
Jon Smirl
jonsmirl@gmail.com