[LinuxBIOS] [PATCH] v3: Intel 82371EB reworking, QEMU fixing

[Uwe Hermann]

On Thu, Jun 14, 2007 at 07:32:33PM +0200, Stefan Reinauer wrote:
> * Uwe Hermann <uwe at hermann-uwe.de> [070607 20:35]:
> > Also, I tried to split up the ACPI/SMBus code into two functions,
> > even though both devices have the same PCI ID. Is this supposed to
> > work?
> 
> LinuxBIOS will only call one constructor per file.

One _list_ of constructors, right? But I guess it won't handle multiple
constructors in that list which apply to the _same_ PCI IDs?

But anyway...


> Can't you call both functions from the same constructor?

...this is the easiest solution, so I did it this way.


> > One issue needs to be resolved: Where/how do we set the config options
> > from config.h? In the dts? For now they seem to default to 0 (off),
> > thus the devices are not initialized (QEMU still boots for some reason).
> 
> See Ron's posting from these days.
> 
> Qemu needs no init. We do it for the sake of it and for the hope that it
> will some time emulate enough of the hw. (Its improving)

OK, then I'll leave the default values for another patch (or until we
have real hardware using one of the 82371XX southbridges).


> > +	/* TODO: ISA DMA init? */
> >  }
>  
> DMA init is there why dont you just call it?

Done. I had some trouble in v2 with this, but that has other reasons, I guess.


> > + // TODO: Rename to ide_init() if it's not 82371EB specific?
> 
> it is specific. So remove TODO

I should have been more precise. It's specific to the 82371XX series,
yes, but it's mostly the same for any of the 82371FB/SB/AB/EB/MB.
Fixed now.


> > +	if (conf->ide0_enable) {
> > +		/* Enable/disable UDMA/33 operation (primary IDE interface). */
> > +		reg8 = pci_read_config8(dev, UDMACTL);
> > +		if (conf->ide0_drive0_udma33_enable) {
> > +			reg8 |= PSDE0;
> > +			printk(BIOS_INFO, "Primary IDE, drive 0: UDMA/33 on\n");
> > +		} else {
> > +			reg8 &= ~(PSDE0);
> > +			printk(BIOS_INFO, "Primary IDE, drive 0: UDMA/33 off\n");
> > +		}
> > +		if (conf->ide0_drive1_udma33_enable) {
> > +			reg8 |= PSDE1;
> > +			printk(BIOS_INFO, "Primary IDE, drive 1: UDMA/33 on\n");
> > +		} else {
> > +			reg8 &= ~(PSDE1);
> > +			printk(BIOS_INFO, "Primary IDE, drive 1: UDMA/33 off\n");
> > +		}
> 
> Suggestion for code size: If you do 
> 
> printk(BIOS_INFO, "%s IDE, drive %d: UDMA/33 %s\n", "Primary", 1, "on");
> 
> Without any other magic, the resulting code will be a lot smaller, since
> it only saves one main string, plus the words Primary/Secondary and
> on/off instead of a full string every time. gcc is great sometimes.

Nice, thanks.

 
> > +	printk(BIOS_INFO, "SMBus controller enabled.\n");
> 
> This should be DEBUG.

OK, done. Shall we make _all_ "FOO init" messages DEBUG?


Attached is a heavily updated patch which now also supports pretty
much _all_ of the 82371FB/SB/AB/EB/MB southbridges...

I also changed some 'int smbus_io = 0xfff0' (SMBus I/O Base) to
a #define in i82371eb.h, I don't see a reason to make this a local
variable. Is there any reason?


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3_i82371eb_impementation.patch
Type: text/x-diff
Size: 21583 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070616/82b7d3d1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070616/82b7d3d1/attachment.sig>