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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello,

Please find the comment inline.

Thanks & Regards
Sharan

On 6/28/19 9:09 AM, Jia He wrote:
> From: Jianyong Wu <jianyong.wu@xxxxxxx>
>
> 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 platform irq number.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   plat/common/include/gic_fdt.h | 48 ++++++++++++++++++++++++++++++++++
>   plat/drivers/gic/gic-v2.c     | 49 +++++++++++++++++++++++++++++++++--
plat/common - Does not seem the right place for this header file. I had 
made a comment previously to put header there but with the current 
structures of the platform library I feel it is better to move it into 
the gic driver or the ofw. I prefer to have it within the ofw driver. If 
you agree we could move both the implementation and the header files 
into the ofw driver.
What do you think about it?


>   2 files changed, 95 insertions(+), 2 deletions(-)
>   create mode 100644 plat/common/include/gic_fdt.h
>
> diff --git a/plat/common/include/gic_fdt.h b/plat/common/include/gic_fdt.h
> new file mode 100644
> index 0000000..b90f564
> --- /dev/null
> +++ b/plat/common/include/gic_fdt.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Wei Chen <wei.chen@xxxxxxx>
> + *
> + * Copyright (c) 2018, Arm Ltd., All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#ifndef __PLAT_CMN_GIC_FDT_H__
> +#define __PLAT_CMN_GIC_FDT_H__
> +
> +/**
> + * Get an interrupt number of given index from device tree
> + * @param fdt Device tree blob
> + * @param nodeoffset device node offset
> + * @param index which interrupt
Missing description of the out_type parameter.
> + * @return 0 on success, a negative errno value on errors
> + */
> +uint32_t gic_get_irq_from_dtb(const void *fdt, int nodeoffset, int index,
> +                               uint32_t *out_type);
> +
> +#endif /* __PLAT_CMN_GIC_FDT_H__ */
> diff --git a/plat/drivers/gic/gic-v2.c b/plat/drivers/gic/gic-v2.c
> index 9321155..9fe9e84 100644
> --- a/plat/drivers/gic/gic-v2.c
> +++ b/plat/drivers/gic/gic-v2.c
> @@ -49,10 +49,12 @@
>   /* 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
>   
>   /* Max support interrupt number for GICv2 */
> @@ -63,6 +65,7 @@ static uint64_t gic_dist_size, gic_cpuif_size;
>   
>   #define GIC_DIST_REG(r)	((void *)(gic_dist_addr + (r)))
>   #define GIC_CPU_REG(r)        ((void *)(gic_cpuif_addr + (r)))
> +#define IRQ_TYPE_MASK  0x0000000f
>   
>   static const char * const gic_device_list[] = {
>         "arm,cortex-a15-gic",
> @@ -288,6 +291,29 @@ void gic_set_irq_type(uint32_t irq, int trigger)
>         write_gicd32(GICD_ICFGR(irq), val);
>   }
>   
> +static uint32_t gic_irq_translate(uint32_t type, uint32_t hw_irq)
> +{
> +       uint32_t irq;
> +
> +       switch (type) {
> +       case GIC_SPI_TYPE:
> +               irq = hw_irq + GIC_SPI_BASE;
> +               if (irq >= GIC_SPI_BASE && irq < __MAX_IRQ)
> +                       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;
> +       default:
> +               uk_pr_warn("Invalid IRQ type [%d]\n", type);
> +       }
> +
> +       uk_pr_err("irq is out of range\n");
> +       return -EINVAL;
> +}
> +
>   static void gic_init_dist(void)
>   {
>         uint32_t val, cpuif_number, irq_number;
> @@ -407,3 +433,22 @@ int _dtb_init_gic(const void *fdt)
>   
>   	return 0;
>   }
> +
> +uint32_t gic_get_irq_from_dtb(const void *fdt, int nodeoffset, int index,
> +                               uint32_t *out_type)
> +{
> +       fdt32_t *prop;
> +       uint32_t type, hwirq;
> +       int ret, size;
> +
> +       ret = fdt_get_interrupt(fdt, nodeoffset, index, &size, &prop);
> +       if (ret < 0)
> +               return -EINVAL;
Why are we returning -EINVAL here. Isn't it wise to forward the error 
from the fdt_get_interrupt instead.
> +
> +       type = fdt32_to_cpu(prop[0]);
> +       hwirq = fdt32_to_cpu(prop[1]);
> +
> +       *out_type = fdt32_to_cpu(prop[2]) & IRQ_TYPE_MASK;
> +
> +       return gic_irq_translate(type, hwirq);
> +}
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel