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

[Philippe Mathieu-Daudé]

On 1/16/21 2:35 PM, Bin Meng wrote:
> 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.

You are right, if the device is in reset, it will return the reset
values (eventually 0, I haven't checked). Simple fix could be to
better place the imx_spi_reset() call in imx_spi_write().

Since we are discussing the reset bit of this device, I wonder if
it wouldn't be clearer to use the the 3-phase-reset API then...

> 
>> +            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;
>>          }