[PATCH v1 0/3] mmc: Add LiteSDCard mmc driver

[PATCH v1 0/3] mmc: Add LiteSDCard mmc driver

[Gabriel Somlo]

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.

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    |  63 ++
 MAINTAINERS                                   |   2 +
 drivers/mmc/host/Kconfig                      |   6 +
 drivers/mmc/host/Makefile                     |   1 +
 drivers/mmc/host/litex_mmc.c                  | 677 ++++++++++++++++++
 5 files changed, 749 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/litex,mmc.yaml
 create mode 100644 drivers/mmc/host/litex_mmc.c

-- 
2.31.1

[PATCH v1 1/3] MAINTAINERS: co-maintain LiteX platform

[Gabriel Somlo]

Add the litex_mmc (LiteSDCard) driver to the list of files maintained
under LiteX, and add myself as co-maintainer. I've helped develop some
of the existing drivers, and am currently curating the out-of-tree
drivers as they are tested and prepared for upstream submission.

Cc: Karol Gugala <kgugala@antmicro.com>
Cc: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index faa9c34d837d..5fc65d4c4969 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11012,12 +11012,14 @@ F:	lib/list-test.c
 LITEX PLATFORM
 M:	Karol Gugala <kgugala@antmicro.com>
 M:	Mateusz Holenko <mholenko@antmicro.com>
+M:	Gabriel Somlo <gsomlo@gmail.com>
 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/mmc/host/litex_mmc.c
 
 LIVE PATCHING
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
-- 
2.31.1

[PATCH v1 2/3] dt-bindings: mmc: Add bindings for LiteSDCard

[Gabriel Somlo]

LiteSDCard is a small footprint, configurable SDCard core for FPGA
based system on chips.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 .../devicetree/bindings/mmc/litex,mmc.yaml    | 63 +++++++++++++++++++
 1 file changed, 63 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..edc5ab7f359b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/litex,mmc.yaml
@@ -0,0 +1,63 @@
+# 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 <gsomlo@gmail.com>
+
+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
+
+  reg-names:
+    items:
+      - const: phy
+      - const: core
+      - const: reader
+      - const: writer
+      - const: irq
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+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";
+        interrupts = <4>;
+    };
-- 
2.31.1

Re: [PATCH v1 2/3] dt-bindings: mmc: Add bindings for LiteSDCard

[Geert Uytterhoeven]

Hi Gabriel,

On Sat, Dec 4, 2021 at 12:42 AM Gabriel Somlo <gsomlo@gmail.com> wrote:
> LiteSDCard is a small footprint, configurable SDCard core for FPGA
> based system on chips.
>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/litex,mmc.yaml
> @@ -0,0 +1,63 @@
> +# 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 <gsomlo@gmail.com>
> +
> +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
> +
> +  reg-names:
> +    items:
> +      - const: phy
> +      - const: core
> +      - const: reader
> +      - const: writer
> +      - const: irq
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg

reg-names?

(and updating litex/tools/litex_json2dts_linux.py to add it)

> +  - interrupts
> +
> +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";
> +        interrupts = <4>;
> +    };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 2/3] dt-bindings: mmc: Add bindings for LiteSDCard

[Gabriel L. Somlo]

On Mon, Dec 06, 2021 at 10:38:07AM +0100, Geert Uytterhoeven wrote:
> Hi Gabriel,
> 
> On Sat, Dec 4, 2021 at 12:42 AM Gabriel Somlo <gsomlo@gmail.com> wrote:
> > LiteSDCard is a small footprint, configurable SDCard core for FPGA
> > based system on chips.
> >
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/litex,mmc.yaml
> > @@ -0,0 +1,63 @@
> > +# 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 <gsomlo@gmail.com>
> > +
> > +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
> > +
> > +  reg-names:
> > +    items:
> > +      - const: phy
> > +      - const: core
> > +      - const: reader
> > +      - const: writer
> > +      - const: irq
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> 
> reg-names?
> 
> (and updating litex/tools/litex_json2dts_linux.py to add it)

Thanks, I missed `reg-names` originally, and have added it to the
list. And yes, once we agree on all this, I'll submit a matching
update to litex_json2dts_linux.py 

Thanks,
--Gabriel
 
> > +  - interrupts
> > +
> > +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";
> > +        interrupts = <4>;
> > +    };
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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

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

[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!

Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
Cc: Mateusz Holenko <mholenko@antmicro.com>
Cc: Karol Gugala <kgugala@antmicro.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Stafford Horne <shorne@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: David Abdurachmanov <david.abdurachmanov@sifive.com>
Cc: Florent Kermarrec <florent@enjoy-digital.fr>
---
 drivers/mmc/host/Kconfig     |   6 +
 drivers/mmc/host/Makefile    |   1 +
 drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
 3 files changed, 684 insertions(+)
 create mode 100644 drivers/mmc/host/litex_mmc.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5af8494c31b5..84c64e72195d 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -1093,3 +1093,9 @@ config MMC_OWL
 
 config MMC_SDHCI_EXTERNAL_DMA
 	bool
+
+config MMC_LITEX
+	tristate "Support for the MMC Controller in LiteX SOCs"
+	depends on OF && LITEX
+	help
+	  Generic MCC driver for LiteX
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..3877379757cd
--- /dev/null
+++ b/drivers/mmc/host/litex_mmc.c
@@ -0,0 +1,677 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LiteX LiteSDCard driver
+ *
+ * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/litex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/slot-gpio.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 SDCARD_CTRL_DATA_TRANSFER_NONE  0
+#define SDCARD_CTRL_DATA_TRANSFER_READ  1
+#define SDCARD_CTRL_DATA_TRANSFER_WRITE 2
+
+#define SDCARD_CTRL_RESPONSE_NONE       0
+#define SDCARD_CTRL_RESPONSE_SHORT      1
+#define SDCARD_CTRL_RESPONSE_LONG       2
+#define SDCARD_CTRL_RESPONSE_SHORT_BUSY 3
+
+#define SD_OK         0
+#define SD_WRITEERROR 1
+#define SD_TIMEOUT    2
+#define SD_CRCERROR   3
+#define SD_ERR_OTHER  4
+
+#define SDIRQ_CARD_DETECT    1
+#define SDIRQ_SD_TO_MEM_DONE 2
+#define SDIRQ_MEM_TO_SD_DONE 4
+#define SDIRQ_CMD_DONE       8
+
+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;
+
+	u32 resp[4];
+	u16 rca;
+
+	void *buffer;
+	size_t buf_size;
+	dma_addr_t dma;
+
+	unsigned int freq;
+	unsigned int clock;
+	bool is_bus_width_set;
+	bool app_cmd;
+
+	int irq;
+	struct completion cmd_done;
+};
+
+static int
+sdcard_wait_done(void __iomem *reg)
+{
+	u8 evt;
+
+	for (;;) {
+		evt = litex_read8(reg);
+		if (evt & 0x1)
+			break;
+		udelay(5);
+	}
+	if (evt == 0x1)
+		return SD_OK;
+	if (evt & 0x2)
+		return SD_WRITEERROR;
+	if (evt & 0x4)
+		return SD_TIMEOUT;
+	if (evt & 0x8)
+		return SD_CRCERROR;
+	pr_err("%s: unknown error evt=%x\n", __func__, evt);
+	return SD_ERR_OTHER;
+}
+
+static int
+send_cmd(struct litex_mmc_host *host,
+	 u8 cmd, u32 arg, u8 response_len, u8 transfer)
+{
+	void __iomem *reg;
+	ulong n;
+	u8 i;
+	int status;
+
+	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 != SDCARD_CTRL_DATA_TRANSFER_NONE ||
+	     response_len == SDCARD_CTRL_RESPONSE_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);
+	}
+
+	status = sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT);
+
+	if (status != SD_OK) {
+		pr_err("Command (cmd %d) failed, status %d\n", cmd, status);
+		return status;
+	}
+
+	if (response_len != SDCARD_CTRL_RESPONSE_NONE) {
+		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 == SDCARD_CTRL_DATA_TRANSFER_NONE)
+		return status; /* SD_OK from prior sdcard_wait_done(cmd_evt) */
+
+	status = sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT);
+	if (status != SD_OK) {
+		pr_err("Data xfer (cmd %d) failed, status %d\n", cmd, status);
+		return status;
+	}
+
+	/* wait for completion of (read or write) DMA transfer */
+	reg = (transfer == SDCARD_CTRL_DATA_TRANSFER_READ) ?
+		host->sdreader + LITEX_BLK2MEM_DONE :
+		host->sdwriter + LITEX_MEM2BLK_DONE;
+	n = jiffies + (HZ << 1);
+	while ((litex_read8(reg) & 0x01) == 0)
+		if (time_after(jiffies, n)) {
+			pr_err("DMA timeout (cmd %d)\n", cmd);
+			return SD_TIMEOUT;
+		}
+
+	return status;
+}
+
+static inline int
+send_app_cmd(struct litex_mmc_host *host)
+{
+	return send_cmd(host, MMC_APP_CMD, host->rca << 16,
+			SDCARD_CTRL_RESPONSE_SHORT,
+			SDCARD_CTRL_DATA_TRANSFER_NONE);
+}
+
+static inline int
+send_app_set_bus_width_cmd(struct litex_mmc_host *host, u32 width)
+{
+	return send_cmd(host, SD_APP_SET_BUS_WIDTH, width,
+			SDCARD_CTRL_RESPONSE_SHORT,
+			SDCARD_CTRL_DATA_TRANSFER_NONE);
+}
+
+static int
+litex_set_bus_width(struct litex_mmc_host *host)
+{
+	bool app_cmd_sent = host->app_cmd; /* was preceding command app_cmd? */
+	int status;
+
+	/* ensure 'app_cmd' precedes 'app_set_bus_width_cmd' */
+	if (!app_cmd_sent)
+		send_app_cmd(host);
+
+	/* litesdcard only supports 4-bit bus width */
+	status = send_app_set_bus_width_cmd(host, MMC_BUS_WIDTH_4);
+
+	/* re-send 'app_cmd' if necessary */
+	if (app_cmd_sent)
+		send_app_cmd(host);
+
+	return status;
+}
+
+static int
+litex_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 = mmc_gpio_get_cd(mmc);
+	if (ret >= 0)
+		/* GPIO based card-detect explicitly specified in DTS */
+		ret = !!ret;
+	else
+		/* use gateware card-detect bit by default */
+		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_response_len(struct mmc_command *cmd)
+{
+	if (cmd->flags & MMC_RSP_136) {
+		return SDCARD_CTRL_RESPONSE_LONG;
+	} else if (cmd->flags & MMC_RSP_PRESENT) {
+		if (cmd->flags & MMC_RSP_BUSY)
+			return SDCARD_CTRL_RESPONSE_SHORT_BUSY;
+		else
+			return SDCARD_CTRL_RESPONSE_SHORT;
+	}
+	return SDCARD_CTRL_RESPONSE_NONE;
+}
+
+static int
+litex_map_status(int status)
+{
+	int error;
+
+	switch (status) {
+	case SD_OK:
+		error = 0;
+		break;
+	case SD_WRITEERROR:
+		error = -EIO;
+		break;
+	case SD_TIMEOUT:
+		error = -ETIMEDOUT;
+		break;
+	case SD_CRCERROR:
+		error = -EILSEQ;
+		break;
+	default:
+		error = -EINVAL;
+		break;
+	}
+	return error;
+}
+
+static void
+litex_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	struct litex_mmc_host *host = mmc_priv(mmc);
+	struct platform_device *pdev = to_platform_device(mmc->parent);
+	struct device *dev = &pdev->dev;
+	struct mmc_data *data = mrq->data;
+	struct mmc_command *sbc = mrq->sbc;
+	struct mmc_command *cmd = mrq->cmd;
+	struct mmc_command *stop = mrq->stop;
+	unsigned int retries = cmd->retries;
+	int status;
+	int sg_count;
+	enum dma_data_direction dir = DMA_TO_DEVICE;
+	bool direct = false;
+	dma_addr_t dma;
+	unsigned int len = 0;
+
+	u32 response_len = litex_response_len(cmd);
+	u32 transfer = SDCARD_CTRL_DATA_TRANSFER_NONE;
+
+	/* First check that the card is still there */
+	if (!litex_get_cd(mmc)) {
+		cmd->error = -ENOMEDIUM;
+		mmc_request_done(mmc, mrq);
+		return;
+	}
+
+	/* Send set-block-count command if needed */
+	if (sbc) {
+		status = send_cmd(host, sbc->opcode, sbc->arg,
+				  litex_response_len(sbc),
+				  SDCARD_CTRL_DATA_TRANSFER_NONE);
+		sbc->error = litex_map_status(status);
+		if (status != SD_OK) {
+			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!
+		 */
+		if (!host->is_bus_width_set) {
+			ulong n = jiffies + 2 * HZ; // 500ms timeout
+
+			while (litex_set_bus_width(host) != SD_OK) {
+				if (time_after(jiffies, n)) {
+					dev_warn(dev, "Can't set bus width!\n");
+					cmd->error = -ETIMEDOUT;
+					mmc_request_done(mmc, mrq);
+					return;
+				}
+			}
+			host->is_bus_width_set = true;
+		}
+
+		/* 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;
+		if (data->flags & MMC_DATA_READ)
+			dir = DMA_FROM_DEVICE;
+		sg_count = dma_map_sg(&host->dev->dev,
+				      data->sg, data->sg_len, dir);
+		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 = SDCARD_CTRL_DATA_TRANSFER_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 = SDCARD_CTRL_DATA_TRANSFER_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);
+	}
+
+	do {
+		status = send_cmd(host, cmd->opcode, cmd->arg,
+				  response_len, transfer);
+	} while (status != SD_OK && retries-- > 0);
+
+	cmd->error = litex_map_status(status);
+	if (status != SD_OK)
+		/* card may be gone; don't assume bus width is still set */
+		host->is_bus_width_set = false;
+
+	if (response_len == SDCARD_CTRL_RESPONSE_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 == SDCARD_CTRL_RESPONSE_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)) {
+		int stop_stat;
+
+		stop_stat = send_cmd(host, stop->opcode, stop->arg,
+				     litex_response_len(stop),
+				     SDCARD_CTRL_DATA_TRANSFER_NONE);
+		stop->error = litex_map_status(stop_stat);
+		if (stop_stat != SD_OK)
+			host->is_bus_width_set = false;
+	}
+
+	if (data)
+		dma_unmap_sg(&host->dev->dev, data->sg, data->sg_len, dir);
+
+	if (status == SD_OK && transfer != SDCARD_CTRL_DATA_TRANSFER_NONE) {
+		data->bytes_xfered = min(len, mmc->max_req_size);
+		if (transfer == SDCARD_CTRL_DATA_TRANSFER_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_set_clk(struct litex_mmc_host *host, unsigned int clk_freq)
+{
+	u32 div = clk_freq ? host->freq / clk_freq : 256;
+
+	div = roundup_pow_of_two(div);
+	div = min_t(u32, max_t(u32, div, 2), 256);
+	dev_info(&host->dev->dev, "clk_freq=%d: set to %d via div=%d\n",
+		 clk_freq, host->freq / div, div);
+	litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div);
+}
+
+static void
+litex_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 sdcard clock */
+	if (ios->clock != host->clock) {
+		litex_set_clk(host, ios->clock);
+		host->clock = ios->clock;
+	}
+}
+
+static const struct mmc_host_ops litex_mmc_ops = {
+	.get_cd = litex_get_cd,
+	.request = litex_request,
+	.set_ios = litex_set_ios,
+};
+
+static int
+litex_mmc_probe(struct platform_device *pdev)
+{
+	struct litex_mmc_host *host;
+	struct mmc_host *mmc;
+	struct device_node *cpu;
+	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;
+
+	host->clock = 0;
+	cpu = of_get_next_cpu_node(NULL);
+	ret = of_property_read_u32(cpu, "clock-frequency", &host->freq);
+	of_node_put(cpu);
+	if (ret) {
+		dev_err(&pdev->dev, "No \"clock-frequency\" property in DT\n");
+		goto err_free_host;
+	}
+
+	init_completion(&host->cmd_done);
+	host->irq = platform_get_irq(pdev, 0);
+	if (host->irq < 0)
+		dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
+
+	/* 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;
+
+	ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret)
+		goto err_free_host;
+
+	host->buf_size = mmc->max_req_size * 2;
+	host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
+					  &host->dma, GFP_DMA);
+	if (host->buffer == NULL) {
+		ret = -ENOMEM;
+		goto err_free_host;
+	}
+
+	host->sdphy = devm_platform_ioremap_resource_byname(pdev, "phy");
+	if (IS_ERR(host->sdphy)) {
+		ret = PTR_ERR(host->sdphy);
+		goto err_free_dma;
+	}
+
+	host->sdcore = devm_platform_ioremap_resource_byname(pdev, "core");
+	if (IS_ERR(host->sdcore)) {
+		ret = PTR_ERR(host->sdcore);
+		goto err_free_dma;
+	}
+
+	host->sdreader = devm_platform_ioremap_resource_byname(pdev, "reader");
+	if (IS_ERR(host->sdreader)) {
+		ret = PTR_ERR(host->sdreader);
+		goto err_free_dma;
+	}
+
+	host->sdwriter = devm_platform_ioremap_resource_byname(pdev, "writer");
+	if (IS_ERR(host->sdwriter)) {
+		ret = PTR_ERR(host->sdwriter);
+		goto err_free_dma;
+	}
+
+	if (host->irq > 0) {
+		host->sdirq = devm_platform_ioremap_resource_byname(pdev, "irq");
+		if (IS_ERR(host->sdirq)) {
+			ret = PTR_ERR(host->sdirq);
+			goto err_free_dma;
+		}
+	}
+
+	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+	mmc->ops = &litex_mmc_ops;
+
+	mmc->f_min = 12.5e6;
+	mmc->f_max = 50e6;
+
+	ret = mmc_of_parse(mmc);
+	if (ret)
+		goto err_free_dma;
+
+	/* 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_FULL_PWR_CYCLE |
+		      MMC_CAP2_NO_SDIO;
+
+	platform_set_drvdata(pdev, host);
+
+	ret = mmc_add_host(mmc);
+	if (ret < 0)
+		goto err_free_dma;
+
+	/* ensure DMA bus masters are disabled */
+	litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
+	litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);
+
+	/* set up interrupt handler */
+	if (host->irq > 0) {
+		ret = request_irq(host->irq, litex_mmc_interrupt, 0,
+				  "litex-mmc", mmc);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"irq setup error %d, using polling\n", ret);
+			host->irq = 0;
+		}
+	}
+
+	/* enable card-change interrupts, or else ask for polling */
+	if (host->irq > 0) {
+		litex_write32(host->sdirq + LITEX_IRQ_PENDING,
+			      SDIRQ_CARD_DETECT);	/* clears it */
+		litex_write32(host->sdirq + LITEX_IRQ_ENABLE,
+			      SDIRQ_CARD_DETECT);
+	} else {
+		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	}
+
+	return 0;
+
+err_free_dma:
+	dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
+err_free_host:
+	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);
+	dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
+	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 <www.antmicro.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1

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

[Randy Dunlap]

Hi--

On 12/3/21 15:41, Gabriel Somlo wrote:
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5af8494c31b5..84c64e72195d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1093,3 +1093,9 @@ config MMC_OWL
>  
>  config MMC_SDHCI_EXTERNAL_DMA
>  	bool
> +
> +config MMC_LITEX
> +	tristate "Support for the MMC Controller in LiteX SOCs"
> +	depends on OF && LITEX
> +	help
> +	  Generic MCC driver for LiteX

	          MMC            LiteX.


-- 
~Randy

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

[Gabriel L. Somlo]

On Fri, Dec 03, 2021 at 04:20:59PM -0800, Randy Dunlap wrote:
> Hi--
> 
> On 12/3/21 15:41, Gabriel Somlo wrote:
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 5af8494c31b5..84c64e72195d 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -1093,3 +1093,9 @@ config MMC_OWL
> >  
> >  config MMC_SDHCI_EXTERNAL_DMA
> >  	bool
> > +
> > +config MMC_LITEX
> > +	tristate "Support for the MMC Controller in LiteX SOCs"
> > +	depends on OF && LITEX
> > +	help
> > +	  Generic MCC driver for LiteX
> 
> 	          MMC            LiteX.

Heh... tackled at the one... :) I'll have that fixed in v2 -- thanks!
--Gabriel

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

[kernel test robot]

Hi Gabriel,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linux/master ulf-hansson-mmc-mirror/next linus/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Gabriel-Somlo/mmc-Add-LiteSDCard-mmc-driver/20211204-074318
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20211204/202112041255.X6DL0sKx-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f967027b6ffe6f577773d3607edcf6677f7e56c5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Gabriel-Somlo/mmc-Add-LiteSDCard-mmc-driver/20211204-074318
        git checkout f967027b6ffe6f577773d3607edcf6677f7e56c5
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash drivers/mmc/host/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/mmc/host/litex_mmc.c: In function 'litex_mmc_probe':
>> drivers/mmc/host/litex_mmc.c:617:23: error: implicit declaration of function 'request_irq'; did you mean 'request_key'? [-Werror=implicit-function-declaration]
     617 |                 ret = request_irq(host->irq, litex_mmc_interrupt, 0,
         |                       ^~~~~~~~~~~
         |                       request_key
   drivers/mmc/host/litex_mmc.c: In function 'litex_mmc_remove':
>> drivers/mmc/host/litex_mmc.c:651:17: error: implicit declaration of function 'free_irq' [-Werror=implicit-function-declaration]
     651 |                 free_irq(host->irq, host->mmc);
         |                 ^~~~~~~~
   cc1: some warnings being treated as errors


vim +617 drivers/mmc/host/litex_mmc.c

   496	
   497	static int
   498	litex_mmc_probe(struct platform_device *pdev)
   499	{
   500		struct litex_mmc_host *host;
   501		struct mmc_host *mmc;
   502		struct device_node *cpu;
   503		int ret;
   504	
   505		mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);
   506		/* NOTE: defaults to max_[req,seg]_size=PAGE_SIZE, max_blk_size=512,
   507		 * and max_blk_count accordingly set to 8;
   508		 * If for some reason we need to modify max_blk_count, we must also
   509		 * re-calculate `max_[req,seg]_size = max_blk_size * max_blk_count;`
   510		 */
   511		if (!mmc)
   512			return -ENOMEM;
   513	
   514		host = mmc_priv(mmc);
   515		host->mmc = mmc;
   516		host->dev = pdev;
   517	
   518		host->clock = 0;
   519		cpu = of_get_next_cpu_node(NULL);
   520		ret = of_property_read_u32(cpu, "clock-frequency", &host->freq);
   521		of_node_put(cpu);
   522		if (ret) {
   523			dev_err(&pdev->dev, "No \"clock-frequency\" property in DT\n");
   524			goto err_free_host;
   525		}
   526	
   527		init_completion(&host->cmd_done);
   528		host->irq = platform_get_irq(pdev, 0);
   529		if (host->irq < 0)
   530			dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
   531	
   532		/* LiteSDCard only supports 4-bit bus width; therefore, we MUST inject
   533		 * a SET_BUS_WIDTH (acmd6) before the very first data transfer, earlier
   534		 * than when the mmc subsystem would normally get around to it!
   535		 */
   536		host->is_bus_width_set = false;
   537		host->app_cmd = false;
   538	
   539		ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
   540		if (ret)
   541			goto err_free_host;
   542	
   543		host->buf_size = mmc->max_req_size * 2;
   544		host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
   545						  &host->dma, GFP_DMA);
   546		if (host->buffer == NULL) {
   547			ret = -ENOMEM;
   548			goto err_free_host;
   549		}
   550	
   551		host->sdphy = devm_platform_ioremap_resource_byname(pdev, "phy");
   552		if (IS_ERR(host->sdphy)) {
   553			ret = PTR_ERR(host->sdphy);
   554			goto err_free_dma;
   555		}
   556	
   557		host->sdcore = devm_platform_ioremap_resource_byname(pdev, "core");
   558		if (IS_ERR(host->sdcore)) {
   559			ret = PTR_ERR(host->sdcore);
   560			goto err_free_dma;
   561		}
   562	
   563		host->sdreader = devm_platform_ioremap_resource_byname(pdev, "reader");
   564		if (IS_ERR(host->sdreader)) {
   565			ret = PTR_ERR(host->sdreader);
   566			goto err_free_dma;
   567		}
   568	
   569		host->sdwriter = devm_platform_ioremap_resource_byname(pdev, "writer");
   570		if (IS_ERR(host->sdwriter)) {
   571			ret = PTR_ERR(host->sdwriter);
   572			goto err_free_dma;
   573		}
   574	
   575		if (host->irq > 0) {
   576			host->sdirq = devm_platform_ioremap_resource_byname(pdev, "irq");
   577			if (IS_ERR(host->sdirq)) {
   578				ret = PTR_ERR(host->sdirq);
   579				goto err_free_dma;
   580			}
   581		}
   582	
   583		mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
   584		mmc->ops = &litex_mmc_ops;
   585	
   586		mmc->f_min = 12.5e6;
   587		mmc->f_max = 50e6;
   588	
   589		ret = mmc_of_parse(mmc);
   590		if (ret)
   591			goto err_free_dma;
   592	
   593		/* force 4-bit bus_width (only width supported by hardware) */
   594		mmc->caps &= ~MMC_CAP_8_BIT_DATA;
   595		mmc->caps |= MMC_CAP_4_BIT_DATA;
   596	
   597		/* set default capabilities */
   598		mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY |
   599			     MMC_CAP_DRIVER_TYPE_D |
   600			     MMC_CAP_CMD23;
   601		mmc->caps2 |= MMC_CAP2_NO_WRITE_PROTECT |
   602			      MMC_CAP2_FULL_PWR_CYCLE |
   603			      MMC_CAP2_NO_SDIO;
   604	
   605		platform_set_drvdata(pdev, host);
   606	
   607		ret = mmc_add_host(mmc);
   608		if (ret < 0)
   609			goto err_free_dma;
   610	
   611		/* ensure DMA bus masters are disabled */
   612		litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
   613		litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);
   614	
   615		/* set up interrupt handler */
   616		if (host->irq > 0) {
 > 617			ret = request_irq(host->irq, litex_mmc_interrupt, 0,
   618					  "litex-mmc", mmc);
   619			if (ret < 0) {
   620				dev_err(&pdev->dev,
   621					"irq setup error %d, using polling\n", ret);
   622				host->irq = 0;
   623			}
   624		}
   625	
   626		/* enable card-change interrupts, or else ask for polling */
   627		if (host->irq > 0) {
   628			litex_write32(host->sdirq + LITEX_IRQ_PENDING,
   629				      SDIRQ_CARD_DETECT);	/* clears it */
   630			litex_write32(host->sdirq + LITEX_IRQ_ENABLE,
   631				      SDIRQ_CARD_DETECT);
   632		} else {
   633			mmc->caps |= MMC_CAP_NEEDS_POLL;
   634		}
   635	
   636		return 0;
   637	
   638	err_free_dma:
   639		dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
   640	err_free_host:
   641		mmc_free_host(mmc);
   642		return ret;
   643	}
   644	
   645	static int
   646	litex_mmc_remove(struct platform_device *pdev)
   647	{
   648		struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
   649	
   650		if (host->irq > 0)
 > 651			free_irq(host->irq, host->mmc);
   652		mmc_remove_host(host->mmc);
   653		dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
   654		mmc_free_host(host->mmc);
   655	
   656		return 0;
   657	}
   658	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[Stafford Horne]

On Fri, Dec 03, 2021 at 06:41:55PM -0500, Gabriel Somlo 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!
> 
> Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
> Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Karol Gugala <kgugala@antmicro.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: David Abdurachmanov <david.abdurachmanov@sifive.com>
> Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> ---
>  drivers/mmc/host/Kconfig     |   6 +
>  drivers/mmc/host/Makefile    |   1 +
>  drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
>  3 files changed, 684 insertions(+)
>  create mode 100644 drivers/mmc/host/litex_mmc.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5af8494c31b5..84c64e72195d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1093,3 +1093,9 @@ config MMC_OWL
>  
>  config MMC_SDHCI_EXTERNAL_DMA
>  	bool
> +
> +config MMC_LITEX
> +	tristate "Support for the MMC Controller in LiteX SOCs"
> +	depends on OF && LITEX

Shall we allow compile test here like this?

       depends on OF || COMPILE_TEST
       depends on LITEX || COMPILE_TEST

This is what was added for liteuart [0].

[0] https://lore.kernel.org/all/CAHp75Ve9MB4MW9KDPoNhnPa8TCabmMgLbt6H7qrGgwmA8CpdNg@mail.gmail.com/T/#mad93ad951031f8e975a1c632873f516598aec272

> +	help
> +	  Generic MCC driver for LiteX
> 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..3877379757cd
> --- /dev/null
> +++ b/drivers/mmc/host/litex_mmc.c
> @@ -0,0 +1,677 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LiteX LiteSDCard driver
> + *
> + * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/litex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/slot-gpio.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 SDCARD_CTRL_DATA_TRANSFER_NONE  0
> +#define SDCARD_CTRL_DATA_TRANSFER_READ  1
> +#define SDCARD_CTRL_DATA_TRANSFER_WRITE 2
> +
> +#define SDCARD_CTRL_RESPONSE_NONE       0
> +#define SDCARD_CTRL_RESPONSE_SHORT      1
> +#define SDCARD_CTRL_RESPONSE_LONG       2
> +#define SDCARD_CTRL_RESPONSE_SHORT_BUSY 3
> +
> +#define SD_OK         0
> +#define SD_WRITEERROR 1
> +#define SD_TIMEOUT    2
> +#define SD_CRCERROR   3
> +#define SD_ERR_OTHER  4
> +
> +#define SDIRQ_CARD_DETECT    1
> +#define SDIRQ_SD_TO_MEM_DONE 2
> +#define SDIRQ_MEM_TO_SD_DONE 4
> +#define SDIRQ_CMD_DONE       8
> +
> +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;
> +
> +	u32 resp[4];
> +	u16 rca;
> +
> +	void *buffer;
> +	size_t buf_size;
> +	dma_addr_t dma;
> +
> +	unsigned int freq;
> +	unsigned int clock;
> +	bool is_bus_width_set;
> +	bool app_cmd;
> +
> +	int irq;
> +	struct completion cmd_done;
> +};
> +
> +static int
> +sdcard_wait_done(void __iomem *reg)
> +{
> +	u8 evt;
> +
> +	for (;;) {
> +		evt = litex_read8(reg);
> +		if (evt & 0x1)
> +			break;
> +		udelay(5);
> +	}
Should we replace this with something like read_poll_timeout?

	if (read_poll_timeout(litex_read8, evt, (evt & 0x1),
			      5, 20000, false, reg)) {
       		pr_err ("read timeout ....");
		return SD_TIMEOUT;
	}

Otherwise we could be locked here forever.

> +	if (evt == 0x1)
> +		return SD_OK;
> +	if (evt & 0x2)
> +		return SD_WRITEERROR;
> +	if (evt & 0x4)
> +		return SD_TIMEOUT;
> +	if (evt & 0x8)
> +		return SD_CRCERROR;
> +	pr_err("%s: unknown error evt=%x\n", __func__, evt);
> +	return SD_ERR_OTHER;
> +}
> +
> +static int
> +send_cmd(struct litex_mmc_host *host,
> +	 u8 cmd, u32 arg, u8 response_len, u8 transfer)
> +{
> +	void __iomem *reg;
> +	ulong n;
> +	u8 i;
> +	int status;
> +
> +	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 != SDCARD_CTRL_DATA_TRANSFER_NONE ||
> +	     response_len == SDCARD_CTRL_RESPONSE_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);
> +	}
> +
> +	status = sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT);
> +
> +	if (status != SD_OK) {
> +		pr_err("Command (cmd %d) failed, status %d\n", cmd, status);
> +		return status;
> +	}
> +
> +	if (response_len != SDCARD_CTRL_RESPONSE_NONE) {
> +		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 == SDCARD_CTRL_DATA_TRANSFER_NONE)
> +		return status; /* SD_OK from prior sdcard_wait_done(cmd_evt) */
> +
> +	status = sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT);
> +	if (status != SD_OK) {
> +		pr_err("Data xfer (cmd %d) failed, status %d\n", cmd, status);
> +		return status;
> +	}
> +
> +	/* wait for completion of (read or write) DMA transfer */
> +	reg = (transfer == SDCARD_CTRL_DATA_TRANSFER_READ) ?
> +		host->sdreader + LITEX_BLK2MEM_DONE :
> +		host->sdwriter + LITEX_MEM2BLK_DONE;
> +	n = jiffies + (HZ << 1);
> +	while ((litex_read8(reg) & 0x01) == 0)
> +		if (time_after(jiffies, n)) {
> +			pr_err("DMA timeout (cmd %d)\n", cmd);
> +			return SD_TIMEOUT;
> +		}

We could use read_poll_timeout here too.

> +
> +	return status;
> +}
> +

-Stafford

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

[Stafford Horne]

On Fri, Dec 03, 2021 at 06:41:55PM -0500, Gabriel Somlo 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!
> 
> Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
> Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Karol Gugala <kgugala@antmicro.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: David Abdurachmanov <david.abdurachmanov@sifive.com>
> Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> ---
>  drivers/mmc/host/Kconfig     |   6 +
>  drivers/mmc/host/Makefile    |   1 +
>  drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
>  3 files changed, 684 insertions(+)
>  create mode 100644 drivers/mmc/host/litex_mmc.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5af8494c31b5..84c64e72195d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1093,3 +1093,9 @@ config MMC_OWL
>  
>  config MMC_SDHCI_EXTERNAL_DMA
>  	bool
> +
> +config MMC_LITEX
> +	tristate "Support for the MMC Controller in LiteX SOCs"
> +	depends on OF && LITEX
> +	help
> +	  Generic MCC driver for LiteX

I just noticed this while configuring the kernel.  This doesn't really follow
the pattern of other drivers, we should think of putting "Litex" near the
beginning of the line. It makes it easier to spot in menuconfig.

For example:

LiteX MMC Controller support

This selects support for the MMC Host Controller found in LiteX SOCs.

It unsure, say N.

-Stafford

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

[Gabriel L. Somlo]

On Mon, Dec 06, 2021 at 06:39:36AM +0900, Stafford Horne wrote:
> On Fri, Dec 03, 2021 at 06:41:55PM -0500, Gabriel Somlo 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!
> > 
> > Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > Cc: Mateusz Holenko <mholenko@antmicro.com>
> > Cc: Karol Gugala <kgugala@antmicro.com>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: Stafford Horne <shorne@gmail.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: David Abdurachmanov <david.abdurachmanov@sifive.com>
> > Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> > ---
> >  drivers/mmc/host/Kconfig     |   6 +
> >  drivers/mmc/host/Makefile    |   1 +
> >  drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 684 insertions(+)
> >  create mode 100644 drivers/mmc/host/litex_mmc.c
> > 
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 5af8494c31b5..84c64e72195d 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -1093,3 +1093,9 @@ config MMC_OWL
> >  
> >  config MMC_SDHCI_EXTERNAL_DMA
> >  	bool
> > +
> > +config MMC_LITEX
> > +	tristate "Support for the MMC Controller in LiteX SOCs"
> > +	depends on OF && LITEX
> > +	help
> > +	  Generic MCC driver for LiteX
> 
> I just noticed this while configuring the kernel.  This doesn't really follow
> the pattern of other drivers, we should think of putting "Litex" near the
> beginning of the line. It makes it easier to spot in menuconfig.
> 
> For example:
> 
> LiteX MMC Controller support
> 
> This selects support for the MMC Host Controller found in LiteX SOCs.
> 
> It unsure, say N.

Done, and also lined up for v3. Thanks!
--Gabriel

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

[Joel Stanley]

 On Fri, 3 Dec 2021 at 23:42, Gabriel Somlo <gsomlo@gmail.com> wrote:
>
> LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
> that targets FPGAs. LiteSDCard is a small footprint, configurable
> SDCard core commonly used in LiteX designs.
>
> The driver was first written in May 2020 and has been maintained
> cooperatively by the LiteX community. Thanks to all contributors!
>
> Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
> Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Karol Gugala <kgugala@antmicro.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: David Abdurachmanov <david.abdurachmanov@sifive.com>
> Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> ---
>  drivers/mmc/host/Kconfig     |   6 +
>  drivers/mmc/host/Makefile    |   1 +
>  drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
>  3 files changed, 684 insertions(+)
>  create mode 100644 drivers/mmc/host/litex_mmc.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5af8494c31b5..84c64e72195d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1093,3 +1093,9 @@ config MMC_OWL
>
>  config MMC_SDHCI_EXTERNAL_DMA
>         bool
> +
> +config MMC_LITEX
> +       tristate "Support for the MMC Controller in LiteX SOCs"

Did you test using this as a module?

> +       depends on OF && LITEX

I don't like having litex drivers depend on the LITEX kconfig. The
symbol is not user visible, and to enable it we need to build in the
litex controller driver, which platforms may or may not have.

The microwatt platform is an example of a SoC that embeds some LITEX
IP, but may or may not be a litex SoC.

> +       help
> +         Generic MCC driver for LiteX
> 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..3877379757cd
> --- /dev/null
> +++ b/drivers/mmc/host/litex_mmc.c
> @@ -0,0 +1,677 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LiteX LiteSDCard driver
> + *
> + * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>

You should list your co-authors here I think.

> +
> +static int
> +sdcard_wait_done(void __iomem *reg)
> +{
> +       u8 evt;
> +
> +       for (;;) {
> +               evt = litex_read8(reg);
> +               if (evt & 0x1)
> +                       break;
> +               udelay(5);

This is an infinite loop. Take a look at the commits here and grab the
fix to make it timeout:

https://github.com/shenki/linux/commits/microwatt-v5.16

Well behaving hardware should be ok, but a broken or missing IP block
will cause the kernel to lock up for ever.

> +       }
> +       if (evt == 0x1)
> +               return SD_OK;
> +       if (evt & 0x2)
> +               return SD_WRITEERROR;
> +       if (evt & 0x4)
> +               return SD_TIMEOUT;
> +       if (evt & 0x8)
> +               return SD_CRCERROR;
> +       pr_err("%s: unknown error evt=%x\n", __func__, evt);
> +       return SD_ERR_OTHER;

I would consider refactoring the driver to not have it's own error
codes that map to real ones. You can get rid of map_status too.

> +}
> +
> +static int
> +send_cmd(struct litex_mmc_host *host,
> +        u8 cmd, u32 arg, u8 response_len, u8 transfer)
> +{
> +       void __iomem *reg;
> +       ulong n;
> +       u8 i;
> +       int status;
> +
> +       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 != SDCARD_CTRL_DATA_TRANSFER_NONE ||
> +            response_len == SDCARD_CTRL_RESPONSE_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);
> +       }
> +
> +       status = sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT);
> +
> +       if (status != SD_OK) {
> +               pr_err("Command (cmd %d) failed, status %d\n", cmd, status);

Take a look at the patch in my tree that fixes up the error messages
to have the driver prefix (dev_err/dev_info/etc).


> +               return status;
> +       }
> +
> +       if (response_len != SDCARD_CTRL_RESPONSE_NONE) {
> +               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 == SDCARD_CTRL_DATA_TRANSFER_NONE)
> +               return status; /* SD_OK from prior sdcard_wait_done(cmd_evt) */
> +
> +       status = sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT);
> +       if (status != SD_OK) {
> +               pr_err("Data xfer (cmd %d) failed, status %d\n", cmd, status);
> +               return status;
> +       }
> +
> +       /* wait for completion of (read or write) DMA transfer */
> +       reg = (transfer == SDCARD_CTRL_DATA_TRANSFER_READ) ?
> +               host->sdreader + LITEX_BLK2MEM_DONE :
> +               host->sdwriter + LITEX_MEM2BLK_DONE;
> +       n = jiffies + (HZ << 1);
> +       while ((litex_read8(reg) & 0x01) == 0)
> +               if (time_after(jiffies, n)) {

Use readx_poll_timeout.

> +                       pr_err("DMA timeout (cmd %d)\n", cmd);
> +                       return SD_TIMEOUT;
> +               }
> +
> +       return status;
> +}
> +
> +static inline int
> +send_app_cmd(struct litex_mmc_host *host)
> +{
> +       return send_cmd(host, MMC_APP_CMD, host->rca << 16,
> +                       SDCARD_CTRL_RESPONSE_SHORT,
> +                       SDCARD_CTRL_DATA_TRANSFER_NONE);
> +}
> +
> +static inline int
> +send_app_set_bus_width_cmd(struct litex_mmc_host *host, u32 width)
> +{
> +       return send_cmd(host, SD_APP_SET_BUS_WIDTH, width,
> +                       SDCARD_CTRL_RESPONSE_SHORT,
> +                       SDCARD_CTRL_DATA_TRANSFER_NONE);
> +}
> +
> +static int
> +litex_set_bus_width(struct litex_mmc_host *host)
> +{
> +       bool app_cmd_sent = host->app_cmd; /* was preceding command app_cmd? */
> +       int status;
> +
> +       /* ensure 'app_cmd' precedes 'app_set_bus_width_cmd' */
> +       if (!app_cmd_sent)
> +               send_app_cmd(host);
> +
> +       /* litesdcard only supports 4-bit bus width */
> +       status = send_app_set_bus_width_cmd(host, MMC_BUS_WIDTH_4);
> +
> +       /* re-send 'app_cmd' if necessary */
> +       if (app_cmd_sent)
> +               send_app_cmd(host);
> +
> +       return status;
> +}
> +
> +static int
> +litex_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 = mmc_gpio_get_cd(mmc);

Bindings.

> +       if (ret >= 0)
> +               /* GPIO based card-detect explicitly specified in DTS */
> +               ret = !!ret;
> +       else
> +               /* use gateware card-detect bit by default */
> +               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_response_len(struct mmc_command *cmd)
> +{
> +       if (cmd->flags & MMC_RSP_136) {
> +               return SDCARD_CTRL_RESPONSE_LONG;
> +       } else if (cmd->flags & MMC_RSP_PRESENT) {
> +               if (cmd->flags & MMC_RSP_BUSY)
> +                       return SDCARD_CTRL_RESPONSE_SHORT_BUSY;
> +               else
> +                       return SDCARD_CTRL_RESPONSE_SHORT;
> +       }
> +       return SDCARD_CTRL_RESPONSE_NONE;
> +}
> +
> +static int
> +litex_map_status(int status)
> +{
> +       int error;
> +
> +       switch (status) {
> +       case SD_OK:
> +               error = 0;
> +               break;
> +       case SD_WRITEERROR:
> +               error = -EIO;
> +               break;
> +       case SD_TIMEOUT:
> +               error = -ETIMEDOUT;
> +               break;
> +       case SD_CRCERROR:
> +               error = -EILSEQ;
> +               break;
> +       default:
> +               error = -EINVAL;
> +               break;
> +       }
> +       return error;
> +}
> +
> +static void
> +litex_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> +       struct litex_mmc_host *host = mmc_priv(mmc);
> +       struct platform_device *pdev = to_platform_device(mmc->parent);
> +       struct device *dev = &pdev->dev;
> +       struct mmc_data *data = mrq->data;
> +       struct mmc_command *sbc = mrq->sbc;
> +       struct mmc_command *cmd = mrq->cmd;
> +       struct mmc_command *stop = mrq->stop;
> +       unsigned int retries = cmd->retries;
> +       int status;
> +       int sg_count;
> +       enum dma_data_direction dir = DMA_TO_DEVICE;
> +       bool direct = false;
> +       dma_addr_t dma;
> +       unsigned int len = 0;
> +
> +       u32 response_len = litex_response_len(cmd);
> +       u32 transfer = SDCARD_CTRL_DATA_TRANSFER_NONE;
> +
> +       /* First check that the card is still there */
> +       if (!litex_get_cd(mmc)) {
> +               cmd->error = -ENOMEDIUM;
> +               mmc_request_done(mmc, mrq);
> +               return;
> +       }
> +
> +       /* Send set-block-count command if needed */
> +       if (sbc) {
> +               status = send_cmd(host, sbc->opcode, sbc->arg,
> +                                 litex_response_len(sbc),
> +                                 SDCARD_CTRL_DATA_TRANSFER_NONE);
> +               sbc->error = litex_map_status(status);
> +               if (status != SD_OK) {
> +                       host->is_bus_width_set = false;
> +                       mmc_request_done(mmc, mrq);
> +                       return;
> +               }
> +       }
> +
> +       if (data) {

This is a big ol' if(). Consider splitting it (or some of it?) out
into some other functions to make it easier to read.

> +               /* 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!
> +                */
> +               if (!host->is_bus_width_set) {
> +                       ulong n = jiffies + 2 * HZ; // 500ms timeout
> +
> +                       while (litex_set_bus_width(host) != SD_OK) {
> +                               if (time_after(jiffies, n)) {
> +                                       dev_warn(dev, "Can't set bus width!\n");
> +                                       cmd->error = -ETIMEDOUT;
> +                                       mmc_request_done(mmc, mrq);
> +                                       return;
> +                               }
> +                       }
> +                       host->is_bus_width_set = true;
> +               }
> +
> +               /* 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;
> +               if (data->flags & MMC_DATA_READ)
> +                       dir = DMA_FROM_DEVICE
> +               sg_count = dma_map_sg(&host->dev->dev,
> +                                     data->sg, data->sg_len, dir)

dma_map_sg(..., 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 = SDCARD_CTRL_DATA_TRANSFER_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 = SDCARD_CTRL_DATA_TRANSFER_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);
> +       }
> +
> +       do {
> +               status = send_cmd(host, cmd->opcode, cmd->arg,
> +                                 response_len, transfer);
> +       } while (status != SD_OK && retries-- > 0);
> +
> +       cmd->error = litex_map_status(status);
> +       if (status != SD_OK)
> +               /* card may be gone; don't assume bus width is still set */
> +               host->is_bus_width_set = false;
> +
> +       if (response_len == SDCARD_CTRL_RESPONSE_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 == SDCARD_CTRL_RESPONSE_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)) {
> +               int stop_stat;
> +
> +               stop_stat = send_cmd(host, stop->opcode, stop->arg,
> +                                    litex_response_len(stop),
> +                                    SDCARD_CTRL_DATA_TRANSFER_NONE);
> +               stop->error = litex_map_status(stop_stat);
> +               if (stop_stat != SD_OK)
> +                       host->is_bus_width_set = false;
> +       }
> +
> +       if (data)
> +               dma_unmap_sg(&host->dev->dev, data->sg, data->sg_len, dir);
> +
> +       if (status == SD_OK && transfer != SDCARD_CTRL_DATA_TRANSFER_NONE) {
> +               data->bytes_xfered = min(len, mmc->max_req_size);
> +               if (transfer == SDCARD_CTRL_DATA_TRANSFER_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_set_clk(struct litex_mmc_host *host, unsigned int clk_freq)
> +{
> +       u32 div = clk_freq ? host->freq / clk_freq : 256;
> +
> +       div = roundup_pow_of_two(div);
> +       div = min_t(u32, max_t(u32, div, 2), 256);
> +       dev_info(&host->dev->dev, "clk_freq=%d: set to %d via div=%d\n",
> +                clk_freq, host->freq / div, div);

dev_dbg?

> +       litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div);
> +}
> +
> +static void
> +litex_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 sdcard clock */
> +       if (ios->clock != host->clock) {
> +               litex_set_clk(host, ios->clock);
> +               host->clock = ios->clock;
> +       }
> +}
> +
> +static const struct mmc_host_ops litex_mmc_ops = {
> +       .get_cd = litex_get_cd,
> +       .request = litex_request,
> +       .set_ios = litex_set_ios,
> +};
> +
> +static int
> +litex_mmc_probe(struct platform_device *pdev)
> +{
> +       struct litex_mmc_host *host;
> +       struct mmc_host *mmc;
> +       struct device_node *cpu;
> +       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;
> +
> +       host->clock = 0;
> +       cpu = of_get_next_cpu_node(NULL);
> +       ret = of_property_read_u32(cpu, "clock-frequency", &host->freq);

> +       of_node_put(cpu);
> +       if (ret) {
> +               dev_err(&pdev->dev, "No \"clock-frequency\" property in DT\n");
> +               goto err_free_host;
> +       }
> +
> +       init_completion(&host->cmd_done);
> +       host->irq = platform_get_irq(pdev, 0);
> +       if (host->irq < 0)
> +               dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");

Can you put all of the irq handling together? Move the hardware sanity
checking up higher and put it together too:

litex_write32(host->sdirq + LITEX_IRQ_PENDING, SDIRQ_CARD_DETECT);
  /* clears it */
/* ensure DMA bus masters are disabled */
litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);

if (host->irq < 0) {
                dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
                mmc->caps |= MMC_CAP_NEEDS_POLL;
} else {
              ret = request_irq(host->irq, litex_mmc_interrupt, 0,
"litex-mmc", mmc);
              if (ret < 0) {
                 goto err_free_host;
               /* enable card-change interrupts */
               litex_write32(host->sdirq + LITEX_IRQ_ENABLE, SDIRQ_CARD_DETECT);
 }

> +
> +       /* 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;
> +
> +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));

Is this going to be true on all platforms? How do we handle those
where it's not true?

> +       if (ret)
> +               goto err_free_host;
> +
> +       host->buf_size = mmc->max_req_size * 2;
> +       host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
> +                                         &host->dma, GFP_DMA);

dmam_alloc_coherent?

> +       if (host->buffer == NULL) {
> +               ret = -ENOMEM;
> +               goto err_free_host;
> +       }
> +
> +
> +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> +       mmc->ops = &litex_mmc_ops;
> +
> +       mmc->f_min = 12.5e6;
> +       mmc->f_max = 50e6;

How did you pick these?

Are these always absolute? (wouldn't they depend on the system they
are in? host clocks?)

> +
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               goto err_free_dma;
> +
> +       /* 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_FULL_PWR_CYCLE |
> +                     MMC_CAP2_NO_SDIO;
> +
> +       platform_set_drvdata(pdev, host);
> +
> +       ret = mmc_add_host(mmc);
> +       if (ret < 0)
> +               goto err_free_dma;
> +

> +
> +       return 0;
> +
> +err_free_dma:
> +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
> +err_free_host:
> +       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);
> +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
> +       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 <www.antmicro.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.31.1
>

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

[Geert Uytterhoeven]

Hi Joel,

On Mon, Dec 6, 2021 at 11:53 AM Joel Stanley <joel@jms.id.au> wrote:
>  On Fri, 3 Dec 2021 at 23:42, Gabriel Somlo <gsomlo@gmail.com> wrote:
> > LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
> > that targets FPGAs. LiteSDCard is a small footprint, configurable
> > SDCard core commonly used in LiteX designs.
> >
> > The driver was first written in May 2020 and has been maintained
> > cooperatively by the LiteX community. Thanks to all contributors!
> >
> > Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig

> Did you test using this as a module?
>
> > +       depends on OF && LITEX
>
> I don't like having litex drivers depend on the LITEX kconfig. The
> symbol is not user visible, and to enable it we need to build in the
> litex controller driver, which platforms may or may not have.
>
> The microwatt platform is an example of a SoC that embeds some LITEX
> IP, but may or may not be a litex SoC.

I do like the LITEX dependency, as it allows us to gate off a bunch of
related drivers, and avoid annoying users with questions about them,
using a single symbol.

Originally, people told me the system controller is always present,
hence the current logic to have LITEX_SOC_CONTROLLER visible, and
an invisible LITEX (which is shorter to type) for individual drivers
to depend on.

Perhaps the logic should be reworked, now the old assumptions are no
longer true?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Joel Stanley]

On Mon, 6 Dec 2021 at 12:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > +       depends on OF && LITEX
> >
> > I don't like having litex drivers depend on the LITEX kconfig. The
> > symbol is not user visible, and to enable it we need to build in the
> > litex controller driver, which platforms may or may not have.
> >
> > The microwatt platform is an example of a SoC that embeds some LITEX
> > IP, but may or may not be a litex SoC.
>
> I do like the LITEX dependency, as it allows us to gate off a bunch of
> related drivers, and avoid annoying users with questions about them,
> using a single symbol.

I appreciate your concern.

We could do this:

        depends on PPC_MICROWATT || LITEX || COMPILE_TEST

It's unfortunate that kconfig doesn't let us describe the difference
between "this driver requires this symbol" as it won't build and "this
driver is only useful when this symbol is enabled". Traditionally I
write kconfig to represent only the former, whereas you prefer both.

> Originally, people told me the system controller is always present,
> hence the current logic to have LITEX_SOC_CONTROLLER visible, and
> an invisible LITEX (which is shorter to type) for individual drivers
> to depend on.

That's another option. I think LITEX either needs to become visible,
become selected by microwatt, or we adopt the proposal I made above
for the litex drivers that the microwatt soc uses.

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

[Gabriel L. Somlo]

On Mon, Dec 06, 2021 at 11:51:22PM +0000, Joel Stanley wrote:
> On Mon, 6 Dec 2021 at 12:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > > > +       depends on OF && LITEX
> > >
> > > I don't like having litex drivers depend on the LITEX kconfig. The
> > > symbol is not user visible, and to enable it we need to build in the
> > > litex controller driver, which platforms may or may not have.
> > >
> > > The microwatt platform is an example of a SoC that embeds some LITEX
> > > IP, but may or may not be a litex SoC.
> >
> > I do like the LITEX dependency, as it allows us to gate off a bunch of
> > related drivers, and avoid annoying users with questions about them,
> > using a single symbol.
> 
> I appreciate your concern.
> 
> We could do this:
> 
>         depends on PPC_MICROWATT || LITEX || COMPILE_TEST

What about the current OF dependency? Is that covered by COMPILE_TEST,
or do we need an additional `depends on` line for it?

Thanks,
--G

> It's unfortunate that kconfig doesn't let us describe the difference
> between "this driver requires this symbol" as it won't build and "this
> driver is only useful when this symbol is enabled". Traditionally I
> write kconfig to represent only the former, whereas you prefer both.
> 
> > Originally, people told me the system controller is always present,
> > hence the current logic to have LITEX_SOC_CONTROLLER visible, and
> > an invisible LITEX (which is shorter to type) for individual drivers
> > to depend on.
> 
> That's another option. I think LITEX either needs to become visible,
> become selected by microwatt, or we adopt the proposal I made above
> for the litex drivers that the microwatt soc uses.

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

[Geert Uytterhoeven]

Hi Joel,

On Tue, Dec 7, 2021 at 12:51 AM Joel Stanley <joel@jms.id.au> wrote:
> On Mon, 6 Dec 2021 at 12:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > +       depends on OF && LITEX
> > >
> > > I don't like having litex drivers depend on the LITEX kconfig. The
> > > symbol is not user visible, and to enable it we need to build in the
> > > litex controller driver, which platforms may or may not have.
> > >
> > > The microwatt platform is an example of a SoC that embeds some LITEX
> > > IP, but may or may not be a litex SoC.
> >
> > I do like the LITEX dependency, as it allows us to gate off a bunch of
> > related drivers, and avoid annoying users with questions about them,
> > using a single symbol.
>
> I appreciate your concern.
>
> We could do this:
>
>         depends on PPC_MICROWATT || LITEX || COMPILE_TEST
>
> It's unfortunate that kconfig doesn't let us describe the difference
> between "this driver requires this symbol" as it won't build and "this
> driver is only useful when this symbol is enabled". Traditionally I
> write kconfig to represent only the former, whereas you prefer both.

The former is expressed using:

    depends on FOO

The latter using:

    depends on BAR || COMPILE_TEST

Traditionally we only had the former. But with the introduction of
more and more dummy symbols for the !CONFIG_BAR case, the amount of
questions asked during configuration has become overwhelming.  At the
current pace, we'll reach 20K config symbols by v5.24 or so...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Gabriel L. Somlo]

Hi Joel,

Thanks for the review. Couple of thoughts and follow-up questions
inline:

On Mon, Dec 06, 2021 at 10:53:17AM +0000, Joel Stanley wrote:
>  On Fri, 3 Dec 2021 at 23:42, Gabriel Somlo <gsomlo@gmail.com> wrote:
> >
> > LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
> > that targets FPGAs. LiteSDCard is a small footprint, configurable
> > SDCard core commonly used in LiteX designs.
> >
> > The driver was first written in May 2020 and has been maintained
> > cooperatively by the LiteX community. Thanks to all contributors!
> >
> > Co-developed-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > Cc: Mateusz Holenko <mholenko@antmicro.com>
> > Cc: Karol Gugala <kgugala@antmicro.com>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: Stafford Horne <shorne@gmail.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: David Abdurachmanov <david.abdurachmanov@sifive.com>
> > Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> > ---
> >  drivers/mmc/host/Kconfig     |   6 +
> >  drivers/mmc/host/Makefile    |   1 +
> >  drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 684 insertions(+)
> >  create mode 100644 drivers/mmc/host/litex_mmc.c
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 5af8494c31b5..84c64e72195d 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -1093,3 +1093,9 @@ config MMC_OWL
> >
> >  config MMC_SDHCI_EXTERNAL_DMA
> >         bool
> > +
> > +config MMC_LITEX
> > +       tristate "Support for the MMC Controller in LiteX SOCs"
> 
> Did you test using this as a module?
> 
> > +       depends on OF && LITEX
> 
> I don't like having litex drivers depend on the LITEX kconfig. The
> symbol is not user visible, and to enable it we need to build in the
> litex controller driver, which platforms may or may not have.
> 
> The microwatt platform is an example of a SoC that embeds some LITEX
> IP, but may or may not be a litex SoC.

I can remove dependency on "LITEX" and, with that, succesfully build
the driver as a module -- same principle as the LiteETH driver. 
I'm queueing that up for the long promised v3, soon as I clear up a
few remaining questions... :)

Right now I have:

	depends on OF || COMPILE_TEST

> > +       help
> > +         Generic MCC driver for LiteX
> > 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..3877379757cd
> > --- /dev/null
> > +++ b/drivers/mmc/host/litex_mmc.c
> > @@ -0,0 +1,677 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteX LiteSDCard driver
> > + *
> > + * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
> 
> You should list your co-authors here I think.

Done.

> > +
> > +static int
> > +sdcard_wait_done(void __iomem *reg)
> > +{
> > +       u8 evt;
> > +
> > +       for (;;) {
> > +               evt = litex_read8(reg);
> > +               if (evt & 0x1)
> > +                       break;
> > +               udelay(5);
> 
> This is an infinite loop. Take a look at the commits here and grab the
> fix to make it timeout:
> 
> https://github.com/shenki/linux/commits/microwatt-v5.16
> 
> Well behaving hardware should be ok, but a broken or missing IP block
> will cause the kernel to lock up for ever.

Replaced with readx_poll_timeout().

> > +       }
> > +       if (evt == 0x1)
> > +               return SD_OK;
> > +       if (evt & 0x2)
> > +               return SD_WRITEERROR;
> > +       if (evt & 0x4)
> > +               return SD_TIMEOUT;
> > +       if (evt & 0x8)
> > +               return SD_CRCERROR;
> > +       pr_err("%s: unknown error evt=%x\n", __func__, evt);
> > +       return SD_ERR_OTHER;
> 
> I would consider refactoring the driver to not have it's own error
> codes that map to real ones. You can get rid of map_status too.

Done.

> > +}
> > +
> > +static int
> > +send_cmd(struct litex_mmc_host *host,
> > +        u8 cmd, u32 arg, u8 response_len, u8 transfer)
> > +{
> > +       void __iomem *reg;
> > +       ulong n;
> > +       u8 i;
> > +       int status;
> > +
> > +       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 != SDCARD_CTRL_DATA_TRANSFER_NONE ||
> > +            response_len == SDCARD_CTRL_RESPONSE_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);
> > +       }
> > +
> > +       status = sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT);
> > +
> > +       if (status != SD_OK) {
> > +               pr_err("Command (cmd %d) failed, status %d\n", cmd, status);
> 
> Take a look at the patch in my tree that fixes up the error messages
> to have the driver prefix (dev_err/dev_info/etc).

Done, thanks.

> > +               return status;
> > +       }
> > +
> > +       if (response_len != SDCARD_CTRL_RESPONSE_NONE) {
> > +               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 == SDCARD_CTRL_DATA_TRANSFER_NONE)
> > +               return status; /* SD_OK from prior sdcard_wait_done(cmd_evt) */
> > +
> > +       status = sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT);
> > +       if (status != SD_OK) {
> > +               pr_err("Data xfer (cmd %d) failed, status %d\n", cmd, status);
> > +               return status;
> > +       }
> > +
> > +       /* wait for completion of (read or write) DMA transfer */
> > +       reg = (transfer == SDCARD_CTRL_DATA_TRANSFER_READ) ?
> > +               host->sdreader + LITEX_BLK2MEM_DONE :
> > +               host->sdwriter + LITEX_MEM2BLK_DONE;
> > +       n = jiffies + (HZ << 1);
> > +       while ((litex_read8(reg) & 0x01) == 0)
> > +               if (time_after(jiffies, n)) {
> 
> Use readx_poll_timeout.

Done.
 
> > +                       pr_err("DMA timeout (cmd %d)\n", cmd);
> > +                       return SD_TIMEOUT;
> > +               }
> > +
> > +       return status;
> > +}
> > +
> > +static inline int
> > +send_app_cmd(struct litex_mmc_host *host)
> > +{
> > +       return send_cmd(host, MMC_APP_CMD, host->rca << 16,
> > +                       SDCARD_CTRL_RESPONSE_SHORT,
> > +                       SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +}
> > +
> > +static inline int
> > +send_app_set_bus_width_cmd(struct litex_mmc_host *host, u32 width)
> > +{
> > +       return send_cmd(host, SD_APP_SET_BUS_WIDTH, width,
> > +                       SDCARD_CTRL_RESPONSE_SHORT,
> > +                       SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +}
> > +
> > +static int
> > +litex_set_bus_width(struct litex_mmc_host *host)
> > +{
> > +       bool app_cmd_sent = host->app_cmd; /* was preceding command app_cmd? */
> > +       int status;
> > +
> > +       /* ensure 'app_cmd' precedes 'app_set_bus_width_cmd' */
> > +       if (!app_cmd_sent)
> > +               send_app_cmd(host);
> > +
> > +       /* litesdcard only supports 4-bit bus width */
> > +       status = send_app_set_bus_width_cmd(host, MMC_BUS_WIDTH_4);
> > +
> > +       /* re-send 'app_cmd' if necessary */
> > +       if (app_cmd_sent)
> > +               send_app_cmd(host);
> > +
> > +       return status;
> > +}
> > +
> > +static int
> > +litex_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 = mmc_gpio_get_cd(mmc);
> 
> Bindings.

This was part of the original Antmicro version of the driver, but I
have never actually used gpio based card detection. I'm inclined to
remove it from this submission entirely (and keep it around as an
out-of-tree fixup patch) until someone with the appropriate hardware
setup can provide a "tested-by" (and preferably an example on how to
configure it in DT).

Thoughts?

> > +       if (ret >= 0)
> > +               /* GPIO based card-detect explicitly specified in DTS */
> > +               ret = !!ret;
> > +       else
> > +               /* use gateware card-detect bit by default */
> > +               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_response_len(struct mmc_command *cmd)
> > +{
> > +       if (cmd->flags & MMC_RSP_136) {
> > +               return SDCARD_CTRL_RESPONSE_LONG;
> > +       } else if (cmd->flags & MMC_RSP_PRESENT) {
> > +               if (cmd->flags & MMC_RSP_BUSY)
> > +                       return SDCARD_CTRL_RESPONSE_SHORT_BUSY;
> > +               else
> > +                       return SDCARD_CTRL_RESPONSE_SHORT;
> > +       }
> > +       return SDCARD_CTRL_RESPONSE_NONE;
> > +}
> > +
> > +static int
> > +litex_map_status(int status)
> > +{
> > +       int error;
> > +
> > +       switch (status) {
> > +       case SD_OK:
> > +               error = 0;
> > +               break;
> > +       case SD_WRITEERROR:
> > +               error = -EIO;
> > +               break;
> > +       case SD_TIMEOUT:
> > +               error = -ETIMEDOUT;
> > +               break;
> > +       case SD_CRCERROR:
> > +               error = -EILSEQ;
> > +               break;
> > +       default:
> > +               error = -EINVAL;
> > +               break;
> > +       }
> > +       return error;
> > +}
> > +
> > +static void
> > +litex_request(struct mmc_host *mmc, struct mmc_request *mrq)
> > +{
> > +       struct litex_mmc_host *host = mmc_priv(mmc);
> > +       struct platform_device *pdev = to_platform_device(mmc->parent);
> > +       struct device *dev = &pdev->dev;
> > +       struct mmc_data *data = mrq->data;
> > +       struct mmc_command *sbc = mrq->sbc;
> > +       struct mmc_command *cmd = mrq->cmd;
> > +       struct mmc_command *stop = mrq->stop;
> > +       unsigned int retries = cmd->retries;
> > +       int status;
> > +       int sg_count;
> > +       enum dma_data_direction dir = DMA_TO_DEVICE;
> > +       bool direct = false;
> > +       dma_addr_t dma;
> > +       unsigned int len = 0;
> > +
> > +       u32 response_len = litex_response_len(cmd);
> > +       u32 transfer = SDCARD_CTRL_DATA_TRANSFER_NONE;
> > +
> > +       /* First check that the card is still there */
> > +       if (!litex_get_cd(mmc)) {
> > +               cmd->error = -ENOMEDIUM;
> > +               mmc_request_done(mmc, mrq);
> > +               return;
> > +       }
> > +
> > +       /* Send set-block-count command if needed */
> > +       if (sbc) {
> > +               status = send_cmd(host, sbc->opcode, sbc->arg,
> > +                                 litex_response_len(sbc),
> > +                                 SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +               sbc->error = litex_map_status(status);
> > +               if (status != SD_OK) {
> > +                       host->is_bus_width_set = false;
> > +                       mmc_request_done(mmc, mrq);
> > +                       return;
> > +               }
> > +       }
> > +
> > +       if (data) {
> 
> This is a big ol' if(). Consider splitting it (or some of it?) out
> into some other functions to make it easier to read.

I'll see what I can do before I submit v3 -- depending on how much
surgery is needed to extricate just the body of that if() from the
rest of the function :)

> > +               /* 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!
> > +                */
> > +               if (!host->is_bus_width_set) {
> > +                       ulong n = jiffies + 2 * HZ; // 500ms timeout
> > +
> > +                       while (litex_set_bus_width(host) != SD_OK) {
> > +                               if (time_after(jiffies, n)) {
> > +                                       dev_warn(dev, "Can't set bus width!\n");
> > +                                       cmd->error = -ETIMEDOUT;
> > +                                       mmc_request_done(mmc, mrq);
> > +                                       return;
> > +                               }
> > +                       }
> > +                       host->is_bus_width_set = true;
> > +               }
> > +
> > +               /* 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;
> > +               if (data->flags & MMC_DATA_READ)
> > +                       dir = DMA_FROM_DEVICE
> > +               sg_count = dma_map_sg(&host->dev->dev,
> > +                                     data->sg, data->sg_len, dir)
> 
> dma_map_sg(..., mmc_get_dma_dir(data))

Nice, and much cleaner -- thanks! And done.

> > +               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 = SDCARD_CTRL_DATA_TRANSFER_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 = SDCARD_CTRL_DATA_TRANSFER_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);
> > +       }
> > +
> > +       do {
> > +               status = send_cmd(host, cmd->opcode, cmd->arg,
> > +                                 response_len, transfer);
> > +       } while (status != SD_OK && retries-- > 0);
> > +
> > +       cmd->error = litex_map_status(status);
> > +       if (status != SD_OK)
> > +               /* card may be gone; don't assume bus width is still set */
> > +               host->is_bus_width_set = false;
> > +
> > +       if (response_len == SDCARD_CTRL_RESPONSE_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 == SDCARD_CTRL_RESPONSE_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)) {
> > +               int stop_stat;
> > +
> > +               stop_stat = send_cmd(host, stop->opcode, stop->arg,
> > +                                    litex_response_len(stop),
> > +                                    SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +               stop->error = litex_map_status(stop_stat);
> > +               if (stop_stat != SD_OK)
> > +                       host->is_bus_width_set = false;
> > +       }
> > +
> > +       if (data)
> > +               dma_unmap_sg(&host->dev->dev, data->sg, data->sg_len, dir);
> > +
> > +       if (status == SD_OK && transfer != SDCARD_CTRL_DATA_TRANSFER_NONE) {
> > +               data->bytes_xfered = min(len, mmc->max_req_size);
> > +               if (transfer == SDCARD_CTRL_DATA_TRANSFER_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_set_clk(struct litex_mmc_host *host, unsigned int clk_freq)
> > +{
> > +       u32 div = clk_freq ? host->freq / clk_freq : 256;
> > +
> > +       div = roundup_pow_of_two(div);
> > +       div = min_t(u32, max_t(u32, div, 2), 256);
> > +       dev_info(&host->dev->dev, "clk_freq=%d: set to %d via div=%d\n",
> > +                clk_freq, host->freq / div, div);
> 
> dev_dbg?

Done.
 
> > +       litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div);
> > +}
> > +
> > +static void
> > +litex_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 sdcard clock */
> > +       if (ios->clock != host->clock) {
> > +               litex_set_clk(host, ios->clock);
> > +               host->clock = ios->clock;
> > +       }
> > +}
> > +
> > +static const struct mmc_host_ops litex_mmc_ops = {
> > +       .get_cd = litex_get_cd,
> > +       .request = litex_request,
> > +       .set_ios = litex_set_ios,
> > +};
> > +
> > +static int
> > +litex_mmc_probe(struct platform_device *pdev)
> > +{
> > +       struct litex_mmc_host *host;
> > +       struct mmc_host *mmc;
> > +       struct device_node *cpu;
> > +       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;
> > +
> > +       host->clock = 0;
> > +       cpu = of_get_next_cpu_node(NULL);
> > +       ret = of_property_read_u32(cpu, "clock-frequency", &host->freq);
> 
> > +       of_node_put(cpu);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "No \"clock-frequency\" property in DT\n");
> > +               goto err_free_host;
> > +       }
> > +
> > +       init_completion(&host->cmd_done);
> > +       host->irq = platform_get_irq(pdev, 0);
> > +       if (host->irq < 0)
> > +               dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
> 
> Can you put all of the irq handling together? Move the hardware sanity
> checking up higher and put it together too:
> 
> litex_write32(host->sdirq + LITEX_IRQ_PENDING, SDIRQ_CARD_DETECT);
>   /* clears it */
> /* ensure DMA bus masters are disabled */
> litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
> litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);
> 
> if (host->irq < 0) {
>                 dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
>                 mmc->caps |= MMC_CAP_NEEDS_POLL;
> } else {
>               ret = request_irq(host->irq, litex_mmc_interrupt, 0,
> "litex-mmc", mmc);
>               if (ret < 0) {
>                  goto err_free_host;
>                /* enable card-change interrupts */
>                litex_write32(host->sdirq + LITEX_IRQ_ENABLE, SDIRQ_CARD_DETECT);
>  }

I moved it all as close together as possible, but it has to all go
*after* all of the `dev_platform_ioremap_resource[_byname]()` calls,
since those pointers are all used when calling `litex_write*()`.

I'll have it in v3, and you can let me know then if it's OK or still
needs yet more work.

> > +
> > +       /* 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;
> > +
> > +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> 
> Is this going to be true on all platforms? How do we handle those
> where it's not true?

I'll need to do a bit of digging here, unless anyone has ideas ready
to go...

> > +       if (ret)
> > +               goto err_free_host;
> > +
> > +       host->buf_size = mmc->max_req_size * 2;
> > +       host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
> > +                                         &host->dma, GFP_DMA);
> 
> dmam_alloc_coherent?

Does this mean I no longer have to `dma[m]_free_coherent()` at [1] and
[2] below, since it's automatically handled by the "managed" bits?

> > +       if (host->buffer == NULL) {
> > +               ret = -ENOMEM;
> > +               goto err_free_host;
> > +       }
> > +
> > +
> > +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > +       mmc->ops = &litex_mmc_ops;
> > +
> > +       mmc->f_min = 12.5e6;
> > +       mmc->f_max = 50e6;
> 
> How did you pick these?
> 
> Are these always absolute? (wouldn't they depend on the system they
> are in? host clocks?)

They are the minimum and maximum frequency empirically observed to work
on typical sdcard media with LiteSDCard, in the wild. I can state that
in a comment (after I do another pass of double-checking, crossing Ts
and dotting Is), hope that's what you were looking for.
 
> > +
> > +       ret = mmc_of_parse(mmc);
> > +       if (ret)
> > +               goto err_free_dma;
> > +
> > +       /* 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_FULL_PWR_CYCLE |
> > +                     MMC_CAP2_NO_SDIO;
> > +
> > +       platform_set_drvdata(pdev, host);
> > +
> > +       ret = mmc_add_host(mmc);
> > +       if (ret < 0)
> > +               goto err_free_dma;
> > +
> 
> > +
> > +       return 0;
> > +
> > +err_free_dma:
> > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);

[1] is this made optional by having used `dmam_alloc_coherent()` above?

> > +err_free_host:
> > +       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);
> > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);

[2] ditto?

Thanks again for all the help and advice!

--Gabriel

> > +       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 <www.antmicro.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.31.1
> >

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

[Joel Stanley]

On Tue, 7 Dec 2021 at 01:23, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> I can remove dependency on "LITEX" and, with that, succesfully build
> the driver as a module -- same principle as the LiteETH driver.
> I'm queueing that up for the long promised v3, soon as I clear up a
> few remaining questions... :)

If we have the driver as a 'tristate' we should make sure it loads and
works as a module.

>
> Right now I have:
>
>         depends on OF || COMPILE_TEST

The OF dependency is a requirement for the symbols you're using. See
the discussion I had with Greet, I think going with this is reasonable
for the first pass:

 depends on OF
 depends on PPC_MICROWATT || LITEX || COMPILE_TEST

> > > +static int
> > > +litex_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 = mmc_gpio_get_cd(mmc);
> >
> > Bindings.
>
> This was part of the original Antmicro version of the driver, but I
> have never actually used gpio based card detection. I'm inclined to
> remove it from this submission entirely (and keep it around as an
> out-of-tree fixup patch) until someone with the appropriate hardware
> setup can provide a "tested-by" (and preferably an example on how to
> configure it in DT).

Agreed, if it's untested and unused then delete it.

> > > +static u32
> > > +litex_response_len(struct mmc_command *cmd)

something else I noticed when re-reading the code; we can put the
return arguments on the same line as the functions. The kernel has a
nice long column limit now, so there's no need to shorten lines
unncessarily. Feel free to go through the driver and fix that up.

> > Can you put all of the irq handling together? Move the hardware sanity
> > checking up higher and put it together too:

> I moved it all as close together as possible, but it has to all go
> *after* all of the `dev_platform_ioremap_resource[_byname]()` calls,
> since those pointers are all used when calling `litex_write*()`.

Makes sense.

> I'll have it in v3, and you can let me know then if it's OK or still
> needs yet more work.

> > > +
> > > +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> >
> > Is this going to be true on all platforms? How do we handle those
> > where it's not true?
>
> I'll need to do a bit of digging here, unless anyone has ideas ready
> to go...

I'm not an expert either, so let's consult the docs:

Documentation/core-api/dma-api-howto.rst

This suggests we should be using dma_set_mask_and_coherent?

But we're setting the mask to 32, which is the default, so perhaps we
don't need this call at all?

(I was thinking of the microwatt soc, which is a 64 bit soc but the
peripherals are on a 32 bit bus, and some of the devices are behind a
smaller bus again. But I think we're ok, as the DMA wishbone is
32-bit).

>
> > > +       if (ret)
> > > +               goto err_free_host;
> > > +
> > > +       host->buf_size = mmc->max_req_size * 2;
> > > +       host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
> > > +                                         &host->dma, GFP_DMA);
> >
> > dmam_alloc_coherent?
>
> Does this mean I no longer have to `dma[m]_free_coherent()` at [1] and
> [2] below, since it's automatically handled by the "managed" bits?

Yep spot on.

> > > +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > +       mmc->ops = &litex_mmc_ops;
> > > +
> > > +       mmc->f_min = 12.5e6;
> > > +       mmc->f_max = 50e6;
> >
> > How did you pick these?
> >
> > Are these always absolute? (wouldn't they depend on the system they
> > are in? host clocks?)
>
> They are the minimum and maximum frequency empirically observed to work
> on typical sdcard media with LiteSDCard, in the wild. I can state that
> in a comment (after I do another pass of double-checking, crossing Ts
> and dotting Is), hope that's what you were looking for.

Lets add a comment describing that.

> > > +
> > > +       return 0;
> > > +
> > > +err_free_dma:
> > > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
>
> [1] is this made optional by having used `dmam_alloc_coherent()` above?

Yes, we can remove this.

> > > +err_free_host:
> > > +       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);
> > > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);
>
> [2] ditto?

Yep.

> Thanks again for all the help and advice!

No problem. Thanks for taking the time to upstream the code.

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

[Gabriel L. Somlo]

Hi Joel,

On Tue, Dec 07, 2021 at 02:46:03AM +0000, Joel Stanley wrote:
> On Tue, 7 Dec 2021 at 01:23, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> > > > [...]
> > > > +
> > > > +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> > >
> > > Is this going to be true on all platforms? How do we handle those
> > > where it's not true?
> >
> > I'll need to do a bit of digging here, unless anyone has ideas ready
> > to go...
> 
> I'm not an expert either, so let's consult the docs:
> 
> Documentation/core-api/dma-api-howto.rst
> 
> This suggests we should be using dma_set_mask_and_coherent?
> 
> But we're setting the mask to 32, which is the default, so perhaps we
> don't need this call at all?
> 
> (I was thinking of the microwatt soc, which is a 64 bit soc but the
> peripherals are on a 32 bit bus, and some of the devices are behind a
> smaller bus again. But I think we're ok, as the DMA wishbone is
> 32-bit).
 
So I did a bit of digging, and as it turns out the LiteX DMA base
registers are 64-bit wide, which I think means that they can
essentially do `xlen` bits of DMA addressing, at least when used
as part of a LiteX SoC (no idea what additional quirks occur if/when
LiteSDCard, or any other 64-bit-DMA-capable LiteX IP block would be
used as a standalone component in a different system).

Does this mean that, depending on maybe CONFIG_ARCH_DMA_ADDR_T_64BIT
or something similar, we should actually set DMA_BIT_MASK(64)? Maybe
something like:

#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
	if (ret)
		goto err;
#endif

Leave it to the default 32 unless we're on a 64-bit-DMA capable
system, in which case it's safe to assume we need the above setting?

What do you think, does that make sense?

Thanks,
--Gabriel

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

[Gabriel L. Somlo]

Hi Joel,

On Mon, Dec 06, 2021 at 10:53:17AM +0000, Joel Stanley wrote:
>  On Fri, 3 Dec 2021 at 23:42, Gabriel Somlo <gsomlo@gmail.com> wrote:
> > +static void
> litex_request> +litex_request(struct mmc_host *mmc, struct mmc_request *mrq)
> > +{
> > +       struct litex_mmc_host *host = mmc_priv(mmc);
> > +       struct platform_device *pdev = to_platform_device(mmc->parent);
> > +       struct device *dev = &pdev->dev;
> > +       struct mmc_data *data = mrq->data;
> > +       struct mmc_command *sbc = mrq->sbc;
> > +       struct mmc_command *cmd = mrq->cmd;
> > +       struct mmc_command *stop = mrq->stop;
> > +       unsigned int retries = cmd->retries;
> > +       int status;
> > +       int sg_count;
> > +       enum dma_data_direction dir = DMA_TO_DEVICE;
> > +       bool direct = false;
> > +       dma_addr_t dma;
> > +       unsigned int len = 0;
> > +
> > +       u32 response_len = litex_response_len(cmd);
> > +       u32 transfer = SDCARD_CTRL_DATA_TRANSFER_NONE;
> > +
> > +       /* First check that the card is still there */
> > +       if (!litex_get_cd(mmc)) {
> > +               cmd->error = -ENOMEDIUM;
> > +               mmc_request_done(mmc, mrq);
> > +               return;
> > +       }
> > +
> > +       /* Send set-block-count command if needed */
> > +       if (sbc) {
> > +               status = send_cmd(host, sbc->opcode, sbc->arg,
> > +                                 litex_response_len(sbc),
> > +                                 SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +               sbc->error = litex_map_status(status);
> > +               if (status != SD_OK) {
> > +                       host->is_bus_width_set = false;
> > +                       mmc_request_done(mmc, mrq);
> > +                       return;
> > +               }
> > +       }
> > +
> > +       if (data) {
> 
> This is a big ol' if(). Consider splitting it (or some of it?) out
> into some other functions to make it easier to read.
> 

I can factor out the dma transfer portion into a dedicated helper
function:

   static void litex_mmc_data_dma_transfer(struct litex_mmc_host *host,
                                           struct mmc_data *data,
                                           unsigned int *len,
                                           bool *direct,
                                           u8 *transfer)

where `len`, `direct`, and `transfer` are passed in as pointers,
because the helper function modifies their values and the caller
(i.e., `litex_[mmc_]request()`) is subsequently using those
potentially modified-by-the-callee values.

If you still feel the extra call overhead is worth the tradeoff in
improved legibility and code grouping, let me know and I can have it
out in version 4 (I sent out v3 earlier this morning without changing
this part).

Thanks,
--Gabriel