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

[Bin Meng]

Hi Philippe,

On Sun, Jan 10, 2021 at 7:15 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/10/21 9:14 AM, 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(), to a new function
> > imx_spi_hard_reset() that is called when the controller is disabled.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> > Changes in v4:
> > - adujst the patch 2,3 order
> > - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
> >
> > Changes in v3:
> > - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> >
> >  hw/ssi/imx_spi.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index e605049a21..2c4c5ec1b8 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
> >      imx_spi_rxfifo_reset(s);
> >      imx_spi_txfifo_reset(s);
> >
> > -    imx_spi_update_irq(s);
> > -
> >      s->burst_length = 0;
> >  }
> >
> > +static void imx_spi_hard_reset(IMXSPIState *s)
> > +{
> > +    imx_spi_reset(DEVICE(s));
> > +
> > +    imx_spi_update_irq(s);
> > +}
> > +
> >  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> >      uint32_t value = 0;
> > @@ -351,8 +356,9 @@ 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));
> > +            /* device is disabled, so this is a hard reset */
> > +            imx_spi_hard_reset(s);
> > +
> >              return;
> >          }
> >
>
> Almost good :)
>
> DeviceReset handler is a hard reset, so you need:
>
>   dc->reset = imx_spi_hard_reset;
>
> Thus you also need this function prototype:
>
> void imx_spi_hard_reset(DeviceState *dev)
>
> Eventually renaming imx_spi_reset() -> imx_spi_soft_reset()
> will make things even easier.

Now I am confused. The v3 patch did the above already, but you
mentioned that usually the hard reset includes the soft reset. But in
the hard reset the imx_spi_update_irq() does not need to be called.

Regards,
Bin