Re: [Qemu-devel] [PATCH v3] [i.MX] fix CS handling during SPI access.

[mar.krzeminski]

W dniu 03.01.2017 o 21:35, Jean-Christophe Dubois pisze:
> 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.
>
> Whit 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 <address@hidden>
> ---
>
> Changes since v1:
> * Fix coding style issue.
>
> Changes since v2:
> * get SS line polarity from config reg.
>
>   hw/ssi/imx_spi.c         | 42 ++++++++++++++++++++++++++++++------------
>   include/hw/ssi/imx_spi.h |  2 ++
>   2 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index e4e395f..3cbf279 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)
> +{
channel variable is unused. Should be instead of 
imx_spi_selected_channel() call?
> +    uint8_t pol = EXTRACT(s->regs[ECSPI_CONFIGREG], ECSPI_CONFIGREG_SS_POL);
> +
> +    return pol & (1 << imx_spi_selected_channel(s)) ? 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));
> +    }
Please make sure that in HW ECSPI_CONFIGREG_SS_POL bits are 0's after 
reset/power up (defaults).

Otherwise Acked-by: Marcin Krzemiński <address@hidden>

Thanks,
Marcin
>   }
>   
>   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)