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 <mdavidsa...@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.
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
