[PATCH 0/2] ppc/pnv: Reset handler registration cleanup

[PATCH 0/2] ppc/pnv: Reset handler registration cleanup

[Greg Kurz]

These patches were initially posted in Cedric's "ppc/pnv: remove the use of
qdev_get_machine()" series, which isn't really related to reset handlers
BTW.

The only change is to call device_reset() instead of open-coding the
invocation of DeviceReset::reset() as suggested by David.

--
Greg

---

Greg Kurz (2):
      pnv/psi: Add device reset hook
      pnv/psi: Consolidate some duplicated code in pnv_psi_realize()


 hw/ppc/pnv_psi.c |   30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

[PATCH 1/2] pnv/psi: Add device reset hook

[Greg Kurz]

And call it from a QEMU reset handler. This allows each PNV child class to
override the reset hook if needed, eg. POWER8 doesn't but POWER9 does.
The proper way to do that would be to use device_class_set_parent_reset(),
but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_reset
pointer adds a fair amount of code. Calling pnv_psi_reset() explicitely is
fine for now.

A subsequent patch will consolidate the call to qemu_register_reset() in
a single place.

Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_psi.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 75e20d9da08b..6c94781e377d 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -455,7 +455,7 @@ static const MemoryRegionOps pnv_psi_xscom_ops = {
     }
 };
 
-static void pnv_psi_reset(void *dev)
+static void pnv_psi_reset(DeviceState *dev)
 {
     PnvPsi *psi = PNV_PSI(dev);
 
@@ -464,6 +464,11 @@ static void pnv_psi_reset(void *dev)
     psi->regs[PSIHB_XSCOM_BAR] = psi->bar | PSIHB_BAR_EN;
 }
 
+static void pnv_psi_reset_handler(void *dev)
+{
+    device_reset(DEVICE(dev));
+}
+
 static void pnv_psi_power8_instance_init(Object *obj)
 {
     Pnv8Psi *psi8 = PNV8_PSI(obj);
@@ -533,7 +538,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
             ((uint64_t) i << PSIHB_XIVR_SRC_SH);
     }
 
-    qemu_register_reset(pnv_psi_reset, dev);
+    qemu_register_reset(pnv_psi_reset_handler, dev);
 }
 
 static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
@@ -816,7 +821,7 @@ static void pnv_psi_power9_irq_set(PnvPsi *psi, int irq, bool state)
     qemu_set_irq(psi->qirqs[irq], state);
 }
 
-static void pnv_psi_power9_reset(void *dev)
+static void pnv_psi_power9_reset(DeviceState *dev)
 {
     Pnv9Psi *psi = PNV9_PSI(dev);
 
@@ -870,7 +875,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
 
     pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
 
-    qemu_register_reset(pnv_psi_power9_reset, dev);
+    qemu_register_reset(pnv_psi_reset_handler, dev);
 }
 
 static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
@@ -882,6 +887,7 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
 
     dc->desc    = "PowerNV PSI Controller POWER9";
     dc->realize = pnv_psi_power9_realize;
+    dc->reset   = pnv_psi_power9_reset;
 
     ppc->xscom_pcba = PNV9_XSCOM_PSIHB_BASE;
     ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
@@ -934,6 +940,7 @@ static void pnv_psi_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "PowerNV PSI Controller";
     dc->props = pnv_psi_properties;
