On Sun, Sep 5, 2021 at 10:08 AM Guenter Roeck <li...@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 <li...@roeck-us.net> wrote: > >>> > >>> On 9/2/21 12:29 PM, Peter Maydell wrote: > >>>> On Thu, 2 Sept 2021 at 17:09, Guenter Roeck <li...@roeck-us.net> wrote: > >>>>> > >>>>> On 9/2/21 8:58 AM, Peter Maydell wrote: > >>>>>> On Sun, 8 Aug 2021 at 02:34, Guenter Roeck <li...@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 <li...@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 <alist...@alistair23.me> (maintainer:SSI) > > Peter Maydell <peter.mayd...@linaro.org> (odd fixer:i.MX31 (kzm)) > > Jean-Christophe Dubois <j...@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