[PATCH 01/11] hw/rtc/mc146818rtc: Inline isa_connect_gpio_out() and remove it

Bernhard Beschow posted 11 patches 3 years, 9 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Jason Wang <jasowang@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>
There is a newer version of this series
[PATCH 01/11] hw/rtc/mc146818rtc: Inline isa_connect_gpio_out() and remove it
Posted by Bernhard Beschow 3 years, 9 months ago
Commit 250263033c5343012b2cd6f01210ffb5b908a159 'isa: introduce wrapper
isa_connect_gpio_out' introduced it in 2016. Since then, its only user
remained mc146818rtc. Remove this one-off solution.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/isa-bus.c     | 6 ------
 hw/rtc/mc146818rtc.c | 3 ++-
 include/hw/isa/isa.h | 1 -
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 0ad1c5fd65..59eb1af9e3 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -85,12 +85,6 @@ qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
     return isabus->irqs[isairq];
 }
 
-void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
-{
-    qemu_irq irq = isa_get_irq(isadev, isairq);
-    qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
-}
-
 void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16)
 {
     assert(bus && dma8 && dma16);
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index f235c2ddbe..79c43391cb 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -972,7 +972,8 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
     if (intercept_irq) {
         qdev_connect_gpio_out(dev, 0, intercept_irq);
     } else {
-        isa_connect_gpio_out(isadev, 0, s->isairq);
+        qemu_irq irq = isa_get_irq(isadev, s->isairq);
+        qdev_connect_gpio_out(dev, 0, irq);
     }
 
     object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(isadev),
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 034d706ba1..1b758ae1ab 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -90,7 +90,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space,
                     MemoryRegion *address_space_io, Error **errp);
 void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
 qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
-void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
 void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
 IsaDma *isa_get_dma(ISABus *bus, int nchan);
 MemoryRegion *isa_address_space(ISADevice *dev);
-- 
2.36.0
Re: [PATCH 01/11] hw/rtc/mc146818rtc: Inline isa_connect_gpio_out() and remove it
Posted by Mark Cave-Ayland 3 years, 9 months ago
On 05/05/2022 17:17, Bernhard Beschow wrote:

> Commit 250263033c5343012b2cd6f01210ffb5b908a159 'isa: introduce wrapper
> isa_connect_gpio_out' introduced it in 2016. Since then, its only user
> remained mc146818rtc. Remove this one-off solution.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/isa-bus.c     | 6 ------
>   hw/rtc/mc146818rtc.c | 3 ++-
>   include/hw/isa/isa.h | 1 -
>   3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 0ad1c5fd65..59eb1af9e3 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -85,12 +85,6 @@ qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
>       return isabus->irqs[isairq];
>   }
>   
> -void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
> -{
> -    qemu_irq irq = isa_get_irq(isadev, isairq);
> -    qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
> -}
> -
>   void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16)
>   {
>       assert(bus && dma8 && dma16);
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index f235c2ddbe..79c43391cb 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -972,7 +972,8 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
>       if (intercept_irq) {
>           qdev_connect_gpio_out(dev, 0, intercept_irq);
>       } else {
> -        isa_connect_gpio_out(isadev, 0, s->isairq);
> +        qemu_irq irq = isa_get_irq(isadev, s->isairq);
> +        qdev_connect_gpio_out(dev, 0, irq);
>       }
>   
>       object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(isadev),
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 034d706ba1..1b758ae1ab 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -90,7 +90,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space,
>                       MemoryRegion *address_space_io, Error **errp);
>   void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
>   qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
> -void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
>   void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
>   IsaDma *isa_get_dma(ISABus *bus, int nchan);
>   MemoryRegion *isa_address_space(ISADevice *dev);

Hi Bernhard,

I've been tinkering again with trying to improve QEMU's ISA implementation to use 
gpios, and actually I believe as per the comment in isa-bus.c that 
isa_connect_gpio_out() will be the preferred way to wire up ISA devices. So really we 
should be trying to use this function more rather than removing it.

BTW if you are interested in helping to work on ISA bus improvements, I can certainly 
come up with a TODO list of useful tasks :)


