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

[Bin Meng]

On Tue, Jan 12, 2021 at 9:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 12 Jan 2021 at 12:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> 
> > wrote:
> > >
> > > On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> 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;
> > > >          }
> > >
> > > The function of the code is correct, but you seem to have the function
> > > naming backwards here. Generally:
> > >  * soft reset == the reset triggered by the register write
> > >  * hard reset == power-on reset == the dc->reset function
> > >
> > > I think this is what Philippe was trying to say.
> >
> > Philippe said: "Hmm usually hard reset include soft reset."
>
> True in hardware, but for QEMU there are some things we don't
> want to do in what we would call a hard or power-on reset.
>

OK, will revert to use imx_spi_soft_reset().

> > Since we are moving imx_spi_update_irq() out of imx_spi_reset() to a
> > new function called imx_spi_soft_reset() (what I did in v3), that
> > confused him (and I felt the same thing), so I renamed
> > imx_spi_soft_reset() to imx_spi_hard_reset() in v4..

Regards,
Bin