Pegasos i8042 broken again

Pegasos i8042 broken again

[pacman]

Pegasos has no keyboard again. I blame commit
540c6c392f01887dcc96bef0a41e63e6c1334f01, which tries to find i8042 IRQs in
the device-tree but doesn't fall back to the old hardcoded 1 and 12 in all
failure cases.

Specifically, the case where the device-tree contains nothing matching
pnpPNP,303 or pnpPNP,f03 doesn't seem to be handled well. It sort of falls
through to the old code, but leaves the IRQs set to 0.

The last time something like this happened, I submitted a patch:
http://lists.ozlabs.org/pipermail/linuxppc-dev/2007-July/039988.html
which got committed, but afterward I was scolded for working around a bug
instead of fixing it in nvramrc.

This time I just won't send my workaround patch, at least until it's decided
that the kernel should be made to understand the device-tree as is.

If it's decided instead that the firmware should be patched... well I just
don't feel comfortable inventing my own patch for nvramrc, since it's written
in a language I don't know and presumably could brick the machine if I get it
wrong. Also I'm not even sure what the kernel is expecting to find there.

-- 
Alan Curry

Re: Pegasos i8042 broken again

[Benjamin Herrenschmidt]

On Sat, 2010-10-09 at 20:37 -0500, pacman@kosh.dhis.org wrote:
> Pegasos has no keyboard again. I blame commit
> 540c6c392f01887dcc96bef0a41e63e6c1334f01, which tries to find i8042 IRQs in
> the device-tree but doesn't fall back to the old hardcoded 1 and 12 in all
> failure cases.
> 
> Specifically, the case where the device-tree contains nothing matching
> pnpPNP,303 or pnpPNP,f03 doesn't seem to be handled well. It sort of falls
> through to the old code, but leaves the IRQs set to 0.
> 
> The last time something like this happened, I submitted a patch:
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2007-July/039988.html
> which got committed, but afterward I was scolded for working around a bug
> instead of fixing it in nvramrc.
> 
> This time I just won't send my workaround patch, at least until it's decided
> that the kernel should be made to understand the device-tree as is.
> 
> If it's decided instead that the firmware should be patched... well I just
> don't feel comfortable inventing my own patch for nvramrc, since it's written
> in a language I don't know and presumably could brick the machine if I get it
> wrong. Also I'm not even sure what the kernel is expecting to find there. 

Those things really suck. They absolutely refuse to fix their FW for
reasons I never quite managed to figure out.

At this stage, I'd say the best is to add yet another pegasos workaround
in prom_init that adds the missing compatible property.

Cheers,
Ben.

Re: Pegasos i8042 broken again

[Gerhard Pircher]

On Sat, 2010-10-09 at 20:37 -0500, pacman@kosh.dhis.org wrote:
> Pegasos has no keyboard again. I blame commit
> 540c6c392f01887dcc96bef0a41e63e6c1334f01, which tries to find i8042 IRQs
> inthe device-tree but doesn't fall back to the old hardcoded 1 and 12 in
> all failure cases.
> 
> Specifically, the case where the device-tree contains nothing matching
> pnpPNP,303 or pnpPNP,f03 doesn't seem to be handled well. It sort of
> falls through to the old code, but leaves the IRQs set to 0.
> 
> The last time something like this happened, I submitted a patch:
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2007-July/039988.html
> which got committed, but afterward I was scolded for working around a
> bug instead of fixing it in nvramrc.
> 
> This time I just won't send my workaround patch, at least until it's
> decided that the kernel should be made to understand the device-tree as
> is.
> If it's decided instead that the firmware should be patched... well I
> just don't feel comfortable inventing my own patch for nvramrc, since
> it's written in a language I don't know and presumably could brick the
> machine if I get it wrong. Also I'm not even sure what the kernel is
> expecting to find there.

Is this a Pegasos 1 or Pegasos 2? I'm just curious, because some Pegasos 1
users told me that newer Linux kernels don't even boot on their machines.

regards,

Gerhard
-- 
Neu: GMX De-Mail - Einfach wie E-Mail, sicher wie ein Brief!  
Jetzt De-Mail-Adresse reservieren: http://portal.gmx.net/de/go/demail

