| Message ID | 1491832618-27536-3-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
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,
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, >
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.
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,
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(-)