Re: [Qemu-devel] [PATCH 01/12] e500: add board config options

[David Gibson <david@gibson.dropbear.id.au>]

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

On Sun, Nov 19, 2017 at 09:24:09PM -0600, Michael Davidsaver wrote:
> allow board code to skip common NIC and guest image setup
> and configure decrementor frequency.
> Existing boards unchanged.
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

So, it's spelled "decrementer".

Other than that, the patch looks correct.  However having a big common
function for overall init with a pile of ad-hoc configuration
parameters is usually not a great way to go.  I think what we want
instead is to eliminate ppce500_init(), instead doing the setup logic
separately in each of the e500 machines.   The large common slabs of
code can be helpers in e500.c, but the overall logic - including most
of the things controlled by the current params - would be under the
individual machine's control.

> ---
>  hw/ppc/e500.c      | 8 ++++++--
>  hw/ppc/e500.h      | 3 +++
>  hw/ppc/e500plat.c  | 1 +
>  hw/ppc/mpc8544ds.c | 1 +
>  4 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 5cf0dabef3..9e7e1b29c4 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -826,7 +826,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          env->mpic_iack = params->ccsrbar_base +
>                           MPC8544_MPIC_REGS_OFFSET + 0xa0;
>  
> -        ppc_booke_timers_init(cpu, 400000000, PPC_TIMER_E500);
> +        ppc_booke_timers_init(cpu, params->decrementor_freq, PPC_TIMER_E500);
>  
>          /* Register reset handler */
>          if (!i) {
> @@ -899,7 +899,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      if (!pci_bus)
>          printf("couldn't create PCI controller!\n");
>  
> -    if (pci_bus) {
> +    if (pci_bus && !params->tsec_nic) {
>          /* Register network interfaces. */
>          for (i = 0; i < nb_nics; i++) {
>              pci_nic_init_nofail(&nd_table[i], pci_bus, "virtio", NULL);
> @@ -948,6 +948,10 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>                                      sysbus_mmio_get_region(s, 0));
>      }
>  
> +    if (params->skip_load) {
> +        return;
> +    }
> +
>      /* Load kernel. */
>      if (machine->kernel_filename) {
>          kernel_base = cur_base;
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 70ba1d8f4f..40f72f2de2 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -22,6 +22,9 @@ typedef struct PPCE500Params {
>      hwaddr pci_mmio_base;
>      hwaddr pci_mmio_bus_base;
>      hwaddr spin_base;
> +    uint32_t decrementor_freq; /* in Hz */
> +    bool skip_load;
> +    bool tsec_nic;
>  } PPCE500Params;
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index e59e80fb9e..3d07987bd1 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -47,6 +47,7 @@ static void e500plat_init(MachineState *machine)
>          .pci_mmio_base = 0xC00000000ULL,
>          .pci_mmio_bus_base = 0xE0000000ULL,
>          .spin_base = 0xFEF000000ULL,
> +        .decrementor_freq = 400000000,
>      };
>  
>      /* Older KVM versions don't support EPR which breaks guests when we announce
> diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
> index 1717953ec7..6d9931c475 100644
> --- a/hw/ppc/mpc8544ds.c
> +++ b/hw/ppc/mpc8544ds.c
> @@ -40,6 +40,7 @@ static void mpc8544ds_init(MachineState *machine)
>          .pci_mmio_bus_base = 0xC0000000ULL,
>          .pci_pio_base = 0xE1000000ULL,
>          .spin_base = 0xEF000000ULL,
> +        .decrementor_freq = 400000000,
>      };
>  
>      if (machine->ram_size > 0xc0000000) {

-- 
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 --]