[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value
Date: Thu, 28 Jan 2021 15:32:15 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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;

What do you think?


reply via email to

[Prev in Thread] Current Thread [Next in Thread]