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 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:
>>>
>>> On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>
>>>> When the block is disabled, all registers are reset with the
>>>> exception of the ECSPI_CONREG. It is initialized to zero
>>>> when the instance is created.
>>>>
>>>> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
>>>>      chapter 21.7.3: Control Register (ECSPIx_CONREG)
>>>
>>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>>>> index 8fb3c9b..c952a3d 100644
>>>> --- a/hw/ssi/imx_spi.c
>>>> +++ b/hw/ssi/imx_spi.c
>>>> @@ -231,12 +231,23 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>>>>  static void imx_spi_reset(DeviceState *dev)
>>>>  {
>>>>      IMXSPIState *s = IMX_SPI(dev);
>>>> +    int i;
>>>>
>>>>      DPRINTF("\n");
>>>>
>>>> -    memset(s->regs, 0, sizeof(s->regs));
>>>> -
>>>> -    s->regs[ECSPI_STATREG] = 0x00000003;
>>>> +    for (i = 0; i < ARRAY_SIZE(s->regs); i++) {
>>>> +        switch (i) {
>>>> +        case ECSPI_CONREG:
>>>> +            /* CONREG is not updated on reset */
>>>> +            break;
>>>> +        case ECSPI_STATREG:
>>>> +            s->regs[i] = 0x00000003;
>>>> +            break;
>>>> +        default:
>>>> +            s->regs[i] = 0;
>>>> +            break;
>>>> +        }
>>>> +    }
>>>
>>> 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."

Most of the codebase assumes QOM provides a zero-initialized
instance state. Do you think it should be explicit?

Regards,

Phil.