Re: Pegasos i8042 broken again

[pacman]

Benjamin Herrenschmidt writes:
> 
> Those things really suck. They absolutely refuse to fix their FW for
> reasons I never quite managed to figure out.

The last time around, they did release a firmware patch (pegasos-dts-20071018)
to fix up the device tree enough to satisfy the kernel. Now that the kernel
has become dissatisfied again, maybe another patch will appear.

> 
> At this stage, I'd say the best is to add yet another pegasos workaround
> in prom_init that adds the missing compatible property.

This one would be more complex than the other fixes in prom_init. It's not
just the compatible property that's missing. The 8042 node in the device tree
has no children.

-- 
Alan Curry

Re: Pegasos i8042 broken again

[Gabriel Paubert]

On Sun, Oct 10, 2010 at 06:35:47PM +1100, Benjamin Herrenschmidt wrote:
> On Sat, 2010-10-09 at 20:37 -0500, pacman@kosh.dhis.org wrote:
> > Pegasos has no keyboard again. I blame commit
> > 540c6c392f01887dcc96bef0a41e63e6c1334f01, which tries to find i8042 IRQs in
> > the device-tree but doesn't fall back to the old hardcoded 1 and 12 in all
> > failure cases.
> > 
> > Specifically, the case where the device-tree contains nothing matching
> > pnpPNP,303 or pnpPNP,f03 doesn't seem to be handled well. It sort of falls
> > through to the old code, but leaves the IRQs set to 0.
> > 
> > The last time something like this happened, I submitted a patch:
> > http://lists.ozlabs.org/pipermail/linuxppc-dev/2007-July/039988.html
> > which got committed, but afterward I was scolded for working around a bug
> > instead of fixing it in nvramrc.
> > 
> > This time I just won't send my workaround patch, at least until it's decided
> > that the kernel should be made to understand the device-tree as is.
> > 
> > If it's decided instead that the firmware should be patched... well I just
> > don't feel comfortable inventing my own patch for nvramrc, since it's written
> > in a language I don't know and presumably could brick the machine if I get it
> > wrong. Also I'm not even sure what the kernel is expecting to find there. 
> 
> Those things really suck. They absolutely refuse to fix their FW for
> reasons I never quite managed to figure out.
> 
> At this stage, I'd say the best is to add yet another pegasos workaround
> in prom_init that adds the missing compatible property.

Ok, I got fed up about it. The patch referred above is obviously wrong since
it leaves interrupts at 0 when a device_type or name of 8042 is found,
so what about the following? 

I can ship it with a signed-off-by and proper comments a bit later if people agree.

Compiled and tested, otherwise I couldn't even type this message :-)

	Regards,
	Gabriel

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 9d4882a..06865ac 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -599,6 +599,10 @@ int check_legacy_ioport(unsigned long base_port)
 		 * name instead */
 		if (!np)
 			np = of_find_node_by_name(NULL, "8042");
+		if (np) {
+			of_i8042_kbd_irq = 1;
+			of_i8042_aux_irq = 12;
+		}
 		break;
 	case FDC_BASE: /* FDC1 */
 		np = of_find_node_by_type(NULL, "fdc");

Re: Pegasos i8042 broken again

[Benjamin Herrenschmidt]

> Ok, I got fed up about it. The patch referred above is obviously wrong since
> it leaves interrupts at 0 when a device_type or name of 8042 is found,
> so what about the following? 
> 
> I can ship it with a signed-off-by and proper comments a bit later if people agree.
> 
> Compiled and tested, otherwise I couldn't even type this message :-)

Shouldn't that be a pegasos specific quirk in chrp/setup.c ?

Cheers,
Ben.

> 	Regards,
> 	Gabriel
> 
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 9d4882a..06865ac 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -599,6 +599,10 @@ int check_legacy_ioport(unsigned long base_port)
>  		 * name instead */
>  		if (!np)
>  			np = of_find_node_by_name(NULL, "8042");
> +		if (np) {
> +			of_i8042_kbd_irq = 1;
> +			of_i8042_aux_irq = 12;
> +		}
>  		break;
>  	case FDC_BASE: /* FDC1 */
>  		np = of_find_node_by_type(NULL, "fdc");

