| Message ID | 3f09b2dd109d19851d786047ad5c2ff459c90cd7.1678188711.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Pegasos2 fixes and audio output support | expand |
On 3/7/23 08:42, BALATON Zoltan wrote: > From: David Woodhouse <dwmw2@infradead.org> > > Back in the mists of time, before EISA came along and required per-pin > level control in the ELCR register, the i8259 had a single chip-wide > level-mode control in bit 3 of ICW1. > > Even in the PIIX3 datasheet from 1996 this is documented as 'This bit is > disabled', but apparently MorphOS is using it in the version of the > i8259 which is in the Pegasos2 board as part of the VT8231 chipset. > > It's easy enough to implement, and I think it's harmless enough to do so > unconditionally. > > Signed-off-by: David Woodhouse <dwmw2@infradead.org> > [balaton: updated commit message as asked by author] > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/intc/i8259.c | 10 ++++------ > hw/intc/i8259_common.c | 24 +++++++++++++++++++++++- > include/hw/isa/i8259_internal.h | 1 + > 3 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c > index 17910f3bcb..bbae2d87f4 100644 > --- a/hw/intc/i8259.c > +++ b/hw/intc/i8259.c > @@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level) > } > #endif > > - if (s->elcr & mask) { > + if (s->ltim || (s->elcr & mask)) { > /* level triggered */ > if (level) { > s->irr |= mask; > @@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq) > s->isr |= (1 << irq); > } > /* We don't clear a level sensitive interrupt here */ > - if (!(s->elcr & (1 << irq))) { > + if (!s->ltim && !(s->elcr & (1 << irq))) { > s->irr &= ~(1 << irq); > } > pic_update_irq(s); > @@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev) > PICCommonState *s = PIC_COMMON(dev); > > s->elcr = 0; > + s->ltim = 0; > pic_init_reset(s); > } > > @@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64, > s->init_state = 1; > s->init4 = val & 1; > s->single_mode = val & 2; > - if (val & 0x08) { > - qemu_log_mask(LOG_UNIMP, > - "i8259: level sensitive irq not supported\n"); > - } > + s->ltim = val & 8; > } else if (val & 0x08) { > if (val & 0x04) { > s->poll = 1; > diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c > index af2e4a2241..c931dc2d07 100644 > --- a/hw/intc/i8259_common.c > +++ b/hw/intc/i8259_common.c > @@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s) > s->special_fully_nested_mode = 0; > s->init4 = 0; > s->single_mode = 0; > - /* Note: ELCR is not reset */ > + /* Note: ELCR and LTIM are not reset */ > } > > static int pic_dispatch_pre_save(void *opaque) > @@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon) > s->special_fully_nested_mode); > } > > +static bool ltim_state_needed(void *opaque) > +{ > + PICCommonState *s = PIC_COMMON(opaque); > + > + return !!s->ltim; > +} > + > +static const VMStateDescription vmstate_pic_ltim = { > + .name = "i8259/ltim", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = ltim_state_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(ltim, PICCommonState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_pic_common = { > .name = "i8259", > .version_id = 1, > @@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = { > VMSTATE_UINT8(single_mode, PICCommonState), > VMSTATE_UINT8(elcr, PICCommonState), > VMSTATE_END_OF_LIST() > + }, > + .subsections = (const VMStateDescription*[]) { Checkpatch will nag about it claiming that we need spaces between '*'. The maintainer can fix it in-tree though. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > + &vmstate_pic_ltim, > + NULL > } > }; > > diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h > index 155b098452..f9dcc4163e 100644 > --- a/include/hw/isa/i8259_internal.h > +++ b/include/hw/isa/i8259_internal.h > @@ -61,6 +61,7 @@ struct PICCommonState { > uint8_t single_mode; /* true if slave pic is not initialized */ > uint8_t elcr; /* PIIX edge/trigger selection*/ > uint8_t elcr_mask; > + uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */ > qemu_irq int_out[1]; > uint32_t master; /* reflects /SP input pin */ > uint32_t iobase;
On Tue, 7 Mar 2023 at 13:36, Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > On 3/7/23 08:42, BALATON Zoltan wrote: > > @@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = { > > VMSTATE_UINT8(single_mode, PICCommonState), > > VMSTATE_UINT8(elcr, PICCommonState), > > VMSTATE_END_OF_LIST() > > + }, > > + .subsections = (const VMStateDescription*[]) { > > Checkpatch will nag about it claiming that we need spaces between '*'. The maintainer > can fix it in-tree though. checkpatch nags because it fails to correctly parse this piece of C syntax and thinks the '*' is a multiplication operator; we should just ignore it in this instance. thanks -- PMM
On Tue, 2023-03-07 at 10:36 -0300, Daniel Henrique Barboza wrote: > > Checkpatch will nag about it claiming that we need spaces between > '*'. The maintainer can fix it in-tree though. I saw that and explicitly didn't care. 3/5 of the existing examples within the tree look like that one — which is how the docs tell us to do it — so I figured checkpatch could just be sad. > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Thanks.
On Tue, 7 Mar 2023, Daniel Henrique Barboza wrote: > On 3/7/23 08:42, BALATON Zoltan wrote: >> From: David Woodhouse <dwmw2@infradead.org> >> >> Back in the mists of time, before EISA came along and required per-pin >> level control in the ELCR register, the i8259 had a single chip-wide >> level-mode control in bit 3 of ICW1. >> >> Even in the PIIX3 datasheet from 1996 this is documented as 'This bit is >> disabled', but apparently MorphOS is using it in the version of the >> i8259 which is in the Pegasos2 board as part of the VT8231 chipset. >> >> It's easy enough to implement, and I think it's harmless enough to do so >> unconditionally. >> >> Signed-off-by: David Woodhouse <dwmw2@infradead.org> >> [balaton: updated commit message as asked by author] >> Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/intc/i8259.c | 10 ++++------ >> hw/intc/i8259_common.c | 24 +++++++++++++++++++++++- >> include/hw/isa/i8259_internal.h | 1 + >> 3 files changed, 28 insertions(+), 7 deletions(-) >> >> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c >> index 17910f3bcb..bbae2d87f4 100644 >> --- a/hw/intc/i8259.c >> +++ b/hw/intc/i8259.c >> @@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int >> level) >> } >> #endif >> - if (s->elcr & mask) { >> + if (s->ltim || (s->elcr & mask)) { >> /* level triggered */ >> if (level) { >> s->irr |= mask; >> @@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq) >> s->isr |= (1 << irq); >> } >> /* We don't clear a level sensitive interrupt here */ >> - if (!(s->elcr & (1 << irq))) { >> + if (!s->ltim && !(s->elcr & (1 << irq))) { >> s->irr &= ~(1 << irq); >> } >> pic_update_irq(s); >> @@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev) >> PICCommonState *s = PIC_COMMON(dev); >> s->elcr = 0; >> + s->ltim = 0; >> pic_init_reset(s); >> } >> @@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr >> addr64, >> s->init_state = 1; >> s->init4 = val & 1; >> s->single_mode = val & 2; >> - if (val & 0x08) { >> - qemu_log_mask(LOG_UNIMP, >> - "i8259: level sensitive irq not >> supported\n"); >> - } >> + s->ltim = val & 8; >> } else if (val & 0x08) { >> if (val & 0x04) { >> s->poll = 1; >> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c >> index af2e4a2241..c931dc2d07 100644 >> --- a/hw/intc/i8259_common.c >> +++ b/hw/intc/i8259_common.c >> @@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s) >> s->special_fully_nested_mode = 0; >> s->init4 = 0; >> s->single_mode = 0; >> - /* Note: ELCR is not reset */ >> + /* Note: ELCR and LTIM are not reset */ >> } >> static int pic_dispatch_pre_save(void *opaque) >> @@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider >> *obj, Monitor *mon) >> s->special_fully_nested_mode); >> } >> +static bool ltim_state_needed(void *opaque) >> +{ >> + PICCommonState *s = PIC_COMMON(opaque); >> + >> + return !!s->ltim; >> +} >> + >> +static const VMStateDescription vmstate_pic_ltim = { >> + .name = "i8259/ltim", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .needed = ltim_state_needed, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT8(ltim, PICCommonState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static const VMStateDescription vmstate_pic_common = { >> .name = "i8259", >> .version_id = 1, >> @@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = { >> VMSTATE_UINT8(single_mode, PICCommonState), >> VMSTATE_UINT8(elcr, PICCommonState), >> VMSTATE_END_OF_LIST() >> + }, >> + .subsections = (const VMStateDescription*[]) { > > Checkpatch will nag about it claiming that we need spaces between '*'. The > maintainer > can fix it in-tree though. I had that before in another patch but was told this is a checkpatch false positive I can disregard. Regards, BALATON Zoltan > > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > >> + &vmstate_pic_ltim, >> + NULL >> } >> }; >> diff --git a/include/hw/isa/i8259_internal.h >> b/include/hw/isa/i8259_internal.h >> index 155b098452..f9dcc4163e 100644 >> --- a/include/hw/isa/i8259_internal.h >> +++ b/include/hw/isa/i8259_internal.h >> @@ -61,6 +61,7 @@ struct PICCommonState { >> uint8_t single_mode; /* true if slave pic is not initialized */ >> uint8_t elcr; /* PIIX edge/trigger selection*/ >> uint8_t elcr_mask; >> + uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */ >> qemu_irq int_out[1]; >> uint32_t master; /* reflects /SP input pin */ >> uint32_t iobase; > >
On 3/7/23 10:39, David Woodhouse wrote: > On Tue, 2023-03-07 at 10:36 -0300, Daniel Henrique Barboza wrote: >> >> Checkpatch will nag about it claiming that we need spaces between >> '*'. The maintainer can fix it in-tree though. > > I saw that and explicitly didn't care. 3/5 of the existing examples > within the tree look like that one — which is how the docs tell us to > do it — so I figured checkpatch could just be sad. Regardless of how much we care about checkpatch happiness my r-b stands. Peter said we can ignore it in this case, so yeah, checkpatch will probably take a L. Thanks, Daniel > >> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > Thanks.
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c index 17910f3bcb..bbae2d87f4 100644 --- a/hw/intc/i8259.c +++ b/hw/intc/i8259.c @@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level) } #endif - if (s->elcr & mask) { + if (s->ltim || (s->elcr & mask)) { /* level triggered */ if (level) { s->irr |= mask; @@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq) s->isr |= (1 << irq); } /* We don't clear a level sensitive interrupt here */ - if (!(s->elcr & (1 << irq))) { + if (!s->ltim && !(s->elcr & (1 << irq))) { s->irr &= ~(1 << irq); } pic_update_irq(s); @@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev) PICCommonState *s = PIC_COMMON(dev); s->elcr = 0; + s->ltim = 0; pic_init_reset(s); } @@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64, s->init_state = 1; s->init4 = val & 1; s->single_mode = val & 2; - if (val & 0x08) { - qemu_log_mask(LOG_UNIMP, - "i8259: level sensitive irq not supported\n"); - } + s->ltim = val & 8; } else if (val & 0x08) { if (val & 0x04) { s->poll = 1; diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c index af2e4a2241..c931dc2d07 100644 --- a/hw/intc/i8259_common.c +++ b/hw/intc/i8259_common.c @@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s) s->special_fully_nested_mode = 0; s->init4 = 0; s->single_mode = 0; - /* Note: ELCR is not reset */ + /* Note: ELCR and LTIM are not reset */ } static int pic_dispatch_pre_save(void *opaque) @@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon) s->special_fully_nested_mode); } +static bool ltim_state_needed(void *opaque) +{ + PICCommonState *s = PIC_COMMON(opaque); + + return !!s->ltim; +} + +static const VMStateDescription vmstate_pic_ltim = { + .name = "i8259/ltim", + .version_id = 1, + .minimum_version_id = 1, + .needed = ltim_state_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT8(ltim, PICCommonState), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_pic_common = { .name = "i8259", .version_id = 1, @@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = { VMSTATE_UINT8(single_mode, PICCommonState), VMSTATE_UINT8(elcr, PICCommonState), VMSTATE_END_OF_LIST() + }, + .subsections = (const VMStateDescription*[]) { + &vmstate_pic_ltim, + NULL } }; diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h index 155b098452..f9dcc4163e 100644 --- a/include/hw/isa/i8259_internal.h +++ b/include/hw/isa/i8259_internal.h @@ -61,6 +61,7 @@ struct PICCommonState { uint8_t single_mode; /* true if slave pic is not initialized */ uint8_t elcr; /* PIIX edge/trigger selection*/ uint8_t elcr_mask; + uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */ qemu_irq int_out[1]; uint32_t master; /* reflects /SP input pin */ uint32_t iobase;