| Message ID | d6608292-9f38-a607-23a9-e063a886011e@ti.com |
|---|---|
| State | Not Applicable |
| Headers | show |
I don't know if this message successfully sent last weekend. My mail server was undergoing maintenance, and I didn't see my original message on the u-boot mailing list archive. So I'm resending, my apologies if this is a repost. R, Vignesh wrote: > On 2/25/2017 1:25 AM, Rush, Jason A. wrote: >> R, Vignesh wrote: >>> On 2/24/2017 12:55 AM, Marek Vasut wrote: >>>> On 02/23/2017 08:22 PM, Rush, Jason A. wrote: >>>>> Marek Vasut wrote: >>>>>> On 02/22/2017 06:37 PM, Rush, Jason A. wrote: >>>>>>> Marek Vasut wrote: >>>>>>>> On 02/21/2017 05:50 PM, Rush, Jason A. wrote: >>> [...] >>>>> >>> >>> You could try reverting my commits: >>> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb: >>> Use 32 bit indirect write transaction when possible >>> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb: >>> Use 32 bit indirect read transaction when possible >>> >> >> When I reverted these two commits and added my patch for the indirect >> trigger_address, it works correctly. >> > > Oops, these patches are required as Cadence QSPI controller(I am not > sure if all versions of IP are newer versions only) has a limitation > that the external master is only permitted to issue 32-bit data > interface reads until the last word of an indirect transfer. > > >> Also, when I disabled the dcache (dcache off) as Marek suggested, it works >> correctly when running from the master branch (again with my indirect >> trigger_address patch). >> > > Just that I understand correctly, with latest master(with no patches > reverted) + your patch for indirect trigger_address + dcache off, you > don't see any problem? > Correct. >>> >>> But there were other patches by others in v2017.01-rc1, like >>> spi: cadence_qspi: Fix CS timings which may have impact. >>> >> >> I left all other commits in except the two Vignesh suggested to revert, so >> it seems to be related to those two commits and caching. As another data >> point, I can load and boot linux with caching on from another source (MMC). >> So I don't think it's a problem with memory/caching in general. >> >> Any suggestions on how to proceed from here? >> > > My patches use common bounce_buffer implementation which does dcache > flush/invalidate and if dcache has issues then I guess those operations > may be causing data corruption. Could you do a bit more research for me? > > 1. As a hack, could you just disable dcache operations in bounce_buffer > implementation? Here is the diff for the same: > This works. So it does seem to be an issue with the CQSPI and dcache on the Altera SoC. > > diff --git a/common/bouncebuf.c b/common/bouncebuf.c > index 054d9e0302cc..2878b9eed1ae 100644 > --- a/common/bouncebuf.c > +++ b/common/bouncebuf.c [...] > > 2. If that works, I guess there is some issue wrt CQSPI and dcache on your platform, > I suggest you to revert my above two patches and try non bounce buffer version of > my changes here: https://patchwork.ozlabs.org/patch/693069/. > This patch takes care of indirect write. I don't have similar patch for > indirect read but that wasn't required. > This also works. Marek - how do you feel about a patch series with the following: 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/ (should I keep Vignesh as the signed-off?) 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark them all as __iomem 5. Load the indirect-trigger property from the DT as a u32. I think this still needs to be a u32 because it's used in a writel(...) 32-bit write call in cadence_qspi_apb.c 6. Add indirect-trigger to dts files that use cadence driver I think that captures all the changes needed. Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns, cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for the same properties. Do you want the 'cdns,' prefix on trigger-address? If so, do you want me to rename all the other properties to align them with the Linux DT as an additional patch? -- Regards, Jason
On 2/28/2017 8:38 PM, Rush, Jason A. wrote: [...] > > This also works. > > Marek - how do you feel about a patch series with the following: > > 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387 > spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible > 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f > spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible > 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/ > (should I keep Vignesh as the signed-off?) Depends on how much you have changed the code. If the change is significant then change the authorship to you and drop my signed-off. Else, keep the authorship and signed-off. If you are adding something new to the patch like adding code to make sure that only 32bit data reads are issued, then I suggest you to submit that change as separate patch. > 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark > them all as __iomem > 5. Load the indirect-trigger property from the DT as a u32. I think this still > needs to be a u32 because it's used in a writel(...) 32-bit write call in > cadence_qspi_apb.c > 6. Add indirect-trigger to dts files that use cadence driver > > I think that captures all the changes needed. > > Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix > (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns, > cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for > the same properties. Do you want the 'cdns,' prefix on trigger-address? > If so, do you want me to rename all the other properties to align them with > the Linux DT as an additional patch? > > -- > Regards, > Jason >
R, Vignesh wrote: > On 2/28/2017 8:38 PM, Rush, Jason A. wrote: > [...] >> >> This also works. >> >> Marek - how do you feel about a patch series with the following: >> >> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387 >> spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible >> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f >> spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible >> 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/ >> (should I keep Vignesh as the signed-off?) > > Depends on how much you have changed the code. If the change is > significant then change the authorship to you and drop my signed-off. > Else, keep the authorship and signed-off. > If you are adding something new to the patch like adding code to make > sure that only 32bit data reads are issued, then I suggest you to submit > that change as separate patch. Very minimal. Another commit changed a #define from CQSPI_REG_INDIRECTWR_START_MASK to CQSPI_REG_INDIRECTWR_START, so I had to modify the patch to drop the _MASK so it would apply. Other than that, it's identical in content. > >> 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark >> them all as __iomem >> 5. Load the indirect-trigger property from the DT as a u32. I think this still >> needs to be a u32 because it's used in a writel(...) 32-bit write call in >> cadence_qspi_apb.c >> 6. Add indirect-trigger to dts files that use cadence driver >> >> I think that captures all the changes needed. >> >> Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix >> (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns, >> cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for >> the same properties. Do you want the 'cdns,' prefix on trigger-address? >> If so, do you want me to rename all the other properties to align them with >> the Linux DT as an additional patch? >> -- Regards, Jason
On Tuesday 28 February 2017 10:36 PM, Rush, Jason A. wrote: > R, Vignesh wrote: >> On 2/28/2017 8:38 PM, Rush, Jason A. wrote: >> [...] >>> >>> This also works. >>> >>> Marek - how do you feel about a patch series with the following: >>> >>> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387 >>> spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible >>> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f >>> spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible >>> 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/ >>> (should I keep Vignesh as the signed-off?) >> >> Depends on how much you have changed the code. If the change is >> significant then change the authorship to you and drop my signed-off. >> Else, keep the authorship and signed-off. >> If you are adding something new to the patch like adding code to make >> sure that only 32bit data reads are issued, then I suggest you to submit >> that change as separate patch. > > Very minimal. Another commit changed a #define from > CQSPI_REG_INDIRECTWR_START_MASK to CQSPI_REG_INDIRECTWR_START, > so I had to modify the patch to drop the _MASK so it would apply. Other > than that, it's identical in content. > In that case you will have to retain my authorship and signed-off by and add your signed-off by below that. Thanks!
diff --git a/common/bouncebuf.c b/common/bouncebuf.c index 054d9e0302cc..2878b9eed1ae 100644 --- a/common/bouncebuf.c +++ b/common/bouncebuf.c @@ -55,21 +55,21 @@ int bounce_buffer_start(struct bounce_buffer *state, void *data, * Flush data to RAM so DMA reads can pick it up, * and any CPU writebacks don't race with DMA writes */ - flush_dcache_range((unsigned long)state->bounce_buffer, - (unsigned long)(state->bounce_buffer) + - state->len_aligned); +// flush_dcache_range((unsigned long)state->bounce_buffer, +// (unsigned long)(state->bounce_buffer) + +// state->len_aligned); return 0; } int bounce_buffer_stop(struct bounce_buffer *state) { - if (state->flags & GEN_BB_WRITE) { - /* Invalidate cache so that CPU can see any newly DMA'd data */ - invalidate_dcache_range((unsigned long)state->bounce_buffer, - (unsigned long)(state->bounce_buffer) + - state->len_aligned); - } +// if (state->flags & GEN_BB_WRITE) { +// /* Invalidate cache so that CPU can see any newly DMA'd data */ +// invalidate_dcache_range((unsigned long)state->bounce_buffer, +// (unsigned long)(state->bounce_buffer) + +// state->len_aligned); +// } if (state->bounce_buffer == state->user_buffer) return 0;