Re: [PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device

[Philippe Mathieu-Daudé]

On 5/2/21 10:45 PM, Peter Maydell wrote:
> On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The OR_IRQ device is bus-less, thus isn't reset automatically.
>> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
>>
>> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/pci-host/prep.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index 0a9162fba97..7481bbf99d4 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int 
>> level)
>>      s->contiguous_map = level;
>>  }
>>
>> +static void raven_pcihost_reset_enter(Object *obj, ResetType type)
>> +{
>> +    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>> +
>> +    if (!s->is_legacy_prep) {
>> +        device_cold_reset(DEVICE(&s->or_irq));
>> +    }
>> +}
>> +
>>  static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>  {
>>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
>> @@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
>>  static void raven_pcihost_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>      dc->realize = raven_pcihost_realizefn;
>>      device_class_set_props(dc, raven_pcihost_properties);
>>      dc->fw_name = "pci";
>> +    rc->phases.enter = raven_pcihost_reset_enter;
>>  }
> 
> Why does this device have an OR gate rather than having its
> map_irq function say "all PCI IRQs go to interrupt 0" ?
> (The PCI core code provides you the "OR" functionality for
> free, because it has to do that anyway for when multiple PCI
> cards share a PCI IRQ -- see pci_change_irq_level() and
> pci_bus_change_irq_level().
> 
> Supplementary question: why does the legacy_prep setup create 4
> outbound sysbus IRQs when the map_irq function can only ever
> return 0 or 1 ?

I'll let the maintainers have a look (I'm just trying to pay the
technical debts so we can remove legacy/deprecated API and move
forward).

Thanks for the careful analysis,

Phil.