Hi Ben, On 1/12/21 2:22 PM, Bin Meng wrote: > On Tue, Jan 12, 2021 at 9:19 PM Peter Maydell <peter.mayd...@linaro.org> > wrote: >> >> On Tue, 12 Jan 2021 at 12:54, Bin Meng <bmeng...@gmail.com> wrote: >>> >>> On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.mayd...@linaro.org> >>> wrote: >>>> >>>> On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng...@gmail.com> wrote: >>>>> >>>>> From: Bin Meng <bin.m...@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.m...@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 >