Hi Phil,
On 12:34 Mon 29 Sep , Philippe Mathieu-Daudé wrote:
> On 26/9/25 09:07, Luc Michel wrote:
> > Add the target field in the IRQ descriptor. This allows to target an IRQ
> > to another IRQ controller than the GIC(s). Other supported targets are
> > the PMC PPU1 CPU interrupt controller and the EAM (Error management)
> > device. Those two devices are currently not implemented so IRQs
> > targeting those will be left unconnected. This is in preparation for
> > versal2.
> >
> > Signed-off-by: Luc Michel <luc.michel@amd.com>
> > Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> > hw/arm/xlnx-versal.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> > index 3d960ed2636..64744401182 100644
> > --- a/hw/arm/xlnx-versal.c
> > +++ b/hw/arm/xlnx-versal.c
> > @@ -50,18 +50,30 @@
> > #include "hw/cpu/cluster.h"
> > #include "hw/arm/bsa.h"
> >
> > /*
> > * IRQ descriptor to catch the following cases:
> > + * - An IRQ can either connect to the GICs, to the PPU1 intc, or the the EAM
> > * - Multiple devices can connect to the same IRQ. They are OR'ed together.
> > */
> > FIELD(VERSAL_IRQ, IRQ, 0, 16)
> > +FIELD(VERSAL_IRQ, TARGET, 16, 2)
> > FIELD(VERSAL_IRQ, ORED, 18, 1)
> > FIELD(VERSAL_IRQ, OR_IDX, 19, 4) /* input index on the IRQ OR gate */
> >
> > +typedef enum VersalIrqTarget {
> > + IRQ_TARGET_GIC,
> > + IRQ_TARGET_PPU1,
> > + IRQ_TARGET_EAM,
>
> Maybe declare IRQ_TARGET_RSVD here,
I'm not convinced. In the future we may need more targets, even more
than 4. In this case we will increase the TARGET field size, probably we
will then have even more reserved fields. I feel the way it's done here
is simple enough to catch all the buggy cases thanks to the default case
in the switch below.
>
> > +} VersalIrqTarget;
> > +
> > +#define PPU1_IRQ(irq) ((IRQ_TARGET_PPU1 << R_VERSAL_IRQ_TARGET_SHIFT) | (irq))
> > +#define EAM_IRQ(irq) ((IRQ_TARGET_EAM << R_VERSAL_IRQ_TARGET_SHIFT) | (irq))
> > #define OR_IRQ(irq, or_idx) \
> > (R_VERSAL_IRQ_ORED_MASK | ((or_idx) << R_VERSAL_IRQ_OR_IDX_SHIFT) | (irq))
> > +#define PPU1_OR_IRQ(irq, or_idx) \
> > + ((IRQ_TARGET_PPU1 << R_VERSAL_IRQ_TARGET_SHIFT) | OR_IRQ(irq, or_idx))
> >
> > typedef struct VersalSimplePeriphMap {
> > uint64_t addr;
> > int irq;
> > } VersalSimplePeriphMap;
> > @@ -412,19 +424,27 @@ static qemu_irq versal_get_gic_irq(Versal *s, int irq_idx)
> > * Or gates are placed under the /soc/irq-or-gates QOM container.
> > */
> > static qemu_irq versal_get_irq_or_gate_in(Versal *s, int irq_idx,
> > qemu_irq target_irq)
> > {
> > + static const char *TARGET_STR[] = {
> > + [IRQ_TARGET_GIC] = "gic",
> > + [IRQ_TARGET_PPU1] = "ppu1",
> > + [IRQ_TARGET_EAM] = "eam",
> > + };
> > +
> > + VersalIrqTarget target;
> > Object *container = versal_get_child(s, "irq-or-gates");
> > DeviceState *dev;
> > g_autofree char *name;
> > int idx, or_idx;
> >
> > idx = FIELD_EX32(irq_idx, VERSAL_IRQ, IRQ);
> > or_idx = FIELD_EX32(irq_idx, VERSAL_IRQ, OR_IDX);
> > + target = FIELD_EX32(irq_idx, VERSAL_IRQ, TARGET);
>
> and assert(target != IRQ_TARGET_RSVD) here?
This is already ensured by the default case of the switch below.
versal_get_irq gets called before versal_get_irq_or_gate_in.
Thanks
--
Luc
>
> >
> > - name = g_strdup_printf("irq[%d]", idx);
> > + name = g_strdup_printf("%s-irq[%d]", TARGET_STR[target], idx);
> > dev = DEVICE(object_resolve_path_at(container, name));
> >
> > if (dev == NULL) {
> > dev = qdev_new(TYPE_OR_IRQ);
> > object_property_add_child(container, name, OBJECT(dev));
> > @@ -436,16 +456,33 @@ static qemu_irq versal_get_irq_or_gate_in(Versal *s, int irq_idx,
> > return qdev_get_gpio_in(dev, or_idx);
> > }
> >
> > static qemu_irq versal_get_irq(Versal *s, int irq_idx)
> > {
> > + VersalIrqTarget target;
> > qemu_irq irq;
> > bool ored;
> >
> > + target = FIELD_EX32(irq_idx, VERSAL_IRQ, TARGET);
> > ored = FIELD_EX32(irq_idx, VERSAL_IRQ, ORED);
> >
> > - irq = versal_get_gic_irq(s, irq_idx);
> > + switch (target) {
> > + case IRQ_TARGET_EAM:
> > + /* EAM not implemented */
> > + return NULL;
> > +
> > + case IRQ_TARGET_PPU1:
> > + /* PPU1 CPU not implemented */
> > + return NULL;
> > +
> > + case IRQ_TARGET_GIC:
> > + irq = versal_get_gic_irq(s, irq_idx);
> > + break;
> > +
> > + default:
>
> And here 'case IRQ_TARGET_RSVD' instead.
>
> > + g_assert_not_reached();
> > + }
> >
> > if (ored) {
> > irq = versal_get_irq_or_gate_in(s, irq_idx, irq);
> > }
> >
>
--