[PATCH] AMD CS5530(A) ROM enable

[Uwe Hermann]

See patch.

Uwe.

[Carl-Daniel Hailfinger]

On 06.10.2009 16:34, Uwe Hermann wrote:
> * Make the ROM write-protected.

Does that mean we have to unprotect the ROM in flashrom?

Regards,
Carl-Daniel

[Luc Verhaegen]

On Tue, Oct 06, 2009 at 04:42:54PM +0200, Carl-Daniel Hailfinger wrote:
> On 06.10.2009 16:34, Uwe Hermann wrote:
> > * Make the ROM write-protected.
>
> Does that mean we have to unprotect the ROM in flashrom?
>
> Regards,
> Carl-Daniel

I am happy with that if, and only if, the flashrom chipset enable takes 
care of that.

Luc Verhaegen.

[Uwe Hermann]

On Tue, Oct 06, 2009 at 04:49:22PM +0200, Luc Verhaegen wrote:
> > > * Make the ROM write-protected.
> >
> > Does that mean we have to unprotect the ROM in flashrom?

Yes.

I think long-term we want a (runtime/CMOS) option for coreboot
to enable/disable ROM protection. For now, I guess every chipset does
a different thing and/or doesn't explicitly protect or unprotect
the ROM at all.

As a safe default I think disabling the ROM write access is the best thing
to do from a user's point of view.
> I am happy with that if, and only if, the flashrom chipset enable takes 
> care of that.

Sure, flashrom does that. And for other chipsets where we might write-protect
the ROM later we also have flashrom chipset support. I cannot think
of any chipset where we have coreboot support but no flashrom support
(even for CK804 and MCP55 we have).

Uwe.

[ron minnich]

On Tue, Oct 6, 2009 at 8:49 AM, Uwe Hermann  wrote:
> I think long-term we want a (runtime/CMOS) option for coreboot
> to enable/disable ROM protection. For now, I guess every chipset does
> a different thing and/or doesn't explicitly protect or unprotect
> the ROM at all.

Going back to the beginning, from the old days, we always left it
write-enabled. That was amistake on our part and it is good to see it
being fixed.

ron

[Paul Menzel]

Dear Uwe,

Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> Index: src/southbridge/amd/cs5530/cs5530.h
>
> --- src/southbridge/amd/cs5530/cs5530.h (Revision 4726)
> +++ src/southbridge/amd/cs5530/cs5530.h (Arbeitskopie)
> @@ -27,7 +27,13 @@

[…]
> +#define LOWER_ROM_ADDRESS_RANGE                (1 << 0)
> +#define ROM_WRITE_ENABLE               (1 << 1)
> +#define UPPER_ROM_ADDRESS_RANGE                (1 << 2)
> +#define BIOS_ROM_POSITIVE_DECODE       (1 << 5)

Is this spacing intentional?

Thanks,

Paul

[Uwe Hermann]

On Tue, Oct 06, 2009 at 08:16:17PM +0200, Paul Menzel wrote:
> Dear Uwe,
>
> Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> > Index: src/southbridge/amd/cs5530/cs5530.h
> >
> > --- src/southbridge/amd/cs5530/cs5530.h (Revision 4726)
> > +++ src/southbridge/amd/cs5530/cs5530.h (Arbeitskopie)
> > @@ -27,7 +27,13 @@
>
> […]
> > +#define LOWER_ROM_ADDRESS_RANGE                (1 << 0)
> > +#define ROM_WRITE_ENABLE               (1 << 1)
> > +#define UPPER_ROM_ADDRESS_RANGE                (1 << 2)
> > +#define BIOS_ROM_POSITIVE_DECODE       (1 << 5)
>
> Is this spacing intentional?

Yes. It looks strange in the patch/email, but if you apply the patch and
look at the file in a text editor it'll be nicely aligned.

Uwe.

[Marc Jones]

On Tue, Oct 6, 2009 at 8:34 AM, Uwe Hermann  wrote:
> See patch.

Uwe,

Can the call go into a cs5530_enable_rom() call go into a the
southbridge init instead of every mainboard? Would it ever be
mainboard specific?

Marc

[Patrick Georgi]

Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> See patch.

Acked-by: Patrick Georgi

The rom enable call can go into some common southbridge code once the
code flow allows it (currently it does not)

[Uwe Hermann]

On Wed, Oct 07, 2009 at 04:18:06PM +0200, Patrick Georgi wrote:
> Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> > See patch.
>
> Acked-by: Patrick Georgi
>
> The rom enable call can go into some common southbridge code once the
> code flow allows it (currently it does not)

Thanks, r4731. Will take care of moving the calls later, as soon
as the infrastructure is there.

Uwe.