On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4...@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é <f4...@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