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

[Michael Davidsaver]

On 11/21/2017 09:28 PM, David Gibson wrote:
> 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 <address@hidden>
> 
> 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.

I agree that ppce500_init() is unwieldy, and am willing to spend some
time on cleanup.  I'm just not sure what to do.

I can see moving initialization of at least the serial, i2c, gpio, and
possibly MPIC and PCI host bridge into the e500-ccsr class.

I'm not sure what to do with all the device tree construction code in
e500.c, which has a bunch of hard coded register offsets.  A comment
here summarizes the problem nicely.

> /* TODO: parameterize */
> #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL

It seems desirable to avoid having these offsets appear in two different
files, which could allow them to get out of sync.

I had the idea of using an Interface to split up device tree
construction, and was encouraged to find PnvXScomInterfaceClass which
seems be a way of combining device tree construction in a device class.
Is this the way to go?

Some guidance would be appreciated.

Michael


>> ---
>>  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) {
>

signature.asc
Description: OpenPGP digital signature