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

[David Gibson]

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 <address@hidden>
> >> ---
> >>  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("address@hidden", 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

signature.asc
Description: PGP signature