[PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Martyn Welch]

Currently the irqs for the i8042, which historically provides keyboard and
mouse (aux) support, is hardwired in the driver rather than parsing the
dts.  This patch modifies the powerpc legacy IO code to attempt to parse
the device tree for this information, failing back to the hardcoded values
if it fails.

Signed-off-by: Martyn Welch <martyn.welch@ge.com>
---

v2: This patch no longer requires the DTS files to be modified, reading the
interrupts from the current location as suggested by Grant.

v3: Code compacted as suggested by Grant.

 arch/powerpc/kernel/setup-common.c |   13 +++++++++++++
 drivers/input/serio/i8042-io.h     |    5 +++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 48f0a00..3d169bb 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -94,6 +94,10 @@ struct screen_info screen_info = {
 	.orig_video_points = 16
 };
 
+/* Variables required to store legacy IO irq routing */
+int of_i8042_kbd_irq;
+int of_i8042_aux_irq;
+
 #ifdef __DO_IRQ_CANON
 /* XXX should go elsewhere eventually */
 int ppc_do_canonicalize_irqs;
@@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
 			np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
 		if (np) {
 			parent = of_get_parent(np);
+
+			of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
+			if (!of_i8042_kbd_irq)
+				of_i8042_kbd_irq = 1;
+
+			of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
+			if (!of_i8042_aux_irq)
+				of_i8042_aux_irq = 12;
+
 			of_node_put(np);
 			np = parent;
 			break;
diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h
index 847f4aa..5d48bb6 100644
--- a/drivers/input/serio/i8042-io.h
+++ b/drivers/input/serio/i8042-io.h
@@ -27,6 +27,11 @@
 #include <asm/irq.h>
 #elif defined(CONFIG_SH_CAYMAN)
 #include <asm/irq.h>
+#elif defined(CONFIG_PPC)
+extern int of_i8042_kbd_irq;
+extern int of_i8042_aux_irq;
+# define I8042_KBD_IRQ  of_i8042_kbd_irq
+# define I8042_AUX_IRQ  of_i8042_aux_irq
 #else
 # define I8042_KBD_IRQ	1
 # define I8042_AUX_IRQ	12


--
Martyn Welch (Principal Software Engineer)   |   Registered in England and
GE Intelligent Platforms                     |   Wales (3828642) at 100
T +44(0)127322748                            |   Barbirolli Square, Manchester,
E martyn.welch@ge.com                        |   M2 3AB  VAT:GB 927559189

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Grant Likely]

On Tue, May 25, 2010 at 2:09 AM, Martyn Welch <martyn.welch@ge.com> wrote:
> Currently the irqs for the i8042, which historically provides keyboard an=
d
> mouse (aux) support, is hardwired in the driver rather than parsing the
> dts. =A0This patch modifies the powerpc legacy IO code to attempt to pars=
e
> the device tree for this information, failing back to the hardcoded value=
s
> if it fails.
>
> Signed-off-by: Martyn Welch <martyn.welch@ge.com>
> ---
>
> v2: This patch no longer requires the DTS files to be modified, reading t=
he
> interrupts from the current location as suggested by Grant.
>
> v3: Code compacted as suggested by Grant.
>
> =A0arch/powerpc/kernel/setup-common.c | =A0 13 +++++++++++++
> =A0drivers/input/serio/i8042-io.h =A0 =A0 | =A0 =A05 +++++
> =A02 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/set=
up-common.c
> index 48f0a00..3d169bb 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -94,6 +94,10 @@ struct screen_info screen_info =3D {
> =A0 =A0 =A0 =A0.orig_video_points =3D 16
> =A0};
>
> +/* Variables required to store legacy IO irq routing */
> +int of_i8042_kbd_irq;
> +int of_i8042_aux_irq;
> +
> =A0#ifdef __DO_IRQ_CANON
> =A0/* XXX should go elsewhere eventually */
> =A0int ppc_do_canonicalize_irqs;
> @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0np =3D of_find_compatible_=
node(NULL, NULL, "pnpPNP,f03");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (np) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0parent =3D of_get_parent(n=
p);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_i8042_kbd_irq =3D irq_of=
_parse_and_map(parent, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!of_i8042_kbd_irq)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_i8042_kb=
d_irq =3D 1;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_i8042_aux_irq =3D irq_of=
_parse_and_map(parent, 1);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!of_i8042_aux_irq)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_i8042_au=
x_irq =3D 12;
> +

The patch looks okay to me.

BTW, where is the i8042 binding documented?  Ben, is this location of
the kbd/mouse irq historical, or is it just something that we happened
to get when the .dts files were first created?  Having the irq
specified directly in the kbd or aux nodes would make a lot more
sense, and if this isn't something already nailed down, then it
probably does make sense to move the irq specification, fall back to
the parent node to still support older trees, and with the hard coded
irq numbers as the last resort.