ATB,

Mark.
Re: [PATCH 01/11] hw/rtc/mc146818rtc: Inline isa_connect_gpio_out() and remove it
Posted by Bernhard Beschow 3 years, 9 months ago
Am 11. Mai 2022 10:24:58 UTC schrieb Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk>:
>On 05/05/2022 17:17, Bernhard Beschow wrote:
>
>> Commit 250263033c5343012b2cd6f01210ffb5b908a159 'isa: introduce wrapper
>> isa_connect_gpio_out' introduced it in 2016. Since then, its only user
>> remained mc146818rtc. Remove this one-off solution.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/isa/isa-bus.c     | 6 ------
>>   hw/rtc/mc146818rtc.c | 3 ++-
>>   include/hw/isa/isa.h | 1 -
>>   3 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
>> index 0ad1c5fd65..59eb1af9e3 100644
>> --- a/hw/isa/isa-bus.c
>> +++ b/hw/isa/isa-bus.c
>> @@ -85,12 +85,6 @@ qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
>>       return isabus->irqs[isairq];
>>   }
>>   -void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
>> -{
>> -    qemu_irq irq = isa_get_irq(isadev, isairq);
>> -    qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
>> -}
>> -
>>   void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16)
>>   {
>>       assert(bus && dma8 && dma16);
>> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>> index f235c2ddbe..79c43391cb 100644
>> --- a/hw/rtc/mc146818rtc.c
>> +++ b/hw/rtc/mc146818rtc.c
>> @@ -972,7 +972,8 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
>>       if (intercept_irq) {
>>           qdev_connect_gpio_out(dev, 0, intercept_irq);
>>       } else {
>> -        isa_connect_gpio_out(isadev, 0, s->isairq);
>> +        qemu_irq irq = isa_get_irq(isadev, s->isairq);
>> +        qdev_connect_gpio_out(dev, 0, irq);
>>       }
>>         object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(isadev),
>> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
>> index 034d706ba1..1b758ae1ab 100644
>> --- a/include/hw/isa/isa.h
>> +++ b/include/hw/isa/isa.h
>> @@ -90,7 +90,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space,
>>                       MemoryRegion *address_space_io, Error **errp);
>>   void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
>>   qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
>> -void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
>>   void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
>>   IsaDma *isa_get_dma(ISABus *bus, int nchan);
>>   MemoryRegion *isa_address_space(ISADevice *dev);
>
>Hi Bernhard,

Hi Mark,

>I've been tinkering again with trying to improve QEMU's ISA implementation to use gpios, and actually I believe as per the comment in isa-bus.c that isa_connect_gpio_out() will be the preferred way to wire up ISA devices. So really we should be trying to use this function more rather than removing it.
>
>BTW if you are interested in helping to work on ISA bus improvements, I can certainly come up with a TODO list of useful tasks :)

Yes, that'd be great!

As expressed earlier [1] I'd like to remove the isabus global in
isa-bus.c. Meanwhile, I implemented a POC [2]. While the result seems
to work for the PC and Malta boards, some of the changes required
don't seem quite clean. So yeah, I'd be interested in the bigger
picture regarding ISA improvements and would like to help out as time
permits, too.

Best regards,
Bernhard

[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg05955.html
[2] https://github.com/shentok/qemu/commits/isa

>
>ATB,
>
>Mark.
Re: [PATCH 01/11] hw/rtc/mc146818rtc: Inline isa_connect_gpio_out() and remove it
Posted by Philippe Mathieu-Daudé 3 years, 9 months ago
On 5/5/22 18:17, Bernhard Beschow wrote:
> Commit 250263033c5343012b2cd6f01210ffb5b908a159 'isa: introduce wrapper
> isa_connect_gpio_out' introduced it in 2016. Since then, its only user
> remained mc146818rtc. Remove this one-off solution.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/isa-bus.c     | 6 ------
>   hw/rtc/mc146818rtc.c | 3 ++-
>   include/hw/isa/isa.h | 1 -
>   3 files changed, 2 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>