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

[Philippe Mathieu-Daudé]

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)

Thanks,

Phil.