[PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Wade Farnsworth]

This is a fix for the LPC47M192 SuperIO on the MPC8641 HPCN.
Specifically this fixes support for the I8042 Keyboard/Mouse and the
GPIO on the chip.

Also, the mouse needs to use IRQ 12, which is currently in use by some
PCI devices.  Move those devices to IRQ 11, and reserve IRQ 12 for the
mouse.

Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

---

Note that I submitted this previously as part of a larger patchset.
Most of the other patches in the set have been obsoleted, so I'm now
submitting this as a standalone patch.

 arch/powerpc/boot/dts/mpc8641_hpcn.dts     |    4 -
 arch/powerpc/platforms/86xx/mpc86xx_hpcn.c |   43 +++++++++++++++----
 2 files changed, 38 insertions(+), 9 deletions(-)


Index: linux-2.6-powerpc-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
===================================================================
--- linux-2.6-powerpc-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ linux-2.6-powerpc-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -268,7 +268,7 @@
 				/* IDSEL 0x1c */
 				e000 0 0 1 &i8259 9 2
 				e000 0 0 2 &i8259 a 2
-				e000 0 0 3 &i8259 c 2
+				e000 0 0 3 &i8259 b 2
 				e000 0 0 4 &i8259 7 2
 
 				/* IDSEL 0x1d */
@@ -278,7 +278,7 @@
 				e800 0 0 4 &i8259 0 0
 
 				/* IDSEL 0x1e */
-				f000 0 0 1 &i8259 c 2
+				f000 0 0 1 &i8259 b 2
 				f000 0 0 2 &i8259 0 0
 				f000 0 0 3 &i8259 0 0
 				f000 0 0 4 &i8259 0 0
Index: linux-2.6-powerpc-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
===================================================================
--- linux-2.6-powerpc-8641.orig/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
+++ linux-2.6-powerpc-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
@@ -141,7 +141,7 @@ const unsigned char uli1575_irq_route_ta
 	0x1,	/* 9: 0b0001 */
 	0x3,	/* 10: 0b0011 */
 	0x9,	/* 11: 0b1001 */
-	0xb,	/* 12: 0b1011 */
+	0,	/* 12: Reserved */
 	0, 	/* 13: Reserved */
 	0xd,	/* 14, 0b1101 */
 	0xf,	/* 15, 0b1111 */
