Message-ID: <CACPK8Xeg2UoAqp55R+UrRLFJqerc1Kqrubh3BiEpSon+Q6bGyQ@mail.gmail.com>
Date:   Tue, 7 Dec 2021 02:46:03 +0000
From:   Joel Stanley <joel@....id.au>
To:     "Gabriel L. Somlo" <gsomlo@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Karol Gugala <kgugala@...micro.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        Kamil Rakoczy <krakoczy@...micro.com>,
        mdudek@...ernships.antmicro.com,
        Paul Mackerras <paulus@...abs.org>,
        Stafford Horne <shorne@...il.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        david.abdurachmanov@...ive.com,
        Florent Kermarrec <florent@...oy-digital.fr>
Subject: Re: [PATCH v1 3/3] mmc: Add driver for LiteX's LiteSDCard interface

On Tue, 7 Dec 2021 at 01:23, Gabriel L. Somlo <gsomlo@...il.com> wrote:
> I can remove dependency on "LITEX" and, with that, succesfully build
> the driver as a module -- same principle as the LiteETH driver.
> I'm queueing that up for the long promised v3, soon as I clear up a
> few remaining questions... :)

If we have the driver as a 'tristate' we should make sure it loads and
works as a module.

>
> Right now I have:
>
>         depends on OF || COMPILE_TEST

The OF dependency is a requirement for the symbols you're using. See
the discussion I had with Greet, I think going with this is reasonable
for the first pass:

 depends on OF
 depends on PPC_MICROWATT || LITEX || COMPILE_TEST

> > > +static int
> > > +litex_get_cd(struct mmc_host *mmc)
> > > +{
> > > +       struct litex_mmc_host *host = mmc_priv(mmc);
> > > +       int ret;
> > > +
> > > +       if (!mmc_card_is_removable(mmc))
> > > +               return 1;
> > > +
> > > +       ret = mmc_gpio_get_cd(mmc);
> >
> > Bindings.
>
> This was part of the original Antmicro version of the driver, but I
> have never actually used gpio based card detection. I'm inclined to
> remove it from this submission entirely (and keep it around as an
> out-of-tree fixup patch) until someone with the appropriate hardware
> setup can provide a "tested-by" (and preferably an example on how to
> configure it in DT).

Agreed, if it's untested and unused then delete it.

> > > +static u32
> > > +litex_response_len(struct mmc_command *cmd)

something else I noticed when re-reading the code; we can put the
return arguments on the same line as the functions. The kernel has a
nice long column limit now, so there's no need to shorten lines
unncessarily. Feel free to go through the driver and fix that up.

> > Can you put all of the irq handling together? Move the hardware sanity
> > checking up higher and put it together too:

> I moved it all as close together as possible, but it has to all go
> *after* all of the `dev_platform_ioremap_resource[_byname]()` calls,
> since those pointers are all used when calling `litex_write*()`.

Makes sense.

> I'll have it in v3, and you can let me know then if it's OK or still
> needs yet more work.

> > > +
> > > +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> >
> > Is this going to be true on all platforms? How do we handle those
> > where it's not true?
>
> I'll need to do a bit of digging here, unless anyone has ideas ready
> to go...

I'm not an expert either, so let's consult the docs:

Documentation/core-api/dma-api-howto.rst

This suggests we should be using dma_set_mask_and_coherent?

But we're setting the mask to 32, which is the default, so perhaps we
don't need this call at all?

(I was thinking of the microwatt soc, which is a 64 bit soc but the
peripherals are on a 32 bit bus, and some of the devices are behind a
smaller bus again. But I think we're ok, as the DMA wishbone is
32-bit).

>
> > > +       if (ret)
> > > +               goto err_free_host;
> > > +
> > > +       host->buf_size = mmc->max_req_size * 2;
> > > +       host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
> > > +                                         &host->dma, GFP_DMA);
> >
> > dmam_alloc_coherent?
>
> Does this mean I no longer have to `dma[m]_free_coherent()` at [1] and
> [2] below, since it's automatically handled by the "managed" bits?

Yep spot on.

> > > +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > +       mmc->ops = &litex_mmc_ops;
> > > +
> > > +       mmc->f_min = 12.5e6;
> > > +       mmc->f_max = 50e6;
> >
> > How did you pick these?
> >
> > Are these always absolute? (wouldn't they depend on the system they
> > are in? host clocks?)
>
> They are the minimum and maximum frequency empirically observed to work
> on typical sdcard media with LiteSDCard, in the wild. I can state that
> in a comment (after I do another pass of double-checking, crossing Ts
> and dotting Is), hope that's what you were looking for.

Lets add a comment describing that.

> > > +
> > > +       return 0;
> > > +
> > > +err_free_dma:
> > > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
>
> [1] is this made optional by having used `dmam_alloc_coherent()` above?

Yes, we can remove this.

> > > +err_free_host:
> > > +       mmc_free_host(mmc);
> > > +       return ret;
> > > +}
> > > +
> > > +static int
> > > +litex_mmc_remove(struct platform_device *pdev)
> > > +{
> > > +       struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
> > > +
> > > +       if (host->irq > 0)
> > > +               free_irq(host->irq, host->mmc);
> > > +       mmc_remove_host(host->mmc);
> > > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
>
> [2] ditto?

Yep.

> Thanks again for all the help and advice!

No problem. Thanks for taking the time to upstream the code.

Powered by blists - more mailing lists