Cheers,
g.

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Mitch Bradley]

Grant Likely wrote:
> On Tue, May 25, 2010 at 2:09 AM, Martyn Welch <martyn.welch@ge.com> wrote:
>   
>> Currently the irqs for the i8042, which historically provides keyboard and
>> mouse (aux) support, is hardwired in the driver rather than parsing the
>> dts.  This patch modifies the powerpc legacy IO code to attempt to parse
>> the device tree for this information, failing back to the hardcoded values
>> if it fails.
>>
>> Signed-off-by: Martyn Welch <martyn.welch@ge.com>
>> ---
>>
>> v2: This patch no longer requires the DTS files to be modified, reading the
>> interrupts from the current location as suggested by Grant.
>>
>> v3: Code compacted as suggested by Grant.
>>
>>  arch/powerpc/kernel/setup-common.c |   13 +++++++++++++
>>  drivers/input/serio/i8042-io.h     |    5 +++++
>>  2 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
>> index 48f0a00..3d169bb 100644
>> --- a/arch/powerpc/kernel/setup-common.c
>> +++ b/arch/powerpc/kernel/setup-common.c
>> @@ -94,6 +94,10 @@ struct screen_info screen_info = {
>>        .orig_video_points = 16
>>  };
>>
>> +/* Variables required to store legacy IO irq routing */
>> +int of_i8042_kbd_irq;
>> +int of_i8042_aux_irq;
>> +
>>  #ifdef __DO_IRQ_CANON
>>  /* XXX should go elsewhere eventually */
>>  int ppc_do_canonicalize_irqs;
>> @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
>>                        np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
>>                if (np) {
>>                        parent = of_get_parent(np);
>> +
>> +                       of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
>> +                       if (!of_i8042_kbd_irq)
>> +                               of_i8042_kbd_irq = 1;
>> +
>> +                       of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
>> +                       if (!of_i8042_aux_irq)
>> +                               of_i8042_aux_irq = 12;
>> +
>>     
>
> The patch looks okay to me.
>
> BTW, where is the i8042 binding documented?  Ben, is this location of
> the kbd/mouse irq historical,

i8042 is, of course, the legacy PC keyboard interrupt controller, and 
the IRQs that it generates on behalf of its attached keyboard and mouse 
have always been 1 and 12 on the PC platforms that drive the hardware 
designs of junk I/O chips.  By the time that PowerPC came on the market, 
the i8042 functionality was always implemented as a subsystem within a 
much larger "SuperIO" chip, which often included the (legacy PC) 
interrupt controller functionality.  Those SuperIO chips let you move 
the IRQ numbers around for some of the included devices, but I don't 
remember whether or not you could move the IRQs for the keyboard and 
mouse.  Even if you could, "nobody" ever did it, because people were so 
accustomed to 1 and 12 being keyboard and mouse that changing them would 
just cause too much confusion.  There was also the issue of setting the 
edge/level and polarity correctly for the various IRQs.  Moving the 
kbd/mouse IRQs would have a ripple effect on other old-and-dusty code 
that nobody wanted to touch.

The design of PReP (PowerPC Reference Platform) was amusing to watch.  
Apple was wanting to make it into a 68K Macintosh I/O system with a 
PowerPC grafted in, while IBM wanted a conventional PC with a PowerPC 
grafted in.  IBM stacked the committee and basically got what they 
wanted - a PC with a different CPU chip.  At the Comdex show where they 
rolled out the PReP spec, Apple had their own announcement to make - 
Apple wasn't going to use PReP.  So IBM's power play backfired.

The PReP committee (consisting of representatives from IBM, Apple, 
Motorola, and a few other bit players like myself) then went into panic 
mode, and came out with CHRP.  CHRP was PReP with a legacy Macintosh I/O 
system bolted onto the side.  So you had the worst of both worlds.  In 
the PReP mode, you had PC-style I/O like IDE, i8042, 8259 interrupt 
controller, etc.  In the Mac mode, you had SCSI (using a horrible 
ancient programming model that nobody else was using anymore), ADB, and 
an OpenPIC interrupt controller.  The chipsets had to support both sets.

The final shoe dropped 18 months later, when Steve Jobs re-took Apple 
and announced that Apple was not in fact going to license MacOS for use 
on third-party CHRP machines.

The original binding documents for the PC-style I/O system were driven 
from the IBM side.  I may have written some of the text (or maybe not; I 
forget), but it was IBM who stipulated how it was going to work.


>  or is it just something that we happened
> to get when the .dts files were first created?  Having the irq
> specified directly in the kbd or aux nodes would make a lot more
> sense, and if this isn't something already nailed down, then it
> probably does make sense to move the irq specification, fall back to
> the parent node to still support older trees, and with the hard coded
> irq numbers as the last resort.
>
> Cheers,
> g.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
>
>   

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Benjamin Herrenschmidt]

