Pegasos i8042 broken again

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

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");
>

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.