Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Bin Meng]

On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> When the block is disabled, only the ECSPI_CONREG register can
> be modified. Setting the EN bit enabled the device, clearing it

I don't know how this conclusion came out. The manual only says the
following 2 registers ignore the write when the block is disabled.

ECSPI_TXDATA, ECSPI_INTREG

> "disables the block and resets the internal logic with the
> exception of the ECSPI_CONREG" register.
>
> Move the imx_spi_is_enabled() check earlier.
>
> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
>      chapter 21.7.3: Control Register (ECSPIx_CONREG)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ssi/imx_spi.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index ba7d3438d87..f06bbf317e2 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
> uint64_t value,
>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
>              (uint32_t)value);
>
> +    if (!imx_spi_is_enabled(s)) {
> +        /* Block is disabled */
> +        if (index != ECSPI_CONREG) {
> +            /* Ignore access */
> +            return;
> +        }
> +        s->regs[ECSPI_CONREG] = value;
> +        if (!(value & ECSPI_CONREG_EN)) {
> +            /* Keep disabled */

So other bits except ECSPI_CONREG_EN are discarded? The manual does
not explicitly mention this but this looks suspicious.

> +            return;
> +        }
> +        /* Enable the block */
> +        imx_spi_reset(DEVICE(s));
> +    }
> +
>      change_mask = s->regs[index] ^ value;
>
>      switch (index) {
> @@ -330,10 +345,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
> uint64_t value,
>                        TYPE_IMX_SPI, __func__);
>          break;
>      case ECSPI_TXDATA:
> -        if (!imx_spi_is_enabled(s)) {
> -            /* Ignore writes if device is disabled */
> -            break;
> -        } else if (fifo32_is_full(&s->tx_fifo)) {
> +        if (fifo32_is_full(&s->tx_fifo)) {
>              /* Ignore writes if queue is full */
>              break;
>          }
> @@ -359,12 +371,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
> uint64_t value,
>      case ECSPI_CONREG:
>          s->regs[ECSPI_CONREG] = value;
>
> -        if (!imx_spi_is_enabled(s)) {
> -            /* device is disabled, so this is a reset */
> -            imx_spi_reset(DEVICE(s));
> -            return;
> -        }
> -
>          if (imx_spi_channel_is_master(s)) {
>              int i;
>

Regards,
Bin