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

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

[Guenter Roeck]

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);
-        }
-
         if (!imx_spi_is_enabled(s)) {
             /* device is disabled, so this is a soft reset */
             imx_spi_soft_reset(s);
@@ -404,9 +399,11 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
 
             /* We are in master mode */
 
-            for (i = 0; i < ECSPI_NUM_CS; i++) {
-                qemu_set_irq(s->cs_lines[i],
-                             i == imx_spi_selected_channel(s) ? 0 : 1);
+            burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH);
+            if (burst == 0) {
+                for (i = 0; i < ECSPI_NUM_CS; i++) {
+                    qemu_set_irq(s->cs_lines[i], 1);
+                }
             }
 
             if ((value & change_mask & ECSPI_CONREG_SMC) &&
-- 
2.25.1

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

[Peter Maydell]

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 ?

>          if (!imx_spi_is_enabled(s)) {
>              /* device is disabled, so this is a soft reset */
>              imx_spi_soft_reset(s);

thanks
-- PMM

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

[Guenter Roeck]

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 ?

Thanks,
Guenter

> 
>>           if (!imx_spi_is_enabled(s)) {
>>               /* device is disabled, so this is a soft reset */
>>               imx_spi_soft_reset(s);
> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

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.

-- PMM

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

[Guenter Roeck]

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.

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

Thanks,
Guenter

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

[Bin Meng]

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".

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

One quick question, did you test this on the latest QEMU? Is that
Linux used for testing? There have been a number of bug fixes in
imx_spi recently.

Regards,
Bin

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

[Philippe Mathieu-Daudé]

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.

> One quick question, did you test this on the latest QEMU? Is that
> Linux used for testing? There have been a number of bug fixes in
> imx_spi recently.

Coming from Guenter I'm almost sure the answer is "yes" to both questions.

I suppose you meant these changes?

$ git log --oneline 1da79ecc7a2..8c495d13792
8c495d13792 hw/ssi: imx_spi: Correct tx and rx fifo endianness
6ed924823c8 hw/ssi: imx_spi: Correct the burst length > 32 bit transfer
logic
24bf8ef3f53 hw/ssi: imx_spi: Round up the burst length to be multiple of 8
50dc25932eb hw/ssi: imx_spi: Disable chip selects when controller is
disabled
fb116b5456c hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled
7c87bb5333f hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled
93722b6f6a6 hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG
register value
9c431a43a62 hw/ssi: imx_spi: Remove pointless variable initialization
3c9829e5746 hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

Regards,

Phil.

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

[Guenter Roeck]

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

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

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

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

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

[Guenter Roeck]

On 9/8/21 2:05 AM, Cheng, Xuzhou wrote:
> 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 */
> 

It is a flash. I don't think dd erases the flash, so unless your flash.sabre
is all-ones you probably won't see the overwritten data.

Guenter

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

[Cheng, Xuzhou]

> 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);
> -        }
> -
>          if (!imx_spi_is_enabled(s)) {
>              /* device is disabled, so this is a soft reset */
>              imx_spi_soft_reset(s);
> @@ -404,9 +399,11 @@  static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>  
>              /* We are in master mode */
>  
> -            for (i = 0; i < ECSPI_NUM_CS; i++) {
> -                qemu_set_irq(s->cs_lines[i],
> -                             i == imx_spi_selected_channel(s) ? 0 : 1);
> +            burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH);
> +            if (burst == 0) {
> +                for (i = 0; i < ECSPI_NUM_CS; i++) {
> +                    qemu_set_irq(s->cs_lines[i], 1);
> +                }
>              }

I got some free time in the past days to investigate this issue. Guenter is right, the Linux imx-spi driver does not work on QEMU.

The reason is that the state of m25p80 machine loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, the CS line should be pulled to high, this make the state of m25p80 back to IDLE.

I have same point with Guenter, it's that set CS to 1 when burst is zero. But i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I will send a new patch to fix this issue.

BTW, the Linux driver uses DMA mode when transfer length is greater than the FIFO size, But QEMU imx-spi model doesn't support DMA now.

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

[Guenter Roeck]

On Thu, Sep 16, 2021 at 10:21:16AM +0000, Cheng, Xuzhou wrote:
> > 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);
> > -        }
> > -
> >          if (!imx_spi_is_enabled(s)) {
> >              /* device is disabled, so this is a soft reset */
> >              imx_spi_soft_reset(s);
> > @@ -404,9 +399,11 @@  static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> >  
> >              /* We are in master mode */
> >  
> > -            for (i = 0; i < ECSPI_NUM_CS; i++) {
> > -                qemu_set_irq(s->cs_lines[i],
> > -                             i == imx_spi_selected_channel(s) ? 0 : 1);
> > +            burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH);
> > +            if (burst == 0) {
> > +                for (i = 0; i < ECSPI_NUM_CS; i++) {
> > +                    qemu_set_irq(s->cs_lines[i], 1);
> > +                }
> >              }
> 
> I got some free time in the past days to investigate this issue. Guenter is right, the Linux imx-spi driver does not work on QEMU.
> 
> The reason is that the state of m25p80 machine loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, the CS line should be pulled to high, this make the state of m25p80 back to IDLE.
> 
> I have same point with Guenter, it's that set CS to 1 when burst is zero. But i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I will send a new patch to fix this issue.
> 

Thanks a lot for looking into this. If you have a better solution than mine, by
all means, please go for it. As I mentioned in my patch, I didn't really like
it, but I was unable to find a better solution.

> BTW, the Linux driver uses DMA mode when transfer length is greater than the FIFO size, But QEMU imx-spi model doesn't support DMA now.

Does it have practical impact ? Obviously my tests were somewhat limited (I was
happy to get Linux booting from flash), but I don't recall seeing a problem.

Thanks,
Guenter

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

[Cheng, Xuzhou]

> > I got some free time in the past days to investigate this issue. Guenter is right, the Linux imx-spi driver does not work on QEMU.
> >
> > The reason is that the state of m25p80 machine loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, the CS line should be pulled to high, this make the state of m25p80 back to IDLE.
> >
> > I have same point with Guenter, it's that set CS to 1 when burst is zero. But i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I will send a new patch to fix this issue.
> >
> 
> Thanks a lot for looking into this. If you have a better solution than mine, by all means, please go for it. As I mentioned in my patch, I didn't really like it, but I was unable to find a better solution.
I am doing some experiment to verify that the new patch is reasonable, so the new patch will be delayed few days.

> 
> > BTW, the Linux driver uses DMA mode when transfer length is greater than the FIFO size, But QEMU imx-spi model doesn't support DMA now.
> 
> Does it have practical impact ? Obviously my tests were somewhat limited (I was happy to get Linux booting from flash), but I don't recall seeing a problem.
There seem have no practical impact. :)

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

[Guenter Roeck]

On 9/17/21 8:09 PM, Cheng, Xuzhou wrote:
>>> I got some free time in the past days to investigate this issue. Guenter is right, the Linux imx-spi driver does not work on QEMU.
>>>
>>> The reason is that the state of m25p80 machine loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, the CS line should be pulled to high, this make the state of m25p80 back to IDLE.
>>>
>>> I have same point with Guenter, it's that set CS to 1 when burst is zero. But i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I will send a new patch to fix this issue.
>>>
>>
>> Thanks a lot for looking into this. If you have a better solution than mine, by all means, please go for it. As I mentioned in my patch, I didn't really like it, but I was unable to find a better solution.
> I am doing some experiment to verify that the new patch is reasonable, so the new patch will be delayed few days.
> 

No problem. Note that I'll be traveling for the next 2-3 weeks, and I won't be able
to test any patches during that time.

Guenter

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

[Bin Meng]

On Sat, Sep 18, 2021 at 12:19 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/17/21 8:09 PM, Cheng, Xuzhou wrote:
> >>> I got some free time in the past days to investigate this issue. Guenter is right, the Linux imx-spi driver does not work on QEMU.
> >>>
> >>> The reason is that the state of m25p80 machine loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, the CS line should be pulled to high, this make the state of m25p80 back to IDLE.
> >>>
> >>> I have same point with Guenter, it's that set CS to 1 when burst is zero. But i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I will send a new patch to fix this issue.
> >>>
> >>
> >> Thanks a lot for looking into this. If you have a better solution than mine, by all means, please go for it. As I mentioned in my patch, I didn't really like it, but I was unable to find a better solution.
> > I am doing some experiment to verify that the new patch is reasonable, so the new patch will be delayed few days.
> >
>
> No problem. Note that I'll be traveling for the next 2-3 weeks, and I won't be able
> to test any patches during that time.
>

I have some updates to share, as I have been working with Xuzhou
internally on this issue for the past days:

Current mods using BURST_LEN to determine the timing to pull up the CS
line in the SPI controller codes is a workaround. Hardware does not do
this. To understand what real hardware behavior is, Xuzhou used an
oscilloscope to verify our guess.

It turns out the root cause is elsewhere, and a proper fix will be
sent out soon.

Regards,
Bin

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

[Guenter Roeck]

On Sun, Sep 26, 2021 at 10:49:53AM +0800, Bin Meng wrote:
> On Sat, Sep 18, 2021 at 12:19 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 9/17/21 8:09 PM, Cheng, Xuzhou wrote:
> > >>> I got some free time in the past days to investigate this issue. Guenter is right, the Linux imx-spi driver does not work on QEMU.
> > >>>
> > >>> The reason is that the state of m25p80 machine loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, the CS line should be pulled to high, this make the state of m25p80 back to IDLE.
> > >>>
> > >>> I have same point with Guenter, it's that set CS to 1 when burst is zero. But i don't think it is necessary to set CS to 0 in imx_spi_flush_txfifo(). I will send a new patch to fix this issue.
> > >>>
> > >>
> > >> Thanks a lot for looking into this. If you have a better solution than mine, by all means, please go for it. As I mentioned in my patch, I didn't really like it, but I was unable to find a better solution.
> > > I am doing some experiment to verify that the new patch is reasonable, so the new patch will be delayed few days.
> > >
> >
> > No problem. Note that I'll be traveling for the next 2-3 weeks, and I won't be able
> > to test any patches during that time.
> >
> 
> I have some updates to share, as I have been working with Xuzhou
> internally on this issue for the past days:
> 
> Current mods using BURST_LEN to determine the timing to pull up the CS
> line in the SPI controller codes is a workaround. Hardware does not do
> this. To understand what real hardware behavior is, Xuzhou used an
> oscilloscope to verify our guess.
> 
> It turns out the root cause is elsewhere, and a proper fix will be
> sent out soon.
> 

Thanks a lot for tracking this down!

Guenter

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

[Guenter Roeck]

On 9/4/21 4:06 PM, 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".
> 

I copy people and mailing lists as provided by scripts/get_maintainer.pl.
I don't think it would be appropriate to copy additional people; anyone
interested in patches for a specific file should be listed in
MAINTAINERS. After all, that is what it is for.

>> 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).
>>
> 
> One quick question, did you test this on the latest QEMU? Is that
> Linux used for testing? There have been a number of bug fixes in
> imx_spi recently.
> 

I implemented and tested this patch on top if qemu v6.0.0, and since then
there has been no patch applied to the affected file. The patch still works
on top of qemu v6.1.0.

Guenter