Re: [coreboot] patch: two bugs in the cs5536 ide code

[Carl-Daniel Hailfinger]

On 08.05.2008 01:03, Carl-Daniel Hailfinger wrote:
> On 08.05.2008 00:38, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger wrote:
> > > See below for my take at this.
> > >
> > > Move CS5536 IDE configuration into a separate dts and its own PCI
> > > device.
> >
> > Carl-Daniel, is this the patch you mentioned?
>
> Yes.
> > > Signed-off-by: Carl-Daniel Hailfinger
> > >
> > > Build-tested on db800, norwich, dbe62, alix.1c, alix.2c3.
> > > Breaks build for dbe61.
> >
> > Why is that? Please fix this before committing.
>
> Will do. I'll also integrate the fixes Mart Raudsepp suggested for dbe61
> and dbe62.

Done. Please note that the dbe61 is now back to the preexisting age-old
breakage, but the new breakage has been fixed. The dbe61 code has too
many v2 leftovers to compile.
> > Without breaks, this patch is Acked-by: Stefan Reinauer
>
> Thanks!

Committed in r677.

Regards,
Carl-Daniel
> Regards,
> Carl-Daniel
> > > Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c
> > >
> > > --- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c   
> > > (revision 676)
> > > +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c   
> > > (working copy)
> > > @@ -590,6 +590,11 @@
> > >  {
> > >      u32 ide_cfg;
> > > struct southbridge_amd_cs5536_ide_config *ide =
> > >    (struct southbridge_amd_cs5536_ide_config
> > >
> > > *)dev->device_configuration;
> > > if (!ide->enable_ide)
> > >    return;
> > > printk(BIOS_DEBUG, "cs5536_ide: %s\n", __func__);
> > >    /* GPIO and IRQ setup are handled in the main chipset code. */
> > >
> > > @@ -654,9 +659,6 @@
> > >          hide_vpci(sb->unwanted_vpci[i]);
> > >      }
> > >
> > > if (sb->enable_ide)
> > >    ide_init(dev);
> > >
> > > cs5536_setup_power_button(sb);
> > >
> > > printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__);
> > >
> > > @@ -688,3 +690,17 @@
> > >      .phase6_init            = southbridge_init,
> > >  };
> > >
> > > +struct device_operations cs5536_ide = {
> > > .id = {.type = DEVICE_ID_PCI,
> > >    .u = {.pci = {.vendor = PCI_VENDOR_ID_AMD,
> > >              .device = PCI_DEVICE_ID_AMD_CS5536_B0_IDE}}},
> > > .constructor         = default_device_constructor,
> > >
> > > +#warning FIXME: what has to go in phase3_scan?
> > > .phase3_scan         = 0,
> > > .phase4_read_resources     = pci_dev_read_resources,
> > > .phase4_set_resources     = pci_dev_set_resources,
> > > .phase5_enable_resources = pci_dev_enable_resources,
> > > .phase6_init         = ide_init,
> > > .ops_pci         = &pci_dev_ops_pci,
> > >
> > > +};
> >
> > In v2 I liked that every hardware driver for every pci device was in a
> > single, small file. Packing all of it in one large cs5536.c is a bit
> > ugly. But that is not within the scope of this patch I guess.

That will come as a separate cleanup, but probably not from me in the
next 3 weeks.