[PATCH] i82801gx updates

[Stefan Reinauer]

See patch

[Carl-Daniel Hailfinger]

I don't know the code well, so this is not a real review. Just a nitpick.

On 11.03.2009 13:23, Stefan Reinauer wrote:
> +static void smbus_set_subsystem(device_t dev, unsigned vendor, unsigned device)
> +{
> if (!vendor || !device) {

This case should output a diagnostic message at SPEW or DEBUG level.
> pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> 		pci_read_config32(dev, 0));

PCI_VENDOR_ID instead of 0 for consistency.
> } else {
> pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> 		((device & 0xffff) << 16) | (vendor & 0xffff));
> }
>
> +}

Same for the other *_set_subsystem. Maybe we even want a generic
function to handle this since the *_set_subsystem functions in the patch
seem to be identical.

Regards,
Carl-Daniel

[Stefan Reinauer]

On 11.03.2009 13:47 Uhr, Carl-Daniel Hailfinger wrote:
> I don't know the code well, so this is not a real review. Just a nitpick.
>
> On 11.03.2009 13:23, Stefan Reinauer wrote:
> > +static void smbus_set_subsystem(device_t dev, unsigned vendor, unsigned device)
> > +{
> > if (!vendor || !device) {
>
> This case should output a diagnostic message at SPEW or DEBUG level.

I don't think this is a good idea. It's a perfectly fine case that we
don't want to warn about. Plus, the calling function already does a
debug level print in the case that a subsystem function is specified.
> > pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> > 		pci_read_config32(dev, 0));
>
> PCI_VENDOR_ID instead of 0 for consistency.

Good point.
> > } else {
> > pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> > 		((device & 0xffff) << 16) | (vendor & 0xffff));
> > }
> >
> > +}
>
> Same for the other *_set_subsystem. Maybe we even want a generic
> function to handle this since the *_set_subsystem functions in the patch
> seem to be identical.

Yes, mostly. I was going to look into that as a next step. So far I only
found information from intel that they suggest this behavior, so I
didn't want to make it a global rule.

[Myles Watson]

+	// NOTE this will break as soon as the Azalia get's a bar above
+	// 4G. Is there anything we can do about it?
    base = (u8 *) ((u32)res->base);
-	printk_debug("base = %08x\n", base);
+	printk_debug("Azalia: base = %08x\n", (u32)base);
    codec_mask = codec_detect(base);

Can't you add your own read_resources and set the limit for the BAR to
0xffffffff?  Then you know the BAR will never get allocated above that.

Acked-by: Myles Watson

Thanks,
Myles

[Stefan Reinauer]

On 11.03.2009 15:24 Uhr, Myles Watson wrote:
> // NOTE this will break as soon as the Azalia get's a bar above
> // 4G. Is there anything we can do about it?
> base = (u8 *) ((u32)res->base);
> printk_debug("base = %08x\n", base);
> printk_debug("Azalia: base = %08x\n", (u32)base);
> codec_mask = codec_detect(base);
>
> Can't you add your own read_resources and set the limit for the BAR to
> 0xffffffff?  Then you know the BAR will never get allocated above that.

Hm... That should work. Maybe we should consider this in the generic
code, though. When coreboot runs in 32bit and maps BARs above 4G it
can't easily access those bars anymore.

Right now, the BAR will never be allocated above 4G because the chipset
can't really cope with it anyways. Future ICHx chips might have the
problem (in case we decide to really put BARs above 4G, which we don't
do per default). It's not a real issue yet, just thought I leave the
comment in so if people copy it around, and some day run into trouble,
they don't have to do the same thinking.
> Acked-by: Myles Watson

Thanks, r3991

[Myles Watson]

On Wed, Mar 11, 2009 at 8:55 AM, Stefan Reinauer  wrote:
> On 11.03.2009 15:24 Uhr, Myles Watson wrote:
> > // NOTE this will break as soon as the Azalia get's a bar above
> > // 4G. Is there anything we can do about it?
> >
> > base = (u8 *) ((u32)res->base);
> > printk_debug("base = %08x\n", base);
> > printk_debug("Azalia: base = %08x\n", (u32)base);
> >
> > codec_mask = codec_detect(base);
> >
> > Can't you add your own read_resources and set the limit for the BAR to
> > 0xffffffff?  Then you know the BAR will never get allocated above that.
>
> Hm... That should work. Maybe we should consider this in the generic
> code, though. When coreboot runs in 32bit and maps BARs above 4G it
> can't easily access those bars anymore.

What are the options?
1. Save scratch address space where you allocate BARs for playing
with, then move them up when you're done.
2. Run coreboot in 64bit mode
3. Confine config space to 4G
4. ?
> Right now, the BAR will never be allocated above 4G because the chipset
> can't really cope with it anyways. Future ICHx chips might have the
> problem (in case we decide to really put BARs above 4G, which we don't
> do per default). It's not a real issue yet, just thought I leave the
> comment in so if people copy it around, and some day run into trouble,
> they don't have to do the same thinking.

Good idea.

Thanks,
Myles

[Stefan Reinauer]

On 11.03.2009 16:02 Uhr, Myles Watson wrote:
> On Wed, Mar 11, 2009 at 8:55 AM, Stefan Reinauer  wrote:
> > On 11.03.2009 15:24 Uhr, Myles Watson wrote:
> > > // NOTE this will break as soon as the Azalia get's a bar above
> > > // 4G. Is there anything we can do about it?
> > > base = (u8 *) ((u32)res->base);
> > > printk_debug("base = %08x\n", base);
> > > printk_debug("Azalia: base = %08x\n", (u32)base);
> > > codec_mask = codec_detect(base);
> > >
> > > Can't you add your own read_resources and set the limit for the BAR to
> > > 0xffffffff?  Then you know the BAR will never get allocated above that.
> >
> > Hm... That should work. Maybe we should consider this in the generic
> > code, though. When coreboot runs in 32bit and maps BARs above 4G it
> > can't easily access those bars anymore.
>
> What are the options?
> Save scratch address space where you allocate BARs for playing
>
> with, then move them up when you're done.
> 2. Run coreboot in 64bit mode
> 3. Confine config space to 4G
> 4. ?

Maybe PAE? I think we do this for memory scrubbing on K8. Does it also
work for MMIO?

I really like the coreboot in 64bit mode idea, but it's a big deal to
switch those cpus in 64bit mode. Can't be done before memory is there,
either.

[ron minnich]

On Wed, Mar 11, 2009 at 8:22 AM, Stefan Reinauer  wrote:
> I really like the coreboot in 64bit mode idea, but it's a big deal to switch
> those cpus in 64bit mode. Can't be done before memory is there, either.

we're going to have to do it someday. The day will come when PAE won't
let us address enough memory without ugly tricks. I have a machine
here with 128 GB, a friend has one with 256GB, and we're discussing
larger machines.

I am guessing it would be easier to do in v3.

ron