| Message ID | 20231110194925.475909-5-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add powernv10 I2C devices and tests | expand |
On 11/10/23 20:49, Glenn Miles wrote: > The PNV I2C Controller was clearing the status register > after a reset without repopulating the "upper threshold > for I2C ports", "Command Complete" and the SCL/SDA input > level fields. > > Fixed this for resets caused by a system reset as well > as from writing to the "Immediate Reset" register. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > --- > hw/ppc/pnv_i2c.c | 42 ++++++++++++++++++------------------------ > 1 file changed, 18 insertions(+), 24 deletions(-) > > diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > index b2c738da50..f80589157b 100644 > --- a/hw/ppc/pnv_i2c.c > +++ b/hw/ppc/pnv_i2c.c > @@ -462,6 +462,23 @@ static uint64_t pnv_i2c_xscom_read(void *opaque, hwaddr addr, > return val; > } > > +static void pnv_i2c_reset(void *dev) > +{ > + PnvI2C *i2c = PNV_I2C(dev); > + > + memset(i2c->regs, 0, sizeof(i2c->regs)); > + > + i2c->regs[I2C_STAT_REG] = > + SETFIELD(I2C_STAT_UPPER_THRS, 0ull, i2c->num_busses - 1) | > + I2C_STAT_CMD_COMP | I2C_STAT_SCL_INPUT_LEVEL | > + I2C_STAT_SDA_INPUT_LEVEL; > + i2c->regs[I2C_EXTD_STAT_REG] = > + SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) | > + SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last version */ > + > + fifo8_reset(&i2c->fifo); > +} > + > static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, > uint64_t val, unsigned size) > { > @@ -499,16 +516,7 @@ static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, > break; > > case I2C_RESET_I2C_REG: > - i2c->regs[I2C_MODE_REG] = 0; > - i2c->regs[I2C_CMD_REG] = 0; > - i2c->regs[I2C_WATERMARK_REG] = 0; > - i2c->regs[I2C_INTR_MASK_REG] = 0; > - i2c->regs[I2C_INTR_COND_REG] = 0; > - i2c->regs[I2C_INTR_RAW_COND_REG] = 0; > - i2c->regs[I2C_STAT_REG] = 0; > - i2c->regs[I2C_RESIDUAL_LEN_REG] = 0; > - i2c->regs[I2C_EXTD_STAT_REG] &= > - (I2C_EXTD_STAT_FIFO_SIZE | I2C_EXTD_STAT_I2C_VERSION); > + pnv_i2c_reset(i2c); or simply call device_cold_reset() Thanks, C. > break; > > case I2C_RESET_ERRORS: > @@ -620,20 +628,6 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface *dev, void *fdt, > return 0; > } > > -static void pnv_i2c_reset(void *dev) > -{ > - PnvI2C *i2c = PNV_I2C(dev); > - > - memset(i2c->regs, 0, sizeof(i2c->regs)); > - > - i2c->regs[I2C_STAT_REG] = I2C_STAT_CMD_COMP; > - i2c->regs[I2C_EXTD_STAT_REG] = > - SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) | > - SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last version */ > - > - fifo8_reset(&i2c->fifo); > -} > - > static void pnv_i2c_realize(DeviceState *dev, Error **errp) > { > PnvI2C *i2c = PNV_I2C(dev);
On Mon, 2023-11-13 at 10:10 +0100, Cédric Le Goater wrote: > On 11/10/23 20:49, Glenn Miles wrote: > > The PNV I2C Controller was clearing the status register > > after a reset without repopulating the "upper threshold > > for I2C ports", "Command Complete" and the SCL/SDA input > > level fields. > > > > Fixed this for resets caused by a system reset as well > > as from writing to the "Immediate Reset" register. > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > --- > > hw/ppc/pnv_i2c.c | 42 ++++++++++++++++++------------------------ > > 1 file changed, 18 insertions(+), 24 deletions(-) > > > > diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > > index b2c738da50..f80589157b 100644 > > --- a/hw/ppc/pnv_i2c.c > > +++ b/hw/ppc/pnv_i2c.c > > @@ -462,6 +462,23 @@ static uint64_t pnv_i2c_xscom_read(void > > *opaque, hwaddr addr, > > return val; > > } > > > > +static void pnv_i2c_reset(void *dev) > > +{ > > + PnvI2C *i2c = PNV_I2C(dev); > > + > > + memset(i2c->regs, 0, sizeof(i2c->regs)); > > + > > + i2c->regs[I2C_STAT_REG] = > > + SETFIELD(I2C_STAT_UPPER_THRS, 0ull, i2c->num_busses - 1) | > > + I2C_STAT_CMD_COMP | I2C_STAT_SCL_INPUT_LEVEL | > > + I2C_STAT_SDA_INPUT_LEVEL; > > + i2c->regs[I2C_EXTD_STAT_REG] = > > + SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) > > | > > + SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last > > version */ > > + > > + fifo8_reset(&i2c->fifo); > > +} > > + > > static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, > > uint64_t val, unsigned size) > > { > > @@ -499,16 +516,7 @@ static void pnv_i2c_xscom_write(void *opaque, > > hwaddr addr, > > break; > > > > case I2C_RESET_I2C_REG: > > - i2c->regs[I2C_MODE_REG] = 0; > > - i2c->regs[I2C_CMD_REG] = 0; > > - i2c->regs[I2C_WATERMARK_REG] = 0; > > - i2c->regs[I2C_INTR_MASK_REG] = 0; > > - i2c->regs[I2C_INTR_COND_REG] = 0; > > - i2c->regs[I2C_INTR_RAW_COND_REG] = 0; > > - i2c->regs[I2C_STAT_REG] = 0; > > - i2c->regs[I2C_RESIDUAL_LEN_REG] = 0; > > - i2c->regs[I2C_EXTD_STAT_REG] &= > > - (I2C_EXTD_STAT_FIFO_SIZE | I2C_EXTD_STAT_I2C_VERSION); > > + pnv_i2c_reset(i2c); > > or simply call device_cold_reset() > > > Thanks, > > C. > The device_cold_reset() function performs a recursive reset, which I believe performs a reset on the specified device as well as a reset on all child devices. For the case of doing an "immediate reset" of the i2c controller, I think we just want to reset the i2c controller and not any child devices, so I think I prefer leaving it how it is if that is ok. Thanks, Glenn > > > > break; > > > > case I2C_RESET_ERRORS: > > @@ -620,20 +628,6 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface > > *dev, void *fdt, > > return 0; > > } > > > > -static void pnv_i2c_reset(void *dev) > > -{ > > - PnvI2C *i2c = PNV_I2C(dev); > > - > > - memset(i2c->regs, 0, sizeof(i2c->regs)); > > - > > - i2c->regs[I2C_STAT_REG] = I2C_STAT_CMD_COMP; > > - i2c->regs[I2C_EXTD_STAT_REG] = > > - SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) > > | > > - SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last > > version */ > > - > > - fifo8_reset(&i2c->fifo); > > -} > > - > > static void pnv_i2c_realize(DeviceState *dev, Error **errp) > > { > > PnvI2C *i2c = PNV_I2C(dev);
On 11/14/23 16:26, Miles Glenn wrote: > On Mon, 2023-11-13 at 10:10 +0100, Cédric Le Goater wrote: >> On 11/10/23 20:49, Glenn Miles wrote: >>> The PNV I2C Controller was clearing the status register >>> after a reset without repopulating the "upper threshold >>> for I2C ports", "Command Complete" and the SCL/SDA input >>> level fields. >>> >>> Fixed this for resets caused by a system reset as well >>> as from writing to the "Immediate Reset" register. >>> >>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> >>> --- >>> hw/ppc/pnv_i2c.c | 42 ++++++++++++++++++------------------------ >>> 1 file changed, 18 insertions(+), 24 deletions(-) >>> >>> diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c >>> index b2c738da50..f80589157b 100644 >>> --- a/hw/ppc/pnv_i2c.c >>> +++ b/hw/ppc/pnv_i2c.c >>> @@ -462,6 +462,23 @@ static uint64_t pnv_i2c_xscom_read(void >>> *opaque, hwaddr addr, >>> return val; >>> } >>> >>> +static void pnv_i2c_reset(void *dev) >>> +{ >>> + PnvI2C *i2c = PNV_I2C(dev); >>> + >>> + memset(i2c->regs, 0, sizeof(i2c->regs)); >>> + >>> + i2c->regs[I2C_STAT_REG] = >>> + SETFIELD(I2C_STAT_UPPER_THRS, 0ull, i2c->num_busses - 1) | >>> + I2C_STAT_CMD_COMP | I2C_STAT_SCL_INPUT_LEVEL | >>> + I2C_STAT_SDA_INPUT_LEVEL; >>> + i2c->regs[I2C_EXTD_STAT_REG] = >>> + SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) >>> | >>> + SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last >>> version */ >>> + >>> + fifo8_reset(&i2c->fifo); >>> +} >>> + >>> static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, >>> uint64_t val, unsigned size) >>> { >>> @@ -499,16 +516,7 @@ static void pnv_i2c_xscom_write(void *opaque, >>> hwaddr addr, >>> break; >>> >>> case I2C_RESET_I2C_REG: >>> - i2c->regs[I2C_MODE_REG] = 0; >>> - i2c->regs[I2C_CMD_REG] = 0; >>> - i2c->regs[I2C_WATERMARK_REG] = 0; >>> - i2c->regs[I2C_INTR_MASK_REG] = 0; >>> - i2c->regs[I2C_INTR_COND_REG] = 0; >>> - i2c->regs[I2C_INTR_RAW_COND_REG] = 0; >>> - i2c->regs[I2C_STAT_REG] = 0; >>> - i2c->regs[I2C_RESIDUAL_LEN_REG] = 0; >>> - i2c->regs[I2C_EXTD_STAT_REG] &= >>> - (I2C_EXTD_STAT_FIFO_SIZE | I2C_EXTD_STAT_I2C_VERSION); >>> + pnv_i2c_reset(i2c); >> >> or simply call device_cold_reset() >> >> >> Thanks, >> >> C. >> > > The device_cold_reset() function performs a recursive reset, which > I believe performs a reset on the specified device as well as a reset > on all child devices. For the case of doing an "immediate reset" of > the i2c controller, I think we just want to reset the i2c controller > and not any child devices, so I think I prefer leaving it how it is > if that is ok. OK. Is is a fix for 8.2 ? Thanks, C. > Thanks, > > Glenn > >> >> >>> break; >>> >>> case I2C_RESET_ERRORS: >>> @@ -620,20 +628,6 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface >>> *dev, void *fdt, >>> return 0; >>> } >>> >>> -static void pnv_i2c_reset(void *dev) >>> -{ >>> - PnvI2C *i2c = PNV_I2C(dev); >>> - >>> - memset(i2c->regs, 0, sizeof(i2c->regs)); >>> - >>> - i2c->regs[I2C_STAT_REG] = I2C_STAT_CMD_COMP; >>> - i2c->regs[I2C_EXTD_STAT_REG] = >>> - SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) >>> | >>> - SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last >>> version */ >>> - >>> - fifo8_reset(&i2c->fifo); >>> -} >>> - >>> static void pnv_i2c_realize(DeviceState *dev, Error **errp) >>> { >>> PnvI2C *i2c = PNV_I2C(dev); >
On Tue, 2023-11-14 at 18:55 +0100, Cédric Le Goater wrote: > On 11/14/23 16:26, Miles Glenn wrote: > > On Mon, 2023-11-13 at 10:10 +0100, Cédric Le Goater wrote: > > > On 11/10/23 20:49, Glenn Miles wrote: > > > > The PNV I2C Controller was clearing the status register > > > > after a reset without repopulating the "upper threshold > > > > for I2C ports", "Command Complete" and the SCL/SDA input > > > > level fields. > > > > > > > > Fixed this for resets caused by a system reset as well > > > > as from writing to the "Immediate Reset" register. > > > > > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > > > --- > > > > hw/ppc/pnv_i2c.c | 42 ++++++++++++++++++------------------ > > > > ------ > > > > 1 file changed, 18 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > > > > index b2c738da50..f80589157b 100644 > > > > --- a/hw/ppc/pnv_i2c.c > > > > +++ b/hw/ppc/pnv_i2c.c > > > > @@ -462,6 +462,23 @@ static uint64_t pnv_i2c_xscom_read(void > > > > *opaque, hwaddr addr, > > > > return val; > > > > } > > > > > > > > +static void pnv_i2c_reset(void *dev) > > > > +{ > > > > + PnvI2C *i2c = PNV_I2C(dev); > > > > + > > > > + memset(i2c->regs, 0, sizeof(i2c->regs)); > > > > + > > > > + i2c->regs[I2C_STAT_REG] = > > > > + SETFIELD(I2C_STAT_UPPER_THRS, 0ull, i2c->num_busses - > > > > 1) | > > > > + I2C_STAT_CMD_COMP | I2C_STAT_SCL_INPUT_LEVEL | > > > > + I2C_STAT_SDA_INPUT_LEVEL; > > > > + i2c->regs[I2C_EXTD_STAT_REG] = > > > > + SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, > > > > PNV_I2C_FIFO_SIZE) > > > > + SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last > > > > version */ > > > > + > > > > + fifo8_reset(&i2c->fifo); > > > > +} > > > > + > > > > static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, > > > > uint64_t val, unsigned size) > > > > { > > > > @@ -499,16 +516,7 @@ static void pnv_i2c_xscom_write(void > > > > *opaque, > > > > hwaddr addr, > > > > break; > > > > > > > > case I2C_RESET_I2C_REG: > > > > - i2c->regs[I2C_MODE_REG] = 0; > > > > - i2c->regs[I2C_CMD_REG] = 0; > > > > - i2c->regs[I2C_WATERMARK_REG] = 0; > > > > - i2c->regs[I2C_INTR_MASK_REG] = 0; > > > > - i2c->regs[I2C_INTR_COND_REG] = 0; > > > > - i2c->regs[I2C_INTR_RAW_COND_REG] = 0; > > > > - i2c->regs[I2C_STAT_REG] = 0; > > > > - i2c->regs[I2C_RESIDUAL_LEN_REG] = 0; > > > > - i2c->regs[I2C_EXTD_STAT_REG] &= > > > > - (I2C_EXTD_STAT_FIFO_SIZE | > > > > I2C_EXTD_STAT_I2C_VERSION); > > > > + pnv_i2c_reset(i2c); > > > > > > or simply call device_cold_reset() > > > > > > > > > Thanks, > > > > > > C. > > > > > > > The device_cold_reset() function performs a recursive reset, which > > I believe performs a reset on the specified device as well as a > > reset > > on all child devices. For the case of doing an "immediate reset" > > of > > the i2c controller, I think we just want to reset the i2c > > controller > > and not any child devices, so I think I prefer leaving it how it is > > if that is ok. > > OK. Is is a fix for 8.2 ? > > Thanks, > > C. > Yes, I will send out a v3 stating that. Thanks, Glenn > > > Thanks, > > > > Glenn > > > > > > > > > break; > > > > > > > > case I2C_RESET_ERRORS: > > > > @@ -620,20 +628,6 @@ static int > > > > pnv_i2c_dt_xscom(PnvXScomInterface > > > > *dev, void *fdt, > > > > return 0; > > > > } > > > > > > > > -static void pnv_i2c_reset(void *dev) > > > > -{ > > > > - PnvI2C *i2c = PNV_I2C(dev); > > > > - > > > > - memset(i2c->regs, 0, sizeof(i2c->regs)); > > > > - > > > > - i2c->regs[I2C_STAT_REG] = I2C_STAT_CMD_COMP; > > > > - i2c->regs[I2C_EXTD_STAT_REG] = > > > > - SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, > > > > PNV_I2C_FIFO_SIZE) > > > > - SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last > > > > version */ > > > > - > > > > - fifo8_reset(&i2c->fifo); > > > > -} > > > > - > > > > static void pnv_i2c_realize(DeviceState *dev, Error **errp) > > > > { > > > > PnvI2C *i2c = PNV_I2C(dev);
diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c index b2c738da50..f80589157b 100644 --- a/hw/ppc/pnv_i2c.c +++ b/hw/ppc/pnv_i2c.c @@ -462,6 +462,23 @@ static uint64_t pnv_i2c_xscom_read(void *opaque, hwaddr addr, return val; } +static void pnv_i2c_reset(void *dev) +{ + PnvI2C *i2c = PNV_I2C(dev); + + memset(i2c->regs, 0, sizeof(i2c->regs)); + + i2c->regs[I2C_STAT_REG] = + SETFIELD(I2C_STAT_UPPER_THRS, 0ull, i2c->num_busses - 1) | + I2C_STAT_CMD_COMP | I2C_STAT_SCL_INPUT_LEVEL | + I2C_STAT_SDA_INPUT_LEVEL; + i2c->regs[I2C_EXTD_STAT_REG] = + SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) | + SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last version */ + + fifo8_reset(&i2c->fifo); +} + static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { @@ -499,16 +516,7 @@ static void pnv_i2c_xscom_write(void *opaque, hwaddr addr, break; case I2C_RESET_I2C_REG: - i2c->regs[I2C_MODE_REG] = 0; - i2c->regs[I2C_CMD_REG] = 0; - i2c->regs[I2C_WATERMARK_REG] = 0; - i2c->regs[I2C_INTR_MASK_REG] = 0; - i2c->regs[I2C_INTR_COND_REG] = 0; - i2c->regs[I2C_INTR_RAW_COND_REG] = 0; - i2c->regs[I2C_STAT_REG] = 0; - i2c->regs[I2C_RESIDUAL_LEN_REG] = 0; - i2c->regs[I2C_EXTD_STAT_REG] &= - (I2C_EXTD_STAT_FIFO_SIZE | I2C_EXTD_STAT_I2C_VERSION); + pnv_i2c_reset(i2c); break; case I2C_RESET_ERRORS: @@ -620,20 +628,6 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface *dev, void *fdt, return 0; } -static void pnv_i2c_reset(void *dev) -{ - PnvI2C *i2c = PNV_I2C(dev); - - memset(i2c->regs, 0, sizeof(i2c->regs)); - - i2c->regs[I2C_STAT_REG] = I2C_STAT_CMD_COMP; - i2c->regs[I2C_EXTD_STAT_REG] = - SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) | - SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last version */ - - fifo8_reset(&i2c->fifo); -} - static void pnv_i2c_realize(DeviceState *dev, Error **errp) { PnvI2C *i2c = PNV_I2C(dev);
The PNV I2C Controller was clearing the status register after a reset without repopulating the "upper threshold for I2C ports", "Command Complete" and the SCL/SDA input level fields. Fixed this for resets caused by a system reset as well as from writing to the "Immediate Reset" register. Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> --- hw/ppc/pnv_i2c.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)