Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Bin Meng]

On Thu, Jan 28, 2021 at 10:49 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/28/21 3:41 PM, Peter Maydell wrote:
> > On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > wrote:
> >> Oh I totally missed that :S
> >>
> >> Bin, I'd correct this as:
> >>
> >> - extract imx_spi_soft_reset(IMXSPIState *s) from imx_spi_reset()
> >> - zero-initialize CONREG in imx_spi_reset().
> >>
> >> static void imx_spi_soft_reset(IMXSPIState *s)
> >> {
> >>  ...
> >> }
> >>
> >> static void imx_spi_reset(DeviceState *dev)
> >> {
> >>     IMXSPIState *s = IMX_SPI(dev);
> >>
> >>     s->regs[ECSPI_CONREG] = 0;
> >>     imx_spi_soft_reset(s);
> >> }
> >>
> >> What do you think?
> >
> > That doesn't give you anywhere to put the imx_spi_update_irq()
> > call, which must happen only on soft reset and not on DeviceState
> > reset. You could do one of:
> >  * have a 'common reset' function that does most of this,
> >    plus an imx_spi_reset which clears CONREG and calls
> >    common reset and an imx_spi_soft_reset which calls
> >    common reset and imx_spi_update_irq()
> >  * have imx_spi_soft_reset save the old CONREG in a local
> >    variable before calling imx_spi_reset and then restore it
> >    to s->regs
>
> Long term maintenance I'd prefer the 'common reset' approach
> (but this is probably subjective to my view on the hardware,
> since this is software, the 2nd approach is also valid but
> harder to represent thinking of hardware).
>
> Bin, can you send a v9? (using the approach you prefer)
>

Yes, I will send a v9.

Thanks Peter for the explanation on the QOM device reset scenarios.

Regards,
Bin