On 1/16/21 2:35 PM, Bin Meng wrote: > On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4...@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 <quint...@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@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; >> }