| Message ID | 1611063546-20278-5-git-send-email-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 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. thanks -- PMM
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? Regards, Bin
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.) thanks -- PMM
On Thu, Jan 28, 2021 at 9:54 PM Peter Maydell <peter.maydell@linaro.org> 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.) I think that's ensured by object_initialize_with_type(). Regards, Bin
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.
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 thanks -- PMM
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; imx_spi_soft_reset(s); } What do you think? Phil.
On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > 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; > imx_spi_soft_reset(s); > } > > What do you think? That doesn't give you anywhere to put the imx_spi_update_irq() call, which must happen only on soft reset and not on DeviceState reset. You could do one of: * have a 'common reset' function that does most of this, plus an imx_spi_reset which clears CONREG and calls common reset and an imx_spi_soft_reset which calls common reset and imx_spi_update_irq() * have imx_spi_soft_reset save the old CONREG in a local variable before calling imx_spi_reset and then restore it to s->regs thanks -- PMM
On 1/28/21 3:41 PM, Peter Maydell wrote: > On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> 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; >> imx_spi_soft_reset(s); >> } >> >> What do you think? > > That doesn't give you anywhere to put the imx_spi_update_irq() > call, which must happen only on soft reset and not on DeviceState > reset. You could do one of: > * have a 'common reset' function that does most of this, > plus an imx_spi_reset which clears CONREG and calls > common reset and an imx_spi_soft_reset which calls > common reset and imx_spi_update_irq() > * have imx_spi_soft_reset save the old CONREG in a local > variable before calling imx_spi_reset and then restore it > to s->regs Long term maintenance I'd prefer the 'common reset' approach (but this is probably subjective to my view on the hardware, since this is software, the 2nd approach is also valid but harder to represent thinking of hardware). Bin, can you send a v9? (using the approach you prefer) Thanks, Phil.
On Thu, Jan 28, 2021 at 10:49 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 1/28/21 3:41 PM, Peter Maydell wrote: > > On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> 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; > >> imx_spi_soft_reset(s); > >> } > >> > >> What do you think? > > > > That doesn't give you anywhere to put the imx_spi_update_irq() > > call, which must happen only on soft reset and not on DeviceState > > reset. You could do one of: > > * have a 'common reset' function that does most of this, > > plus an imx_spi_reset which clears CONREG and calls > > common reset and an imx_spi_soft_reset which calls > > common reset and imx_spi_update_irq() > > * have imx_spi_soft_reset save the old CONREG in a local > > variable before calling imx_spi_reset and then restore it > > to s->regs > > Long term maintenance I'd prefer the 'common reset' approach > (but this is probably subjective to my view on the hardware, > since this is software, the 2nd approach is also valid but > harder to represent thinking of hardware). > > Bin, can you send a v9? (using the approach you prefer) > Yes, I will send a v9. Thanks Peter for the explanation on the QOM device reset scenarios. Regards, Bin
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; + } + } imx_spi_rxfifo_reset(s); imx_spi_txfifo_reset(s);