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)
>New in v5:
>
>MAINTAINERS:
>
> - picked up a/b Mateusz
>
>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 Joel
>
>>New in v4:
>>
>>Doc/dt/bindings/mmc/litex,mmc.yaml:
>>
>> - fixed `dt_binding_check` errors uncovered by Rob's script
>>
>>drivers/mmc/host/litex_mmc.c:
>>
>> - struct litex_mmc_host fields re-ordered so that `pahole` reports
>> no holes in either 32- or 64-bit builds
>> - litex_mmc_set_bus_width() now encapsulates check for
>> host->is_bus_width_set
>> - litex_mmc_request() - factor out dma data setup into separate
>> helper function: litex_mmc_do_dma()
>>
>>>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 informative MODULE_AUTHOR() entries
>>> - also added matching "Copyright" entries in file header
>>> - fixed description in Kconfig
>>> - fixed DT documentation
>>> - removed magic constants
>>> - removed litex_map_status(), have sdcard_wait_done() return *real*
>>> error codes directly instead.
>>> - streamlined litex_mmc_reponse_len()
>>> - call litex_mmc_set_bus_width() only once, and ensure it returns
>>> correct error code(s)
>>> - use readx_poll_timeout() -- more concise -- instead of
>>> read_poll_timeout()
>>> - use dev_err() in litex_mmc_send_cmd() (instead of pr_err())
>>> - litex_mmc_setclk() will update host->clock before returning
>>> - separate irq initialization into its own function,
>>> litex_mmc_irq_init()
>>> - document rationale for f_min, f_max
>>> - use dmam_alloc_coherent(), which simplifies cleanup significantly
>>> - large `if (data) { ... }` block in litex_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!
>>> - bump dma_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)
>>>
>>>> New in v2:
>>>> - reword info message in litex_set_clk()
>>>> - streamline code in litex_map_status()
>>>> - fix typos in Kconfig (thanks Randy Dunlap <[email protected]>)
>>>> - improvements suggested by Stafford Horne <[email protected]>
>>>> - allow COMPILE_TEST in Kconfig
>>>> - use read_poll_timeout() when waiting for cmd/data/DMA
>>>> xfer completion
>>>> - include interrupt.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
--
2.31.1
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.
Cc: Karol Gugala <[email protected]>
Cc: Mateusz Holenko <[email protected]>
Signed-off-by: Gabriel Somlo <[email protected]>
Acked-by: Joel Stanley <[email protected]>
Acked-by: Mateusz Holenko <[email protected]>
---
MAINTAINERS | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index dd36acc87ce6..88f105711b85 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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]>
--
2.31.1
LiteSDCard is a small footprint, configurable SDCard core for FPGA
based system on chips.
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Joel Stanley <[email protected]>
---
.../devicetree/bindings/mmc/litex,mmc.yaml | 72 +++++++++++++++++++
1 file changed, 72 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/litex,mmc.yaml
diff --git a/Documentation/devicetree/bindings/mmc/litex,mmc.yaml b/Documentation/devicetree/bindings/mmc/litex,mmc.yaml
new file mode 100644
index 000000000000..f57cf42b8db7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/litex,mmc.yaml
@@ -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>;
+ };
--
2.31.1
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!
Co-developed-by: Kamil Rakoczy <[email protected]>
Signed-off-by: Kamil Rakoczy <[email protected]>
Co-developed-by: Maciej Dudek <[email protected]>
Signed-off-by: Maciej Dudek <[email protected]>
Co-developed-by: Paul Mackerras <[email protected]>
Signed-off-by: Paul Mackerras <[email protected]>
Signed-off-by: Gabriel Somlo <[email protected]>
Cc: Mateusz Holenko <[email protected]>
Cc: Karol Gugala <[email protected]>
Cc: Joel Stanley <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: David Abdurachmanov <[email protected]>
Cc: Florent Kermarrec <[email protected]>
Reviewed-by: Joel Stanley <[email protected]>
---
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)
>New in v5:
> - shorter #define constant names (cosmetic, make them less unwieldy)
> - picked up reviewed-by Joel
>
>>New in v4:
>> - struct litex_mmc_host fields re-ordered so that `pahole` reports
>> no holes on either 32- or 64-bit builds
>> - litex_mmc_set_bus_width() now encapsulates check for
>> host->is_bus_width_set
>> - litex_mmc_request() - factor out dma data setup into separate
>> helper function: litex_mmc_do_dma()
>>
>>> New in v3:
>>> - fixed function signature (no line split), and naming (litex_mmc_*)
>>> - more informative MODULE_AUTHOR() entries
>>> - also added matching "Copyright" entries in file header
>>> - fixed description and dependencies in Kconfig
>>> - removed magic constants
>>> - removed litex_map_status(), have sdcard_wait_done() return *real*
>>> error codes directly instead.
>>> - streamlined litex_mmc_reponse_len()
>>> - call litex_mmc_set_bus_width() only once, and ensure it returns
>>> correct error code(s)
>>> - use readx_poll_timeout() -- more concise -- instead of
>>> read_poll_timeout()
>>> - use dev_err() in litex_mmc_send_cmd() (instead of pr_err())
>>> - litex_mmc_setclk() will update host->clock before returning
>>> - separate irq initialization into its own function,
>>> litex_mmc_irq_init()
>>> - document rationale for f_min, f_max
>>> - use dmam_alloc_coherent(), which simplifies cleanup significantly
>>> - bump dma_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)
>>> - large `if (data) { ... }` block in litex_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
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5af8494c31b5..c1b66d06d1c9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -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.
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index ea36d379bd3c..4e4ceb32c4b4 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -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
diff --git a/drivers/mmc/host/litex_mmc.c b/drivers/mmc/host/litex_mmc.c
new file mode 100644
index 000000000000..cbed97ca70d0
--- /dev/null
+++ b/drivers/mmc/host/litex_mmc.c
@@ -0,0 +1,659 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 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]>
+ *
+ */
+
+#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>
+
+#define LITEX_PHY_CARDDETECT 0x00
+#define LITEX_PHY_CLOCKERDIV 0x04
+#define LITEX_PHY_INITIALIZE 0x08
+#define LITEX_PHY_WRITESTATUS 0x0C
+#define LITEX_CORE_CMDARG 0x00
+#define LITEX_CORE_CMDCMD 0x04
+#define LITEX_CORE_CMDSND 0x08
+#define LITEX_CORE_CMDRSP 0x0C
+#define LITEX_CORE_CMDEVT 0x1C
+#define LITEX_CORE_DATAEVT 0x20
+#define LITEX_CORE_BLKLEN 0x24
+#define LITEX_CORE_BLKCNT 0x28
+#define LITEX_BLK2MEM_BASE 0x00
+#define LITEX_BLK2MEM_LEN 0x08
+#define LITEX_BLK2MEM_ENA 0x0C
+#define LITEX_BLK2MEM_DONE 0x10
+#define LITEX_BLK2MEM_LOOP 0x14
+#define LITEX_MEM2BLK_BASE 0x00
+#define LITEX_MEM2BLK_LEN 0x08
+#define LITEX_MEM2BLK_ENA 0x0C
+#define LITEX_MEM2BLK_DONE 0x10
+#define LITEX_MEM2BLK_LOOP 0x14
+#define LITEX_MEM2BLK 0x18
+#define LITEX_IRQ_STATUS 0x00
+#define LITEX_IRQ_PENDING 0x04
+#define LITEX_IRQ_ENABLE 0x08
+
+#define SD_CTL_DATA_XFER_NONE 0
+#define SD_CTL_DATA_XFER_READ 1
+#define SD_CTL_DATA_XFER_WRITE 2
+
+#define SD_CTL_RESP_NONE 0
+#define SD_CTL_RESP_SHORT 1
+#define SD_CTL_RESP_LONG 2
+#define SD_CTL_RESP_SHORT_BUSY 3
+
+#define SD_BIT_DONE BIT(0)
+#define SD_BIT_WR_ERR BIT(1)
+#define SD_BIT_TIMEOUT BIT(2)
+#define SD_BIT_CRC_ERR BIT(3)
+
+#define SD_SLEEP_US 5
+#define SD_TIMEOUT_US 20000
+
+#define SDIRQ_CARD_DETECT 1
+#define SDIRQ_SD_TO_MEM_DONE 2
+#define SDIRQ_MEM_TO_SD_DONE 4
+#define SDIRQ_CMD_DONE 8
+
+#define LITEX_MMC_OCR (MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 | \
+ MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 | \
+ MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36)
+
+struct litex_mmc_host {
+ struct mmc_host *mmc;
+ struct platform_device *dev;
+
+ void __iomem *sdphy;
+ void __iomem *sdcore;
+ void __iomem *sdreader;
+ void __iomem *sdwriter;
+ void __iomem *sdirq;
+
+ void *buffer;
+ size_t buf_size;
+ dma_addr_t dma;
+
+ struct completion cmd_done;
+ int irq;
+
+ unsigned int ref_clk;
+ unsigned int sd_clk;
+
+ u32 resp[4];
+ u16 rca;
+
+ bool is_bus_width_set;
+ bool app_cmd;
+};
+
+static int litex_mmc_sdcard_wait_done(void __iomem *reg)
+{
+ u8 evt;
+ int ret;
+
+ ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE),
+ SD_SLEEP_US, SD_TIMEOUT_US);
+ if (ret || (evt & SD_BIT_TIMEOUT))
+ return -ETIMEDOUT;
+ if (evt == SD_BIT_DONE)
+ return 0;
+ if (evt & SD_BIT_WR_ERR)
+ return -EIO;
+ if (evt & SD_BIT_CRC_ERR)
+ return -EILSEQ;
+ pr_err("%s: unknown error evt=%x\n", __func__, evt);
+ return -EINVAL;
+}
+
+static int litex_mmc_send_cmd(struct litex_mmc_host *host,
+ u8 cmd, u32 arg, u8 response_len, u8 transfer)
+{
+ struct device *dev = mmc_dev(host->mmc);
+ void __iomem *reg;
+ int ret;
+ u8 evt;
+
+ litex_write32(host->sdcore + LITEX_CORE_CMDARG, arg);
+ litex_write32(host->sdcore + LITEX_CORE_CMDCMD,
+ cmd << 8 | transfer << 5 | response_len);
+ litex_write8(host->sdcore + LITEX_CORE_CMDSND, 1);
+
+ /* 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.
+ */
+ if (host->irq > 0 &&
+ (transfer != SD_CTL_DATA_XFER_NONE ||
+ response_len == SD_CTL_RESP_SHORT_BUSY)) {
+ reinit_completion(&host->cmd_done);
+ litex_write32(host->sdirq + LITEX_IRQ_ENABLE,
+ SDIRQ_CMD_DONE | SDIRQ_CARD_DETECT);
+ wait_for_completion(&host->cmd_done);
+ }
+
+ ret = litex_mmc_sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT);
+ if (ret) {
+ dev_err(dev, "Command (cmd %d) error, status %d\n", cmd, ret);
+ return ret;
+ }
+
+ if (response_len != SD_CTL_RESP_NONE) {
+ int i;
+
+ reg = host->sdcore + LITEX_CORE_CMDRSP;
+ for (i = 0; i < 4; i++) {
+ host->resp[i] = litex_read32(reg);
+ reg += sizeof(u32);
+ }
+ }
+
+ if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR)
+ host->rca = (host->resp[3] >> 16) & 0xffff;
+
+ host->app_cmd = (cmd == MMC_APP_CMD);
+
+ if (transfer == SD_CTL_DATA_XFER_NONE)
+ return ret; /* OK from prior litex_mmc_sdcard_wait_done() */
+
+ ret = litex_mmc_sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT);
+ if (ret) {
+ dev_err(dev, "Data xfer (cmd %d) error, status %d\n", cmd, ret);
+ return ret;
+ }
+
+ /* wait for completion of (read or write) DMA transfer */
+ reg = (transfer == SD_CTL_DATA_XFER_READ) ?
+ host->sdreader + LITEX_BLK2MEM_DONE :
+ host->sdwriter + LITEX_MEM2BLK_DONE;
+ ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE),
+ SD_SLEEP_US, SD_TIMEOUT_US);
+ if (ret)
+ dev_err(dev, "DMA timeout (cmd %d)\n", cmd);
+
+ return ret;
+}
+
+static int litex_mmc_send_app_cmd(struct litex_mmc_host *host)
+{
+ return litex_mmc_send_cmd(host, MMC_APP_CMD, host->rca << 16,
+ SD_CTL_RESP_SHORT, SD_CTL_DATA_XFER_NONE);
+}
+
+static int litex_mmc_send_set_bus_w_cmd(struct litex_mmc_host *host, u32 width)
+{
+ return litex_mmc_send_cmd(host, SD_APP_SET_BUS_WIDTH, width,
+ SD_CTL_RESP_SHORT, SD_CTL_DATA_XFER_NONE);
+}
+
+static int litex_mmc_set_bus_width(struct litex_mmc_host *host)
+{
+ bool app_cmd_sent;
+ int ret;
+
+ if (host->is_bus_width_set)
+ return 0;
+
+ /* ensure 'app_cmd' precedes 'app_set_bus_width_cmd' */
+ app_cmd_sent = host->app_cmd; /* was preceding command app_cmd? */
+ if (!app_cmd_sent) {
+ ret = litex_mmc_send_app_cmd(host);
+ if (ret)
+ return ret;
+ }
+
+ /* litesdcard only supports 4-bit bus width */
+ ret = litex_mmc_send_set_bus_w_cmd(host, MMC_BUS_WIDTH_4);
+ if (ret)
+ return ret;
+
+ /* re-send 'app_cmd' if necessary */
+ if (app_cmd_sent) {
+ ret = litex_mmc_send_app_cmd(host);
+ if (ret)
+ return ret;
+ }
+
+ host->is_bus_width_set = true;
+
+ return 0;
+}
+
+static int litex_mmc_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 = !litex_read8(host->sdphy + LITEX_PHY_CARDDETECT);
+
+ /* ensure bus width will be set (again) upon card (re)insertion */
+ if (ret == 0)
+ host->is_bus_width_set = false;
+
+ return ret;
+}
+
+static irqreturn_t litex_mmc_interrupt(int irq, void *arg)
+{
+ struct mmc_host *mmc = arg;
+ struct litex_mmc_host *host = mmc_priv(mmc);
+ u32 pending = litex_read32(host->sdirq + LITEX_IRQ_PENDING);
+
+ /* Check for card change interrupt */
+ if (pending & SDIRQ_CARD_DETECT) {
+ litex_write32(host->sdirq + LITEX_IRQ_PENDING,
+ SDIRQ_CARD_DETECT);
+ mmc_detect_change(mmc, msecs_to_jiffies(10));
+ }
+
+ /* Check for command completed */
+ if (pending & SDIRQ_CMD_DONE) {
+ /* Disable it so it doesn't keep interrupting */
+ litex_write32(host->sdirq + LITEX_IRQ_ENABLE,
+ SDIRQ_CARD_DETECT);
+ complete(&host->cmd_done);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static u32 litex_mmc_response_len(struct mmc_command *cmd)
+{
+ if (cmd->flags & MMC_RSP_136)
+ return SD_CTL_RESP_LONG;
+ if (!(cmd->flags & MMC_RSP_PRESENT))
+ return SD_CTL_RESP_NONE;
+ if (cmd->flags & MMC_RSP_BUSY)
+ return SD_CTL_RESP_SHORT_BUSY;
+ return SD_CTL_RESP_SHORT;
+}
+
+static void litex_mmc_do_dma(struct litex_mmc_host *host, struct mmc_data *data,
+ unsigned int *len, bool *direct, u8 *transfer)
+{
+ struct device *dev = mmc_dev(host->mmc);
+ dma_addr_t dma;
+ int sg_count;
+
+ /* Try to DMA directly to/from the data buffer.
+ * We can do that if the buffer can be mapped for DMA
+ * in one contiguous chunk.
+ */
+ dma = host->dma;
+ *len = data->blksz * data->blocks;
+ sg_count = dma_map_sg(dev, data->sg, data->sg_len,
+ mmc_get_dma_dir(data));
+ if (sg_count == 1) {
+ dma = sg_dma_address(data->sg);
+ *len = sg_dma_len(data->sg);
+ *direct = true;
+ } else if (*len > host->buf_size)
+ *len = host->buf_size;
+
+ if (data->flags & MMC_DATA_READ) {
+ litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
+ litex_write64(host->sdreader + LITEX_BLK2MEM_BASE, dma);
+ litex_write32(host->sdreader + LITEX_BLK2MEM_LEN, *len);
+ litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 1);
+ *transfer = SD_CTL_DATA_XFER_READ;
+ } else if (data->flags & MMC_DATA_WRITE) {
+ if (!*direct)
+ sg_copy_to_buffer(data->sg, data->sg_len,
+ host->buffer, *len);
+ litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);
+ litex_write64(host->sdwriter + LITEX_MEM2BLK_BASE, dma);
+ litex_write32(host->sdwriter + LITEX_MEM2BLK_LEN, *len);
+ litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 1);
+ *transfer = SD_CTL_DATA_XFER_WRITE;
+ } else {
+ dev_warn(dev, "Data present w/o read or write flag.\n");
+ /* Continue: set cmd status, mark req done */
+ }
+
+ litex_write16(host->sdcore + LITEX_CORE_BLKLEN, data->blksz);
+ litex_write32(host->sdcore + LITEX_CORE_BLKCNT, data->blocks);
+}
+
+static void litex_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct litex_mmc_host *host = mmc_priv(mmc);
+ struct device *dev = mmc_dev(mmc);
+ struct mmc_command *cmd = mrq->cmd;
+ struct mmc_command *sbc = mrq->sbc;
+ struct mmc_data *data = mrq->data;
+ struct mmc_command *stop = mrq->stop;
+ unsigned int retries = cmd->retries;
+ unsigned int len = 0;
+ bool direct = false;
+ u32 response_len = litex_mmc_response_len(cmd);
+ u8 transfer = SD_CTL_DATA_XFER_NONE;
+
+ /* First check that the card is still there */
+ if (!litex_mmc_get_cd(mmc)) {
+ cmd->error = -ENOMEDIUM;
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+
+ /* Send set-block-count command if needed */
+ if (sbc) {
+ sbc->error = litex_mmc_send_cmd(host, sbc->opcode, sbc->arg,
+ litex_mmc_response_len(sbc),
+ SD_CTL_DATA_XFER_NONE);
+ if (sbc->error) {
+ host->is_bus_width_set = false;
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+ }
+
+ if (data) {
+ /* LiteSDCard only supports 4-bit bus width; therefore, we MUST
+ * inject a SET_BUS_WIDTH (acmd6) before the very first data
+ * transfer, earlier than when the mmc subsystem would normally
+ * get around to it!
+ */
+ cmd->error = litex_mmc_set_bus_width(host);
+ if (cmd->error) {
+ dev_err(dev, "Can't set bus width!\n");
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+
+ litex_mmc_do_dma(host, data, &len, &direct, &transfer);
+ }
+
+ do {
+ cmd->error = litex_mmc_send_cmd(host, cmd->opcode, cmd->arg,
+ response_len, transfer);
+ } while (cmd->error && retries-- > 0);
+
+ if (cmd->error) {
+ /* card may be gone; don't assume bus width is still set */
+ host->is_bus_width_set = false;
+ }
+
+ if (response_len == SD_CTL_RESP_SHORT) {
+ /* pull short response fields from appropriate host registers */
+ cmd->resp[0] = host->resp[3];
+ cmd->resp[1] = host->resp[2] & 0xFF;
+ } else if (response_len == SD_CTL_RESP_LONG) {
+ cmd->resp[0] = host->resp[0];
+ cmd->resp[1] = host->resp[1];
+ cmd->resp[2] = host->resp[2];
+ cmd->resp[3] = host->resp[3];
+ }
+
+ /* Send stop-transmission command if required */
+ if (stop && (cmd->error || !sbc)) {
+ stop->error = litex_mmc_send_cmd(host, stop->opcode, stop->arg,
+ litex_mmc_response_len(stop),
+ SD_CTL_DATA_XFER_NONE);
+ if (stop->error)
+ host->is_bus_width_set = false;
+ }
+
+ if (data) {
+ dma_unmap_sg(dev, data->sg, data->sg_len,
+ mmc_get_dma_dir(data));
+ }
+
+ if (!cmd->error && transfer != SD_CTL_DATA_XFER_NONE) {
+ data->bytes_xfered = min(len, mmc->max_req_size);
+ if (transfer == SD_CTL_DATA_XFER_READ && !direct) {
+ sg_copy_from_buffer(data->sg, sg_nents(data->sg),
+ host->buffer, data->bytes_xfered);
+ }
+ }
+
+ mmc_request_done(mmc, mrq);
+}
+
+static void litex_mmc_setclk(struct litex_mmc_host *host, unsigned int freq)
+{
+ struct device *dev = mmc_dev(host->mmc);
+ u32 div;
+
+ div = freq ? host->ref_clk / freq : 256U;
+ div = roundup_pow_of_two(div);
+ div = min(max(div, 2U), 256U);
+ dev_dbg(dev, "sd_clk_freq=%d: set to %d via div=%d\n",
+ freq, host->ref_clk / div, div);
+ litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div);
+ host->sd_clk = freq;
+}
+
+static void litex_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct litex_mmc_host *host = mmc_priv(mmc);
+
+ /* NOTE: Ignore any ios->bus_width updates; they occur right after
+ * the mmc core sends its own acmd6 bus-width change notification,
+ * which is redundant since we snoop on the command flow and inject
+ * an early acmd6 before the first data transfer command is sent!
+ */
+
+ /* update sd_clk */
+ if (ios->clock != host->sd_clk)
+ litex_mmc_setclk(host, ios->clock);
+}
+
+static const struct mmc_host_ops litex_mmc_ops = {
+ .get_cd = litex_mmc_get_cd,
+ .request = litex_mmc_request,
+ .set_ios = litex_mmc_set_ios,
+};
+
+static int litex_mmc_irq_init(struct litex_mmc_host *host)
+{
+ struct device *dev = mmc_dev(host->mmc);
+ int ret;
+
+ 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;
+
+ host->sdirq = devm_platform_ioremap_resource_byname(host->dev, "irq");
+ if (IS_ERR(host->sdirq))
+ return PTR_ERR(host->sdirq);
+
+ ret = request_irq(host->irq, litex_mmc_interrupt, 0,
+ "litex-mmc", host->mmc);
+ if (ret < 0) {
+ dev_warn(dev, "IRQ request error %d, using polling\n", ret);
+ goto use_polling;
+ }
+
+ /* clear & enable card-change interrupts */
+ litex_write32(host->sdirq + LITEX_IRQ_PENDING, SDIRQ_CARD_DETECT);
+ litex_write32(host->sdirq + LITEX_IRQ_ENABLE, SDIRQ_CARD_DETECT);
+
+ return 0;
+
+use_polling:
+ host->mmc->caps |= MMC_CAP_NEEDS_POLL;
+ host->irq = 0;
+ return 0;
+}
+
+static int litex_mmc_probe(struct platform_device *pdev)
+{
+ struct litex_mmc_host *host;
+ struct mmc_host *mmc;
+ struct clk *clk;
+ int ret;
+
+ 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;`
+ */
+ if (!mmc)
+ return -ENOMEM;
+
+ host = mmc_priv(mmc);
+ host->mmc = mmc;
+ host->dev = pdev;
+
+ /* initialize clock source */
+ clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(clk)) {
+ ret = dev_err_probe(&pdev->dev,
+ PTR_ERR(clk), "can't get clock\n");
+ goto err;
+ }
+ host->ref_clk = clk_get_rate(clk);
+ host->sd_clk = 0;
+
+ /* LiteSDCard only supports 4-bit bus width; therefore, we MUST inject
+ * a SET_BUS_WIDTH (acmd6) before the very first data transfer, earlier
+ * than when the mmc subsystem would normally get around to it!
+ */
+ host->is_bus_width_set = false;
+ host->app_cmd = false;
+
+ /* LiteSDCard can support 64-bit DMA addressing */
+ ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+ if (ret)
+ goto err;
+
+ host->buf_size = mmc->max_req_size * 2;
+ host->buffer = dmam_alloc_coherent(&pdev->dev, host->buf_size,
+ &host->dma, GFP_DMA);
+ if (host->buffer == NULL) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ host->sdphy = devm_platform_ioremap_resource_byname(pdev, "phy");
+ if (IS_ERR(host->sdphy)) {
+ ret = PTR_ERR(host->sdphy);
+ goto err;
+ }
+
+ host->sdcore = devm_platform_ioremap_resource_byname(pdev, "core");
+ if (IS_ERR(host->sdcore)) {
+ ret = PTR_ERR(host->sdcore);
+ goto err;
+ }
+
+ host->sdreader = devm_platform_ioremap_resource_byname(pdev, "reader");
+ if (IS_ERR(host->sdreader)) {
+ ret = PTR_ERR(host->sdreader);
+ goto err;
+ }
+
+ host->sdwriter = devm_platform_ioremap_resource_byname(pdev, "writer");
+ if (IS_ERR(host->sdwriter)) {
+ ret = PTR_ERR(host->sdwriter);
+ goto err;
+ }
+
+ /* ensure DMA bus masters are disabled */
+ litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
+ litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);
+
+ init_completion(&host->cmd_done);
+ ret = litex_mmc_irq_init(host);
+ if (ret)
+ goto err;
+
+ /* allow full generic 2.7-3.6V range; no software tuning available */
+ mmc->ocr_avail = LITEX_MMC_OCR;
+
+ mmc->ops = &litex_mmc_ops;
+
+ /* set default sd_clk frequency range based on empirical observations
+ * of LiteSDCard gateware behavior on typical SDCard media
+ */
+ mmc->f_min = 12.5e6;
+ mmc->f_max = 50e6;
+
+ ret = mmc_of_parse(mmc);
+ if (ret)
+ goto err;
+
+ /* force 4-bit bus_width (only width supported by hardware) */
+ mmc->caps &= ~MMC_CAP_8_BIT_DATA;
+ mmc->caps |= MMC_CAP_4_BIT_DATA;
+
+ /* set default capabilities */
+ mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY |
+ MMC_CAP_DRIVER_TYPE_D |
+ MMC_CAP_CMD23;
+ mmc->caps2 |= MMC_CAP2_NO_WRITE_PROTECT |
+ MMC_CAP2_NO_SDIO |
+ MMC_CAP2_NO_MMC;
+
+ platform_set_drvdata(pdev, host);
+
+ ret = mmc_add_host(mmc);
+ if (ret < 0)
+ goto err;
+
+ return 0;
+
+err:
+ 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);
+ mmc_free_host(host->mmc);
+
+ return 0;
+}
+
+static const struct of_device_id litex_match[] = {
+ { .compatible = "litex,mmc" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, litex_match);
+
+static struct platform_driver litex_mmc_driver = {
+ .probe = litex_mmc_probe,
+ .remove = litex_mmc_remove,
+ .driver = {
+ .name = "litex-mmc",
+ .of_match_table = of_match_ptr(litex_match),
+ },
+};
+module_platform_driver(litex_mmc_driver);
+
+MODULE_DESCRIPTION("LiteX SDCard driver");
+MODULE_AUTHOR("Antmicro <[email protected]>");
+MODULE_AUTHOR("Kamil Rakoczy <[email protected]>");
+MODULE_AUTHOR("Maciej Dudek <[email protected]>");
+MODULE_AUTHOR("Paul Mackerras <[email protected]>");
+MODULE_AUTHOR("Gabriel Somlo <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.31.1
On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <[email protected]> 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 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.
> + */
...
> +#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.
...
> + ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE),
Too many parentheses.
> + SD_SLEEP_US, SD_TIMEOUT_US);
> + if (ret || (evt & SD_BIT_TIMEOUT))
Redundant second condition. If you want +1 iteration, increase timeout.
> + return -ETIMEDOUT;
Why shadowed error code?
...
> + pr_err("%s: unknown error evt=%x\n", __func__, evt);
Use dev_err().
...
> + /* 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.
...
> + 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()?
...
> + 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?
...
> + div = min(max(div, 2U), 256U);
clamp_t() / clamp_val() ?
...
> + 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;
}
?
...
> +use_polling:
> + host->mmc->caps |= MMC_CAP_NEEDS_POLL;
> + host->irq = 0;
Isn't it 0 by default?
...
> + 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?
> + 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.
...
> +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?
> + return 0;
> +}
...
> + .of_match_table = of_match_ptr(litex_match),
Wrong usage of of_match_ptr().
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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 <[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.
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
Hi Gabriel,
On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
> On Thu, Jan 06, 2022 at 08:19:39PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <[email protected]> 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)?
Many (most?) blurbs do mention the module name.
> > > + 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
On Fri, Jan 07, 2022 at 10:36:12AM +0100, Geert Uytterhoeven wrote:
> Hi Gabriel,
>
> On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
> > On Thu, Jan 06, 2022 at 08:19:39PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <[email protected]> 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)?
>
> 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
On Fri, Jan 7, 2022 at 12:12 PM Stafford Horne <[email protected]> wrote:
> On Fri, Jan 07, 2022 at 10:36:12AM +0100, Geert Uytterhoeven wrote:
> > On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
...
> > 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
> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Fri, Jan 7, 2022 at 11:25 AM Andy Shevchenko
<[email protected]> wrote:
> On Fri, Jan 7, 2022 at 12:12 PM Stafford Horne <[email protected]> wrote:
> > On Fri, Jan 07, 2022 at 10:36:12AM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
>
> ...
>
> > > 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.
Indeed.
> 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
On Fri, Jan 7, 2022 at 11:29 AM Geert Uytterhoeven <[email protected]> wrote:
> On Fri, Jan 7, 2022 at 11:25 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, Jan 7, 2022 at 12:12 PM Stafford Horne <[email protected]> wrote:
> > > On Fri, Jan 07, 2022 at 10:36:12AM +0100, Geert Uytterhoeven wrote:
> > > > On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
> >
> > ...
> >
> > > > 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.
>
> Indeed.
>
> > 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> -- ...)
>
> ;-)
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
On Fri, Jan 7, 2022 at 12:30 PM Geert Uytterhoeven <[email protected]> wrote:
> On Fri, Jan 7, 2022 at 11:29 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Fri, Jan 7, 2022 at 11:25 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Fri, Jan 7, 2022 at 12:12 PM Stafford Horne <[email protected]> wrote:
...
> > > 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> -- ...)
> >
> > ;-)
>
> 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.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 07, 2022 at 12:24:23PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 7, 2022 at 12:12 PM Stafford Horne <[email protected]> wrote:
> > On Fri, Jan 07, 2022 at 10:36:12AM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
>
> ...
>
> > > 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.
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.659s
> 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.
> > 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.
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
On Fri, Jan 07, 2022 at 07:12:43PM +0900, Stafford Horne wrote:
> On Fri, Jan 07, 2022 at 10:36:12AM +0100, Geert Uytterhoeven wrote:
> > Hi Gabriel,
> >
> > On Thu, Jan 6, 2022 at 11:50 PM Gabriel L. Somlo <[email protected]> wrote:
> > > On Thu, Jan 06, 2022 at 08:19:39PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <[email protected]> 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)?
> >
> > 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
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