Re: [coreboot] [PATCH] WPCD376I Super I/O support

[Sean Young]

On Thu, Oct 29, 2009 at 08:54:47PM +0100, Uwe Hermann wrote:
> Hi,
>
> On Thu, Oct 29, 2009 at 11:23:14AM +0000, Sean Young wrote:
> > On the Intel DG33TL motherboard, the Super I/O is an WPCD376I. Output:
> >
> > Signed-off-by: Sean Young
>
> Thanks a lot for the output and for the patch! I'll commit the code
> soon, just two questions. The dump will be linked to from the wiki, so
> we have a reference for later usage.
> > @@ -532,17 +572,28 @@
> >    	rev = INB(port + 1);
> >    }
> > if (superio_unknown(reg_table, id)) {
> > /*
> > * NSC PC8374L has id 0xf1, rev 0b000x xxxx according to datasheet
> > * Winbond WPCD376I has id 0xf1, rev 0b100x xxxx according to datasheet
>
> Why is the code for this Winbond Super I/O in the nsc.c file? I assume
> the WPCD376I doesn't need any special enter/exit procedures? But even if
> that's the case, shouldn't the code still work fine in the winbond.c file?

These two super I/O chips are almost identical -- as you can see above, it's
just a revision. Current svn version misdetects the WPCD376I as a PC8374L.

In winbond.c, there is some code to do the detection four times, after 
writing 0x88, 0x89, etc do the super I/O port. This is completely ignored
by the WPCD376I, causing the chip to be detected four times if it does 
there.

winbond.c can be changed to detect the WPCD376I, but since it looks and
smells like an NSC chip, it fits better in nsc.c.
> > @@ -512,6 +551,7 @@
> >  void probe_idregs_nsc(uint16_t port)
> >  {
> >    uint8_t id, rev;
> > int16_t magic;
>
> This can be uint16_t right? Or is there some reason to make it signed
> explicitly?

There is no need for it to be signed. Unsigned makes more sense actually.

Thanks,
Sean