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

[Ludovic BARRE <ludovic.barre@st.com>]

On 07/05/2018 05:17 PM, Ulf Hansson wrote:
> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote:
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> 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
>