Re: [PATCH 00/14] mmc: mmci: prepare dma callbacks with mmci_host_ops

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

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
>