[Qemu-devel] [PATCH 0/5] cmd646 tidy-up and interrupt status fixes

[Qemu-devel] [PATCH 0/5] cmd646 tidy-up and interrupt status fixes

[Mark Cave-Ayland]

This patchset came out of my work trying to boot NetBSD on SPARC64.

According to the datasheet, the 646U2 UDMA interrupt status bits are exact 
mirrors of the normal DMA interrupt status bits, and an interrupt can be
cleared by writing a 1 to the relevant bit in PCI configuration space.

The existing implementation caused NetBSD to fail since it would always check
and clear the normal DMA interrupt status bit, even if UDMA was being used.
Hence this patchset ensures that the current interrupt status is always 
consistent between both normal DMA and UDMA registers, including when either 
one of the interrupt status bits is cleared by writing to PCI configuration 
space.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (5):
  cmd646: add constants for CNTRL register access
  cmd646: synchronise DMA interrupt status with UDMA interrupt status
  cmd646: switch cmd646_update_irq() to accept PCIDevice instead of
    PCIIDEState
  cmd646: allow MRDMODE interrupt status bits clearing from PCI config
    space
  cmd646: synchronise UDMA interrupt status with DMA interrupt status

 hw/ide/cmd646.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 9 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/5] cmd646: add constants for CNTRL register access

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a8e35fe..d8395ef 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -33,6 +33,9 @@
 #include <hw/ide/pci.h>
 
 /* CMD646 specific */
+#define CNTRL		0x51
+#define   CNTRL_EN_CH0	0x04
+#define   CNTRL_EN_CH1	0x08
 #define MRDMODE		0x71
 #define   MRDMODE_INTR_CH0	0x04
 #define   MRDMODE_INTR_CH1	0x08
@@ -269,10 +272,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
 
     pci_conf[PCI_CLASS_PROG] = 0x8f;
 
-    pci_conf[0x51] = 0x04; // enable IDE0
+    pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
     if (d->secondary) {
         /* XXX: if not enabled, really disable the seconday IDE controller */
-        pci_conf[0x51] |= 0x08; /* enable IDE1 */
+        pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
     }
 
     setup_cmd646_bar(d, 0);
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/5] cmd646: synchronise DMA interrupt status with UDMA interrupt status

[Mark Cave-Ayland]

Make sure that the standard DMA interrupt status bits reflect any changes made
to the UDMA interrupt status bits. The CMD646U2 datasheet claims that these
bits are equivalent, and they must be synchronised for guests that manipulate
both registers.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index d8395ef..c3c6c53 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -33,9 +33,13 @@
 #include <hw/ide/pci.h>
 
 /* CMD646 specific */
+#define CFR		0x50
+#define   CFR_INTR_CH0	0x04
 #define CNTRL		0x51
 #define   CNTRL_EN_CH0	0x04
 #define   CNTRL_EN_CH1	0x08
+#define ARTTIM23	0x57
+#define    ARTTIM23_INTR_CH1	0x10
 #define MRDMODE		0x71
 #define   MRDMODE_INTR_CH0	0x04
 #define   MRDMODE_INTR_CH1	0x08
@@ -126,6 +130,22 @@ static void setup_cmd646_bar(PCIIDEState *d, int bus_num)
                           "cmd646-data", 8);
 }
 
+static void cmd646_update_dma_interrupts(PCIDevice *pd)
+{
+    /* Sync DMA interrupt status from UDMA interrupt status */
+    if (pd->config[MRDMODE] & MRDMODE_INTR_CH0) {
+        pd->config[CFR] |= CFR_INTR_CH0;
+    } else {
+        pd->config[CFR] &= ~CFR_INTR_CH0;
+    }
+
+    if (pd->config[MRDMODE] & MRDMODE_INTR_CH1) {
+        pd->config[ARTTIM23] |= ARTTIM23_INTR_CH1;
+    } else {
+        pd->config[ARTTIM23] &= ~ARTTIM23_INTR_CH1;
+    }
+}
+
 static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
 {
@@ -184,6 +204,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
     case 1:
         pci_dev->config[MRDMODE] =
             (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
+        cmd646_update_dma_interrupts(pci_dev);
         cmd646_update_irq(bm->pci_dev);
         break;
     case 2:
@@ -249,6 +270,7 @@ static void cmd646_set_irq(void *opaque, int channel, int level)
     } else {
         pd->config[MRDMODE] &= ~irq_mask;
     }
+    cmd646_update_dma_interrupts(pd);
     cmd646_update_irq(d);
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/5] cmd646: switch cmd646_update_irq() to accept PCIDevice instead of PCIIDEState

