Re: [Qemu-devel] [PATCH] e1000: Do reset when E1000_CTRL_RST bit is set.

2011-08-09 Thread Peter Maydell 
On 9 August 2011 15:10, Anthony PERARD  wrote:
> On Fri, Aug 5, 2011 at 17:53, Anthony Liguori  wrote:
>>
>> You'll break some GCCs with -Wall -Werror with this.  Please do:
>>
>> if ((val & E1000_CTRL_RST)) {
>
> :(, I never heard of this. But OK, I will do that.

Please don't unless Anthony L can actually demonstrate a compiler
which has this property and doesn't already complain about all
the existing equivalent code in qemu...

-- PMM

Re: [Qemu-devel] [PATCH] e1000: Do reset when E1000_CTRL_RST bit is set.

2011-08-09 Thread Anthony PERARD 
On Fri, Aug 5, 2011 at 17:53, Anthony Liguori  wrote:
>
> You'll break some GCCs with -Wall -Werror with this.  Please do:
>
> if ((val & E1000_CTRL_RST)) {

:(, I never heard of this. But OK, I will do that.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH] e1000: Do reset when E1000_CTRL_RST bit is set.

2011-08-05 Thread Peter Maydell 
On 5 August 2011 17:53, Anthony Liguori  wrote:
> You'll break some GCCs with -Wall -Werror with this.  Please do:
>
> if ((val & E1000_CTRL_RST)) {

Hmm? There's lots of examples of that in the codebase:
$ git grep 'if ([a-zA-Z]* & ' | wc -l
1558

'=' (assignment) needs those extra braces, but logical
ops don't AFAIK.

-- PMM

Re: [Qemu-devel] [PATCH] e1000: Do reset when E1000_CTRL_RST bit is set.

2011-08-05 Thread Richard Henderson 
On 08/05/2011 09:53 AM, Anthony Liguori wrote:
>> +if (val&  E1000_CTRL_RST) {
> 
> You'll break some GCCs with -Wall -Werror with this.  Please do:
> 
> if ((val & E1000_CTRL_RST)) {

Err, really?  What versions?

I don't recall that ever being true.


r~

Re: [Qemu-devel] [PATCH] e1000: Do reset when E1000_CTRL_RST bit is set.

2011-08-05 Thread Anthony Liguori 

On 08/05/2011 09:36 AM, Anthony PERARD wrote:

Signed-off-by: Anthony PERARD
---
  hw/e1000.c |   10 --
  1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 96d84f9..a1388e9 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -150,6 +150,8 @@ static const char phy_regcap[0x20] = {
  [PHY_ID2] = PHY_R,[M88E1000_PHY_SPEC_STATUS] = PHY_R
  };

+static void e1000_reset(void *opaque);
+
  static void
  ioport_map(PCIDevice *pci_dev, int region_num, pcibus_t addr,
 pcibus_t size, int type)
@@ -202,8 +204,12 @@ rxbufsize(uint32_t v)
  static void
  set_ctrl(E1000State *s, int index, uint32_t val)
  {
-/* RST is self clearing */
-s->mac_reg[CTRL] = val&  ~E1000_CTRL_RST;
+DBGOUT(IO, "set ctrl = %08x\n", val);
+if (val&  E1000_CTRL_RST) {


You'll break some GCCs with -Wall -Werror with this.  Please do:

if ((val & E1000_CTRL_RST)) {

Regards,

Anthony Liguori


+e1000_reset(s);
+return;
+}
+s->mac_reg[CTRL] = val;
  }

  static void

[Qemu-devel] [PATCH] e1000: Do reset when E1000_CTRL_RST bit is set.

2011-08-05 Thread Anthony PERARD 
Signed-off-by: Anthony PERARD 
---
 hw/e1000.c |   10 --
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 96d84f9..a1388e9 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -150,6 +150,8 @@ static const char phy_regcap[0x20] = {
 [PHY_ID2] = PHY_R, [M88E1000_PHY_SPEC_STATUS] = PHY_R
 };
 
+static void e1000_reset(void *opaque);
+
 static void
 ioport_map(PCIDevice *pci_dev, int region_num, pcibus_t addr,
pcibus_t size, int type)
@@ -202,8 +204,12 @@ rxbufsize(uint32_t v)
 static void
 set_ctrl(E1000State *s, int index, uint32_t val)
 {
-/* RST is self clearing */
-s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
+DBGOUT(IO, "set ctrl = %08x\n", val);
+if (val & E1000_CTRL_RST) {
+e1000_reset(s);
+return;
+}
+s->mac_reg[CTRL] = val;
 }
 
 static void
-- 
Anthony PERARD