[U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"

[Rush, Jason A.]

Marek Vasut wrote:
> On 03/14/2017 03:23 PM, Rush, Jason A. wrote:
>> Marek Vasut wrote:
>>> On 03/07/2017 04:18 PM, Rush, Jason A. wrote:
>>>> Marek Vasut wrote:
>>>>> On 03/03/2017 04:17 PM, Rush, Jason A. wrote:
>>>>>> Marek Vasut wrote:
>>>>>>> On 03/01/2017 05:36 PM, Rush, Jason A. wrote:
>>>>>>>> This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f.
>>>>>>>>
>>>>>>>> The Cadence QSPI device does not work with caching (introduced with
>>>>>>>> the bounce buffer in this commit) on the Altera SoC platform.
>>>>>>>>
>>>>>>>> Signed-off-by: Jason A. Rush <jason.rush at gd-ms.com>
>>>>>>>
>>>>>>> Do we really need the reverts or can we just fix the commit(s) up somehow ?
>>>>>>>
>>>>>>
>>>>>> Would you prefer I squash the 2 reverts and the subsequent patch together
>>>>>> as a single commit?
>>>>>
>>>>> I would prefer if you answered my question :) So let me re-iterate, can
>>>>> we incrementally fix the driver instead of doing the revert(s) ?
>>>>
>>>> I think I misunderstood your question.  Could you clarify what you mean by
>>>> incrementally fix the driver?
>>>
>>> That means change it to the final form without reverting patches.
>>
>> Yes, I can definitely change this so it does not revert the patches.
>
> OK
>
>>>> Are you asking if there is a way to fix the cache issue with the CQSPI on the
>>>> Altera SoC platform?  If so, I don't know if I have the knowledge to answer that.
>>>> Do you have any suggestions on where one would start looking to fix the
>>>> caching problem?
>>>
>>> Uh, there is a cache problem on socfpga ? Is this series working around
>>> it then ?
>>>
>>
>> Yes, there seems to be a cache issue with the CQSPI on the socfpga.  And yes,
>> this patch series is working around this cache issue.
>
> OK, that means I'm not accepting this series. Let's fix the cache issue
> properly.
>
>> In addition, this series
>> introduces the trigger-address DT that Linux introduced and is required by
>> the CQSPI on socfpga.
>
> Can we split this part from the patchset, so it can go in separately ?
>
>> Here's a quick recap of the cache issue and this patch series... The original
>> commits used a bounce buffer implementation to fix 32-bit read alignment
>> issues with the CQSPI on a TI SoC device.  The bounce buffer implementation
>> was a clean solution to the 32-bit alignment issue, but this implementation
>> returned random data from the QSPI flash on socfpga.  You suggested I try
>> disabling the dcache, as you recalled some caching problem on the CQSPI in
>> the past.  Running 'dcache off' solved the random data issue on socfpga
>> while using the bounce buffer implementation.
>>
>> That's when Vignesh and I decided to work around the cache problem by
>> reverting the two original commits and using a previous patch Vignesh had
>> written to fix the alignment problem.
>>
>> If I clean the series up to change it to the final form and not revert the
>> patches, is this an acceptable approach?
>
> No, if we know there is a bug and we only pile workarounds, we will end
> up with shitty code. Let's fix this properly please.
>

I agree a workaround is no good.  I have no knowledge or experience with
fixing a cache problem though, so I think someone else is going to have to
tackle this bug.

If you'd like, I can submit a separate patch for adopting the Linux DT
trigger-address property, but the CQSPI will still be broken for the socfpga
until someone fixes the cache problem with the CQSPI.

--
Regards,
Jason