On Wed, 27 Nov 2024 at 11:26, Cédric Le Goater <c...@kaod.org> wrote:
>
>
> > Having to modify sdhci.c internals is dubious, since inversion
> > occurs out of this block. If this is the soc/board layer, isn't
> > better to model at this level? Smth like:
> >
> > -- >8 --
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index be3eb70cdd7..aad9be66b75 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState
> > *dev, Error **errp)
> > }
> > aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
> > sc->memmap[ASPEED_DEV_SDHCI]);
> > + irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
> > sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
> > - aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
> > + sc->sdhci_wp_inverted ? qemu_irq_invert(irq) : irq);
> >
> > /* eMMC */
> > if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
> > ---
>
> Nice ! I didn't know about qemu_irq_invert().
I am not a fan of qemu_irq_invert(). It's one of those ancient
pre-QOM APIs that we ideally would get rid of at some point.
Two problems with it:
(1) It allocates and returns a qemu_irq directly,
so in the patch above you're effectively leaking that
allocation. (Not a big deal since the SoC object is going to
be around for the life of the QEMU process, but probably
the clang leak-sanitizer will complain.)
(2) It calls qemu_irq_raise() directly, immediately. This is
kind of bogus in a realize function, where you're not supposed
to be raising IRQ lines yet; it also doesn't do anything about
reset, so if the device on the other end *did* care about seeing
that 0->1 transition then it will be broken on system-reset
because the transition won't happen. (Handling "device is supposed
to have an asserted-as-1 line coming out of reset" is not
something that we do very well in QEMU generally. In theory
3-phase reset is supposed to help with this by letting you do
the assert-the-line in the reset-exit phase, but in practice we
typically just don't model the line-assertion at all and
trust that the reset state of the device on the far end is
what it ought to be anyway...)
I would not recommend using qemu_irq_invert() in new code.
I guess in an ideal world we'd implement a QOM object
that encapsulated the the "not gate" logic, similar to
TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
so it doesn't get reset properly on system reset and so
the "what happens to the output on reset" is still not
really correct.)
thanks
-- PMM
