support ADC and I2C for AST2700

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

Posted by Jamin Lin via 2 months ago

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();
+}
+
 static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
                                                     unsigned int size)
 {
-- 
2.34.1

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

Posted by Cédric Le Goater 1 month, 4 weeks ago

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,

C.




>   static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
>                                                       unsigned int size)
>   {

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

Posted by Jamin Lin 1 month, 3 weeks ago

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);
}

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.

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

Posted by Jamin Lin 1 month, 3 weeks ago

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.