> The patch looks okay to me.
> 
> BTW, where is the i8042 binding documented?  Ben, is this location of
> the kbd/mouse irq historical, or is it just something that we happened
> to get when the .dts files were first created?  Having the irq
> specified directly in the kbd or aux nodes would make a lot more
> sense, and if this isn't something already nailed down, then it
> probably does make sense to move the irq specification, fall back to
> the parent node to still support older trees, and with the hard coded
> irq numbers as the last resort.

That's just ISA crap. It should be in the existing OF bindings.

Cheers,
Ben.

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Benjamin Herrenschmidt]

O
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 48f0a00..3d169bb 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -94,6 +94,10 @@ struct screen_info screen_info = {
>  	.orig_video_points = 16
>  };
>  
> +/* Variables required to store legacy IO irq routing */
> +int of_i8042_kbd_irq;
> +int of_i8042_aux_irq;

Is there a reasonable ifdef to use for the above or we don't care ?

>  #ifdef __DO_IRQ_CANON
>  /* XXX should go elsewhere eventually */
>  int ppc_do_canonicalize_irqs;
> @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
>  			np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
>  		if (np) {
>  			parent = of_get_parent(np);
> +
> +			of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
> +			if (!of_i8042_kbd_irq)
> +				of_i8042_kbd_irq = 1;
> +
> +			of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
> +			if (!of_i8042_aux_irq)
> +				of_i8042_aux_irq = 12;
> +
>  			of_node_put(np);
>  			np = parent;
>  			break;
> diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h
> index 847f4aa..5d48bb6 100644
> --- a/drivers/input/serio/i8042-io.h
> +++ b/drivers/input/serio/i8042-io.h
> @@ -27,6 +27,11 @@
>  #include <asm/irq.h>
>  #elif defined(CONFIG_SH_CAYMAN)
>  #include <asm/irq.h>
> +#elif defined(CONFIG_PPC)
> +extern int of_i8042_kbd_irq;
> +extern int of_i8042_aux_irq;
> +# define I8042_KBD_IRQ  of_i8042_kbd_irq
> +# define I8042_AUX_IRQ  of_i8042_aux_irq
>  #else
>  # define I8042_KBD_IRQ	1
>  # define I8042_AUX_IRQ	12

Now while that works, I do tend to dislike global variables like that.

_Maybe_ a better approach would be to have those #define resolve to
functions:

#define I8042_KBD_IRQ	of_find_i8042_kbd_irq()

Or something like that ?

That means ending up having 2 functions which more/less reproduce the
loop to find the driver in the device-tree but that's a minor
inconvenience.

Now, maybe the variables are less bloat here. What do you think ? Which
way do you prefer ?

Cheers,
Ben.

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Martyn Welch]

Benjamin Herrenschmidt wrote:
> O
>   
>> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
>> index 48f0a00..3d169bb 100644
>> --- a/arch/powerpc/kernel/setup-common.c
>> +++ b/arch/powerpc/kernel/setup-common.c
>> @@ -94,6 +94,10 @@ struct screen_info screen_info = {
>>  	.orig_video_points = 16
>>  };
>>  
>> +/* Variables required to store legacy IO irq routing */
>> +int of_i8042_kbd_irq;
>> +int of_i8042_aux_irq;
>>     
>
> Is there a reasonable ifdef to use for the above or we don't care ?
>
>   
>>  #ifdef __DO_IRQ_CANON
>>  /* XXX should go elsewhere eventually */
>>  int ppc_do_canonicalize_irqs;
>> @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
>>  			np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
>>  		if (np) {
>>  			parent = of_get_parent(np);
>> +
>> +			of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
>> +			if (!of_i8042_kbd_irq)
>> +				of_i8042_kbd_irq = 1;
>> +
>> +			of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
>> +			if (!of_i8042_aux_irq)
>> +				of_i8042_aux_irq = 12;
>> +
>>  			of_node_put(np);
>>  			np = parent;
>>  			break;
>> diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h
>> index 847f4aa..5d48bb6 100644
>> --- a/drivers/input/serio/i8042-io.h
>> +++ b/drivers/input/serio/i8042-io.h
>> @@ -27,6 +27,11 @@
>>  #include <asm/irq.h>
>>  #elif defined(CONFIG_SH_CAYMAN)
>>  #include <asm/irq.h>
>> +#elif defined(CONFIG_PPC)
>> +extern int of_i8042_kbd_irq;
>> +extern int of_i8042_aux_irq;
>> +# define I8042_KBD_IRQ  of_i8042_kbd_irq
>> +# define I8042_AUX_IRQ  of_i8042_aux_irq
>>  #else
>>  # define I8042_KBD_IRQ	1
>>  # define I8042_AUX_IRQ	12
>>     
>
> Now while that works, I do tend to dislike global variables like that.
>
> _Maybe_ a better approach would be to have those #define resolve to
> functions:
>
> #define I8042_KBD_IRQ	of_find_i8042_kbd_irq()
>
> Or something like that ?
>
> That means ending up having 2 functions which more/less reproduce the
> loop to find the driver in the device-tree but that's a minor
> inconvenience.
>
> Now, maybe the variables are less bloat here. What do you think ? Which
> way do you prefer ?
>
>   

Personally, I'm happy either way. If you'd prefer I implement these as
functions, I can't quickly see why that wouldn't work. I thought using
variables would be the least disruptive.

I'm also happy to change it so that the irqs are specified in the kbd
and aux nodes (I agree it would make much more sense and was how the
first revision of this patch worked).

Either way, my preferred solution is that contained in this patch,
unless those with more experience agree otherwise... ;-)

