Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5287 bytes --]

On Sun, 10 Jan 2021, BALATON Zoltan wrote:
> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>> +PCI experts
>> 
>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>> Hi Zoltan,
>>>> 
>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>>> superio chip are wired up by board code. Move creation of these ISA
>>>>> devices to the VIA ISA bridge model so that board code does not need
>>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>>> internal to vt82c686 which allows implementing its configuration via
>>>>> registers in subseqent commits.
>>>> 
>>>> Is this patch dependent of the VT82C686B_PM changes
>>>> or can it be applied before them?
>>> 
>>> I don't know but why would that be better? I thought it's clearer to
>>> clean up pm related parts first before moving more stuff to this file so
>>> that's why this patch comes after (and also because that's the order I
>>> did it).
>> 
>> Not any better, but easier for me to get your patches integrated,
>> as I'm reviewing your patches slowly. Finding other reviewers
>> would certainly help.
>
> No problem, I'll wait for your review. Merging parts of the series does not 
> help much because the whole series is needed for vt8231 which is prerequisite 
> for pegasos2 so eventually all of these are needed so it does not matter if 
> this one patch gets in earlier or later.
>
> Not sure who could help with review. Maybe Jiaxun or Huacai as this is used 
> by fuloong2e so they might be interested and could have info on this chip. 
> Most of these patches just cleaning up the vt82c686b and adding some missing 
> features so these can be reused by the vt8231 model in last 3 patches (which 
> is very similar to 686b only some reg addresses and ids seem to be different 
> for what we are concerned).

Ping? There are still a few patches needing review:

http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512

Jiaxun, Hiacai, or anybody else could you please help reviewing or testing 
if this works with fuloong2e?

Thank you,
BALATON Zoltan

>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>>> 
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 58c0bba1d0..5df9be8ff4 100644
>>>>> --- a/hw/isa/vt82c686.c
>>>>> +++ b/hw/isa/vt82c686.c
>>>>> @@ -16,6 +16,11 @@
>>>>>  #include "hw/qdev-properties.h"
>>>>>  #include "hw/isa/isa.h"
>>>>>  #include "hw/isa/superio.h"
>>>>> +#include "hw/intc/i8259.h"
>>>>> +#include "hw/irq.h"
>>>>> +#include "hw/dma/i8257.h"
>>>>> +#include "hw/timer/i8254.h"
>>>>> +#include "hw/rtc/mc146818rtc.h"
>>>>>  #include "migration/vmstate.h"
>>>>>  #include "hw/isa/apm.h"
>>>>>  #include "hw/acpi/acpi.h"
>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>>>> VT82C686B_ISA)
>>>>> 
>>>>>  struct VT82C686BISAState {
>>>>>      PCIDevice dev;
>>>>> +    qemu_irq cpu_intr;
>>>>>      SuperIOConfig superio_cfg;
>>>>>  };
>>>>> 
>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>>>>> +{
>>>>> +    VT82C686BISAState *s = opaque;
>>>>> +    qemu_set_irq(s->cpu_intr, level);
>>>>> +}
>>>>> +
>>>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>>>                                     uint32_t val, int len)
>>>>>  {
>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>>>> Error **errp)
>>>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>>>      DeviceState *dev = DEVICE(d);
>>>>>      ISABus *isa_bus;
>>>>> +    qemu_irq *isa_irq;
>>>>>      int i;
>>>>> 
>>>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>> 
>>>> Why not use the SysBus API?
>>> 
>>> How? This is a PCIDevice not a SysBusDevice.
>> 
>> Indeed :)
>> 
>>>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>>>> pci_address_space_io(d),
>>>>>                            &error_fatal);
>>>> 
>>>> Isn't it get_system_memory() -> pci_address_space(d)?
>>> 
>>> I don't really know. Most other places that create an isa bus seem to
>>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so
>>> I thought if those others are OK this should be too.
>> 
>> I'm not a PCI expert but my understanding is PCI device functions are
>> restricted to the PCI bus address space. The host bridge may map this
>> space within the host.
>> 
>> QEMU might be using get_system_memory() because for some host bridge
>> the mapping is not implemented so it was easier this way?
>
> Maybe, also one less indirection which if not really needed is a good thing 
> for performance so unless it's found to be needed to use another address 
> space here I'm happy with this as it matches what other similar devices do 
> and it seems to work. Maybe a separate address space is only really needed if 
> we have an iommu?
>
> Regards,
> BALATON Zoltan