Re: [PATCH] hw/ssi: imx_spi: Improve chip select handling

[Guenter Roeck]

On Thu, Sep 16, 2021 at 10:21:16AM +0000, Cheng, Xuzhou wrote:
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index 189423bb3a..7a093156bd 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -167,6 +167,8 @@  static void imx_spi_flush_txfifo(IMXSPIState *s)
> >      DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n",
> >              fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
> >  
> > +    qemu_set_irq(s->cs_lines[imx_spi_selected_channel(s)], 0);
> > +
> >      while (!fifo32_is_empty(&s->tx_fifo)) {
> >          int tx_burst = 0;
> >  
> > @@ -385,13 +387,6 @@  static void imx_spi_write(void *opaque, hwaddr 
> > offset, uint64_t value,
> >      case ECSPI_CONREG:
> >          s->regs[ECSPI_CONREG] = value;
> >  
> > -        burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) 
> > + 1;
> > -        if (burst % 8) {
> > -            qemu_log_mask(LOG_UNIMP,
> > -                          "[%s]%s: burst length %d not supported: rounding 
> > up to next multiple of 8\n",
> > -                          TYPE_IMX_SPI, __func__, burst);
> > -        }
> > -
> >          if (!imx_spi_is_enabled(s)) {
> >              /* device is disabled, so this is a soft reset */
> >              imx_spi_soft_reset(s);
> > @@ -404,9 +399,11 @@  static void imx_spi_write(void *opaque, hwaddr 
> > offset, uint64_t value,
> >  
> >              /* We are in master mode */
> >  
> > -            for (i = 0; i < ECSPI_NUM_CS; i++) {
> > -                qemu_set_irq(s->cs_lines[i],
> > -                             i == imx_spi_selected_channel(s) ? 0 : 1);
> > +            burst = EXTRACT(s->regs[ECSPI_CONREG], 
> > ECSPI_CONREG_BURST_LENGTH);
> > +            if (burst == 0) {
> > +                for (i = 0; i < ECSPI_NUM_CS; i++) {
> > +                    qemu_set_irq(s->cs_lines[i], 1);
> > +                }
> >              }
> 
> I got some free time in the past days to investigate this issue. Guenter is 
> right, the Linux imx-spi driver does not work on QEMU.
> 
> The reason is that the state of m25p80 machine loops in STATE_READING_DATA 
> state after receiving RDSR command, the new command is ignored. Before 
> sending a new command, the CS line should be pulled to high, this make the 
> state of m25p80 back to IDLE.
> 
> I have same point with Guenter, it's that set CS to 1 when burst is zero. But 
> i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I 
> will send a new patch to fix this issue.
> 

Thanks a lot for looking into this. If you have a better solution than mine, by
all means, please go for it. As I mentioned in my patch, I didn't really like
it, but I was unable to find a better solution.

> BTW, the Linux driver uses DMA mode when transfer length is greater than the 
> FIFO size, But QEMU imx-spi model doesn't support DMA now.

Does it have practical impact ? Obviously my tests were somewhat limited (I was
happy to get Linux booting from flash), but I don't recall seeing a problem.

Thanks,
Guenter