[PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[BALATON Zoltan]

Add the differential clock input feature bit to the generated SPD
data. Most guests don't seem to care but pegasos2 firmware version 1.2
checks for this bit and stops with unsupported module type error if
it's not present. Since this feature is likely present on real memory
modules add it in the general code rather than patching the generated
SPD data in pegasos2 board only.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
I've tested it with the firmware of pegasos2, sam460ex, fuloong2e and
g3beige (latter is not upstream yet) that are the only ones using this
function currently. Probably this could go in via PPC tree with my
other pegasos2 fix if respective maitainers ack this patch.

 hw/i2c/smbus_eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 4d2bf99207..12c5741f38 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -276,7 +276,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
     spd[18] = 12;   /* ~CAS latencies supported */
     spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */
     spd[20] = 2;    /* DIMM type / ~WE latencies */
-                    /* module features */
+    spd[21] = (type < DDR2 ? 0x20 : 0); /* module features */
                     /* memory chip features */
     spd[23] = 0x12; /* clock cycle time @ medium CAS latency */
                     /* data access time */
-- 
2.21.4

[PATCH 1/2] ppc/pegasos2: Fix spurious warning with -bios

[BALATON Zoltan]

The -append option is currently not compatible with -bios (as we don't
yet emulate nvram so we can only put it in the environment with VOF).
Therefore a warning is printed if -append is used with -bios but
because the default value of kernel_cmdline seems to be an empty
string instead of NULL this warning was printed even without -append
when -bios is used. Only print warning if -append is given.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/pegasos2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 9fad1854b1..b8ce859f1a 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -191,7 +191,7 @@ static void pegasos2_init(MachineState *machine)
             warn_report("Option -kernel may be ineffective with -bios.");
         }
     }
-    if (machine->kernel_cmdline && !pm->vof) {
+    if (!pm->vof && machine->kernel_cmdline && machine->kernel_cmdline[0]) {
         warn_report("Option -append may be ineffective with -bios.");
     }
 }
-- 
2.21.4

[PATCH 0/2] Misc pegasos2 fixes

[BALATON Zoltan]

Some misc fixes that suppress a spurious warning and allows pegasos2
firmware version 1.2 (another version available on line) to work with
the board emulation.

BALATON Zoltan (2):
  ppc/pegasos2: Fix spurious warning with -bios
  i2c/smbus_eeprom: Add feature bit to SPD data

 hw/i2c/smbus_eeprom.c | 2 +-
 hw/ppc/pegasos2.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.21.4

Re: [PATCH 1/2] ppc/pegasos2: Fix spurious warning with -bios

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

On Thu, Jul 15, 2021 at 06:50:44PM +0200, BALATON Zoltan wrote:
> The -append option is currently not compatible with -bios (as we don't
> yet emulate nvram so we can only put it in the environment with VOF).
> Therefore a warning is printed if -append is used with -bios but
> because the default value of kernel_cmdline seems to be an empty
> string instead of NULL this warning was printed even without -append
> when -bios is used. Only print warning if -append is given.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Applied to ppc-for-6.1.

> ---
>  hw/ppc/pegasos2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 9fad1854b1..b8ce859f1a 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -191,7 +191,7 @@ static void pegasos2_init(MachineState *machine)
>              warn_report("Option -kernel may be ineffective with -bios.");
>          }
>      }
> -    if (machine->kernel_cmdline && !pm->vof) {
> +    if (!pm->vof && machine->kernel_cmdline && machine->kernel_cmdline[0]) {
>          warn_report("Option -append may be ineffective with -bios.");
>      }
>  }

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

On Thu, Jul 15, 2021 at 06:50:44PM +0200, BALATON Zoltan wrote:
> Add the differential clock input feature bit to the generated SPD
> data. Most guests don't seem to care but pegasos2 firmware version 1.2
> checks for this bit and stops with unsupported module type error if
> it's not present. Since this feature is likely present on real memory
> modules add it in the general code rather than patching the generated
> SPD data in pegasos2 board only.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> I've tested it with the firmware of pegasos2, sam460ex, fuloong2e and
> g3beige (latter is not upstream yet) that are the only ones using this
> function currently. Probably this could go in via PPC tree with my
> other pegasos2 fix if respective maitainers ack this patch.
> 
>  hw/i2c/smbus_eeprom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This isn't really my area, so I'd need acks to take it through my
tree.


> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 4d2bf99207..12c5741f38 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -276,7 +276,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
>      spd[18] = 12;   /* ~CAS latencies supported */
>      spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */
>      spd[20] = 2;    /* DIMM type / ~WE latencies */
> -                    /* module features */
> +    spd[21] = (type < DDR2 ? 0x20 : 0); /* module features */
>                      /* memory chip features */
>      spd[23] = 0x12; /* clock cycle time @ medium CAS latency */
>                      /* data access time */

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[BALATON Zoltan]

On Sun, 18 Jul 2021, David Gibson wrote:
> On Thu, Jul 15, 2021 at 06:50:44PM +0200, BALATON Zoltan wrote:
>> Add the differential clock input feature bit to the generated SPD
>> data. Most guests don't seem to care but pegasos2 firmware version 1.2
>> checks for this bit and stops with unsupported module type error if
>> it's not present. Since this feature is likely present on real memory
>> modules add it in the general code rather than patching the generated
>> SPD data in pegasos2 board only.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> I've tested it with the firmware of pegasos2, sam460ex, fuloong2e and
>> g3beige (latter is not upstream yet) that are the only ones using this
>> function currently. Probably this could go in via PPC tree with my
>> other pegasos2 fix if respective maitainers ack this patch.
>>
>>  hw/i2c/smbus_eeprom.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> This isn't really my area, so I'd need acks to take it through my
> tree.

Since this is only used by fuloong2e apart from pegasos2 and sam460ex an 
ack from Philippe may be what's needed here. Technically it's in i2c 
because the SPD EEPROM is connected via i2c but other than that it has 
nothing to do with that so Corey is just included because a file in hw/i2c 
is changed so it could go in via any of you three. Since there's another 
pegasos2 related fix queued going via PPC tree would make sense I think.

Regards,
BALATON Zoltan

>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index 4d2bf99207..12c5741f38 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -276,7 +276,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
>>      spd[18] = 12;   /* ~CAS latencies supported */
>>      spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */
>>      spd[20] = 2;    /* DIMM type / ~WE latencies */
>> -                    /* module features */
>> +    spd[21] = (type < DDR2 ? 0x20 : 0); /* module features */
>>                      /* memory chip features */
>>      spd[23] = 0x12; /* clock cycle time @ medium CAS latency */
>>                      /* data access time */
>
>

Re: [PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[Corey Minyard]

On Thu, Jul 15, 2021 at 06:50:44PM +0200, BALATON Zoltan wrote:
> Add the differential clock input feature bit to the generated SPD
> data. Most guests don't seem to care but pegasos2 firmware version 1.2
> checks for this bit and stops with unsupported module type error if
> it's not present. Since this feature is likely present on real memory
> modules add it in the general code rather than patching the generated
> SPD data in pegasos2 board only.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

I checked this all out and it looks correct to me.  I can take it in my
tree, if necessary.  Feature freeze is in two days, so probably not for
6.1, though it could be pushed into there if its needed in 6.1.

Or:

Acked-by: Corey Minyard <cminyard@mvista.com>

if someone else wants to take it.  This particular code really doesn't
belong in eeprom.c, I don't think, but I'm not sure where else to put
it.  And I can look in the SPD tables as well as anyone :).

corey

> ---
> I've tested it with the firmware of pegasos2, sam460ex, fuloong2e and
> g3beige (latter is not upstream yet) that are the only ones using this
> function currently. Probably this could go in via PPC tree with my
> other pegasos2 fix if respective maitainers ack this patch.
> 
>  hw/i2c/smbus_eeprom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 4d2bf99207..12c5741f38 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -276,7 +276,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
>      spd[18] = 12;   /* ~CAS latencies supported */
>      spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */
>      spd[20] = 2;    /* DIMM type / ~WE latencies */
> -                    /* module features */
> +    spd[21] = (type < DDR2 ? 0x20 : 0); /* module features */
>                      /* memory chip features */
>      spd[23] = 0x12; /* clock cycle time @ medium CAS latency */
>                      /* data access time */
> -- 
> 2.21.4
> 

