[U-Boot] [PATCH] spi: cadence_qspi: Fix the indirect ahb trigger address setting

[Graham Moore]

Hello,

OK, first of all, I'm not the original author, just the one who has to 
fix it nowadays.  There are many things that have confused me in this 
driver.

The mask issue seems familiar, let me look at it, and I'll get back to you.

-Graham

On 06/02/2015 11:11 AM, Vikas MANOCHA wrote:
> Hi Graham,
>
>> -----Original Message-----
>> From: Stefan Roese [mailto:sr at denx.de]
>> Sent: Monday, June 01, 2015 4:34 AM
>> To: Vikas MANOCHA; u-boot at lists.denx.de; Graham Moore
>> Cc: Dinh Nguyen; Marek Vasut
>> Subject: Re: [PATCH] spi: cadence_qspi: Fix the indirect ahb trigger address
>> setting
>>
>> Hi Vikas,
>>
>> On 29.05.2015 18:53, vikasm wrote:
>>> On 05/28/2015 11:20 PM, Stefan Roese wrote:
>>>> Hi Vikas,
>>>>
>>>> (added Dinh, Graham and Marek to Cc)
>>>>
>>>> On 29.05.2015 03:22, Vikas Manocha wrote:
>>>>> Trigger base address can be set to the spi flash address without any
>>>>> masking, here is the explanation of the register.
>>>>>
>>>>> QSPI_IND_AHB_ADDR_TRIGGER :
>>>>> Trigger Address is the base address that is used by the AHB
>>>>> controller for indirect accesses. When the incoming AHB access
>>>>> address matches a range of addresses from this trigger address to
>>>>> the trigger address + 15, then the AHB request is completed by
>> fetching/storing data from/to the Controllers SRAM.
>>>>>
>>>>> Signed-off-by: Vikas Manocha <vikas.manocha at st.com>
>>>>> ---
>>>>>     drivers/spi/cadence_qspi_apb.c |    7 ++-----
>>>>>     1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>>>> b/drivers/spi/cadence_qspi_apb.c index 00a115f..855e5c7 100644
>>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>>> @@ -50,7 +50,6 @@
>>>>>     #define CQSPI_INST_TYPE_QUAD			(2)
>>>>>
>>>>>     #define CQSPI_STIG_DATA_LEN_MAX			(8)
>>>>> -#define CQSPI_INDIRECTTRIGGER_ADDR_MASK		(0xFFFFF)
>>>>>
>>>>>     #define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
>>>>>     #define CQSPI_DUMMY_BYTES_MAX			(4)
>>>>> @@ -697,8 +696,7 @@ int
>> cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
>>>>>     		addr_bytes = cmdlen - 1;
>>>>>
>>>>>     	/* Setup the indirect trigger address */
>>>>> -	writel(((u32)plat->ahbbase &
>> CQSPI_INDIRECTTRIGGER_ADDR_MASK),
>>>>> -	       plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>>>>> +	writel((u32)plat->ahbbase, plat->regbase +
>>>>> +CQSPI_REG_INDIRECTTRIGGER);
>>>>>
>>>>>     	/* Configure SRAM partition for read. */
>>>>>     	writel(CQSPI_REG_SRAM_PARTITION_RD, plat->regbase + @@ -
>> 798,8
>>>>> +796,7 @@ int cadence_qspi_apb_indirect_write_setup(struct
>> cadence_spi_platdata *plat,
>>>>>     		return -EINVAL;
>>>>>     	}
>>>>>     	/* Setup the indirect trigger address */
>>>>> -	writel(((u32)plat->ahbbase &
>> CQSPI_INDIRECTTRIGGER_ADDR_MASK),
>>>>> -	       plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>>>>> +	writel((u32)plat->ahbbase, plat->regbase +
>>>>> +CQSPI_REG_INDIRECTTRIGGER);
>>>> Might I ask on which SoC you are using this driver? Which problem are
>>>> you experiencing exactly?
>>>
>>> I am using ST's soc stv0991. The spi flash memory is mapped at address
>>> 0x4000_0000. By Masking with 0xF_FFFF, the value to be loaded in the
>>> indirect trigger register becomes zero & then spi read/write does not
>>> work. By removing or changing the mask to 0xFFFF_FFFF, the loaded
>>> value in indirect trigger register is 0x4000_0000 & spi read/write works fine.
>>>
>>> It should be same of socfpga also after masking with 0xF_FFFF,
>>> wondering how it works there.
>>
>> Yes, I'm wondering a bit myself. Thats why I added Graham to Cc as he is the
>> author of the original driver. And knows this far better than I do.
>>
>>> No masking is done in the driver while polling fifo for read/write,
>>> plat->ahbbase was used directly. Any idea why this masking was done
>>> for ahb trigger base register?
>>
>> Graham? Do you have any insight here please?
>
> Any idea ?
>
> Rgds,
> Vikas
>
>>
>> Thanks,
>> Stefan
>