1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v2 0/8] pnv: improvement of LPC support and IPMI support
  4. View patch

[Qemu-devel] [PATCH v2 2/8] ppc/pnv: enable only one LPC bus

Cédric Le Goater posted 8 patches 8 years, 10 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 2/8] ppc/pnv: enable only one LPC bus
Posted by Cédric Le Goater 8 years, 10 months ago 
The firmware (skiboot) chooses the default LPC bus of a multichip
systems using a "primary" property. The LPC bus of chip 0 should be
the only connected in the system. Let's advertise it in the device
tree.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 Changes since v1:

 - the device tree is populated for all LPC busses of the system but
   only the one on chip 0 has the "primary" property.

 hw/ppc/pnv.c             |  2 ++
 hw/ppc/pnv_lpc.c         | 23 ++++++++++++++---------
 include/hw/ppc/pnv_lpc.h |  2 ++
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 27589b91d1cf..7d742b6e34e1 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -765,6 +765,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
     g_free(typename);
 
     /* Create LPC controller */
+    object_property_set_int(OBJECT(&chip->lpc), chip->chip_id, "chip-id",
+                            &error_fatal);
     object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
                              &error_fatal);
     pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index baee366d386a..13d7a695678d 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -92,14 +92,6 @@ enum {
 #define LPC_HC_REGS_OPB_SIZE    0x00001000
 
 
-/*
- * TODO: the "primary" cell should only be added on chip 0. This is
- * how skiboot chooses the default LPC controller on multichip
- * systems.
- *
- * It would be easly done if we can change the populate() interface to
- * replace the PnvXScomInterface parameter by a PnvChip one
- */
 static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
 {
     const char compat[] = "ibm,power8-lpc\0ibm,lpc";
@@ -110,6 +102,7 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
         cpu_to_be32(lpc_pcba),
         cpu_to_be32(PNV_XSCOM_LPC_SIZE)
     };
+    PnvLpcController *lpc = PNV_LPC(dev);
 
     name = g_strdup_printf("isa@%x", lpc_pcba);
     offset = fdt_add_subnode(fdt, xscom_offset, name);
@@ -119,7 +112,13 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
     _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
     _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
     _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
-    _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
+
+    /* The firmware (skiboot) chooses the default LPC bus of the
+     * system using a "primary" property.
+     */
+    if (lpc->chip_id == 0x0) {
+        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
+    }
     _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
     return 0;
 }
@@ -486,6 +485,11 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
     lpc->psi = PNV_PSI(obj);
 }
 
