| Message ID | 20231024212944.34043-1-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,RESEND] ppc/pnv: Fix number of I2C engines and ports for power9/10 | expand |
On 10/24/23 23:29, 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). > > 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> you could have kept : Reviewed-by: Cédric Le Goater <clg@kaod.org> one comment below, > --- > Based-on: <20231017221434.810363-1-milesg@linux.vnet.ibm.com> > ([PATCH] ppc/pnv: Connect PNV I2C controller to powernv10) > > Changes from v1: > - Added i2c_ports_per_engine to PnvChipClass > - replaced the word "ctlr" with "engine" > > hw/ppc/pnv.c | 14 ++++++++++---- > include/hw/ppc/pnv_chip.h | 6 ++---- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 2655b6e506..f6dc84b869 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_engine[PNV9_CHIP_MAX_I2C] = {2, 13, 2, 2}; > + Generally, these class constants are located close to the class definitions in the file. 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", > + pcc->i2c_ports_per_engine[i], > &error_fatal); > object_property_set_link(obj, "chip", OBJECT(chip), &error_abort); > if (!qdev_realize(DEVICE(obj), NULL, errp)) { > @@ -1667,7 +1670,7 @@ 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; > + k->i2c_ports_per_engine = pnv_power9_i2c_ports_per_engine; > > device_class_set_parent_realize(dc, pnv_chip_power9_realize, > &k->parent_realize); > @@ -1751,6 +1754,8 @@ static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp) > } > } > > +static int pnv_power10_i2c_ports_per_engine[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 +1882,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", > + pcc->i2c_ports_per_engine[i], > &error_fatal); > object_property_set_link(obj, "chip", OBJECT(chip), &error_abort); > if (!qdev_realize(DEVICE(obj), NULL, errp)) { > @@ -1918,7 +1924,7 @@ 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; > + k->i2c_ports_per_engine = pnv_power10_i2c_ports_per_engine; > > 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..3643e0fd86 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,7 @@ struct PnvChipClass { > uint32_t num_phbs; > > uint32_t i2c_num_engines; > - uint32_t i2c_num_ports; > + int *i2c_ports_per_engine; > > DeviceRealize parent_realize; >
On 25/10/23 08:56, Cédric Le Goater wrote: > On 10/24/23 23:29, 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). >> >> 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> > > you could have kept : > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > one comment below, > >> --- >> Based-on: <20231017221434.810363-1-milesg@linux.vnet.ibm.com> >> ([PATCH] ppc/pnv: Connect PNV I2C controller to powernv10) >> >> Changes from v1: >> - Added i2c_ports_per_engine to PnvChipClass >> - replaced the word "ctlr" with "engine" >> >> hw/ppc/pnv.c | 14 ++++++++++---- >> include/hw/ppc/pnv_chip.h | 6 ++---- >> 2 files changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index 2655b6e506..f6dc84b869 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_engine[PNV9_CHIP_MAX_I2C] = {2, >> 13, 2, 2}; >> + > > Generally, these class constants are located close to the class definitions > in the file. Either keep them close by for comparison, or, since there is a single use, declare it in the function using it here pnv_chip_power9_class_init(). > > Thanks, > > C.
On Fri, 2023-10-27 at 07:05 +0200, Philippe Mathieu-Daudé wrote: > On 25/10/23 08:56, Cédric Le Goater wrote: > > On 10/24/23 23:29, 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). > > > > > > 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> > > > > you could have kept : > > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > > > one comment below, > > > > > --- > > > Based-on: <20231017221434.810363-1-milesg@linux.vnet.ibm.com> > > > ([PATCH] ppc/pnv: Connect PNV I2C controller to powernv10) > > > > > > Changes from v1: > > > - Added i2c_ports_per_engine to PnvChipClass > > > - replaced the word "ctlr" with "engine" > > > > > > hw/ppc/pnv.c | 14 ++++++++++---- > > > include/hw/ppc/pnv_chip.h | 6 ++---- > > > 2 files changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > > index 2655b6e506..f6dc84b869 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_engine[PNV9_CHIP_MAX_I2C] = > > > {2, > > > 13, 2, 2}; > > > + > > > > Generally, these class constants are located close to the class > > definitions > > in the file. > > Either keep them close by for comparison, or, since there > is a single use, declare it in the function using it here > pnv_chip_power9_class_init(). Yeah, I agree. Version 3 of this patch places the arrays inside the functions that use them. Thanks, Glenn > > > Thanks, > > > > C.
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 2655b6e506..f6dc84b869 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_engine[PNV9_CHIP_MAX_I2C] = {2, 13, 2, 2}; + 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", + pcc->i2c_ports_per_engine[i], &error_fatal); object_property_set_link(obj, "chip", OBJECT(chip), &error_abort); if (!qdev_realize(DEVICE(obj), NULL, errp)) { @@ -1667,7 +1670,7 @@ 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; + k->i2c_ports_per_engine = pnv_power9_i2c_ports_per_engine; device_class_set_parent_realize(dc, pnv_chip_power9_realize, &k->parent_realize); @@ -1751,6 +1754,8 @@ static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp) } } +static int pnv_power10_i2c_ports_per_engine[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 +1882,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", + pcc->i2c_ports_per_engine[i], &error_fatal); object_property_set_link(obj, "chip", OBJECT(chip), &error_abort); if (!qdev_realize(DEVICE(obj), NULL, errp)) { @@ -1918,7 +1924,7 @@ 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; + k->i2c_ports_per_engine = pnv_power10_i2c_ports_per_engine; 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..3643e0fd86 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,7 @@ struct PnvChipClass { uint32_t num_phbs; uint32_t i2c_num_engines; - uint32_t i2c_num_ports; + int *i2c_ports_per_engine; DeviceRealize parent_realize;
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). 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) Changes from v1: - Added i2c_ports_per_engine to PnvChipClass - replaced the word "ctlr" with "engine" hw/ppc/pnv.c | 14 ++++++++++---- include/hw/ppc/pnv_chip.h | 6 ++---- 2 files changed, 12 insertions(+), 8 deletions(-)