[U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

I don't know if this message successfully sent last weekend.  My mail server was undergoing maintenance, and I didn't see my original message on the u-boot mailing list archive.  So I'm resending, my apologies if this is a repost.

R, Vignesh wrote:
> On 2/25/2017 1:25 AM, Rush, Jason A. wrote:
>> R, Vignesh wrote:
>>> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>>>> On 02/23/2017 08:22 PM, Rush, Jason A. wrote:
>>>>> Marek Vasut wrote:
>>>>>> On 02/22/2017 06:37 PM, Rush, Jason A. wrote:
>>>>>>> Marek Vasut wrote:
>>>>>>>> On 02/21/2017 05:50 PM, Rush, Jason A. wrote:
>>> [...]
>>>>>
>>>
>>> You could try reverting my commits:
>>> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
>>> Use 32 bit indirect write transaction when possible
>>> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
>>> Use 32 bit indirect read transaction when possible
>>>
>>
>> When I reverted these two commits and added my patch for the indirect
>> trigger_address, it works correctly.
>>
> 
> Oops, these patches are required as Cadence QSPI controller(I am not
> sure if all versions of IP are newer versions only) has a limitation
> that the external master is only permitted to issue 32-bit data
> interface reads until the last word of an indirect transfer.
> 
> 
>> Also, when I disabled the dcache (dcache off) as Marek suggested, it works
>> correctly when running from the master branch (again with my indirect
>> trigger_address patch).
>>
> 
> Just that I understand correctly, with latest master(with no patches
> reverted) + your patch for indirect trigger_address + dcache off, you
> don't see any problem?
> 

Correct.

>>>
>>> But there were other patches by others in v2017.01-rc1, like
>>> spi: cadence_qspi: Fix CS timings which may have impact.
>>>
>>
>> I left all other commits in except the two Vignesh suggested to revert, so
>> it seems to be related to those two commits and caching.  As another data
>> point, I can load and boot linux with caching on from another source (MMC).
>> So I don't think it's a problem with memory/caching in general.
>>
>> Any suggestions on how to proceed from here?
>>
> 
> My patches use common bounce_buffer implementation which does dcache
> flush/invalidate and if dcache has issues then I guess those operations
> may be causing data corruption. Could you do a bit more research for me?
> 
> 1. As a hack, could you just disable dcache operations in bounce_buffer
> implementation? Here is the diff for the same:
> 

This works.  So it does seem to be an issue with the CQSPI and dcache on
the Altera SoC.

> 
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index 054d9e0302cc..2878b9eed1ae 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
[...]
> 
> 2. If that works, I guess there is some issue wrt CQSPI and dcache on your platform,
> I suggest you to revert my above two patches and try non bounce buffer version of
> my changes here: https://patchwork.ozlabs.org/patch/693069/.
> This patch takes care of indirect write. I don't have similar patch for
> indirect read but that wasn't required.
> 

This also works.

Marek - how do you feel about a patch series with the following:

1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
     spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible
2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
     spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/
     (should I keep Vignesh as the signed-off?)
4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark
     them all as __iomem
5. Load the indirect-trigger property from the DT as a u32.  I think this still
     needs to be a u32 because it's used in a writel(...) 32-bit write call in
     cadence_qspi_apb.c
6. Add indirect-trigger to dts files that use cadence driver

I think that captures all the changes needed.

Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
(e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for
the same properties.  Do you want the 'cdns,' prefix on trigger-address? 
If so, do you want me to rename all the other properties to align them with
the Linux DT as an additional patch?

--
Regards,
Jason