| Message ID | 20171120032420.9134-2-mdavidsaver@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add MVME3100 PPC SBC | expand |
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) {
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 <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. 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) { >
On 11/22/2017 06:55 PM, Michael Davidsaver wrote: > 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 <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. > > 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. each of these controllers should have a QOM model possibly. I think that the part above : /* Load kernel. */ is machine specific and the part below is generic and could be called from each machine init routine. You would still need some op/struct for the device tree. > 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? yes. You would need to QOM'ify the whole machine and have all controllers be children of a 'board' object to loop on them. > Some guidance would be appreciated. The models of the ARM SoC are good examples to follow. You could check the Aspeed machines which have been slowly growing and which have had some good reviews from Peter. The ARM machines are all QOM'ified and that might be difficult to apply to the e500 boards quickly. At least, the models for the new controllers should follow QOM. It will ease the future work. Thanks, C. > > 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) { >> > >
On Wed, Nov 22, 2017 at 11:55:04AM -0600, Michael Davidsaver wrote: > 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 <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. > > 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. So, those aren't bad ideas for cleanups, but I'm not going to insist on that before merging this stuff. What I'm after is something simpler. At the moment the structure looks something like this: machine_init() { common_init(big pile of parameters); } common_init() { chunk1; if (param1) chunk2; chunk3; if (param2) chunk4; ... } What I would prefer is: machine_init1() { chunk1(); chunk2(param1) chunk3(param3); } machine_init2() { chunk1(); chunk3(param3); chunk4(param2); } > > > /* 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? At some point, quite likely (I've actually had thoughts on making devicetree construction more convenient qemu wide, but haven't had time to do much about it).
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) {
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> --- 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(-)