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

[Uwe Hermann]

See patch.

Comments welcome. Not sure this is already committable, may need some
fixes.

Uwe.

[Stefan Reinauer]

* Uwe Hermann  [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.

Can't you call both functions from the same constructor?
> 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)
> Signed-off-by: Uwe Hermann 
>  static void i82371eb_isa_init(struct device *dev)
>  {
>    rtc_init(0);
> /* TODO: Select full ISA (instead of EIO)? */
> /* TODO: ISA DMA init? */
>
> }

DMA init is there why dont you just call it?
> // TODO: Rename to ide_init() if it's not 82371EB specific?

it is specific. So remove TODO
> 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.
> reg16 = pci_read_config16(dev, IDETIM_SEC);

printk(BIOS_INFO, "Secondary IDE interface %s\n",
                        conf->ide1_enable?"enabled":"disabled");

The IDE code is nice, but very wasteful.
> printk(BIOS_INFO, "SMBus controller enabled.\n");

This should be DEBUG.
> printk(BIOS_INFO, "Power management functions enabled\n");

Dito.

Let's make the code more silent when not _debugging_ it. output takes
time. Spent time makes us look bad.

[Uwe Hermann]

On Thu, Jun 14, 2007 at 07:32:33PM +0200, Stefan Reinauer wrote:
> Uwe Hermann  [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.