lkml.org 
[lkml]   [2018]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 01/19] mmc: mmci: regroup and define dma operations
From
Date


On 07/11/2018 02:16 PM, Ulf Hansson wrote:
> On 11 July 2018 at 11:41, Ludovic BARRE <ludovic.barre@st.com> wrote:
>>
>>
>> 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.
>
> 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.

Yes, when examine dmaengine_XX ops and sdmmc_idma_XX ops (in patch 01
and 17) there are few common piece of code. So I think we will have same
dma functions pointer in mmci_ops. However, the cookie management may be
shared

>
> [...]
>
> Kind regards
> Uffe
>
\
 
 \ /
  Last update: 2018-07-15 22:05    [W:0.111 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site