| Message ID | 20170104220624.26557-1-jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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 > >
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 >> >> > > >
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
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 > >
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
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 >
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 >> > >
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 --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)