[Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Maksim Ratnikov]

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

Previous realization doesn't consider flags in the status register.
Add DS and INTR bits of HST_STS register set after transaction execution.
Update bits resetting in HST_STS register. Update error processing: if
DEV_ERR bit are set
transaction isn't execution.


Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com>
---
 hw/i2c/pm_smbus.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 0b5bb89..d5f5c56 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -50,9 +50,16 @@ static void smb_transaction(PMSMBus *s)
     i2c_bus *bus = s->smbus;

     SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
+    /* Transaction isn't exec if DEV_ERR bit set */
+    if ((s->smb_stat & 0x04) != 0)
+        goto error;
     switch(prot) {
     case 0x0:
         smbus_quick_command(bus, addr, read);
+        /* Set successfully transaction end:
+         * ByteDoneStatus = 1 (HST_STS bit #7) and INTR = 1 (HST_STS bit
#1)
+         */
+        s->smb_stat |= 0x82;
         break;
     case 0x1:
         if (read) {
@@ -60,6 +67,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_send_byte(bus, addr, cmd);
         }
+        s->smb_stat |= 0x82;
         break;
     case 0x2:
         if (read) {
@@ -67,6 +75,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_write_byte(bus, addr, cmd, s->smb_data0);
         }
+        s->smb_stat |= 0x82;
         break;
     case 0x3:
         if (read) {
@@ -77,6 +86,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) |
s->smb_data0);
         }
+        s->smb_stat |= 0x82;
         break;
     case 0x5:
         if (read) {
@@ -84,6 +94,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0);
         }
+        s->smb_stat |= 0x82;
         break;
     default:
         goto error;
@@ -102,7 +113,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr
addr, uint64_t val,
     SMBUS_DPRINTF("SMB writeb port=0x%04x val=0x%02x\n", addr, val);
     switch(addr) {
     case SMBHSTSTS:
-        s->smb_stat = 0;
+        s->smb_stat = (~(val & 0xff)) & s->smb_stat;
         s->smb_index = 0;
         break;
     case SMBHSTCNT:
-- 
1.7.3.4

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

Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Peter Maydell]

On 28 April 2013 19:26, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote:
> Previous realization doesn't consider flags in the status register.
> Add DS and INTR bits of HST_STS register set after transaction execution.
> Update bits resetting in HST_STS register. Update error processing: if
> DEV_ERR bit are set
> transaction isn't execution.

Hi; thanks for this patch. A minor comment below...

> Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com>
> ---
>  hw/i2c/pm_smbus.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 0b5bb89..d5f5c56 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -50,9 +50,16 @@ static void smb_transaction(PMSMBus *s)
>      i2c_bus *bus = s->smbus;
>
>      SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
> +    /* Transaction isn't exec if DEV_ERR bit set */
> +    if ((s->smb_stat & 0x04) != 0)
> +        goto error;

QEMU coding style requires braces on this if(). (You can run
your patch through scripts/codingstyle.pl to check for this
kind of thing.)

>      switch(prot) {
>      case 0x0:
>          smbus_quick_command(bus, addr, read);
> +        /* Set successfully transaction end:
> +         * ByteDoneStatus = 1 (HST_STS bit #7) and INTR = 1 (HST_STS bit
> #1)
> +         */
> +        s->smb_stat |= 0x82;

I think it would be nicer to define some constants for the
HST_STS bits. Then you could write
           s->smb_stat |= STS_BYTE_DONE | STS_INTR;

and you wouldn't need to have the comment here explaining
the magic numbers. (feel free to pick better constant names,
ideally ones matching whatever the datasheet uses.)
(then you can use the constants for all the smb_stat uses).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Maksim Ratnikov]

Previous realization doesn't consider flags in the status register.
Add DS and INTR bits of HST_STS register set after transaction execution.
Update bits resetting in HST_STS register. Update error processing: if 
DEV_ERR bit are set
transaction isn't execution.


Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com>
---
  hw/i2c/pm_smbus.c |   25 +++++++++++++++++++++++--
  1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 0b5bb89..35f154e 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -32,6 +32,18 @@
  #define SMBHSTDAT1      0x06
  #define SMBBLKDAT       0x07

+#define STS_HOST_BUSY   (1)
+#define STS_INTR        (1<<1)
+#define STS_DEV_ERR     (1<<2)
+#define STS_BUS_ERR     (1<<3)
+#define STS_FAILED      (1<<4)
+#define STS_SMBALERT    (1<<5)
+#define STS_INUSE_STS   (1<<6)
+#define STS_BYTE_DONE   (1<<7)
+/* Signs of successfully transaction end :
+*  ByteDoneStatus = 1 (STS_BYTE_DONE) and INTR = 1 (STS_INTR )
+*/
+
  //#define DEBUG

  #ifdef DEBUG
@@ -50,9 +62,14 @@ static void smb_transaction(PMSMBus *s)
      i2c_bus *bus = s->smbus;

      SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
+    /* Transaction isn't exec if STS_DEV_ERR bit set */
+    if ((s->smb_stat & STS_DEV_ERR) != 0)  {
+            goto error;
+        }
      switch(prot) {
      case 0x0:
          smbus_quick_command(bus, addr, read);
+        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
          break;
      case 0x1:
          if (read) {
@@ -60,6 +77,7 @@ static void smb_transaction(PMSMBus *s)
          } else {
              smbus_send_byte(bus, addr, cmd);
          }
+        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
          break;
      case 0x2:
          if (read) {
@@ -67,6 +85,7 @@ static void smb_transaction(PMSMBus *s)
          } else {
              smbus_write_byte(bus, addr, cmd, s->smb_data0);
          }
+        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
          break;
      case 0x3:
          if (read) {
@@ -77,6 +96,7 @@ static void smb_transaction(PMSMBus *s)
          } else {
              smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) | 
s->smb_data0);
          }
