Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On Fri, 05/01/2018, Marek Vasut wrote:
On 01/05/2018 08:31 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018 Marek Vasut wrote:
On 01/05/2018 04:49 PM, Goldschmidt Simon wrote:
<snip>
OK, so I need these patches to get qspi work on socfpga:
- Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: https://patchwork.ozlabs.org/project/uboot/list/?series=13864
- Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible" (v2) https://patchwork.ozlabs.org/patch/838871/
I've waited for ack/tested-by from marek or someone who usually worked on these cadence.
Vignesh acked, who already did some of the last changes. But Ok, I've added Marek to the loop.
Marek, do you see any problems here? Are you running QSPI on the socfpga platform anywhere?
I am not entirely sure what this partial thread is all about, do you need some patches Acked ? Repost them including the Acks collected already and CC me, I want to review them. PW link is not enough.
Marek, you were one of the people addressed in "to:" when Jason Rush sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" and successors on Nov. 16th 2017.
You were also in the "to:" field when I sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" on Nov. 17th 2017.
Should I resend them anyway?
I really dunno what to make of this sparse thread, sorry. Also, I don't quite remember what happened in this thread on November 17th, sorry.
So maybe you should elaborate what you expect me to do ... or just resend the patches, yes.
OK, I'll resend the patches then. I'll try to combine the 2 series into one but have to check with Jason first.
Thanks, Simon
On 01/06/2018 02:39 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018, Marek Vasut wrote:
On 01/05/2018 08:31 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018 Marek Vasut wrote:
On 01/05/2018 04:49 PM, Goldschmidt Simon wrote:
<snip>
> OK, so I need these patches to get qspi work on socfpga: > > - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: > https://patchwork.ozlabs.org/project/uboot/list/?series=13864 > - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible" (v2) > https://patchwork.ozlabs.org/patch/838871/
I've waited for ack/tested-by from marek or someone who usually worked on these cadence.
Vignesh acked, who already did some of the last changes. But Ok, I've added Marek to the loop.
Marek, do you see any problems here? Are you running QSPI on the socfpga platform anywhere?
I am not entirely sure what this partial thread is all about, do you need some patches Acked ? Repost them including the Acks collected already and CC me, I want to review them. PW link is not enough.
Marek, you were one of the people addressed in "to:" when Jason Rush sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" and successors on Nov. 16th 2017.
You were also in the "to:" field when I sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" on Nov. 17th 2017.
Should I resend them anyway?
I really dunno what to make of this sparse thread, sorry. Also, I don't quite remember what happened in this thread on November 17th, sorry.
So maybe you should elaborate what you expect me to do ... or just resend the patches, yes.
OK, I'll resend the patches then. I'll try to combine the 2 series into one but have to check with Jason first.
Super, thanks. We really need to sort this QSPI stuff out, it's been going on for way too long.
Just make sure to submit it soon, MW opens on the 8th.
On 1/6/2018 9:42 AM, Marek Vasut wrote:
On 01/06/2018 02:39 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018, Marek Vasut wrote:
On 01/05/2018 08:31 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018 Marek Vasut wrote:
On 01/05/2018 04:49 PM, Goldschmidt Simon wrote:
<snip>
>> OK, so I need these patches to get qspi work on socfpga: >> >> - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: >> https://patchwork.ozlabs.org/project/uboot/list/?series=13864 >> - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible" (v2) >> https://patchwork.ozlabs.org/patch/838871/ > I've waited for ack/tested-by from marek or someone who usually worked > on these cadence. Vignesh acked, who already did some of the last changes. But Ok, I've added Marek to the loop.
Marek, do you see any problems here? Are you running QSPI on the socfpga platform anywhere?
I am not entirely sure what this partial thread is all about, do you need some patches Acked ? Repost them including the Acks collected already and CC me, I want to review them. PW link is not enough.
Marek, you were one of the people addressed in "to:" when Jason Rush sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" and successors on Nov. 16th 2017.
You were also in the "to:" field when I sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" on Nov. 17th 2017.
Should I resend them anyway?
I really dunno what to make of this sparse thread, sorry. Also, I don't quite remember what happened in this thread on November 17th, sorry.
So maybe you should elaborate what you expect me to do ... or just resend the patches, yes.
OK, I'll resend the patches then. I'll try to combine the 2 series into one but have to check with Jason first.
Super, thanks. We really need to sort this QSPI stuff out, it's been going on for way too long.
Just make sure to submit it soon, MW opens on the 8th.
There was a minor upstream change to one of the files since I submitted v4 of my cadence device-tree patchset, so I rebased and resent the patchset as a v5. I included everyone that was originally involved with the patches and added a CC for Marek.
This is only the patchset for the device tree changes for the cadence qspi driver, Simon will still have to add the patch that fixes the cache invalidation bug in the cadence driver.
-- Jason
On 01/06/2018 07:46 PM, Jason Rush wrote:
On 1/6/2018 9:42 AM, Marek Vasut wrote:
On 01/06/2018 02:39 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018, Marek Vasut wrote:
On 01/05/2018 08:31 PM, Goldschmidt Simon wrote:
On Fri, 05/01/2018 Marek Vasut wrote:
On 01/05/2018 04:49 PM, Goldschmidt Simon wrote:
<snip>
>>> OK, so I need these patches to get qspi work on socfpga: >>> >>> - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: >>> https://patchwork.ozlabs.org/project/uboot/list/?series=13864 >>> - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible" (v2) >>> https://patchwork.ozlabs.org/patch/838871/ >> I've waited for ack/tested-by from marek or someone who usually worked >> on these cadence. > Vignesh acked, who already did some of the last changes. But Ok, I've > added Marek to the loop. > > Marek, do you see any problems here? Are you running QSPI on the > socfpga platform anywhere? I am not entirely sure what this partial thread is all about, do you need some patches Acked ? Repost them including the Acks collected already and CC me, I want to review them. PW link is not enough.
Marek, you were one of the people addressed in "to:" when Jason Rush sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" and successors on Nov. 16th 2017.
You were also in the "to:" field when I sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" on Nov. 17th 2017.
Should I resend them anyway?
I really dunno what to make of this sparse thread, sorry. Also, I don't quite remember what happened in this thread on November 17th, sorry.
So maybe you should elaborate what you expect me to do ... or just resend the patches, yes.
OK, I'll resend the patches then. I'll try to combine the 2 series into one but have to check with Jason first.
Super, thanks. We really need to sort this QSPI stuff out, it's been going on for way too long.
Just make sure to submit it soon, MW opens on the 8th.
There was a minor upstream change to one of the files since I submitted v4 of my cadence device-tree patchset, so I rebased and resent the patchset as a v5. I included everyone that was originally involved with the patches and added a CC for Marek.
This is only the patchset for the device tree changes for the cadence qspi driver, Simon will still have to add the patch that fixes the cache invalidation bug in the cadence driver.
Sigh, can we get a single patchset out which fixes the problem ?
I mean, if I understand this correctly, you're all addressing one single problem, but with two patchsets, yes ?
On 1/6/2018 1:29 PM, Marek Vasut wrote:
On 01/06/2018 07:46 PM, Jason Rush wrote:
On 1/6/2018 9:42 AM, Marek Vasut wrote:
<snip>
There was a minor upstream change to one of the files since I submitted v4 of my cadence device-tree patchset, so I rebased and resent the patchset as a v5. I included everyone that was originally involved with the patches and added a CC for Marek.
This is only the patchset for the device tree changes for the cadence qspi driver, Simon will still have to add the patch that fixes the cache invalidation bug in the cadence driver.
Sigh, can we get a single patchset out which fixes the problem ?
I mean, if I understand this correctly, you're all addressing one single problem, but with two patchsets, yes ?
Well... The one issue we're trying to fix is that the cadence QSPI hasn't worked on the socfpga arch since late 2016. However, it's two different issues that have caused this bigger problem:
1. The indaddrtrig register was being programmed with an incorrect value for socfpga as the result of assuming it should be programmed with the same address as the ahbbase address. This issue is resolved by adopting the Linux DT bindings, which has an independent setting for the indaddrtrig register so the register can be set correctly on all architectures. Plus it aligns the DT between u-boot and Linux.
2. The cadence driver was modified at one point to use the bouncebuf functions to fix an issue on a TI architecture that expected, where if I recall correctly all reads except the last have to be 32-bit reads. However, since the bouncebuf was designed for DMA transfers, it invalidates the data cache after reading, but since the cadence is using cpu transfers the newly read data is thrown away when the cache is invalidated. This issue is resolved by reverting the commit that introduced using the bounce buffer for read operations, which according to Vignesh don't cause any issues to the TI architecture.
So, would you prefer two patchsets or one that fixes both issues? If you'd like just one, Simon can collapse my patches along with his revert patch into a single patchset and resubmit.
-- Jason
On 01/06/2018 10:09 PM, Jason Rush wrote:
On 1/6/2018 1:29 PM, Marek Vasut wrote:
On 01/06/2018 07:46 PM, Jason Rush wrote:
On 1/6/2018 9:42 AM, Marek Vasut wrote:
<snip>
There was a minor upstream change to one of the files since I submitted v4 of my cadence device-tree patchset, so I rebased and resent the patchset as a v5. I included everyone that was originally involved with the patches and added a CC for Marek.
This is only the patchset for the device tree changes for the cadence qspi driver, Simon will still have to add the patch that fixes the cache invalidation bug in the cadence driver.
Sigh, can we get a single patchset out which fixes the problem ?
I mean, if I understand this correctly, you're all addressing one single problem, but with two patchsets, yes ?
Well... The one issue we're trying to fix is that the cadence QSPI hasn't worked on the socfpga arch since late 2016. However, it's two different issues that have caused this bigger problem:
- The indaddrtrig register was being programmed with an incorrect value for socfpga
as the result of assuming it should be programmed with the same address as the ahbbase address. This issue is resolved by adopting the Linux DT bindings, which has an independent setting for the indaddrtrig register so the register can be set correctly on all architectures. Plus it aligns the DT between u-boot and Linux.
That should be an easy patch, so this is the patchset 0/5..5/5 that you just submitted ?
- The cadence driver was modified at one point to use the bouncebuf functions to fix
an issue on a TI architecture that expected, where if I recall correctly all reads except the last have to be 32-bit reads. However, since the bouncebuf was designed for DMA transfers, it invalidates the data cache after reading, but since the cadence is using cpu transfers the newly read data is thrown away when the cache is invalidated. This issue is resolved by reverting the commit that introduced using the bounce buffer for read operations, which according to Vignesh don't cause any issues to the TI architecture.
Hmmmmm, I wonder why you need bounce buffer at all here. The CQSPI literally reads/writes a register space (or some FIFO in register space), there is no DMA involved at all. I also wonder why we have to manipulate with cache at all here.
So, would you prefer two patchsets or one that fixes both issues? If you'd like just one, Simon can collapse my patches along with his revert patch into a single patchset and resubmit.
If 1) is fixed by this patchset 0/5..5/5 and 2) is fixed by some other patchset, then that's fine as is. Thanks for explaining it like so, it really did help.
-- Jason
On 1/7/2018 5:39 AM, Marek Vasut wrote:
On 01/06/2018 10:09 PM, Jason Rush wrote:
On 1/6/2018 1:29 PM, Marek Vasut wrote:
On 01/06/2018 07:46 PM, Jason Rush wrote:
On 1/6/2018 9:42 AM, Marek Vasut wrote:
<snip>
There was a minor upstream change to one of the files since I submitted v4 of my cadence device-tree patchset, so I rebased and resent the patchset as a v5. I included everyone that was originally involved with the patches and added a CC for Marek.
This is only the patchset for the device tree changes for the cadence qspi driver, Simon will still have to add the patch that fixes the cache invalidation bug in the cadence driver.
Sigh, can we get a single patchset out which fixes the problem ?
I mean, if I understand this correctly, you're all addressing one single problem, but with two patchsets, yes ?
Well... The one issue we're trying to fix is that the cadence QSPI hasn't worked on the socfpga arch since late 2016. However, it's two different issues that have caused this bigger problem:
- The indaddrtrig register was being programmed with an incorrect value for socfpga
as the result of assuming it should be programmed with the same address as the ahbbase address. This issue is resolved by adopting the Linux DT bindings, which has an independent setting for the indaddrtrig register so the register can be set correctly on all architectures. Plus it aligns the DT between u-boot and Linux.
That should be an easy patch, so this is the patchset 0/5..5/5 that you just submitted ?
Yes. I saw you Acked it, thank you.
- The cadence driver was modified at one point to use the bouncebuf functions to fix
an issue on a TI architecture that expected, where if I recall correctly all reads except the last have to be 32-bit reads. However, since the bouncebuf was designed for DMA transfers, it invalidates the data cache after reading, but since the cadence is using cpu transfers the newly read data is thrown away when the cache is invalidated. This issue is resolved by reverting the commit that introduced using the bounce buffer for read operations, which according to Vignesh don't cause any issues to the TI architecture.
Hmmmmm, I wonder why you need bounce buffer at all here. The CQSPI literally reads/writes a register space (or some FIFO in register space), there is no DMA involved at all. I also wonder why we have to manipulate with cache at all here.
I agree, I don't believe this needs a bounce buffer at all. This isn't a DMA, there is no need for cache manipulation. Vignesh understands the problem better than I do on the TI platform, but I believe it was used since it was an easy way to ensure the register read/writes were all 32-bits wide up until the last read/write. I believe the bounce buffer should be removed from the CQSPI driver and a different solution should be implemented, but Vignesh should weigh in on that since it effects his architecture.
-- Jason
On Monday 08 January 2018 09:10 AM, Jason Rush wrote: [...]
- The indaddrtrig register was being programmed with an incorrect value for socfpga
as the result of assuming it should be programmed with the same address as the ahbbase address. This issue is resolved by adopting the Linux DT bindings, which has an independent setting for the indaddrtrig register so the register can be set correctly on all architectures. Plus it aligns the DT between u-boot and Linux.
That should be an easy patch, so this is the patchset 0/5..5/5 that you just submitted ?
Yes. I saw you Acked it, thank you.
- The cadence driver was modified at one point to use the bouncebuf functions to fix
an issue on a TI architecture that expected, where if I recall correctly all reads except the last have to be 32-bit reads. However, since the bouncebuf was designed for DMA transfers, it invalidates the data cache after reading, but since the cadence is using cpu transfers the newly read data is thrown away when the cache is invalidated. This issue is resolved by reverting the commit that introduced using the bounce buffer for read operations, which according to Vignesh don't cause any issues to the TI architecture.
Hmmmmm, I wonder why you need bounce buffer at all here. The CQSPI literally reads/writes a register space (or some FIFO in register space), there is no DMA involved at all. I also wonder why we have to manipulate with cache at all here.
I agree, I don't believe this needs a bounce buffer at all. This isn't a DMA, there is no need for cache manipulation. Vignesh understands the problem better than I do on the TI platform, but I believe it was used since it was an easy way to ensure the register read/writes were all 32-bits wide up until the last read/write.
Yes, that was the intention. Unfortunately, I chose to use common bounce buffer implementation which was doing cache manipulations.
I believe the bounce buffer should be removed from the CQSPI driver and a different solution should be implemented, but Vignesh should weigh in on that since it effects his architecture.
CQSPI on TI K2G has problems with non 32 bit aligned write operations. But read operations are unaffected. Therefore I have Ack'ed Simon's patch reverting bouncebuf for read. For writes, I have patches to revert common bouncebuf usage and use a local pagesize buffer for overcome alignment issue. I am waiting for current patch backlogs to be merged so that its easy for testing w/o specifying bunch of dependent patches.
Or if Simon agrees, I can add his patch to my series post it to mailing list (rebased on top of Jason's series)?
participants (4)
-
Goldschmidt Simon -
Jason Rush -
Marek Vasut -
Vignesh R