+    dc->reset = pnv_psi_reset;
 }
 
 static const TypeInfo pnv_psi_info = {

Re: [PATCH 1/2] pnv/psi: Add device reset hook

[Philippe Mathieu-Daudé]

On 1/7/20 5:32 PM, Greg Kurz wrote:
> And call it from a QEMU reset handler. This allows each PNV child class to
> override the reset hook if needed, eg. POWER8 doesn't but POWER9 does.
> The proper way to do that would be to use device_class_set_parent_reset(),
> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_reset
> pointer adds a fair amount of code. Calling pnv_psi_reset() explicitely is
> fine for now.
> 
> A subsequent patch will consolidate the call to qemu_register_reset() in
> a single place.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/ppc/pnv_psi.c |   15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 75e20d9da08b..6c94781e377d 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -455,7 +455,7 @@ static const MemoryRegionOps pnv_psi_xscom_ops = {
>       }
>   };
>   
> -static void pnv_psi_reset(void *dev)
> +static void pnv_psi_reset(DeviceState *dev)
>   {
>       PnvPsi *psi = PNV_PSI(dev);
>   
> @@ -464,6 +464,11 @@ static void pnv_psi_reset(void *dev)
>       psi->regs[PSIHB_XSCOM_BAR] = psi->bar | PSIHB_BAR_EN;
>   }
>   
> +static void pnv_psi_reset_handler(void *dev)
> +{
> +    device_reset(DEVICE(dev));
> +}
> +
>   static void pnv_psi_power8_instance_init(Object *obj)
>   {
>       Pnv8Psi *psi8 = PNV8_PSI(obj);
> @@ -533,7 +538,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>               ((uint64_t) i << PSIHB_XIVR_SRC_SH);
>       }
>   
> -    qemu_register_reset(pnv_psi_reset, dev);
> +    qemu_register_reset(pnv_psi_reset_handler, dev);
>   }
>   
>   static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> @@ -816,7 +821,7 @@ static void pnv_psi_power9_irq_set(PnvPsi *psi, int irq, bool state)
>       qemu_set_irq(psi->qirqs[irq], state);
>   }
>   
> -static void pnv_psi_power9_reset(void *dev)
> +static void pnv_psi_power9_reset(DeviceState *dev)
>   {
>       Pnv9Psi *psi = PNV9_PSI(dev);
>   
> @@ -870,7 +875,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
>   
>       pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
>   
> -    qemu_register_reset(pnv_psi_power9_reset, dev);
> +    qemu_register_reset(pnv_psi_reset_handler, dev);
>   }
>   
>   static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> @@ -882,6 +887,7 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>   
>       dc->desc    = "PowerNV PSI Controller POWER9";
>       dc->realize = pnv_psi_power9_realize;
> +    dc->reset   = pnv_psi_power9_reset;
>   
>       ppc->xscom_pcba = PNV9_XSCOM_PSIHB_BASE;
>       ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
> @@ -934,6 +940,7 @@ static void pnv_psi_class_init(ObjectClass *klass, void *data)
>   
>       dc->desc = "PowerNV PSI Controller";
>       dc->props = pnv_psi_properties;
> +    dc->reset = pnv_psi_reset;
>   }
>   
>   static const TypeInfo pnv_psi_info = {
> 
> 

Re: [PATCH 1/2] pnv/psi: Add device reset hook

[David Gibson]

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

On Tue, Jan 07, 2020 at 05:32:40PM +0100, Greg Kurz wrote:
> And call it from a QEMU reset handler. This allows each PNV child class to
> override the reset hook if needed, eg. POWER8 doesn't but POWER9 does.
> The proper way to do that would be to use device_class_set_parent_reset(),
> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_reset
> pointer adds a fair amount of code. Calling pnv_psi_reset() explicitely is
> fine for now.
> 
> A subsequent patch will consolidate the call to qemu_register_reset() in
> a single place.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-5.0, thanks.

> ---
>  hw/ppc/pnv_psi.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 75e20d9da08b..6c94781e377d 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -455,7 +455,7 @@ static const MemoryRegionOps pnv_psi_xscom_ops = {
>      }
>  };
>  
> -static void pnv_psi_reset(void *dev)
> +static void pnv_psi_reset(DeviceState *dev)
>  {
>      PnvPsi *psi = PNV_PSI(dev);
>  
> @@ -464,6 +464,11 @@ static void pnv_psi_reset(void *dev)
>      psi->regs[PSIHB_XSCOM_BAR] = psi->bar | PSIHB_BAR_EN;
>  }
>  
> +static void pnv_psi_reset_handler(void *dev)
> +{
> +    device_reset(DEVICE(dev));
> +}
> +
>  static void pnv_psi_power8_instance_init(Object *obj)
>  {
>      Pnv8Psi *psi8 = PNV8_PSI(obj);
> @@ -533,7 +538,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>              ((uint64_t) i << PSIHB_XIVR_SRC_SH);
>      }
>  
> -    qemu_register_reset(pnv_psi_reset, dev);
> +    qemu_register_reset(pnv_psi_reset_handler, dev);
>  }
>  
>  static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> @@ -816,7 +821,7 @@ static void pnv_psi_power9_irq_set(PnvPsi *psi, int irq, bool state)
>      qemu_set_irq(psi->qirqs[irq], state);
>  }
>  
> -static void pnv_psi_power9_reset(void *dev)
> +static void pnv_psi_power9_reset(DeviceState *dev)
>  {
>      Pnv9Psi *psi = PNV9_PSI(dev);
>  
> @@ -870,7 +875,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
>  
>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
>  
> -    qemu_register_reset(pnv_psi_power9_reset, dev);
> +    qemu_register_reset(pnv_psi_reset_handler, dev);
>  }
>  
>  static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> @@ -882,6 +887,7 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc    = "PowerNV PSI Controller POWER9";
>      dc->realize = pnv_psi_power9_realize;
> +    dc->reset   = pnv_psi_power9_reset;
>  
>      ppc->xscom_pcba = PNV9_XSCOM_PSIHB_BASE;
>      ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
> @@ -934,6 +940,7 @@ static void pnv_psi_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc = "PowerNV PSI Controller";
>      dc->props = pnv_psi_properties;
> +    dc->reset = pnv_psi_reset;
>  }
>  
>  static const TypeInfo pnv_psi_info = {
> 

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

[PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[Greg Kurz]

The proper way to do that would be to use device_class_set_parent_realize(),
but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
is fine for now.

This should probably be achieved with a device realize hook in the
PSI base class and device_class_set_parent_realize() in the children
classes.

Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_psi.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 6c94781e377d..546232106756 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
     device_reset(DEVICE(dev));
 }
 
+static void pnv_psi_realize(DeviceState *dev, Error **errp)
+{
+    PnvPsi *psi = PNV_PSI(dev);
+
+    /* Default BAR for MMIO region */
+    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
+
+    qemu_register_reset(pnv_psi_reset_handler, dev);
+}
+
 static void pnv_psi_power8_instance_init(Object *obj)
 {
     Pnv8Psi *psi8 = PNV8_PSI(obj);
@@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
                           "psihb", PNV_PSIHB_SIZE);
 
-    /* Default BAR for MMIO region */
-    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
-
     /* Default sources in XIVR */
     for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
         uint8_t xivr = irq_to_xivr[i];
@@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
             ((uint64_t) i << PSIHB_XIVR_SRC_SH);
     }
 
-    qemu_register_reset(pnv_psi_reset_handler, dev);
+    pnv_psi_realize(dev, errp);
 }
 
 static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
@@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
                           "psihb", PNV9_PSIHB_SIZE);
 
-    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
-
-    qemu_register_reset(pnv_psi_reset_handler, dev);
+    pnv_psi_realize(dev, errp);
 }
 
 static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)

Re: [PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[Philippe Mathieu-Daudé]

Hi Greg,

On 1/7/20 5:32 PM, Greg Kurz wrote:
> The proper way to do that would be to use device_class_set_parent_realize(),
> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> is fine for now.
> 
> This should probably be achieved with a device realize hook in the
> PSI base class and device_class_set_parent_realize() in the children
> classes.

Can you add a note explaining why the POWER10 PSI doesn't need it?

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/pnv_psi.c |   19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 6c94781e377d..546232106756 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
>       device_reset(DEVICE(dev));
>   }
>   
> +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvPsi *psi = PNV_PSI(dev);
> +
> +    /* Default BAR for MMIO region */
> +    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> +
> +    qemu_register_reset(pnv_psi_reset_handler, dev);
> +}
> +
>   static void pnv_psi_power8_instance_init(Object *obj)
>   {
>       Pnv8Psi *psi8 = PNV8_PSI(obj);
> @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>                             "psihb", PNV_PSIHB_SIZE);
>   
> -    /* Default BAR for MMIO region */
> -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
>       /* Default sources in XIVR */
>       for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
>           uint8_t xivr = irq_to_xivr[i];
> @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>               ((uint64_t) i << PSIHB_XIVR_SRC_SH);
>       }
>   
> -    qemu_register_reset(pnv_psi_reset_handler, dev);
> +    pnv_psi_realize(dev, errp);
>   }
>   
>   static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
>       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
>                             "psihb", PNV9_PSIHB_SIZE);
>   
> -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
> -    qemu_register_reset(pnv_psi_reset_handler, dev);
> +    pnv_psi_realize(dev, errp);
>   }
>   
>   static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> 
> 

