Patchwork Extend support of SMBUS(module pm_smbus.c) HST_STS register.

login
register
mail settings
Submitter Maksim Ratnikov
Date April 28, 2013, 6:26 p.m.
Message ID <CAAWx3aQybzcH8L4rxpNpSQsxwq_9F0KC3Aqhq4g6seKQLxy-jw@mail.gmail.com>
Download mbox | patch
Permalink /patch/240271/
State New
Headers show

Comments

Maksim Ratnikov - April 28, 2013, 6:26 p.m.
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(-)

         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:
Peter Maydell - April 28, 2013, 8:03 p.m.
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
Maksim Ratnikov - April 29, 2013, 10:39 p.m.
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.

Patch

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: