Re: [coreboot] [ patch] v3 Geode graphics init.

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 21.02.2008 01:09, Marc Jones wrote:
> > This patch assumes the vsa/idt patch is in place.
>
> Comments inline.
> > Removed CONFIG_VIDE0_MB in Kconfig and set the Geode
> > graphics memory size through the Geode northbridge pci dts.
> > This is a better way to handle a cpu/mainboard specific value.
> >
> > Signed-off-by: Marc Jones
> > Index: coreboot-v3/northbridge/amd/geodelx/geodelx.c
> >
> > --- coreboot-v3.orig/northbridge/amd/geodelx/geodelx.c    2008-02-20 
> > 10:07:27.000000000 -0700
> > +extern void graphics_init(struct northbridge_amd_geodelx_pci_config 
> > *nb_pci);
> > +extern int sizeram(void);
>
> Do we want sizeram() as a function on all subarches? If yes, we have
> to either call it something like sizeram_4G() or we have to change the
> return type to u64.

I don't know if any other subarchs will implement this but I like the 
idea. I will make it u64.

...
> > @@ -127,7 +162,7 @@
> >          ram_resource(dev, idx++, 0, 640);
> >          /* 1 MB .. (Systop - 1 MB) (converted to KB) */
>
> You subtract only 1 MB here.
> >      ram_resource(dev, idx++, 1024,
> >             (get_systop() - (1 * 1024 * 1024)) / 1024);
> >             (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
> > }

As the comment say. 0-640K is claimed prior to the 1MB to systop.
> > /* SoftVG initialization */
>
> That's scary. Do we really need VSA support even if there is a native
> geodelx framebuffer/X driver?

Not scary. Geode needs VSA for the PCI headers. VSA needs to know how 
much memory is claimed to report it in the BAR.

...
> > /*
> > * Graphics Driver Enabled (13)            0, NO (lets BIOS 
> >
> > controls the GP)
> > * External Monochrome Card Support(12)        0, NO
> > * Controller Priority Select(11)        1, Primary
> > * Display Select(10:8)                0x0, CRT
> > * Graphics Memory Size(7:1)            CONFIG_VIDEO_MB >> 1,
> > *    defined in mainboard/../dts
> > * PLL Reference Clock Bypass(0)        0, Default
> > */
> > /* Video RAM has to be given in 2MB chunks
> > *   the value is read @ 7:1 (value in 7:0 looks like /2)
>
> Typo?
> the value is read @ 7:1 (value in 7:0 looks like *2)
>
> Besides that, being limited to multiples of 2MB for VRAM seems not to mix
> well with subtracting only 1MB from systop.

Maybe that needs a better explanation. Bit 0 is used but video_mb is not 
shifted. Or you could look at it as video_mb/2 << 1.

...
> > +#ifndef GEODELINK_H
> > +#define GEODELINK_H
> >
> > +struct gliutable {
> > unsigned long desc_name;
> > unsigned short desc_type;
> > unsigned long hi, lo;
> >
> > +};
> >
> > +static struct gliutable gliu0table[] = {
> > /* 0-7FFFF to MC */
> > {.desc_name = MSR_GLIU0_BASE1,.desc_type = BM,.hi = MSR_MC + 0x0,
> > .lo = 0x0FFF80},
> > /* 80000-9FFFF to MC */
> > {.desc_name = MSR_GLIU0_BASE2,.desc_type = BM,.hi = MSR_MC + 0x0,
> > .lo = (0x80 << 20) + 0x0FFFE0},
> > /* C0000-FFFFF split to MC and PCI (sub decode) A0000-BFFFF
> > * handled by SoftVideo.
> > */
> > {.desc_name = MSR_GLIU0_SHADOW,.desc_type = SC_SHADOW,
> > .hi = MSR_MC + 0x0,.lo = 0x03},
> > /* Catch and fix dynamically. */
> > {.desc_name = MSR_GLIU0_SYSMEM,.desc_type = R_SYSMEM,
> > .hi = MSR_MC,.lo = 0x0},
> > /* Catch and fix dynamically. */
> > {.desc_name = MSR_GLIU0_SMM,.desc_type = BMO_SMM,
> > .hi = MSR_MC,.lo = 0x0},
> > {.desc_name = GLIU0_GLD_MSR_COH,.desc_type = OTHER,
> > .hi = 0x0,.lo = GL0_CPU},
> > {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
> >
> > +};
> >
> > +static struct gliutable gliu1table[] = {
> > /* 0-7FFFF to MC */
> > {.desc_name = MSR_GLIU1_BASE1,.desc_type = BM,.hi = MSR_GL0 + 0x0,
> > .lo = 0x0FFF80},
> > /* 80000-9FFFF to MC */
> > {.desc_name = MSR_GLIU1_BASE2,.desc_type = BM,.hi = MSR_GL0 + 0x0,
> > .lo = (0x80 << 20) + 0x0FFFE0},
> > /* C0000-Fffff split to MC and PCI (sub decode) */
> > {.desc_name = MSR_GLIU1_SHADOW,.desc_type = SC_SHADOW,
> > .hi = MSR_GL0 + 0x0,.lo = 0x03},
> > /* Catch and fix dynamically. */
> > {.desc_name = MSR_GLIU1_SYSMEM,.desc_type = R_SYSMEM,
> > .hi = MSR_GL0,.lo = 0x0},
> > /* Catch and fix dynamically. */
> > {.desc_name = MSR_GLIU1_SMM,.desc_type = BM_SMM,
> > .hi = MSR_GL0,.lo = 0x0},
> > {.desc_name = GLIU1_GLD_MSR_COH,.desc_type = OTHER,
> > .hi = 0x0,.lo = GL1_GLIU0},
> > /* FooGlue FPU 0xF0 */
> > {.desc_name = MSR_GLIU1_FPU_TRAP,.desc_type = SCIO,
> > .hi = (GL1_GLCP << 29) + 0x0,.lo = 0x033000F0},
> > {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
> >
> > +};
>
> It's debatable whether gliu0table and gliu1table should really be in a
> header file as opposed to a normal .c code file. This max cause additional
> warnings from gcc.
> If any of the tables are constant, please mark them as const.

These need to be marked const. I will think about if this is the right 
way to do this.

...
> With my comments addressed, the patch is
> Acked-by: Carl-Daniel Hailfinger
>
> Regards,
> Carl-Daniel

Thanks, I'll work on these things.
Marc