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

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

On 9/24/21 20:43, Titus Rwantare wrote:
> On Fri, 24 Sept 2021 at 03:55, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> 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()
> 
> Yes, that side makes sense. I tried to get at that irq from
> aspeed_ibt.c as follows:
> 
> qemu_irq_lower(ibt->lpc->subdevice_irqs[aspeed_lpc_ibt]); // or raise
> 
> static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
> {
> AspeedIBTState *ibt = ASPEED_IBT(dev);
> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> IPMIInterface *ii = IPMI_INTERFACE(dev);
> ibt->lpc = ASPEED_LPC(dev);
> ...
> 
> but this doesn't work and maybe I'm misusing the dynamic cast?
> 
>>>>> +                       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 ?
>>
> 
> Host:
> -chardev socket,id=ipmichr1,host=localhost,port=9999,reconnect=10 \
> -device ipmi-bmc-extern,chardev=ipmichr1,id=bmc0 \
> -device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults
> 
> BMC:
> -chardev socket,id=ipmichr1,host=localhost,port=9999,server=on,wait=off \
> -device ipmi-host-extern,chardev=ipmichr1,responder=/machine/soc/ibt

This looks correct.

> But for this to work you need Hao's patch as well: [PATCH 7/8]
> hw/ipmi: Add an IPMI external host device.
  
Is it based on my aspeed branch which contains the iBT model ?
Could you send the patchset as an RFC maybe ?


Thanks,

C.