Re: [PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[BALATON Zoltan]

On Sun, 18 Jul 2021, Corey Minyard wrote:
> On Thu, Jul 15, 2021 at 06:50:44PM +0200, BALATON Zoltan wrote:
>> Add the differential clock input feature bit to the generated SPD
>> data. Most guests don't seem to care but pegasos2 firmware version 1.2
>> checks for this bit and stops with unsupported module type error if
>> it's not present. Since this feature is likely present on real memory
>> modules add it in the general code rather than patching the generated
>> SPD data in pegasos2 board only.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> I checked this all out and it looks correct to me.  I can take it in my
> tree, if necessary.  Feature freeze is in two days, so probably not for
> 6.1, though it could be pushed into there if its needed in 6.1.
>
> Or:
>
> Acked-by: Corey Minyard <cminyard@mvista.com>

Thanks. As this fixes pegasos2 ROM 1.2 that some users may find instead of 
the 1.1 version I consider this a bugfix not a feature and would like to 
get merged for 6.1 if possible (hopefully can be in David's tree with the 
other patch that's also a bugfix) because that way it won't happen that 
some users will get problems if they find the wrong ROM. With this patch 
both versions available on line work so there should be no problem for 
anyone. Other firmware ROMs don't seem to care so they're unlikely to 
break and the only machines using it now are pegasos2, sam460ex and 
fuloong2e and only when using firmware ROM so this does not need to wait 
until 6.2 I think as 6.1 is the first version pegasos2 is available and 
it would be nice to get it working well in the first version.

Regards,
BALATON Zoltan

> if someone else wants to take it.  This particular code really doesn't
> belong in eeprom.c, I don't think, but I'm not sure where else to put
> it.  And I can look in the SPD tables as well as anyone :).
>
> corey
>
>> ---
>> I've tested it with the firmware of pegasos2, sam460ex, fuloong2e and
>> g3beige (latter is not upstream yet) that are the only ones using this
>> function currently. Probably this could go in via PPC tree with my
>> other pegasos2 fix if respective maitainers ack this patch.
>>
>>  hw/i2c/smbus_eeprom.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index 4d2bf99207..12c5741f38 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -276,7 +276,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
>>      spd[18] = 12;   /* ~CAS latencies supported */
>>      spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */
>>      spd[20] = 2;    /* DIMM type / ~WE latencies */
>> -                    /* module features */
>> +    spd[21] = (type < DDR2 ? 0x20 : 0); /* module features */
>>                      /* memory chip features */
>>      spd[23] = 0x12; /* clock cycle time @ medium CAS latency */
>>                      /* data access time */
>> --
>> 2.21.4
>>
>
>

Re: [PATCH 2/2] i2c/smbus_eeprom: Add feature bit to SPD data

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

On Sun, Jul 18, 2021 at 10:39:16PM +0200, BALATON Zoltan wrote:
> On Sun, 18 Jul 2021, Corey Minyard wrote:
> > On Thu, Jul 15, 2021 at 06:50:44PM +0200, BALATON Zoltan wrote:
> > > Add the differential clock input feature bit to the generated SPD
> > > data. Most guests don't seem to care but pegasos2 firmware version 1.2
> > > checks for this bit and stops with unsupported module type error if
> > > it's not present. Since this feature is likely present on real memory
> > > modules add it in the general code rather than patching the generated
> > > SPD data in pegasos2 board only.
> > > 
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > 
> > I checked this all out and it looks correct to me.  I can take it in my
> > tree, if necessary.  Feature freeze is in two days, so probably not for
> > 6.1, though it could be pushed into there if its needed in 6.1.
> > 
> > Or:
> > 
> > Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> Thanks. As this fixes pegasos2 ROM 1.2 that some users may find instead of
> the 1.1 version I consider this a bugfix not a feature and would like to get
> merged for 6.1 if possible (hopefully can be in David's tree with the other
> patch that's also a bugfix) because that way it won't happen that some users
> will get problems if they find the wrong ROM. With this patch both versions
> available on line work so there should be no problem for anyone. Other
> firmware ROMs don't seem to care so they're unlikely to break and the only
> machines using it now are pegasos2, sam460ex and fuloong2e and only when
> using firmware ROM so this does not need to wait until 6.2 I think as 6.1 is
> the first version pegasos2 is available and it would be nice to get it
> working well in the first version.

I'll accept the argument that this is a bugfix, and reasonable to
merge at least early in the freeze.  So, I've merged it into my tree
with Corey's ack.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]