On 11/30/2024 9:35 AM, Bjorn Andersson wrote:
On Fri, Nov 29, 2024 at 05:02:22PM +0530, Jyothi Kumar Seerapu wrote:
On 11/28/2024 8:53 PM, Bjorn Andersson wrote:
On Thu, Nov 28, 2024 at 07:03:50PM +0530, Jyothi Kumar Seerapu wrote:
diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
[..]
/* first create config tre if applicable */
if (direction == DMA_MEM_TO_DEV && spi->set_config) {
@@ -1763,14 +1767,32 @@ static int gpi_create_spi_tre(struct gchan *chan, struct gpi_desc *desc,
tre_idx++;
address = sg_dma_address(sgl);
- tre->dword[0] = lower_32_bits(address);
- tre->dword[1] = upper_32_bits(address);
+ len = sg_dma_len(sgl);
- tre->dword[2] = u32_encode_bits(sg_dma_len(sgl), TRE_DMA_LEN);
+ /* Support Immediate dma for write transfers for data length up to 8 bytes */
And what happens if the developer writing the SPI driver forgets to read
this comment and sets QCOM_GPI_IMMEDIATE_DMA for a 9 byte transfer?
In V2 patch, QCOM_GPI_IMMEDIATE_DMA is set based on
QCOM_GPI_IMMEDIATE_DMA_LEN only.
I assume you mean "patch 2/2". So, what happens if someone refactors the
SPI driver in the future, will they read this comment?
As per Hardware programming guide, immediate dma support is for up to 8
bytes only.
Need to check what is the behavior if we want to handle 9 bytes using
immediate dma feature support.
I'm saying that you have a comment here which says that the caller must
not pass len > 8. Write that comment in code to avoid mistakes - either
now or in the future.
Sure, i will update the comment in V3.
+ if ((spi->flags & QCOM_GPI_IMMEDIATE_DMA) && direction == DMA_MEM_TO_DEV) {
Why is this flag introduced?
If I understand the next patch, all DMA_MEM_TO_DEV transfers of <=
QCOM_GPI_IMMEDIATE_DMA_LEN can use the immediate mode, so why not move
the condition here?
Also ordering[1].
if (direction == DMA_MEM_TO_DEV && len <= 2 * sizeof(tre->dword[0]))
Sure, thanks for the suggestion.
so, instead using "QCOM_GPI_IMMEDIATE_DMA_LEN" need to use " 2 *
sizeof(tre->dword[0])" for 8 bytes length check.
Either one works, but I'm guessing that while 8 is the right number the
reason for 8 is that the data is passed in 2 * dword.
Okay, i will use "2 * sizeof(tre->dword[0]" which gives 8 only.
The important thing is that you're encoding the length check here, so
that the client can't by mistake trigger immediate mode with > 8 bytes.
As a side effect, you no longer need the QCOM_GPI_IMMEDIATE_DMA flag and
should be able to drop patch 2.
Sure thanks, will update the changes in V3.
[1] Compare "all transfers of length 8 or less, which are mem to device"
vs "all transfers which are mem to device, with a length of 8 or less".
The bigger "selection criteria" is the direction, then that's fine tuned
by the length query.
+ buf = sg_virt(sgl);
It's a question of style, but I think you could just put the sg_virt()
directly in the memcpy() call and avoid the extra variable.
Okay, i will directly put sg_virt() in memcpy().
Try it out, pick the option that look the best.
Yes, will do it in V3.
Regards,
Bjorn