@@ -211,7 +211,7 @@ static void __devinit quirk_uli1575(stru
 			pirq_map_word |= (uli1575_irq_route_table[i] & 0xf)
 				<< ((irq2pin[i] - PIRQA) * 4);
 
-	/* ULI1575 IRQ mapping conf register default value is 0xb9317542 */
+	/* ULI1575 IRQ mapping conf register default value is 0x09317542 */
 	DBG("Setup ULI1575 IRQ mapping configuration register value = 0x%x\n",
 			pirq_map_word);
 	pci_write_config_dword(dev, 0x48, pirq_map_word);
@@ -266,9 +266,9 @@ static void __devinit quirk_uli1575(stru
 	pci_write_config_byte(dev, 0x44, 0x30 | uli1575_irq_route_table[14]);
 	pci_write_config_byte(dev, 0x75, uli1575_irq_route_table[15]);
 
-	/* Set IRQ14 and IRQ15 to legacy IRQs */
+	/* Set IRQ1, IRQ12, IRQ14 and IRQ15 to legacy IRQs */
 	pci_read_config_word(dev, 0x46, &temp);
-	temp |= 0xc000;
+	temp |= 0xd002;
 	pci_write_config_word(dev, 0x46, temp);
 
 	/* Set i8259 interrupt trigger
@@ -280,12 +280,12 @@ static void __devinit quirk_uli1575(stru
 	 * IRQ 9:  Level
 	 * IRQ 10: Level
 	 * IRQ 11: Level
-	 * IRQ 12: Level
+	 * IRQ 12: Edge
 	 * IRQ 14: Edge
 	 * IRQ 15: Edge
 	 */
-	outb(0xfa, 0x4d0);
-	outb(0x1e, 0x4d1);
+	outb(0xf8, 0x4d0);
+	outb(0x0e, 0x4d1);
 
 #undef ULI1575_SET_DEV_IRQ
 
@@ -293,6 +293,35 @@ static void __devinit quirk_uli1575(stru
 	pci_read_config_byte(dev, 0xb8, &c);
 	c &= 0x7f;
 	pci_write_config_byte(dev, 0xb8, c);
+
+	/* enable superio @ 0x4e and keyboard/mouse address decoding */
+	pci_write_config_byte(dev, 0x63, 0x90);
+
+	/* LPC47M192 Super I/O configuration */
+	outb(0x55, 0x4e);	/* enter superio config mode */
+
+	/* Enable keyboard and mouse */
+	outb(0x07, 0x4e);	/* device selector register */
+	outb(0x07, 0x4f);	/* select keyboard registers (device 7) */
+	outb(0x30, 0x4e);	/* keyboard activation register */
+	outb(0x01, 0x4f);	/* activate keyboard */
+	outb(0x70, 0x4e);	/* keyboard IRQ register */
+	outb(0x01, 0x4f);	/* IRQ1 for keyboard */
+	outb(0x72, 0x4e);	/* mouse IRQ register */
+	outb(0x0c, 0x4f);	/* IRQ12 for mouse */
+
+	/* Enable superio runtime registers for gpio in pci i/o space */
+	outb(0x20, 0x4e);	/* device id register */
+	outb(0x07, 0x4e);	/* device selector register */
+	outb(0x0a, 0x4f);	/* select runtime registers (device A) */
+	outb(0x60, 0x4e);	/* select runtime register address high byte */
+	outb(0x04, 0x4f);	/* runtime register address high byte */
+	outb(0x61, 0x4e);	/* select runtime register address low byte */
+	outb(0x01, 0x4f);	/* runtime register address low byte */
+	outb(0x30, 0x4e);	/* runtime registers activation register */
+	outb(0x01, 0x4f);	/* activate runtime registers */
+
+	outb(0xaa, 0x4e);	/* exit superio config mode */
 }
 
 static void __devinit quirk_uli5288(struct pci_dev *dev)

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Arnd Bergmann]

On Wednesday 06 June 2007, Wade Farnsworth wrote:
> +=A0=A0=A0=A0=A0=A0=A0/* LPC47M192 Super I/O configuration */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x55, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* enter supe=
rio config mode */
> +
> +=A0=A0=A0=A0=A0=A0=A0/* Enable keyboard and mouse */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x07, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* device sel=
ector register */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x07, 0x4f);=A0=A0=A0=A0=A0=A0=A0/* select key=
board registers (device 7) */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x30, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* keyboard a=
ctivation register */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x01, 0x4f);=A0=A0=A0=A0=A0=A0=A0/* activate k=
eyboard */

Hardcoded I/O port numbers always worry me a little. I know that this is
supposed to work in general, but can't you read the I/O port range from
a device tree property?

	Arnd <><

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Segher Boessenkool]

>> +=A0=A0=A0=A0=A0=A0=A0/* LPC47M192 Super I/O configuration */
>> +=A0=A0=A0=A0=A0=A0=A0outb(0x55, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* enter =
superio config mode */
>> +
>> +=A0=A0=A0=A0=A0=A0=A0/* Enable keyboard and mouse */
>> +=A0=A0=A0=A0=A0=A0=A0outb(0x07, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* device =
selector register */
>> +=A0=A0=A0=A0=A0=A0=A0outb(0x07, 0x4f);=A0=A0=A0=A0=A0=A0=A0/* select =
keyboard registers (device=20
>> 7) */
>> +=A0=A0=A0=A0=A0=A0=A0outb(0x30, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* =
keyboard activation register */
>> +=A0=A0=A0=A0=A0=A0=A0outb(0x01, 0x4f);=A0=A0=A0=A0=A0=A0=A0/* =
activate keyboard */
>
> Hardcoded I/O port numbers always worry me a little. I know that this=20=

> is
> supposed to work in general, but can't you read the I/O port range =
from
> a device tree property?

This code is writing hardcoded values into IRQ#
and address window registers, the superio configuration
address should be the least of your worries ;-)


Segher

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Wade Farnsworth]

On Thu, 2007-06-07 at 00:39 +0200, Arnd Bergmann wrote:
> On Wednesday 06 June 2007, Wade Farnsworth wrote:
> > +       /* LPC47M192 Super I/O configuration */
> > +       outb(0x55, 0x4e);       /* enter superio config mode */
> > +
> > +       /* Enable keyboard and mouse */
> > +       outb(0x07, 0x4e);       /* device selector register */
> > +       outb(0x07, 0x4f);       /* select keyboard registers (device 7) */
> > +       outb(0x30, 0x4e);       /* keyboard activation register */
> > +       outb(0x01, 0x4f);       /* activate keyboard */
> 
> Hardcoded I/O port numbers always worry me a little. I know that this is
> supposed to work in general, but can't you read the I/O port range from
> a device tree property?

I suppose I could create a device node for the Super I/O config
registers and use those instead of hardcoding it here.

Something to the effect of:

superio_cfg@4e {
	reg = <1 4e 2>;
	compatible = "smsc-lpc47m192-cfg";
};

I'm not sure if the name and compatible properties are appropriate
though.  Any recommendations?

--Wade

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Segher Boessenkool]

