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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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
