Re: [Minios-devel] [UNIKRAFT PATCH 6/7] plat/common: Add a platform API to get IRQ from device tree

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello,

This patch has checkpatch error/warning. The rest of the comments are 
inline.
Thanks & Regards
Sharan


On 12/13/18 10:19 AM, Wei Chen wrote:
> When we get irq number from device tree, it contains more than
> one items, like irq type, hardware irq number. This function will
> help us to translate these items into one unique platfomr irq number.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> ---
>   plat/common/arm/gic-v2.c  | 52 +++++++++++++++++++++++++++++++++++++--
>   plat/common/include/irq.h |  9 +++++++
>   2 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/plat/common/arm/gic-v2.c b/plat/common/arm/gic-v2.c
> index 26f4a7f..87e8168 100644
> --- a/plat/common/arm/gic-v2.c
> +++ b/plat/common/arm/gic-v2.c
> @@ -46,12 +46,18 @@
>   /* Max CPU interface for GICv2 */
>   #define GIC_MAX_CPUIF         8
>   
> -/* SPI interrupt base ID */
> +/* SPI interrupt definitions */
> +#define GIC_SPI_TYPE           0
>   #define GIC_SPI_BASE          32
>   
> -/* PPI interrupt base ID */
> +/* PPI interrupt definitions */
> +#define GIC_PPI_TYPE           1
>   #define GIC_PPI_BASE          16
>   
> +/* SGI interrupt definitions */
> +#define GIC_SGI_TYPE           2
> +#define GIC_SGI_BASE           0
> +
>   /* Max support interrupt number for GICv2 */
>   #define GIC_MAX_IRQ           __MAX_IRQ
>   
> @@ -292,6 +298,33 @@ void gic_set_irq_type(uint32_t irq, int trigger, int polarity)
>         write_gicd32(GICD_ICFGR(irq), val);
>   }
>   
Why do we want to expose the function to the outside? Shouldn't we add 
static
> +int gic_irq_translate(int type, int hw_irq)
> +{
> +       int irq;
> +
> +        switch (type) {
> +       case GIC_SPI_TYPE:
> +               irq = hw_irq + GIC_SPI_BASE;
Shouldn't it be (irq < __MAX_IRQ) or the max number of interrupt that 
were supported on the device?
> +               if (irq >= GIC_SPI_BASE && irq < GIC_SPI_BASE)
> +                       return irq;
> +               break;
> +       case GIC_PPI_TYPE:
> +               irq = hw_irq + GIC_PPI_BASE;
> +               if (irq >= GIC_PPI_BASE && irq < GIC_SPI_BASE)
> +                       return irq;
> +               break;
> +       case GIC_SGI_TYPE:
> +               irq = hw_irq + GIC_SGI_BASE;
> +               if (irq >= GIC_SGI_BASE && irq < GIC_PPI_BASE)
> +                       return irq;
> +               break;
> +       default:
> +               uk_pr_warn("Invalid IRQ type [%d]\n", type);
> +        }
> +
Maybe wise to add

uk_pr_err("Out of range\n");
> +        return -EINVAL;
> +}
> +
>   static void gic_init_dist(void)
>   {
>         uint32_t val, cpuif_number, irq_number;
> @@ -413,3 +446,18 @@ int _dtb_init_gic(const void *fdt)
>   
>   	return 0;
>   }
> +
I would avoid using the ukplat_* as it gic specific function.
> +int ukplat_get_irq_from_dtb(const void *fdt, int nodeoffset, int index)
> +{
> +       const fdt32_t *prop;
> +       int type, hwirq, size;
> +
> +       prop = fdt_get_interrupt(fdt, nodeoffset, index, &size);
> +       if (!prop)
> +               return -EINVAL;
> +
> +       type = fdt32_to_cpu(prop[0]);
> +       hwirq = fdt32_to_cpu(prop[1]);
> +
> +       return gic_irq_translate(type , hwirq);
> +}
> diff --git a/plat/common/include/irq.h b/plat/common/include/irq.h
> index fac5022..fc268b7 100644
> --- a/plat/common/include/irq.h
> +++ b/plat/common/include/irq.h
> @@ -61,4 +61,13 @@ enum uk_irq_polarity {
>         UK_IRQ_POLARITY_MAX
>   };
>   
I prefer adding it to gic_fdt.h or a suitable name  in 
plat/common/include because this device tree parsing for gic.
> +/**
> + * Get an interrupt number of given index from device tree
> + * @param fdt Device tree blob
> + * @param nodeoffset device node offset
> + * @param index which interrupt
> + * @return 0 on success, a negative errno value on errors
> + */
> +int ukplat_get_irq_from_dtb(const void *fdt, int nodeoffset, int index);
> +
>   #endif /* __PLAT_CMN_IRQ_H__ */
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel