Re: [PATCH v4 2/3] hw/intc/loongarch_pch_pic: add irq number property

[Philippe Mathieu-Daudé]

On 30/12/22 10:59, Tianrui Zhao wrote:
> With loongarch 7A1000 manual, irq number supported can be set
> in PCH_PIC_INT_ID_HI register. This patch adds irq number property
> for loongarch_pch_pic, so that virt machine can set different
> irq number when pch_pic intc is added.
>
> Signed-off-by: Tianrui Zhao <zhaotianrui@loongson.cn>
> ---
>   hw/intc/loongarch_pch_pic.c         | 33 +++++++++++++++++++++++++----
>   hw/loongarch/virt.c                 |  8 ++++---
>   include/hw/intc/loongarch_pch_pic.h |  5 ++---
>   3 files changed, 36 insertions(+), 10 deletions(-)


> @@ -78,7 +80,12 @@ static uint64_t loongarch_pch_pic_low_readw(void *opaque, 
> hwaddr addr,
>           val = PCH_PIC_INT_ID_VAL;
>           break;
>       case PCH_PIC_INT_ID_HI:
> -        val = PCH_PIC_INT_ID_NUM;
> +        /*
> +         * With 7A1000 manual
> +         *   bit  0-15 pch irqchip version
> +         *   bit 16-31 irq number supported with pch irqchip
> +         */
> +        val = PCH_PIC_INT_ID_VER + ((s->irq_num - 1) << 16);

We usually use the '|' operator instead of '+', but you can also
use deposit32() from "qemu/bitops.h":

          val = deposit32(PCH_PIC_INT_ID_VER, 16, 15, s->irq_num - 1);

>           break;
>       case PCH_PIC_INT_MASK_LO:
>           val = (uint32_t)s->int_mask;
> @@ -365,6 +372,19 @@ static void loongarch_pch_pic_reset(DeviceState *d)
>       s->int_polarity = 0x0;
>   }


> diff --git a/include/hw/intc/loongarch_pch_pic.h 
> b/include/hw/intc/loongarch_pch_pic.h
> index 2d4aa9ed6f..ba3a47fa88 100644
> --- a/include/hw/intc/loongarch_pch_pic.h
> +++ b/include/hw/intc/loongarch_pch_pic.h
> @@ -9,11 +9,9 @@
>   #define PCH_PIC_NAME(name) TYPE_LOONGARCH_PCH_PIC#name
>   OBJECT_DECLARE_SIMPLE_TYPE(LoongArchPCHPIC, LOONGARCH_PCH_PIC)
>   
> -#define PCH_PIC_IRQ_START               0
> -#define PCH_PIC_IRQ_END                 63
>   #define PCH_PIC_IRQ_NUM                 64
>   #define PCH_PIC_INT_ID_VAL              0x7000000UL
> -#define PCH_PIC_INT_ID_NUM              0x3f0001UL
> +#define PCH_PIC_INT_ID_VER              0x1UL
>   
>   #define PCH_PIC_INT_ID_LO               0x00
>   #define PCH_PIC_INT_ID_HI               0x04
> @@ -66,4 +64,5 @@ struct LoongArchPCHPIC {
>       MemoryRegion iomem32_low;
>       MemoryRegion iomem32_high;
>       MemoryRegion iomem8;
> +    unsigned int  irq_num;

There is one extra space here, otherwise LGTM.

>   };