Message-ID: <CAPDyKFo0q5Yps1whPmaUQJAV3tXx91WPmxtMF_ov43qP=djQfg@mail.gmail.com>
Date:   Wed, 11 Jul 2018 14:16:00 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Ludovic BARRE <ludovic.barre@...com>
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 11 July 2018 at 11:41, Ludovic BARRE <ludovic.barre@...com> wrote:
>
>
> 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.

Apologize if I may have lead you in a wrong direction, that was not my intent.

However, by looking at $subject patch, your seems to be unnecessarily
shuffling code around. I would like to avoid that.

>>>
>>> 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
> -...

If we can use a mmci ops callback to manage the variant differences,
that would be perfect. That combined with making the existing DMA
functions in mmci.c converted to "library" functions, which the mmci
ops callbacks can call, in order to re-use code.

When that isn't really suitable, we may need to add a "quirk" instead,
which would be specific for that particular variant. Along the lines
of what we already do for variant specifics inside mmci.c.

I think we have to decide on case by case basis, what fits best.

Hope this makes a better explanation. If not, please tell, and I can
take an initial stab and post a patch to show you with code how I mean
to move forward.

>
>
>>
>> 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)

Right. This was just one example.

If I understand, what you are suggesting is to make all of them being
variant specific callbacks, so I assume that would solve the problems.
Just to be clear, I have no problem with that.

Although, that doesn't mean we can't re-use existing dma functions in
mmci.c, when that makes sense.

[...]

Kind regards
Uffe

Powered by blists - more mailing lists