lkml.org 
[lkml]   [2018]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 00/14] mmc: mmci: prepare dma callbacks with mmci_host_ops
From
Date


On 09/04/2018 12:00 PM, Ulf Hansson wrote:
> On 1 August 2018 at 11:36, Ludovic Barre <ludovic.Barre@st.com> wrote:
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> This patch series prepares and adds callbacks for dma transfert at
>> mmci_host_ops. This series is composed of 3 parts:
>> -Internalize specific needs of legacy dmaengine.
>> -Create and setup dma_priv pointer
>> -Create generic callbacks which share some features
>> (like cookie...) and call specific needs
>
> I have now reviewed part of this series and provided you with some
> comments, but will stop at this point.

thanks for your review

>
> Overall, the comments are about renaming and picking better function
> names. Those comments should be easy to address in a new version.

yes, there is no problem for the naming, I will change following your
recommendations.

>
> However, the other more important point is the number of variant
> callbacks you are adding. It's of course a balance to pick the right
> level, to get both flexibility but also to avoid open coding. In the
> end we don't want to get too many callbacks, but then it's better to
> share common mmci code for variants, through mmci.h.
>
> Finally, I would like to see a patch on top adding the support for the
> new ST variant, so I can see how the callbacks and changes really are
> being used. Can you please add that?
>

yes, I prepare a patch with sdmmc variant to show how callbacks are used.

About comment on patch 07/14:
> So, having callbacks for dealing with dma_map|unmap() kind of
> operations, becomes rather fine-grained and not very flexible.
> Instead, what do you think of allowing the variant init function to
> dynamically assign the ->pre_req() and the ->post_req() callbacks in
> the struct mmc_host_ops. Common mmci functions to manage these
> operations can instead be shared via mmci.h and called by the
> variants.

I think we have the same goal or idea, regroup the common needs to avoid
too much specific drift.

I will try to describe the functions which are commons and the link to
specific mmci_host_ops callbacks.

(I use function names of this series, but it's just for this example)

commons function used by mmci core:
-mmci_pre_request:
check data and cookie, call common mmci_validate_data
and mmci_prepare_data.

-mmci_post_request:
check data and cookie then call common mmci_unprepare_data

-mmci_validate_data:
check the common constraint of variants, call specific
validate_data if defined.

-mmci_prepare_data:
setup common next cookie, call specific prepare data if defined

-mmci_unprepare_data:
clear the common cookie, call specific unprepare data if defined

-mmci_get_next_data:
check cookie, call specific get_next_data if defined

-mmci_dma_setup:
initialize common next_cookie, call specific dma_setup if defined

-mmci_dma_release
just call dma_release if defined

-mmci_dma_start
call common prepare data if not yet done (by next)
call specific dma_start
write common registers to start transfer and setup mmci mask

-mmci_dma_finalize:
just call dma_finalize if defined

-mmci_dma_error
just call dma_error if defined

mmci_host_ops specific:
struct mmci_host_ops
validate_data: could be use to check specific constraint of variant.
sdmmc has constraints on base & size for each element
excepted the last element which has no constraint on
size.
prepare_data: specific needs to prepare current or next data request.
mmci: dma_map_sg on channel, use dmaengine api
"dmaengine_prep_slave_sg" to queue a transfer
sdmmc: dma_map_sg on sdmmc device and prepare the link
list of internal dma

unprepare_data: specific needs to unprepare the data of request
mmci: dma_unmap_sg on channel, use dmaengine api to
terminate transfert.
sdmmc: just unmap on sdmmc device.

get_next_data: manage specific needs to move on next data
mmci: get next dmaengine descriptor and channel
sdmmc: today, nothing

dma_setup: setup specific need if you use a Direct Memory Access
mmci: use dmaengine api to request slave channel.
sdmmc: alloc memory for link list, and define specific
mmc->max_segs and mmc->max_seg_size.

dma_release: release specific resource if you use a Direct Memory
access.
mmci: use dmaengine api to release channel
sdmmc: nothing

dma_start: set specific needs to start dma request
mmci: use dmaengine api to submit and pending a dma
transfert.
sdmmc: set specific sdmmc registers to start an internal
dma transfert

dma_finalize: set specific needs to finalize a request
mmci: specific check on fifo status and
channel/descriptor management.
sdmmc: just clear a specific register of sdmmc

dma_error: specific error management.
mmci: dmaengine api to terminate a transfer
sdmmc: nothing

BR
Ludo

>>
>> This patch series must be applied on top of
>> "mmc: mmci: Add and implement a ->dma_setup() callback for qcom dml"
>>
>> Ludovic Barre (14):
>> mmc: mmci: fix qcom dma issue during mmci init with new dma_setup
>> callback
>> mmc: mmci: internalize dma map/unmap into mmci dma functions
>> mmc: mmci: internalize dma_inprogress into mmci dma functions
>> mmc: mmci: introduce dma_priv pointer to mmci_host
>> mmc: mmci: move mmci next cookie to mci host
>> mmc: mmci: merge prepare data functions
>> mmc: mmci: add prepare/unprepare_data callbacks
>> mmc: mmci: add get_next_data callback
>> mmc: mmci: modify dma_setup callback
>> mmc: mmci: add dma_release callback
>> mmc: mmci: add dma_start callback
>> mmc: mmci: add dma_finalize callback
>> mmc: mmci: add dma_error callback
>> mmc: mmci: add validate_data callback
>>
>> drivers/mmc/host/mmci.c | 458 ++++++++++++++++++++++++---------------
>> drivers/mmc/host/mmci.h | 45 ++--
>> drivers/mmc/host/mmci_qcom_dml.c | 15 +-
>> 3 files changed, 322 insertions(+), 196 deletions(-)
>>
>> --
>> 2.7.4
>>
>
> Kind regards
> Uffe
>

\
 
 \ /
  Last update: 2018-09-05 11:15    [W:0.195 / U:6.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site