Patchwork [06/17] eepro100: symbolic names for pci registers

login
register
mail settings
Submitter Michael S. Tsirkin
Date Dec. 10, 2009, 6:10 p.m.
Message ID <20091210181046.GG25707@redhat.com>
Download mbox | patch
Permalink /patch/40856/
State New
Headers show

Comments

Michael S. Tsirkin - Dec. 10, 2009, 6:10 p.m.
No functional changes. I verified that the generated binary
does not change in meaningful ways. Survived light usage
with linux guest.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/eepro100.c |   49 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 32 insertions(+), 17 deletions(-)
Stefan Weil - Jan. 7, 2010, 11:14 a.m.
Michael S. Tsirkin schrieb:
> No functional changes. I verified that the generated binary
> does not change in meaningful ways. Survived light usage
> with linux guest.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 2a9e3b5..82e3766 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> /* PCI Device ID depends on device and is set below. */
> /* PCI Command */
> + /* TODO: this is the default, do not override. */
> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
> /* PCI Status */
> - PCI_CONFIG_16(PCI_STATUS, 0x2800);
> + /* TODO: this seems to make no sense. */
> + /* TODO: Value at RST# should be 0. */
> + PCI_CONFIG_16(PCI_STATUS,
> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> /* PCI Revision ID */
Hi,

this PCI status value is wrong. The correct value for PCI_STATUS is 0x0280
and was fixed in the maintainer version in 2007:
http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79

It was also fixed in a patch sent to qemu-devel (which was never applied):
http://patchwork.ozlabs.org/patch/33962/

I'll send a new patch which fixes the wrong status value.

Antony, how can we speed up the synchronisation process for
eepro100.c? Today, patches get lost without feedback.
This is no wonder because you have to work on hundreds of patches.
It was suggested that I should send patch series.
My last patch serie with 3 patches is ready for integration
since 2009-12-20.

Paul, if I had commit rights, I could integrate the missing
parts myself and maintain eepro100.c in the future. Of course
I'd send the single changes to the list before comitting them.
Don't you think that would be the best solution for all of us?

Regards,
Stefan
Michael S. Tsirkin - Jan. 7, 2010, 12:51 p.m.
On Thu, Jan 07, 2010 at 12:14:17PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > No functional changes. I verified that the generated binary
> > does not change in meaningful ways. Survived light usage
> > with linux guest.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
> > 1 files changed, 32 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index 2a9e3b5..82e3766 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
> > pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> > /* PCI Device ID depends on device and is set below. */
> > /* PCI Command */
> > + /* TODO: this is the default, do not override. */
> > PCI_CONFIG_16(PCI_COMMAND, 0x0000);
> > /* PCI Status */
> > - PCI_CONFIG_16(PCI_STATUS, 0x2800);
> > + /* TODO: this seems to make no sense. */
> > + /* TODO: Value at RST# should be 0. */
> > + PCI_CONFIG_16(PCI_STATUS,
> > + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> > /* PCI Revision ID */
> Hi,
> 
> this PCI status value is wrong. The correct value for PCI_STATUS is 0x0280
> and was fixed in the maintainer version in 2007:
> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79
> 
> It was also fixed in a patch sent to qemu-devel (which was never applied):
> http://patchwork.ozlabs.org/patch/33962/
> 
> I'll send a new patch which fixes the wrong status value.
> 
> Antony, how can we speed up the synchronisation process for
> eepro100.c? Today, patches get lost without feedback.
> This is no wonder because you have to work on hundreds of patches.
> It was suggested that I should send patch series.
> My last patch serie with 3 patches is ready for integration
> since 2009-12-20.

A simple solution that worked for me is to publish a git tree and send
pull requests (in addition to individual patches).  This guarantees that
nothing is lost and makes it easy for Anthony to apply a set of changes
in one go. git merge also has smarter heuristics than git am.

I put the patch you just posted on my tree as it's PCI related and I
understand it, but if you create your own tree and want me to drop this
patch from my tree, pls let me know and I will.

> Paul, if I had commit rights, I could integrate the missing
> parts myself and maintain eepro100.c in the future. Of course
> I'd send the single changes to the list before comitting them.
> Don't you think that would be the best solution for all of us?
> 
> Regards,
> Stefan

Personally, I find it comforting that someone else pulls my tree instead
of commit rights: this ensures at least build testing is done before
my changes are inflicted on others :)
Anthony Liguori - Jan. 7, 2010, 1:32 p.m.
On 01/07/2010 05:14 AM, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
>    
>> No functional changes. I verified that the generated binary
>> does not change in meaningful ways. Survived light usage
>> with linux guest.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> ---
>> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
>> 1 files changed, 32 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 2a9e3b5..82e3766 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
>> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
>> /* PCI Device ID depends on device and is set below. */
>> /* PCI Command */
>> + /* TODO: this is the default, do not override. */
>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>> /* PCI Status */
>> - PCI_CONFIG_16(PCI_STATUS, 0x2800);
>> + /* TODO: this seems to make no sense. */
>> + /* TODO: Value at RST# should be 0. */
>> + PCI_CONFIG_16(PCI_STATUS,
>> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>> /* PCI Revision ID */
>>      
> Hi,
>
> this PCI status value is wrong. The correct value for PCI_STATUS is 0x0280
> and was fixed in the maintainer version in 2007:
> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79
>
> It was also fixed in a patch sent to qemu-devel (which was never applied):
> http://patchwork.ozlabs.org/patch/33962/
>
> I'll send a new patch which fixes the wrong status value.
>
> Antony, how can we speed up the synchronisation process for
> eepro100.c? Today, patches get lost without feedback.
> This is no wonder because you have to work on hundreds of patches.
> It was suggested that I should send patch series.
> My last patch serie with 3 patches is ready for integration
> since 2009-12-20.
>    

The big problem with eepro100 is that you're doing a bunch of work in an 
external tree, and then trickling things slowly onto the list.  Take 
some time and send out one big series getting your internal tree in 
sync.  Of course, the end target must conform to CodingStyle which is a 
problem right now in your tree.

> Paul, if I had commit rights, I could integrate the missing
> parts myself and maintain eepro100.c in the future. Of course
> I'd send the single changes to the list before comitting them.
> Don't you think that would be the best solution for all of us?
>    

Pull requests work just as well as commit rights.  However, pull 
requests only work when the patches are ready to go and don't need 
iteration.  A bare minimum for that is going to be conforming to 
CodingStyle which has been a problem with eepro100.

But look, you send out patches during a holiday, and we're still 
catching up.  If it had been during a normal time period, that would be 
one thing, but please exercise a bit of patience.

Regards,

Anthony Liguori
Anthony Liguori - Jan. 7, 2010, 1:41 p.m.
On 01/07/2010 06:51 AM, Michael S. Tsirkin wrote:
> A simple solution that worked for me is to publish a git tree and send
> pull requests (in addition to individual patches).  This guarantees that
> nothing is lost and makes it easy for Anthony to apply a set of changes
> in one go. git merge also has smarter heuristics than git am.
>
> I put the patch you just posted on my tree as it's PCI related and I
> understand it, but if you create your own tree and want me to drop this
> patch from my tree, pls let me know and I will.
>    

I would actually be happy to pull eepro100 changes through Michael's tree.

Michael has been very good about reviewing patches quickly and so far, 
it's been working pretty well.

Regards,

Anthony Liguori
Stefan Weil - Jan. 7, 2010, 2:26 p.m.
Anthony Liguori schrieb:
> On 01/07/2010 05:14 AM, Stefan Weil wrote:
>> Michael S. Tsirkin schrieb:
>>   
>>> No functional changes. I verified that the generated binary
>>> does not change in meaningful ways. Survived light usage
>>> with linux guest.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>> ---
>>> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
>>> 1 files changed, 32 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>> index 2a9e3b5..82e3766 100644
>>> --- a/hw/eepro100.c
>>> +++ b/hw/eepro100.c
>>> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
>>> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
>>> /* PCI Device ID depends on device and is set below. */
>>> /* PCI Command */
>>> + /* TODO: this is the default, do not override. */
>>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>>> /* PCI Status */
>>> - PCI_CONFIG_16(PCI_STATUS, 0x2800);
>>> + /* TODO: this seems to make no sense. */
>>> + /* TODO: Value at RST# should be 0. */
>>> + PCI_CONFIG_16(PCI_STATUS,
>>> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>> /* PCI Revision ID */
>>>      
>> Hi,
>>
>> this PCI status value is wrong. The correct value for PCI_STATUS is
>> 0x0280
>> and was fixed in the maintainer version in 2007:
>> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79
>>
>>
>> It was also fixed in a patch sent to qemu-devel (which was never
>> applied):
>> http://patchwork.ozlabs.org/patch/33962/
>>
>> I'll send a new patch which fixes the wrong status value.
>>
>> Antony, how can we speed up the synchronisation process for
>> eepro100.c? Today, patches get lost without feedback.
>> This is no wonder because you have to work on hundreds of patches.
>> It was suggested that I should send patch series.
>> My last patch serie with 3 patches is ready for integration
>> since 2009-12-20.
>>    
>
> The big problem with eepro100 is that you're doing a bunch of work in
> an external tree, and then trickling things slowly onto the list. 
> Take some time and send out one big series getting your internal tree
> in sync.  Of course, the end target must conform to CodingStyle which
> is a problem right now in your tree.
>
>> Paul, if I had commit rights, I could integrate the missing
>> parts myself and maintain eepro100.c in the future. Of course
>> I'd send the single changes to the list before comitting them.
>> Don't you think that would be the best solution for all of us?
>>    
>
> Pull requests work just as well as commit rights.  However, pull
> requests only work when the patches are ready to go and don't need
> iteration.  A bare minimum for that is going to be conforming to
> CodingStyle which has been a problem with eepro100.
>
> But look, you send out patches during a holiday, and we're still
> catching up.  If it had been during a normal time period, that would
> be one thing, but please exercise a bit of patience.
>
> Regards,
>
> Anthony Liguori


Hello Antony,

thank you for your fast feedback. I also think that coding style
is important, so don't hesitate to tell me when there are problems.

The only violations of coding style in eepro100.c I am aware of
are C++ comments and data types using _t. They are relicts from
the time when QEMU did not have a documented coding style,
were addressed in previous patches and are also fixed in new patches.

Are there other coding style issues which I did not see?

Regards,

Stefan Weil

Patch

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2a9e3b5..82e3766 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -412,19 +412,24 @@  static void pci_reset(EEPRO100State * s)
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
     /* PCI Device ID depends on device and is set below. */
     /* PCI Command */
+    /* TODO: this is the default, do not override. */
     PCI_CONFIG_16(PCI_COMMAND, 0x0000);
     /* PCI Status */
-    PCI_CONFIG_16(PCI_STATUS, 0x2800);
+    /* TODO: this seems to make no sense. */
+    /* TODO: Value at RST# should be 0. */
+    PCI_CONFIG_16(PCI_STATUS,
+                  PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
     /* PCI Revision ID */
     PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
+    /* TODO: this is the default, do not override. */
     /* PCI Class Code */
-    PCI_CONFIG_8(0x09, 0x00);
+    PCI_CONFIG_8(PCI_CLASS_PROG, 0x00);
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     /* PCI Cache Line Size */
     /* check cache line size!!! */
     //~ PCI_CONFIG_8(0x0c, 0x00);
     /* PCI Latency Timer */
-    PCI_CONFIG_8(0x0d, 0x20);   // latency timer = 32 clocks
+    PCI_CONFIG_8(PCI_LATENCY_TIMER, 0x20);   // latency timer = 32 clocks
     /* PCI Header Type */
     /* BIST (built-in self test) */
 #if defined(TARGET_I386)
@@ -446,16 +451,20 @@  static void pci_reset(EEPRO100State * s)
 #endif
 #endif
     /* Expansion ROM Base Address (depends on boot disable!!!) */
-    PCI_CONFIG_32(0x30, 0x00000000);
+    /* TODO: not needed, set when BAR is registered */
+    PCI_CONFIG_32(PCI_ROM_ADDRESS, PCI_BASE_ADDRESS_SPACE_MEMORY);
     /* Capability Pointer */
-    PCI_CONFIG_8(0x34, 0xdc);
+    /* TODO: revisions with power_management 1 use this but
+     * do not set new capability list bit in status register. */
+    PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0xdc);
     /* Interrupt Line */
     /* Interrupt Pin */
-    PCI_CONFIG_8(0x3d, 1);      // interrupt pin 0
+    /* TODO: RST# value should be 0 */
+    PCI_CONFIG_8(PCI_INTERRUPT_PIN, 1);      // interrupt pin 0
     /* Minimum Grant */
-    PCI_CONFIG_8(0x3e, 0x08);
+    PCI_CONFIG_8(PCI_MIN_GNT, 0x08);
     /* Maximum Latency */
-    PCI_CONFIG_8(0x3f, 0x18);
+    PCI_CONFIG_8(PCI_MAX_LAT, 0x18);
 
     switch (device) {
     case i82550:
@@ -479,52 +488,57 @@  static void pci_reset(EEPRO100State * s)
     case i82557A:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x01);
-        PCI_CONFIG_8(0x34, 0x00);
+        PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0x00);
         power_management = 0;
         break;
     case i82557B:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x02);
-        PCI_CONFIG_8(0x34, 0x00);
+        PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0x00);
         power_management = 0;
         break;
     case i82557C:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x03);
-        PCI_CONFIG_8(0x34, 0x00);
+        PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0x00);
         power_management = 0;
         break;
     case i82558A:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x04);
         s->stats_size = 76;
         s->has_extended_tcb_support = 1;
         break;
     case i82558B:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x05);
         s->stats_size = 76;
         s->has_extended_tcb_support = 1;
         break;
     case i82559A:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x06);
         s->stats_size = 80;
         s->has_extended_tcb_support = 1;
         break;
     case i82559B:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x07);
         s->stats_size = 80;
         s->has_extended_tcb_support = 1;
         break;
     case i82559C:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
         // TODO: Windows wants revision id 0x0c.
         PCI_CONFIG_8(PCI_REVISION_ID, 0x0c);
@@ -537,7 +551,8 @@  static void pci_reset(EEPRO100State * s)
         break;
     case i82559ER:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82551IT);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x09);
         s->stats_size = 80;
         s->has_extended_tcb_support = 1;