diff mbox

[v4,i.MX] fix CS handling during SPI access.

Message ID 20170104220624.26557-1-jcd@tribudubois.net
State New
Headers show

Commit Message

Jean-Christophe DUBOIS Jan. 4, 2017, 10:06 p.m. UTC
The i.MX SPI device was not de-asserting the CS line at the end of
memory access.

This triggered a SIGSEGV in Qemu when the sabrelite emulator was acessing
a SPI flash memory.

Whith this path the CS signal is correctly asserted and deasserted arround
memory access.

Assertion level is now based on SPI device configuration.

This was tested by:
* booting linux on Sabrelite Qemu emulator.
* booting xvisor on Sabrelite Qemu emultor.

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>
---

Changes since v1:
* Fix coding style issue.

Changes since v2:
* get SS line polarity from config reg.

Changes since v3:
* Fix imx_spi_channel_pol() after review

 hw/ssi/imx_spi.c         | 42 ++++++++++++++++++++++++++++++------------
 include/hw/ssi/imx_spi.h |  2 ++
 2 files changed, 32 insertions(+), 12 deletions(-)

Comments

Peter Maydell Jan. 9, 2017, 10:46 a.m. UTC | #1
On 4 January 2017 at 22:06, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
> The i.MX SPI device was not de-asserting the CS line at the end of
> memory access.
>
> This triggered a SIGSEGV in Qemu when the sabrelite emulator was acessing
> a SPI flash memory.
>
> Whith this path the CS signal is correctly asserted and deasserted arround
> memory access.
>
> Assertion level is now based on SPI device configuration.
>
> This was tested by:
> * booting linux on Sabrelite Qemu emulator.
> * booting xvisor on Sabrelite Qemu emultor.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>