Re: [PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[David Gibson]

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

On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Greg,
> 
> On 1/7/20 5:32 PM, Greg Kurz wrote:
> > The proper way to do that would be to use device_class_set_parent_realize(),
> > but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> > pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> > is fine for now.
> > 
> > This should probably be achieved with a device realize hook in the
> > PSI base class and device_class_set_parent_realize() in the children
> > classes.
> 
> Can you add a note explaining why the POWER10 PSI doesn't need it?

For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
doesn't arise.

> 
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >   hw/ppc/pnv_psi.c |   19 ++++++++++++-------
> >   1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > index 6c94781e377d..546232106756 100644
> > --- a/hw/ppc/pnv_psi.c
> > +++ b/hw/ppc/pnv_psi.c
> > @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> >       device_reset(DEVICE(dev));
> >   }
> > +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> > +{
> > +    PnvPsi *psi = PNV_PSI(dev);
> > +
> > +    /* Default BAR for MMIO region */
> > +    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > +
> > +    qemu_register_reset(pnv_psi_reset_handler, dev);
> > +}
> > +
> >   static void pnv_psi_power8_instance_init(Object *obj)
> >   {
> >       Pnv8Psi *psi8 = PNV8_PSI(obj);
> > @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> >       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> >                             "psihb", PNV_PSIHB_SIZE);
> > -    /* Default BAR for MMIO region */
> > -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > -
> >       /* Default sources in XIVR */
> >       for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> >           uint8_t xivr = irq_to_xivr[i];
> > @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> >               ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> >       }
> > -    qemu_register_reset(pnv_psi_reset_handler, dev);
> > +    pnv_psi_realize(dev, errp);
> >   }
> >   static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> > @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> >       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> >                             "psihb", PNV9_PSIHB_SIZE);
> > -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > -
> > -    qemu_register_reset(pnv_psi_reset_handler, dev);
> > +    pnv_psi_realize(dev, errp);
> >   }
> >   static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> > 
> > 
> 

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

