Hi Philippe,
On Sun, Jan 10, 2021 at 7:53 AM Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
>
> On 1/9/21 1:35 PM, Bin Meng 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(), along with the
> > disabling of chip selects, to a new function imx_spi_soft_reset()
> > that is called when the controller is disabled.
>
> Now I read this patch, forget my comment on previous patch.
>
> >
> > Signed-off-by: Bin Meng <bin.m...@windriver.com>
> >
> > ---
> >
> > Changes in v3:
> > - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> >
> > hw/ssi/imx_spi.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index 8d429e703f..880939f595 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -241,9 +241,20 @@ static void imx_spi_reset(DeviceState *dev)
> > imx_spi_rxfifo_reset(s);
> > imx_spi_txfifo_reset(s);
> >
> > + s->burst_length = 0;
> > +}
> > +
> > +static void imx_spi_soft_reset(IMXSPIState *s)
> > +{
> > + int i;
> > +
> > + imx_spi_reset(DEVICE(s));
>
> Hmm usually hard reset include soft reset.
That's my understanding as well.
>
> > +
> > imx_spi_update_irq(s);
> >
> > - s->burst_length = 0;
> > + for (i = 0; i < ECSPI_NUM_CS; i++) {
> > + qemu_set_irq(s->cs_lines[i], 1);
>
> Isn't this part of the hard reset?
>
I think we can rename the name to imx_spi_hard_reset() to avoid such confusion.
> > + }
> > }
> >
> > static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> > @@ -351,12 +362,8 @@ 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));
> > -
> > - for (int i = 0; i < ECSPI_NUM_CS; i++) {
> > - qemu_set_irq(s->cs_lines[i], 1);
> > - }
> > + /* device is disabled, so this is a soft reset */
> > + imx_spi_soft_reset(s);
>
> Maybe you can restructure patches 2/3, first introduce
> imx_spi_soft_reset() - this patch - then fix ECSPI_CONREG
> - the previous patch -.
Sure.
Regards,
Bin
