[PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[~ssinprem]

From: Sittisak Sinprem <ssinprem@celestca.com>

---
 hw/nvram/eeprom_at24c.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 2d4d8b952f..693212b661 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -87,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
     EEPROMState *ee = AT24C_EE(s);
     uint8_t ret;
 
-    if (ee->haveaddr == 1) {
+    if (ee->rsize > 256 && ee->haveaddr == 1) {
         return 0xff;
     }
 
@@ -104,11 +104,13 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
 {
     EEPROMState *ee = AT24C_EE(s);
 
-    if (ee->haveaddr < 2) {
+    if ((ee->rsize > 256 && ee->haveaddr < 2) ||
+        (ee->rsize <= 256 && ee->haveaddr < 1)) {
         ee->cur <<= 8;
         ee->cur |= data;
         ee->haveaddr++;
-        if (ee->haveaddr == 2) {
+        if ((ee->rsize > 256 && ee->haveaddr == 2) ||
+            (ee->rsize <= 256 && ee->haveaddr == 1)) {
             ee->cur %= ee->rsize;
             DPRINTK("Set pointer %04x\n", ee->cur);
         }
-- 
2.34.6

[PATCH qemu 2/2] aspeed/fuji : correct the eeprom size

[~ssinprem]

From: Sittisak Sinprem <ssinprem@celestca.com>

Device 24C64 the size is 64 kilobits
Device 24C02 the size is 2 kilobits
---
 hw/arm/aspeed.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 55f114ef72..8e6a1579e4 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -846,42 +846,42 @@ static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
     i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
     i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
 
-    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
-    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
+    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB / 8);
+    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB / 8);
 
     i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
     i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
     i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
     i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
 
-    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
+    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB / 8);
     i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
 
     i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
-    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
+    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB / 8);
     i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
     i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
 
     i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
     i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
 
-    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
+    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB / 8);
     i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
     i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
-    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
+    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB / 8);
 
-    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
+    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB / 8);
     i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
     i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
-    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
-    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
+    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB / 8);
+    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB / 8);
 
     for (int i = 0; i < 8; i++) {
         aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
-- 
2.34.6

[PATCH qemu 0/2] hw/at24c support eeprom size less than equal 256 byte

[~ssinprem]

This series includes 2 patches
1. Improve at24c to support 1-byte address EEPROM such as 24c01 and
24c02.
2. correct the EEPROM size for aspeed/fuji platform.
ref: https://gitlab.com/qemu-project/qemu/-/issues/1485

Sittisak Sinprem (2):
  hw/at24c : modify at24c to support 1 byte address mode
  aspeed/fuji : correct the eeprom size

 hw/arm/aspeed.c         | 32 ++++++++++++++++----------------
 hw/nvram/eeprom_at24c.c |  8 +++++---
 2 files changed, 21 insertions(+), 19 deletions(-)

-- 
2.34.6

Re: [PATCH qemu 2/2] aspeed/fuji : correct the eeprom size

[Cédric Le Goater]

Hello,

Adding Peter since he contributed the fuji machine,

On 2/14/23 10:06, ~ssinprem wrote:
> From: Sittisak Sinprem <ssinprem@celestca.com>
> 
> Device 24C64 the size is 64 kilobits
> Device 24C02 the size is 2 kilobits

Could you please specify the size in bytes ?

You will need to add a Signed-off-by tag.

Thanks,

C.

> ---
>   hw/arm/aspeed.c | 32 ++++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 55f114ef72..8e6a1579e4 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -846,42 +846,42 @@ static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
>       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
>       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
>   
> -    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
> -    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
> +    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB / 8);
> +    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB / 8);
>   
>       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
>       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
>       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
>       i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
>   
> -    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
> +    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB / 8);
>       i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
>   
>       i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
> -    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
> +    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB / 8);
>       i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
>       i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
>   
>       i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
>       i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
>   
> -    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
> +    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB / 8);
>       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
>       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
> -    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
> +    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB / 8);
>   
> -    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
> +    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB / 8);
>       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
>       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
> -    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
> -    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
> +    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB / 8);
> +    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB / 8);
>   
>       for (int i = 0; i < 8; i++) {
>           aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);