+static Property pnv_lpc_properties[] = {
+    DEFINE_PROP_UINT32("chip-id", PnvLpcController, chip_id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pnv_lpc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -494,6 +498,7 @@ static void pnv_lpc_class_init(ObjectClass *klass, void *data)
     xdc->populate = pnv_lpc_populate;
 
     dc->realize = pnv_lpc_realize;
+    dc->props = pnv_lpc_properties;
 }
 
 static const TypeInfo pnv_lpc_info = {
diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index ccf969af9448..c78ee4a98c62 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -67,6 +67,8 @@ typedef struct PnvLpcController {
 
     /* PSI to generate interrupts */
     PnvPsi *psi;
+
+    uint32_t chip_id;
 } PnvLpcController;
 
 qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
-- 
2.7.4
Re: [Qemu-devel] [PATCH v2 2/8] ppc/pnv: enable only one LPC bus
Posted by David Gibson 8 years, 10 months ago 
On Mon, Apr 10, 2017 at 03:56:52PM +0200, Cédric Le Goater wrote:
> The firmware (skiboot) chooses the default LPC bus of a multichip
> systems using a "primary" property. The LPC bus of chip 0 should be
> the only connected in the system. Let's advertise it in the device
> tree.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  Changes since v1:
> 
>  - the device tree is populated for all LPC busses of the system but
>    only the one on chip 0 has the "primary" property.
> 
>  hw/ppc/pnv.c             |  2 ++
>  hw/ppc/pnv_lpc.c         | 23 ++++++++++++++---------
>  include/hw/ppc/pnv_lpc.h |  2 ++
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 27589b91d1cf..7d742b6e34e1 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -765,6 +765,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>      g_free(typename);
>  
>      /* Create LPC controller */
> +    object_property_set_int(OBJECT(&chip->lpc), chip->chip_id, "chip-id",
> +                            &error_fatal);
>      object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
>                               &error_fatal);
>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index baee366d386a..13d7a695678d 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -92,14 +92,6 @@ enum {
>  #define LPC_HC_REGS_OPB_SIZE    0x00001000
>  
>  
> -/*
> - * TODO: the "primary" cell should only be added on chip 0. This is
> - * how skiboot chooses the default LPC controller on multichip
> - * systems.
> - *
> - * It would be easly done if we can change the populate() interface to
> - * replace the PnvXScomInterface parameter by a PnvChip one
> - */
>  static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>  {
>      const char compat[] = "ibm,power8-lpc\0ibm,lpc";
> @@ -110,6 +102,7 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>          cpu_to_be32(lpc_pcba),
>          cpu_to_be32(PNV_XSCOM_LPC_SIZE)
>      };
> +    PnvLpcController *lpc = PNV_LPC(dev);
>  
>      name = g_strdup_printf("isa@%x", lpc_pcba);
>      offset = fdt_add_subnode(fdt, xscom_offset, name);
> @@ -119,7 +112,13 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>      _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
>      _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
>      _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
> -    _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
> +
> +    /* The firmware (skiboot) chooses the default LPC bus of the
> +     * system using a "primary" property.
> +     */
> +    if (lpc->chip_id == 0x0) {
> +        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
> +    }

So, the choice of primary bus is really a machine level thing, rather
than chip level.

So I think it would make more sense for the machine to poke the
'primary' property into the device tree afterwards, rather than adding
it initially within the chip/LPC code.  That will then avoid having to
pass the chip-id property into the LPC.

>      _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
>      return 0;
>  }
> @@ -486,6 +485,11 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      lpc->psi = PNV_PSI(obj);
>  }
>  
> +static Property pnv_lpc_properties[] = {
> +    DEFINE_PROP_UINT32("chip-id", PnvLpcController, chip_id, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -494,6 +498,7 @@ static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>      xdc->populate = pnv_lpc_populate;
>  
>      dc->realize = pnv_lpc_realize;
> +    dc->props = pnv_lpc_properties;
>  }
>  
>  static const TypeInfo pnv_lpc_info = {
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index ccf969af9448..c78ee4a98c62 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -67,6 +67,8 @@ typedef struct PnvLpcController {
>  
>      /* PSI to generate interrupts */
>      PnvPsi *psi;
> +
> +    uint32_t chip_id;
>  } PnvLpcController;
>  
>  qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,

-- 
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
Re: [Qemu-devel] [PATCH v2 2/8] ppc/pnv: enable only one LPC bus
Posted by Cédric Le Goater 8 years, 10 months ago 
On 04/11/2017 04:40 AM, David Gibson wrote:
> On Mon, Apr 10, 2017 at 03:56:52PM +0200, Cédric Le Goater wrote:
>> The firmware (skiboot) chooses the default LPC bus of a multichip
>> systems using a "primary" property. The LPC bus of chip 0 should be
>> the only connected in the system. Let's advertise it in the device
>> tree.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  Changes since v1:
>>
>>  - the device tree is populated for all LPC busses of the system but
>>    only the one on chip 0 has the "primary" property.
>>
>>  hw/ppc/pnv.c             |  2 ++
>>  hw/ppc/pnv_lpc.c         | 23 ++++++++++++++---------
>>  include/hw/ppc/pnv_lpc.h |  2 ++
>>  3 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 27589b91d1cf..7d742b6e34e1 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -765,6 +765,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>>      g_free(typename);
>>  
>>      /* Create LPC controller */
>> +    object_property_set_int(OBJECT(&chip->lpc), chip->chip_id, "chip-id",
>> +                            &error_fatal);
>>      object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
>>                               &error_fatal);
>>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
>> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
>> index baee366d386a..13d7a695678d 100644
>> --- a/hw/ppc/pnv_lpc.c
>> +++ b/hw/ppc/pnv_lpc.c
>> @@ -92,14 +92,6 @@ enum {
>>  #define LPC_HC_REGS_OPB_SIZE    0x00001000
>>  
>>  
>> -/*
>> - * TODO: the "primary" cell should only be added on chip 0. This is
>> - * how skiboot chooses the default LPC controller on multichip
>> - * systems.
>> - *
>> - * It would be easly done if we can change the populate() interface to
>> - * replace the PnvXScomInterface parameter by a PnvChip one
>> - */
>>  static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>>  {
>>      const char compat[] = "ibm,power8-lpc\0ibm,lpc";
>> @@ -110,6 +102,7 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>>          cpu_to_be32(lpc_pcba),
>>          cpu_to_be32(PNV_XSCOM_LPC_SIZE)
>>      };
>> +    PnvLpcController *lpc = PNV_LPC(dev);
>>  
>>      name = g_strdup_printf("isa@%x", lpc_pcba);
>>      offset = fdt_add_subnode(fdt, xscom_offset, name);
>> @@ -119,7 +112,13 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>>      _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
>>      _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
>>      _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
>> -    _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
>> +
>> +    /* The firmware (skiboot) chooses the default LPC bus of the
>> +     * system using a "primary" property.
>> +     */
>> +    if (lpc->chip_id == 0x0) {
>> +        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
>> +    }
> 
> So, the choice of primary bus is really a machine level thing, rather
> than chip level.
> 
> So I think it would make more sense for the machine to poke the
> 'primary' property into the device tree afterwards, rather than adding
> it initially within the chip/LPC code.  That will then avoid having to
> pass the chip-id property into the LPC.

ok. I will see how I can do that. I think that I will change 
pnv_xscom_populate() to return the xscom offset in the device
tree. From this node, I can look for the ISA bus and add the 
"primary" property if this is chip 0.

Thanks,

C. 

> 
>>      _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
>>      return 0;
>>  }
>> @@ -486,6 +485,11 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>>      lpc->psi = PNV_PSI(obj);
>>  }
>>  
>> +static Property pnv_lpc_properties[] = {
>> +    DEFINE_PROP_UINT32("chip-id", PnvLpcController, chip_id, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -494,6 +498,7 @@ static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>>      xdc->populate = pnv_lpc_populate;
>>  
>>      dc->realize = pnv_lpc_realize;
>> +    dc->props = pnv_lpc_properties;
>>  }
>>  
>>  static const TypeInfo pnv_lpc_info = {
>> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
>> index ccf969af9448..c78ee4a98c62 100644
>> --- a/include/hw/ppc/pnv_lpc.h
>> +++ b/include/hw/ppc/pnv_lpc.h
>> @@ -67,6 +67,8 @@ typedef struct PnvLpcController {
>>  
>>      /* PSI to generate interrupts */
>>      PnvPsi *psi;
>> +
>> +    uint32_t chip_id;
>>  } PnvLpcController;
>>  
>>  qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
>
Re: [Qemu-devel] [PATCH v2 2/8] ppc/pnv: enable only one LPC bus
Posted by David Gibson 8 years, 10 months ago 
On Tue, Apr 11, 2017 at 09:06:16AM +0200, Cédric Le Goater wrote:
> On 04/11/2017 04:40 AM, David Gibson wrote:
> > On Mon, Apr 10, 2017 at 03:56:52PM +0200, Cédric Le Goater wrote:
> >> The firmware (skiboot) chooses the default LPC bus of a multichip
> >> systems using a "primary" property. The LPC bus of chip 0 should be
> >> the only connected in the system. Let's advertise it in the device
> >> tree.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  Changes since v1:
> >>
> >>  - the device tree is populated for all LPC busses of the system but
> >>    only the one on chip 0 has the "primary" property.
> >>
> >>  hw/ppc/pnv.c             |  2 ++
> >>  hw/ppc/pnv_lpc.c         | 23 ++++++++++++++---------
> >>  include/hw/ppc/pnv_lpc.h |  2 ++
> >>  3 files changed, 18 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 27589b91d1cf..7d742b6e34e1 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -765,6 +765,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >>      g_free(typename);
> >>  
> >>      /* Create LPC controller */
> >> +    object_property_set_int(OBJECT(&chip->lpc), chip->chip_id, "chip-id",
> >> +                            &error_fatal);
> >>      object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
> >>                               &error_fatal);
> >>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
> >> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> >> index baee366d386a..13d7a695678d 100644
> >> --- a/hw/ppc/pnv_lpc.c
> >> +++ b/hw/ppc/pnv_lpc.c
> >> @@ -92,14 +92,6 @@ enum {
> >>  #define LPC_HC_REGS_OPB_SIZE    0x00001000
> >>  
> >>  
> >> -/*
> >> - * TODO: the "primary" cell should only be added on chip 0. This is
> >> - * how skiboot chooses the default LPC controller on multichip
> >> - * systems.
> >> - *
> >> - * It would be easly done if we can change the populate() interface to
> >> - * replace the PnvXScomInterface parameter by a PnvChip one
> >> - */
> >>  static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> >>  {
> >>      const char compat[] = "ibm,power8-lpc\0ibm,lpc";
> >> @@ -110,6 +102,7 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> >>          cpu_to_be32(lpc_pcba),
> >>          cpu_to_be32(PNV_XSCOM_LPC_SIZE)
> >>      };
> >> +    PnvLpcController *lpc = PNV_LPC(dev);
> >>  
> >>      name = g_strdup_printf("isa@%x", lpc_pcba);
> >>      offset = fdt_add_subnode(fdt, xscom_offset, name);
> >> @@ -119,7 +112,13 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
> >>      _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
> >>      _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
> >>      _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
> >> -    _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
> >> +
> >> +    /* The firmware (skiboot) chooses the default LPC bus of the
> >> +     * system using a "primary" property.
> >> +     */
> >> +    if (lpc->chip_id == 0x0) {
> >> +        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
> >> +    }
> > 
> > So, the choice of primary bus is really a machine level thing, rather
> > than chip level.
> > 
> > So I think it would make more sense for the machine to poke the
> > 'primary' property into the device tree afterwards, rather than adding
> > it initially within the chip/LPC code.  That will then avoid having to
> > pass the chip-id property into the LPC.
> 
> ok. I will see how I can do that. I think that I will change 
> pnv_xscom_populate() to return the xscom offset in the device
> tree. From this node, I can look for the ISA bus and add the 
> "primary" property if this is chip 0.

I wouldn't even bother storing the offset, which may not be safe
anyway.  You can just look it up again by path from the machine.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.