[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d93e6886-1dee-7915-459f-466a54d0c242@st.com>
Date: Wed, 11 Jul 2018 11:41:06 +0200
From: Ludovic BARRE <ludovic.barre@...com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Rob Herring <robh+dt@...nel.org>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...com>,
Gerald Baeza <gerald.baeza@...com>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 07/05/2018 05:17 PM, Ulf Hansson wrote:
> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@...com> wrote:
>> From: Ludovic Barre <ludovic.barre@...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
>
Powered by blists - more mailing lists