[Mark Cave-Ayland]

This is in preparation for adding configuration space accessors which accept
PCIDevice as a parameter.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c3c6c53..11a3e52 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -48,7 +48,7 @@
 #define UDIDETCR0	0x73
 #define UDIDETCR1	0x7B
 
-static void cmd646_update_irq(PCIIDEState *d);
+static void cmd646_update_irq(PCIDevice *pd);
 
 static uint64_t cmd646_cmd_read(void *opaque, hwaddr addr,
                                 unsigned size)
@@ -205,7 +205,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
         pci_dev->config[MRDMODE] =
             (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
         cmd646_update_dma_interrupts(pci_dev);
-        cmd646_update_irq(bm->pci_dev);
+        cmd646_update_irq(pci_dev);
         break;
     case 2:
         bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
@@ -245,9 +245,8 @@ static void bmdma_setup_bar(PCIIDEState *d)
 
 /* XXX: call it also when the MRDMODE is changed from the PCI config
    registers */
-static void cmd646_update_irq(PCIIDEState *d)
+static void cmd646_update_irq(PCIDevice *pd)
 {
-    PCIDevice *pd = PCI_DEVICE(d);
     int pci_level;
 
     pci_level = ((pd->config[MRDMODE] & MRDMODE_INTR_CH0) &&
@@ -271,7 +270,7 @@ static void cmd646_set_irq(void *opaque, int channel, int level)
         pd->config[MRDMODE] &= ~irq_mask;
     }
     cmd646_update_dma_interrupts(pd);
-    cmd646_update_irq(d);
+    cmd646_update_irq(pd);
 }
 
 static void cmd646_reset(void *opaque)
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/5] cmd646: allow MRDMODE interrupt status bits clearing from PCI config space

[Mark Cave-Ayland]

Make sure that we also update the normal DMA interrupt status bits at the
same time, and alter the IRQ if being cleared accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c |   32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 11a3e52..b8dc4ab 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -243,8 +243,6 @@ static void bmdma_setup_bar(PCIIDEState *d)
     }
 }
 
-/* XXX: call it also when the MRDMODE is changed from the PCI config
-   registers */
 static void cmd646_update_irq(PCIDevice *pd)
 {
     int pci_level;
@@ -283,6 +281,30 @@ static void cmd646_reset(void *opaque)
     }
 }
 
+static uint32_t cmd646_pci_config_read(PCIDevice *d,
+                                       uint32_t address, int len)
+{
+    return pci_default_read_config(d, address, len);
+}
+
+static void cmd646_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t val,
+                                    int l)
+{
+    uint32_t i;
+
+    pci_default_write_config(d, addr, val, l);
+
+    for (i = addr; i < addr + l; i++) {
+        switch (i) {
+        case MRDMODE:
+            cmd646_update_dma_interrupts(d);
+            break;
+        }
+    }
+
+    cmd646_update_irq(d);
+}
+
 /* CMD646 PCI IDE controller */
 static int pci_cmd646_ide_initfn(PCIDevice *dev)
 {
@@ -299,6 +321,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
         pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
     }
 
+    /* Set write-to-clear interrupt bits */
+    dev->wmask[MRDMODE] = 0x0;
+    dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
+
     setup_cmd646_bar(d, 0);
     setup_cmd646_bar(d, 1);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd646_bar[0].data);
@@ -371,6 +397,8 @@ static void cmd646_ide_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_CMD_646;
     k->revision = 0x07;
     k->class_id = PCI_CLASS_STORAGE_IDE;
+    k->config_read = cmd646_pci_config_read;
+    k->config_write = cmd646_pci_config_write;
     dc->props = cmd646_ide_properties;
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/5] cmd646: synchronise UDMA interrupt status with DMA interrupt status

[Mark Cave-Ayland]

Make sure that both registers are synchronised when being accessed through
PCI configuration space.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index b8dc4ab..74d0deb 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -146,6 +146,22 @@ static void cmd646_update_dma_interrupts(PCIDevice *pd)
     }
 }
 
