| Submitter | Artyom Tarasenko |
|---|---|
| Date | May 21, 2010, 9:53 p.m. |
| Message ID | <1274478829-10840-1-git-send-email-atar4qemu@gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/53214/ |
| State | New |
| Headers | show |
Comments
On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko <atar4qemu@googlemail.com> wrote: > On a real hardware changing read-only bits has no effect > Use a mask common for SCSI and Ethernet registers. The crucial > bit is DMA_INTR, because setting or clearing it may produce > spurious interrupts. > > This patch allows booting Solaris 2.3 Great! > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > hw/sparc32_dma.c | 11 +++++++---- > 1 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c > index 3ceb851..d54e165 100644 > --- a/hw/sparc32_dma.c > +++ b/hw/sparc32_dma.c > @@ -62,6 +62,9 @@ > #define DMA_DRAIN_FIFO 0x40 > #define DMA_RESET 0x80 > > +/* XXX SCSI and ethernet should have different read-only bit masks */ > +#define DMA_CSR_RO_MASK 0xfe000007 I'm preparing (again) some generic DMA patches, it looks like I have to make Lance and ESP DMA controllers separate. Your patch highlights yet another problem with the current shared design. This part of the patch is fine. > + > typedef struct DMAState DMAState; > > struct DMAState { > @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) > switch (saddr) { > case 0: > if (val & DMA_INTREN) { > - if (val & DMA_INTR) { > + if (s->dmaregs[0] & DMA_INTR) { Doesn't this change the way irqs are raised so that a pending irq is only generated on the write access _after_ the access that enables the irq. Currently we check for pending irqs immediately when the irq is enabled. > DPRINTF("Raise IRQ\n"); > qemu_irq_raise(s->irq); > } > @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) > val &= ~DMA_DRAIN_FIFO; > } else if (val == 0) > val = DMA_DRAIN_FIFO; > - val &= 0x0fffffff; > + val &= ~DMA_CSR_RO_MASK; > val |= DMA_VER; > + s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val; > break; > case 1: > s->dmaregs[0] |= DMA_LOADED; > - break; A comment about fall through should be added. > default: > + s->dmaregs[saddr] = val; > break; > } > - s->dmaregs[saddr] = val; > } > > static CPUReadMemoryFunc * const dma_mem_read[3] = { > -- > 1.6.2.5 > >
2010/5/22 Blue Swirl <blauwirbel@gmail.com>: > On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko > <atar4qemu@googlemail.com> wrote: >> On a real hardware changing read-only bits has no effect >> Use a mask common for SCSI and Ethernet registers. The crucial >> bit is DMA_INTR, because setting or clearing it may produce >> spurious interrupts. >> >> This patch allows booting Solaris 2.3 > > Great! > >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >> --- >> hw/sparc32_dma.c | 11 +++++++---- >> 1 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c >> index 3ceb851..d54e165 100644 >> --- a/hw/sparc32_dma.c >> +++ b/hw/sparc32_dma.c >> @@ -62,6 +62,9 @@ >> #define DMA_DRAIN_FIFO 0x40 >> #define DMA_RESET 0x80 >> >> +/* XXX SCSI and ethernet should have different read-only bit masks */ >> +#define DMA_CSR_RO_MASK 0xfe000007 > > I'm preparing (again) some generic DMA patches, it looks like I have > to make Lance and ESP DMA controllers separate. Good idea! They are too different. And also if we remember that there is a parallel port dma too... Are you splitting them just to improve the design, or are you adding some features too? A Test CSR register in the Lance would be great: it would allow network boot with OBP (which is the default when it's used with qemu) and hence automated Solaris boot tests. > Your patch highlights > yet another problem with the current shared design. This part of the > patch is fine. > >> + >> typedef struct DMAState DMAState; >> >> struct DMAState { >> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) >> switch (saddr) { >> case 0: >> if (val & DMA_INTREN) { >> - if (val & DMA_INTR) { >> + if (s->dmaregs[0] & DMA_INTR) { > > Doesn't this change the way irqs are raised so that a pending irq is > only generated on the write access _after_ the access that enables the > irq. Currently we check for pending irqs immediately when the irq is > enabled. No, we still check for _pending_ irqs immediately, but don't allow making a spurious interrupt by writing 1 to the DMA_INTR bit. And frankly speaking I don't think timing can be a problem here: the real hardware would have some latency too. Unless you have a test case which I broke... >> DPRINTF("Raise IRQ\n"); >> qemu_irq_raise(s->irq); >> } >> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) >> val &= ~DMA_DRAIN_FIFO; >> } else if (val == 0) >> val = DMA_DRAIN_FIFO; >> - val &= 0x0fffffff; >> + val &= ~DMA_CSR_RO_MASK; >> val |= DMA_VER; >> + s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val; >> break; >> case 1: >> s->dmaregs[0] |= DMA_LOADED; >> - break; > > A comment about fall through should be added. ok. >> default: >> + s->dmaregs[saddr] = val; >> break; >> } >> - s->dmaregs[saddr] = val; >> } >> >> static CPUReadMemoryFunc * const dma_mem_read[3] = { >> -- >> 1.6.2.5 >> >> >
On Sat, May 22, 2010 at 8:32 AM, Artyom Tarasenko <atar4qemu@googlemail.com> wrote: > 2010/5/22 Blue Swirl <blauwirbel@gmail.com>: >> On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko >> <atar4qemu@googlemail.com> wrote: >>> On a real hardware changing read-only bits has no effect >>> Use a mask common for SCSI and Ethernet registers. The crucial >>> bit is DMA_INTR, because setting or clearing it may produce >>> spurious interrupts. >>> >>> This patch allows booting Solaris 2.3 >> >> Great! >> >>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>> --- >>> hw/sparc32_dma.c | 11 +++++++---- >>> 1 files changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c >>> index 3ceb851..d54e165 100644 >>> --- a/hw/sparc32_dma.c >>> +++ b/hw/sparc32_dma.c >>> @@ -62,6 +62,9 @@ >>> #define DMA_DRAIN_FIFO 0x40 >>> #define DMA_RESET 0x80 >>> >>> +/* XXX SCSI and ethernet should have different read-only bit masks */ >>> +#define DMA_CSR_RO_MASK 0xfe000007 >> >> I'm preparing (again) some generic DMA patches, it looks like I have >> to make Lance and ESP DMA controllers separate. > > Good idea! They are too different. And also if we remember that there > is a parallel port dma too... Also cs4231. > Are you splitting them just to improve the design, or are you adding > some features too? No new features, it's just needed by the overall design. > A Test CSR register in the Lance would be great: it would allow > network boot with OBP (which is the default when it's used with qemu) > and hence automated Solaris boot tests. > >> Your patch highlights >> yet another problem with the current shared design. This part of the >> patch is fine. >> >>> + >>> typedef struct DMAState DMAState; >>> >>> struct DMAState { >>> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) >>> switch (saddr) { >>> case 0: >>> if (val & DMA_INTREN) { >>> - if (val & DMA_INTR) { >>> + if (s->dmaregs[0] & DMA_INTR) { >> >> Doesn't this change the way irqs are raised so that a pending irq is >> only generated on the write access _after_ the access that enables the >> irq. Currently we check for pending irqs immediately when the irq is >> enabled. > > No, we still check for _pending_ irqs immediately, but don't allow > making a spurious interrupt by writing 1 to the DMA_INTR bit. > > And frankly speaking I don't think timing can be a problem here: the > real hardware would have some latency too. > > Unless you have a test case which I broke... > >>> DPRINTF("Raise IRQ\n"); >>> qemu_irq_raise(s->irq); >>> } >>> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) >>> val &= ~DMA_DRAIN_FIFO; >>> } else if (val == 0) >>> val = DMA_DRAIN_FIFO; >>> - val &= 0x0fffffff; >>> + val &= ~DMA_CSR_RO_MASK; >>> val |= DMA_VER; >>> + s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val; >>> break; >>> case 1: >>> s->dmaregs[0] |= DMA_LOADED; >>> - break; >> >> A comment about fall through should be added. > > ok. > >>> default: >>> + s->dmaregs[saddr] = val; >>> break; >>> } >>> - s->dmaregs[saddr] = val; >>> } >>> >>> static CPUReadMemoryFunc * const dma_mem_read[3] = { >>> -- >>> 1.6.2.5 >>> >>> >> > > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ >
Patch
diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c index 3ceb851..d54e165 100644 --- a/hw/sparc32_dma.c +++ b/hw/sparc32_dma.c @@ -62,6 +62,9 @@ #define DMA_DRAIN_FIFO 0x40 #define DMA_RESET 0x80 +/* XXX SCSI and ethernet should have different read-only bit masks */ +#define DMA_CSR_RO_MASK 0xfe000007 + typedef struct DMAState DMAState; struct DMAState { @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) switch (saddr) { case 0: if (val & DMA_INTREN) { - if (val & DMA_INTR) { + if (s->dmaregs[0] & DMA_INTR) { DPRINTF("Raise IRQ\n"); qemu_irq_raise(s->irq); } @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) val &= ~DMA_DRAIN_FIFO; } else if (val == 0) val = DMA_DRAIN_FIFO; - val &= 0x0fffffff; + val &= ~DMA_CSR_RO_MASK; val |= DMA_VER; + s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val; break; case 1: s->dmaregs[0] |= DMA_LOADED; - break; default: + s->dmaregs[saddr] = val; break; } - s->dmaregs[saddr] = val; } static CPUReadMemoryFunc * const dma_mem_read[3] = {
On a real hardware changing read-only bits has no effect Use a mask common for SCSI and Ethernet registers. The crucial bit is DMA_INTR, because setting or clearing it may produce spurious interrupts. This patch allows booting Solaris 2.3 Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> --- hw/sparc32_dma.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)