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

[Philippe Mathieu-Daudé]

On 9/5/21 1:06 AM, Bin Meng wrote:
> 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".

Even better if you add yourself as designated reviewer ;)

$ ./scripts/get_maintainer.pl -f hw/ssi/imx_spi.c
Alistair Francis <alistair@alistair23.me> (maintainer:SSI)
Peter Maydell <peter.maydell@linaro.org> (odd fixer:i.MX31 (kzm))
Jean-Christophe Dubois <jcd@tribudubois.net> (reviewer:SABRELITE / i.MX6)

> 
>> 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).

Plausible hypothesis.

> 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.

Coming from Guenter I'm almost sure the answer is "yes" to both questions.

I suppose you meant these changes?

$ git log --oneline 1da79ecc7a2..8c495d13792
8c495d13792 hw/ssi: imx_spi: Correct tx and rx fifo endianness
6ed924823c8 hw/ssi: imx_spi: Correct the burst length > 32 bit transfer
logic
24bf8ef3f53 hw/ssi: imx_spi: Round up the burst length to be multiple of 8
50dc25932eb hw/ssi: imx_spi: Disable chip selects when controller is
disabled
fb116b5456c hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled
7c87bb5333f hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled
93722b6f6a6 hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG
register value
9c431a43a62 hw/ssi: imx_spi: Remove pointless variable initialization
3c9829e5746 hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

Regards,

Phil.