+static void cmd646_update_udma_interrupts(PCIDevice *pd)
+{
+    /* Sync UDMA interrupt status from DMA interrupt status */
+    if (pd->config[CFR] & CFR_INTR_CH0) {
+        pd->config[MRDMODE] |= MRDMODE_INTR_CH0;
+    } else {
+        pd->config[MRDMODE] &= ~MRDMODE_INTR_CH0;
+    }
+
+    if (pd->config[ARTTIM23] & ARTTIM23_INTR_CH1) {
+        pd->config[MRDMODE] |= MRDMODE_INTR_CH1;
+    } else {
+        pd->config[MRDMODE] &= ~MRDMODE_INTR_CH1;
+    }
+}
+
 static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
 {
@@ -296,6 +312,10 @@ static void cmd646_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t val,
 
     for (i = addr; i < addr + l; i++) {
         switch (i) {
+        case CFR:
+        case ARTTIM23:
+            cmd646_update_udma_interrupts(d);
+            break;
         case MRDMODE:
             cmd646_update_dma_interrupts(d);
             break;
@@ -322,6 +342,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     }
 
     /* Set write-to-clear interrupt bits */
+    dev->wmask[CFR] = 0x0;
+    dev->w1cmask[CFR] = CFR_INTR_CH0;
+    dev->wmask[ARTTIM23] = 0x0;
+    dev->w1cmask[ARTTIM23] = ARTTIM23_INTR_CH1;
     dev->wmask[MRDMODE] = 0x0;
     dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 5/5] cmd646: synchronise UDMA interrupt status with DMA interrupt status

[Stefan Hajnoczi]

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

On Fri, Aug 08, 2014 at 05:23:36PM +0100, Mark Cave-Ayland wrote:
> @@ -322,6 +342,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>      }
>  
>      /* Set write-to-clear interrupt bits */
> +    dev->wmask[CFR] = 0x0;
> +    dev->w1cmask[CFR] = CFR_INTR_CH0;
> +    dev->wmask[ARTTIM23] = 0x0;
> +    dev->w1cmask[ARTTIM23] = ARTTIM23_INTR_CH1;
>      dev->wmask[MRDMODE] = 0x0;
>      dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;

It is not clear to me why the mask for MRDMODE has both Channel 0 and 1
but the ARTTIM23 and CFR masks only have one channel each.

Please post a link to the datasheet.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/5] cmd646 tidy-up and interrupt status fixes

[Stefan Hajnoczi]

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

On Fri, Aug 08, 2014 at 05:23:31PM +0100, Mark Cave-Ayland wrote:
> This patchset came out of my work trying to boot NetBSD on SPARC64.
> 
> According to the datasheet, the 646U2 UDMA interrupt status bits are exact 
> mirrors of the normal DMA interrupt status bits, and an interrupt can be
> cleared by writing a 1 to the relevant bit in PCI configuration space.
> 
> The existing implementation caused NetBSD to fail since it would always check
> and clear the normal DMA interrupt status bit, even if UDMA was being used.
> Hence this patchset ensures that the current interrupt status is always 
> consistent between both normal DMA and UDMA registers, including when either 
> one of the interrupt status bits is cleared by writing to PCI configuration 
> space.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Mark Cave-Ayland (5):
>   cmd646: add constants for CNTRL register access
>   cmd646: synchronise DMA interrupt status with UDMA interrupt status
>   cmd646: switch cmd646_update_irq() to accept PCIDevice instead of
>     PCIIDEState
>   cmd646: allow MRDMODE interrupt status bits clearing from PCI config
>     space
>   cmd646: synchronise UDMA interrupt status with DMA interrupt status
> 
>  hw/ide/cmd646.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 85 insertions(+), 9 deletions(-)

Looks good but I don't know the CMD646 registers.  I left a question
about the last patch.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 5/5] cmd646: synchronise UDMA interrupt status with DMA interrupt status

[Mark Cave-Ayland]

On 11/08/14 16:12, Stefan Hajnoczi wrote:

> On Fri, Aug 08, 2014 at 05:23:36PM +0100, Mark Cave-Ayland wrote:
>> @@ -322,6 +342,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>>       }
>>
>>       /* Set write-to-clear interrupt bits */
>> +    dev->wmask[CFR] = 0x0;
>> +    dev->w1cmask[CFR] = CFR_INTR_CH0;
>> +    dev->wmask[ARTTIM23] = 0x0;
>> +    dev->w1cmask[ARTTIM23] = ARTTIM23_INTR_CH1;
>>       dev->wmask[MRDMODE] = 0x0;
>>       dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
>
> It is not clear to me why the mask for MRDMODE has both Channel 0 and 1
> but the ARTTIM23 and CFR masks only have one channel each.
>
> Please post a link to the datasheet.