Martyn

> Cheers,
> Ben.
>
>
>   


-- 
Martyn Welch (Principal Software Engineer)   |   Registered in England and
GE Intelligent Platforms                     |   Wales (3828642) at 100
T +44(0)127322748                            |   Barbirolli Square, Manchester,
E martyn.welch@ge.com                        |   M2 3AB  VAT:GB 927559189

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Dmitry Torokhov]

On Tue, Jun 01, 2010 at 01:41:59PM +0100, Martyn Welch wrote:
> Benjamin Herrenschmidt wrote:
> > O
> >   
> >> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> >> index 48f0a00..3d169bb 100644
> >> --- a/arch/powerpc/kernel/setup-common.c
> >> +++ b/arch/powerpc/kernel/setup-common.c
> >> @@ -94,6 +94,10 @@ struct screen_info screen_info = {
> >>  	.orig_video_points = 16
> >>  };
> >>  
> >> +/* Variables required to store legacy IO irq routing */
> >> +int of_i8042_kbd_irq;
> >> +int of_i8042_aux_irq;
> >>     
> >
> > Is there a reasonable ifdef to use for the above or we don't care ?
> >
> >   
> >>  #ifdef __DO_IRQ_CANON
> >>  /* XXX should go elsewhere eventually */
> >>  int ppc_do_canonicalize_irqs;
> >> @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
> >>  			np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
> >>  		if (np) {
> >>  			parent = of_get_parent(np);
> >> +
> >> +			of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
> >> +			if (!of_i8042_kbd_irq)
> >> +				of_i8042_kbd_irq = 1;
> >> +
> >> +			of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
> >> +			if (!of_i8042_aux_irq)
> >> +				of_i8042_aux_irq = 12;
> >> +
> >>  			of_node_put(np);
> >>  			np = parent;
> >>  			break;
> >> diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h
> >> index 847f4aa..5d48bb6 100644
> >> --- a/drivers/input/serio/i8042-io.h
> >> +++ b/drivers/input/serio/i8042-io.h
> >> @@ -27,6 +27,11 @@
> >>  #include <asm/irq.h>
> >>  #elif defined(CONFIG_SH_CAYMAN)
> >>  #include <asm/irq.h>
> >> +#elif defined(CONFIG_PPC)
> >> +extern int of_i8042_kbd_irq;
> >> +extern int of_i8042_aux_irq;
> >> +# define I8042_KBD_IRQ  of_i8042_kbd_irq
> >> +# define I8042_AUX_IRQ  of_i8042_aux_irq
> >>  #else
> >>  # define I8042_KBD_IRQ	1
> >>  # define I8042_AUX_IRQ	12
> >>     
> >
> > Now while that works, I do tend to dislike global variables like that.
> >
> > _Maybe_ a better approach would be to have those #define resolve to
> > functions:
> >
> > #define I8042_KBD_IRQ	of_find_i8042_kbd_irq()
> >
> > Or something like that ?
> >
> > That means ending up having 2 functions which more/less reproduce the
> > loop to find the driver in the device-tree but that's a minor
> > inconvenience.
> >
> > Now, maybe the variables are less bloat here. What do you think ? Which
> > way do you prefer ?
> >
> >   
> 
> Personally, I'm happy either way. If you'd prefer I implement these as
> functions, I can't quickly see why that wouldn't work. I thought using
> variables would be the least disruptive.

FYI: i8042 core expects I8042_{KBD|AUX}_IRQ to be integers, however it
is certainly fixable...

-- 
Dmitry

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Benjamin Herrenschmidt]

On Tue, 2010-06-01 at 23:26 -0700, Dmitry Torokhov wrote:
> 
> FYI: i8042 core expects I8042_{KBD|AUX}_IRQ to be integers, however it
> is certainly fixable...

Let's not bother.

Cheers,
Ben.