>> Hardcoded I/O port numbers always worry me a little. I know that this 
>> is
>> supposed to work in general, but can't you read the I/O port range 
>> from
>> a device tree property?
>
> I suppose I could create a device node for the Super I/O config
> registers and use those instead of hardcoding it here.

I'd just hide it all, do this setup in the firmware,
where it belongs, and don't expose the superio config
in the device tree.

> superio_cfg@4e {
> 	reg = <1 4e 2>;
> 	compatible = "smsc-lpc47m192-cfg";
> };
>
> I'm not sure if the name and compatible properties are appropriate
> though.  Any recommendations?

"superio" and "smsc,lpc47m192" I'd say.  You also
then should link the logical devices on the superio
to the device nodes that represent those.  I'm not
sure this is all worth it, this is low-level setup
the firmware should do and everything else can treat
it as a black box.


Segher

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Wade Farnsworth]

On Thu, 2007-06-07 at 18:35 +0200, Segher Boessenkool wrote:
> >> Hardcoded I/O port numbers always worry me a little. I know that this 
> >> is
> >> supposed to work in general, but can't you read the I/O port range 
> >> from
> >> a device tree property?
> >
> > I suppose I could create a device node for the Super I/O config
> > registers and use those instead of hardcoding it here.
> 
> I'd just hide it all, do this setup in the firmware,
> where it belongs, and don't expose the superio config
> in the device tree.

That's a valid point.  This probably could (should?) be handled by
U-boot.

Jon, or others do you have any opinions on this?

> 
> > superio_cfg@4e {
> > 	reg = <1 4e 2>;
> > 	compatible = "smsc-lpc47m192-cfg";
> > };
> >
> > I'm not sure if the name and compatible properties are appropriate
> > though.  Any recommendations?
> 
> "superio" and "smsc,lpc47m192" I'd say.  You also
> then should link the logical devices on the superio
> to the device nodes that represent those.  I'm not
> sure this is all worth it, this is low-level setup
> the firmware should do and everything else can treat
> it as a black box.

OK, if we decide to keep this, I'll use those instead.

Thanks.

--Wade

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Segher Boessenkool]

>> I'd just hide it all, do this setup in the firmware,
>> where it belongs, and don't expose the superio config
>> in the device tree.
>
> That's a valid point.  This probably could (should?) be handled by
> U-boot.

Yeah.  One more argument for this is that _every_
possible OS would need this code executed before
it runs.


Segher

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Andy Fleming]

On Jun 7, 2007, at 11:35, Segher Boessenkool wrote:

>>> Hardcoded I/O port numbers always worry me a little. I know that  
>>> this
>>> is
>>> supposed to work in general, but can't you read the I/O port range
>>> from
>>> a device tree property?
>>
>> I suppose I could create a device node for the Super I/O config
>> registers and use those instead of hardcoding it here.
>
> I'd just hide it all, do this setup in the firmware,
> where it belongs, and don't expose the superio config
> in the device tree.


No more.  No more firmware-only initializations.  It sounds great, in  
principle, until you actually have to figure out why someone's setup  
isn't working.  I'm tired of having to see if the dts, u-boot, and  
Linux are in sync.  If Linux wants to use a device, I think it's not  
unreasonable to have it setup the device itself.  That way, Linux can  
do whatever it wants with the device, and not have to rely on U-Boot  
(or some other firmware) setting up the appropriate bits.


