Re: [PATCH v2 4/8] ppc/pnv: Fix PNV I2C invalid status after reset

[Miles Glenn <milesg@linux.vnet.ibm.com>]

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);