Messages in this thread | | | Date | Thu, 6 Jan 2022 17:50:24 -0500 | From | "Gabriel L. Somlo" <> | Subject | 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 <gsomlo@gmail.com> wrote: > > > > 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! > > > +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)?
> ... > > > +/* > > + * 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> > > > + * > > Redundant blank line.
OK, removed.
> > > + */ > > ... > > > +#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! :)
> > > + ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE), > > Too many parentheses.
OK, removed (from both occurrences of `readx_poll_timeout()`).
> > > + SD_SLEEP_US, SD_TIMEOUT_US); > > > + 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. > > + 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; ...
> ... > > > + pr_err("%s: unknown error evt=%x\n", __func__, evt); > > Use dev_err().
OK, done.
> ... > > > + /* 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.
> ... > > > + 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.
> ... > > > + 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 :)
> ... > > > + div = min(max(div, 2U), 256U); > > clamp_t() / clamp_val() ?
OK, changed to `div = clamp_val(div, 2U, 256U);`
> ... > > > + 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.
> ... > > > +use_polling: > > + host->mmc->caps |= MMC_CAP_NEEDS_POLL; > > > + 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.
> ... > > > + mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev); > > > + /* 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.
> > + if (!mmc) > > + return -ENOMEM; > > ... > > > + /* 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.
> ... > > > +err: > > + mmc_free_host(mmc); > > + return ret; > > This... > > > +} > > + > > +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? > > + return 0; > > +} > > ... > > > + .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
| |