Messages in this thread | | | Date | Fri, 21 Apr 2023 23:51:29 +0530 | Subject | Re: [PATCH v4 5/5] spi: spi-qcom-qspi: Add DMA mode support | From | Vijaya Krishna Nivarthi <> |
| |
Hi,
On 4/21/2023 11:05 PM, Doug Anderson wrote: > Hi, > > On Fri, Apr 21, 2023 at 9:58 AM Vijaya Krishna Nivarthi > <quic_vnivarth@quicinc.com> wrote: >> Hi, >> >> Thanks a lot for the review and inputs... >> >> >> On 4/20/2023 10:49 PM, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Apr 20, 2023 at 6:13 AM Vijaya Krishna Nivarthi >>> <quic_vnivarth@quicinc.com> wrote: >>>> @@ -137,11 +155,29 @@ enum qspi_clocks { >>>> QSPI_NUM_CLKS >>>> }; >>>> >>>> +enum qspi_xfer_mode { >>>> + QSPI_FIFO, >>>> + QSPI_DMA >>>> +}; >>>> + >>>> +/* >>>> + * Number of entries in sgt returned from spi framework that- >>>> + * will be supported. Can be modified as required. >>>> + * In practice, given max_dma_len is 64KB, the number of >>>> + * entries is not expected to exceed 1. >>>> + */ >>>> +#define QSPI_MAX_SG 5 >>> I actually wonder if this would be more nicely done just using a >>> linked list, which naturally mirrors how SGs work anyway. You'd add >>> "struct list_head" to the end of "struct qspi_cmd_desc" and just store >>> a pointer to the head in "struct qcom_qspi". >>> >>> For freeing, you can always get back the "virtual" address because >>> it's just the address of each node. You can always get back the >>> physical address because it's stored in "data_address". >>> >> Please note that in "struct qspi_cmd_desc" >> >> data_address - dma_address of data buffer returned by spi framework >> >> next_descriptor - dma_address of the next descriptor in chain >> >> >> If we were to have a linked list of descriptors that we can parse and >> free, it would require 2 more fields >> >> this_descriptor_dma - dma address of the current descriptor > Isn't that exactly the same value as "data_address"? Sure, > "data_address" is a u32 and the DMA address is 64-bits, but elsewhere > in the code you already rely on the fact that the upper bits of the > DMA address are 0 when you do:
No; data_address is the dma_address mapped to the xfer buffer.
This is provided by spi framework and retrieved from sgl.
"this_descriptor" would be the dma_address of the current cmd_descriptor.
this is returned from dma_pool_alloc()
this would be required for freeing.
> virt_cmd_desc->data_address = dma_ptr > > >> next_descriptor_virt - virtual address of the next descriptor in chain > Right, this would be the value of the next node in the linked list, > right? So basically by adding a list_node_t you can find it easily. > > >>>> +static int qcom_qspi_alloc_desc(struct qcom_qspi *ctrl, dma_addr_t dma_ptr, >>>> + uint32_t n_bytes) >>>> +{ >>>> + struct qspi_cmd_desc *virt_cmd_desc, *prev; >>>> + dma_addr_t dma_cmd_desc; >>>> + >>>> + /* allocate for dma cmd descriptor */ >>>> + virt_cmd_desc = (struct qspi_cmd_desc *)dma_pool_alloc(ctrl->dma_cmd_pool, >>>> + GFP_KERNEL, &dma_cmd_desc); >>> Remove unnecessary cast; "void *" assigns fine w/out a cast. >>> >>> Add "| GFP_ZERO" and then get rid of the need to clear the "reserved" >>> and "next_descriptor" stuff below. >>> >> I needed __GFP_ZERO to prevent a compile error, used same. > Ah, sorry. Sounds good. > > -Doug
| |