>  static void imx_spi_reset(DeviceState *dev)
>  {
>      IMXSPIState *s = IMX_SPI(dev);
> +    uint32_t i;
>
>      DPRINTF("\n");
>
> @@ -243,6 +263,11 @@ static void imx_spi_reset(DeviceState *dev)
>      imx_spi_update_irq(s);
>
>      s->burst_length = 0;
> +
> +    /* Disable all CS lines */
> +    for (i = 0; i < 4; i++) {
> +        qemu_set_irq(s->cs_lines[i], !imx_spi_channel_pol(s, i));
> +    }

Calling qemu_set_irq() in a device reset function is a bit
tricky, because in a full system reset the device at the other
end might have already reset or might not, and calling into
its handler function for the irq line change might provoke
an unwanted change of its state. We don't really have a coherent
model here but for the moment we just try to avoid calling
set_irq in a reset method.

thanks
-- PMM
mar.krzeminski Jan. 9, 2017, 7:04 p.m. UTC | #2
W dniu 09.01.2017 o 11:46, Peter Maydell pisze:
> On 4 January 2017 at 22:06, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> The i.MX SPI device was not de-asserting the CS line at the end of
>> memory access.
>>
>> This triggered a SIGSEGV in Qemu when the sabrelite emulator was acessing
>> a SPI flash memory.
>>
>> Whith this path the CS signal is correctly asserted and deasserted arround
>> memory access.
>>
>> Assertion level is now based on SPI device configuration.
>>
>> This was tested by:
>> * booting linux on Sabrelite Qemu emulator.
>> * booting xvisor on Sabrelite Qemu emultor.
>>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>
>>   static void imx_spi_reset(DeviceState *dev)
>>   {
>>       IMXSPIState *s = IMX_SPI(dev);
>> +    uint32_t i;
>>
>>       DPRINTF("\n");
>>
>> @@ -243,6 +263,11 @@ static void imx_spi_reset(DeviceState *dev)
>>       imx_spi_update_irq(s);
>>
>>       s->burst_length = 0;
>> +
>> +    /* Disable all CS lines */
>> +    for (i = 0; i < 4; i++) {
>> +        qemu_set_irq(s->cs_lines[i], !imx_spi_channel_pol(s, i));
>> +    }
> Calling qemu_set_irq() in a device reset function is a bit
> tricky, because in a full system reset the device at the other
> end might have already reset or might not, and calling into
> its handler function for the irq line change might provoke
> an unwanted change of its state. We don't really have a coherent
> model here but for the moment we just try to avoid calling
> set_irq in a reset method.
JC, if you remove qemu_set_irq() call from reset, at least m25p80 behavior
should not change since m25p80 reset handler will reset it's whole 
internal state.

Thanks,
Marcin
>
> thanks
> -- PMM
>
Peter Maydell Jan. 9, 2017, 7:06 p.m. UTC | #3
On 9 January 2017 at 19:04, mar.krzeminski <mar.krzeminski@gmail.com> wrote:
>
>
> W dniu 09.01.2017 o 11:46, Peter Maydell pisze:
>> Calling qemu_set_irq() in a device reset function is a bit
>> tricky, because in a full system reset the device at the other
>> end might have already reset or might not, and calling into
>> its handler function for the irq line change might provoke
>> an unwanted change of its state. We don't really have a coherent
>> model here but for the moment we just try to avoid calling
>> set_irq in a reset method.
>
> JC, if you remove qemu_set_irq() call from reset, at least m25p80 behavior
> should not change since m25p80 reset handler will reset it's whole internal
> state.

I noticed the reset function is also called from within the
register emulation too, so it's maybe not quite that simple...

thanks
-- PMM
Jean-Christophe DUBOIS Jan. 9, 2017, 8:05 p.m. UTC | #4
Le 09/01/2017 à 20:06, Peter Maydell a écrit :
> On 9 January 2017 at 19:04, mar.krzeminski <mar.krzeminski@gmail.com> wrote:
>>
>> W dniu 09.01.2017 o 11:46, Peter Maydell pisze:
>>> Calling qemu_set_irq() in a device reset function is a bit
>>> tricky, because in a full system reset the device at the other
>>> end might have already reset or might not, and calling into
>>> its handler function for the irq line change might provoke
>>> an unwanted change of its state. We don't really have a coherent
>>> model here but for the moment we just try to avoid calling
>>> set_irq in a reset method.
>> JC, if you remove qemu_set_irq() call from reset, at least m25p80 behavior
>> should not change since m25p80 reset handler will reset it's whole internal
>> state.
> I noticed the reset function is also called from within the
> register emulation too, so it's maybe not quite that simple...
OK, I'll fix it.

JC

>
> thanks
> -- PMM
>
>
Jean-Christophe DUBOIS Jan. 9, 2017, 9:19 p.m. UTC | #5
Hum, ... I think I have a problem.

With the default register value (that I get a reset) the CS line is 
deselected when the CS is high.

So at reset I would need to set my 4 CS lines to high in order to be 
able to drive them low later.

So during the "reset" I need to set my 4 CS line to 1 but according to 
you feedback I should not do it with qemu_set_irq()...

Is there another way than qemu_set_irq() to do set my lines to high level ?

Regards.

JC

Le 09/01/2017 à 21:05, Jean-Christophe DUBOIS a écrit :
> Le 09/01/2017 à 20:06, Peter Maydell a écrit :
>> On 9 January 2017 at 19:04, mar.krzeminski <mar.krzeminski@gmail.com> 
>> wrote:
>>>
>>> W dniu 09.01.2017 o 11:46, Peter Maydell pisze:
>>>> Calling qemu_set_irq() in a device reset function is a bit
>>>> tricky, because in a full system reset the device at the other
>>>> end might have already reset or might not, and calling into
>>>> its handler function for the irq line change might provoke
>>>> an unwanted change of its state. We don't really have a coherent
>>>> model here but for the moment we just try to avoid calling
>>>> set_irq in a reset method.
>>> JC, if you remove qemu_set_irq() call from reset, at least m25p80 
>>> behavior
>>> should not change since m25p80 reset handler will reset it's whole 
>>> internal
>>> state.
>> I noticed the reset function is also called from within the
>> register emulation too, so it's maybe not quite that simple...
> OK, I'll fix it.
>
> JC
>
>>
>> thanks
>> -- PMM
>>
>>
>
>
>
Peter Maydell Jan. 9, 2017, 9:45 p.m. UTC | #6
On 9 January 2017 at 21:19, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> Hum, ... I think I have a problem.
>
> With the default register value (that I get a reset) the CS line is
> deselected when the CS is high.
>
> So at reset I would need to set my 4 CS lines to high in order to be able to
> drive them low later.
>
> So during the "reset" I need to set my 4 CS line to 1 but according to you
> feedback I should not do it with qemu_set_irq()...
>
> Is there another way than qemu_set_irq() to do set my lines to high level ?

"Line should be asserted at device reset" is an awkward case
that we can't really handle cleanly at the moment,
unfortunately. Assuming that the device at the other end
comes out of reset as "not selected" it should still work,
though -- there is no state stored in a qemu_set_irq(),
so if both ends believe that the reset state of the line
is 1 then there's no need to call qemu_set_irq().

thanks
-- PMM
Jean-Christophe DUBOIS Jan. 9, 2017, 10:27 p.m. UTC | #7
Le 09/01/2017 à 22:45, Peter Maydell a écrit :
> On 9 January 2017 at 21:19, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>> Hum, ... I think I have a problem.
>>
>> With the default register value (that I get a reset) the CS line is
>> deselected when the CS is high.
>>
>> So at reset I would need to set my 4 CS lines to high in order to be able to
>> drive them low later.
>>
>> So during the "reset" I need to set my 4 CS line to 1 but according to you
>> feedback I should not do it with qemu_set_irq()...
>>
>> Is there another way than qemu_set_irq() to do set my lines to high level ?
> "Line should be asserted at device reset" is an awkward case
> that we can't really handle cleanly at the moment,
> unfortunately. Assuming that the device at the other end
> comes out of reset as "not selected" it should still work,
> though -- there is no state stored in a qemu_set_irq(),
> so if both ends believe that the reset state of the line
> is 1 then there's no need to call qemu_set_irq().

I might be wrong but I think they are coming out of reset with their CS 
line set to low (so they are selected by default) because this is the 
default level at reset.

If I have 4 devices on the same SPI bus/controller (attached to 
different CS line) they would all be selected by default I think.

JC

>
> thanks
> -- PMM
>
>
Peter Maydell Jan. 9, 2017, 11:02 p.m. UTC | #8
On 9 January 2017 at 22:27, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> I might be wrong but I think they are coming out of reset with
> their CS line set to low (so they are selected by default)
> because this is the default level at reset.

If that's true then you're in difficulties, because
there's no guarantee about device reset order. So
even if your SPI controller calls qemu_set_irq in
its reset function, if the devices on the other
end happen to have their reset called after the
controller then they'll still reset into selected...

thanks
-- PMM
Jean-Christophe DUBOIS Jan. 11, 2017, 4:12 p.m. UTC | #9
Le 10/01/2017 à 00:02, Peter Maydell a écrit :
> On 9 January 2017 at 22:27, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>> I might be wrong but I think they are coming out of reset with
>> their CS line set to low (so they are selected by default)
>> because this is the default level at reset.
> If that's true then you're in difficulties, because
> there's no guarantee about device reset order. So
> even if your SPI controller calls qemu_set_irq in
> its reset function, if the devices on the other
> end happen to have their reset called after the
> controller then they'll still reset into selected...
How does it work for platforms that would have pull-up resistors on some 
signals? Is it something we cannot model in QEMU?

On a related note, it seems quite a few SPI controller emulator are 
actually calling qemu_set_irq() in their reset handler.

JC

>
> thanks
> -- PMM
>
mar.krzeminski Jan. 11, 2017, 6:08 p.m. UTC | #10
W dniu 11.01.2017 o 17:12, Jean-Christophe DUBOIS pisze:
> Le 10/01/2017 à 00:02, Peter Maydell a écrit :
>> On 9 January 2017 at 22:27, Jean-Christophe DUBOIS 
>> <jcd@tribudubois.net> wrote:
>>> I might be wrong but I think they are coming out of reset with
>>> their CS line set to low (so they are selected by default)
>>> because this is the default level at reset.
>> If that's true then you're in difficulties, because
>> there's no guarantee about device reset order. So
>> even if your SPI controller calls qemu_set_irq in
>> its reset function, if the devices on the other
>> end happen to have their reset called after the
>> controller then they'll still reset into selected...
I think the simplest solution in this case is to go back what you had 
before:
set each CS line state at the beginning of each new transfer - then at 
first transfer
you will properly set all CS line before transfer begin. After transfer 
end just negate
active CS line.

> How does it work for platforms that would have pull-up resistors on 
> some signals? Is it something we cannot model in QEMU?
>
> On a related note, it seems quite a few SPI controller emulator are 
> actually calling qemu_set_irq() in their reset handler.
I also have the same problem.

Thanks,
Marcin
>
> JC
>
>>
>> thanks
>> -- PMM
>>
>
>
Jean-Christophe DUBOIS Jan. 11, 2017, 6:47 p.m. UTC | #11
Le 11/01/2017 à 19:08, mar.krzeminski a écrit :
> W dniu 11.01.2017 o 17:12, Jean-Christophe DUBOIS pisze:
>> Le 10/01/2017 à 00:02, Peter Maydell a écrit :
>>> On 9 January 2017 at 22:27, Jean-Christophe DUBOIS 
>>> <jcd@tribudubois.net> wrote:
>>>> I might be wrong but I think they are coming out of reset with
>>>> their CS line set to low (so they are selected by default)
>>>> because this is the default level at reset.
>>> If that's true then you're in difficulties, because
>>> there's no guarantee about device reset order. So
>>> even if your SPI controller calls qemu_set_irq in
>>> its reset function, if the devices on the other
>>> end happen to have their reset called after the
>>> controller then they'll still reset into selected...
> I think the simplest solution in this case is to go back what you had 
> before:
> set each CS line state at the beginning of each new transfer - then at 
> first transfer
> you will properly set all CS line before transfer begin. After 
> transfer end just negate
> active CS line.

OK, let's do this then ...

JC

>
>> How does it work for platforms that would have pull-up resistors on 
>> some signals? Is it something we cannot model in QEMU?
>>
>> On a related note, it seems quite a few SPI controller emulator are 
>> actually calling qemu_set_irq() in their reset handler.
> I also have the same problem.
>
> Thanks,
> Marcin
>>
>> JC
>>
>>>
>>> thanks
>>> -- PMM
>>>
>>
>>
>
diff mbox

Patch

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index e4e395f..e57cb9a 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -141,6 +141,18 @@  static bool imx_spi_channel_is_master(IMXSPIState *s)
     return (mode & (1 << imx_spi_selected_channel(s))) ? true : false;
 }
 
+static uint8_t imx_spi_channel_pol(IMXSPIState *s, uint8_t channel)
+{
+    uint8_t pol = EXTRACT(s->regs[ECSPI_CONFIGREG], ECSPI_CONFIGREG_SS_POL);
+
+    return pol & (1 << channel) ? 1 : 0;
+}
+
+static uint8_t imx_spi_current_channel_pol(IMXSPIState *s)
+{
+    return imx_spi_channel_pol(s, imx_spi_selected_channel(s));
+}
+
 static bool imx_spi_is_multiple_master_burst(IMXSPIState *s)
 {
     uint8_t wave = EXTRACT(s->regs[ECSPI_CONFIGREG], ECSPI_CONFIGREG_SS_CTL);
@@ -152,13 +164,16 @@  static bool imx_spi_is_multiple_master_burst(IMXSPIState *s)
 
 static void imx_spi_flush_txfifo(IMXSPIState *s)
 {
-    uint32_t tx;
-    uint32_t rx;
-
     DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n",
             fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
 
+    /* Activate the requested CS line */
+    qemu_set_irq(s->cs_lines[imx_spi_selected_channel(s)],
+                 imx_spi_current_channel_pol(s));
+
     while (!fifo32_is_empty(&s->tx_fifo)) {
+        uint32_t tx;
+        uint32_t rx = 0;
         int tx_burst = 0;
         int index = 0;
 
@@ -178,8 +193,6 @@  static void imx_spi_flush_txfifo(IMXSPIState *s)
 
         tx_burst = MIN(s->burst_length, 32);
 
-        rx = 0;
-
         while (tx_burst) {
             uint8_t byte = tx & 0xff;
 
@@ -221,6 +234,12 @@  static void imx_spi_flush_txfifo(IMXSPIState *s)
         s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
     }
 
+    /* Deselect all SS lines if transfert if completed */
+    if (s->regs[ECSPI_STATREG] & ECSPI_STATREG_TC) {
+        qemu_set_irq(s->cs_lines[imx_spi_selected_channel(s)],
+                     !imx_spi_current_channel_pol(s));
+    }
+
     /* TODO: We should also use TDR and RDR bits */
 
     DPRINTF("End: TX Fifo Size = %d, RX Fifo Size = %d\n",
@@ -230,6 +249,7 @@  static void imx_spi_flush_txfifo(IMXSPIState *s)
 static void imx_spi_reset(DeviceState *dev)
 {
     IMXSPIState *s = IMX_SPI(dev);
+    uint32_t i;
 
     DPRINTF("\n");
 
@@ -243,6 +263,11 @@  static void imx_spi_reset(DeviceState *dev)
     imx_spi_update_irq(s);
 
     s->burst_length = 0;
+
+    /* Disable all CS lines */
+    for (i = 0; i < 4; i++) {
+        qemu_set_irq(s->cs_lines[i], !imx_spi_channel_pol(s, i));
+    }
 }
 
 static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
@@ -359,15 +384,8 @@  static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
         }
 
         if (imx_spi_channel_is_master(s)) {
-            int i;
-
             /* We are in master mode */
 
-            for (i = 0; i < 4; i++) {
-                qemu_set_irq(s->cs_lines[i],
-                             i == imx_spi_selected_channel(s) ? 0 : 1);
-            }
-
             if ((value & change_mask & ECSPI_CONREG_SMC) &&
                 !fifo32_is_empty(&s->tx_fifo)) {
                 /* SMC bit is set and TX FIFO has some slots filled in */
diff --git a/include/hw/ssi/imx_spi.h b/include/hw/ssi/imx_spi.h
index 7103953..b9b9819 100644
--- a/include/hw/ssi/imx_spi.h
+++ b/include/hw/ssi/imx_spi.h
@@ -46,6 +46,8 @@ 
 /* ECSPI_CONFIGREG */
 #define ECSPI_CONFIGREG_SS_CTL_SHIFT 8
 #define ECSPI_CONFIGREG_SS_CTL_LENGTH 4
+#define ECSPI_CONFIGREG_SS_POL_SHIFT 12
+#define ECSPI_CONFIGREG_SS_POL_LENGTH 4
 
 /* ECSPI_INTREG */
 #define ECSPI_INTREG_TEEN (1 << 0)