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

[Peter Maydell]

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

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().

thanks
-- PMM