Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Peter Maydell]

On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> 
> wrote:
> >
> > On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >
> > > When the block is disabled, all registers are reset with the
> > > exception of the ECSPI_CONREG. It is initialized to zero
> > > when the instance is created.
> > >
> > > Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
> > >      chapter 21.7.3: Control Register (ECSPIx_CONREG)
> >
> > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > > index 8fb3c9b..c952a3d 100644
> > > --- a/hw/ssi/imx_spi.c
> > > +++ b/hw/ssi/imx_spi.c
> > > @@ -231,12 +231,23 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
> > >  static void imx_spi_reset(DeviceState *dev)
> > >  {
> > >      IMXSPIState *s = IMX_SPI(dev);
> > > +    int i;
> > >
> > >      DPRINTF("\n");
> > >
> > > -    memset(s->regs, 0, sizeof(s->regs));
> > > -
> > > -    s->regs[ECSPI_STATREG] = 0x00000003;
> > > +    for (i = 0; i < ARRAY_SIZE(s->regs); i++) {
> > > +        switch (i) {
> > > +        case ECSPI_CONREG:
> > > +            /* CONREG is not updated on reset */
> > > +            break;
> > > +        case ECSPI_STATREG:
> > > +            s->regs[i] = 0x00000003;
> > > +            break;
> > > +        default:
> > > +            s->regs[i] = 0;
> > > +            break;
> > > +        }
> > > +    }
> >
> > This retains the CONREG register value for both:
> >  * 'soft' reset caused by write to device register to disable
> >    the block
> >    -- this is corrcet as per the datasheet quote
> >  * 'power on' reset via TYPE_DEVICE's reset method
> >    -- but in this case we should reset CONREG, because the Device
> >    reset method is like a complete device powercycle and should
> >    return the device state to what it was when QEMU was first
> >    started.
>
> The POR value of CONREG is zero, which should be the default value, no?

But you're not setting it to zero here, you're leaving it with
whatever value it had before. (That's correct for soft reset,
but wrong for power-on.)

thanks
-- PMM