Re: [Minios-devel] [UNIKRAFT PATCHv7 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 7/24/19 12:05 PM, 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/drivers/gic/gic-v2.c          | 30 ++++++++++++++++--
>   plat/drivers/include/ofw/gic_fdt.h | 51 ++++++++++++++++++++++++++++++
>   plat/drivers/ofw/fdt.c             | 20 ++++++++++++
>   3 files changed, 99 insertions(+), 2 deletions(-)
>   create mode 100644 plat/drivers/include/ofw/gic_fdt.h
>
> diff --git a/plat/drivers/gic/gic-v2.c b/plat/drivers/gic/gic-v2.c
> index 41f769c..d2857f8 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)
In the previous version of the implementation gic_irq_translate was 
called from gic_get_irq_from_dtb. Now since we have moved the 
implementation into the gic driver, we need a public interface in the 
gic driver which queries the device tree and translate the interrupt line.
> +{
> +       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;
> diff --git a/plat/drivers/include/ofw/gic_fdt.h 
> b/plat/drivers/include/ofw/gic_fdt.h
> new file mode 100644
> index 0000000..a503b7a
> --- /dev/null
> +++ b/plat/drivers/include/ofw/gic_fdt.h
> @@ -0,0 +1,51 @@
> +/* 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__
s/CMN/DRV

> +#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 the index of interrupt we want to retrieve
> + * @param irq_type output the interrupt type e.g. SPI, PPI, SGI
> + * @param hwirq output the hardware irq number
> + * @param trigger_type. output to tell e.g. edge or level trigger
> + * @return 0 on success, a negative errno value on errors
> + */
> +int gic_get_irq_from_dtb(const void *fdt, int nodeoffset, int index,
> +                       uint32_t *irq_type, uint32_t *hwirq,
> +                       uint32_t *trigger_type)
> +#endif /* __PLAT_CMN_GIC_FDT_H__ */
> diff --git a/plat/drivers/ofw/fdt.c b/plat/drivers/ofw/fdt.c
> index 69e5e05..afe3f91 100644
> --- a/plat/drivers/ofw/fdt.c
> +++ b/plat/drivers/ofw/fdt.c
> @@ -289,3 +289,23 @@ int fdt_get_interrupt(const void *fdt, int nodeoffset,
>   
>   	return 0;
>   }
> +
> +int gic_get_irq_from_dtb(const void *fdt, int nodeoffset, int index,
> +                       uint32_t *irq_type, uint32_t *hwirq,
> +                       uint32_t *trigger_type)
A suggestion would be to move it to separate file gic_fdt.c. This can be 
done either in this patch series or as a part of the series where we 
split the gic and fdt driver from the kvm library. I will leave it upto 
you.
> +{
> +       fdt32_t *prop;
> +       int ret, size;
> +
> +       UK_ASSERT(irq_type != NULL && hwirq != NULL && trigger_type != NULL);
> +
> +       ret = fdt_get_interrupt(fdt, nodeoffset, index, &size, &prop);
> +       if (ret < 0)
> +               return ret;
> +
> +       *irq_type = fdt32_to_cpu(prop[0]);
> +       *hwirq = fdt32_to_cpu(prop[1]);
> +       *trigger_type = fdt32_to_cpu(prop[2]);
> +
> +       return 0;
> +}
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel