On 5 September 2018 at 11:13, Ludovic BARRE [off-list ref] wrote:

On 09/04/2018 12:00 PM, Ulf Hansson wrote:

quoted

On 1 August 2018 at 11:36, Ludovic Barre [off-list ref] wrote:

quoted

From: Ludovic Barre <redacted>

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

quoted

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.

quoted

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:

quoted

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

Ludo, thanks for the detailed explanation and summary!

Following this, it looks like it makes sense to keep the callbacks as
on the level you have suggested. At least, I don't want to delay you
from getting this upstream, by just giving some vague ideas of how to
change the number of callbacks.

So, I am fine if you stick to the existing approach! Then, if we later
on realizes that it makes sense to share more common code through
mmci.h, to get rid of some callback, we can always do that on top.

[...]

Kind regards
Uffe