diff mbox series

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

Message ID 167636845806.2107.2382940753543768501-1@git.sr.ht
State New
Headers show
Series hw/at24c support eeprom size less than equal 256 byte | expand

Commit Message

~ssinprem Feb. 10, 2023, 6:20 a.m. UTC 
From: Sittisak Sinprem <ssinprem@celestca.com>

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

Comments

Cédric Le Goater Feb. 14, 2023, 2:29 p.m. UTC | #1 
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);
>           }
Peter Delevoryas Feb. 15, 2023, 8:39 p.m. UTC | #2 
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);
> >           }
>
Philippe Mathieu-Daudé Feb. 16, 2023, 8:25 a.m. UTC | #3 
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.
Sittisak Sinprem Feb. 16, 2023, 8:47 a.m. UTC | #4 
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.
>
Sittisak Sinprem Feb. 16, 2023, 2:27 p.m. UTC | #5 
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()
diff mbox series

Patch

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);
         }