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:22 PM, Peter Maydell wrote:
> On Thu, 28 Jan 2021 at 14:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 1/28/21 2:54 PM, Peter Maydell wrote:
>>> On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> 
>>>> wrote:
>>>>> This retains the CONREG register value for both:
>>>>>  * 'soft' reset caused by write to device register to disable
>>>>>    the block
>>>>>    -- this is corrcet as per the datasheet quote
>>>>>  * 'power on' reset via TYPE_DEVICE's reset method
>>>>>    -- but in this case we should reset CONREG, because the Device
>>>>>    reset method is like a complete device powercycle and should
>>>>>    return the device state to what it was when QEMU was first
>>>>>    started.
>>>>
>>>> The POR value of CONREG is zero, which should be the default value, no?
>>>
>>> But you're not setting it to zero here, you're leaving it with
>>> whatever value it had before. (That's correct for soft reset,
>>> but wrong for power-on.)
>>
>> zero value on power-on is what I tried to describe as
>> "It is initialized to zero when the instance is created."
> 
> Yes, but QOM device reset does not happen just once at startup and
> not thereafter. Consider:
> 
>  * user starts QEMU
>  * QOM devices are created and realized
>  * QOM device reset happens
>      -- CONREG is zero here because QOM structs are zero-initialized
>  * guest runs
>  * guest modifies CONREG from its initial value
>  * system reset is requested (perhaps by user, perhaps by
>    guest writing some register or another)
>  * QOM device reset happens
>      -- CONREG is not zero here, so reset must clear it

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?

Phil.