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
next_descriptor_virt - virtual address of the next descriptor in chain
I tried adding same and it seemed like it was getting a little confusing.
Given the SGL is also an array in SGT, it seemed ok to retain an array,
though it would have good to have a chain of cmd_descriptors in SW like
in HW.
Agreed the fixed size array comes at a cost of artificial limitation of
5 entries, which anyway seems like something we are not going to
encounter in practice.
So for now, I retained the array.
All the other comments are addressed as recommended, except 1 change below
Please let know what you think.
Thank you,
Vijay/
+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.
