[PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Avoid using a magic number (4) everywhere for the number of chip
selects supported.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---

(no changes since v1)

 hw/ssi/imx_spi.c         | 4 ++--
 include/hw/ssi/imx_spi.h | 5 ++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index d8885ae454..e605049a21 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -361,7 +361,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
 
             /* We are in master mode */
 
-            for (i = 0; i < 4; i++) {
+            for (i = 0; i < ECSPI_NUM_CS; i++) {
                 qemu_set_irq(s->cs_lines[i],
                              i == imx_spi_selected_channel(s) ? 0 : 1);
             }
@@ -424,7 +424,7 @@ static void imx_spi_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
 
-    for (i = 0; i < 4; ++i) {
+    for (i = 0; i < ECSPI_NUM_CS; ++i) {
         sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cs_lines[i]);
     }
 
diff --git a/include/hw/ssi/imx_spi.h b/include/hw/ssi/imx_spi.h
index b82b17f364..eeaf49bbac 100644
--- a/include/hw/ssi/imx_spi.h
+++ b/include/hw/ssi/imx_spi.h
@@ -77,6 +77,9 @@
 
 #define EXTRACT(value, name) extract32(value, name##_SHIFT, name##_LENGTH)
 
+/* number of chip selects supported */
+#define ECSPI_NUM_CS 4
+
 #define TYPE_IMX_SPI "imx.spi"
 OBJECT_DECLARE_SIMPLE_TYPE(IMXSPIState, IMX_SPI)
 
@@ -89,7 +92,7 @@ struct IMXSPIState {
 
     qemu_irq irq;
 
-    qemu_irq cs_lines[4];
+    qemu_irq cs_lines[ECSPI_NUM_CS];
 
     SSIBus *bus;
 
-- 
2.25.1

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

[Bin Meng]

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

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

Hi Alistair, Peter,

On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Avoid using a magic number (4) everywhere for the number of chip
> selects supported.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>
> (no changes since v1)
>
>  hw/ssi/imx_spi.c         | 4 ++--
>  include/hw/ssi/imx_spi.h | 5 ++++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>

Ping, not sure who is going to pick up this series?

Regards,
Bin

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

On Wed, Dec 16, 2020 at 6:24 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alistair, Peter,
>
> On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Avoid using a magic number (4) everywhere for the number of chip
> > selects supported.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >
> > (no changes since v1)
> >
> >  hw/ssi/imx_spi.c         | 4 ++--
> >  include/hw/ssi/imx_spi.h | 5 ++++-
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
>
> Ping, not sure who is going to pick up this series?

Ping?

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

On Tue, Dec 22, 2020 at 2:24 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Dec 16, 2020 at 6:24 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair, Peter,
> >
> > On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Avoid using a magic number (4) everywhere for the number of chip
> > > selects supported.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  hw/ssi/imx_spi.c         | 4 ++--
> > >  include/hw/ssi/imx_spi.h | 5 ++++-
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > >
> >
> > Ping, not sure who is going to pick up this series?
>
> Ping?

Ping?

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Alistair Francis]

On Wed, Dec 16, 2020 at 2:25 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alistair, Peter,
>
> On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Avoid using a magic number (4) everywhere for the number of chip
> > selects supported.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >
> > (no changes since v1)
> >
> >  hw/ssi/imx_spi.c         | 4 ++--
> >  include/hw/ssi/imx_spi.h | 5 ++++-
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
>
> Ping, not sure who is going to pick up this series?

It should be reviewed by Jean-Christophe and then probably go via the ARM tree.

Alistair

>
> Regards,
> Bin
>

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Peter Maydell]

On Tue, 5 Jan 2021 at 21:09, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Dec 16, 2020 at 2:25 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair, Peter,
> >
> > On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Avoid using a magic number (4) everywhere for the number of chip
> > > selects supported.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  hw/ssi/imx_spi.c         | 4 ++--
> > >  include/hw/ssi/imx_spi.h | 5 ++++-
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > >
> >
> > Ping, not sure who is going to pick up this series?
>
> It should be reviewed by Jean-Christophe and then probably go via the ARM tree.

It doesn't seem to have been sent with a cover letter. Multi-patch
patchsets without a cover letter tend to get missed because when
I scan through my email looking for patches I should review or
pick up I'm looking for either (a) single patches or (b) the 00/nn
cover letter email...

thanks
-- PMM

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Alistair Francis]

On Tue, Jan 5, 2021 at 1:08 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Dec 16, 2020 at 2:25 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair, Peter,
> >
> > On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Avoid using a magic number (4) everywhere for the number of chip
> > > selects supported.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  hw/ssi/imx_spi.c         | 4 ++--
> > >  include/hw/ssi/imx_spi.h | 5 ++++-
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > >
> >
> > Ping, not sure who is going to pick up this series?
>
> It should be reviewed by Jean-Christophe and then probably go via the ARM tree.

Ah, I just realised I apparently have `hw/ssi/*` listed in
MAINTAINERS. As this file is specifically mentioned somewhere else in
MAINTAINERS and I know very little about IMX I'm going to leave this
alone still.

Alistair

>
> Alistair
>
> >
> > Regards,
> > Bin
> >

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

On Wed, Jan 6, 2021 at 5:11 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 5 Jan 2021 at 21:09, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Dec 16, 2020 at 2:25 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Alistair, Peter,
> > >
> > > On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > Avoid using a magic number (4) everywhere for the number of chip
> > > > selects supported.
> > > >
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  hw/ssi/imx_spi.c         | 4 ++--
> > > >  include/hw/ssi/imx_spi.h | 5 ++++-
> > > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > > >
> > >
> > > Ping, not sure who is going to pick up this series?
> >
> > It should be reviewed by Jean-Christophe and then probably go via the ARM tree.
>
> It doesn't seem to have been sent with a cover letter. Multi-patch
> patchsets without a cover letter tend to get missed because when
> I scan through my email looking for patches I should review or
> pick up I'm looking for either (a) single patches or (b) the 00/nn
> cover letter email...

Thanks. I will resend it with a cover-letter.

Regards,
Bin

Re: [PATCH v2 1/2] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

On Wed, Jan 6, 2021 at 8:22 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Jan 6, 2021 at 5:11 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 5 Jan 2021 at 21:09, Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Dec 16, 2020 at 2:25 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Alistair, Peter,
> > > >
> > > > On Wed, Dec 2, 2020 at 10:45 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > From: Bin Meng <bin.meng@windriver.com>
> > > > >
> > > > > Avoid using a magic number (4) everywhere for the number of chip
> > > > > selects supported.
> > > > >
> > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  hw/ssi/imx_spi.c         | 4 ++--
> > > > >  include/hw/ssi/imx_spi.h | 5 ++++-
> > > > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > >
> > > > Ping, not sure who is going to pick up this series?
> > >
> > > It should be reviewed by Jean-Christophe and then probably go via the ARM tree.
> >
> > It doesn't seem to have been sent with a cover letter. Multi-patch
> > patchsets without a cover letter tend to get missed because when
> > I scan through my email looking for patches I should review or
> > pick up I'm looking for either (a) single patches or (b) the 00/nn
> > cover letter email...
>
> Thanks. I will resend it with a cover-letter.

This series is resent and included in the following series with a cover letter:
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=222931

Regards,
Bin

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

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

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

[Peter Maydell]

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