On Wed, Jan 02, 2019 at 01:36:04PM +0100, BALATON Zoltan wrote: > On Wed, 2 Jan 2019, David Gibson wrote: > > On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote: > > > There are several boards with SPD EEPROMs that are now using > > > duplicated or slightly different hard coded data. Add a helper to > > > generate SPD data for a memory module of given type and size that > > > could be used by these boards (either as is or with further changes if > > > needed) which should help cleaning this up and avoid further duplication. > > > > > > Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu> > > > --- > > > hw/i2c/smbus_eeprom.c | 128 > > > +++++++++++++++++++++++++++++++++++++++++++++++++ > > > include/hw/i2c/smbus.h | 3 ++ > > > 2 files changed, 131 insertions(+) > > > > > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > > > index f18aa3de35..a1f51eb921 100644 > > > --- a/hw/i2c/smbus_eeprom.c > > > +++ b/hw/i2c/smbus_eeprom.c > > > @@ -23,6 +23,8 @@ > > > */ > > > > > > #include "qemu/osdep.h" > > > +#include "qemu/error-report.h" > > > +#include "qemu/units.h" > > > #include "hw/hw.h" > > > #include "hw/i2c/i2c.h" > > > #include "hw/i2c/smbus.h" > > > @@ -162,3 +164,129 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > > > smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256)); > > > } > > > } > > > + > > > +/* Generate SDRAM SPD EEPROM data describing a module of type and size */ > > > +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size) > > > +{ > > > + uint8_t *spd; > > > + uint8_t nbanks; > > > + uint16_t density; > > > + uint32_t size; > > > + int min_log2, max_log2, sz_log2; > > > + int i; > > > + > > > + switch (type) { > > > + case SDR: > > > + min_log2 = 2; > > > + max_log2 = 9; > > > + break; > > > + case DDR: > > > + min_log2 = 5; > > > + max_log2 = 12; > > > + break; > > > + case DDR2: > > > + min_log2 = 7; > > > + max_log2 = 14; > > > + break; > > > + default: > > > + error_report("Unknown SDRAM type"); > > > + abort(); > > > > The error handling might work a little cleaner if you give this > > function an Error ** parameter, then just pass in &error_abort from > > the callers. > > Good idea but I'm not sure it's needed. This is the only fatal error here > (apart from g_malloc0 failing which should also abort) and in practice this > could only happen if a caller specifies wrong type which is quite unlikely > given that it's an enum so value outside of that would fail to compile with > a warning (promoted to error by default). So this default case is only > really here to please the compiler.
Ok. If the only reason you'd hit the default case is a bug in the caller, then just use a g_assert_not_reached() rather than error_report(). As a rule of thumb use asserts or aborts for things that have to be bugs in the code, error_report() for things that indicate a user or configuration error. > > > + } > > > + size = ram_size >> 20; /* work in terms of megabytes */ > > > + if (size < 4) { > > > + error_report("SDRAM size is too small"); > > > + return NULL; > > > + } > > > + sz_log2 = 31 - clz32(size); > > > + size = 1U << sz_log2; > > > + if (ram_size > size * MiB) { > > > + warn_report("SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, " > > > + "truncating to %u MB", ram_size, size); > > > + } > > > + if (sz_log2 < min_log2) { > > > + warn_report("Memory size is too small for SDRAM type, adjusting > > > type"); > > > + if (size >= 32) { > > > + type = DDR; > > > + min_log2 = 5; > > > + max_log2 = 12; > > > + } else { > > > + type = SDR; > > > + min_log2 = 2; > > > + max_log2 = 9; > > > + } > > > > What do these various fall back cases represent? Are they bugs in the > > callers, or a user configuration error? > > The memory size is given by the user so it can be a config error (like when > board has DDR2 but user sets memory size to 64MB). > > > If the first, we should just assert or abort. If the second I think > > we should still die with a fatal error - allowing broken > > configurations to work with just a warning is kind of begging to > > maintain nasty compatiliby cruft in the future. > > I'd leave that to the caller to decide and not abort in this > function. Right. The obvious way to do that is to have this function take an Error *, and use error_setg() where you have explicit warns now. The caller can pass &error_fatal if it just wants to treat that as a user error, and do something else if it wants to implement a fallback. > It > will warn user that config is unexpected for the board but does not prevent > it and try to give something that might still work (e.g. DDR instead of > DDR2). Then the caller can check the returned data and abort if it insists > that only DDR2 will work for the machine. Otherwise we'd make it impossible > to use non-standard memory sizes for cases when it would still work (like > when the OS does not check SPD EEPROMs and would happily use whatever memory > size). I think it's already possible to start machines with odd memory sizes > so that's not new and I did not want to prevent this if SPD EEPROMs are > added (just SPD can't describe all memory in that case which may only be a > problem for firmware but not when directly booting a kernel for example). > > The idea of this function is to generate some data to start from instead of > the static data some boards now have. which is often sufficient for the > board as is but it could be checked or modified further to fit the specific > needs of the board. As those needs can be widely different I did not attempt > to handle all of them in this function to keep it generic. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature