Skip to contentSkip to conversation

mmc: Add LiteSDCard mmc driver

PATCH Superseded Gabriel Somlo 2 participants 13 messages Jan 7, 2022 2 messages not in the archive

Subsystems (3)

v7 of 14 Superseded Latest is v14 →

lore
Share & tools
lore
#0 lore raw

v7 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 v7:

drivers/mmc/host/Kconfig:
- added module name in LiteSDCard Kconfig entry

drivers/mmc/host/litex_mmc.c:
- fixed comment formatting, ordering, and capitalization throughout the entire file
- sorted header #include statements
- removed redundant parantheses in readx_poll_timeout() condition
- explicit handling of readx_poll_timeout() timeout scenarios
- dev_err() used in litex_mmc_sdcard_wait_done()
- use memcpy_fromio() to grab command response
- no need to apply 0xffff mask to a 32-bit value right-shifted by 16

    (host->resp[3])

- use clamp() instead of min(max(...)...)
- reworked platform_get_irq_optional() error handling logic
- no need to explicitly zero host->irq, kzalloc() does that already
- added missing free_irq() in litex_mmc_probe() error path
- reordered calls inside litex_mmc_remove() (calling mmc_free_host() before free_irq()

Quoted 98 lines

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 86 lines

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

Quoted 71 lines

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()

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

Quoted 9 lines

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                      |  10 +
 drivers/mmc/host/Makefile                     |   1 +
 drivers/mmc/host/litex_mmc.c                  | 670 ++++++++++++++++++
 5 files changed, 760 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
#1 lore raw

PATCH v7 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
index dd36acc87ce6..88f105711b85 100644
@@ -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
#2 lore raw

PATCH v7 2/3 dt-bindings: mmc: Add bindings for LiteSDCard

In reply to Gabriel Somlo

LiteSDCard 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
new file mode 100644
index 000000000000..f57cf42b8db7
@@ -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
#3 lore raw

PATCH v7 3/3 mmc: Add driver for LiteX's LiteSDCard interface

In reply to 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!

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]>

---

New in v7:
drivers/mmc/host/Kconfig:
- added module name in LiteSDCard Kconfig entry
drivers/mmc/host/litex_mmc.c:
- fixed comment formatting, ordering, and capitalization throughout the entire file
- sorted header #include statements
- removed redundant parantheses in readx_poll_timeout() condition
- explicit handling of readx_poll_timeout() timeout scenarios
- dev_err() used in litex_mmc_sdcard_wait_done()
- use memcpy_fromio() to grab command response
- no need to apply 0xffff mask to a 32-bit value right-shifted by 16

    (host->resp[3])

- use clamp() instead of min(max(...)...)
- reworked platform_get_irq_optional() error handling logic
- no need to explicitly zero host->irq, kzalloc() does that already
- added missing free_irq() in litex_mmc_probe() error path
- reordered calls inside litex_mmc_remove() (calling mmc_free_host() before free_irq()

Quoted 51 lines

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 Joel

Quoted 37 lines

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()

Quoted 29 lines

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     |  10 +
 drivers/mmc/host/Makefile    |   1 +
 drivers/mmc/host/litex_mmc.c | 670 +++++++++++++++++++++++++++++++++++
 3 files changed, 681 insertions(+)
 create mode 100644 drivers/mmc/host/litex_mmc.c
drivers/mmc/host/Kconfig +10 -0
index 5af8494c31b5..a24561b74f18 100644
@@ -1093,3 +1093,13 @@ 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.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called litex_mmc.
drivers/mmc/host/Makefile +1 -0
index ea36d379bd3c..4e4ceb32c4b4 100644
@@ -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 +670 -0

670 lines not on this page · raw message

Signature · 1 line
2.31.1
#4 lore raw

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

Hi Andy,

A couple of remaining questions regarding your review inline, below:

Quoted 139 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!

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]>

---

New in v7:
drivers/mmc/host/Kconfig:
- added module name in LiteSDCard Kconfig entry
drivers/mmc/host/litex_mmc.c:
- fixed comment formatting, ordering, and capitalization throughout the entire file
- sorted header #include statements
- removed redundant parantheses in readx_poll_timeout() condition
- explicit handling of readx_poll_timeout() timeout scenarios
- dev_err() used in litex_mmc_sdcard_wait_done()
- use memcpy_fromio() to grab command response
- no need to apply 0xffff mask to a 32-bit value right-shifted by 16

    (host->resp[3])

- use clamp() instead of min(max(...)...)
- reworked platform_get_irq_optional() error handling logic
- no need to explicitly zero host->irq, kzalloc() does that already
- added missing free_irq() in litex_mmc_probe() error path
- reordered calls inside litex_mmc_remove() (calling mmc_free_host() before free_irq()

Quoted 51 lines

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 Joel

Quoted 37 lines

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()

Quoted 29 lines

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     |  10 +
 drivers/mmc/host/Makefile    |   1 +
 drivers/mmc/host/litex_mmc.c | 670 +++++++++++++++++++++++++++++++++++
 3 files changed, 681 insertions(+)
 create mode 100644 drivers/mmc/host/litex_mmc.c
drivers/mmc/host/Kconfig +10 -0
index 5af8494c31b5..a24561b74f18 100644
@@ -1093,3 +1093,13 @@ 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.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called litex_mmc.
drivers/mmc/host/Makefile +1 -0
index ea36d379bd3c..4e4ceb32c4b4 100644
@@ -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 +1 -0
new file mode 100644
index 000000000000..3719115850d9
@@ -0,0 +1,670 @@
+// SPDX-License-Identifier: GPL-2.0

drivers/mmc/host/litex_mmc.c line 11

Quoted 10 lines
drivers/mmc/host/litex_mmc.c new file +10 -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]>
+ *
+ */

It looked to me like you thought I should #include <linux/bits.h> here (even though I'm not getting any compiler warnings regarding it). If so, why? If not, apologies for the misunderstanding :)

Quoted 611 lines
drivers/mmc/host/litex_mmc.c new file +611 -0
+#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/of.h>
+#include <linux/platform_device.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_DATEVT     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, struct device *dev)
+{
+	u8 evt;
+	int ret;
+
+	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)
+		return -ETIMEDOUT;
+	if (evt & SD_BIT_CRC_ERR)
+		return -EILSEQ;
+	dev_err(dev, "%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, dev);
+	if (ret) {
+		dev_err(dev, "Command (cmd %d) error, status %d\n", cmd, ret);
+		return ret;
+	}
+
+	if (response_len != SD_CTL_RESP_NONE) {
+		/*
+		 * NOTE: this matches the semantics of litex_read32()
+		 * regardless of underlying arch endianness!
+		 */
+		memcpy_fromio(host->resp,
+			      host->sdcore + LITEX_CORE_CMDRSP, 0x10);
+	}
+
+	if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR)
+		host->rca = (host->resp[3] >> 16);
+
+	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_DATEVT, dev);
+	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 = clamp(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 < 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;
+	}
+
+	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;
+	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;
+
+	/*
+	 * 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;`
+	 */
+	mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);
+	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)

drivers/mmc/host/litex_mmc.c line 633

Quoted 10 lines
drivers/mmc/host/litex_mmc.c new file +10 -0
+		goto err;
+
+	return 0;
+
+err:
+	if (host->irq > 0)
+		free_irq(host->irq, mmc);
+	mmc_free_host(mmc);
+	return ret;
+}

Any more ordering or devm vs. non-devm mixing violations here? If so, can you please link me to an example or some docs where I ould figure out what it is I'm still doing wrong?

drivers/mmc/host/litex_mmc.c line 646

Quoted 12 lines
drivers/mmc/host/litex_mmc.c new file +12 -0
+static int litex_mmc_remove(struct platform_device *pdev)
+{
+	struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
+	struct mmc_host *mmc = host->mmc;
+
+	mmc_remove_host(mmc);
+	if (host->irq > 0)
+		free_irq(host->irq, mmc);
+	mmc_free_host(mmc);
+
+	return 0;
+}

Ditto here...

drivers/mmc/host/litex_mmc.c line 659

Quoted 12 lines
drivers/mmc/host/litex_mmc.c new file +12 -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),

You said "Wrong usage of of_match_ptr()" here, and all I have to go by is a bunch of other drivers/mmc/host/*.c files that use it in a similar way, so can you please clarify and/or provide an example of how to do it properly?

Thanks much,
--Gabriel

Quoted 14 lines
drivers/mmc/host/litex_mmc.c new file +11 -0
+	},
+};
+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");
Signature · 1 line
2.31.1
#5 lore raw

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

Quoted 1 line · Gabriel L. Somlo

On Fri, Jan 07, 2022 at 12:06:16PM -0500, Gabriel Somlo wrote:

...

Quoted 7 lines
Quoted 7 lines
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]>

It would be nice if you can use git send-email --cc ... instead of putting a long list into a commit message.

...

Quoted 3 lines

It looked to me like you thought I should #include <linux/bits.h> here (even though I'm not getting any compiler warnings regarding it). If so, why? If not, apologies for the misunderstanding :)

The rule of thumb is to explicitly use the headers you are the direct user of with the remark that some of them are guaranteed to be included by others and some of them should be used (in most cases) instead of their low-level parts (the example is types.h vs compiler_attributes.h, so former is more standard than the letter and it's almost 100% guarantee you want to have something from types.h anyway in your code).

So, BIT() is defined in bits.h and in the below list none of the header _guarantees_ its indirect inclusion.

Quoted 12 lines
Quoted 12 lines
drivers/mmc/host/litex_mmc.c new file +12 -0
+#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/of.h>
+#include <linux/platform_device.h>

...

Quoted 3 lines

Any more ordering or devm vs. non-devm mixing violations here? If so, can you please link me to an example or some docs where I ould figure out what it is I'm still doing wrong?

Device managed resources are attached to the instance of the device object and removed in the order they have been attached to, but with the caveat that they have no clue about non-managed calls in between. Now you may figure out what happens. Ex.:

probe()
A
devm_B
C
devm_D

remove()
un_C
un_A

WRONG!

Quoted 6 lines
Quoted 6 lines
drivers/mmc/host/litex_mmc.c new file +6 -0
+static int litex_mmc_remove(struct platform_device *pdev)
+{
+     struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
+     struct mmc_host *mmc = host->mmc;
+
+     mmc_remove_host(mmc);
Quoted 8 lines

drivers/mmc/host/litex_mmc.c line 646

Quoted 6 lines
drivers/mmc/host/litex_mmc.c new file +6 -0
+     if (host->irq > 0)
+             free_irq(host->irq, mmc);
+     mmc_free_host(mmc);
+
+     return 0;
+}

Ditto here...

Ditto.

...

Quoted 6 lines
Quoted 1 line

+ .of_match_table = of_match_ptr(litex_match),

You said "Wrong usage of of_match_ptr()" here, and all I have to go by is a bunch of other drivers/mmc/host/*.c files that use it in a similar way, so can you please clarify and/or provide an example of how to do it properly?

First of all, you have a dependency to OF, try to remove it and compile with OF=n and you will immediately see the issue. You may also go for git log --no-merges --grep of_match_ptr and analyze the result.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#6 lore raw

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

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Gabriel L. Somlo

On Fri, Jan 07, 2022 at 12:06:16PM -0500, Gabriel Somlo wrote:

...

Quoted 15 lines
Quoted 3 lines

Any more ordering or devm vs. non-devm mixing violations here? If so, can you please link me to an example or some docs where I ould figure out what it is I'm still doing wrong?

Device managed resources are attached to the instance of the device object and removed in the order they have been attached to, but with the caveat that they have no clue about non-managed calls in between. Now you may figure out what happens. Ex.:

probe()
A
devm_B
C
devm_D

Quoted 5 lines

remove()
un_C
un_A

WRONG!

For the sake of comprehensivity of the examples the right one(s) depicted below:

->probe()

1)
devm_A
devm_B
C
D

2)
A
B
C
D

3)
devm_A
devm_B
devm_C
devm_D

Hint:
git log --no-merges --grep devm_add_action_or_reset

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#7 lore raw

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

Quoted 44 lines · Andy Shevchenko
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Gabriel L. Somlo

On Fri, Jan 07, 2022 at 12:06:16PM -0500, Gabriel Somlo wrote:

...

Quoted 15 lines
Quoted 3 lines

Any more ordering or devm vs. non-devm mixing violations here? If so, can you please link me to an example or some docs where I ould figure out what it is I'm still doing wrong?

Device managed resources are attached to the instance of the device object and removed in the order they have been attached to, but with the caveat that they have no clue about non-managed calls in between. Now you may figure out what happens. Ex.:

probe()
A
devm_B
C
devm_D

Quoted 5 lines

remove()
un_C
un_A

WRONG!

For the sake of comprehensivity of the examples the right one(s) depicted below:

->probe()

1)
devm_A
devm_B
C
D

2)
A
B
C
D

Quoted 8 lines

3)
devm_A
devm_B
devm_C
devm_D

Hint:
git log --no-merges --grep devm_add_action_or_reset

Thanks again!

As far as I can tell, I *meant* (but failed to) use devm_request_irq(), which would then have justified the absence of free_irq() on the probe() function's error path!

Similarly, I would no longer have to call it during remove() either:

drivers/mmc/host/litex_mmc.c +3 -7
index d96da0bcba55..38952f169a27 100644
@@ -486,8 +486,8 @@ static int litex_mmc_irq_init(struct litex_mmc_host *host)
        if (IS_ERR(host->sdirq))
                return PTR_ERR(host->sdirq);
 
-       ret = request_irq(host->irq, litex_mmc_interrupt, 0,
-                         "litex-mmc", host->mmc);
+       ret = devm_request_irq(dev, 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;
@@ -626,20 +626,16 @@ static int litex_mmc_probe(struct platform_device *pdev)
        return 0;
 
 err:
-       if (host->irq > 0)
-               free_irq(host->irq, 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);
+       struct litex_mmc_host *host = platform_get_drvdata(pdev);
        struct mmc_host *mmc = host->mmc;
 
        mmc_remove_host(mmc);
-       if (host->irq > 0)
-               free_irq(host->irq, mmc);
        mmc_free_host(mmc);
 
        return 0;

I'll send out v8 shortly, hopeuflly with all ordering and devm-ness issues fixed.

Thanks,
--Gabriel

#8 lore raw

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

In reply to Andy Shevchenko
Quoted 11 lines · Andy Shevchenko
Quoted 6 lines
Quoted 1 line

+ .of_match_table = of_match_ptr(litex_match),

You said "Wrong usage of of_match_ptr()" here, and all I have to go by is a bunch of other drivers/mmc/host/*.c files that use it in a similar way, so can you please clarify and/or provide an example of how to do it properly?

First of all, you have a dependency to OF, try to remove it and compile with OF=n and you will immediately see the issue. You may also go for git log --no-merges --grep of_match_ptr and analyze the result.

Ah, grepping for "of_match_ptr" was *never* going to show me the "right way of using of_match_ptr()" :)

OTOH, grepping for "of_match_table" (and of course the commits returned by your command line) suggests that I simply should *not* use it at all, and assign its argument directly to .of_match_table.

Got it, fixed it, thanks!

Now, on to the probe ordering issue... :)

Thanks,
--G

A message that is not in the archive (sent off-list or not yet received): CAHp75VfMJyeF4dpz63TqZv9UCMBWmQu0U622SOV7wLmn7Qm0mQ@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): CAHp75Vfo=uJ3eY0GtjdjTk32KkaQfOFYw6kF5oZqQLb38AnhbA@mail.gmail.com

#11 lore raw

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

Quoted 35 lines · Andy Shevchenko

On Saturday, January 8, 2022, Andy Shevchenko <[email protected]> wrote:

On Friday, January 7, 2022, Gabriel L. Somlo <[email protected]> wrote:

On Fri, Jan 07, 2022 at 10:50:02PM +0200, Andy Shevchenko wrote:
> > > +             .of_match_table = of_match_ptr(litex_match), > >
> > You said "Wrong usage of of_match_ptr()" here, and all I have to go by
> > is a bunch of other drivers/mmc/host/*.c files that use it in a > > similar way, so can you please clarify and/or provide an example of how
> > to do it properly?
>
> First of all, you have a dependency to OF, try to remove it and > compile with OF=n and you will immediately see the issue. You may also
> go for  git log --no-merges --grep of_match_ptr and analyze the > result.

Ah, grepping for "of_match_ptr" was *never* going to show me the "right way of using of_match_ptr()" :)

Hmm... this is strange. The above command is grepping in the commit messages, and not in the code.

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?qt=
grep&q=of_match_ptr

Shows a lot of the commits that I referred to.

Right. I was originally looking at *code* for current usage examples.

I get that you were referring to grepping through *commits*, which seems to be the new way of figuring out what's canonical (as a lot of the existing code seems to be in need of update :)

Thanks,
--Gabriel

Quoted 28 lines

OTOH, grepping for "of_match_table" (and of course the commits returned by your command line) suggests that I simply should *not* use it at all, and assign its argument directly to .of_match_table.

Got it, fixed it, thanks!

Now, on to the probe ordering issue... :)

Thanks,
--G

--
With Best Regards,
Andy Shevchenko

--
With Best Regards,
Andy Shevchenko

#12 lore raw

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

In reply to Gabriel Somlo

Hi Ulf,

Couple of follow-up questions inline, below:

Quoted 205 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!

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]>

---

New in v7:
drivers/mmc/host/Kconfig:
- added module name in LiteSDCard Kconfig entry
drivers/mmc/host/litex_mmc.c:
- fixed comment formatting, ordering, and capitalization throughout the entire file
- sorted header #include statements
- removed redundant parantheses in readx_poll_timeout() condition
- explicit handling of readx_poll_timeout() timeout scenarios
- dev_err() used in litex_mmc_sdcard_wait_done()
- use memcpy_fromio() to grab command response
- no need to apply 0xffff mask to a 32-bit value right-shifted by 16

    (host->resp[3])

- use clamp() instead of min(max(...)...)
- reworked platform_get_irq_optional() error handling logic
- no need to explicitly zero host->irq, kzalloc() does that already
- added missing free_irq() in litex_mmc_probe() error path
- reordered calls inside litex_mmc_remove() (calling mmc_free_host() before free_irq()

Quoted 51 lines

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 Joel

Quoted 37 lines

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()

Quoted 29 lines

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     |  10 +
 drivers/mmc/host/Makefile    |   1 +
 drivers/mmc/host/litex_mmc.c | 670 +++++++++++++++++++++++++++++++++++
 3 files changed, 681 insertions(+)
 create mode 100644 drivers/mmc/host/litex_mmc.c
drivers/mmc/host/Kconfig +10 -0
index 5af8494c31b5..a24561b74f18 100644
@@ -1093,3 +1093,13 @@ 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.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called litex_mmc.
drivers/mmc/host/Makefile +1 -0
index ea36d379bd3c..4e4ceb32c4b4 100644
@@ -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 +67 -0
new file mode 100644
index 000000000000..3719115850d9
@@ -0,0 +1,670 @@
+// 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/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/of.h>
+#include <linux/platform_device.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_DATEVT     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

drivers/mmc/host/litex_mmc.c line 77

Quoted 10 lines
drivers/mmc/host/litex_mmc.c new file +10 -0
+#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)

I'd like to hear back from you on what you think of this, and, more importantly, the place it's referenced further below...

Quoted 505 lines
drivers/mmc/host/litex_mmc.c new file +505 -0
+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, struct device *dev)
+{
+	u8 evt;
+	int ret;
+
+	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)
+		return -ETIMEDOUT;
+	if (evt & SD_BIT_CRC_ERR)
+		return -EILSEQ;
+	dev_err(dev, "%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, dev);
+	if (ret) {
+		dev_err(dev, "Command (cmd %d) error, status %d\n", cmd, ret);
+		return ret;
+	}
+
+	if (response_len != SD_CTL_RESP_NONE) {
+		/*
+		 * NOTE: this matches the semantics of litex_read32()
+		 * regardless of underlying arch endianness!
+		 */
+		memcpy_fromio(host->resp,
+			      host->sdcore + LITEX_CORE_CMDRSP, 0x10);
+	}
+
+	if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR)
+		host->rca = (host->resp[3] >> 16);
+
+	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_DATEVT, dev);
+	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 = clamp(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 < 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;
+	}
+
+	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;
+	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;
+
+	/*
+	 * 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;`
+	 */
+	mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);
+	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 */

