Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface

Hi Andy,

Thanks again, replies and follow-up questions inline below:

On Thu, Jan 06, 2022 at 08:19:39PM +0200, Andy Shevchenko wrote:

On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo [off-list ref] wrote:

quoted

LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
that targets FPGAs. LiteSDCard is a small footprint, configurable
SDCard core commonly used in LiteX designs.

The driver was first written in May 2020 and has been maintained
cooperatively by the LiteX community. Thanks to all contributors!

quoted

+config MMC_LITEX
+       tristate "LiteX MMC Host Controller support"
+       depends on OF
+       depends on PPC_MICROWATT || LITEX || COMPILE_TEST
+       help
+         This selects support for the MMC Host Controller found in LiteX SoCs.
+
+         If unsure, say N.

What would be the module name if built as a module?

litex_mmc.ko -- why are you asking? I.e., should I mention that anywhere
in the Kconfig blurb (I don't see other blurbs doing that, fwiw)?
 

...

quoted

+/*
+ * LiteX LiteSDCard driver
+ *
+ * Copyright (C) 2019-2020 Antmicro <contact@antmicro.com>
+ * Copyright (C) 2019-2020 Kamil Rakoczy <krakoczy@antmicro.com>
+ * Copyright (C) 2019-2020 Maciej Dudek <mdudek@internships.antmicro.com>
+ * Copyright (C) 2020 Paul Mackerras <paulus@ozlabs.org>
+ * Copyright (C) 2020-2021 Gabriel Somlo <gsomlo@gmail.com>

quoted

+ *

Redundant blank line.

OK, removed.

quoted

+ */

...

quoted

+#include <linux/module.h>
+#include <linux/litex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/host.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>

Perhaps keep it sorted?

OK, sorted. Now looks like this:

#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/iopoll.h>
#include <linux/litex.h>
#include <linux/module.h>
#include <linux/mmc/host.h>
#include <linux/mmc/mmc.h>
#include <linux/mmc/sd.h>
#include <linux/mmc/slot-gpio.h>
#include <linux/of.h>
#include <linux/platform_device.h>

It will easily show, for example, absence of bits.h.

 
I'm not getting any errors or warnings re. missing bits.h -- why are
you mentioning it explicitly? (for the record, I try not to explicitly
include anything unless I absolutely have to, so I try to avoid
including headers included via other headers I already *have* to
include explicitly!)

I'm expecting you're going to tell me that's wrong, but then I'm much
less confident of a clean canonical way of determining exactly what
headers must be mentioned explicitly in a driver -- please advise! :)

quoted

+       ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE),

Too many parentheses.

OK, removed (from both occurrences of `readx_poll_timeout()`).

quoted

+                                SD_SLEEP_US, SD_TIMEOUT_US);

quoted

+       if (ret || (evt & SD_BIT_TIMEOUT))

Redundant second condition. If you want +1 iteration, increase timeout.

Well, readx_poll_timeout() returns -ETIMEDOUT or 0, depending on whether
or not it read the SD_BIT_DONE bit from the provided register reg.
But even if it did return 0, the hardware might have returned its own
timeout flag (SD_BIT_TIMEOUT) before readx_poll_timeout() decided to
give up on polling the register. So we can time out in one of two
different ways, and we need to check for both.
 

quoted

+               return -ETIMEDOUT;

Why shadowed error code?

See above -- if readx_poll_timeout returned 0, we should still return
-ETIMEDOUT in case the hardware set the SD_BIT_TIMEOUT flag. I figured
I'd deal with all timeout scenarios in one place, but we can definitely
split that out to avoid confusion, something like this:

	...
        ret = readx_poll_timeout(litex_read8, reg, evt, evt & SD_BIT_DONE,
                                 SD_SLEEP_US, SD_TIMEOUT_US);
	if (ret)
                return ret;
        if (evt == SD_BIT_DONE)
                return 0;
        if (evt & SD_BIT_WR_ERR)
                return -EIO;
        if (evt & SD_BIT_TIMEOUT)	// <<< HERE
                return -ETIMEDOUT;
        if (evt & SD_BIT_CRC_ERR)
                return -EILSEQ;
	...

...

quoted

+       pr_err("%s: unknown error evt=%x\n", __func__, evt);

Use dev_err().

OK, done.

...

quoted

+       /* Wait for an interrupt if we have an interrupt and either there is
+        * data to be transferred, or if the card can report busy via DAT0.
+        */

This comment style is for the net subsystem, for others we use
/*
 * Starting here...
 */

Fix it everywhere in your code.

OK, multi-line comment style updated across the entire file.

...

quoted

+               reg = host->sdcore + LITEX_CORE_CMDRSP;
+               for (i = 0; i < 4; i++) {
+                       host->resp[i] = litex_read32(reg);
+                       reg += sizeof(u32);
+               }

Isn't it memcpy_fromio()?

Yes, that appears to be the case. Looking at `litex_read32()`
(in include/linux/litex.h), it's defined as

	le32_to_cpu((__le32 __force)readl(addr))

which counts as a "__raw_readl()", i.e., without the byteswap occurring
on BE architectures. It looks like memcpy_fromio() doesn't play byteswap
games on BE either, so they should be equivalent. With the caveat that
I only have the ability to test on LE!

For now, based on the above observations, I think it's safe to use
memcpy_fromio() here.

...

quoted

+       if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR)
+               host->rca = (host->resp[3] >> 16) & 0xffff;

Are you expecting a 32-bit value to be bigger than 2^32-1?

Fair enough :)

...

quoted

+       div = min(max(div, 2U), 256U);

clamp_t() / clamp_val() ?

OK, changed to `div = clamp_val(div, 2U, 256U);`

...

quoted

+       ret = platform_get_irq_optional(host->dev, 0);
+       if (ret == -ENXIO || ret == 0) {
+               dev_warn(dev, "Failed to get IRQ, using polling\n");
+               goto use_polling;
+       }
+       if (ret < 0)
+               return ret; /* e.g., deferred probe */
+       host->irq = ret;

Can it be rather written as

       ret = platform_get_irq_optional(host->dev, 0);
       if (ret < 0 && ret != -ENXIO)
         return ret;
       if (ret > 0)
         host->irq = ret;
       else {
               dev_warn(dev, "Failed to get IRQ, using polling\n");
               goto use_polling;
       }

?

OK, done.

...

quoted

+use_polling:
+       host->mmc->caps |= MMC_CAP_NEEDS_POLL;

quoted

+       host->irq = 0;

Isn't it 0 by default?

Yes, because of kzalloc() by way of mmc_alloc_host(). So I don't have to
set it to 0 explicitly -- Done.

...

quoted

+       mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);

quoted

+       /* NOTE: defaults to max_[req,seg]_size=PAGE_SIZE, max_blk_size=512,
+        * and max_blk_count accordingly set to 8;
+        * If for some reason we need to modify max_blk_count, we must also
+        * re-calculate `max_[req,seg]_size = max_blk_size * max_blk_count;`
+        */

Can you rather not split code by this comment. It makes sense to be above, no?

OK -- Done.

quoted

+       if (!mmc)
+               return -ENOMEM;

...

quoted

+       /* set default sd_clk frequency range based on empirical observations
+        * of LiteSDCard gateware behavior on typical SDCard media
+        */

Start sentences from capital letters and keep proper style of
multi-line comments.

Multi-line comment style fixed per your earlier observation.

I also went and capitalized the first letter of each comment throughout
the source.

...

quoted

+err:
+       mmc_free_host(mmc);
+       return ret;

This...

quoted

+}
+
+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);
+       mmc_free_host(host->mmc);

...and this have ordering issues. You mixed devm_*() with non-devm_*()
APIs in the wrong way.

Also, I haven't noticed the free_irq() call in the error path of
->probe(). Isn't it missed?

OK, I've reordered everything (and added the missing free_irq() call
like so:

	static int litex_mmc_probe(struct platform_device *pdev)
	{
		...
		...
		return 0;

	err:
	        if (host->irq > 0)
	                free_irq(host->irq, host->mmc);
	        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);

	        mmc_remove_host(host->mmc);
	        if (host->irq > 0)
	                free_irq(host->irq, host->mmc);
	        mmc_free_host(host->mmc);

	        return 0;
	}

Looking at other examples in drivers/mmc/host/* I'm not sure if/what
other ordering issues or devm/non-devm mixing violations I'm
perpetrating: if there are any left, could you please point them out
in a bit more detail?
 

quoted

+       return 0;
+}

...

quoted

+               .of_match_table = of_match_ptr(litex_match),

Wrong usage of of_match_ptr().

Huh? It's consistent with how a whole bunch of other files in
drivers/mmc/host/*.c are using it, can you please elaborate?

Thanks much for taking the time to review the driver, much appreciated!
I'll send out a v7 as soon as I get some clarity on the portions above
that I'm still confused about... :)

Thanks again,
--Gabriel