[U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"
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@gd-ms.com --- 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(&bb, (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(&bb);
return 0;
@@ -692,7 +683,6 @@ failrd: /* Cancel the indirect read */ writel(CQSPI_REG_INDIRECTRD_CANCEL, plat->regbase + CQSPI_REG_INDIRECTRD); - bounce_buffer_stop(&bb); return ret; }
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@gd-ms.com
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(&bb, (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);elsereadsl(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(&bb);
return 0;
@@ -692,7 +683,6 @@ failrd: /* Cancel the indirect read */ writel(CQSPI_REG_INDIRECTRD_CANCEL, plat->regbase + CQSPI_REG_INDIRECTRD);
- bounce_buffer_stop(&bb); return ret;
}
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@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?
-- Regards, Jason
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@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) ?
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@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?
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
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@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.
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
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@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.
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
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@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.
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@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
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@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 ...
participants (2)
-
Marek Vasut -
Rush, Jason A.