Re: [U-Boot] [PATCH 1/2] common: make bouncebuf work for non-DMA transfers

Hi Vignesh,
Vignesh R wrote:
[..] Its not actually unaligned access, cadence QSPI IP on TI platforms do not support non-byte accesses except for the last word. As per the TRM: "The external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer"
OK, I thought it was unaligned access because of reading your commit message talking about data aborts. But this was in the write part indeed.
So given my explanation above, what's the preferred way to fix this?
Sorry, I overlooked the fact that bounce_buffer_stop() is calling invalidate_dcache_range(). Somehow, this did not show problems on my platform although its a writeback cache.
I would suggest to revert commit b63b46313 ("spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"). I have seen that non 32 bit accesses cause problems only while writing to flash but not during read operations although the TRM states that both reads and writes are affected. But, since reverting b63b46313 as such does not break TI platforms, I would prefer sending a revert. Meanwhile, I will work on a better patch later.
I have not actually tested writing with dcache enabled, but from reading the code, it should not be a problem. I'll test that then.
Lets not touch bouncebuf for now.
OK. I take it that bouncebuf should be only used for dma transfers. In that case, shouldn't we revert the write patch, too, eventually? Even if it does not break data consistency like in the read direction, messing around with the cache while doing cpu transfers at the very least will drop performance. Plus it seems like a bad example.
We can do that later, of course, as it should work like it is now.
I need this driver fixed, so whatever way will be accepted is fine by me, I guess. Just let me know.
Sorry for the trouble. I am okay with reverting the patch affecting read path.
OK. I'll send a patch hat reverts the read path then.
Thank you for your help!
Regards, Simon

On Thursday 16 November 2017 04:21 PM, Goldschmidt Simon wrote:
Hi Vignesh,
Vignesh R wrote:
[..] Its not actually unaligned access, cadence QSPI IP on TI platforms do not support non-byte accesses except for the last word. As per the TRM: "The external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer"
OK, I thought it was unaligned access because of reading your commit message talking about data aborts. But this was in the write part indeed.
So given my explanation above, what's the preferred way to fix this?
Sorry, I overlooked the fact that bounce_buffer_stop() is calling invalidate_dcache_range(). Somehow, this did not show problems on my platform although its a writeback cache.
I would suggest to revert commit b63b46313 ("spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"). I have seen that non 32 bit accesses cause problems only while writing to flash but not during read operations although the TRM states that both reads and writes are affected. But, since reverting b63b46313 as such does not break TI platforms, I would prefer sending a revert. Meanwhile, I will work on a better patch later.
I have not actually tested writing with dcache enabled, but from reading the code, it should not be a problem. I'll test that then.
Lets not touch bouncebuf for now.
OK. I take it that bouncebuf should be only used for dma transfers.
Not bouncebuf in general. But, bouncebuf.c seems to written for DMA transfers.
In that case, shouldn't we revert the write patch, too, eventually? Even if it does not break data consistency like in the read direction, messing around with the cache while doing cpu transfers at the very least will drop performance. Plus it seems like a bad example.
Yes, I will work on that.
participants (2)
-
Goldschmidt Simon
-
Vignesh R