diff mbox series

[v2,06/20] piix4: Add a i8257 DMA Controller as specified in datasheet

Message ID 20191018134754.16362-7-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge | expand

Commit Message

Philippe Mathieu-Daudé Oct. 18, 2019, 1:47 p.m. UTC 
From: Hervé Poussineau <hpoussin@reactos.org>

Remove i8257 instantiated in malta board, to not have it twice.

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
Message-Id: <20171216090228.28505-9-hpoussin@reactos.org>
Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
[PMD: rebased]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/isa/piix4.c       | 4 ++++
 hw/mips/mips_malta.c | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Li Qiang Oct. 21, 2019, 3:25 p.m. UTC | #1 
Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月18日周五 下午9:55写道：

> From: Hervé Poussineau <hpoussin@reactos.org>
>
> Remove i8257 instantiated in malta board, to not have it twice.
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Message-Id: <20171216090228.28505-9-hpoussin@reactos.org>
> Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> [PMD: rebased]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/piix4.c       | 4 ++++
>  hw/mips/mips_malta.c | 2 --
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index ac9383a658..0b24d8323c 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -29,6 +29,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/isa/isa.h"
>  #include "hw/sysbus.h"
> +#include "hw/dma/i8257.h"
>  #include "migration/vmstate.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/runstate.h"
> @@ -167,6 +168,9 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>      /* initialize ISA irqs */
>      isa_bus_irqs(isa_bus, s->isa);
>
> +    /* DMA */
> +    i8257_dma_init(isa_bus, 0);
> +
>      piix4_dev = dev;
>  }
>
>
Could you please explain why this is better calling 'i8257_dma_init' in
piix4 realize function
instead of calling it in mips_malta_init.

I'm still a little of which things should be done in realize and which
should be done in qom instance init function.

Thanks,
Li Qiang



> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index e499b7a6bb..df247177ca 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -28,7 +28,6 @@
>  #include "cpu.h"
>  #include "hw/i386/pc.h"
>  #include "hw/isa/superio.h"
> -#include "hw/dma/i8257.h"
>  #include "hw/char/serial.h"
>  #include "net/net.h"
>  #include "hw/boards.h"
> @@ -1430,7 +1429,6 @@ void mips_malta_init(MachineState *machine)
>      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
>                            isa_get_irq(NULL, 9), NULL, 0, NULL);
>      pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
> -    i8257_dma_init(isa_bus, 0);
>      mc146818_rtc_init(isa_bus, 2000, NULL);
>
>      /* generate SPD EEPROM data */
> --
> 2.21.0
>
>
>
Philippe Mathieu-Daudé Oct. 21, 2019, 3:56 p.m. UTC | #2 
On 10/21/19 5:25 PM, Li Qiang wrote:
> 
> 
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 
> 2019年10月18日周五 下午9:55写道：
> 
>     From: Hervé Poussineau <hpoussin@reactos.org
>     <mailto:hpoussin@reactos.org>>
> 
>     Remove i8257 instantiated in malta board, to not have it twice.
> 
>     Acked-by: Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>>
>     Acked-by: Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>>
>     Signed-off-by: Hervé Poussineau <hpoussin@reactos.org
>     <mailto:hpoussin@reactos.org>>
>     Message-Id: <20171216090228.28505-9-hpoussin@reactos.org
>     <mailto:20171216090228.28505-9-hpoussin@reactos.org>>
>     Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com
>     <mailto:amarkovic@wavecomp.com>>
>     [PMD: rebased]
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>       hw/isa/piix4.c       | 4 ++++
>       hw/mips/mips_malta.c | 2 --
>       2 files changed, 4 insertions(+), 2 deletions(-)
> 
>     diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>     index ac9383a658..0b24d8323c 100644
>     --- a/hw/isa/piix4.c
>     +++ b/hw/isa/piix4.c
>     @@ -29,6 +29,7 @@
>       #include "hw/pci/pci.h"
>       #include "hw/isa/isa.h"
>       #include "hw/sysbus.h"
>     +#include "hw/dma/i8257.h"
>       #include "migration/vmstate.h"
>       #include "sysemu/reset.h"
>       #include "sysemu/runstate.h"
>     @@ -167,6 +168,9 @@ static void piix4_realize(PCIDevice *dev, Error
>     **errp)
>           /* initialize ISA irqs */
>           isa_bus_irqs(isa_bus, s->isa);
> 
>     +    /* DMA */
>     +    i8257_dma_init(isa_bus, 0);
>     +
>           piix4_dev = dev;
>       }
> 
> 
> Could you please explain why this is better calling 'i8257_dma_init' in 
> piix4 realize function
> instead of calling it in mips_malta_init.

