Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Bin Meng]

Hi Philippe,

On Sun, Jan 17, 2021 at 12:12 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/16/21 4:59 PM, Bin Meng wrote:
> > Hi Philippe,
> >
> > On Sat, Jan 16, 2021 at 11:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > wrote:
> >>
> >> Hi Bin,
> >>
> >> On 1/16/21 2:57 PM, Bin Meng wrote:
> >>> On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> >>> wrote:
> >>>>
> >>>> When the block is disabled, only the ECSPI_CONREG register can
> >>>> be modified. Setting the EN bit enabled the device, clearing it
> >>>
> >>> I don't know how this conclusion came out. The manual only says the
> >>> following 2 registers ignore the write when the block is disabled.
> >>>
> >>> ECSPI_TXDATA, ECSPI_INTREG
> >>
> >> 21.4.5 Reset
> >>
> >>   Whenever a device reset occurs, a reset is performed on the
> >>   ECSPI, resetting all registers to their default values.
> >>
> >> My understanding is it is pointless to update them when the
> >> device is in reset, as they will get their default value when
> >> going out of reset.
> >
> > I have a different understanding. When ECSPI_CONREG[EN] is cleared,
> > it's like a hardware reset, and the ECSPI takes the following action:
> >
> >     "Whenever a device reset occurs, a reset is performed on the
> > ECSPI, resetting all registers to their default values."
> >
> > Chapter 21.4.5 Reset does not mention what's the hardware behavior 
> > afterwards.
> >
> > So my understanding is: afterwards, the software can still write to
> > various registers, unless the register description tells us it's
> > ignored.
> >
> >>
> >>>
> >>>> "disables the block and resets the internal logic with the
> >>>> exception of the ECSPI_CONREG" register.
> >>>>
> >>>> Move the imx_spi_is_enabled() check earlier.
> >>>>
> >>>> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
> >>>>      chapter 21.7.3: Control Register (ECSPIx_CONREG)
> >>>>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>> ---
> >>>>  hw/ssi/imx_spi.c | 26 ++++++++++++++++----------
> >>>>  1 file changed, 16 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> >>>> index ba7d3438d87..f06bbf317e2 100644
> >>>> --- a/hw/ssi/imx_spi.c
> >>>> +++ b/hw/ssi/imx_spi.c
> >>>> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr 
> >>>> offset, uint64_t value,
> >>>>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
> >>>>              (uint32_t)value);
> >>>>
> >>>> +    if (!imx_spi_is_enabled(s)) {
> >>>> +        /* Block is disabled */
> >>>> +        if (index != ECSPI_CONREG) {
> >>>> +            /* Ignore access */
> >>>> +            return;
> >>>> +        }
> >>>> +        s->regs[ECSPI_CONREG] = value;
> >>
> >>                                    [*]
> >>
> >>>> +        if (!(value & ECSPI_CONREG_EN)) {
> >>>> +            /* Keep disabled */
> >>>
> >>> So other bits except ECSPI_CONREG_EN are discarded? The manual does
> >>> not explicitly mention this but this looks suspicious.
> >>
> >> See in [*], all bits from the register are updated. We simply check
> >> ECSPI_CONREG_EN to see if we need to go out of reset.
> >
> > Oops, I missed the [*] line. Now I have read this carefully, and found
> > there is one problem:
> >
> > Now with the new logic the device reset activity has been postponed
> > until next time a device register is written. This is wrong.
>
> Yes, I just realized that in the imx_spi_read() function.
>
> >
> >>
> >> See:
> >>
> >> 21.5 Initialization
> >>
> >>   This section provides initialization information for ECSPI.
> >>
> >>   To initialize the block:
> >>
> >>   1. Clear the EN bit in ECSPI_CONREG to reset the block.
> >>   2. Enable the clocks for ECSPI.
> >>   3. Set the EN bit in ECSPI_CONREG to put ECSPI out of reset.
> >>   4. Configure corresponding IOMUX for ECSPI external signals.
> >>   5 Configure registers of ECSPI properly according to the
> >>     specifications of the external SPI device.
> >>
> >> And ECSPI_CONREG_EN bit description:
> >>
> >>   SPI Block Enable Control. This bit enables the ECSPI. This bit
> >>   must be set before writing to other registers or initiating an
> >>   exchange. Writing zero to this bit disables the block and resets
> >>   the internal logic with the exception of the ECSPI_CONREG. The
> >>   block's internal clocks are gated off whenever the block is
> >>   disabled.
> >>
> >>
> >> I simply wanted to help you. I don't want to delay your work, so
> >> if you think my approach is incorrect, suggest Peter to queue your
> >> v5 or resend it (once riscv-next is merged) as v8.
> >
> > Thank you for the help. I mentioned in an earlier thread before, that
> > my view was not to fix it until it's broken as the v5 series can
> > satisfy my work. But since you pointed out various spec violation
> > stuff related to device reset, I do think your findings make sense. So
> > let's improve this model together. :)
>
> I'm not mad, just I'm doing too many things and I should rather review
> your ssi-sd series. I don't have the physical hardware (neither know the
> firmware using it) so it is a bit dumb of me to code blindly with no
> possibility of testing. If you think this series is going the good way,
> it would be great if you can give it another try, and I will be happy
> to review.

Sure I will see if I can find a hardware to verify the register write
behavior when ECSPI is disabled.

Regards,
Bin