On Tue, 7 Dec 2021 at 01:23, Gabriel L. Somlo [off-list ref] 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

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

quoted

+
+       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).

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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