qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Guenter Roeck
Subject: Re: [PATCH] hw/ssi: imx_spi: Improve chip select handling
Date: Sat, 4 Sep 2021 19:08:12 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

On 9/4/21 4:19 PM, Philippe Mathieu-Daudé wrote:
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.


Possibly. Note that I did check the flash chip specification, but I don't
see a notable difference to the qemu implementation. But then, again,
I may be missing something.

Guenter



reply via email to

[Prev in Thread] Current Thread [Next in Thread]