Re: Pegasos i8042 broken again

[Gabriel Paubert]

On Tue, Apr 05, 2011 at 08:28:50AM +1000, Benjamin Herrenschmidt wrote:
> 
> > Ok, I got fed up about it. The patch referred above is obviously wrong since
> > it leaves interrupts at 0 when a device_type or name of 8042 is found,
> > so what about the following? 
> > 
> > I can ship it with a signed-off-by and proper comments a bit later if people agree.
> > 
> > Compiled and tested, otherwise I couldn't even type this message :-)
> 
> Shouldn't that be a pegasos specific quirk in chrp/setup.c ?

In this case I don't think so: 
1) The code looks for the pnp id for the 8042 controllers and tries
  to fill the interrupt fields from OF/DT, but falls back to defaults 
  1 and 12 if it does not get them.

2) Then it tries to find device_type of 8042 or node name of 8042
  and returns success if it finds them, but in this case leaves
  the interrupt numbers at zero. Note that in this case the code does 
  not even attempt to get interrupt information from OF/DT, this looks
  like a fallback case where filling with defaults seems reasonable.
  Actually, it is not filling with defaults which seems wrong since
  the other case always provides interrupt information.

	Regards,
	Gabriel
> > 
> > diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> > index 9d4882a..06865ac 100644
> > --- a/arch/powerpc/kernel/setup-common.c
> > +++ b/arch/powerpc/kernel/setup-common.c
> > @@ -599,6 +599,10 @@ int check_legacy_ioport(unsigned long base_port)
> >  		 * name instead */
> >  		if (!np)
> >  			np = of_find_node_by_name(NULL, "8042");
> > +		if (np) {
> > +			of_i8042_kbd_irq = 1;
> > +			of_i8042_aux_irq = 12;
> > +		}
> >  		break;
> >  	case FDC_BASE: /* FDC1 */
> >  		np = of_find_node_by_type(NULL, "fdc");
> 

Re: Pegasos i8042 broken again

[pacman]

Gabriel Paubert writes:
> 
> Ok, I got fed up about it. The patch referred above is obviously wrong since
> it leaves interrupts at 0 when a device_type or name of 8042 is found,
> so what about the following? 

Looks like the workaround I was using for a while.

In the original report I said I wasn't sending my kernel workaround patch
because of the previous disagreements about whether the kernel should work
around this type of bug. (In fact the current difficulty is the result of
changes being made without considering the special case that was created by
my first workaround... what a mess.) I also said I wasn't comfortable hacking
the Forth-based part of the boot sequence because I didn't know the language.

As it turned out, learning Forth was much easier than getting any guidance
from the kernel people on how to proceed with a workaround, so I wrote this:

=== CUT HERE ===
" /isa/8042" find-device
: open true ;
: close ;
: decode-unit ( addr len -- phys )
  1 <> if
    abort" invalid unit address"
  then
  c@
  dup ascii 0 = if
    drop
    0 exit
  then
  ascii 1 = if
    1 exit
  then
  abort" invalid unit address"
;
: encode-unit ( phys -- addr len )
  dup 0 = if
    " 0" exit
  then
  1 = if
    " 1" exit
  then
  abort" invalid unit address"
;

1 encode-int
3 encode-int encode+
d# 12 encode-int encode+
3 encode-int encode+
" interrupts" property

0 0 " 0" " /isa/8042" begin-package
 " keyboard" device-name
 " keyboard" device-type
 " pnpPNP,303" encode-string " compatible" property
 0 encode-int " reg" property
end-package

0 0 " 1" " /isa/8042" begin-package
 " mouse" device-name
 " mouse" device-type
 " pnpPNP,f03" encode-string " compatible" property
 1 encode-int " reg" property
end-package
=== CUT HERE ===

Along with the previous device tree patch (pegasos-dts-20071018), this should
present the kernel with a properly filled-out 8042 device-tree node,
preventing the need for any more patching the next time the kernel changes
its mind about how to initialize the keyboard driver.

-- 
Alan Curry