Re: [coreboot] [coreboot-gerrit] Patch set updated for coreboot: 26eaf9d Family14: Add support for solder-down DDR memory

[Paul Menzel]

Dear Dave, dear coreboot folks,

hopefully this can be discussed and agreed upon more quickly on the
list.

Am Samstag, den 14.12.2013, 01:25 +0100 schrieb Dave Frodin:
> Dave Frodin ([e-mail]) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/4532
>
> commit 26eaf9ded8eac82dff31dc546f5e1c84c5e63063
> Author: Dave Frodin 
> Date:   Wed Dec 11 13:28:25 2013 -0700
> Family14: Add support for solder-down DDR memory
>
> Normally the memory configuration/settings would be
> read from a DIMM/SODIMMs SPD. This change adds support
> for solder-down memory that doesn't have a SPD. The
> data that would normally be read from the SPD instead
> comes from a file.
>
> Change-Id: Ic10b0c5ca16cbc6af9400ef706392adcb151ac97
> Signed-off-by: Dave Frodin <[e-mail]>
>
> src/northbridge/amd/agesa/Kconfig            | 12 +++++
>  src/northbridge/amd/agesa/family14/dimmSpd.c | 75 ++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)

Are there boards in the tree with hard-coded memory
configuration/settings?
> diff --git a/src/northbridge/amd/agesa/Kconfig b/src/northbridge/amd/agesa/Kconfig
> index d5c039f..5af88f7 100644
> --- a/src/northbridge/amd/agesa/Kconfig
> +++ b/src/northbridge/amd/agesa/Kconfig
> @@ -2,6 +2,7 @@
>  # This file is part of the coreboot project.
>  #
>  # Copyright (C) 2011 - 2012 Advanced Micro Devices, Inc.
> +# Copyright (C) 2013 Sage Electronic Engineering, LLC
>  #
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -31,6 +32,17 @@ config S3_VGA_ROM_RUN
>    bool
>    default n
>
> +config DDR3_SOLDERED_DOWN
> bool
> default n

In my opinion such an option should be introduced globally and not just
for AMD. Especially as the name does not included »AMD«.
> +config PATH_TO_DDR3_SPD
> string
> default ""
> help
>  This is the path from where it is being used/built to where
>  it is stored.

I have no idea how much data such a file contains. If it is rather small
can it be put into `devicetree.cb` directly?

[…]

Thanks,

Paul

PS: Dave, Bruce, sorry for sending the message to you twice.

[coreboot-gerrit] Patch set updated for coreboot: 26eaf9d Family14: Add support for solder-down DDR memory

[mrnuke]

On 12/14/2013 06:08 AM, Paul Menzel wrote:
> Dear Dave, dear coreboot folks,
>
> hopefully this can be discussed and agreed upon more quickly on the
> list.
> [...]
>
> Are there boards in the tree with hard-coded memory
> configuration/settings?

Yes. A number of google boards have DRAM chips hardwired, and
non-replaceable. Their solution is to read the SPD from CBFS during
romstage, before raminit.
> > +config DDR3_SOLDERED_DOWN
> > bool
> > default n
>
> In my opinion such an option should be introduced globally and not just
> for AMD. Especially as the name does not included »AMD«.

And what happens when the board may have more than one SPD. See
google/slippy. I think the solution presented here is inelegant, but I'm
willing to let it fly. However, since we don't have the patch that adds
the mainboard mentioned, we really don't have a way to judge this
change. I think this is a hack, and has the potential to turn really
ugly, really soon. I want to see the mainboard patch so I can see if
this is a well-designed change (R), or if we made this quick hack
because we didn't think it over well enough(TM).
> > +config PATH_TO_DDR3_SPD
> > string
> > default ""
> > help
> >  This is the path from where it is being used/built to where
> >  it is stored.
>
> I have no idea how much data such a file contains. If it is rather small
> can it be put into `devicetree.cb` directly?

Devicetree is used in ramstage, but you need the SPD in romstage.
> […]
>
> Thanks,
>
> Paul
>
> PS: Dave, Bruce, sorry for sending the message to you twice.

[coreboot-gerrit] Patch set updated for coreboot: 26eaf9d Family14: Add support for solder-down DDR memory

[Patrick Georgi]

Am Samstag, den 14.12.2013, 09:35 -0600 schrieb mrnuke:
> > I have no idea how much data such a file contains. If it is rather small
> > can it be put into `devicetree.cb` directly?
>
> Devicetree is used in ramstage, but you need the SPD in romstage.

The devicetree is available in romstage (read-only).

Patrick

[coreboot-gerrit] Patch set updated for coreboot: 26eaf9d Family14: Add support for solder-down DDR memory

[Dave Frodin]

Thanks everybody for the review and comments.
I'm going to push a new commit that does what needs to be done in
hopefully a better way.
The reading of the SPD will no longer be in the northbridge/ path but will
be in a lib function.

Thanks,
Dave

On Sat, Dec 14, 2013 at 9:40 AM, Patrick Georgi wrote:
> Am Samstag, den 14.12.2013, 09:35 -0600 schrieb mrnuke:
> > > I have no idea how much data such a file contains. If it is rather
>
> small
> > > can it be put into `devicetree.cb` directly?
> >
> > Devicetree is used in ramstage, but you need the SPD in romstage.
>
> The devicetree is available in romstage (read-only).
>
> Patrick