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

[Cheng, Xuzhou]

Thanks Bin added me into this loop.

Hi, Guenter

I am interested in your patch and the issue what you found. I want to reproduce 
your issue on Linux, but I failed, the spi-nor of sabrelite on Linux does not 
work.

Could you share your Linux kernel version? It would be great if you can share 
the detailed steps to reproduce.

My test record:
Linux version: https://github.com/torvalds/linux, the last commit is 
ac08b1c68d1b1ed3cebb218fc3ea2c07484eb07d.
Linux configuration: imx_v6_v7_defconfig.

QEMU command:
qemu-system-arm -nographic -M sabrelite -smp 4 -m 1G -serial null -serial 
/dev/pts/50 -kernel zImage -initrd rootfs.ext4 -append "root=/dev/ram 
rdinit=sbin/init" -dtb imx6q-sabrelite.dtb -net nic -net 
tap,ifname=tap_xcheng,script=no -drive file=flash.sabre,format=raw,if=mtd

Logs: there are same logs when I apply your patch or not apply. So I don't 
understand what this patch fixes, maybe I missed something? :(...

# cat /proc/mtd
dev:    size   erasesize  name
mtd0: 00200000 00001000 "spi0.0"
# ls /dev/mtd*
/dev/mtd0       /dev/mtd0ro     /dev/mtdblock0
# echo "01234567899876543210" > test
# dd if=test of=/dev/mtd0  /* flash.sabre is empty */
0+1 records in
0+1 records out
# dd if=/dev/mtd0 of=test_out
4096+0 records in
4096+0 records out
# cat test_out             /* test_out is empty */

Regards,
Xuzhou

-----Original Message-----
From: Bin Meng <bmeng.cn@gmail.com> 
Sent: 2021年9月8日 14:31
To: Guenter Roeck <linux@roeck-us.net>; Cheng, Xuzhou 
<Xuzhou.Cheng@windriver.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>; Peter Maydell 
<peter.maydell@linaro.org>; Alistair Francis <alistair@alistair23.me>; QEMU 
Developers <qemu-devel@nongnu.org>; qemu-arm <qemu-arm@nongnu.org>; 
Jean-Christophe Dubois <jcd@tribudubois.net>
Subject: Re: [PATCH] hw/ssi: imx_spi: Improve chip select handling

[Please note: This e-mail is from an EXTERNAL e-mail address]

On Wed, Sep 8, 2021 at 2:29 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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

Actually adding him this time :)