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

[Bin Meng]

On Sun, Sep 5, 2021 at 1:13 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/2/21 12:29 PM, Peter Maydell wrote:
> > 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.
> >
>
> Makes sense. Of course, it would be even better if someone can explain
> how this works on real hardware.
>

I happened to notice this patch today. Better to cc people who once
worked on this part from "git blame" or "git log".

> In this context, it would be useful to know if real SPI flash chips
> reset their state to idle under some conditions which are not covered
> by the current code in hw/block/m25p80.c. Maybe the real problem is
> as simple as that code setting data_read_loop when it should not,
> or that it doesn't reset that flag when it should (unless I am missing
> something, the flag is currently only reset by disabling chip select).
>

One quick question, did you test this on the latest QEMU? Is that
Linux used for testing? There have been a number of bug fixes in
imx_spi recently.

Regards,
Bin