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

[Bin Meng]

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