RE: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC orgate information

[Jamin Lin]

Sorry for typo

> Subject: RE: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC
> orgate information
>
> Hi Cedric,
>
> > Subject: Re: [PATCH v1 12/15] aspeed/soc: introduce a new API to get
> > the INTC orgate information
> >
> > On 7/18/24 08:49, Jamin Lin wrote:
> > > Currently, users can set the intc mapping table with enumerated
> > > device id and device irq to get the INTC orgate input pins. However,
> > > some devices use the continuous bits number in the same orgate. To
> > > reduce the enumerated device id definition, create a new API to get
> > > the INTC orgate index and source bit number if users only provide
> > > the start bus number of device.
> > >
> > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > ---
> > >   hw/arm/aspeed_ast27x0.c | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > >
> > > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > > 4257b5e8af..0bbd66110b 100644
> > > --- a/hw/arm/aspeed_ast27x0.c
> > > +++ b/hw/arm/aspeed_ast27x0.c
> > > @@ -164,6 +164,11 @@ struct gic_intc_irq_info {
> > >       const int *ptr;
> > >   };
> > >
> > > +struct gic_intc_orgate_info {
> > > +    int index;
> > > +    int int_num;
> > > +};
> > > +
> > >   static const struct gic_intc_irq_info
> > > aspeed_soc_ast2700_gic_intcmap[] =
> > {
> > >       {128,  aspeed_soc_ast2700_gic128_intcmap},
> > >       {129,  NULL},
> > > @@ -193,6 +198,27 @@ static qemu_irq
> > aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev)
> > >       return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]);
> > >   }
> > >
> > > +static void aspeed_soc_ast2700_get_intc_orgate(AspeedSoCState *s,
> > > +int
> > dev,
> > > +    struct gic_intc_orgate_info *orgate_info) {
> > > +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> > > +    int i;
> > > +
> > > +    for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
> > > +        if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq)
> {
> > > +            assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
> > > +            orgate_info->index = i;
> > > +            orgate_info->int_num =
> > aspeed_soc_ast2700_gic_intcmap[i].ptr[dev];
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > > +    /*
> > > +     * Invalid orgate index, device irq should be 128 to 136.
> > > +     */
> > > +    g_assert_not_reached();
> > > +}
> >
> >
> > This looks redundant. Couldn't we extend aspeed_soc_ast2700_get_irq()
> > with an index parameter instead ?
> >
>
> Thanks for review and suggestion.
>
> According to the current design of aspeed_soc_get_irq, the function type of
> get_irq callback function should be XXX(AspeedSoCState *s, int dev)
>
> qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev) {
>     return ASPEED_SOC_GET_CLASS(s)->get_irq(s, dev); }
>
> struct AspeedSoCClass {
>     qemu_irq (*get_irq)(AspeedSoCState *s, int dev); }
>
> If we want to add a new index parameter in aspeed_soc_ast2700_get_irq, I
> will change as following.
> 1.
> static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev, int
> index) {
>     Aspeed27x0SoCState *a = ASPEED27X0_SOC(s);
>     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>     int i;
>
>     for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
>         if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq) {
>             assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
>             return qdev_get_gpio_in(DEVICE(&a->intc.orgates[i]),
>                 aspeed_soc_ast2700_gic_intcmap[i].ptr[dev] + index);
>         }
>     }
>
>     return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]); }
>
> 2. introduce a new get_irq_index function pointer and the function type of 
> this
> callback function as following.
> struct AspeedSoCClass {
>    qemu_irq_index (*get_irq)(AspeedSoCState *s, int dev, int index); }
>
qemu_irq (*get_irq_index)(AspeedSoCState *s, int dev, int index);


3. Add aspeed_soc_get_irq_index
qemu_irq aspeed_soc_get_irq_index(AspeedSoCState *s, int dev, int index)
{
    return ASPEED_SOC_GET_CLASS(s)->get_irq_index(s, dev, index);
}

4. I need to modify this function, too
bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
{
 sysbus_connect_irq(SYS_BUS_DEVICE(smm), 0, aspeed_soc_get_irq(s, uart));
}

Thanks-Jamin
> Do you have any concern or could you please give me any suggestion?
> Thanks-Jamin
>
>
> > Thanks,
> >
> > C.
> >
> >
> >
> >
> > >   static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
> > >
> unsigned
> > int size)
> > >   {

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 
並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other 
confidential information. If you have received it in error, please notify the 
sender by reply e-mail and immediately delete the e-mail and any attachments 
without copying or disclosing the contents. Thank you.