i8257_dma_init() is a bit misnamed as it instantiate 2x i8257.

The PIIX4 integrates 2x i8237 (very similar to the i8257).

The i8237 are parts of the PIIX4 chip, and are not chips on the Malta 
board PCB.

So when you instantiate a PIIX4 in QEMU, one expects them integrated, 
and should not have to manually manage them outside of the southbridge 
chipset.

> I'm still a little of which things should be done in realize and which 
> should be done in qom instance init function.

I remember a thread started by Peter Maydell when he was working on the 
MPS2 boards, but I can't find it.

Anyway this thread is more recent:
"Object instantiation vs. device realization: what to do when?"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg596361.html

> 
> Thanks,
> Li Qiang
> 
>     diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>     index e499b7a6bb..df247177ca 100644
>     --- a/hw/mips/mips_malta.c
>     +++ b/hw/mips/mips_malta.c
>     @@ -28,7 +28,6 @@
>       #include "cpu.h"
>       #include "hw/i386/pc.h"
>       #include "hw/isa/superio.h"
>     -#include "hw/dma/i8257.h"
>       #include "hw/char/serial.h"
>       #include "net/net.h"
>       #include "hw/boards.h"
>     @@ -1430,7 +1429,6 @@ void mips_malta_init(MachineState *machine)
>           smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
>                                 isa_get_irq(NULL, 9), NULL, 0, NULL);
>           pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     -    i8257_dma_init(isa_bus, 0);
>           mc146818_rtc_init(isa_bus, 2000, NULL);
> 
>           /* generate SPD EEPROM data */
>     -- 
>     2.21.0
> 
>
Esteban Bosse Oct. 22, 2019, 9:01 a.m. UTC | #3 
El vie, 18-10-2019 a las 15:47 +0200, Philippe Mathieu-Daudé escribió:
> From: Hervé Poussineau <hpoussin@reactos.org>
> 
> Remove i8257 instantiated in malta board, to not have it twice.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Message-Id: <20171216090228.28505-9-hpoussin@reactos.org>
> Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> [PMD: rebased]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/piix4.c       | 4 ++++
>  hw/mips/mips_malta.c | 2 --
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index ac9383a658..0b24d8323c 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -29,6 +29,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/isa/isa.h"
>  #include "hw/sysbus.h"
> +#include "hw/dma/i8257.h"
>  #include "migration/vmstate.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/runstate.h"
> @@ -167,6 +168,9 @@ static void piix4_realize(PCIDevice *dev, Error
> **errp)
>      /* initialize ISA irqs */
>      isa_bus_irqs(isa_bus, s->isa);
>  
> +    /* DMA */
> +    i8257_dma_init(isa_bus, 0);
> +
>      piix4_dev = dev;
>  }
>  
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index e499b7a6bb..df247177ca 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -28,7 +28,6 @@
>  #include "cpu.h"
>  #include "hw/i386/pc.h"
>  #include "hw/isa/superio.h"
> -#include "hw/dma/i8257.h"
>  #include "hw/char/serial.h"
>  #include "net/net.h"
>  #include "hw/boards.h"
> @@ -1430,7 +1429,6 @@ void mips_malta_init(MachineState *machine)
>      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
>                            isa_get_irq(NULL, 9), NULL, 0, NULL);
>      pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
> -    i8257_dma_init(isa_bus, 0);
>      mc146818_rtc_init(isa_bus, 2000, NULL);
>  
>      /* generate SPD EEPROM data */
Reviewed-by: Esteban Bosse <estebanbosse@gmail.com>
diff mbox series

Patch

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index ac9383a658..0b24d8323c 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -29,6 +29,7 @@ 
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/dma/i8257.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
@@ -167,6 +168,9 @@  static void piix4_realize(PCIDevice *dev, Error **errp)
     /* initialize ISA irqs */
     isa_bus_irqs(isa_bus, s->isa);
 
+    /* DMA */
+    i8257_dma_init(isa_bus, 0);
+
     piix4_dev = dev;
 }
 
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e499b7a6bb..df247177ca 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -28,7 +28,6 @@ 
 #include "cpu.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/superio.h"
-#include "hw/dma/i8257.h"
 #include "hw/char/serial.h"
 #include "net/net.h"
 #include "hw/boards.h"
@@ -1430,7 +1429,6 @@  void mips_malta_init(MachineState *machine)
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
                           isa_get_irq(NULL, 9), NULL, 0, NULL);
     pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
-    i8257_dma_init(isa_bus, 0);
     mc146818_rtc_init(isa_bus, 2000, NULL);
 
     /* generate SPD EEPROM data */