[v2] hw/intc/loongarch_pch: Cleanup with memory region ops

[PATCH v2 04/16] hw/intc/loongarch_pch: Set version information at initial stage

Posted by Bibo Mao 10 months, 3 weeks ago

Register PCH_PIC_INT_ID constains version and supported irq number
information, and it is read only register. The detailed value can
be set at initial stage, rather than read callback.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/intc/loongarch_pch_pic.c            | 17 ++++++++++-------
 include/hw/intc/loongarch_pic_common.h | 17 +++++++++++++++--
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
index a2d9930ac9..7043b8b9f4 100644
--- a/hw/intc/loongarch_pch_pic.c
+++ b/hw/intc/loongarch_pch_pic.c
@@ -80,15 +80,10 @@ static uint64_t loongarch_pch_pic_low_readw(void *opaque, hwaddr addr,
 
     switch (offset) {
     case PCH_PIC_INT_ID:
-        val = PCH_PIC_INT_ID_VAL;
+        val = s->id.data & UINT_MAX;
         break;
     case PCH_PIC_INT_ID + 4:
-        /*
-         * With 7A1000 manual
-         *   bit  0-15 pch irqchip version
-         *   bit 16-31 irq number supported with pch irqchip
-         */
-        val = deposit32(PCH_PIC_INT_ID_VER, 16, 16, s->irq_num - 1);
+        val = s->id.data >> 32;
         break;
     case PCH_PIC_INT_MASK:
         val = (uint32_t)s->int_mask;
@@ -361,6 +356,14 @@ static void loongarch_pch_pic_reset(DeviceState *d)
     LoongArchPICCommonState *s = LOONGARCH_PIC_COMMON(d);
     int i;
 
+    /*
+     * With 7A1000 manual
+     *   bit  0-15 pch irqchip version
+     *   bit 16-31 irq number supported with pch irqchip
+     */
+    s->id.desc.id = PCH_PIC_INT_ID_VAL;
+    s->id.desc.version = PCH_PIC_INT_ID_VER;
+    s->id.desc.irq_num = s->irq_num - 1;
     s->int_mask = -1;
     s->htmsi_en = 0x0;
     s->intedge  = 0x0;
diff --git a/include/hw/intc/loongarch_pic_common.h b/include/hw/intc/loongarch_pic_common.h
index ef6edc15bf..fb848da4b8 100644
--- a/include/hw/intc/loongarch_pic_common.h
+++ b/include/hw/intc/loongarch_pic_common.h
@@ -10,9 +10,9 @@
 #include "hw/pci-host/ls7a.h"
 #include "hw/sysbus.h"
 
-#define PCH_PIC_INT_ID_VAL              0x7000000UL
-#define PCH_PIC_INT_ID_VER              0x1UL
 #define PCH_PIC_INT_ID                  0x00
+#define  PCH_PIC_INT_ID_VAL             0x7
+#define  PCH_PIC_INT_ID_VER             0x1
 #define PCH_PIC_INT_MASK                0x20
 #define PCH_PIC_HTMSI_EN                0x40
 #define PCH_PIC_INT_EDGE                0x60
@@ -30,10 +30,23 @@
 OBJECT_DECLARE_TYPE(LoongArchPICCommonState,
                     LoongArchPICCommonClass, LOONGARCH_PIC_COMMON)
 
+union LoongArchPIC_ID {
+    struct {
+        uint64_t _reserved_0:24;
+        uint64_t id:8;
+        uint64_t version:8;
+        uint64_t _reserved_1:8;
+        uint64_t irq_num:8;
+        uint64_t _reserved_2:8;
+    } QEMU_PACKED desc;
+    uint64_t data;
+};
+
 struct LoongArchPICCommonState {
     SysBusDevice parent_obj;
 
     qemu_irq parent_irq[64];
+    union LoongArchPIC_ID id; /* 0x00  interrupt ID register */
     uint64_t int_mask;        /* 0x020 interrupt mask register */
     uint64_t htmsi_en;        /* 0x040 1=msi */
     uint64_t intedge;         /* 0x060 edge=1 level=0 */
-- 
2.39.3

Re: [PATCH v2 04/16] hw/intc/loongarch_pch: Set version information at initial stage

Posted by Philippe Mathieu-Daudé 9 months, 3 weeks ago

On 24/3/25 10:37, Bibo Mao wrote:
> Register PCH_PIC_INT_ID constains version and supported irq number
> information, and it is read only register. The detailed value can
> be set at initial stage, rather than read callback.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   hw/intc/loongarch_pch_pic.c            | 17 ++++++++++-------
>   include/hw/intc/loongarch_pic_common.h | 17 +++++++++++++++--
>   2 files changed, 25 insertions(+), 9 deletions(-)


> diff --git a/include/hw/intc/loongarch_pic_common.h b/include/hw/intc/loongarch_pic_common.h
> index ef6edc15bf..fb848da4b8 100644
> --- a/include/hw/intc/loongarch_pic_common.h
> +++ b/include/hw/intc/loongarch_pic_common.h
> @@ -10,9 +10,9 @@
>   #include "hw/pci-host/ls7a.h"
>   #include "hw/sysbus.h"
>   
> -#define PCH_PIC_INT_ID_VAL              0x7000000UL
> -#define PCH_PIC_INT_ID_VER              0x1UL
>   #define PCH_PIC_INT_ID                  0x00
> +#define  PCH_PIC_INT_ID_VAL             0x7
> +#define  PCH_PIC_INT_ID_VER             0x1
>   #define PCH_PIC_INT_MASK                0x20
>   #define PCH_PIC_HTMSI_EN                0x40
>   #define PCH_PIC_INT_EDGE                0x60
> @@ -30,10 +30,23 @@
>   OBJECT_DECLARE_TYPE(LoongArchPICCommonState,
>                       LoongArchPICCommonClass, LOONGARCH_PIC_COMMON)
>   
> +union LoongArchPIC_ID {
> +    struct {
> +        uint64_t _reserved_0:24;
> +        uint64_t id:8;

Why not use:

            uint8_t _reserved_0[3];
            uint8_t id;

Otherwise see commit ecbf3567e21 ("docs/devel/style: add a section about
bitfield, and disallow them for packed structures"), this might give
troubles on Windows or big-endian hosts such s390x.

> +        uint64_t version:8;
> +        uint64_t _reserved_1:8;
> +        uint64_t irq_num:8;
> +        uint64_t _reserved_2:8;
 > +    } QEMU_PACKED desc;> +    uint64_t data;
> +};

Re: [PATCH v2 04/16] hw/intc/loongarch_pch: Set version information at initial stage

Posted by bibo mao 9 months, 2 weeks ago


On 2025/4/25 下午5:57, Philippe Mathieu-Daudé wrote:
> On 24/3/25 10:37, Bibo Mao wrote:
>> Register PCH_PIC_INT_ID constains version and supported irq number
>> information, and it is read only register. The detailed value can
>> be set at initial stage, rather than read callback.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/intc/loongarch_pch_pic.c            | 17 ++++++++++-------
>>   include/hw/intc/loongarch_pic_common.h | 17 +++++++++++++++--
>>   2 files changed, 25 insertions(+), 9 deletions(-)
> 
> 
>> diff --git a/include/hw/intc/loongarch_pic_common.h 
>> b/include/hw/intc/loongarch_pic_common.h
>> index ef6edc15bf..fb848da4b8 100644
>> --- a/include/hw/intc/loongarch_pic_common.h
>> +++ b/include/hw/intc/loongarch_pic_common.h
>> @@ -10,9 +10,9 @@
>>   #include "hw/pci-host/ls7a.h"
>>   #include "hw/sysbus.h"
>> -#define PCH_PIC_INT_ID_VAL              0x7000000UL
>> -#define PCH_PIC_INT_ID_VER              0x1UL
>>   #define PCH_PIC_INT_ID                  0x00
>> +#define  PCH_PIC_INT_ID_VAL             0x7
>> +#define  PCH_PIC_INT_ID_VER             0x1
>>   #define PCH_PIC_INT_MASK                0x20
>>   #define PCH_PIC_HTMSI_EN                0x40
>>   #define PCH_PIC_INT_EDGE                0x60
>> @@ -30,10 +30,23 @@
>>   OBJECT_DECLARE_TYPE(LoongArchPICCommonState,
>>                       LoongArchPICCommonClass, LOONGARCH_PIC_COMMON)
>> +union LoongArchPIC_ID {
>> +    struct {
>> +        uint64_t _reserved_0:24;
>> +        uint64_t id:8;
> 
> Why not use:
> 
>             uint8_t _reserved_0[3];
>             uint8_t id;
> 
> Otherwise see commit ecbf3567e21 ("docs/devel/style: add a section about
> bitfield, and disallow them for packed structures"), this might give
> troubles on Windows or big-endian hosts such s390x.
Good suggestion, will use this method rather than bit-field method.

Regards
Bibo Mao
> 
>> +        uint64_t version:8;
>> +        uint64_t _reserved_1:8;
>> +        uint64_t irq_num:8;
>> +        uint64_t _reserved_2:8;
>  > +    } QEMU_PACKED desc;> +    uint64_t data;
>> +};