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

[Marek Vasut]

On 03/16/2017 11:11 PM, Rush, Jason A. wrote:
> 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.

Why don't you just gain that experience ? You can ask questions on the
list ...

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

I think that's still an improvement ...

-- 
Best regards,
Marek Vasut