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

[Peter Maydell]

On Thu, 2 Sept 2021 at 17:09, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/2/21 8:58 AM, Peter Maydell wrote:
> > 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 ?
>
> What I wanted to do is:
>
> "Stop complaining if the burst length field is set to a value of 0,
>   since that is done by Linux for every transfer."
>
> What I did instead is to remove the message entirely.
>
> How about the rest of the patch ? Is it worth a resend with the message
> restored (except for burst size == 0), or is it not acceptable anyway ?

I did the easy bit of the code review because answering this
question is probably a multiple-hour job...this is still on my
todo list, but I'm hoping somebody who understands the MIX
SPI device gets to it first.

-- PMM