Re: [PATCH v7 4/9] hw/ssi: imx_spi: Rework imx_spi_read() 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, it stay it is 'internal reset logic'
> (internal clocks are gated off). Reading any register returns
> its reset value. Only update this value if the device is enabled.
>
> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
>      chapter 21.7.3: Control Register (ECSPIx_CONREG)
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ssi/imx_spi.c | 60 +++++++++++++++++++++++-------------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 78b19c2eb91..ba7d3438d87 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -269,42 +269,40 @@ static uint64_t imx_spi_read(void *opaque, hwaddr 
> offset, unsigned size)
>          return 0;
>      }
>
> -    switch (index) {
> -    case ECSPI_RXDATA:
> -        if (!imx_spi_is_enabled(s)) {
> -            value = 0;
> -        } else if (fifo32_is_empty(&s->rx_fifo)) {
> -            /* value is undefined */
> -            value = 0xdeadbeef;
> -        } else {
> -            /* read from the RX FIFO */
> -            value = fifo32_pop(&s->rx_fifo);
> +    value = s->regs[index];
> +
> +    if (imx_spi_is_enabled(s)) {
> +        switch (index) {
> +        case ECSPI_RXDATA:
> +            if (fifo32_is_empty(&s->rx_fifo)) {
> +                /* value is undefined */
> +                value = 0xdeadbeef;
> +            } else {
> +                /* read from the RX FIFO */
> +                value = fifo32_pop(&s->rx_fifo);
> +            }
> +            break;
> +        case ECSPI_TXDATA:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "[%s]%s: Trying to read from TX FIFO\n",
> +                          TYPE_IMX_SPI, __func__);
> +
> +            /* Reading from TXDATA gives 0 */

The new logic is a little bit non straight forward as the value 0
comes from s->regs[index] which was never written hence 0. While the
previous logic is returning explicitly zero. Perhaps a comment update
is needed.

> +            break;
> +        case ECSPI_MSGDATA:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "[%s]%s: Trying to read from MSG FIFO\n",
> +                          TYPE_IMX_SPI, __func__);
> +            /* Reading from MSGDATA gives 0 */

ditto

> +            break;
> +        default:
> +            break;
>          }
>
> -        break;
> -    case ECSPI_TXDATA:
> -        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from TX 
> FIFO\n",
> -                      TYPE_IMX_SPI, __func__);
> -
> -        /* Reading from TXDATA gives 0 */
> -
> -        break;
> -    case ECSPI_MSGDATA:
> -        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from MSG 
> FIFO\n",
> -                      TYPE_IMX_SPI, __func__);
> -
> -        /* Reading from MSGDATA gives 0 */
> -
> -        break;
> -    default:
> -        value = s->regs[index];
> -        break;
> +        imx_spi_update_irq(s);
>      }
> -
>      DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
>
> -    imx_spi_update_irq(s);
> -
>      return (uint64_t)value;
>  }

Regards,
Bin