Re: [PATCH v3 3/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Philippe Mathieu-Daudé]

On 1/9/21 1:35 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Usually the approach is that the device on the other end of the line
> is going to reset its state anyway, so there's no need to actively
> signal an irq line change during the reset hook.
> 
> Move imx_spi_update_irq() out of imx_spi_reset(), along with the
> disabling of chip selects, to a new function imx_spi_soft_reset()
> that is called when the controller is disabled.

Now I read this patch, forget my comment on previous patch.

> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v3:
> - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> 
>  hw/ssi/imx_spi.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 8d429e703f..880939f595 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -241,9 +241,20 @@ static void imx_spi_reset(DeviceState *dev)
>      imx_spi_rxfifo_reset(s);
>      imx_spi_txfifo_reset(s);
>  
> +    s->burst_length = 0;
> +}
> +
> +static void imx_spi_soft_reset(IMXSPIState *s)
> +{
> +    int i;
> +
> +    imx_spi_reset(DEVICE(s));

Hmm usually hard reset include soft reset.

> +
>      imx_spi_update_irq(s);
>  
> -    s->burst_length = 0;
> +    for (i = 0; i < ECSPI_NUM_CS; i++) {
> +        qemu_set_irq(s->cs_lines[i], 1);

Isn't this part of the hard reset?

> +    }
>  }
>  
>  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> @@ -351,12 +362,8 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
> uint64_t value,
>          s->regs[ECSPI_CONREG] = value;
>  
>          if (!imx_spi_is_enabled(s)) {
> -            /* device is disabled, so this is a reset */
> -            imx_spi_reset(DEVICE(s));
> -
> -            for (int i = 0; i < ECSPI_NUM_CS; i++) {
> -                qemu_set_irq(s->cs_lines[i], 1);
> -            }
> +            /* device is disabled, so this is a soft reset */
> +            imx_spi_soft_reset(s);

Maybe you can restructure patches 2/3, first introduce
imx_spi_soft_reset() - this patch - then fix ECSPI_CONREG
- the previous patch -.

>  
>              return;
>          }
>