Re: [PATCH v2 2/2] hw/ssi: imx_spi: Disable chip selects in imx_spi_reset()

[Bin Meng]

On Fri, Jan 8, 2021 at 10:40 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 2 Dec 2020 at 14:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> >
> > When a write to ECSPI_CONREG register to disable the SPI controller,
> > imx_spi_reset() is called to reset the controller, during which CS
> > lines should have been disabled, otherwise the state machine of any
> > devices (e.g.: SPI flashes) connected to the SPI master is stuck to
> > its last state and responds incorrectly to any follow-up commands.
> >
> > Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller")
> > Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > Acked-by: Alistair Francis <alistair.francis@wdc.com>
> >
> > ---
> >
> > Changes in v2:
> > - Fix the "Fixes" tag in the commit message
> >
> >  hw/ssi/imx_spi.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index e605049a21..85c172e815 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -231,6 +231,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
> >  static void imx_spi_reset(DeviceState *dev)
> >  {
> >      IMXSPIState *s = IMX_SPI(dev);
> > +    int i;
> >
> >      DPRINTF("\n");
> >
> > @@ -243,6 +244,10 @@ static void imx_spi_reset(DeviceState *dev)
> >
> >      imx_spi_update_irq(s);
> >
> > +    for (i = 0; i < ECSPI_NUM_CS; i++) {
> > +        qemu_set_irq(s->cs_lines[i], 1);
> > +    }
>
> Calling qemu_set_irq() in a device reset function is a bad
> idea, because you don't know whether the thing on the other
> end of the IRQ line (a) has already reset before you or
> (b) is going to reset after you. If you need to do this then
> I think you need to convert this device (and perhaps whatever
> it's connected to) to the 3-phase-reset API. (But you probably
> don't, see below.)
>

Thanks for the review. What about the imx_spi_update_irq() in the
imx_spi_reset()? Should we remove that from the imx_spi_reset() as
well?

> Usually the approach is that the device on the other end
> of the line is going to reset its state anyway, so there's
> no need to actively signal an irq line change.
>
> If this is required only for the case of "guest requested
> a controller reset via the ECSPI_CONREG register" and not
> for full system reset, then you can handle that by having
> an imx_spi_soft_reset() which calls imx_spi_reset() and then
> does the qemu_set_irq() calls, so full system (power-cycle)
> reset still goes to imx_spi_reset() but guest-commanded
> reset via the register interface calls imx_spi_soft_reset().
>

Regards,
Bin