[PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

Mark Cave-Ayland posted 8 patches 4 years, 3 months ago
Maintainers: Laurent Vivier <laurent@vivier.eu>
There is a newer version of this series
[PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
Posted by Mark Cave-Ayland 4 years, 3 months ago
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;
+
+    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 */
-- 
2.20.1


Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
Posted by Laurent Vivier 4 years, 3 months ago
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

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
Posted by BALATON Zoltan 4 years, 3 months ago
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
Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
Posted by Mark Cave-Ayland 4 years, 3 months ago
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.

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
Posted by Mark Cave-Ayland 4 years, 3 months ago
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.

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
Posted by Laurent Vivier 4 years, 3 months ago
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