Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations

[Ludovic BARRE]

On 07/05/2018 05:17 PM, Ulf Hansson wrote:
> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> > From: Ludovic Barre <ludovic.barre@xxxxxx>
> >
> > Prepare mmci driver to manage dma interface by new property.
> > This patch defines and regroups dma operations for mmci drivers.
> > mmci_dma_XX prototypes are added to call member of mmci_dma_ops
> > if not null. Based on legacy need, a mmci dma interface has been
> > defined with:
> > -mmci_dma_setup
> > -mmci_dma_release
> > -mmci_dma_pre_req
> > -mmci_dma_start
> > -mmci_dma_finalize
> > -mmci_dma_post_req
> > -mmci_dma_error
> > -mmci_dma_get_next_data
>
> As I suggested for one of the other patches, I would rather turn core
> mmci functions into library functions, which can be either invoked
> from variant callbacks or assigned directly to them.
>
> In other words, I would leave the functions that you move in this
> patch to stay in mmci.c. Although some needs to be re-factored and we
> also needs to make some of them available to be called from another
> file, hence the functions needs to be shared via mmci.h rather than
> being declared static.

In previous exchange mail "STM32MP1 SDMMC driver review"
we are said:

>>> -dma variant à should fit in Qualcomm implementation, reuse (rename)
>>> mmci_qcom_dml.c file and integrate ST dma in.
>>
>> stm32 sdmmc has an internal dma, no need to use dmaengine API;
>> So some modifications in mmci (pre/post request, mmci_dma_xx). perhaps
>> should be done with an ops or not.
>
>Yes.
>
>The Qualcomm variant is also using an internal DMA, hence I thought
>there may be something we could re-use, or at least have some new
>common ops for.

It's not crystal clear for me.
Do you always agree with a dma ops which allow to address different
DMA transfer:
-with dmaengine API
-sdmmc idma, without dmaengine API
-...

> Let me take a concrete example on how I would move forward, hopefully
> that explains it a bit better. Please see below.
>
> [...]
>
> > -/*
> > - * All the DMA operation mode stuff goes inside this ifdef.
> > - * This assumes that you have a generic DMA device interface,
> > - * no custom DMA interfaces are supported.
> > - */
> > -#ifdef CONFIG_DMA_ENGINE
> > -static void mmci_dma_setup(struct mmci_host *host)
> > -{
> > -       const char *rxname, *txname;
> > -       struct variant_data *variant = host->variant;
> > -
> > -       host->dma_rx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "rx");
> > -       host->dma_tx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "tx");
> > -
> > -       /* initialize pre request cookie */
> > -       host->next_data.cookie = 1;
> > -
> > -       /*
> > -        * If only an RX channel is specified, the driver will
> > -        * attempt to use it bidirectionally, however if it is
> > -        * is specified but cannot be located, DMA will be disabled.
> > -        */
> > -       if (host->dma_rx_channel && !host->dma_tx_channel)
> > -               host->dma_tx_channel = host->dma_rx_channel;
> > -
> > -       if (host->dma_rx_channel)
> > -               rxname = dma_chan_name(host->dma_rx_channel);
> > -       else
> > -               rxname = "none";
> > -
> > -       if (host->dma_tx_channel)
> > -               txname = dma_chan_name(host->dma_tx_channel);
> > -       else
> > -               txname = "none";
> > -
> > -       dev_info(mmc_dev(host->mmc), "DMA channels RX %s, TX %s\n",
> > -                rxname, txname);
> > -
> > -       /*
> > -        * Limit the maximum segment size in any SG entry according to
> > -        * the parameters of the DMA engine device.
> > -        */
> > -       if (host->dma_tx_channel) {
> > -               struct device *dev = host->dma_tx_channel->device->dev;
> > -               unsigned int max_seg_size = dma_get_max_seg_size(dev);
> > -
> > -               if (max_seg_size < host->mmc->max_seg_size)
> > -                       host->mmc->max_seg_size = max_seg_size;
> > -       }
> > -       if (host->dma_rx_channel) {
> > -               struct device *dev = host->dma_rx_channel->device->dev;
> > -               unsigned int max_seg_size = dma_get_max_seg_size(dev);
> > -
> > -               if (max_seg_size < host->mmc->max_seg_size)
> > -                       host->mmc->max_seg_size = max_seg_size;
> > -       }
>
> Everything above shall be left as generic library function,
> mmci_dma_setup() and I would share it via mmci.h and thus change it
> from being static.

each interfaces mmci_dma_XXX have very different needs depending
dma_ops (legacy, sdmmc idma)

> > -
> > -       if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel)
> > -               if (dml_hw_init(host, host->mmc->parent->of_node))
> > -                       variant->qcom_dml = false;
>
> This piece of code, should be made specific to the qcom variant and
> managed though a "mmci_host_ops" callback. The corresponding code in
> that callback would then start by invoking mmci_dma_setup(), before it
> continues with the qcom specific operations.
>
> For legacy variants, the corresponding callback would be set directly
> to mmci_dma_setup() and called through the callback from mmci.c when
> needed. There is no need to have a separate file for DMA for the
> legacy variants, I think.
>
> [...]
>
> Kind regards
> Uffe