Re: [PATCH] ppc/pnv: Fix number of I2C engines and ports for power9/10

[Miles Glenn]

On Tue, 2023-10-24 at 07:47 +0200, Cédric Le Goater wrote:
> Hello Glenn,
> 
> On 10/23/23 18:52, Glenn Miles wrote:
> > Power9 is supposed to have 4 PIB-connected I2C engines with the
> > following number of ports on each engine:
> > 
> >      0: 2
> >      1: 13
> >      2: 2
> >      3: 2
> > 
> > Power10 also has 4 engines but has the following number of ports
> > on each engine:
> > 
> >      0: 14
> >      1: 14
> >      2: 2
> >      3: 16
> > 
> > Current code assumes that they all have the same (maximum) number.
> > This can be a problem if software expects to see a certain number
> > of ports present (Power Hypervisor seems to care).
> 
> Nice ! Do you have plans to populate the I2C busses ?
>   

Yes, I'll be adding a pca9552 (possibly two) very soon.  There's also a
a change coming for fixing an i2c master reset issue and something for
resetting the connected I2C buses at realization so that the slave
devices also are reset.  I think I'll add these as a series.

> > Fixed this by adding separate tables for power9 and power10 that
> > map the I2C controller number to the number of I2C buses that
> > should
> > be attached for that engine.
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> > 
> > Based-on: <20231017221434.810363-1-milesg@linux.vnet.ibm.com>
> > ([PATCH] ppc/pnv: Connect PNV I2C controller to powernv10)
> > 
> >   hw/ppc/pnv.c              | 12 ++++++++----
> >   include/hw/ppc/pnv_chip.h |  5 +----
> >   2 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 2655b6e506..6ad4a1a7b1 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1507,6 +1507,8 @@ static void
> > pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
> >       }
> >   }
> >   
> > +static int pnv_power9_i2c_ports_per_ctlr[PNV9_CHIP_MAX_I2C] = {2,
> > 13, 2, 2};
> 
> The ports array could be under PnvChipClass. 

Ok, I can look into doing that.

> Anyhow,
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
> >   static void pnv_chip_power9_realize(DeviceState *dev, Error
> > **errp)
> >   {
> >       PnvChipClass *pcc = PNV_CHIP_GET_CLASS(dev);
> > @@ -1626,7 +1628,8 @@ static void
> > pnv_chip_power9_realize(DeviceState *dev, Error **errp)
> >           Object *obj =  OBJECT(&chip9->i2c[i]);
> >   
> >           object_property_set_int(obj, "engine", i + 1,
> > &error_fatal);
> > -        object_property_set_int(obj, "num-busses", pcc-
> > >i2c_num_ports,
> > +        object_property_set_int(obj, "num-busses",
> > +                                pnv_power9_i2c_ports_per_ctlr[i],
> >                                   &error_fatal);
> >           object_property_set_link(obj, "chip", OBJECT(chip),
> > &error_abort);
> >           if (!qdev_realize(DEVICE(obj), NULL, errp)) {
> > @@ -1667,7 +1670,6 @@ static void
> > pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >       dc->desc = "PowerNV Chip POWER9";
> >       k->num_pecs = PNV9_CHIP_MAX_PEC;
> >       k->i2c_num_engines = PNV9_CHIP_MAX_I2C;
> > -    k->i2c_num_ports = PNV9_CHIP_MAX_I2C_PORTS;
> >   
> >       device_class_set_parent_realize(dc, pnv_chip_power9_realize,
> >                                       &k->parent_realize);
> > @@ -1751,6 +1753,8 @@ static void
> > pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
> >       }
> >   }
> >   
> > +static int pnv_power10_i2c_ports_per_ctlr[PNV10_CHIP_MAX_I2C] =
> > {14, 14, 2, 16};
> > +
> >   static void pnv_chip_power10_realize(DeviceState *dev, Error
> > **errp)
> >   {
> >       PnvChipClass *pcc = PNV_CHIP_GET_CLASS(dev);
> > @@ -1877,7 +1881,8 @@ static void
> > pnv_chip_power10_realize(DeviceState *dev, Error **errp)
> >           Object *obj =  OBJECT(&chip10->i2c[i]);
> >   
> >           object_property_set_int(obj, "engine", i + 1,
> > &error_fatal);
> > -        object_property_set_int(obj, "num-busses", pcc-
> > >i2c_num_ports,
> > +        object_property_set_int(obj, "num-busses",
> > +                                pnv_power10_i2c_ports_per_ctlr[i],
> >                                   &error_fatal);
> >           object_property_set_link(obj, "chip", OBJECT(chip),
> > &error_abort);
> >           if (!qdev_realize(DEVICE(obj), NULL, errp)) {
> > @@ -1918,7 +1923,6 @@ static void
> > pnv_chip_power10_class_init(ObjectClass *klass, void *data)
> >       dc->desc = "PowerNV Chip POWER10";
> >       k->num_pecs = PNV10_CHIP_MAX_PEC;
> >       k->i2c_num_engines = PNV10_CHIP_MAX_I2C;
> > -    k->i2c_num_ports = PNV10_CHIP_MAX_I2C_PORTS;
> >   
> >       device_class_set_parent_realize(dc, pnv_chip_power10_realize,
> >                                       &k->parent_realize);
> > diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
> > index 5815d96ecf..be1fec5698 100644
> > --- a/include/hw/ppc/pnv_chip.h
> > +++ b/include/hw/ppc/pnv_chip.h
> > @@ -88,8 +88,7 @@ struct Pnv9Chip {
> >   #define PNV9_CHIP_MAX_PEC 3
> >       PnvPhb4PecState pecs[PNV9_CHIP_MAX_PEC];
> >   
> > -#define PNV9_CHIP_MAX_I2C 3
> > -#define PNV9_CHIP_MAX_I2C_PORTS 1
> > +#define PNV9_CHIP_MAX_I2C 4
> >       PnvI2C      i2c[PNV9_CHIP_MAX_I2C];
> >   };
> >   
> > @@ -122,7 +121,6 @@ struct Pnv10Chip {
> >       PnvPhb4PecState pecs[PNV10_CHIP_MAX_PEC];
> >   
> >   #define PNV10_CHIP_MAX_I2C 4
> > -#define PNV10_CHIP_MAX_I2C_PORTS 2
> >       PnvI2C       i2c[PNV10_CHIP_MAX_I2C];
> >   };
> >   
> > @@ -140,7 +138,6 @@ struct PnvChipClass {
> >       uint32_t     num_phbs;
> >   
> >       uint32_t     i2c_num_engines;
> > -    uint32_t     i2c_num_ports;
> >   
> >       DeviceRealize parent_realize;
> >