qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled
Date: Sat, 16 Jan 2021 16:21:28 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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.

> 
>> "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.

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.

Regards,

Phil.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]