diff mbox series

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

Message ID 20201202144523.24526-2-bmeng.cn@gmail.com
State Superseded
Headers show
Series [v2,1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported | expand

Commit Message

Bin Meng Dec. 2, 2020, 2:45 p.m. UTC 
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(+)

Comments

Peter Maydell Jan. 8, 2021, 2:40 p.m. UTC | #1 
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
Bin Meng Jan. 8, 2021, 3:55 p.m. UTC | #2 
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
Peter Maydell Jan. 8, 2021, 4 p.m. UTC | #3 
On Fri, 8 Jan 2021 at 15:55, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Jan 8, 2021 at 10:40 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > 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?

Yes, I think so.

thanks
-- PMM
diff mbox series

Patch

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);
+    }
+
     s->burst_length = 0;
 }