Re: [PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[Greg Kurz]

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

On Wed, 8 Jan 2020 11:54:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Greg,
> > 
> > On 1/7/20 5:32 PM, Greg Kurz wrote:
> > > The proper way to do that would be to use device_class_set_parent_realize(),
> > > but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> > > pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> > > is fine for now.
> > > 
> > > This should probably be achieved with a device realize hook in the
> > > PSI base class and device_class_set_parent_realize() in the children
> > > classes.
> > 

I realize that this last paragraph is a leftover. First paragraph already
mentions device_class_set_parent_realize() as being the "proper way".

David,

Can you remove it in your tree ? No big deal if you can't.

> > Can you add a note explaining why the POWER10 PSI doesn't need it?
> 
> For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
> doesn't arise.
> 

This is correct and also a bit confusing, as proves Philippe's remark.
Maybe we should come up with a PnvXivePsiClass and specialize it for
POWER9 and POWER10.

> > 
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > ---
> > >   hw/ppc/pnv_psi.c |   19 ++++++++++++-------
> > >   1 file changed, 12 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > > index 6c94781e377d..546232106756 100644
> > > --- a/hw/ppc/pnv_psi.c
> > > +++ b/hw/ppc/pnv_psi.c
> > > @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> > >       device_reset(DEVICE(dev));
> > >   }
> > > +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    PnvPsi *psi = PNV_PSI(dev);
> > > +
> > > +    /* Default BAR for MMIO region */
> > > +    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > +
> > > +    qemu_register_reset(pnv_psi_reset_handler, dev);
> > > +}
> > > +
> > >   static void pnv_psi_power8_instance_init(Object *obj)
> > >   {
> > >       Pnv8Psi *psi8 = PNV8_PSI(obj);
> > > @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > >       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> > >                             "psihb", PNV_PSIHB_SIZE);
> > > -    /* Default BAR for MMIO region */
> > > -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > -
> > >       /* Default sources in XIVR */
> > >       for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> > >           uint8_t xivr = irq_to_xivr[i];
> > > @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > >               ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> > >       }
> > > -    qemu_register_reset(pnv_psi_reset_handler, dev);
> > > +    pnv_psi_realize(dev, errp);
> > >   }
> > >   static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> > > @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> > >       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> > >                             "psihb", PNV9_PSIHB_SIZE);
> > > -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > -
> > > -    qemu_register_reset(pnv_psi_reset_handler, dev);
> > > +    pnv_psi_realize(dev, errp);
> > >   }
> > >   static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> > > 
> > > 
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[Cédric Le Goater]

On 1/8/20 11:58 AM, Greg Kurz wrote:
> On Wed, 8 Jan 2020 11:54:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
>>> Hi Greg,
>>>
>>> On 1/7/20 5:32 PM, Greg Kurz wrote:
>>>> The proper way to do that would be to use device_class_set_parent_realize(),
>>>> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
>>>> pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
>>>> is fine for now.
>>>>
>>>> This should probably be achieved with a device realize hook in the
>>>> PSI base class and device_class_set_parent_realize() in the children
>>>> classes.
>>>
> 
> I realize that this last paragraph is a leftover. First paragraph already
> mentions device_class_set_parent_realize() as being the "proper way".
> 
> David,
> 
> Can you remove it in your tree ? No big deal if you can't.
> 
>>> Can you add a note explaining why the POWER10 PSI doesn't need it?
>>
>> For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
>> doesn't arise.
>>
> 
> This is correct and also a bit confusing, as proves Philippe's remark.
> Maybe we should come up with a PnvXivePsiClass and specialize it for
> POWER9 and POWER10.

Yes. I think this is the way to go. 

P8 has a PSI device using the XICS interrupt interface. P9 and P10 use 
the XIVE interface.

C.

