| Message ID | 20210115153049.3353008-5-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ssi: imx_spi: Fix various bugs in the imx_spi model | expand |
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
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; >> }
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 */ + 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: + 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; }
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(-)