Re: [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 

 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 
>>>
>>> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 
>>
>> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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/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 
>
> 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.

>
>> 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.  In addition, this series
introduces the trigger-address DT that Linux introduced and is required by
the CQSPI on socfpga.

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?

--
Regards,
Jason

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Marek Vasut]

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 

 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.

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

> Just so we have a common understanding, the reverts along with the
> subsequent patches get the CQSPI device working again on the Altera SoC as
> it used to in previous versions of U-Boot, and the CQSPI should continue to
> work for the TI devices Vignesh's patches were intended to fix.
> 
> Regards,
> Jason
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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/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 
>>>
>>> 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?

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?

Just so we have a common understanding, the reverts along with the
subsequent patches get the CQSPI device working again on the Altera SoC as
it used to in previous versions of U-Boot, and the CQSPI should continue to
work for the TI devices Vignesh's patches were intended to fix.

Regards,
Jason

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Marek Vasut]

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 


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) ?


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/listinfo/u-boot

Re: [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/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 
> 
> 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?

--
Regards,
Jason
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/listinfo/u-boot

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

[Marek Vasut]

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 


Do we really need the reverts or can we just fix the commit(s) up somehow ?


---
Changed in v2: None

 drivers/spi/cadence_qspi_apb.c | 22 ++
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index a6787c3b47..df6a91fc9f 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -633,8 +633,6 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
 {
unsigned int remaining = n_rx;
unsigned int bytes_to_read = 0;
-   struct bounce_buffer bb;
-   u8 *bb_rxbuf;
int ret;

writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES);
@@ -643,11 +641,6 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
writel(CQSPI_REG_INDIRECTRD_START,
   plat->regbase + CQSPI_REG_INDIRECTRD);

-   ret = bounce_buffer_start(, (void *)rxbuf, n_rx, GEN_BB_WRITE);
-   if (ret)
-   return ret;
-   bb_rxbuf = bb.bounce_buffer;
-
while (remaining > 0) {
ret = cadence_qspi_wait_for_data(plat);
if (ret < 0) {
@@ -661,13 +654,12 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
bytes_to_read *= CQSPI_FIFO_WIDTH;
bytes_to_read = bytes_to_read > remaining ?
remaining : bytes_to_read;
-   readsl(plat->ahbbase, bb_rxbuf, bytes_to_read >> 2);
-   if (bytes_to_read % 4)
-   readsb(plat->ahbbase,
-  bb_rxbuf + rounddown(bytes_to_read, 4),
-  bytes_to_read % 4);
-
-   bb_rxbuf += bytes_to_read;
+   /* Handle non-4-byte aligned access to avoid data 
abort. */
+   if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4))
+   readsb(plat->ahbbase, rxbuf, bytes_to_read);
+   else
+   readsl(plat->ahbbase, rxbuf, bytes_to_read >> 
2);
+   rxbuf += bytes_to_read;
remaining -= bytes_to_read;
bytes_to_read = cadence_qspi_get_rd_sram_level(plat);
}
@@ -684,7 +676,6 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
/* Clear indirect completion status */
writel(CQSPI_REG_INDIRECTRD_DONE,
   plat->regbase + CQSPI_REG_INDIRECTRD);
-   bounce_buffer_stop();

return 0;

@@ -692,7 +683,6 @@ failrd:
/* Cancel the indirect read */
writel(CQSPI_REG_INDIRECTRD_CANCEL,
   plat->regbase + CQSPI_REG_INDIRECTRD);
-   bounce_buffer_stop();
return ret;
 }




___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/listinfo/u-boot