| Message ID | 20210110081429.10126-3-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ssi: imx_spi: Fix various bugs in the imx_spi model | expand |
On 1/10/21 9:14 AM, Bin Meng 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; > } > Almost good :) DeviceReset handler is a hard reset, so you need: dc->reset = imx_spi_hard_reset; Thus you also need this function prototype: void imx_spi_hard_reset(DeviceState *dev) Eventually renaming imx_spi_reset() -> imx_spi_soft_reset() will make things even easier. Regards, Phil.
Hi Philippe, On Sun, Jan 10, 2021 at 7:15 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 1/10/21 9:14 AM, Bin Meng 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; > > } > > > > Almost good :) > > DeviceReset handler is a hard reset, so you need: > > dc->reset = imx_spi_hard_reset; > > Thus you also need this function prototype: > > void imx_spi_hard_reset(DeviceState *dev) > > Eventually renaming imx_spi_reset() -> imx_spi_soft_reset() > will make things even easier. Now I am confused. The v3 patch did the above already, but you mentioned that usually the hard reset includes the soft reset. But in the hard reset the imx_spi_update_irq() does not need to be called. Regards, Bin
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. thanks -- PMM
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." 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
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. > 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.. thanks -- PMM
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. > 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
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 >
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; }