drivers/mmc/host/litex_mmc.c line 593

Quoted 10 lines
drivers/mmc/host/litex_mmc.c new file +10 -0
+	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;

...here. Per the "Part 1 Simplified"/"Physical Layer Simplified Spec." at https://www.sdcard.org/downloads/pls/ (page 85 of the pdf document), I'm hard-coding the 2.7-3.6V range as valid, and the hardware offers no interface for tuning or configuring voltage via software in any way whatsoever.

IMHO it's cleaner to hardcode something like that in the driver rather than pretend it's configurable via DTS only to hard-code it *there* using a (dummy) voltage regulator.

But I'm certainly willing to be educated if you think I'm wrong, so please let me know.

Thanks much,
--Gabriel

Quoted 79 lines
drivers/mmc/host/litex_mmc.c new file +76 -0
+	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:
+	if (host->irq > 0)
+		free_irq(host->irq, 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);
+	struct mmc_host *mmc = host->mmc;
+
+	mmc_remove_host(mmc);
+	if (host->irq > 0)
+		free_irq(host->irq, mmc);
+	mmc_free_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");
Signature · 1 line
2.31.1
#13 lore raw

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

In reply to Gabriel Somlo
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!

...

drivers/mmc/host/litex_mmc.c line 10

Quoted 11 lines
drivers/mmc/host/litex_mmc.c new file +10 -0
+// 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]>
 
+ *

I guess I have commented on this and there was no discussion about the necessity of this blank line. I dunno what else has been ignored, so I will wait either for the continuation of the discussion (as per v6) or amending code in v8.

Quoted 1 line

+ */

...

On top of that it might still be the ordering issues in the error path of ->probe() and in ->remove(). I believe we will likely see v8.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#14 lore raw

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

Quoted 21 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 9 lines
drivers/mmc/host/litex_mmc.c new file +9 -0
+// 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]>
Quoted 6 lines
Quoted 1 line

+ *

I guess I have commented on this and there was no discussion about the necessity of this blank line. I dunno what else has been ignored, so I will wait either for the continuation of the discussion (as per v6) or amending code in v8.

Yeah, not sure how that got lost, it "should have" been in v7 already, but will definitely be in v8... :)

BTW, I also added the bit.h include per your explanation, and removed all the Cc: lines from the commit blurb (not sure when it's actually appropriate to put those in vs. just cc-ing people from git send-email, but I have no strong feelings about it of my own :)

Quoted 6 lines
Quoted 1 line

+ */

...

On top of that it might still be the ordering issues in the error path of ->probe() and in ->remove(). I believe we will likely see v8.

I'll need to follow your bread crumbs on ordering and of_match_table (also in some of the other emails you've sent in this thread), and will send out v8 once I've made some sense of it all...

Thanks much,
--Gabriel