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

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