mmc: Add LiteSDCard mmc driver
Subsystems (3)
v6 cover mmc: Add LiteSDCard mmc driver
Add support for the LiteX SD-Card device, LiteSDCard.
LiteSDCard is a simple SD-Card interface available as part of the LiteX environment, used with various RISC-V and other FPGA based SoCs.
New in v6:
drivers/mmc/host/litex_mmc.c:
- fix handling of deferred probe vs. platform_get_irq_optional()
- don't #ifdef dma_set_mask_and_coherent(), since it automatically does the right thing on both 32- and 64-bit DMA capable arches
- remove MMC_CAP2_FULL_PWR_CYCLE, add MMC_CAP2_NO_MMC to list of hardcoded capabilities during litex_mmc_probe()
- hardcode mmc->ocr_avail to the full 2.7-3.6V range allowed by the SDCard spec (the LiteSDCard device doesn't accept software configuration)
Quoted 6 lines
New in v5:
MAINTAINERS:- picked up a/b Mateusz
Quoted 80 lines
Doc/dt/bindings/mmc/litex,mmc.yaml:- picked up r/b Rob, Joel
drivers/mmc/host/litex_mmc.c:- shorten #define constant names (cosmetic, make them less unwieldy)
- picked up r/b JoelQuoted 71 lines
New in v4:
Doc/dt/bindings/mmc/litex,mmc.yaml:- fixed
dt_binding_checkerrors uncovered by Rob's script
drivers/mmc/host/litex_mmc.c:- struct
litex_mmc_hostfields re-ordered so thatpaholereports no holes in either 32- or 64-bit builds
-litex_mmc_set_bus_width()now encapsulates check forhost->is_bus_width_set
-litex_mmc_request()- factor out dma data setup into separate helper function:litex_mmc_do_dma()Quoted 56 lines
New in v3:
MAINTAINERS:- picked up acked-by Joel
- added listing for liteeth driver
- added Joel as additional co-maintainer (thanks!)
Doc/dt/bindings/mmc/litex,mmc.yaml:- picked up r/b Geert Uytterhoeven <[email protected]> in DT bindings document (please let me know if that was premature, and happy to take further review if needed :)
- add dedicated DT property for source clock frequency
drivers/mmc/host/litex_mmc.c:- fixed function signature (no line split), and naming (litex_mmc_*)
- more informativeMODULE_AUTHOR()entries
- also added matching "Copyright" entries in file header
- fixed description inKconfig
- fixed DT documentation
- removed magic constants
- removedlitex_map_status(), havesdcard_wait_done()return *real* error codes directly instead.
- streamlinedlitex_mmc_reponse_len()
- calllitex_mmc_set_bus_width()only once, and ensure it returns correct error code(s)
- usereadx_poll_timeout()-- more concise -- instead ofread_poll_timeout()
- usedev_err()inlitex_mmc_send_cmd()(instead ofpr_err())
-litex_mmc_setclk()will updatehost->clockbefore returning
- separate irq initialization into its own function,litex_mmc_irq_init()
- document rationale forf_min,f_max
- usedmam_alloc_coherent(), which simplifies cleanup significantly
- largeif (data) { ... }block inlitex_mmc_request()left as-is, there are too many variables shared with the rest of the parent function body to easily separate (e.g.,len,transfer,direct). If this is indeed a blocker, I can take another shot at refactoring it in a future revision!
- bumpdma_set_mask_and_coherent()to 64-bits on suitable architectures
- clock source picked up from dedicated DT clock reference property
- remove gpio card-detect logic (needs testing and a dt binding example before being eligible for upstream inclusion)Quoted 9 lines
New in v2:
- reword info message inlitex_set_clk()
- streamline code inlitex_map_status()
- fix typos inKconfig(thanks Randy Dunlap <[email protected]>)
- improvements suggested by Stafford Horne <[email protected]>
- allowCOMPILE_TESTinKconfig
- useread_poll_timeout()when waiting for cmd/data/DMA xfer completion
- includeinterrupt.h(thanks kernel test robot <[email protected]>)
Gabriel Somlo (3):
MAINTAINERS: co-maintain LiteX platform
dt-bindings: mmc: Add bindings for LiteSDCard
mmc: Add driver for LiteX's LiteSDCard interface
.../devicetree/bindings/mmc/litex,mmc.yaml | 72 ++ MAINTAINERS | 9 +- drivers/mmc/host/Kconfig | 9 + drivers/mmc/host/Makefile | 1 + drivers/mmc/host/litex_mmc.c | 659 ++++++++++++++++++ 5 files changed, 748 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/mmc/litex,mmc.yaml create mode 100644 drivers/mmc/host/litex_mmc.c
Signature · 1 line
2.31.1
PATCH v6 1/3 MAINTAINERS: co-maintain LiteX platform
Add the litex_mmc (LiteSDCard) and LiteETH drivers to the list of files maintained under LiteX.
Add Gabriel Somlo and Joel Stanley as maintainers; Joel authored the LiteETH driver, and Gabriel is currently curating the LiteX out-of-tree device drivers as they are tested and prepared for upstream submission, having also co-authored a number of them.
--- MAINTAINERS | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
MAINTAINERS +7 -2
@@ -11012,12 +11012,17 @@ F: lib/list-test.c LITEX PLATFORM M: Karol Gugala <[email protected]> M: Mateusz Holenko <[email protected]> +M: Gabriel Somlo <[email protected]> +M: Joel Stanley <[email protected]> S: Maintained F: Documentation/devicetree/bindings/*/litex,*.yaml F: arch/openrisc/boot/dts/or1klitex.dts -F: drivers/soc/litex/litex_soc_ctrl.c -F: drivers/tty/serial/liteuart.c F: include/linux/litex.h +F: drivers/tty/serial/liteuart.c +F: drivers/soc/litex/* +F: drivers/net/ethernet/litex/* +F: drivers/mmc/host/litex_mmc.c +N: litex LIVE PATCHING M: Josh Poimboeuf <[email protected]>
Signature · 1 line
2.31.1
PATCH v6 2/3 dt-bindings: mmc: Add bindings for LiteSDCard
In reply to Gabriel SomloLiteSDCard is a small footprint, configurable SDCard core for FPGA based system on chips.
--- .../devicetree/bindings/mmc/litex,mmc.yaml | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 Documentation/devicetree/bindings/mmc/litex,mmc.yaml
Documentation/devicetree/bindings/mmc/litex,mmc.yaml new file +72 -0
@@ -0,0 +1,72 @@ +# SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mmc/litex,mmc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LiteX LiteSDCard device + +maintainers: + - Gabriel Somlo <[email protected]> + +description: | + LiteSDCard is a small footprint, configurable SDCard core for FPGA based + system on chips. + + The hardware source is Open Source and can be found on at + https://github.com/enjoy-digital/litesdcard/. + +allOf: + - $ref: mmc-controller.yaml# + +properties: + compatible: + const: litex,mmc + + reg: + items: + - description: PHY registers + - description: CORE registers + - description: DMA Reader buffer + - description: DMA Writer buffer + - description: IRQ registers + minItems: 4 + + reg-names: + items: + - const: phy + - const: core + - const: reader + - const: writer + - const: irq + minItems: 4 + + clocks: + maxItems: 1 + description: + Handle to reference clock. + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - reg-names + - clocks + +additionalProperties: false + +examples: + - | + mmc: mmc@12005000 { + compatible = "litex,mmc"; + reg = <0x12005000 0x100>, + <0x12003800 0x100>, + <0x12003000 0x100>, + <0x12004800 0x100>, + <0x12004000 0x100>; + reg-names = "phy", "core", "reader", "writer", "irq"; + clocks = <&reference_clk>; + interrupts = <4>; + };
Signature · 1 line
2.31.1
PATCH v6 3/3 mmc: Add driver for LiteX's LiteSDCard interface
In reply to Gabriel SomloLiteX (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!
---
New in v6:
- fix handling of deferred probe vs. platform_get_irq_optional()
- don't #ifdef dma_set_mask_and_coherent(), since it automatically does the right thing on both 32- and 64-bit DMA capable arches
- remove MMC_CAP2_FULL_PWR_CYCLE, add MMC_CAP2_NO_MMC to list of hardcoded capabilities during litex_mmc_probe()
- hardcode mmc->ocr_avail to the full 2.7-3.6V range allowed by the SDCard spec (the LiteSDCard device doesn't accept software configuration)
Quoted 41 lines
New in v5:
- shorter #define constant names (cosmetic, make them less unwieldy)
- picked up reviewed-by JoelQuoted 37 lines
New in v4:
- structlitex_mmc_hostfields re-ordered so thatpaholereports no holes on either 32- or 64-bit builds
-litex_mmc_set_bus_width()now encapsulates check forhost->is_bus_width_set
-litex_mmc_request()- factor out dma data setup into separate helper function:litex_mmc_do_dma()Quoted 29 lines
New in v3:
- fixed function signature (no line split), and naming (litex_mmc_*)
- more informativeMODULE_AUTHOR()entries
- also added matching "Copyright" entries in file header
- fixed description and dependencies inKconfig
- removed magic constants
- removedlitex_map_status(), havesdcard_wait_done()return *real* error codes directly instead.
- streamlinedlitex_mmc_reponse_len()
- calllitex_mmc_set_bus_width()only once, and ensure it returns correct error code(s)
- usereadx_poll_timeout()-- more concise -- instead ofread_poll_timeout()
- usedev_err()inlitex_mmc_send_cmd()(instead ofpr_err())
-litex_mmc_setclk()will updatehost->clockbefore returning
- separate irq initialization into its own function,litex_mmc_irq_init()
- document rationale forf_min,f_max
- usedmam_alloc_coherent(), which simplifies cleanup significantly
- bumpdma_set_mask_and_coherent()to 64-bits on suitable architectures
- clock source picked up from dedicated DT clock reference property
- remove gpio card-detect logic (needs testing and a dt binding example before being eligible for upstream inclusion)
- largeif (data) { ... }block inlitex_mmc_request()left as-is, there are too many variables shared with the rest of the parent function body to easily separate (e.g.,len,transfer,direct). If this is indeed a blocker, I can take another shot at refactoring it in a future revision!
drivers/mmc/host/Kconfig | 9 + drivers/mmc/host/Makefile | 1 + drivers/mmc/host/litex_mmc.c | 659 +++++++++++++++++++++++++++++++++++ 3 files changed, 669 insertions(+) create mode 100644 drivers/mmc/host/litex_mmc.c
drivers/mmc/host/Kconfig +9 -0
@@ -1093,3 +1093,12 @@ config MMC_OWL config MMC_SDHCI_EXTERNAL_DMA bool + +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.
drivers/mmc/host/Makefile +1 -0
@@ -101,6 +101,7 @@ obj-$(CONFIG_MMC_CQHCI) += cqhci.o cqhci-y += cqhci-core.o cqhci-$(CONFIG_MMC_CRYPTO) += cqhci-crypto.o obj-$(CONFIG_MMC_HSQ) += mmc_hsq.o +obj-$(CONFIG_MMC_LITEX) += litex_mmc.o ifeq ($(CONFIG_CB710_DEBUG),y) CFLAGS-cb710-mmc += -DDEBUG
drivers/mmc/host/litex_mmc.c new file +659 -0
659 lines not on this page · raw message
Signature · 1 line
2.31.1
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Quoted 8 lines · Gabriel Somlo
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!
drivers/mmc/host/Kconfig config MMC_OWL · line 1104
Quoted 8 lines
drivers/mmc/host/Kconfig +8
-0+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?
...
Quoted 10 lines
+/*
+ * LiteX LiteSDCard driver
+ *
+ * Copyright (C) 2019-2020 Antmicro <[email protected]>
+ * Copyright (C) 2019-2020 Kamil Rakoczy <[email protected]>
+ * Copyright (C) 2019-2020 Maciej Dudek <[email protected]>
+ * Copyright (C) 2020 Paul Mackerras <[email protected]>
+ * Copyright (C) 2020-2021 Gabriel Somlo <[email protected]>+ *
Redundant blank line.
Quoted 1 line
+ */
...
drivers/mmc/host/litex_mmc.c line 24
Quoted 12 lines
drivers/mmc/host/litex_mmc.c new file +12
-0+#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?
It will easily show, for example, absence of bits.h.
...
Quoted 1 line
+ ret = readx_poll_timeout(litex_read8, reg, evt, (evt &
SD_BIT_DONE),
Too many parentheses.
drivers/mmc/host/litex_mmc.c line 113
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +2
-0+ SD_SLEEP_US, SD_TIMEOUT_US); +if (ret || (evt &SD_BIT_TIMEOUT ))
Redundant second condition. If you want +1 iteration, increase timeout.
Quoted 1 line
+ return
-ETIMEDOUT;
Why shadowed error code?
...
Quoted 1 line
+
pr_err("%s: unknown error evt=%x\n", __func__, evt);
Use dev_err().
...
drivers/mmc/host/litex_mmc.c line 140
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+/* 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.
...
drivers/mmc/host/litex_mmc.c line 163
Quoted 5 lines
drivers/mmc/host/litex_mmc.c new file +5
-0+ 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()?
...
drivers/mmc/host/litex_mmc.c line 167
Quoted 2 lines
drivers/mmc/host/litex_mmc.c new file +2
-0+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?
...
Quoted 1 line
+ div =
min(max(div, 2U), 256U);
clamp_t() / clamp_val() ?
...
drivers/mmc/host/litex_mmc.c line 477
Quoted 8 lines
drivers/mmc/host/litex_mmc.c new file +8
-0+ 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;
}?
...
drivers/mmc/host/litex_mmc.c line 498
Quoted 4 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+use_polling: + host->mmc->caps |=MMC_CAP_NEEDS_POLL ; + host->irq =0 ;
Isn't it 0 by default?
...
drivers/mmc/host/litex_mmc.c line 514
Quoted 7 lines
drivers/mmc/host/litex_mmc.c new file +6
-0+ 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?
Quoted 2 lines
drivers/mmc/host/litex_mmc.c new file +2
-0+if (!mmc) +return -ENOMEM ;
...
drivers/mmc/host/litex_mmc.c line 592
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+/* 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.
...
drivers/mmc/host/litex_mmc.c line 622
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+err: +mmc_free_host (mmc); +return ret;
This...
drivers/mmc/host/litex_mmc.c line 632
Quoted 10 lines
drivers/mmc/host/litex_mmc.c new file +10
-0+} + +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?
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
...
Quoted 1 line
+ .of_match_table =
of_match_ptr(litex_match),
Wrong usage of of_match_ptr().
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Hi Andy,
Thanks again, replies and follow-up questions inline below:
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Gabriel Somlo
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 10 lines
drivers/mmc/host/Kconfig config MMC_OWL · line 1104
Quoted 8 lines
drivers/mmc/host/Kconfig +8
-0+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 11 lines
...
Quoted 8 lines
+/*
+ * LiteX LiteSDCard driver
+ *
+ * Copyright (C) 2019-2020 Antmicro <[email protected]>
+ * Copyright (C) 2019-2020 Kamil Rakoczy <[email protected]>
+ * Copyright (C) 2019-2020 Maciej Dudek <[email protected]>
+ * Copyright (C) 2020 Paul Mackerras <[email protected]>
+ * Copyright (C) 2020-2021 Gabriel Somlo <[email protected]>
Quoted 3 lines
Quoted 1 line
+ *
Redundant blank line.
OK, removed.
Quoted 5 lines
Quoted 2 lines
+ */
...
Quoted 14 lines
drivers/mmc/host/litex_mmc.c line 24
Quoted 12 lines
drivers/mmc/host/litex_mmc.c new file +12
-0+#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>
Quoted 1 line
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 4 lines
Quoted 2 lines
+ 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 6 lines
drivers/mmc/host/litex_mmc.c line 113
Quoted 4 lines
drivers/mmc/host/litex_mmc.c new file +2
-0+ 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.
Quoted 3 lines
Quoted 1 line
+ 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 5 lines
...
Quoted 1 line
+
pr_err("%s: unknown error evt=%x\n", __func__, evt);Use
dev_err().
OK, done.
Quoted 6 lines
...
drivers/mmc/host/litex_mmc.c line 140
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+/* 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. +*/
Quoted 6 lines
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 9 lines
...
drivers/mmc/host/litex_mmc.c line 163
Quoted 5 lines
drivers/mmc/host/litex_mmc.c new file +5
-0+ 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 6 lines
...
drivers/mmc/host/litex_mmc.c line 167
Quoted 2 lines
drivers/mmc/host/litex_mmc.c new file +2
-0+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 5 lines
...
Quoted 1 line
+ div =
min(max(div, 2U), 256U);
clamp_t()/clamp_val()?
OK, changed to div = clamp_val(div, 2U, 256U);
Quoted 23 lines
...
drivers/mmc/host/litex_mmc.c line 477
Quoted 8 lines
drivers/mmc/host/litex_mmc.c new file +8
-0+ 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; }
Quoted 1 line
?
OK, done.
Quoted 8 lines
...
drivers/mmc/host/litex_mmc.c line 498
Quoted 4 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+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.
Quoted 11 lines
...
drivers/mmc/host/litex_mmc.c line 514
Quoted 7 lines
drivers/mmc/host/litex_mmc.c new file +6
-0+ 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.
Quoted 11 lines
Quoted 2 lines
drivers/mmc/host/litex_mmc.c new file +2
-0+if (!mmc) +return -ENOMEM ;...
drivers/mmc/host/litex_mmc.c line 592
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+/* 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 22 lines
...
drivers/mmc/host/litex_mmc.c line 622
Quoted 3 lines
drivers/mmc/host/litex_mmc.c new file +3
-0+err: +mmc_free_host (mmc); +return ret;This...
drivers/mmc/host/litex_mmc.c line 632
Quoted 10 lines
drivers/mmc/host/litex_mmc.c new file +10
-0+} + +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.
Quoted 2 lines
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 8 lines
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}...
Quoted 1 line
+ .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
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Hi Gabriel,
Quoted 21 lines · Gabriel L. Somlo
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Gabriel Somlo
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 10 lines
drivers/mmc/host/Kconfig config MMC_OWL · line 1104
Quoted 8 lines
drivers/mmc/host/Kconfig +8
-0+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?
Quoted 2 lines
litex_mmc.ko -- why are you asking? I.e., should I mention that anywhere in the
Kconfigblurb (I don't see other blurbs doing that, fwiw)?
Many (most?) blurbs do mention the module name.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+ div =
min(max(div, 2U), 256U);
clamp_t()/clamp_val()?OK, changed to
div = clamp_val(div, 2U, 256U);
Please use clamp() instead of clamp_val(), as all three parameters have the same type (clamp_val() uses casts to align all parameters; casts are evil).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Quoted 24 lines · Geert Uytterhoeven
Hi Gabriel,
Quoted 21 lines · Gabriel L. Somlo
Quoted 19 lines · Andy Shevchenko
Quoted 8 lines · Gabriel Somlo
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!
drivers/mmc/host/Kconfig config MMC_OWL · line 1104
Quoted 8 lines
drivers/mmc/host/Kconfig +8
-0+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?
Quoted 4 lines
Quoted 2 lines
litex_mmc.ko -- why are you asking? I.e., should I mention that anywhere in the
Kconfigblurb (I don't see other blurbs doing that, fwiw)?Many (most?) blurbs do mention the module name.
I was doubting this as well, but I searched and its true. The text 'module will be called' shows up many times, there is also different text.
$ grep -r 'module will be called' drivers/ | wc 1347 9023 9086
$ grep -r 'tristate \"' drivers/ | wc 7169 47486 521795
So maybe >10% have module name in the blurb. Example:
To compile this driver as a module, choose M here: the module will be called tifm_sd.
Thanks,
-Stafford
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Quoted 2 lines · Stafford Horne
Quoted 1 line · Geert Uytterhoeven
On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
...
Quoted 10 lines
Quoted 1 line
Many (most?) blurbs do mention the module name.
I was doubting this as well, but I searched and its true. The text 'module will be called' shows up many times, there is also different text.
$ grep -r 'module will be called' drivers/ | wc 1347 9023 9086$ grep -r 'tristate \"' drivers/ | wc 7169 47486 521795
Just a side note: git grep ... is much faster in the Git trees.
And for this particular case I dare to advertise a script I wrote [1] to help with recursive searches.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/gl4func.sh
Quoted 4 lines
So maybe >10% have module name in the blurb. Example:
To compile this driver as a module, choose M here: the module will be called
tifm_sd.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Hi Andy,
Quoted 11 lines · Andy Shevchenko
Quoted 2 lines · Stafford Horne
Quoted 1 line · Geert Uytterhoeven
On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
...
Quoted 5 lines
Quoted 1 line
Many (most?) blurbs do mention the module name.
I was doubting this as well, but I searched and its true. The text 'module will be called' shows up many times, there is also different text.
Quoted 7 lines
Quoted 5 lines
$ grep -r 'module will be called' drivers/ | wc 1347 9023 9086$ grep -r 'tristate \"' drivers/ | wc 7169 47486 521795Just a side note:
git grep ...is much faster in the Git trees.
Indeed.
Quoted 4 lines
And for this particular case I dare to advertise a script I wrote [1] to help with recursive searches.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/gl4func.sh
Cool!
My fingers are used to type
git grep -w <pat1> -- $(git grep -lw <pat2> -- ...)
;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Quoted 28 lines · Geert Uytterhoeven
Quoted 11 lines · Andy Shevchenko
Quoted 2 lines · Stafford Horne
Quoted 1 line · Geert Uytterhoeven
On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
...
Quoted 5 lines
Quoted 1 line
Many (most?) blurbs do mention the module name.
I was doubting this as well, but I searched and its true. The text 'module will be called' shows up many times, there is also different text.
Quoted 7 lines
Quoted 5 lines
$ grep -r 'module will be called' drivers/ | wc 1347 9023 9086$ grep -r 'tristate \"' drivers/ | wc 7169 47486 521795Just a side note:
git grep ...is much faster in the Git trees.Indeed.
Quoted 4 lines
And for this particular case I dare to advertise a script I wrote [1] to help with recursive searches.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/gl4func.sh
Quoted 7 lines
Cool!
My fingers are used to type
git grep -w <pat1> -- $(git grep -lw <pat2> -- ...);-)
Actually you can just use git grep --and?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Quoted 4 lines · Geert Uytterhoeven
Quoted 3 lines · Geert Uytterhoeven
Quoted 1 line · Andy Shevchenko
On Fri, Jan 7, 2022 at 12:12 PM Stafford Horne <[email protected]> wrote:
...
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
And for this particular case I dare to advertise a script I wrote [1] to help with recursive searches.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/gl4func.sh
Cool!
Quoted 7 lines
Quoted 5 lines
My fingers are used to type
git grep -w <pat1> -- $(git grep -lw <pat2> -- ...);-)
Actually you can just use git grep --and?
Thanks for the hint (and patches are welcome :-).
The last time I have read the man of git grep was a long time ago, and I definitely missed any new features.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
In reply to Andy ShevchenkoQuoted 11 lines · Andy Shevchenko
Quoted 2 lines · Stafford Horne
Quoted 1 line · Geert Uytterhoeven
On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
...
Quoted 5 lines
Quoted 1 line
Many (most?) blurbs do mention the module name.
I was doubting this as well, but I searched and its true. The text 'module will be called' shows up many times, there is also different text.
Quoted 7 lines
Quoted 5 lines
$ grep -r 'module will be called' drivers/ | wc 1347 9023 9086$ grep -r 'tristate \"' drivers/ | wc 7169 47486 521795Just a side note:
git grep ...is much faster in the Git trees.
Yes, it is quite a lot faster, I always wondered why one would use it rather than just grep. Thanks for the tip.
< shorne@antec ~/work/linux > time grep -r 'module will be called' drivers/ >/dev/null
real 0m0.338s
user 0m0.220s
sys 0m0.113s
< shorne@antec ~/work/linux > time git grep 'module will be called' -- drivers/ >/dev/null
real 0m0.153s
user 0m0.205s
sys 0m0.659sQuoted 4 lines
And for this particular case I dare to advertise a script I wrote [1] to help with recursive searches.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/gl4func.sh
Neat script.
Quoted 10 lines
Quoted 4 lines
So maybe >10% have module name in the blurb. Example:
To compile this driver as a module, choose M here: the module will be called
tifm_sd.Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
In reply to Stafford HorneQuoted 30 lines · Stafford Horne
Quoted 24 lines · Geert Uytterhoeven
Hi Gabriel,
Quoted 21 lines · Gabriel L. Somlo
Quoted 19 lines · Andy Shevchenko
Quoted 8 lines · Gabriel Somlo
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!
drivers/mmc/host/Kconfig config MMC_OWL · line 1104
Quoted 8 lines
drivers/mmc/host/Kconfig +8
-0+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?
Quoted 4 lines
Quoted 2 lines
litex_mmc.ko -- why are you asking? I.e., should I mention that anywhere in the
Kconfigblurb (I don't see other blurbs doing that, fwiw)?Many (most?) blurbs do mention the module name.
Quoted 8 lines
I was doubting this as well, but I searched and its true. The text 'module will be called' shows up many times, there is also different text.
$ grep -r 'module will be called' drivers/ | wc 1347 9023 9086$ grep -r 'tristate \"' drivers/ | wc 7169 47486 521795
OK, so initially I quickly looked at a few entries before LiteX in the drivers/mmc/host/Kconfig file, and saw nothing useful, and asked a follow-up question... :)
Knowing the magic "module will be called" incantation to grep for helped, and now I created a similar one for LiteSDCard's Kconfig entry, and lined it up for v7.
Thanks to everyone who chimed in!
Best,
--Gabriel