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);
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);
--
2.31.1On 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);
© 2016 - 2024 Red Hat, Inc.