Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 07/11/2018 02:16 PM, Ulf Hansson wrote:
On 11 July 2018 at 11:41, Ludovic BARRE wrote:
On 07/05/2018 05:17 PM, Ulf Hansson wrote:
On 12 June 2018 at 15:14, Ludovic Barre wrote:
From: Ludovic Barre
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
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 07/11/2018 02:16 PM, Ulf Hansson wrote:
On 11 July 2018 at 11:41, Ludovic BARRE wrote:
On 07/05/2018 05:17 PM, Ulf Hansson wrote:
On 12 June 2018 at 15:14, Ludovic Barre wrote:
From: Ludovic Barre
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
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 11 July 2018 at 11:41, Ludovic BARRE wrote:
>
>
> On 07/05/2018 05:17 PM, Ulf Hansson wrote:
>>
>> On 12 June 2018 at 15:14, Ludovic Barre wrote:
>>>
>>> From: Ludovic Barre
>>>
>>> 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) {
>>> -
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 11 July 2018 at 11:41, Ludovic BARRE wrote:
>
>
> On 07/05/2018 05:17 PM, Ulf Hansson wrote:
>>
>> On 12 June 2018 at 15:14, Ludovic Barre wrote:
>>>
>>> From: Ludovic Barre
>>>
>>> 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) {
>>> -
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 07/05/2018 05:17 PM, Ulf Hansson wrote:
On 12 June 2018 at 15:14, Ludovic Barre wrote:
From: Ludovic Barre
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.
>>
>> 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
-...
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)
-
- if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel)
- if (dml_hw_init(host, host->mmc->parent->of_node))
- variant->qcom_dml = false;
This piece of code, should be made specific to the qcom variant and
managed though a "mmci_host_ops" callback. The corresponding code in
that callback would then start by invoking mmci_dma_setup(), before it
continues with the qcom specific operations.
For legacy variants, the corresponding callback would be set directly
to mmci_dma_setup() and called through the callback from mmci.c when
needed. There is no need to have a separate file for DMA for the
legacy variants, I think.
[...]
Kind regards
Uffe
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 07/05/2018 05:17 PM, Ulf Hansson wrote:
On 12 June 2018 at 15:14, Ludovic Barre wrote:
From: Ludovic Barre
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.
>>
>> 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
-...
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)
-
- if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel)
- if (dml_hw_init(host, host->mmc->parent->of_node))
- variant->qcom_dml = false;
This piece of code, should be made specific to the qcom variant and
managed though a "mmci_host_ops" callback. The corresponding code in
that callback would then start by invoking mmci_dma_setup(), before it
continues with the qcom specific operations.
For legacy variants, the corresponding callback would be set directly
to mmci_dma_setup() and called through the callback from mmci.c when
needed. There is no need to have a separate file for DMA for the
legacy variants, I think.
[...]
Kind regards
Uffe
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 12 June 2018 at 15:14, Ludovic Barre wrote:
> From: Ludovic Barre
>
> 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.
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.
> -
> - if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel)
> - if (dml_hw_init(host, host->mmc->parent->of_node))
> - variant->qcom_dml = false;
This piece of code, should be made specific to the qcom variant and
managed though a "mmci_host_ops" callback. The corresponding code in
that callback would then start by invoking mmci_dma_setup(), before it
continues with the qcom specific operations.
For legacy variants, the corresponding callback would be set directly
to mmci_dma_setup() and called through the callback from mmci.c when
needed. There is no need to have a separate file for DMA for the
legacy variants, I think.
[...]
Kind regards
Uffe
Re: [PATCH 01/19] mmc: mmci: regroup and define dma operations
On 12 June 2018 at 15:14, Ludovic Barre wrote:
> From: Ludovic Barre
>
> 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.
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.
> -
> - if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel)
> - if (dml_hw_init(host, host->mmc->parent->of_node))
> - variant->qcom_dml = false;
This piece of code, should be made specific to the qcom variant and
managed though a "mmci_host_ops" callback. The corresponding code in
that callback would then start by invoking mmci_dma_setup(), before it
continues with the qcom specific operations.
For legacy variants, the corresponding callback would be set directly
to mmci_dma_setup() and called through the callback from mmci.c when
needed. There is no need to have a separate file for DMA for the
legacy variants, I think.
[...]
Kind regards
Uffe