Hi Stefan,

Thanks for the review. You can find a copy of the 646U2 datasheet at 
http://gkernel.sourceforge.net/specs/sii/PCI0646U2Specr030399.PDF.bz2.

My understanding from the datasheet is that CFR is the primary channel 
interrupt whilst ARTTIM23 is the secondary channel interrupt, and the 
note on page 28 explains that these bits are also accessible via the 
relevant bits of the MRDMODE register.

This fixes cmd646 under NetBSD since it programs UDMA via MRDMODE but 
clears the interrupts via CFR and ARTTIM23, so without this patchset the 
driver hangs because the interrupt isn't properly cleared across both 
registers. And I can also verify that Linux experiences no regressions 
with this patchset applied.


Many thanks,

Mark.

Re: [Qemu-devel] [PATCH 5/5] cmd646: synchronise UDMA interrupt status with DMA interrupt status

[Stefan Hajnoczi]

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

On Mon, Aug 11, 2014 at 04:33:01PM +0100, Mark Cave-Ayland wrote:
> On 11/08/14 16:12, Stefan Hajnoczi wrote:
> 
> >On Fri, Aug 08, 2014 at 05:23:36PM +0100, Mark Cave-Ayland wrote:
> >>@@ -322,6 +342,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
> >>      }
> >>
> >>      /* Set write-to-clear interrupt bits */
> >>+    dev->wmask[CFR] = 0x0;
> >>+    dev->w1cmask[CFR] = CFR_INTR_CH0;
> >>+    dev->wmask[ARTTIM23] = 0x0;
> >>+    dev->w1cmask[ARTTIM23] = ARTTIM23_INTR_CH1;
> >>      dev->wmask[MRDMODE] = 0x0;
> >>      dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
> >
> >It is not clear to me why the mask for MRDMODE has both Channel 0 and 1
> >but the ARTTIM23 and CFR masks only have one channel each.
> >
> >Please post a link to the datasheet.
> 
> Hi Stefan,
> 
> Thanks for the review. You can find a copy of the 646U2 datasheet at
> http://gkernel.sourceforge.net/specs/sii/PCI0646U2Specr030399.PDF.bz2.
> 
> My understanding from the datasheet is that CFR is the primary channel
> interrupt whilst ARTTIM23 is the secondary channel interrupt, and the note
> on page 28 explains that these bits are also accessible via the relevant
> bits of the MRDMODE register.
> 
> This fixes cmd646 under NetBSD since it programs UDMA via MRDMODE but clears
> the interrupts via CFR and ARTTIM23, so without this patchset the driver
> hangs because the interrupt isn't properly cleared across both registers.
> And I can also verify that Linux experiences no regressions with this
> patchset applied.

Great, thanks!

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/5] cmd646 tidy-up and interrupt status fixes

[Stefan Hajnoczi]

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

On Fri, Aug 08, 2014 at 05:23:31PM +0100, Mark Cave-Ayland wrote:
> This patchset came out of my work trying to boot NetBSD on SPARC64.
> 
> According to the datasheet, the 646U2 UDMA interrupt status bits are exact 
> mirrors of the normal DMA interrupt status bits, and an interrupt can be
> cleared by writing a 1 to the relevant bit in PCI configuration space.
> 
> The existing implementation caused NetBSD to fail since it would always check
> and clear the normal DMA interrupt status bit, even if UDMA was being used.
> Hence this patchset ensures that the current interrupt status is always 
> consistent between both normal DMA and UDMA registers, including when either 
> one of the interrupt status bits is cleared by writing to PCI configuration 
> space.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Mark Cave-Ayland (5):
>   cmd646: add constants for CNTRL register access
>   cmd646: synchronise DMA interrupt status with UDMA interrupt status
>   cmd646: switch cmd646_update_irq() to accept PCIDevice instead of
>     PCIIDEState
>   cmd646: allow MRDMODE interrupt status bits clearing from PCI config
>     space
>   cmd646: synchronise UDMA interrupt status with DMA interrupt status
> 
>  hw/ide/cmd646.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 85 insertions(+), 9 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]