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 <c...@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,
>
