| Message ID | 1491396106-26376-5-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 05, 2017 at 02:41:29PM +0200, Cédric Le Goater wrote: > Multi chip systems only have one LPC bus, on chip 0. The PnvLPC object > will still be created under the PnvChip objects but only the one under > chip 0 will be advertise in the device tree. > > Also remove the comment which is slightly wrong. Only chip 0 has a LPC > device node : xscom@3fc0000000000/isa@b0020 > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> This seems a very round about way of accomplishing the goal. Wouldn't it make more sense for the chip to only construct (or only realize) the LPC if it's chip zero, rather than passing the chip id through to the lpc object. > --- > hw/ppc/pnv.c | 2 ++ > hw/ppc/pnv_lpc.c | 20 ++++++++++++-------- > include/hw/ppc/pnv_lpc.h | 2 ++ > 3 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 78133e5d20e1..493c7eed7980 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -811,6 +811,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 20cbb6a0dbbd..1a212a2a399f 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,12 @@ 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); > + > + /* Only populate one LPC bus per system, the one on chip 0.*/ > + if (lpc->chip_id) { > + return 0; > + } > > name = g_strdup_printf("isa@%x", lpc_pcba); > offset = fdt_add_subnode(fdt, xscom_offset, name); > @@ -486,6 +484,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 +497,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 53040026c37b..dcfadda90090 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; > > #define LPC_HC_IRQ_SERIRQ0 0x80000000 /* all bits down to ... */
On 04/06/2017 06:23 AM, David Gibson wrote: > On Wed, Apr 05, 2017 at 02:41:29PM +0200, Cédric Le Goater wrote: >> Multi chip systems only have one LPC bus, on chip 0. The PnvLPC object >> will still be created under the PnvChip objects but only the one under >> chip 0 will be advertise in the device tree. >> >> Also remove the comment which is slightly wrong. Only chip 0 has a LPC >> device node : xscom@3fc0000000000/isa@b0020 >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > This seems a very round about way of accomplishing the goal. Wouldn't > it make more sense for the chip to only construct (or only realize) > the LPC if it's chip zero, rather than passing the chip id through to > the lpc object. hmm, yes. I can do better on this. The object will be initialized which raises some concern because we don't have the chip id at the moment but the object is still valid in some way. I think I need to remove it from the list of children of the chip or use a pointer instead. Thanks, C. > >> --- >> hw/ppc/pnv.c | 2 ++ >> hw/ppc/pnv_lpc.c | 20 ++++++++++++-------- >> include/hw/ppc/pnv_lpc.h | 2 ++ >> 3 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index 78133e5d20e1..493c7eed7980 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -811,6 +811,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 20cbb6a0dbbd..1a212a2a399f 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,12 @@ 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); >> + >> + /* Only populate one LPC bus per system, the one on chip 0.*/ >> + if (lpc->chip_id) { >> + return 0; >> + } >> >> name = g_strdup_printf("isa@%x", lpc_pcba); >> offset = fdt_add_subnode(fdt, xscom_offset, name); >> @@ -486,6 +484,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 +497,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 53040026c37b..dcfadda90090 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; >> >> #define LPC_HC_IRQ_SERIRQ0 0x80000000 /* all bits down to ... */ >
On Thu, 2017-04-06 at 11:06 +0200, Cédric Le Goater wrote: > The object will be initialized which raises some concern because we don't > have the chip id at the moment but the object is still valid in some way. > I think I need to remove it from the list of children of the chip or use > a pointer instead. Well... so it exists on all chips. All chips have an LPC controller... Whether firmware enables it and whether it's connected to anything is a system issue. Cheers, Ben.
On 04/06/2017 11:16 AM, Benjamin Herrenschmidt wrote: > On Thu, 2017-04-06 at 11:06 +0200, Cédric Le Goater wrote: >> The object will be initialized which raises some concern because we don't >> have the chip id at the moment but the object is still valid in some way. >> I think I need to remove it from the list of children of the chip or use >> a pointer instead. > > Well... so it exists on all chips. All chips have an LPC controller... > > Whether firmware enables it and whether it's connected to anything is a > system issue. So, looking at hostboot, the lower level firmware, I think the initial patch is more in sync with it : https://github.com/open-power/hostboot/blob/master-p8/src/usr/devtree/bld_devtree.C#L1038 David, can we keep it as it is ? I will change the commit log which is a bit fuzzy. Cheers, C.
On Thu, 2017-04-06 at 13:50 +0200, Cédric Le Goater wrote: > > So, looking at hostboot, the lower level firmware, I think the > initial > patch is more in sync with it : > > https://github.com/open-power/hostboot/blob/master-p8/src/usr/devt > ree/bld_devtree.C#L1038 > > David, can we keep it as it is ? I will change the commit log which > is a bit fuzzy. Note that OPAL should be happy to have multiple LPCs in the DT as long as one of them has the "primary" property in the node to tell it that's where the UART etc... are. Cheers, Ben.
On 04/06/2017 02:01 PM, Benjamin Herrenschmidt wrote: > On Thu, 2017-04-06 at 13:50 +0200, Cédric Le Goater wrote: >> >> So, looking at hostboot, the lower level firmware, I think the >> initial >> patch is more in sync with it : >> >> https://github.com/open-power/hostboot/blob/master-p8/src/usr/devt >> ree/bld_devtree.C#L1038 >> >> David, can we keep it as it is ? I will change the commit log which >> is a bit fuzzy. > > Note that OPAL should be happy to have multiple LPCs in the DT as long > as one of them has the "primary" property in the node to tell it that's > where the UART etc... are. yes, that works also : [ 0.024439731,5] LPC: LPC[000]: Initialized, access via XSCOM @0xb0020 [ 0.024499074,5] LPC: LPC[001]: Initialized, access via XSCOM @0xb0020 [ 0.024517962,5] LPC: LPC: Default bus on chip 0x0 but real HW (2 sockets OpenPOWER systems, garrison and firestone) does not expose the LPC bus on the second chip, should we do so in qemu ? Thanks, C.
On Thu, 2017-04-06 at 14:35 +0200, Cédric Le Goater wrote: > but real HW (2 sockets OpenPOWER systems, garrison and firestone) > does > not expose the LPC bus on the second chip, should we do so in qemu ? It's not so much HW as it it HostBoot. Not a huge deal. Cheers, Ben.
On 04/06/2017 11:53 PM, Benjamin Herrenschmidt wrote: > On Thu, 2017-04-06 at 14:35 +0200, Cédric Le Goater wrote: >> but real HW (2 sockets OpenPOWER systems, garrison and firestone) >> does >> not expose the LPC bus on the second chip, should we do so in qemu ? > > It's not so much HW as it it HostBoot. Not a huge deal. So let's have QEMU populate the second LPC bus without the "primary" prop then. It's a different approach than hostboot, but it's good for testing anyhow. Thanks, C.
On Fri, Apr 07, 2017 at 08:14:36AM +0200, Cédric Le Goater wrote: > On 04/06/2017 11:53 PM, Benjamin Herrenschmidt wrote: > > On Thu, 2017-04-06 at 14:35 +0200, Cédric Le Goater wrote: > >> but real HW (2 sockets OpenPOWER systems, garrison and firestone) > >> does > >> not expose the LPC bus on the second chip, should we do so in qemu ? > > > > It's not so much HW as it it HostBoot. Not a huge deal. > > So let's have QEMU populate the second LPC bus without the "primary" > prop then. It's a different approach than hostboot, but it's good for > testing anyhow. Works for me.
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 78133e5d20e1..493c7eed7980 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -811,6 +811,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 20cbb6a0dbbd..1a212a2a399f 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,12 @@ 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); + + /* Only populate one LPC bus per system, the one on chip 0.*/ + if (lpc->chip_id) { + return 0; + } name = g_strdup_printf("isa@%x", lpc_pcba); offset = fdt_add_subnode(fdt, xscom_offset, name); @@ -486,6 +484,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 +497,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 53040026c37b..dcfadda90090 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; #define LPC_HC_IRQ_SERIRQ0 0x80000000 /* all bits down to ... */
Multi chip systems only have one LPC bus, on chip 0. The PnvLPC object will still be created under the PnvChip objects but only the one under chip 0 will be advertise in the device tree. Also remove the comment which is slightly wrong. Only chip 0 has a LPC device node : xscom@3fc0000000000/isa@b0020 Signed-off-by: Cédric Le Goater <clg@kaod.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- hw/ppc/pnv.c | 2 ++ hw/ppc/pnv_lpc.c | 20 ++++++++++++-------- include/hw/ppc/pnv_lpc.h | 2 ++ 3 files changed, 16 insertions(+), 8 deletions(-)