On Mon, Sep 6, 2010 at 4:03 AM, Stefan Reinauer <[email protected]> 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". >> http://www.coreboot.org/pipermail/coreboot/2010-September/060090.html > 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 <[email protected]> > > With the explanation above added to each of the two files, this is > > Acked-by: Stefan Reinauer <[email protected]> > > 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[1]. 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! > > Stefan
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. r5818 Marc -- http://se-eng.com -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
