[Qemu-trivial] [PATCH] SMBUS module update.

[Qemu-trivial] [PATCH] SMBUS module update.

[Maksim Ratnikov]

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

Subject: [PATCH] SMBUS module update. Previous realization doesn't consider
flags in the status register. In this patch processing of bits of DS and
INTR of the register HST_STS is added. Mechanism of clearing bits value in
the register HST_STS is updated. Error processing is updated: if DEV_ERR
bit are set transaction isn't evaluated.
Signed-off-by: Maksim Ratnikov <m.o.ratnikov@gmail.com>
---
 hw/pm_smbus.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
index ea1380c..3a7b42d 100644
--- a/hw/pm_smbus.c
+++ b/hw/pm_smbus.c
@@ -17,10 +17,10 @@
  * License along with this library; if not, see
  * <http://www.gnu.org/licenses/>.
  */
-#include "hw.h"
-#include "pc.h"
-#include "pm_smbus.h"
-#include "smbus.h"
+#include "hw/hw.h"
+#include "hw/pc.h"
+#include "hw/pm_smbus.h"
+#include "hw/smbus.h"

 /* no save/load? */

@@ -40,7 +40,6 @@
 # define SMBUS_DPRINTF(format, ...)     do { } while (0)
 #endif

-
 static void smb_transaction(PMSMBus *s)
 {
     uint8_t prot = (s->smb_ctl >> 2) & 0x07;
@@ -48,11 +47,16 @@ static void smb_transaction(PMSMBus *s)
     uint8_t cmd = s->smb_cmd;
     uint8_t addr = s->smb_addr >> 1;
     i2c_bus *bus = s->smbus;
-
+
+    if ( (s->smb_stat & 0x04) != 0) goto error;            // DEV_ERR
evaluate
+
+
     SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
     switch(prot) {
     case 0x0:
-        smbus_quick_command(bus, addr, read);
+        smbus_quick_command(bus, addr, read);
+     s->smb_stat |= 0x82;            // set ByteDoneStatus = 1 (bit #7)
and INTR = (bit #1)
+
         break;
     case 0x1:
         if (read) {
@@ -60,6 +64,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_send_byte(bus, addr, cmd);
         }
+     s->smb_stat |= 0x82;            // set ByteDoneStatus = 1 (bit #7)
and INTR = (bit #1)
         break;
     case 0x2:
         if (read) {
@@ -67,6 +72,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_write_byte(bus, addr, cmd, s->smb_data0);
         }
+     s->smb_stat |= 0x82;            // set ByteDoneStatus = 1 (bit #7)
and INTR = (bit #1)
         break;
     case 0x3:
         if (read) {
@@ -77,6 +83,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;            // set ByteDoneStatus = 1 (bit #7)
and INTR = (bit #1)
         break;
     case 0x5:
         if (read) {
@@ -84,6 +91,7 @@ static void smb_transaction(PMSMBus *s)
         } else {
             smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0);
         }
+     s->smb_stat |= 0x82;            // set ByteDoneStatus = 1 (bit #7)
and INTR = (bit #1)
         break;
     default:
         goto error;
@@ -102,7 +110,8 @@ 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;            // clear that
bits of smb_stat where _val_ have "1"
         s->smb_index = 0;
         break;
     case SMBHSTCNT:
@@ -140,6 +149,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr
addr, unsigned width)
     switch(addr) {
     case SMBHSTSTS:
         val = s->smb_stat;
+        //val = 0x1e;
         break;
     case SMBHSTCNT:
         s->smb_index = 0;
-- 
1.7.3.4

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

Re: [Qemu-trivial] [PATCH] SMBUS module update.

[Stefan Hajnoczi]

On Thu, Apr 25, 2013 at 02:20:18AM +0400, Maksim Ratnikov wrote:
> Subject: [PATCH] SMBUS module update. Previous realization doesn't consider

Please choose a more specific commit message, "SMBUS module update" does
not say what this patch does.

> flags in the status register. In this patch processing of bits of DS and
> INTR of the register HST_STS is added. Mechanism of clearing bits value in
> the register HST_STS is updated. Error processing is updated: if DEV_ERR
> bit are set transaction isn't evaluated.
> Signed-off-by: Maksim Ratnikov <m.o.ratnikov@gmail.com>
> ---
>  hw/pm_smbus.c |   26 ++++++++++++++++++--------
>  1 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
> index ea1380c..3a7b42d 100644
> --- a/hw/pm_smbus.c
> +++ b/hw/pm_smbus.c
> @@ -17,10 +17,10 @@
>   * License along with this library; if not, see
>   * <http://www.gnu.org/licenses/>.
>   */
> -#include "hw.h"
> -#include "pc.h"
> -#include "pm_smbus.h"
> -#include "smbus.h"
> +#include "hw/hw.h"
> +#include "hw/pc.h"
> +#include "hw/pm_smbus.h"
> +#include "hw/smbus.h"

Looks like this patch is against an outdated QEMU source tree.  Please
rebase onto qemu.git/master.

> 
>  /* no save/load? */
> 
> @@ -40,7 +40,6 @@
>  # define SMBUS_DPRINTF(format, ...)     do { } while (0)
>  #endif
> 
> -

Please do not make random whitespace changes.

>  static void smb_transaction(PMSMBus *s)
>  {
>      uint8_t prot = (s->smb_ctl >> 2) & 0x07;
> @@ -48,11 +47,16 @@ static void smb_transaction(PMSMBus *s)
>      uint8_t cmd = s->smb_cmd;
>      uint8_t addr = s->smb_addr >> 1;
>      i2c_bus *bus = s->smbus;
> -
> +
> +    if ( (s->smb_stat & 0x04) != 0) goto error;            // DEV_ERR
> evaluate
> +
> +
>      SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
>      switch(prot) {
>      case 0x0:
> -        smbus_quick_command(bus, addr, read);
> +        smbus_quick_command(bus, addr, read);
> +     s->smb_stat |= 0x82;            // set ByteDoneStatus = 1 (bit #7)
> and INTR = (bit #1)
> +

Indentation is off here.  Please use 4 spaces, see ./CODING_STYLE.

Line wrap is 80 characters, comments therefore usually look like this:

/* set ByteDoneStatus = 1 (bit #7) and INTR = (bit #1) */
s->smb_stat |= 0x82;

This particular comment suggests defining ByteDoneStatus and Intr
constants.  that way you don't need comments to explain the 0x82 magic
number.

> @@ -140,6 +149,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr
> addr, unsigned width)
>      switch(addr) {
>      case SMBHSTSTS:
>          val = s->smb_stat;
> +        //val = 0x1e;
>          break;
>      case SMBHSTCNT:
>          s->smb_index = 0;

Random commented out code that should be dropped?

Re: [Qemu-trivial] [PATCH] SMBUS module update.

[Stefan Hajnoczi]

On Thu, Apr 25, 2013 at 02:20:18AM +0400, Maksim Ratnikov wrote:
> Subject: [PATCH] SMBUS module update. Previous realization doesn't consider
> flags in the status register. In this patch processing of bits of DS and
> INTR of the register HST_STS is added. Mechanism of clearing bits value in
> the register HST_STS is updated. Error processing is updated: if DEV_ERR
> bit are set transaction isn't evaluated.
> Signed-off-by: Maksim Ratnikov <m.o.ratnikov@gmail.com>
> ---
>  hw/pm_smbus.c |   26 ++++++++++++++++++--------
>  1 files changed, 18 insertions(+), 8 deletions(-)

Also, please always send patches to qemu-devel.  They must be on the
QEMU mailing list even if they go through the trivial patches queue.

Stefan

Re: [Qemu-trivial] [PATCH] SMBUS module update.

[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: 2851 bytes --]

Re: [Qemu-trivial] [PATCH] SMBUS module update.

[Michael Tokarev]

28.04.2013 22:26, Maksim Ratnikov 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.

I'm not sure this qualifies for -trivial branch.  Maybe the device
itself isn't of first importance or trivial, but the change this
patch introduces are a bit more than qemu-trivial branch considers.
I think anyway - I'm new here in -trivial.

Also, please don't us HTML to format patches.

Thanks,

/mjt