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

[Jia He <jiakernel2@xxxxxxxxx>]

On 2019/7/10 17:53, Sharan Santhanam wrote:
> 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?
Ok, I will move it into ofw directory

> >   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.
ok


> > + * @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.
Yes, reasonable to me, thanks
> > +
> > +    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
--

---
Cheers,
Justin (Jia He)


_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel