Re: [PATCH v4 5/5] spi: spi-qcom-qspi: Add DMA mode support

[Vijaya Krishna Nivarthi]

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.