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

[Philippe Mathieu-Daudé]

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.

Regards,

Phil.