Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Philippe Mathieu-Daudé]

Hi Ben,

On 1/12/21 2:22 PM, Bin Meng wrote:
> On Tue, Jan 12, 2021 at 9:19 PM Peter Maydell <peter.maydell@linaro.org> 
> wrote:
>>
>> On Tue, 12 Jan 2021 at 12:54, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> 
>>> wrote:
>>>>
>>>> On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>
>>>>> From: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> Usually the approach is that the device on the other end of the line
>>>>> is going to reset its state anyway, so there's no need to actively
>>>>> signal an irq line change during the reset hook.
>>>>>
>>>>> Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
>>>>> imx_spi_hard_reset() that is called when the controller is disabled.
>>>>>
>>>>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v4:
>>>>> - adujst the patch 2,3 order
>>>>> - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
>>>>>
>>>>> Changes in v3:
>>>>> - new patch: remove imx_spi_update_irq() in imx_spi_reset()
>>>>>
>>>>>  hw/ssi/imx_spi.c | 14 ++++++++++----
>>>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>>>>> index e605049a21..2c4c5ec1b8 100644
>>>>> --- a/hw/ssi/imx_spi.c
>>>>> +++ b/hw/ssi/imx_spi.c
>>>>> @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
>>>>>      imx_spi_rxfifo_reset(s);
>>>>>      imx_spi_txfifo_reset(s);
>>>>>
>>>>> -    imx_spi_update_irq(s);
>>>>> -
>>>>>      s->burst_length = 0;
>>>>>  }
>>>>>
>>>>> +static void imx_spi_hard_reset(IMXSPIState *s)
>>>>> +{
>>>>> +    imx_spi_reset(DEVICE(s));
>>>>> +
>>>>> +    imx_spi_update_irq(s);
>>>>> +}
>>>>> +
>>>>>  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
>>>>>  {
>>>>>      uint32_t value = 0;
>>>>> @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr 
>>>>> offset, uint64_t value,
>>>>>          s->regs[ECSPI_CONREG] = value;
>>>>>
>>>>>          if (!imx_spi_is_enabled(s)) {
>>>>> -            /* device is disabled, so this is a reset */
>>>>> -            imx_spi_reset(DEVICE(s));
>>>>> +            /* device is disabled, so this is a hard reset */
>>>>> +            imx_spi_hard_reset(s);
>>>>> +
>>>>>              return;
>>>>>          }
>>>>
>>>> The function of the code is correct, but you seem to have the function
>>>> naming backwards here. Generally:
>>>>  * soft reset == the reset triggered by the register write
>>>>  * hard reset == power-on reset == the dc->reset function
>>>>
>>>> I think this is what Philippe was trying to say.
>>>
>>> Philippe said: "Hmm usually hard reset include soft reset."
>>
>> True in hardware, but for QEMU there are some things we don't
>> want to do in what we would call a hard or power-on reset.

Sorry for the confusion. I guess you understood me well, but
I was wrong. Anyhow I'll try to sort this discussion out with
my English teacher so the next time such confusion doesn't
happen again.

Thanks,

Phil.

> OK, will revert to use imx_spi_soft_reset().
> 
>>> Since we are moving imx_spi_update_irq() out of imx_spi_reset() to a
>>> new function called imx_spi_soft_reset() (what I did in v3), that
>>> confused him (and I felt the same thing), so I renamed
>>> imx_spi_soft_reset() to imx_spi_hard_reset() in v4..
> 
> Regards,
> Bin
>