| Message ID | 20210808013406.223506-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ssi: imx_spi: Improve chip select handling | expand |
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
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 >
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
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
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
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.
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
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
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
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 :)
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 :)
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
> 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.
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
> > 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. :)
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
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
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
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) &&
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(-)