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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut