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

[Ludovic BARRE]

On 10/01/2018 03:39 PM, Ulf Hansson wrote:
> [...]
>
> > > >    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?

OK

> [...]
>
> > > > +
> > > > +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.

Okay, that make sense.

> [...]
>
> Kind regards
> Uffe