| Message ID | 1407515016-26273-6-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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;
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(+)