Re: [PATCH 2/4] hw/pci-host/prep: Remove legacy PReP machine temporary workaround

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 10/12/20 9:28 AM, Thomas Huth wrote:
> On 12/10/2020 09.19, Philippe Mathieu-Daudé wrote:
>> The legacy PReP machine has been removed in commit b2ce76a0730
>> ("hw/ppc/prep: Remove the deprecated "prep" machine and the
>> OpenHackware BIOS"). This temporary workaround is no more
>> required, remove it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/pci-host/prep.c | 32 +++++++++++---------------------
>>   1 file changed, 11 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index 80dfb67da43..064593d1e52 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -75,7 +75,6 @@ struct PRePPCIState {
>>       RavenPCIState pci_dev;
>>   
>>       int contiguous_map;
>> -    bool is_legacy_prep;
>>   };
>>   
>>   #define BIOS_SIZE (1 * MiB)
>> @@ -229,24 +228,18 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>       MemoryRegion *address_space_mem = get_system_memory();
>>       int i;
>>   
>> -    if (s->is_legacy_prep) {
>> -        for (i = 0; i < PCI_NUM_PINS; i++) {
>> -            sysbus_init_irq(dev, &s->pci_irqs[i]);
>> -        }
>> -    } else {
>> -        /*
>> -         * According to PReP specification section 6.1.6 "System Interrupt
>> -         * Assignments", all PCI interrupts are routed via IRQ 15
>> -         */
>> -        s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
>> -        object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
>> -                                &error_fatal);
>> -        qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
>> -        sysbus_init_irq(dev, &s->or_irq->out_irq);
>> +    /*
>> +     * According to PReP specification section 6.1.6 "System Interrupt
>> +     * Assignments", all PCI interrupts are routed via IRQ 15.
>> +     */
> 
> Since you're changing the indentation of these lines anyway, I think you
> could also simply squash patch 1 into this one here (just a matter of taste,
> though).

Then the diff is not trivial to review :/

> 
>> +    s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
>> +    object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
>> +                            &error_fatal);
>> +    qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
>> +    sysbus_init_irq(dev, &s->or_irq->out_irq);
>>   
>> -        for (i = 0; i < PCI_NUM_PINS; i++) {
>> -            s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
>> -        }
>> +    for (i = 0; i < PCI_NUM_PINS; i++) {
>> +        s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
>>       }
>>   
>>       qdev_init_gpio_in(d, raven_change_gpio, 1);
>> @@ -403,9 +396,6 @@ static Property raven_pcihost_properties[] = {
>>       DEFINE_PROP_UINT32("elf-machine", PREPPCIState, pci_dev.elf_machine,
>>                          EM_NONE),
>>       DEFINE_PROP_STRING("bios-name", PREPPCIState, pci_dev.bios_name),
>> -    /* Temporary workaround until legacy prep machine is removed */
>> -    DEFINE_PROP_BOOL("is-legacy-prep", PREPPCIState, is_legacy_prep,
>> -                     false),
>>       DEFINE_PROP_END_OF_LIST()
>>   };
>>   
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!