+        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
          break;
      case 0x5:
          if (read) {
@@ -84,6 +104,7 @@ static void smb_transaction(PMSMBus *s)
          } else {
              smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0);
          }
+        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
          break;
      default:
          goto error;
@@ -91,7 +112,7 @@ static void smb_transaction(PMSMBus *s)
      return;

    error:
-    s->smb_stat |= 0x04;
+    s->smb_stat |= STS_DEV_ERR;
  }

  static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
@@ -102,7 +123,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr 
addr, uint64_t val,
      SMBUS_DPRINTF("SMB writeb port=0x%04x val=0x%02x\n", addr, val);
      switch(addr) {
      case SMBHSTSTS:
-        s->smb_stat = 0;
+        s->smb_stat = (~(val & 0xff)) & s->smb_stat;
          s->smb_index = 0;
          break;
      case SMBHSTCNT:
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Maksim Ratnikov]

29.04.2013 00:03, Peter Maydell пишет:
> On 28 April 2013 19:26, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote:
>> Previous realization doesn't consider flags in the status register.
>> Add DS and INTR bits of HST_STS register set after transaction execution.
>> Update bits resetting in HST_STS register. Update error processing: if
>> DEV_ERR bit are set
>> transaction isn't execution.
> Hi; thanks for this patch. A minor comment below...
>
>> Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com>
>> ---
>>   hw/i2c/pm_smbus.c |   13 ++++++++++++-
>>   1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
>> index 0b5bb89..d5f5c56 100644
>> --- a/hw/i2c/pm_smbus.c
>> +++ b/hw/i2c/pm_smbus.c
>> @@ -50,9 +50,16 @@ static void smb_transaction(PMSMBus *s)
>>       i2c_bus *bus = s->smbus;
>>
>>       SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
>> +    /* Transaction isn't exec if DEV_ERR bit set */
>> +    if ((s->smb_stat & 0x04) != 0)
>> +        goto error;
> QEMU coding style requires braces on this if(). (You can run
> your patch through scripts/codingstyle.pl to check for this
> kind of thing.)
>
>>       switch(prot) {
>>       case 0x0:
>>           smbus_quick_command(bus, addr, read);
>> +        /* Set successfully transaction end:
>> +         * ByteDoneStatus = 1 (HST_STS bit #7) and INTR = 1 (HST_STS bit
>> #1)
>> +         */
>> +        s->smb_stat |= 0x82;
> I think it would be nicer to define some constants for the
> HST_STS bits. Then you could write
>             s->smb_stat |= STS_BYTE_DONE | STS_INTR;
>
> and you wouldn't need to have the comment here explaining
> the magic numbers. (feel free to pick better constant names,
> ideally ones matching whatever the datasheet uses.)
> (then you can use the constants for all the smb_stat uses).
>
> thanks
> -- PMM
Thank you for your answer. I made some correction and sent new version 
of patch. I can't find script codingstyle.pl, but I use checkpatch.pl .  
It shows 0 error and 0 warning.

Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Peter Maydell]

On 27 June 2013 14:16, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote:
> Ping?
> I send update of my patch at April 29. Link for my patch at patchwork:
> http://patchwork.ozlabs.org/patch/240525/   . Link for my response in
> mailing-list:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg05850.html
> Can you tell: is my patch in?

No, your patch doesn't seem to have been committed yet.
Since pm_smbus is a PC device, you should CC the maintainer
of the PC platform, who is Anthony (and who I've cc'd).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Maksim Ratnikov]

27.06.2013 20:33, Peter Maydell пишет:
> On 27 June 2013 14:16, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote:
>> Ping?
>> I send update of my patch at April 29. Link for my patch at patchwork:
>> http://patchwork.ozlabs.org/patch/240525/   . Link for my response in
>> mailing-list:
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg05850.html
>> Can you tell: is my patch in?
> No, your patch doesn't seem to have been committed yet.
> Since pm_smbus is a PC device, you should CC the maintainer
> of the PC platform, who is Anthony (and who I've cc'd).
>
> thanks
> -- PMM
Thanks for you answer. I will wait his response.

Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Anthony Liguori]

Maksim Ratnikov <m.o.ratnikov@gmail.com> writes:

> 27.06.2013 20:33, Peter Maydell пишет:
>> On 27 June 2013 14:16, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote:
>>> Ping?
>>> I send update of my patch at April 29. Link for my patch at patchwork:
>>> http://patchwork.ozlabs.org/patch/240525/   . Link for my response in
>>> mailing-list:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg05850.html
>>> Can you tell: is my patch in?
>> No, your patch doesn't seem to have been committed yet.
>> Since pm_smbus is a PC device, you should CC the maintainer
>> of the PC platform, who is Anthony (and who I've cc'd).
>>
>> thanks
>> -- PMM
> Thanks for you answer. I will wait his response.

Please resubmit as a top level patch rebased against latest git HEAD.

Regards,

Anthony Liguori