| Message ID | 20091210181046.GG25707@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 :)
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
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
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
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;
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(-)