Re: [PATCH] hw/misc: Add an iBT device model

["Cédric Le Goater" <clg@kaod.org>]

Hello Titus,

On 9/24/21 10:42, Philippe Mathieu-Daudé wrote:
> On 9/24/21 01:48, Titus Rwantare wrote:
>> Hello all,
>>
>> I'd like some clarification on how the following code transfers irqs
>> back and forth:
>>> b/hw/arm/aspeed_soc.c
>>> +    /* iBT */
>>> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->ibt), errp)) {
>>> +        return;
>>> +    }
>>> +    memory_region_add_subregion(&s->lpc.iomem,
>>> +                   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
>>> +                   &s->ibt.iomem);
>>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_ibt,


The iBT device IRQ is connected to a subdevice irq of the LPC device.
See aspeed_lpc_realize(). And triggered in aspeed_lpc_set_irq()

> Code smell indeed, likely:
> 
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->ibt), 1 + aspeed_lpc_ibt,

why ? See what's done above for the LPC device. Commit c59f781e3bcc
("hw/misc: Model KCS devices in the Aspeed LPC controller") and comment


     /*
      * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
      * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
      * contrast, on the AST2600, the subdevice IRQs are connected straight to
      * the GIC).
      *
      * LPC subdevice IRQ sources are offset from 1 because the shared IRQ output
      * to the VIC is at offset 0.
      */

> 
>>> +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_ibt));
>>> }
>>
>> and
>>
>>> hw/misc/aspeed_ibt.c
>>> +static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> +    AspeedIBTState *ibt = ASPEED_IBT(dev);
>> ...
>>> +
>>> +    sysbus_init_irq(sbd, &ibt->irq);
>>
>> I ask because the code in aspeed_soc.c seems to connect to the
>> lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
>> hw/misc/aspeed_lpc.c:408.
>> I noticed that bmc firmware running in qemu was checking the BT_CTRL
>> register less frequently than I'd like while editing this patch to use
>> the IPMIInterface.

OK.

This might be a problem in aspeed_ibt_update_irq(). This patch is
an experiment from some few years ago. It still works good enough
for the witherspoon-bmc and powernv9 machines for simple IPMI
commands: fru, sdr, lan, power off (to be checked).

Could you share your BMC and host command line ?

Thanks,

C.