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

[Peter Maydell]

On Sun, 8 Aug 2021 at 02:34, Guenter Roeck <linux@roeck-us.net> wrote:
>
> The control register does not really have a means to deselect
> all chip selects directly. As result, CS is effectively never
> deselected, and connected flash chips fail to perform read
> operations since they don't get the expected chip select signals
> to reset their state machine.
>
> Normally and per controller documentation one would assume that
> chip select should be set whenever a transfer starts (XCH is
> set or the tx fifo is written into), and that it should be disabled
> whenever a transfer is complete. However, that does not work in
> practice: attempts to implement this approach resulted in failures,
> presumably because a single transaction can be split into multiple
> transfers.
>
> At the same time, there is no explicit signal from the host indicating
> if chip select should be active or not. In the absence of such a direct
> signal, use the burst length written into the control register to
> determine if an access is ongoing or not. Disable all chip selects
> if the burst length field in the configuration register is set to 0,
> and (re-)enable chip select if a transfer is started. This is possible
> because the Linux driver clears the burst length field whenever it
> prepares the controller for the next transfer.
> This solution  is less than perfect since it effectively only disables
> chip select when initiating the next transfer, but it does work with
> Linux and should otherwise do no harm.
>
> Stop complaining if the burst length field is set to a value of 0,
> since that is done by Linux for every transfer.
>
> With this patch, a command line parameter such as "-drive
> file=flash.sabre,format=raw,if=mtd" can be used to instantiate the
> flash chip in the sabrelite emulation. Without this patch, the
> flash instantiates, but it only reads zeroes.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> I am not entirely happy with this solution, but it is the best I was
> able to come up with. If anyone has a better idea, I'll be happy
> to give it a try.
>
>  hw/ssi/imx_spi.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> 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);
> -        }

Why has this log message been removed ?

>          if (!imx_spi_is_enabled(s)) {
>              /* device is disabled, so this is a soft reset */
>              imx_spi_soft_reset(s);

thanks
-- PMM