[coreboot] [PATCH] fix 'AMD Fam10 code breaks with gcc 4.5.0'
marcj303 at gmail.com
Fri Sep 17 23:39:36 CEST 2010
On Mon, Sep 6, 2010 at 4:03 AM, Stefan Reinauer
<stefan.reinauer at coresystems.de> wrote:
> On 9/6/10 5:32 AM, Scott Duplichan wrote:
>> Resend: one more attempt to get this patch right. The previous
>> submission included the patch as an attachement. The attachment
>> contained Windows-style line endings. The attachment is missing
>> from the mailing list archive: "A non-text attachment was scrubbed".
> The patch is there, just click on the link below the message.
> (Unfortunately with a wrong name)
>> Root cause: After function STOP_CAR_AND_CPU disables cache as
>> ram, the cache as ram stack can no longer be used. Called
>> functions must be inlined to avoid stack usage. Also, the
>> compiler must keep local variables register based and not
>> allocated them from the stack. With gcc 4.5.0, some functions
>> declared as inline are not being inlined. This patch forces
>> these functions to always be inlined by adding the qualifier
>> __attribute__((always_inline)) to their declaration.
> I think this explanation should go into the two files that get the
> always_inline attributes so people looking at this code 3ys (or months)
> from now know why it was done this way.
>> Update: Still no test reports for real hardware are available.
>> If we cannot get this change tested on real hardware, I suggest
>> we conditionally compile in only if gcc 4.5.0 or later is used.
> I think it's safe to use it as is, though I don't have a Tilapia to test
> it on.
>> Signed-off-by: Scott Duplichan <scott at notabs.org>
> With the explanation above added to each of the two files, this is
> Acked-by: Stefan Reinauer <stepan at coresystems.de>
> However, I think we should additionally look at "fixing" AMD's CAR code
> to not call C functions with neither CAR or RAM backing them. I reworked
> all CAR code to behave like that a while ago (ie. , but the AMD code was
> considerably more complex . The complexity of the code also brings along
> some bugs that currently need workarounds. A proper fix for this
> would be nice, but it's hard to determine what's wrong with the current
> code. Should you have some insights on this, please share!
Yes, this is "wrong" since memory is working at this point, I don't
see why we don't do a stack switch and then disable CAR.
I added comments to the code to explain why they require the new attribute.
More information about the coreboot