| Message ID | 20211013212132.31519-4-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | q800: GLUE updates for A/UX mode | expand |
Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit : > In order to allow dynamic routing of IRQs to different IRQ levels on the CPU > depending upon port B bit 6, use GLUE IRQ numbers and map them to the the > corresponding CPU IRQ level accordingly. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++---- > 1 file changed, 28 insertions(+), 4 deletions(-) > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c > index 15f3067811..81c335bf16 100644 > --- a/hw/m68k/q800.c > +++ b/hw/m68k/q800.c > @@ -102,11 +102,34 @@ struct GLUEState { > uint8_t ipr; > }; > > +#define GLUE_IRQ_IN_VIA1 0 > +#define GLUE_IRQ_IN_VIA2 1 > +#define GLUE_IRQ_IN_SONIC 2 > +#define GLUE_IRQ_IN_ESCC 3 > + > static void GLUE_set_irq(void *opaque, int irq, int level) > { > GLUEState *s = opaque; > int i; > > + switch (irq) { > + case GLUE_IRQ_IN_VIA1: > + irq = 5; > + break; Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is mapped to irq 5 (before patch 2 it would be to 0). > + > + case GLUE_IRQ_IN_VIA2: > + irq = 1; > + break; > + > + case GLUE_IRQ_IN_SONIC: > + irq = 2; > + break; > + > + case GLUE_IRQ_IN_ESCC: > + irq = 3; > + break; > + } > + > if (level) { > s->ipr |= 1 << irq; perhaps you can rename here "irq" to "shift"? Thanks, Laurent
On Fri, 15 Oct 2021, Laurent Vivier wrote: > Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit : >> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU >> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the >> corresponding CPU IRQ level accordingly. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c >> index 15f3067811..81c335bf16 100644 >> --- a/hw/m68k/q800.c >> +++ b/hw/m68k/q800.c >> @@ -102,11 +102,34 @@ struct GLUEState { >> uint8_t ipr; >> }; >> >> +#define GLUE_IRQ_IN_VIA1 0 >> +#define GLUE_IRQ_IN_VIA2 1 >> +#define GLUE_IRQ_IN_SONIC 2 >> +#define GLUE_IRQ_IN_ESCC 3 >> + >> static void GLUE_set_irq(void *opaque, int irq, int level) >> { >> GLUEState *s = opaque; >> int i; >> >> + switch (irq) { >> + case GLUE_IRQ_IN_VIA1: >> + irq = 5; >> + break; > > Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is > mapped to irq 5 (before patch 2 it would be to 0). > >> + >> + case GLUE_IRQ_IN_VIA2: >> + irq = 1; >> + break; >> + >> + case GLUE_IRQ_IN_SONIC: >> + irq = 2; >> + break; >> + >> + case GLUE_IRQ_IN_ESCC: >> + irq = 3; >> + break; >> + } >> + >> if (level) { >> s->ipr |= 1 << irq; > > perhaps you can rename here "irq" to "shift"? I think if it's the irq number calling it irq is clearer than shift. Maybe use BIT(irq) instead? Regards, BALATON Zoltan
On 15/10/2021 07:31, Laurent Vivier wrote: > Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit : >> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU >> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the >> corresponding CPU IRQ level accordingly. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c >> index 15f3067811..81c335bf16 100644 >> --- a/hw/m68k/q800.c >> +++ b/hw/m68k/q800.c >> @@ -102,11 +102,34 @@ struct GLUEState { >> uint8_t ipr; >> }; >> >> +#define GLUE_IRQ_IN_VIA1 0 >> +#define GLUE_IRQ_IN_VIA2 1 >> +#define GLUE_IRQ_IN_SONIC 2 >> +#define GLUE_IRQ_IN_ESCC 3 >> + >> static void GLUE_set_irq(void *opaque, int irq, int level) >> { >> GLUEState *s = opaque; >> int i; >> >> + switch (irq) { >> + case GLUE_IRQ_IN_VIA1: >> + irq = 5; >> + break; > > Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is > mapped to irq 5 (before patch 2 it would be to 0). I think it should stay in the existing order because patch 2 is really a bug fix: all of the other IRQs are statically wired in A/UX mode except for VIA1. Once this is fixed, this patch then abstracts the *input* IRQs away from the CPU level so they can be swizzled independently depending upon whether A/UX mode is selected. >> + >> + case GLUE_IRQ_IN_VIA2: >> + irq = 1; >> + break; >> + >> + case GLUE_IRQ_IN_SONIC: >> + irq = 2; >> + break; >> + >> + case GLUE_IRQ_IN_ESCC: >> + irq = 3; >> + break; >> + } >> + >> if (level) { >> s->ipr |= 1 << irq; > > perhaps you can rename here "irq" to "shift"? At this point it is the CPU level IRQ so "irq" seems correct here. Are you thinking that using a different intermediate variable from the function parameter would help? ATB, Mark.
On 15/10/2021 07:31, Laurent Vivier wrote: > Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit : >> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU >> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the >> corresponding CPU IRQ level accordingly. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c >> index 15f3067811..81c335bf16 100644 >> --- a/hw/m68k/q800.c >> +++ b/hw/m68k/q800.c >> @@ -102,11 +102,34 @@ struct GLUEState { >> uint8_t ipr; >> }; >> >> +#define GLUE_IRQ_IN_VIA1 0 >> +#define GLUE_IRQ_IN_VIA2 1 >> +#define GLUE_IRQ_IN_SONIC 2 >> +#define GLUE_IRQ_IN_ESCC 3 >> + >> static void GLUE_set_irq(void *opaque, int irq, int level) >> { >> GLUEState *s = opaque; >> int i; >> >> + switch (irq) { >> + case GLUE_IRQ_IN_VIA1: >> + irq = 5; >> + break; > > Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is > mapped to irq 5 (before patch 2 it would be to 0). > >> + >> + case GLUE_IRQ_IN_VIA2: >> + irq = 1; >> + break; >> + >> + case GLUE_IRQ_IN_SONIC: >> + irq = 2; >> + break; >> + >> + case GLUE_IRQ_IN_ESCC: >> + irq = 3; >> + break; >> + } >> + >> if (level) { >> s->ipr |= 1 << irq; > > perhaps you can rename here "irq" to "shift"? Were you happy to leave this as irq? Another alternative may be to use the BIT() macro as suggested by Zoltan. ATB, Mark.
Le 17/10/2021 à 11:40, Mark Cave-Ayland a écrit : > On 15/10/2021 07:31, Laurent Vivier wrote: > >> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit : >>> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU >>> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the >>> corresponding CPU IRQ level accordingly. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++---- >>> 1 file changed, 28 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c >>> index 15f3067811..81c335bf16 100644 >>> --- a/hw/m68k/q800.c >>> +++ b/hw/m68k/q800.c >>> @@ -102,11 +102,34 @@ struct GLUEState { >>> uint8_t ipr; >>> }; >>> +#define GLUE_IRQ_IN_VIA1 0 >>> +#define GLUE_IRQ_IN_VIA2 1 >>> +#define GLUE_IRQ_IN_SONIC 2 >>> +#define GLUE_IRQ_IN_ESCC 3 >>> + >>> static void GLUE_set_irq(void *opaque, int irq, int level) >>> { >>> GLUEState *s = opaque; >>> int i; >>> + switch (irq) { >>> + case GLUE_IRQ_IN_VIA1: >>> + irq = 5; >>> + break; >> >> Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is >> mapped to irq 5 (before patch 2 it would be to 0). >> >>> + >>> + case GLUE_IRQ_IN_VIA2: >>> + irq = 1; >>> + break; >>> + >>> + case GLUE_IRQ_IN_SONIC: >>> + irq = 2; >>> + break; >>> + >>> + case GLUE_IRQ_IN_ESCC: >>> + irq = 3; >>> + break; >>> + } >>> + >>> if (level) { >>> s->ipr |= 1 << irq; >> >> perhaps you can rename here "irq" to "shift"? > > Were you happy to leave this as irq? Another alternative may be to use the BIT() macro as suggested > by Zoltan. I have no problem to keep this like that. Thanks, Laurent
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c index 15f3067811..81c335bf16 100644 --- a/hw/m68k/q800.c +++ b/hw/m68k/q800.c @@ -102,11 +102,34 @@ struct GLUEState { uint8_t ipr; }; +#define GLUE_IRQ_IN_VIA1 0 +#define GLUE_IRQ_IN_VIA2 1 +#define GLUE_IRQ_IN_SONIC 2 +#define GLUE_IRQ_IN_ESCC 3 + static void GLUE_set_irq(void *opaque, int irq, int level) { GLUEState *s = opaque; int i; + switch (irq) { + case GLUE_IRQ_IN_VIA1: + irq = 5; + break; + + case GLUE_IRQ_IN_VIA2: + irq = 1; + break; + + case GLUE_IRQ_IN_SONIC: + irq = 2; + break; + + case GLUE_IRQ_IN_ESCC: + irq = 3; + break; + } + if (level) { s->ipr |= 1 << irq; } else { @@ -284,7 +307,7 @@ static void q800_init(MachineState *machine) sysbus = SYS_BUS_DEVICE(via1_dev); sysbus_realize_and_unref(sysbus, &error_fatal); sysbus_mmio_map(sysbus, 1, VIA_BASE); - sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 5)); + sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA1)); adb_bus = qdev_get_child_bus(via1_dev, "adb.0"); dev = qdev_new(TYPE_ADB_KEYBOARD); @@ -297,7 +320,7 @@ static void q800_init(MachineState *machine) sysbus = SYS_BUS_DEVICE(via2_dev); sysbus_realize_and_unref(sysbus, &error_fatal); sysbus_mmio_map(sysbus, 1, VIA_BASE + VIA_SIZE); - sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 1)); + sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA2)); /* MACSONIC */ @@ -330,7 +353,7 @@ static void q800_init(MachineState *machine) sysbus = SYS_BUS_DEVICE(dev); sysbus_realize_and_unref(sysbus, &error_fatal); sysbus_mmio_map(sysbus, 0, SONIC_BASE); - sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 2)); + sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_SONIC)); memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom", SONIC_PROM_SIZE, &error_fatal); @@ -366,7 +389,8 @@ static void q800_init(MachineState *machine) qdev_realize_and_unref(escc_orgate, NULL, &error_fatal); sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0)); sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1)); - qdev_connect_gpio_out(DEVICE(escc_orgate), 0, qdev_get_gpio_in(glue, 3)); + qdev_connect_gpio_out(DEVICE(escc_orgate), 0, + qdev_get_gpio_in(glue, GLUE_IRQ_IN_ESCC)); sysbus_mmio_map(sysbus, 0, SCC_BASE); /* SCSI */
In order to allow dynamic routing of IRQs to different IRQ levels on the CPU depending upon port B bit 6, use GLUE IRQ numbers and map them to the the corresponding CPU IRQ level accordingly. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)