Re: [PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[Cédric Le Goater]

On 2/10/23 07:20, ~ssinprem wrote:
> From: Sittisak Sinprem <ssinprem@celestca.com>



You will need to add a Signed-off-by tag

Thanks,

C.

> ---
>   hw/nvram/eeprom_at24c.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 2d4d8b952f..693212b661 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -87,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
>       EEPROMState *ee = AT24C_EE(s);
>       uint8_t ret;
>   
> -    if (ee->haveaddr == 1) {
> +    if (ee->rsize > 256 && ee->haveaddr == 1) {
>           return 0xff;
>       }
>   
> @@ -104,11 +104,13 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
>   {
>       EEPROMState *ee = AT24C_EE(s);
>   
> -    if (ee->haveaddr < 2) {
> +    if ((ee->rsize > 256 && ee->haveaddr < 2) ||
> +        (ee->rsize <= 256 && ee->haveaddr < 1)) {
>           ee->cur <<= 8;
>           ee->cur |= data;
>           ee->haveaddr++;
> -        if (ee->haveaddr == 2) {
> +        if ((ee->rsize > 256 && ee->haveaddr == 2) ||
> +            (ee->rsize <= 256 && ee->haveaddr == 1)) {
>               ee->cur %= ee->rsize;
>               DPRINTK("Set pointer %04x\n", ee->cur);
>           }

Re: [PATCH qemu 2/2] aspeed/fuji : correct the eeprom size

[Sittisak Sinprem]

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

Hi,

Follow the question " Could you please specify the size in bytes ? "

Yes, In the source code we need to set the size value as byte.

I add "divide by 8" to convert it to bytes unit.
24c64  size is  64kbit  = 64k / 8  bytes

-    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
+    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB / 8);

On Tue, Feb 14, 2023 at 9:29 PM Cédric Le Goater <clg@kaod.org> wrote:

> Hello,
>
> Adding Peter since he contributed the fuji machine,
>
> On 2/14/23 10:06, ~ssinprem wrote:
> > From: Sittisak Sinprem <ssinprem@celestca.com>
> >
> > Device 24C64 the size is 64 kilobits
> > Device 24C02 the size is 2 kilobits
>
> Could you please specify the size in bytes ?
>
> You will need to add a Signed-off-by tag.
>
> Thanks,
>
> C.
>
> > ---
> >   hw/arm/aspeed.c | 32 ++++++++++++++++----------------
> >   1 file changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 55f114ef72..8e6a1579e4 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -846,42 +846,42 @@ static void fuji_bmc_i2c_init(AspeedMachineState
> *bmc)
> >       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
> >       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
> >
> > -    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
> > -    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
> > +    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB / 8);
> >
> >       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
> >       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
> >       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
> >       i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
> >
> > -    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
> >
> >       i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
> > -    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
> >       i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
> >
> >       i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
> >       i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
> >
> > -    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
> >       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
> > -    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB / 8);
> >
> > -    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
> >       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
> > -    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
> > +    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB / 8);
> >
> >       for (int i = 0; i < 8; i++) {
> >           aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
>
>

[-- Attachment #2: Type: text/html, Size: 5504 bytes --]

Re: [PATCH qemu 2/2] aspeed/fuji : correct the eeprom size

[Peter Delevoryas]

On Tue, Feb 14, 2023 at 03:28:58PM +0100, Cédric Le Goater wrote:
> Hello,
> 
> Adding Peter since he contributed the fuji machine,
> 
> On 2/14/23 10:06, ~ssinprem wrote:
> > From: Sittisak Sinprem <ssinprem@celestca.com>
> > 
> > Device 24C64 the size is 64 kilobits
> > Device 24C02 the size is 2 kilobits
> 
> Could you please specify the size in bytes ?
> 
> You will need to add a Signed-off-by tag.
> 
> Thanks,
> 
> C.

Thanks Cedric,

Sittisak, thanks for this fix, I didn't realize the size was in bits (oops lol,
I'm pretty new to embedded systems)

I agree with Cedric though, instead of adding "/ 8", could we just replace 64 *
KiB / 8 with 8 * KiB, and 2 * KiB / 8 with 256?

- Peter

Reviewed-by: Peter Delevoryas <peter@pjd.dev>

> 
> > ---
> >   hw/arm/aspeed.c | 32 ++++++++++++++++----------------
> >   1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 55f114ef72..8e6a1579e4 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -846,42 +846,42 @@ static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
> >       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
> >       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
> > -    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
> > -    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
> > +    aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB / 8);
> >       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
> >       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
> >       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
> >       i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
> > -    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
> >       i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
> > -    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
> >       i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
> >       i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
> >       i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
> > -    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
> >       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
> > -    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB / 8);
> > -    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
> > +    aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB / 8);
> >       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
> >       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
> > -    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
> > -    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
> > +    aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB / 8);
> > +    aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB / 8);
> >       for (int i = 0; i < 8; i++) {
> >           aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
> 

Re: [PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[Peter Delevoryas]

On Tue, Feb 14, 2023 at 03:29:33PM +0100, Cédric Le Goater wrote:
> On 2/10/23 07:20, ~ssinprem wrote:
> > From: Sittisak Sinprem <ssinprem@celestca.com>
> 
> 
> 
> You will need to add a Signed-off-by tag
> 
> Thanks,
> 
> C.

Oh, yeah this is a pretty good change: I mean, at first I had no idea what's
going on here, so it would be nice if we could leave a comment or refactor it
to be simpler.

Maybe instead of if-statements for > 256 or <= 256, we could do an address size
attribute and compute the 256 bound from the address size.

Anyways, this is a really small change to fix behavior, we can do a refactoring
like that later (If Cedric is ok with it).

Reviewed-by: Peter Delevoryas <peter@pjd.dev>

> 
> > ---
> >   hw/nvram/eeprom_at24c.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> > index 2d4d8b952f..693212b661 100644
> > --- a/hw/nvram/eeprom_at24c.c
> > +++ b/hw/nvram/eeprom_at24c.c
> > @@ -87,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
> >       EEPROMState *ee = AT24C_EE(s);
> >       uint8_t ret;
> > -    if (ee->haveaddr == 1) {
> > +    if (ee->rsize > 256 && ee->haveaddr == 1) {
> >           return 0xff;
> >       }
> > @@ -104,11 +104,13 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
> >   {
> >       EEPROMState *ee = AT24C_EE(s);
> > -    if (ee->haveaddr < 2) {
> > +    if ((ee->rsize > 256 && ee->haveaddr < 2) ||
> > +        (ee->rsize <= 256 && ee->haveaddr < 1)) {
> >           ee->cur <<= 8;
> >           ee->cur |= data;
> >           ee->haveaddr++;
> > -        if (ee->haveaddr == 2) {
> > +        if ((ee->rsize > 256 && ee->haveaddr == 2) ||
> > +            (ee->rsize <= 256 && ee->haveaddr == 1)) {
> >               ee->cur %= ee->rsize;
> >               DPRINTK("Set pointer %04x\n", ee->cur);
> >           }
> 

Re: [PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[Philippe Mathieu-Daudé]

On 10/2/23 07:20, ~ssinprem wrote:
> From: Sittisak Sinprem <ssinprem@celestca.com>
> 
> ---
>   hw/nvram/eeprom_at24c.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 2d4d8b952f..693212b661 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -87,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
>       EEPROMState *ee = AT24C_EE(s);
>       uint8_t ret;
>   
> -    if (ee->haveaddr == 1) {
> +    if (ee->rsize > 256 && ee->haveaddr == 1) {
>           return 0xff;
>       }

What represents this '256' magic value? Please add a definition.

Re: [PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[Sittisak Sinprem]

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

Hi Philippe,

the EEPROM size less than or equal to 256 , such as 24c02
use only 1 byte to mention the memory (0 - 255) (0x00 - 0xff)

 write byte
  [ Start sAddr/W ] -> [ mem Addr ] -> [ mem data ] -> [ Stop ]

 read byte
  [ Start sAddr/W ] -> [ mem Addr ]
    -> [ reStart sAddr/R] -> [ mem data ]-> [ Stop ]

---------------------------------------------------------------
Meanwhile, in EEPROM size more 256 , such as 24c64 has size 8192 bytes
(0x2000)
use 2 bytes for pointing the memory (0x0000 - 0x2000 )

 write byte
  [ Start sAddr/W ] -> [ 1st mem Addr ] -> [ 2nd mem Addr ]
   -> [ Mem Data ] -> [ Stop ]

  read byte
  [ Start sAddr/W ] -> [ 1st mem Addr ] -> [ 2nd mem Addr ]
   -> [ reStart sAddr/R] -> [ mem data ]-> [ Stop ]

On Thu, Feb 16, 2023 at 3:25 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 10/2/23 07:20, ~ssinprem wrote:
> > From: Sittisak Sinprem <ssinprem@celestca.com>
> >
> > ---
> >   hw/nvram/eeprom_at24c.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> > index 2d4d8b952f..693212b661 100644
> > --- a/hw/nvram/eeprom_at24c.c
> > +++ b/hw/nvram/eeprom_at24c.c
> > @@ -87,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
> >       EEPROMState *ee = AT24C_EE(s);
> >       uint8_t ret;
> >
> > -    if (ee->haveaddr == 1) {
> > +    if (ee->rsize > 256 && ee->haveaddr == 1) {
> >           return 0xff;
> >       }
>
> What represents this '256' magic value? Please add a definition.
>

[-- Attachment #2: Type: text/html, Size: 2327 bytes --]

Re: [PATCH qemu 2/2] aspeed/fuji : correct the eeprom size

[Sittisak Sinprem]

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

From: Sittisak Sinprem <ssinprem@celestca.com>

Device 24C64 the size is 64 kilobits = 8kilobyte
Device 24C02 the size is 2 kilobits = 256byte

Signed-off-by: Sittisak Sinprem <ssinprem@celestca.com>
---
 hw/arm/aspeed.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 27dda58338..40f6076b44 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -840,42 +840,46 @@ static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
     i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
     i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);

-    at24c_eeprom_init(i2c[19], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[20], 0x50, 2 * KiB);
-    at24c_eeprom_init(i2c[22], 0x52, 2 * KiB);
+    /*
+    * EEPROM 24c64 size is 64Kbits or 8 Kbytes
+    *        24c02 size is 2Kbits or 256 bytes
+    */
+    at24c_eeprom_init(i2c[19], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[20], 0x50, 256);
+    at24c_eeprom_init(i2c[22], 0x52, 256);

     i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
     i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
     i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
     i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);

-    at24c_eeprom_init(i2c[8], 0x51, 64 * KiB);
+    at24c_eeprom_init(i2c[8], 0x51, 8 * KiB);
     i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);

     i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
-    at24c_eeprom_init(i2c[50], 0x52, 64 * KiB);
+    at24c_eeprom_init(i2c[50], 0x52, 8 * KiB);
     i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
     i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);

     i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
     i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);

-    at24c_eeprom_init(i2c[65], 0x53, 64 * KiB);
+    at24c_eeprom_init(i2c[65], 0x53, 8 * KiB);
     i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
     i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
-    at24c_eeprom_init(i2c[68], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[69], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[70], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[71], 0x52, 64 * KiB);
+    at24c_eeprom_init(i2c[68], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[69], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[70], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[71], 0x52, 8 * KiB);

-    at24c_eeprom_init(i2c[73], 0x53, 64 * KiB);
+    at24c_eeprom_init(i2c[73], 0x53, 8 * KiB);
     i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
     i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
-    at24c_eeprom_init(i2c[76], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[77], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[78], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[79], 0x52, 64 * KiB);
-    at24c_eeprom_init(i2c[28], 0x50, 2 * KiB);
+    at24c_eeprom_init(i2c[76], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[77], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[78], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[79], 0x52, 8 * KiB);
+    at24c_eeprom_init(i2c[28], 0x50, 256);

     for (int i = 0; i < 8; i++) {
         at24c_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
-- 
2.34.6

[-- Attachment #2: Type: text/html, Size: 4041 bytes --]

Re: [PATCH qemu 1/2] hw/at24c : modify at24c to support 1 byte address mode

[Sittisak Sinprem]

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

From: Sittisak Sinprem <ssinprem@celestca.com>

Signed-off-by: Sittisak Sinprem <ssinprem@celestca.com>
---
 hw/nvram/eeprom_at24c.c | 46 +++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 3328c32814..0cb650d635 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -41,6 +41,12 @@ struct EEPROMState {
     uint16_t cur;
     /* total size in bytes */
     uint32_t rsize;
+    /* address byte number
+     *  for  24c01, 24c02 size <= 256 byte, use only 1 byte
+     *  otherwise size > 256, use 2 byte
+     */
+    uint8_t asize;
+
     bool writable;
     /* cells changed since last START? */
     bool changed;
@@ -91,7 +97,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
     EEPROMState *ee = AT24C_EE(s);
     uint8_t ret;

-    if (ee->haveaddr == 1) {
+    if (ee->haveaddr > 0 && ee->haveaddr < ee->asize) {
         return 0xff;
     }

@@ -108,11 +114,11 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
 {
     EEPROMState *ee = AT24C_EE(s);

-    if (ee->haveaddr < 2) {
+    if (ee->haveaddr < ee->asize) {
         ee->cur <<= 8;
         ee->cur |= data;
         ee->haveaddr++;
-        if (ee->haveaddr == 2) {
+        if (ee->haveaddr == ee->asize) {
             ee->cur %= ee->rsize;
             DPRINTK("Set pointer %04x\n", ee->cur);
         }
@@ -184,6 +190,29 @@ static void at24c_eeprom_realize(DeviceState *dev,
Error **errp)
     }

     ee->mem = g_malloc0(ee->rsize);
+
+    /*
+     * If address size didn't define with property set
+     *  setting it from Rom size
+     */
+    if (ee->asize == 0) {
+        if (ee->rsize <= 256) {
+            ee->asize = 1;
+        } else {
+            ee->asize = 2;
+        }
+    }
+}
+
+static
+void at24c_eeprom_reset(DeviceState *state)
+{
+    EEPROMState *ee = AT24C_EE(state);
+
+    ee->changed = false;
+    ee->cur = 0;
+    ee->haveaddr = 0;
+
     memset(ee->mem, 0, ee->rsize);

     if (ee->init_rom) {
@@ -201,18 +230,9 @@ static void at24c_eeprom_realize(DeviceState *dev,
Error **errp)
     }
 }

-static
-void at24c_eeprom_reset(DeviceState *state)
-{
-    EEPROMState *ee = AT24C_EE(state);
-
-    ee->changed = false;
-    ee->cur = 0;
-    ee->haveaddr = 0;
-}
-
 static Property at24c_eeprom_props[] = {
     DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+    DEFINE_PROP_UINT8("address-size", EEPROMState, asize, 0),
     DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
     DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
     DEFINE_PROP_END_OF_LIST()
-- 
2.34.6

[-- Attachment #2: Type: text/html, Size: 3886 bytes --]

Re: [PATCH qemu 2/2] aspeed/fuji : correct the eeprom size

[Cédric Le Goater]

Hello Sittisak,

On 2/16/23 15:23, Sittisak Sinprem wrote:
> From: Sittisak Sinprem <ssinprem@celestca.com <mailto:ssinprem@celestca.com>>
> 
> Device 24C64 the size is 64 kilobits = 8kilobyte
> Device 24C02 the size is 2 kilobits = 256byte
> 
> Signed-off-by: Sittisak Sinprem <ssinprem@celestca.com <mailto:ssinprem@celestca.com>>

Could you please address the comments on patch 1 also and resend both
patches as a v2 ?

You can use ./scripts/get_maintainer.pl to get the list of persons to send to,
which should be :

  git send-email --to qemu-arm@nongnu.org --to qemu-devel@nongnu.org \
     --cc "Joel Stanley <joel@jms.id.au>" \
     --cc "Andrew Jeffery <andrew@aj.id.au>" \
     --cc "Peter Maydell <peter.maydell@linaro.org>" \
     --cc "Cédric Le Goater <clg@kaod.org>" \
     ./v2-000*patches

Thanks,

C.


> ---
>   hw/arm/aspeed.c | 36 ++++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 27dda58338..40f6076b44 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -840,42 +840,46 @@ static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
>       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
>       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
> 
> -    at24c_eeprom_init(i2c[19], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[20], 0x50, 2 * KiB);
> -    at24c_eeprom_init(i2c[22], 0x52, 2 * KiB);
> +    /*
> +    * EEPROM 24c64 size is 64Kbits or 8 Kbytes
> +    *        24c02 size is 2Kbits or 256 bytes
> +    */
> +    at24c_eeprom_init(i2c[19], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[20], 0x50, 256);
> +    at24c_eeprom_init(i2c[22], 0x52, 256);
> 
>       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
>       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
>       i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
>       i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
> 
> -    at24c_eeprom_init(i2c[8], 0x51, 64 * KiB);
> +    at24c_eeprom_init(i2c[8], 0x51, 8 * KiB);
>       i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
> 
>       i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
> -    at24c_eeprom_init(i2c[50], 0x52, 64 * KiB);
> +    at24c_eeprom_init(i2c[50], 0x52, 8 * KiB);
>       i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
>       i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
> 
>       i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
>       i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
> 
> -    at24c_eeprom_init(i2c[65], 0x53, 64 * KiB);
> +    at24c_eeprom_init(i2c[65], 0x53, 8 * KiB);
>       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
>       i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
> -    at24c_eeprom_init(i2c[68], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[69], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[70], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[71], 0x52, 64 * KiB);
> +    at24c_eeprom_init(i2c[68], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[69], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[70], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[71], 0x52, 8 * KiB);
> 
> -    at24c_eeprom_init(i2c[73], 0x53, 64 * KiB);
> +    at24c_eeprom_init(i2c[73], 0x53, 8 * KiB);
>       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
>       i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
> -    at24c_eeprom_init(i2c[76], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[77], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[78], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[79], 0x52, 64 * KiB);
> -    at24c_eeprom_init(i2c[28], 0x50, 2 * KiB);
> +    at24c_eeprom_init(i2c[76], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[77], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[78], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[79], 0x52, 8 * KiB);
> +    at24c_eeprom_init(i2c[28], 0x50, 256);
> 
>       for (int i = 0; i < 8; i++) {
>           at24c_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
> -- 
> 2.34.6
>