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

[Bin Meng]

On Sun, Sep 5, 2021 at 10:08 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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.
>

+Xuzhou Cheng who once worked on imx_spi for some comments

Regards,
Bin