Re: [PATCH V2 27/27] mmc: mmci: add stm32 sdmmc variant

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Ludovic BARRE <ludovic.barre@st.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Gerald Baeza <gerald.baeza@st.com>,
	Loic Pallardy <loic.pallardy@st.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH V2 27/27] mmc: mmci: add stm32 sdmmc variant
Date: Mon, 1 Oct 2018 15:39:33 +0200	[thread overview]
Message-ID: <CAPDyKFr45FF-wbhT7C=+G5ptbCMRiKie304UM9P2vRypKd=PvQ@mail.gmail.com> (raw)
In-Reply-To: <50222b8d-3b53-acea-7935-092fa149d54c@st.com>

[...]

>>>   struct variant_data {
>>>          unsigned int            clkreg;
>>> @@ -348,6 +350,8 @@ struct variant_data {
>>>          unsigned int            irq_pio_mask;
>>>          u32                     start_err;
>>>          u32                     opendrain;
>>> +       bool                    dma_lli;
>>> +       u32                     stm32_idmabsize_mask;
>>
>>
>> What are these?
>
>
> This property is specific for sdmmc variants:
> sdmmc has a Internal DMA and the number bytes per buffer
> could be different between sdmmc variants
> (depend of SDMMC_IDMABSIZER register).

Okay. Thanks for clarifying.

Could you please add some information about this in the changelog as well?

[...]

>>> +
>>> +static int _sdmmc_idma_prep_data(struct mmci_host *host,
>>> +                                struct mmc_data *data)
>>> +{
>>> +       int n_elem;
>>> +
>>> +       n_elem = dma_map_sg(mmc_dev(host->mmc),
>>> +                           data->sg,
>>> +                           data->sg_len,
>>> +                           mmc_get_dma_dir(data));
>>> +
>>> +       if (!n_elem) {
>>> +               dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int sdmmc_idma_prep_data(struct mmci_host *host,
>>> +                               struct mmc_data *data, bool next)
>>> +{
>>> +       /* Check if job is already prepared. */
>>> +       if (!next && data->host_cookie == host->next_cookie)
>>> +               return 0;
>>> +
>>> +       return _sdmmc_idma_prep_data(host, data);
>>> +}
>>> +
>>> +static void sdmmc_idma_unprep_data(struct mmci_host *host,
>>> +                                  struct mmc_data *data, int err)
>>> +{
>>> +       dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
>>> +                    mmc_get_dma_dir(data));
>>> +}
>>
>>
>> The sdmmc_idma_prep_data() and sdmmc_idma_unprep_data(), seems very
>> similar to what the mmci core driver needs to do in this regards.
>>
>> Can we perhaps avoid adding these callbacks altogether, but rather
>> rely on common code in the mmci core driver?
>
>
> Actually, these callbacks allow to manage prepare/unprepare of
> dmaengine interface for mmci variant, (not needed for sdmmc which uses an
> internal dma).
>
> For Sdmmc, today there are no special case, just dma_map/unmap.
> But in the future, I hope manage the lli list in these callback.
>
> Only dma_map/unmap could be common, but the error management may
> be complicated (in mmci variant).
>
> Personally, I prefer keep prep_data/unprep_data mmci_host_ops
> interfaces.
> What do you suggest ?

Okay, let's keep them for now. We can always change things on top, in
case we see later that those callbacks can be removed.

[...]

Kind regards
Uffe