Re: [Qemu-devel] [PATCH 10/17] e500: move uarts CCSR

From: David Gibson <david@gibson.dropbear.id.au>
To: Michael Davidsaver <mdavidsaver@gmail.com>
Cc: Alexander Graf <agraf@suse.de>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 10/17] e500: move uarts CCSR
Date: Tue, 5 Dec 2017 17:37:10 +1100	[thread overview]
Message-ID: <20171205063710.GH3057@umbus.fritz.box> (raw)
In-Reply-To: <5d63d645603b83d24187c3844bd0728bd4c4b682.1511731946.git.mdavidsaver@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

On Sun, Nov 26, 2017 at 03:59:08PM -0600, Michael Davidsaver wrote:
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/ppc/e500.c      | 13 -------------
>  hw/ppc/e500_ccsr.c | 18 ++++++++++++++++++
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 1872bb8eaa..2d87d91582 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -22,7 +22,6 @@
>  #include "net/net.h"
>  #include "qemu/config-file.h"
>  #include "hw/hw.h"
> -#include "hw/char/serial.h"
>  #include "hw/pci/pci.h"
>  #include "hw/boards.h"
>  #include "sysemu/sysemu.h"
> @@ -770,18 +769,6 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      mpicdev = DEVICE(object_resolve_path("/machine/pic", 0));
>      assert(mpicdev);
>  
> -    /* Serial */
> -    if (serial_hds[0]) {
> -        serial_mm_init(ccsr_addr_space, MPC8544_SERIAL0_REGS_OFFSET,
> -                       0, qdev_get_gpio_in(mpicdev, 42), 399193,
> -                       serial_hds[0], DEVICE_BIG_ENDIAN);
> -    }
> -
> -    if (serial_hds[1]) {
> -        serial_mm_init(ccsr_addr_space, MPC8544_SERIAL1_REGS_OFFSET,
> -                       0, qdev_get_gpio_in(mpicdev, 42), 399193,
> -                       serial_hds[1], DEVICE_BIG_ENDIAN);
> -    }
>  
>      /* PCI */
>      dev = qdev_create(NULL, "e500-pcihost");
> diff --git a/hw/ppc/e500_ccsr.c b/hw/ppc/e500_ccsr.c
> index 68d952794e..f1adba4e54 100644
> --- a/hw/ppc/e500_ccsr.c
> +++ b/hw/ppc/e500_ccsr.c
> @@ -31,6 +31,7 @@
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "hw/sysbus.h"
> +#include "hw/char/serial.h"
>  #include "hw/ppc/openpic.h"
>  
>  /* E500_ denotes registers common to all */
> @@ -45,6 +46,8 @@
>  #define E500_ERR_DETECT  (0x2e40)
>  #define E500_ERR_DISABLE (0x2e44)
>  
> +#define E500_DUART_OFFSET(N) (0x4500 + (N) * 0x100)
> +
>  #define E500_PORPLLSR    (0xE0000)
>  #define E500_PVR         (0xE00A0)
>  #define E500_SVR         (0xE00A4)
> @@ -266,6 +269,21 @@ static void e500_ccsr_realize(DeviceState *dev, Error **errp)
>  
>      memory_region_add_subregion(&ccsr->iomem, E500_MPIC_OFFSET,
>                                  sysbus_mmio_get_region(pic, 0));
> +    /* Note: MPIC internal interrupts are offset by 16 */
> +
> +    /* DUARTS */
> +    if (serial_hds[0]) {
> +        serial_mm_init(&ccsr->iomem, E500_DUART_OFFSET(0),

By changing the constants in use here for the base address you're now
creating the actual device and creating the device tree entry from
different (but same valued) constants.  That seems risky.

> +                       0, qdev_get_gpio_in(ccsr->pic, 16 + 26), 399193,
> +                       serial_hds[0], DEVICE_BIG_ENDIAN);
> +    }
> +
> +    if (serial_hds[1]) {
> +        serial_mm_init(&ccsr->iomem, E500_DUART_OFFSET(1),
> +                       0, qdev_get_gpio_in(ccsr->pic, 16 + 26), 399193,
> +                       serial_hds[1], DEVICE_BIG_ENDIAN);
> +    }
> +
>  }
>  
>  static Property e500_ccsr_props[] = {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]