Re: [PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[David Gibson]

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

On Wed, Jan 08, 2020 at 11:58:45AM +0100, Greg Kurz wrote:
> On Wed, 8 Jan 2020 11:54:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jan 07, 2020 at 07:32:03PM +0100, Philippe Mathieu-Daudé wrote:
> > > Hi Greg,
> > > 
> > > On 1/7/20 5:32 PM, Greg Kurz wrote:
> > > > The proper way to do that would be to use device_class_set_parent_realize(),
> > > > but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> > > > pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> > > > is fine for now.
> > > > 
> > > > This should probably be achieved with a device realize hook in the
> > > > PSI base class and device_class_set_parent_realize() in the children
> > > > classes.
> > > 
> 
> I realize that this last paragraph is a leftover. First paragraph already
> mentions device_class_set_parent_realize() as being the "proper way".
> 
> David,
> 
> Can you remove it in your tree ? No big deal if you can't.

Sorry, not any more - I've already sent the PR.

> 
> > > Can you add a note explaining why the POWER10 PSI doesn't need it?
> > 
> > For now, POWER10 uses the Pnv9PsiClass, I believe, so the question
> > doesn't arise.
> > 
> 
> This is correct and also a bit confusing, as proves Philippe's remark.
> Maybe we should come up with a PnvXivePsiClass and specialize it for
> POWER9 and POWER10.
> 
> > > 
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >   hw/ppc/pnv_psi.c |   19 ++++++++++++-------
> > > >   1 file changed, 12 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > > > index 6c94781e377d..546232106756 100644
> > > > --- a/hw/ppc/pnv_psi.c
> > > > +++ b/hw/ppc/pnv_psi.c
> > > > @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
> > > >       device_reset(DEVICE(dev));
> > > >   }
> > > > +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    PnvPsi *psi = PNV_PSI(dev);
> > > > +
> > > > +    /* Default BAR for MMIO region */
> > > > +    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > > +
> > > > +    qemu_register_reset(pnv_psi_reset_handler, dev);
> > > > +}
> > > > +
> > > >   static void pnv_psi_power8_instance_init(Object *obj)
> > > >   {
> > > >       Pnv8Psi *psi8 = PNV8_PSI(obj);
> > > > @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > > >       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> > > >                             "psihb", PNV_PSIHB_SIZE);
> > > > -    /* Default BAR for MMIO region */
> > > > -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > > -
> > > >       /* Default sources in XIVR */
> > > >       for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
> > > >           uint8_t xivr = irq_to_xivr[i];
> > > > @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
> > > >               ((uint64_t) i << PSIHB_XIVR_SRC_SH);
> > > >       }
> > > > -    qemu_register_reset(pnv_psi_reset_handler, dev);
> > > > +    pnv_psi_realize(dev, errp);
> > > >   }
> > > >   static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> > > > @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
> > > >       memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
> > > >                             "psihb", PNV9_PSIHB_SIZE);
> > > > -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> > > > -
> > > > -    qemu_register_reset(pnv_psi_reset_handler, dev);
> > > > +    pnv_psi_realize(dev, errp);
> > > >   }
> > > >   static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> > > > 
> > > > 
> > > 
> > 
> 



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

Re: [PATCH 2/2] pnv/psi: Consolidate some duplicated code in pnv_psi_realize()

[David Gibson]

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

On Tue, Jan 07, 2020 at 05:32:46PM +0100, Greg Kurz wrote:
> The proper way to do that would be to use device_class_set_parent_realize(),
> but defining a Pnv8PsiClass and a Pnv9PsiClass types with a parent_realize
> pointer adds a fair amount of code. Calling pnv_psi_realize() explicitely
> is fine for now.
> 
> This should probably be achieved with a device realize hook in the
> PSI base class and device_class_set_parent_realize() in the children
> classes.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-5.0.

> ---
>  hw/ppc/pnv_psi.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 6c94781e377d..546232106756 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -469,6 +469,16 @@ static void pnv_psi_reset_handler(void *dev)
>      device_reset(DEVICE(dev));
>  }
>  
> +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvPsi *psi = PNV_PSI(dev);
> +
> +    /* Default BAR for MMIO region */
> +    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> +
> +    qemu_register_reset(pnv_psi_reset_handler, dev);
> +}
> +
>  static void pnv_psi_power8_instance_init(Object *obj)
>  {
>      Pnv8Psi *psi8 = PNV8_PSI(obj);
> @@ -528,9 +538,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>                            "psihb", PNV_PSIHB_SIZE);
>  
> -    /* Default BAR for MMIO region */
> -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
>      /* Default sources in XIVR */
>      for (i = 0; i < PSI_NUM_INTERRUPTS; i++) {
>          uint8_t xivr = irq_to_xivr[i];
> @@ -538,7 +545,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>              ((uint64_t) i << PSIHB_XIVR_SRC_SH);
>      }
>  
> -    qemu_register_reset(pnv_psi_reset_handler, dev);
> +    pnv_psi_realize(dev, errp);
>  }
>  
>  static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> @@ -873,9 +880,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &pnv_psi_p9_mmio_ops, psi,
>                            "psihb", PNV9_PSIHB_SIZE);
>  
> -    pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> -
> -    qemu_register_reset(pnv_psi_reset_handler, dev);
> +    pnv_psi_realize(dev, errp);
>  }
>  
>  static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
> 

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