>
>> superio_cfg@4e {
>> 	reg = <1 4e 2>;
>> 	compatible = "smsc-lpc47m192-cfg";
>> };
>>
>> I'm not sure if the name and compatible properties are appropriate
>> though.  Any recommendations?
>
> "superio" and "smsc,lpc47m192" I'd say.  You also
> then should link the logical devices on the superio
> to the device nodes that represent those.  I'm not
> sure this is all worth it, this is low-level setup
> the firmware should do and everything else can treat
> it as a black box.


Please...no.  What happens next is that we find a small bug that  
requires we modify U-Boot to do the initialization slightly  
differently, and then requires Linux to act slightly differently.   
And then I get emails every day from people wondering why their  
boards don't work.

Andy

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Segher Boessenkool]

>>> I suppose I could create a device node for the Super I/O config
>>> registers and use those instead of hardcoding it here.
>>
>> I'd just hide it all, do this setup in the firmware,
>> where it belongs, and don't expose the superio config
>> in the device tree.
>
> No more.  No more firmware-only initializations.

This setup is very board specific, and the board cannot
reasonably work without that setup being done right.  You
really want to push _that_ into Linux?  Alternatively,
you could put it into the device tree, but that doesn't
help anything either.

> It sounds great, in principle, until you actually have to figure out 
> why someone's setup isn't working.  I'm tired of having to see if the 
> dts, u-boot, and Linux are in sync.  If Linux wants to use a device, I 
> think it's not unreasonable to have it setup the device itself.  That 
> way, Linux can do whatever it wants with the device, and not have to 
> rely on U-Boot (or some other firmware) setting up the appropriate 
> bits.

There is one and only one way to set up the superio for
a certain board (assuming the legacy I/O and IRQ values
are considered fixed values).

> Please...no.  What happens next is that we find a small bug that 
> requires we modify U-Boot to do the initialization slightly 
> differently, and then requires Linux to act slightly differently.

This is equivalent to needing a board-level fix really.


Segher

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Segher Boessenkool]

> +	/* enable superio @ 0x4e and keyboard/mouse address decoding */
> +	pci_write_config_byte(dev, 0x63, 0x90);

I doubt that comment is accurate.

> +	outb(0x07, 0x4e);	/* device selector register */
> +	outb(0x07, 0x4f);	/* select keyboard registers (device 7) */

Please abstract out the 4e,4f access sequence, so
you can just say  write_sio(0x30, 1);  or similar.

> +	/* Enable superio runtime registers for gpio in pci i/o space */
> +	outb(0x20, 0x4e);	/* device id register */

This write is superfluous as far as I can see.


Segher

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Wade Farnsworth]

On Thu, 2007-06-07 at 15:01 +0200, Segher Boessenkool wrote:
> > +	/* enable superio @ 0x4e and keyboard/mouse address decoding */
> > +	pci_write_config_byte(dev, 0x63, 0x90);
> 
> I doubt that comment is accurate.

m1575 legacy interface device register 0x63 is the legacy I/O decoding
control.

Writing 0x90 enables ports 0x4e and 0x4f for Super I/O configuration and
ports 0x60 and 0x64 for keyboard/mouse.

> 
> > +	outb(0x07, 0x4e);	/* device selector register */
> > +	outb(0x07, 0x4f);	/* select keyboard registers (device 7) */
> 
> Please abstract out the 4e,4f access sequence, so
> you can just say  write_sio(0x30, 1);  or similar.

Sure.

> 
> > +	/* Enable superio runtime registers for gpio in pci i/o space */
> > +	outb(0x20, 0x4e);	/* device id register */
> 
> This write is superfluous as far as I can see.

Yes, it appears so.  I'll remove it.

--Wade

Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN

[Segher Boessenkool]

>>> +	/* enable superio @ 0x4e and keyboard/mouse address decoding */
>>> +	pci_write_config_byte(dev, 0x63, 0x90);
>>
>> I doubt that comment is accurate.
>
> m1575 legacy interface device register 0x63 is the legacy I/O decoding
> control.
>
> Writing 0x90 enables ports 0x4e and 0x4f for Super I/O configuration 
> and
> ports 0x60 and 0x64 for keyboard/mouse.

Ah I see.  Copy this explanation into your next
version of the patch?  :-)


Segher