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

2017-11-23 Thread David Gibson 
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 
> > 
> > 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   0x0010ULL
> > #define MPC8544_MPIC_REGS_OFFSET   0x4ULL
> 
> 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).

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


signature.asc
Description: PGP signature

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

2017-11-22 Thread 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 
> 
> 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   0x0010ULL
> #define MPC8544_MPIC_REGS_OFFSET   0x4ULL

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, 4, 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(_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 = 0xCULL,
>>  .pci_mmio_bus_base = 0xE000ULL,
>>  .spin_base = 0xFEF00ULL,
>> +.decrementor_freq = 4,
>>  };
>>  
>>  /* 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 = 0xC000ULL,
>>  .pci_pio_base = 0xE100ULL,
>>  .spin_base = 0xEF00ULL,
>> +.decrementor_freq = 4,
>>  };
>>  
>>  if (machine->ram_size > 0xc000) {
> 




signature.asc
Description: OpenPGP digital signature

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

2017-11-21 Thread David Gibson 
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 

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, 4, 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(_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 = 0xCULL,
>  .pci_mmio_bus_base = 0xE000ULL,
>  .spin_base = 0xFEF00ULL,
> +.decrementor_freq = 4,
>  };
>  
>  /* 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 = 0xC000ULL,
>  .pci_pio_base = 0xE100ULL,
>  .spin_base = 0xEF00ULL,
> +.decrementor_freq = 4,
>  };
>  
>  if (machine->ram_size > 0xc000) {

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


signature.asc
Description: PGP signature

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

2017-11-19 Thread Michael Davidsaver 
allow board code to skip common NIC and guest image setup
and configure decrementor frequency.
Existing boards unchanged.

Signed-off-by: Michael Davidsaver 
---
 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, 4, 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(_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 = 0xCULL,
 .pci_mmio_bus_base = 0xE000ULL,
 .spin_base = 0xFEF00ULL,
+.decrementor_freq = 4,
 };
 
 /* 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 = 0xC000ULL,
 .pci_pio_base = 0xE100ULL,
 .spin_base = 0xEF00ULL,
+.decrementor_freq = 4,
 };
 
 if (machine->ram_size > 0xc000) {
-- 
2.11.0