Discussion:
[RFC 0/2] Add support for Meson MX "SDIO" MMC driver
Martin Blumenstingl
2017-05-06 17:18:55 UTC
Permalink
This is the successor to Carlo Caione's "Add support for Amlogic Meson
MMC driver" series (v5) from [0].

I would like you to specifically review:
- whether I've (ab)used the MMC framework properly (as this is my first
"larger" contribution to an MMC driver)
- I think I have improved the locking compared to Carlo's version,
however I'd still like feedback on whether this looks sane now or if I
can improve that even further

(notable) changes since Carlo's latest version are:
- renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls
the driver "aml_sdio" as there is a second MMC controller in these SoCs
which they call the "SDHC controller"). do the same with our driver to
avoid confusion once we add support for the second controller (which uses
a completely different register layout)
- add support for the internal "mux" in this MMC controller (which allows
connecting up to three devices to the the controller - at the cost of
performance though since the controller can only process one request at
a time). The driver registers a new device for each sub-node, which is
then fed into the MMC framework to allow per-slot configuration using
devicetree (see the example in the documentation)
- use the common clock framework internally for managing the MMC clock
(there is a fixed-factor clock in the controller which takes clk81 as
input and divides it's clock by two and a divider clock which takes
the result from the fixed-factor clock as input)
- support the regulators provided by the MMC framework
- support for GPIO-based card-detection and read-only-detection through
the MMC framework
- use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it
make sense (and thus the code easier to read)
- re-worked locking (based on the locking in dw_mmc as that also provides
multiple "MMC slots")

tests done so far:
- reading an OLD 256MiB SD card (which uses only a 1-bit bus) works fine
(sha1sum of the whole device matches with what I get on my PC's
card-reader)
- reading a somewhat more modern class 10 SD card and putting Arch Linux
ARM on it (and using that as root file system)
- it successfully detects the RTL8723BS SDIO wifi chip in my device (even
if the SD card is also enabled)
- reading a 128MiB file from the SD card while scanning wifi networks on
the RTL8723BS card does not seem to result in any corruption (sha1sum
of the read file seems to match)
- read speed of my class 10 SD card: ~15MiB/s
- (unfortunately I could NOT test downloading a file over wifi to the SD
card because the RTL8723BS driver refuses to see any wifi networks, but
that might be a problem on the RTL8723BS driver side since I don't get
any error and the driver has just landed a few weeks ago in staging)


[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/412136.html

Carlo Caione (2):
dt-bindings: mmc: Document the Amlogic Meson8 and Meson8b SDIO
bindings
mmc: meson-mx-sdio: Add a driver for the Amlogic Meson8 and Meson8b
SoCs

.../bindings/mmc/amlogic,meson-mx-sdio.txt | 50 ++
drivers/mmc/host/Kconfig | 12 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/meson-mx-sdio.c | 978 +++++++++++++++++++++
4 files changed, 1041 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
create mode 100644 drivers/mmc/host/meson-mx-sdio.c
--
2.12.2
Martin Blumenstingl
2017-05-06 17:18:56 UTC
Permalink
From: Carlo Caione <***@endlessm.com>

This documents the devicetree bindings for the SDIO/MMC host found in
Amlogic Meson8 and Meson8b SoCs. It supports the SD specification v2.0
and the eMMC specification v4.41.
It has an internal "mux" which allows connecting up to three MMC devices
to it. The maximum supported bus-width is 4-bits.

Amlogic's GPL kernel sources call it "SDIO" to differentiate it from the
other MMC controller in (at least the Meson8 and Meson8b) the SoCs (they
call the other one "SDHC", which supports a bus-width of up to 8-bits).

Signed-off-by: Carlo Caione <***@endlessm.com>
Signed-off-by: Martin Blumenstingl <***@googlemail.com>
---
.../bindings/mmc/amlogic,meson-mx-sdio.txt | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt

diff --git a/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt b/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
new file mode 100644
index 000000000000..991c43a602bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
@@ -0,0 +1,50 @@
+* Amlogic Meson6, Meson8 and Meson8b SDIO/MMC controller
+
+The highspeed MMC host controller on Amlogic SoCs provides an interface
+for MMC, SD, SDIO and SDHC types of memory cards.
+
+Supported maximum speeds are the ones of the eMMC standard 4.41 as well
+as the speed of SD standard 2.0.
+
+The hardware provides an internal "mux" which allows up to three slots
+to be controlled. Only one device can be accessed at a time.
+
+Required properties:
+ - compatible : must be one of
+ - "amlogic,meson8-sdio"
+ - "amlogic,meson8b-sdio"
+ along with the generic "amlogic,meson-mx-sdio"
+ - reg : mmc controller base registers
+ - interrupts : mmc controller interrupt
+ - #address-cells : must be 1
+ - size-cells : must be 0
+ - clocks : phandle to clock providers
+ - clock-names : must contain "core" and "clkin"
+
+Required child nodes:
+A node for each slot provided by the MMC controller is required.
+
+Required properties on each child node (= slot):
+ - reg : the slot (or "port") ID
+
+Optional properties on each child node (= slot):
+ - bus-width : must be 1 or 4 (8-bit bus is not supported)
+ - for cd and all other additional generic mmc parameters
+ please refer to mmc.txt within this directory
+
+Examples:
+ ***@c1108c20 {
+ compatible = "amlogic,meson8-sdio", "amlogic,meson-mx-sdio";
+ reg = <0xc1108c20 0x20>;
+ interrupts = <0 28 1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clkc CLKID_SDIO>, <&clkc CLKID_CLK81>;
+ clock-names = "core", "clkin";
+
+ ***@1 {
+ reg = <1>;
+
+ bus-width = <4>;
+ };
+ };
--
2.12.2
Rob Herring
2017-05-12 19:05:21 UTC
Permalink
Post by Martin Blumenstingl
This documents the devicetree bindings for the SDIO/MMC host found in
Amlogic Meson8 and Meson8b SoCs. It supports the SD specification v2.0
and the eMMC specification v4.41.
It has an internal "mux" which allows connecting up to three MMC devices
to it. The maximum supported bus-width is 4-bits.
Amlogic's GPL kernel sources call it "SDIO" to differentiate it from the
other MMC controller in (at least the Meson8 and Meson8b) the SoCs (they
call the other one "SDHC", which supports a bus-width of up to 8-bits).
---
.../bindings/mmc/amlogic,meson-mx-sdio.txt | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
Acked-by: Rob Herring <***@kernel.org>
Ulf Hansson
2017-05-29 10:04:48 UTC
Permalink
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
This documents the devicetree bindings for the SDIO/MMC host found in
Amlogic Meson8 and Meson8b SoCs. It supports the SD specification v2.0
and the eMMC specification v4.41.
It has an internal "mux" which allows connecting up to three MMC devices
to it. The maximum supported bus-width is 4-bits.
Amlogic's GPL kernel sources call it "SDIO" to differentiate it from the
other MMC controller in (at least the Meson8 and Meson8b) the SoCs (they
call the other one "SDHC", which supports a bus-width of up to 8-bits).
---
.../bindings/mmc/amlogic,meson-mx-sdio.txt | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
diff --git a/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt b/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
new file mode 100644
index 000000000000..991c43a602bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt
@@ -0,0 +1,50 @@
+* Amlogic Meson6, Meson8 and Meson8b SDIO/MMC controller
+
+The highspeed MMC host controller on Amlogic SoCs provides an interface
+for MMC, SD, SDIO and SDHC types of memory cards.
+
+Supported maximum speeds are the ones of the eMMC standard 4.41 as well
+as the speed of SD standard 2.0.
+
+The hardware provides an internal "mux" which allows up to three slots
+to be controlled. Only one device can be accessed at a time.
+
+ - compatible : must be one of
+ - "amlogic,meson8-sdio"
+ - "amlogic,meson8b-sdio"
+ along with the generic "amlogic,meson-mx-sdio"
+ - reg : mmc controller base registers
+ - interrupts : mmc controller interrupt
+ - #address-cells : must be 1
+ - size-cells : must be 0
+ - clocks : phandle to clock providers
+ - clock-names : must contain "core" and "clkin"
+
+A node for each slot provided by the MMC controller is required.
+
+ - reg : the slot (or "port") ID
Please add also that we need a compatible = "mmc-slot", as we are
using req for childnodes to describe SDIO func nodes and embedded
cards.

While doing this, could you perhaps also update the generic mmc DT
doc, as currently the compatible is described only in
Documentation/devicetree/bindings/mmc/cavium-mmc.txt.
Post by Martin Blumenstingl
+
+ - bus-width : must be 1 or 4 (8-bit bus is not supported)
+ - for cd and all other additional generic mmc parameters
+ please refer to mmc.txt within this directory
+
+ compatible = "amlogic,meson8-sdio", "amlogic,meson-mx-sdio";
+ reg = <0xc1108c20 0x20>;
+ interrupts = <0 28 1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clkc CLKID_SDIO>, <&clkc CLKID_CLK81>;
+ clock-names = "core", "clkin";
+
+ reg = <1>;
+
+ bus-width = <4>;
+ };
+ };
--
2.12.2
Kind regards
Uffe
Martin Blumenstingl
2017-05-06 17:18:57 UTC
Permalink
From: Carlo Caione <***@endlessm.com>

Add a driver for the SDIO/MMC host found on the Amlogic Meson SoCs. This
is an MMC controller which provides an interface between the application
processor and various memory cards. It supports the SD specification
v2.0 and the eMMC specification v4.41.

The controller provides an internal "mux" which allows connecting up to
three MMC devices to it. Only one device can be used at a time though
since the registers are shared across all devices. The driver takes care
of synchronizing access (similar to the dw_mmc driver).
The maximum supported bus-width is 4-bits.

Amlogic's GPL kernel sources call the corresponding driver "aml_sdio" to
differentiate it from the other MMC controller in (at least the Meson8
and Meson8b) the SoCs (they call the other drivers aml_sdhc and
aml_sdhc_m8, which seem to support a bus-width of up to 8-bits).

Signed-off-by: Carlo Caione <***@endlessm.com>
Signed-off-by: Martin Blumenstingl <***@googlemail.com>
---
drivers/mmc/host/Kconfig | 12 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/meson-mx-sdio.c | 978 +++++++++++++++++++++++++++++++++++++++
3 files changed, 991 insertions(+)
create mode 100644 drivers/mmc/host/meson-mx-sdio.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index a638cd0d80be..c557482ae327 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -343,6 +343,18 @@ config MMC_MESON_GX

If you have a controller with this interface, say Y here.

+config MMC_MESON_MX_SDIO
+ tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller support"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on HAS_DMA
+ depends on OF
+ help
+ This selects support for the SD/MMC Host Controller on
+ Amlogic Meson6, Meson8 and Meson8b SoCs.
+
+ If you have a controller with this interface, say Y or M here.
+ If unsure, say N.
+
config MMC_MOXART
tristate "MOXART SD/MMC Host Controller support"
depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index bc2c2e2c68c0..f9500ab2bc86 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_VUB300) += vub300.o
obj-$(CONFIG_MMC_USHC) += ushc.o
obj-$(CONFIG_MMC_WMT) += wmt-sdmmc.o
obj-$(CONFIG_MMC_MESON_GX) += meson-gx-mmc.o
+obj-$(CONFIG_MMC_MESON_MX_SDIO) += meson-mx-sdio.o
obj-$(CONFIG_MMC_MOXART) += moxart-mmc.o
obj-$(CONFIG_MMC_SUNXI) += sunxi-mmc.o
obj-$(CONFIG_MMC_USDHI6ROL0) += usdhi6rol0.o
diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c
new file mode 100644
index 000000000000..2da9c5249ae7
--- /dev/null
+++ b/drivers/mmc/host/meson-mx-sdio.c
@@ -0,0 +1,978 @@
+/*
+ * meson-mx-sdio.c - Meson6, Meson8 and Meson8b SDIO/MMC Host Controller
+ *
+ * Copyright (C) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <***@endlessm.com>
+ * Copyright (C) 2017 Martin Blumenstingl <***@googlemail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/timer.h>
+#include <linux/types.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/slot-gpio.h>
+
+#define MESON_MX_SDIO_ARGU 0x00
+
+#define MESON_MX_SDIO_SEND 0x04
+ #define MESON_MX_SDIO_SEND_COMMAND_INDEX_MASK GENMASK(7, 0)
+ #define MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK GENMASK(15, 8)
+ #define MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7 BIT(16)
+ #define MESON_MX_SDIO_SEND_RESP_HAS_DATA BIT(17)
+ #define MESON_MX_SDIO_SEND_RESP_CRC7_FROM_8 BIT(18)
+ #define MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY BIT(19)
+ #define MESON_MX_SDIO_SEND_DATA BIT(20)
+ #define MESON_MX_SDIO_SEND_USE_INT_WINDOW BIT(21)
+ #define MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK GENMASK(31, 24)
+
+#define MESON_MX_SDIO_CONF 0x08
+ #define MESON_MX_SDIO_CONF_CMD_CLK_DIV_SHIFT 0
+ #define MESON_MX_SDIO_CONF_CMD_CLK_DIV_WIDTH 10
+ #define MESON_MX_SDIO_CONF_CMD_DISABLE_CRC BIT(10)
+ #define MESON_MX_SDIO_CONF_CMD_OUT_AT_POSITIVE_EDGE BIT(11)
+ #define MESON_MX_SDIO_CONF_CMD_ARGUMENT_BITS_MASK GENMASK(17, 12)
+ #define MESON_MX_SDIO_CONF_RESP_LATCH_AT_NEGATIVE_EDGE BIT(18)
+ #define MESON_MX_SDIO_CONF_DATA_LATCH_AT_NEGATIVE_EDGE BIT(19)
+ #define MESON_MX_SDIO_CONF_BUS_WIDTH BIT(20)
+ #define MESON_MX_SDIO_CONF_M_ENDIAN_MASK GENMASK(22, 21)
+ #define MESON_MX_SDIO_CONF_WRITE_NWR_MASK GENMASK(28, 23)
+ #define MESON_MX_SDIO_CONF_WRITE_CRC_OK_STATUS_MASK GENMASK(31, 29)
+
+#define MESON_MX_SDIO_IRQS 0x0c
+ #define MESON_MX_SDIO_IRQS_STATUS_STATE_MACHINE_MASK GENMASK(3, 0)
+ #define MESON_MX_SDIO_IRQS_CMD_BUSY BIT(4)
+ #define MESON_MX_SDIO_IRQS_RESP_CRC7_OK BIT(5)
+ #define MESON_MX_SDIO_IRQS_DATA_READ_CRC16_OK BIT(6)
+ #define MESON_MX_SDIO_IRQS_DATA_WRITE_CRC16_OK BIT(7)
+ #define MESON_MX_SDIO_IRQS_IF_INT BIT(8)
+ #define MESON_MX_SDIO_IRQS_CMD_INT BIT(9)
+ #define MESON_MX_SDIO_IRQS_STATUS_INFO_MASK GENMASK(15, 12)
+ #define MESON_MX_SDIO_IRQS_TIMING_OUT_INT BIT(16)
+ #define MESON_MX_SDIO_IRQS_AMRISC_TIMING_OUT_INT_EN BIT(17)
+ #define MESON_MX_SDIO_IRQS_ARC_TIMING_OUT_INT_EN BIT(18)
+ #define MESON_MX_SDIO_IRQS_TIMING_OUT_COUNT_MASK GENMASK(31, 19)
+
+#define MESON_MX_SDIO_IRQC 0x10
+ #define MESON_MX_SDIO_IRQC_ARC_IF_INT_EN BIT(3)
+ #define MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN BIT(4)
+ #define MESON_MX_SDIO_IRQC_IF_CONFIG_MASK GENMASK(7, 6)
+ #define MESON_MX_SDIO_IRQC_SOFT_RESET BIT(15)
+ #define MESON_MX_SDIO_IRQC_FORCE_HALT BIT(30)
+ #define MESON_MX_SDIO_IRQC_HALT_HOLE BIT(31)
+
+#define MESON_MX_SDIO_MULT 0x14
+ #define MESON_MX_SDIO_MULT_PORT_SEL_MASK GENMASK(1, 0)
+ #define MESON_MX_SDIO_MULT_MEMORY_STICK_ENABLE BIT(2)
+ #define MESON_MX_SDIO_MULT_MEMORY_STICK_SCLK_ALWAYS BIT(3)
+ #define MESON_MX_SDIO_MULT_STREAM_ENABLE BIT(4)
+ #define MESON_MX_SDIO_MULT_STREAM_8BITS_MODE BIT(5)
+ #define MESON_MX_SDIO_MULT_WR_RD_OUT_INDEX BIT(8)
+ #define MESON_MX_SDIO_MULT_DAT0_DAT1_SWAPPED BIT(10)
+ #define MESON_MX_SDIO_MULT_DAT1_DAT0_SWAPPED BIT(11)
+ #define MESON_MX_SDIO_MULT_RESP_READ_INDEX_MASK GENMASK(15, 12)
+
+#define MESON_MX_SDIO_ADDR 0x18
+
+#define MESON_MX_SDIO_EXT 0x1c
+ #define MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK GENMASK(29, 16)
+
+#define MESON_MX_SDIO_BOUNCE_REQ_SIZE (128 * 1024)
+#define MESON_MX_SDIO_RESPONSE_CRC16_BITS (16 - 1)
+#define MESON_MX_SDIO_MAX_SLOTS 3
+
+enum meson_mx_mmc_host_status {
+ MESON_MX_MMC_STATUS_IDLE,
+ MESON_MX_MMC_STATUS_BUSY,
+ MESON_MX_MMC_STATUS_SHUTTING_DOWN,
+};
+
+struct meson_mx_mmc_slot {
+ struct mmc_host *mmc;
+ struct meson_mx_mmc_host *host;
+
+ struct mmc_request *mrq;
+ struct mmc_command *cmd;
+ int error;
+
+ unsigned int id;
+ struct list_head queue_node;
+};
+
+struct meson_mx_mmc_host {
+ struct device *dev;
+
+ struct clk *parent_clk;
+ struct clk *core_clk;
+ struct clk_divider cfg_div;
+ struct clk *cfg_div_clk;
+ struct clk_fixed_factor fixed_factor;
+ struct clk *fixed_factor_clk;
+
+ void __iomem *base;
+ int irq;
+ spinlock_t lock;
+ spinlock_t irq_lock;
+
+ enum meson_mx_mmc_host_status status;
+ struct list_head queue;
+ struct timer_list cmd_timeout;
+
+ struct device slot_devices[MESON_MX_SDIO_MAX_SLOTS];
+ struct meson_mx_mmc_slot *slots[MESON_MX_SDIO_MAX_SLOTS];
+ struct meson_mx_mmc_slot *current_cmd_slot;
+ struct meson_mx_mmc_slot *sdio_irq_slot;
+};
+
+static u32 meson_mx_mmc_readl(struct mmc_host *mmc, char reg)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ return readl(slot->host->base + reg);
+}
+
+static void meson_mx_mmc_writel(struct mmc_host *mmc, u32 val, char reg)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ writel(val, slot->host->base + reg);
+}
+
+static void meson_mx_mmc_mask_bits(struct mmc_host *mmc, char reg, u32 mask,
+ u32 val)
+{
+ u32 regval = meson_mx_mmc_readl(mmc, reg);
+
+ regval &= ~mask;
+ regval |= (val & mask);
+ meson_mx_mmc_writel(mmc, regval, reg);
+}
+
+static void meson_mx_mmc_soft_reset(struct meson_mx_mmc_host *host)
+{
+ writel(MESON_MX_SDIO_IRQC_SOFT_RESET, host->base + MESON_MX_SDIO_IRQC);
+ udelay(2);
+}
+
+static struct mmc_command *meson_mx_mmc_get_next_cmd(struct mmc_command *cmd)
+{
+ if (cmd->opcode == MMC_SET_BLOCK_COUNT && !cmd->error)
+ return cmd->mrq->cmd;
+ else if (mmc_op_multi(cmd->opcode) &&
+ (!cmd->mrq->sbc || cmd->error || cmd->data->error))
+ return cmd->mrq->stop;
+ else
+ return NULL;
+}
+
+static void meson_mx_mmc_apply_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long clk_rate = ios->clock;
+ int ret;
+
+ switch (ios->bus_width) {
+ case MMC_BUS_WIDTH_1:
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH, 0);
+ break;
+
+ case MMC_BUS_WIDTH_4:
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH,
+ MESON_MX_SDIO_CONF_BUS_WIDTH);
+ break;
+
+ case MMC_BUS_WIDTH_8:
+ default:
+ dev_err(mmc_dev(mmc), "unsupported bus width: %d\n",
+ ios->bus_width);
+ slot->error = -EINVAL;
+ return;
+ }
+
+ if (clk_rate) {
+ if (WARN_ON(clk_rate > mmc->f_max))
+ clk_rate = mmc->f_max;
+ else if (WARN_ON(clk_rate < mmc->f_min))
+ clk_rate = mmc->f_min;
+
+ ret = clk_set_rate(slot->host->cfg_div_clk, ios->clock);
+ if (ret) {
+ dev_warn(mmc_dev(mmc),
+ "failed to set MMC clock to %lu: %d\n",
+ clk_rate, ret);
+ slot->error = ret;
+ return;
+ }
+
+ mmc->actual_clock = clk_get_rate(slot->host->cfg_div_clk);
+ }
+}
+
+static void meson_mx_mmc_start_cmd(struct mmc_host *mmc,
+ struct mmc_command *cmd)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned int pack_size;
+ unsigned long irqflags, timeout;
+ u32 mult, send = 0, ext = 0;
+
+ slot->cmd = cmd;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ switch (mmc_resp_type(cmd)) {
+ case MMC_RSP_R1:
+ case MMC_RSP_R1B:
+ case MMC_RSP_R3:
+ /* 7 (CMD) + 32 (response) + 7 (CRC) -1 */
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK, 45);
+ break;
+ case MMC_RSP_R2:
+ /* 7 (CMD) + 120 (response) + 7 (CRC) -1 */
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK, 133);
+ send |= MESON_MX_SDIO_SEND_RESP_CRC7_FROM_8;
+ break;
+ default:
+ break;
+ }
+
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
+
+ if (cmd->data) {
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
+ (cmd->data->blocks - 1));
+
+ pack_size = cmd->data->blksz * BITS_PER_BYTE;
+ if (mmc->ios.bus_width == MMC_BUS_WIDTH_4)
+ pack_size += MESON_MX_SDIO_RESPONSE_CRC16_BITS * 4;
+ else
+ pack_size += MESON_MX_SDIO_RESPONSE_CRC16_BITS * 1;
+
+ ext |= FIELD_PREP(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
+ pack_size);
+
+ if (cmd->data->flags & MMC_DATA_WRITE)
+ send |= MESON_MX_SDIO_SEND_DATA;
+ else
+ send |= MESON_MX_SDIO_SEND_RESP_HAS_DATA;
+
+ cmd->data->bytes_xfered = 0;
+ }
+
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_COMMAND_INDEX_MASK,
+ (0x40 | cmd->opcode));
+
+ /*
+ * soft-reset the MMC core and re-apply the IOS to make sure that these
+ * are correct for the slot which is selected below (as the IOS
+ * registers are shared across all slots).
+ */
+ meson_mx_mmc_soft_reset(slot->host);
+ meson_mx_mmc_apply_ios(mmc, &mmc->ios);
+
+ mult = meson_mx_mmc_readl(mmc, MESON_MX_SDIO_MULT);
+ mult &= ~MESON_MX_SDIO_MULT_PORT_SEL_MASK;
+ mult |= FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK, slot->id);
+ mult |= BIT(31);
+ meson_mx_mmc_writel(mmc, mult, MESON_MX_SDIO_MULT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN,
+ MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN);
+
+ /* clear pending interrupts */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_CMD_INT,
+ MESON_MX_SDIO_IRQS_CMD_INT);
+
+ meson_mx_mmc_writel(mmc, cmd->arg, MESON_MX_SDIO_ARGU);
+ meson_mx_mmc_writel(mmc, ext, MESON_MX_SDIO_EXT);
+ meson_mx_mmc_writel(mmc, send, MESON_MX_SDIO_SEND);
+
+ spin_unlock_irqrestore(&slot->host->irq_lock, irqflags);
+
+ if (cmd->opcode == MMC_ERASE)
+ timeout = msecs_to_jiffies(30000);
+ else if (cmd->data)
+ timeout = msecs_to_jiffies(5000);
+ else
+ timeout = msecs_to_jiffies(1000);
+
+ mod_timer(&slot->host->cmd_timeout, jiffies + timeout);
+}
+
+static void meson_mx_mmc_start_request(struct mmc_host *mmc,
+ struct mmc_request *mrq)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ struct meson_mx_mmc_host *host = slot->host;
+
+ host->status = MESON_MX_MMC_STATUS_BUSY;
+ host->current_cmd_slot = slot;
+
+ if (mrq->data)
+ meson_mx_mmc_writel(mmc, sg_dma_address(mrq->data->sg),
+ MESON_MX_SDIO_ADDR);
+
+ if (mrq->sbc)
+ meson_mx_mmc_start_cmd(mmc, mrq->sbc);
+ else
+ meson_mx_mmc_start_cmd(mmc, mrq->cmd);
+}
+
+static void meson_mx_mmc_request_done(struct meson_mx_mmc_host *host,
+ struct meson_mx_mmc_slot *current_slot)
+ __releases(&host->lock)
+ __acquires(&host->lock)
+{
+ struct meson_mx_mmc_slot *next_slot;
+ struct mmc_request *mrq = current_slot->mrq;
+
+ current_slot->mrq = NULL;
+ current_slot->cmd = NULL;
+
+ if (list_empty(&host->queue)) {
+ dev_dbg(host->dev, "slot queue is empty\n");
+
+ host->status = MESON_MX_MMC_STATUS_IDLE;
+ host->current_cmd_slot = NULL;
+ } else {
+ next_slot = list_entry(host->queue.next,
+ struct meson_mx_mmc_slot, queue_node);
+ list_del(&next_slot->queue_node);
+
+ dev_dbg(host->dev, "next slot from queue is %s\n",
+ mmc_hostname(next_slot->mmc));
+
+ meson_mx_mmc_start_request(next_slot->mmc, next_slot->mrq);
+ }
+
+ spin_unlock(&host->lock);
+ mmc_request_done(current_slot->mmc, mrq);
+ spin_lock(&host->lock);
+}
+
+static void meson_mx_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ if (spin_trylock(&slot->host->lock)) {
+ /*
+ * only apply the mmc_ios if we are idle to not break any
+ * ongoing transfer. in case we are busy meson_mx_mmc_start_cmd
+ * will take care of applying the mmc_ios later on.
+ */
+ if (slot->host->status == MESON_MX_MMC_STATUS_IDLE)
+ meson_mx_mmc_apply_ios(mmc, ios);
+
+ spin_unlock(&slot->host->lock);
+ }
+
+ switch (ios->power_mode) {
+ case MMC_POWER_OFF:
+ if (!IS_ERR(mmc->supply.vmmc))
+ mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
+ break;
+
+ case MMC_POWER_UP:
+ if (!IS_ERR(mmc->supply.vmmc))
+ mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
+ break;
+ }
+}
+
+static void meson_mx_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_MULT,
+ MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ slot->id));
+
+ /* ACK pending interrupt */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_IF_INT,
+ MESON_MX_SDIO_IRQS_IF_INT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_IF_INT_EN,
+ enable ? MESON_MX_SDIO_IRQC_ARC_IF_INT_EN : 0);
+
+ if (enable)
+ slot->host->sdio_irq_slot = slot;
+ else
+ slot->host->sdio_irq_slot = NULL;
+
+ spin_unlock_irqrestore(&slot->host->irq_lock, irqflags);
+}
+
+static int meson_mx_mmc_map_dma(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct mmc_data *data = mrq->data;
+ int dma_len;
+ struct scatterlist *sg;
+
+ if (!data)
+ return 0;
+
+ sg = data->sg;
+ if (sg->offset & 3 || sg->length & 3) {
+ dev_err(mmc_dev(mmc),
+ "unaligned scatterlist: offset %x length %d\n",
+ sg->offset, sg->length);
+ return -EINVAL;
+ }
+
+ dma_len = dma_map_sg(mmc_dev(mmc), data->sg, data->sg_len,
+ mmc_get_dma_dir(data));
+ if (dma_len <= 0) {
+ dev_err(mmc_dev(mmc), "dma_map_sg failed\n");
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void meson_mx_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ struct meson_mx_mmc_host *host = slot->host;
+ struct mmc_command *cmd = mrq->cmd;
+
+ if (!slot->error)
+ slot->error = meson_mx_mmc_map_dma(mmc, mrq);
+
+ if (slot->error) {
+ cmd->error = slot->error;
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+
+ spin_lock(&host->lock);
+
+ slot->mrq = mrq;
+
+ if (host->status == MESON_MX_MMC_STATUS_IDLE)
+ meson_mx_mmc_start_request(mmc, mrq);
+ else
+ list_add_tail(&slot->queue_node, &host->queue);
+
+ spin_unlock(&host->lock);
+}
+
+static void meson_mx_mmc_read_response(struct mmc_host *mmc,
+ struct mmc_command *cmd)
+{
+ u32 mult;
+ int i, resp[4];
+
+ mult = meson_mx_mmc_readl(mmc, MESON_MX_SDIO_MULT);
+ mult |= MESON_MX_SDIO_MULT_WR_RD_OUT_INDEX;
+ mult &= ~MESON_MX_SDIO_MULT_RESP_READ_INDEX_MASK;
+ mult |= FIELD_PREP(MESON_MX_SDIO_MULT_RESP_READ_INDEX_MASK, 0);
+ meson_mx_mmc_writel(mmc, mult, MESON_MX_SDIO_MULT);
+
+ if (cmd->flags & MMC_RSP_136) {
+ for (i = 0; i <= 3; i++)
+ resp[3 - i] = meson_mx_mmc_readl(mmc,
+ MESON_MX_SDIO_ARGU);
+ cmd->resp[0] = (resp[0] << 8) | ((resp[1] >> 24) & 0xff);
+ cmd->resp[1] = (resp[1] << 8) | ((resp[2] >> 24) & 0xff);
+ cmd->resp[2] = (resp[2] << 8) | ((resp[3] >> 24) & 0xff);
+ cmd->resp[3] = (resp[3] << 8);
+ } else if (cmd->flags & MMC_RSP_PRESENT) {
+ cmd->resp[0] = meson_mx_mmc_readl(mmc, MESON_MX_SDIO_ARGU);
+ }
+}
+
+static irqreturn_t meson_mx_mmc_process_cmd_irq(struct meson_mx_mmc_slot *slot,
+ u32 irqs, u32 send)
+{
+ struct mmc_command *cmd;
+
+ if (WARN_ON(!slot))
+ return IRQ_HANDLED;
+
+ cmd = slot->cmd;
+ if (WARN_ON(!cmd))
+ return IRQ_HANDLED;
+
+ cmd->error = 0;
+ meson_mx_mmc_read_response(slot->mmc, cmd);
+
+ if (cmd->data) {
+ if (!((irqs & MESON_MX_SDIO_IRQS_DATA_READ_CRC16_OK) ||
+ (irqs & MESON_MX_SDIO_IRQS_DATA_WRITE_CRC16_OK)))
+ cmd->error = -EILSEQ;
+ } else {
+ if (!((irqs & MESON_MX_SDIO_IRQS_RESP_CRC7_OK) ||
+ (send & MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7)))
+ cmd->error = -EILSEQ;
+ }
+
+ return IRQ_WAKE_THREAD;
+}
+
+static void meson_mx_mmc_process_sdio_irq(struct meson_mx_mmc_slot *slot)
+{
+ /*
+ * ignore SDIO interrupts without corresponding slot as the SDIO
+ * interrupt seems to enable itself automatically - in this case we
+ * didn't assign a slot for this IRQ so we simply ignore it.
+ */
+ if (!slot)
+ return;
+
+ mmc_signal_sdio_irq(slot->mmc);
+}
+
+static irqreturn_t meson_mx_mmc_irq(int irq, void *data)
+{
+ struct meson_mx_mmc_host *host = (void *) data;
+ struct meson_mx_mmc_slot *cmd_slot, *sdio_irq_slot;
+ bool sdio_irq, cmd_irq;
+ u32 irqs, send;
+ irqreturn_t ret;
+
+ spin_lock(&host->irq_lock);
+
+ irqs = readl(host->base + MESON_MX_SDIO_IRQS);
+ send = readl(host->base + MESON_MX_SDIO_SEND);
+
+ cmd_slot = host->current_cmd_slot;
+ sdio_irq_slot = host->sdio_irq_slot;
+
+ sdio_irq = !!(irqs & MESON_MX_SDIO_IRQS_IF_INT);
+ cmd_irq = !!(irqs & MESON_MX_SDIO_IRQS_CMD_INT);
+
+ if (cmd_irq) {
+ del_timer(&host->cmd_timeout);
+
+ ret = meson_mx_mmc_process_cmd_irq(cmd_slot, irqs, send);
+ } else
+ ret = IRQ_HANDLED;
+
+ /* and finally ACK all pending interrupts */
+ writel(irqs, host->base + MESON_MX_SDIO_IRQS);
+
+ spin_unlock(&host->irq_lock);
+
+ if (sdio_irq)
+ meson_mx_mmc_process_sdio_irq(sdio_irq_slot);
+
+ return ret;
+}
+
+static irqreturn_t meson_mx_mmc_irq_thread(int irq, void *irq_data)
+{
+ struct meson_mx_mmc_host *host = (void *) irq_data;
+ struct meson_mx_mmc_slot *slot;
+ struct mmc_command *cmd, *next_cmd;
+
+ spin_lock(&host->lock);
+
+ slot = host->current_cmd_slot;
+ if (WARN_ON(!slot)) {
+ spin_unlock(&host->lock);
+ return IRQ_HANDLED;
+ }
+
+ cmd = slot->cmd;
+
+ if (cmd->data) {
+ dma_unmap_sg(mmc_dev(slot->mmc), cmd->data->sg,
+ cmd->data->sg_len,
+ mmc_get_dma_dir(cmd->data));
+
+ cmd->data->bytes_xfered = cmd->data->blksz * cmd->data->blocks;
+ }
+
+ next_cmd = meson_mx_mmc_get_next_cmd(cmd);
+ if (next_cmd)
+ meson_mx_mmc_start_cmd(slot->mmc, next_cmd);
+ else
+ meson_mx_mmc_request_done(host, slot);
+
+ spin_unlock(&host->lock);
+
+ return IRQ_HANDLED;
+}
+
+static void meson_mx_mmc_timeout(unsigned long arg)
+{
+ struct meson_mx_mmc_host *host = (void *) arg;
+ struct meson_mx_mmc_slot *slot;
+ unsigned long irqflags;
+ u32 irqc;
+
+ spin_lock(&host->lock);
+
+ /* Do not run after meson_mx_mmc_remove() */
+ if (host->status == MESON_MX_MMC_STATUS_SHUTTING_DOWN)
+ goto unlock;
+
+ /* request was completed in the meantime */
+ if (host->status == MESON_MX_MMC_STATUS_IDLE)
+ goto unlock;
+
+ spin_lock_irqsave(&host->irq_lock, irqflags);
+
+ /* disable the CMD interrupt */
+ irqc = readl(host->base + MESON_MX_SDIO_IRQC);
+ irqc &= ~MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN;
+ writel(irqc, host->base + MESON_MX_SDIO_IRQC);
+
+ spin_unlock_irqrestore(&host->irq_lock, irqflags);
+
+ slot = host->current_cmd_slot;
+
+ if (WARN_ON(!slot))
+ goto unlock;
+
+ dev_dbg(mmc_dev(slot->mmc),
+ "Timeout on CMD%u (IRQS = 0x%08x, ARGU = 0x%08x)\n",
+ slot->cmd->opcode,
+ meson_mx_mmc_readl(slot->mmc, MESON_MX_SDIO_IRQS),
+ meson_mx_mmc_readl(slot->mmc, MESON_MX_SDIO_ARGU));
+
+ slot->cmd->error = -ETIMEDOUT;
+
+ meson_mx_mmc_request_done(host, slot);
+
+unlock:
+ spin_unlock(&host->lock);
+}
+
+static struct mmc_host_ops meson_mx_mmc_ops = {
+ .request = meson_mx_mmc_request,
+ .set_ios = meson_mx_mmc_set_ios,
+ .enable_sdio_irq = meson_mx_mmc_enable_sdio_irq,
+ .get_cd = mmc_gpio_get_cd,
+ .get_ro = mmc_gpio_get_ro,
+};
+
+static void meson_mx_mmc_slot_device_release(struct device *dev)
+{
+}
+
+static int meson_mx_mmc_register_slot_device(struct device_node *np,
+ unsigned int id,
+ struct meson_mx_mmc_host *host)
+{
+ struct device *dev = &host->slot_devices[id];
+
+ dev->parent = host->dev;
+ dev->of_node = np;
+ dev->release = meson_mx_mmc_slot_device_release;
+ dev_set_name(dev, "%s.%d", dev_name(dev->parent), id);
+
+ return device_register(dev);
+}
+
+static int meson_mx_mmc_add_slot(struct device_node *np,
+ struct meson_mx_mmc_host *host)
+{
+ struct meson_mx_mmc_slot *slot;
+ struct mmc_host *mmc;
+ unsigned int id;
+ int ret;
+
+ if (of_property_read_u32(np, "reg", &id)) {
+ dev_err(host->dev, "missing 'reg' property for %s\n",
+ of_node_full_name(np));
+ return -EINVAL;
+ }
+
+ if (id >= MESON_MX_SDIO_MAX_SLOTS) {
+ dev_err(host->dev,
+ "invalid 'reg' property value of %s\n",
+ of_node_full_name(np));
+ return -EINVAL;
+ }
+
+ ret = meson_mx_mmc_register_slot_device(np, id, host);
+ if (ret)
+ return ret;
+
+ mmc = mmc_alloc_host(sizeof(*slot), &host->slot_devices[id]);
+ if (!mmc) {
+ ret = -ENOMEM;
+ goto error_unregister_dev;
+ }
+
+ slot = mmc_priv(mmc);
+ slot->mmc = mmc;
+ slot->id = id;
+ slot->host = host;
+
+ host->slots[id] = slot;
+
+ /* Get regulators and the supported OCR mask */
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto error_free_host;
+
+ mmc->max_req_size = MESON_MX_SDIO_BOUNCE_REQ_SIZE;
+ mmc->max_seg_size = mmc->max_req_size;
+ mmc->max_blk_count =
+ FIELD_GET(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
+ 0xffffffff);
+ mmc->max_blk_size = FIELD_GET(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
+ 0xffffffff);
+ mmc->max_blk_size -= (4 * MESON_MX_SDIO_RESPONSE_CRC16_BITS);
+ mmc->max_blk_size /= BITS_PER_BYTE;
+
+ /* Get the min and max supported clock rates */
+ mmc->f_min = clk_round_rate(host->cfg_div_clk, 1);
+ mmc->f_max = clk_round_rate(host->cfg_div_clk,
+ clk_get_rate(host->parent_clk));
+
+ mmc->caps |= MMC_CAP_CMD23;
+ mmc->ops = &meson_mx_mmc_ops;
+
+ ret = mmc_of_parse(mmc);
+ if (ret)
+ goto error_free_host;
+
+ ret = mmc_add_host(mmc);
+ if (ret)
+ goto error_free_host;
+
+ return 0;
+
+error_free_host:
+ mmc_free_host(mmc);
+error_unregister_dev:
+ device_unregister(&host->slot_devices[id]);
+ return ret;
+}
+
+static int meson_mx_mmc_probe_slots(struct meson_mx_mmc_host *host)
+{
+ struct device_node *slot_node, *controller_node;
+ int num_slots, ret;
+
+ controller_node = host->dev->of_node;
+
+ num_slots = of_get_available_child_count(controller_node);
+ if (num_slots > MESON_MX_SDIO_MAX_SLOTS) {
+ dev_err(host->dev, "more slots configured than supported\n");
+ return -EINVAL;
+ }
+
+ for_each_available_child_of_node(controller_node, slot_node) {
+ ret = meson_mx_mmc_add_slot(slot_node, host);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static int meson_mx_mmc_register_clks(struct meson_mx_mmc_host *host)
+{
+ struct clk_init_data init;
+ const char *clk_div_parents[1], *clk_fixed_factor_parents[1];
+
+ clk_fixed_factor_parents[0] = __clk_get_name(host->parent_clk);
+ init.name = devm_kasprintf(host->dev, GFP_KERNEL, "%s#fixed_factor",
+ dev_name(host->dev));
+ init.ops = &clk_fixed_factor_ops;
+ init.flags = 0;
+ init.parent_names = clk_fixed_factor_parents;
+ init.num_parents = 1;
+ host->fixed_factor.div = 2;
+ host->fixed_factor.mult = 1;
+ host->fixed_factor.hw.init = &init;
+
+ host->fixed_factor_clk = devm_clk_register(host->dev,
+ &host->fixed_factor.hw);
+ if (WARN_ON(PTR_ERR_OR_ZERO(host->fixed_factor_clk)))
+ return PTR_ERR(host->fixed_factor_clk);
+
+ clk_div_parents[0] = __clk_get_name(host->fixed_factor_clk);
+ init.name = devm_kasprintf(host->dev, GFP_KERNEL, "%s#div",
+ dev_name(host->dev));
+ init.ops = &clk_divider_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.parent_names = clk_div_parents;
+ init.num_parents = 1;
+ host->cfg_div.reg = host->base + MESON_MX_SDIO_CONF;
+ host->cfg_div.shift = MESON_MX_SDIO_CONF_CMD_CLK_DIV_SHIFT;
+ host->cfg_div.width = MESON_MX_SDIO_CONF_CMD_CLK_DIV_WIDTH;
+ host->cfg_div.hw.init = &init;
+ host->cfg_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+
+ host->cfg_div_clk = devm_clk_register(host->dev, &host->cfg_div.hw);
+ if (WARN_ON(PTR_ERR_OR_ZERO(host->cfg_div_clk)))
+ return PTR_ERR(host->fixed_factor_clk);
+
+ return 0;
+}
+
+static int meson_mx_mmc_probe(struct platform_device *pdev)
+{
+ struct meson_mx_mmc_host *host;
+ struct resource *res;
+ int ret, irq;
+ u32 conf;
+
+ host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
+ if (!host)
+ return -ENOMEM;
+
+ spin_lock_init(&host->lock);
+ spin_lock_init(&host->irq_lock);
+ setup_timer(&host->cmd_timeout, meson_mx_mmc_timeout,
+ (unsigned long)host);
+ INIT_LIST_HEAD(&host->queue);
+ host->dev = &pdev->dev;
+
+ platform_set_drvdata(pdev, host);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ host->base = devm_ioremap_resource(host->dev, res);
+ if (IS_ERR(host->base)) {
+ ret = PTR_ERR(host->base);
+ goto error_out;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ ret = devm_request_threaded_irq(host->dev, irq, meson_mx_mmc_irq,
+ meson_mx_mmc_irq_thread, 0, NULL,
+ host);
+ if (ret)
+ goto error_out;
+
+ host->core_clk = devm_clk_get(host->dev, "core");
+ if (IS_ERR(host->core_clk)) {
+ ret = PTR_ERR(host->core_clk);
+ goto error_out;
+ }
+
+ host->parent_clk = devm_clk_get(host->dev, "clkin");
+ if (IS_ERR(host->parent_clk)) {
+ ret = PTR_ERR(host->parent_clk);
+ goto error_out;
+ }
+
+ ret = meson_mx_mmc_register_clks(host);
+ if (ret)
+ goto error_out;
+
+ ret = clk_prepare_enable(host->core_clk);
+ if (ret) {
+ dev_err(host->dev, "Failed to enable core clock\n");
+ goto error_out;
+ }
+
+ ret = clk_prepare_enable(host->cfg_div_clk);
+ if (ret) {
+ dev_err(host->dev, "Failed to enable MMC clock\n");
+ goto error_disable_core_clk;
+ }
+
+ conf = 0;
+ conf |= FIELD_PREP(MESON_MX_SDIO_CONF_CMD_ARGUMENT_BITS_MASK, 39);
+ conf |= FIELD_PREP(MESON_MX_SDIO_CONF_M_ENDIAN_MASK, 0x3);
+ conf |= FIELD_PREP(MESON_MX_SDIO_CONF_WRITE_NWR_MASK, 0x2);
+ conf |= FIELD_PREP(MESON_MX_SDIO_CONF_WRITE_CRC_OK_STATUS_MASK, 0x2);
+ writel(conf, host->base + MESON_MX_SDIO_CONF);
+
+ meson_mx_mmc_soft_reset(host);
+
+ ret = meson_mx_mmc_probe_slots(host);
+ if (ret)
+ goto error_disable_clks;
+
+ return 0;
+
+error_disable_clks:
+ clk_disable_unprepare(host->cfg_div_clk);
+error_disable_core_clk:
+ clk_disable_unprepare(host->core_clk);
+error_out:
+ return ret;
+}
+
+static int meson_mx_mmc_remove(struct platform_device *pdev)
+{
+ struct meson_mx_mmc_host *host = platform_get_drvdata(pdev);
+ struct meson_mx_mmc_slot *slot;
+ int i;
+
+ del_timer_sync(&host->cmd_timeout);
+
+ host->status = MESON_MX_MMC_STATUS_SHUTTING_DOWN;
+
+ for (i = 0; i < MESON_MX_SDIO_MAX_SLOTS; i++) {
+ slot = host->slots[i];
+
+ if (!slot->mmc)
+ continue;
+
+ mmc_remove_host(slot->mmc);
+ mmc_free_host(slot->mmc);
+ }
+
+ for (i = 0; i < MESON_MX_SDIO_MAX_SLOTS; i++) {
+ if (!device_is_registered(&host->slot_devices[i]))
+ continue;
+
+ device_unregister(&host->slot_devices[i]);
+ }
+
+ clk_disable_unprepare(host->cfg_div_clk);
+ clk_disable_unprepare(host->core_clk);
+
+ return 0;
+}
+
+static const struct of_device_id meson_mx_mmc_of_match[] = {
+ { .compatible = "amlogic,meson8-sdio", },
+ { .compatible = "amlogic,meson8b-sdio", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, meson_mx_mmc_of_match);
+
+static struct platform_driver meson_mx_mmc_driver = {
+ .probe = meson_mx_mmc_probe,
+ .remove = meson_mx_mmc_remove,
+ .driver = {
+ .name = "meson-mx-sdio",
+ .of_match_table = of_match_ptr(meson_mx_mmc_of_match),
+ },
+};
+
+module_platform_driver(meson_mx_mmc_driver);
+
+MODULE_DESCRIPTION("Meson6, Meson8 and Meson8b SDIO/MMC Host Driver");
+MODULE_AUTHOR("Carlo Caione <***@endlessm.com>");
+MODULE_LICENSE("GPL v2");
--
2.12.2
Martin Blumenstingl
2017-05-07 20:25:07 UTC
Permalink
On Sat, May 6, 2017 at 7:18 PM, Martin Blumenstingl
Post by Martin Blumenstingl
Add a driver for the SDIO/MMC host found on the Amlogic Meson SoCs. This
is an MMC controller which provides an interface between the application
processor and various memory cards. It supports the SD specification
v2.0 and the eMMC specification v4.41.
The controller provides an internal "mux" which allows connecting up to
three MMC devices to it. Only one device can be used at a time though
since the registers are shared across all devices. The driver takes care
of synchronizing access (similar to the dw_mmc driver).
The maximum supported bus-width is 4-bits.
Amlogic's GPL kernel sources call the corresponding driver "aml_sdio" to
differentiate it from the other MMC controller in (at least the Meson8
and Meson8b) the SoCs (they call the other drivers aml_sdhc and
aml_sdhc_m8, which seem to support a bus-width of up to 8-bits).
---
drivers/mmc/host/Kconfig | 12 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/meson-mx-sdio.c | 978 +++++++++++++++++++++++++++++++++++++++
3 files changed, 991 insertions(+)
create mode 100644 drivers/mmc/host/meson-mx-sdio.c
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index a638cd0d80be..c557482ae327 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -343,6 +343,18 @@ config MMC_MESON_GX
If you have a controller with this interface, say Y here.
+config MMC_MESON_MX_SDIO
+ tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller support"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on HAS_DMA
+ depends on OF
just got an email from the kbuild test robot reminding me that I
missed a COMMON_CLK dependency here. not a big deal - I'll fix this in
the next version
Post by Martin Blumenstingl
+ help
+ This selects support for the SD/MMC Host Controller on
+ Amlogic Meson6, Meson8 and Meson8b SoCs.
+
+ If you have a controller with this interface, say Y or M here.
+ If unsure, say N.
+
config MMC_MOXART
tristate "MOXART SD/MMC Host Controller support"
depends on ARCH_MOXART && MMC
[snip]
Ulf Hansson
2017-05-29 14:38:36 UTC
Permalink
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
Add a driver for the SDIO/MMC host found on the Amlogic Meson SoCs. This
is an MMC controller which provides an interface between the application
processor and various memory cards. It supports the SD specification
v2.0 and the eMMC specification v4.41.
The controller provides an internal "mux" which allows connecting up to
three MMC devices to it. Only one device can be used at a time though
since the registers are shared across all devices. The driver takes care
of synchronizing access (similar to the dw_mmc driver).
The maximum supported bus-width is 4-bits.
Amlogic's GPL kernel sources call the corresponding driver "aml_sdio" to
differentiate it from the other MMC controller in (at least the Meson8
and Meson8b) the SoCs (they call the other drivers aml_sdhc and
aml_sdhc_m8, which seem to support a bus-width of up to 8-bits).
Would you mind to extend this change log to include some more of the
information about which SoC this is being used on, according to our
recent discussions. Just wanted to make sure we don't mix it up with
any other meson mmc controller/driver.
Post by Martin Blumenstingl
---
drivers/mmc/host/Kconfig | 12 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/meson-mx-sdio.c | 978 +++++++++++++++++++++++++++++++++++++++
3 files changed, 991 insertions(+)
create mode 100644 drivers/mmc/host/meson-mx-sdio.c
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index a638cd0d80be..c557482ae327 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -343,6 +343,18 @@ config MMC_MESON_GX
If you have a controller with this interface, say Y here.
+config MMC_MESON_MX_SDIO
+ tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller support"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on HAS_DMA
+ depends on OF
+ help
+ This selects support for the SD/MMC Host Controller on
+ Amlogic Meson6, Meson8 and Meson8b SoCs.
+
+ If you have a controller with this interface, say Y or M here.
+ If unsure, say N.
+
config MMC_MOXART
tristate "MOXART SD/MMC Host Controller support"
depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index bc2c2e2c68c0..f9500ab2bc86 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_VUB300) += vub300.o
obj-$(CONFIG_MMC_USHC) += ushc.o
obj-$(CONFIG_MMC_WMT) += wmt-sdmmc.o
obj-$(CONFIG_MMC_MESON_GX) += meson-gx-mmc.o
+obj-$(CONFIG_MMC_MESON_MX_SDIO) += meson-mx-sdio.o
obj-$(CONFIG_MMC_MOXART) += moxart-mmc.o
obj-$(CONFIG_MMC_SUNXI) += sunxi-mmc.o
obj-$(CONFIG_MMC_USDHI6ROL0) += usdhi6rol0.o
diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c
new file mode 100644
index 000000000000..2da9c5249ae7
--- /dev/null
+++ b/drivers/mmc/host/meson-mx-sdio.c
@@ -0,0 +1,978 @@
+/*
+ * meson-mx-sdio.c - Meson6, Meson8 and Meson8b SDIO/MMC Host Controller
+ *
+ * Copyright (C) 2015 Endless Mobile, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/timer.h>
+#include <linux/types.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/slot-gpio.h>
+
+#define MESON_MX_SDIO_ARGU 0x00
+
+#define MESON_MX_SDIO_SEND 0x04
+ #define MESON_MX_SDIO_SEND_COMMAND_INDEX_MASK GENMASK(7, 0)
+ #define MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK GENMASK(15, 8)
+ #define MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7 BIT(16)
+ #define MESON_MX_SDIO_SEND_RESP_HAS_DATA BIT(17)
+ #define MESON_MX_SDIO_SEND_RESP_CRC7_FROM_8 BIT(18)
+ #define MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY BIT(19)
+ #define MESON_MX_SDIO_SEND_DATA BIT(20)
+ #define MESON_MX_SDIO_SEND_USE_INT_WINDOW BIT(21)
+ #define MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK GENMASK(31, 24)
+
+#define MESON_MX_SDIO_CONF 0x08
+ #define MESON_MX_SDIO_CONF_CMD_CLK_DIV_SHIFT 0
+ #define MESON_MX_SDIO_CONF_CMD_CLK_DIV_WIDTH 10
+ #define MESON_MX_SDIO_CONF_CMD_DISABLE_CRC BIT(10)
+ #define MESON_MX_SDIO_CONF_CMD_OUT_AT_POSITIVE_EDGE BIT(11)
+ #define MESON_MX_SDIO_CONF_CMD_ARGUMENT_BITS_MASK GENMASK(17, 12)
+ #define MESON_MX_SDIO_CONF_RESP_LATCH_AT_NEGATIVE_EDGE BIT(18)
+ #define MESON_MX_SDIO_CONF_DATA_LATCH_AT_NEGATIVE_EDGE BIT(19)
+ #define MESON_MX_SDIO_CONF_BUS_WIDTH BIT(20)
+ #define MESON_MX_SDIO_CONF_M_ENDIAN_MASK GENMASK(22, 21)
+ #define MESON_MX_SDIO_CONF_WRITE_NWR_MASK GENMASK(28, 23)
+ #define MESON_MX_SDIO_CONF_WRITE_CRC_OK_STATUS_MASK GENMASK(31, 29)
+
+#define MESON_MX_SDIO_IRQS 0x0c
+ #define MESON_MX_SDIO_IRQS_STATUS_STATE_MACHINE_MASK GENMASK(3, 0)
+ #define MESON_MX_SDIO_IRQS_CMD_BUSY BIT(4)
+ #define MESON_MX_SDIO_IRQS_RESP_CRC7_OK BIT(5)
+ #define MESON_MX_SDIO_IRQS_DATA_READ_CRC16_OK BIT(6)
+ #define MESON_MX_SDIO_IRQS_DATA_WRITE_CRC16_OK BIT(7)
+ #define MESON_MX_SDIO_IRQS_IF_INT BIT(8)
+ #define MESON_MX_SDIO_IRQS_CMD_INT BIT(9)
+ #define MESON_MX_SDIO_IRQS_STATUS_INFO_MASK GENMASK(15, 12)
+ #define MESON_MX_SDIO_IRQS_TIMING_OUT_INT BIT(16)
+ #define MESON_MX_SDIO_IRQS_AMRISC_TIMING_OUT_INT_EN BIT(17)
+ #define MESON_MX_SDIO_IRQS_ARC_TIMING_OUT_INT_EN BIT(18)
+ #define MESON_MX_SDIO_IRQS_TIMING_OUT_COUNT_MASK GENMASK(31, 19)
+
+#define MESON_MX_SDIO_IRQC 0x10
+ #define MESON_MX_SDIO_IRQC_ARC_IF_INT_EN BIT(3)
+ #define MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN BIT(4)
+ #define MESON_MX_SDIO_IRQC_IF_CONFIG_MASK GENMASK(7, 6)
+ #define MESON_MX_SDIO_IRQC_SOFT_RESET BIT(15)
+ #define MESON_MX_SDIO_IRQC_FORCE_HALT BIT(30)
+ #define MESON_MX_SDIO_IRQC_HALT_HOLE BIT(31)
+
+#define MESON_MX_SDIO_MULT 0x14
+ #define MESON_MX_SDIO_MULT_PORT_SEL_MASK GENMASK(1, 0)
+ #define MESON_MX_SDIO_MULT_MEMORY_STICK_ENABLE BIT(2)
+ #define MESON_MX_SDIO_MULT_MEMORY_STICK_SCLK_ALWAYS BIT(3)
+ #define MESON_MX_SDIO_MULT_STREAM_ENABLE BIT(4)
+ #define MESON_MX_SDIO_MULT_STREAM_8BITS_MODE BIT(5)
+ #define MESON_MX_SDIO_MULT_WR_RD_OUT_INDEX BIT(8)
+ #define MESON_MX_SDIO_MULT_DAT0_DAT1_SWAPPED BIT(10)
+ #define MESON_MX_SDIO_MULT_DAT1_DAT0_SWAPPED BIT(11)
+ #define MESON_MX_SDIO_MULT_RESP_READ_INDEX_MASK GENMASK(15, 12)
+
+#define MESON_MX_SDIO_ADDR 0x18
+
+#define MESON_MX_SDIO_EXT 0x1c
+ #define MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK GENMASK(29, 16)
+
+#define MESON_MX_SDIO_BOUNCE_REQ_SIZE (128 * 1024)
+#define MESON_MX_SDIO_RESPONSE_CRC16_BITS (16 - 1)
+#define MESON_MX_SDIO_MAX_SLOTS 3
+
+enum meson_mx_mmc_host_status {
+ MESON_MX_MMC_STATUS_IDLE,
+ MESON_MX_MMC_STATUS_BUSY,
+ MESON_MX_MMC_STATUS_SHUTTING_DOWN,
+};
+
+struct meson_mx_mmc_slot {
+ struct mmc_host *mmc;
+ struct meson_mx_mmc_host *host;
+
+ struct mmc_request *mrq;
+ struct mmc_command *cmd;
+ int error;
+
+ unsigned int id;
+ struct list_head queue_node;
+};
+
+struct meson_mx_mmc_host {
+ struct device *dev;
+
+ struct clk *parent_clk;
+ struct clk *core_clk;
+ struct clk_divider cfg_div;
+ struct clk *cfg_div_clk;
+ struct clk_fixed_factor fixed_factor;
+ struct clk *fixed_factor_clk;
+
+ void __iomem *base;
+ int irq;
+ spinlock_t lock;
+ spinlock_t irq_lock;
Could you please explain what these locks are being used for?
Post by Martin Blumenstingl
+
+ enum meson_mx_mmc_host_status status;
+ struct list_head queue;
+ struct timer_list cmd_timeout;
+
+ struct device slot_devices[MESON_MX_SDIO_MAX_SLOTS];
Please convert to use an array of pointers instead.

The cavium mmc driver uses of_platform_device_create|destroy(), seem
like that should work in this case as well.
Post by Martin Blumenstingl
+ struct meson_mx_mmc_slot *slots[MESON_MX_SDIO_MAX_SLOTS];
+ struct meson_mx_mmc_slot *current_cmd_slot;
+ struct meson_mx_mmc_slot *sdio_irq_slot;
+};
+
+static u32 meson_mx_mmc_readl(struct mmc_host *mmc, char reg)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ return readl(slot->host->base + reg);
+}
+
+static void meson_mx_mmc_writel(struct mmc_host *mmc, u32 val, char reg)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ writel(val, slot->host->base + reg);
+}
Please remove these two wrapper functions, as those just make the code
less readable.

[...]
Post by Martin Blumenstingl
+static void meson_mx_mmc_apply_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long clk_rate = ios->clock;
+ int ret;
+
+ switch (ios->bus_width) {
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH, 0);
+ break;
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH,
+ MESON_MX_SDIO_CONF_BUS_WIDTH);
+ break;
+
+ dev_err(mmc_dev(mmc), "unsupported bus width: %d\n",
+ ios->bus_width);
+ slot->error = -EINVAL;
+ return;
+ }
+
+ if (clk_rate) {
+ if (WARN_ON(clk_rate > mmc->f_max))
+ clk_rate = mmc->f_max;
+ else if (WARN_ON(clk_rate < mmc->f_min))
+ clk_rate = mmc->f_min;
+
+ ret = clk_set_rate(slot->host->cfg_div_clk, ios->clock);
+ if (ret) {
+ dev_warn(mmc_dev(mmc),
+ "failed to set MMC clock to %lu: %d\n",
+ clk_rate, ret);
+ slot->error = ret;
+ return;
+ }
+
+ mmc->actual_clock = clk_get_rate(slot->host->cfg_div_clk);
+ }
In some cases the mmc core request the clock rate to be zero (to gate
the clock) which is needed to for example switch to UHS speed mode. If
you intend to support that, you need to manage this at this point.
Post by Martin Blumenstingl
+}
+
+static void meson_mx_mmc_start_cmd(struct mmc_host *mmc,
+ struct mmc_command *cmd)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned int pack_size;
+ unsigned long irqflags, timeout;
+ u32 mult, send = 0, ext = 0;
+
+ slot->cmd = cmd;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ switch (mmc_resp_type(cmd)) {
+ /* 7 (CMD) + 32 (response) + 7 (CRC) -1 */
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK, 45);
+ break;
+ /* 7 (CMD) + 120 (response) + 7 (CRC) -1 */
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK, 133);
+ send |= MESON_MX_SDIO_SEND_RESP_CRC7_FROM_8;
+ break;
+ break;
+ }
+
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
Post by Martin Blumenstingl
+
+ if (cmd->data) {
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
+ (cmd->data->blocks - 1));
+
+ pack_size = cmd->data->blksz * BITS_PER_BYTE;
+ if (mmc->ios.bus_width == MMC_BUS_WIDTH_4)
+ pack_size += MESON_MX_SDIO_RESPONSE_CRC16_BITS * 4;
+ else
+ pack_size += MESON_MX_SDIO_RESPONSE_CRC16_BITS * 1;
+
+ ext |= FIELD_PREP(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
+ pack_size);
+
+ if (cmd->data->flags & MMC_DATA_WRITE)
+ send |= MESON_MX_SDIO_SEND_DATA;
+ else
+ send |= MESON_MX_SDIO_SEND_RESP_HAS_DATA;
+
+ cmd->data->bytes_xfered = 0;
+ }
+
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_COMMAND_INDEX_MASK,
+ (0x40 | cmd->opcode));
+
+ /*
+ * soft-reset the MMC core and re-apply the IOS to make sure that these
+ * are correct for the slot which is selected below (as the IOS
+ * registers are shared across all slots).
+ */
+ meson_mx_mmc_soft_reset(slot->host);
+ meson_mx_mmc_apply_ios(mmc, &mmc->ios);
The above function calls clk_set_rate(), which triggers the common
clock framework to take the mutex.

Doing that while disabled irqs via spin_lock_irqsave() earlier above,
seems like a bad idea.
Post by Martin Blumenstingl
+
+ mult = meson_mx_mmc_readl(mmc, MESON_MX_SDIO_MULT);
+ mult &= ~MESON_MX_SDIO_MULT_PORT_SEL_MASK;
+ mult |= FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK, slot->id);
+ mult |= BIT(31);
+ meson_mx_mmc_writel(mmc, mult, MESON_MX_SDIO_MULT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN,
+ MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN);
+
+ /* clear pending interrupts */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_CMD_INT,
+ MESON_MX_SDIO_IRQS_CMD_INT);
+
+ meson_mx_mmc_writel(mmc, cmd->arg, MESON_MX_SDIO_ARGU);
+ meson_mx_mmc_writel(mmc, ext, MESON_MX_SDIO_EXT);
+ meson_mx_mmc_writel(mmc, send, MESON_MX_SDIO_SEND);
+
+ spin_unlock_irqrestore(&slot->host->irq_lock, irqflags);
+
+ if (cmd->opcode == MMC_ERASE)
+ timeout = msecs_to_jiffies(30000);
+ else if (cmd->data)
+ timeout = msecs_to_jiffies(5000);
+ else
+ timeout = msecs_to_jiffies(1000);
Please don't hard-code these timeouts. Instead make use of the
cmd.busy_timeout, as that should contain a proper timeout value.
Post by Martin Blumenstingl
+
+ mod_timer(&slot->host->cmd_timeout, jiffies + timeout);
+}
+
[...]
Post by Martin Blumenstingl
+static void meson_mx_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ if (spin_trylock(&slot->host->lock)) {
+ /*
+ * only apply the mmc_ios if we are idle to not break any
+ * ongoing transfer. in case we are busy meson_mx_mmc_start_cmd
+ * will take care of applying the mmc_ios later on.
+ */
+ if (slot->host->status == MESON_MX_MMC_STATUS_IDLE)
+ meson_mx_mmc_apply_ios(mmc, ios);
No this doesn't work!

In case the status != MESON_MX_MMC_STATUS_IDLE or if the attempt to
take the lock fails, you will just silently ignore to set the new ios
settings.

The mmc core implements the mmc/sd/sdio specifications, so when you
return from the ->set_ios() host ops, the mmc core relies on the host
to have applied the settings to conform the the specs. You can not
delay that to a later point.
Post by Martin Blumenstingl
+
+ spin_unlock(&slot->host->lock);
+ }
+
+ switch (ios->power_mode) {
+ if (!IS_ERR(mmc->supply.vmmc))
+ mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
+ break;
+
+ if (!IS_ERR(mmc->supply.vmmc))
+ mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
+ break;
+ }
+}
+
+static void meson_mx_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_MULT,
+ MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ slot->id));
+
+ /* ACK pending interrupt */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_IF_INT,
+ MESON_MX_SDIO_IRQS_IF_INT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_IF_INT_EN,
+ enable ? MESON_MX_SDIO_IRQC_ARC_IF_INT_EN : 0);
+
+ if (enable)
+ slot->host->sdio_irq_slot = slot;
+ else
+ slot->host->sdio_irq_slot = NULL;
This looks weird. You support up to three slots per host, but only one
can do sdio_irq?

BTW, what happens if there are is a ongoing data transfer on an SD
card slot, while there is an SDIO irq raised on the SDIO card slot? Do
you cope with this correctly?
Post by Martin Blumenstingl
+
+ spin_unlock_irqrestore(&slot->host->irq_lock, irqflags);
+}
+
[...]
Post by Martin Blumenstingl
+
+static void meson_mx_mmc_process_sdio_irq(struct meson_mx_mmc_slot *slot)
+{
+ /*
+ * ignore SDIO interrupts without corresponding slot as the SDIO
+ * interrupt seems to enable itself automatically - in this case we
+ * didn't assign a slot for this IRQ so we simply ignore it.
+ */
+ if (!slot)
+ return;
+
+ mmc_signal_sdio_irq(slot->mmc);
Please use the new sdio_signal_irq() API and method instead. This also
means you should implement the ->ack_sdio_irq() host ops.

[...]
Post by Martin Blumenstingl
+
+static int meson_mx_mmc_register_slot_device(struct device_node *np,
+ unsigned int id,
+ struct meson_mx_mmc_host *host)
+{
+ struct device *dev = &host->slot_devices[id];
+
+ dev->parent = host->dev;
+ dev->of_node = np;
+ dev->release = meson_mx_mmc_slot_device_release;
+ dev_set_name(dev, "%s.%d", dev_name(dev->parent), id);
+
+ return device_register(dev);
As stated, please try to use of_platform_device_create() instead.
Post by Martin Blumenstingl
+}
+
+static int meson_mx_mmc_add_slot(struct device_node *np,
+ struct meson_mx_mmc_host *host)
+{
+ struct meson_mx_mmc_slot *slot;
+ struct mmc_host *mmc;
+ unsigned int id;
+ int ret;
+
+ if (of_property_read_u32(np, "reg", &id)) {
+ dev_err(host->dev, "missing 'reg' property for %s\n",
+ of_node_full_name(np));
+ return -EINVAL;
+ }
+
+ if (id >= MESON_MX_SDIO_MAX_SLOTS) {
+ dev_err(host->dev,
+ "invalid 'reg' property value of %s\n",
+ of_node_full_name(np));
+ return -EINVAL;
+ }
+
+ ret = meson_mx_mmc_register_slot_device(np, id, host);
+ if (ret)
+ return ret;
+
+ mmc = mmc_alloc_host(sizeof(*slot), &host->slot_devices[id]);
+ if (!mmc) {
+ ret = -ENOMEM;
+ goto error_unregister_dev;
+ }
+
+ slot = mmc_priv(mmc);
+ slot->mmc = mmc;
+ slot->id = id;
+ slot->host = host;
+
+ host->slots[id] = slot;
+
+ /* Get regulators and the supported OCR mask */
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto error_free_host;
+
+ mmc->max_req_size = MESON_MX_SDIO_BOUNCE_REQ_SIZE;
+ mmc->max_seg_size = mmc->max_req_size;
+ mmc->max_blk_count =
+ FIELD_GET(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
+ 0xffffffff);
+ mmc->max_blk_size = FIELD_GET(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
+ 0xffffffff);
+ mmc->max_blk_size -= (4 * MESON_MX_SDIO_RESPONSE_CRC16_BITS);
+ mmc->max_blk_size /= BITS_PER_BYTE;
+
+ /* Get the min and max supported clock rates */
+ mmc->f_min = clk_round_rate(host->cfg_div_clk, 1);
+ mmc->f_max = clk_round_rate(host->cfg_div_clk,
+ clk_get_rate(host->parent_clk));
+
+ mmc->caps |= MMC_CAP_CMD23;
+ mmc->ops = &meson_mx_mmc_ops;
+
+ ret = mmc_of_parse(mmc);
+ if (ret)
+ goto error_free_host;
+
+ ret = mmc_add_host(mmc);
+ if (ret)
+ goto error_free_host;
+
+ return 0;
+
+ mmc_free_host(mmc);
+ device_unregister(&host->slot_devices[id]);
+ return ret;
+}
+
+static int meson_mx_mmc_probe_slots(struct meson_mx_mmc_host *host)
+{
+ struct device_node *slot_node, *controller_node;
+ int num_slots, ret;
+
+ controller_node = host->dev->of_node;
+
+ num_slots = of_get_available_child_count(controller_node);
According to the comment I had on the DT doc change in patch1, this
doesn't work as we are using child nodes to describe an embedded card
(embedded SDIO/eMMC) and SDIO func devices.

You need to also search for a compatible string, "mmc-slot".
Post by Martin Blumenstingl
+ if (num_slots > MESON_MX_SDIO_MAX_SLOTS) {
+ dev_err(host->dev, "more slots configured than supported\n");
+ return -EINVAL;
+ }
+
+ for_each_available_child_of_node(controller_node, slot_node) {
+ ret = meson_mx_mmc_add_slot(slot_node, host);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static int meson_mx_mmc_register_clks(struct meson_mx_mmc_host *host)
+{
+ struct clk_init_data init;
+ const char *clk_div_parents[1], *clk_fixed_factor_parents[1];
It seems silly to use and array of size 1. Below should work as well, right!?

const char *clk_div_parents, *clk_fixed_factor_parents;
Post by Martin Blumenstingl
+
+ clk_fixed_factor_parents[0] = __clk_get_name(host->parent_clk);
+ init.name = devm_kasprintf(host->dev, GFP_KERNEL, "%s#fixed_factor",
+ dev_name(host->dev));
+ init.ops = &clk_fixed_factor_ops;
+ init.flags = 0;
+ init.parent_names = clk_fixed_factor_parents;
+ init.num_parents = 1;
+ host->fixed_factor.div = 2;
+ host->fixed_factor.mult = 1;
+ host->fixed_factor.hw.init = &init;
+
+ host->fixed_factor_clk = devm_clk_register(host->dev,
+ &host->fixed_factor.hw);
+ if (WARN_ON(PTR_ERR_OR_ZERO(host->fixed_factor_clk)))
+ return PTR_ERR(host->fixed_factor_clk);
+
+ clk_div_parents[0] = __clk_get_name(host->fixed_factor_clk);
+ init.name = devm_kasprintf(host->dev, GFP_KERNEL, "%s#div",
+ dev_name(host->dev));
+ init.ops = &clk_divider_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.parent_names = clk_div_parents;
+ init.num_parents = 1;
+ host->cfg_div.reg = host->base + MESON_MX_SDIO_CONF;
+ host->cfg_div.shift = MESON_MX_SDIO_CONF_CMD_CLK_DIV_SHIFT;
+ host->cfg_div.width = MESON_MX_SDIO_CONF_CMD_CLK_DIV_WIDTH;
+ host->cfg_div.hw.init = &init;
+ host->cfg_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+
+ host->cfg_div_clk = devm_clk_register(host->dev, &host->cfg_div.hw);
+ if (WARN_ON(PTR_ERR_OR_ZERO(host->cfg_div_clk)))
+ return PTR_ERR(host->fixed_factor_clk);
+
+ return 0;
+}
+
[...]

Another overall comment, which relates to the host locking mechanism
and the problem with ->set_ios(). Perhaps you can look into how the
cavium mmc driver has solved the similar problems as it also manages
several slots per host.

Kind regards
Uffe
Martin Blumenstingl
2017-06-03 16:37:26 UTC
Permalink
Hi Ulf,

first of all: many thanks for the well-written comments - they help me
a lot understanding *why* some parts of the code are wrong!
Post by Ulf Hansson
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
Add a driver for the SDIO/MMC host found on the Amlogic Meson SoCs. This
is an MMC controller which provides an interface between the application
processor and various memory cards. It supports the SD specification
v2.0 and the eMMC specification v4.41.
The controller provides an internal "mux" which allows connecting up to
three MMC devices to it. Only one device can be used at a time though
since the registers are shared across all devices. The driver takes care
of synchronizing access (similar to the dw_mmc driver).
The maximum supported bus-width is 4-bits.
Amlogic's GPL kernel sources call the corresponding driver "aml_sdio" to
differentiate it from the other MMC controller in (at least the Meson8
and Meson8b) the SoCs (they call the other drivers aml_sdhc and
aml_sdhc_m8, which seem to support a bus-width of up to 8-bits).
Would you mind to extend this change log to include some more of the
information about which SoC this is being used on, according to our
recent discussions. Just wanted to make sure we don't mix it up with
any other meson mmc controller/driver.
noted - will do this
Post by Ulf Hansson
Post by Martin Blumenstingl
---
drivers/mmc/host/Kconfig | 12 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/meson-mx-sdio.c | 978 +++++++++++++++++++++++++++++++++++++++
3 files changed, 991 insertions(+)
create mode 100644 drivers/mmc/host/meson-mx-sdio.c
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index a638cd0d80be..c557482ae327 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -343,6 +343,18 @@ config MMC_MESON_GX
If you have a controller with this interface, say Y here.
+config MMC_MESON_MX_SDIO
+ tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller support"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on HAS_DMA
+ depends on OF
+ help
+ This selects support for the SD/MMC Host Controller on
+ Amlogic Meson6, Meson8 and Meson8b SoCs.
+
+ If you have a controller with this interface, say Y or M here.
+ If unsure, say N.
+
config MMC_MOXART
tristate "MOXART SD/MMC Host Controller support"
depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index bc2c2e2c68c0..f9500ab2bc86 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_VUB300) += vub300.o
obj-$(CONFIG_MMC_USHC) += ushc.o
obj-$(CONFIG_MMC_WMT) += wmt-sdmmc.o
obj-$(CONFIG_MMC_MESON_GX) += meson-gx-mmc.o
+obj-$(CONFIG_MMC_MESON_MX_SDIO) += meson-mx-sdio.o
obj-$(CONFIG_MMC_MOXART) += moxart-mmc.o
obj-$(CONFIG_MMC_SUNXI) += sunxi-mmc.o
obj-$(CONFIG_MMC_USDHI6ROL0) += usdhi6rol0.o
diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c
new file mode 100644
index 000000000000..2da9c5249ae7
--- /dev/null
+++ b/drivers/mmc/host/meson-mx-sdio.c
@@ -0,0 +1,978 @@
+/*
+ * meson-mx-sdio.c - Meson6, Meson8 and Meson8b SDIO/MMC Host Controller
+ *
+ * Copyright (C) 2015 Endless Mobile, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/timer.h>
+#include <linux/types.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/slot-gpio.h>
+
+#define MESON_MX_SDIO_ARGU 0x00
+
+#define MESON_MX_SDIO_SEND 0x04
+ #define MESON_MX_SDIO_SEND_COMMAND_INDEX_MASK GENMASK(7, 0)
+ #define MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK GENMASK(15, 8)
+ #define MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7 BIT(16)
+ #define MESON_MX_SDIO_SEND_RESP_HAS_DATA BIT(17)
+ #define MESON_MX_SDIO_SEND_RESP_CRC7_FROM_8 BIT(18)
+ #define MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY BIT(19)
+ #define MESON_MX_SDIO_SEND_DATA BIT(20)
+ #define MESON_MX_SDIO_SEND_USE_INT_WINDOW BIT(21)
+ #define MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK GENMASK(31, 24)
+
+#define MESON_MX_SDIO_CONF 0x08
+ #define MESON_MX_SDIO_CONF_CMD_CLK_DIV_SHIFT 0
+ #define MESON_MX_SDIO_CONF_CMD_CLK_DIV_WIDTH 10
+ #define MESON_MX_SDIO_CONF_CMD_DISABLE_CRC BIT(10)
+ #define MESON_MX_SDIO_CONF_CMD_OUT_AT_POSITIVE_EDGE BIT(11)
+ #define MESON_MX_SDIO_CONF_CMD_ARGUMENT_BITS_MASK GENMASK(17, 12)
+ #define MESON_MX_SDIO_CONF_RESP_LATCH_AT_NEGATIVE_EDGE BIT(18)
+ #define MESON_MX_SDIO_CONF_DATA_LATCH_AT_NEGATIVE_EDGE BIT(19)
+ #define MESON_MX_SDIO_CONF_BUS_WIDTH BIT(20)
+ #define MESON_MX_SDIO_CONF_M_ENDIAN_MASK GENMASK(22, 21)
+ #define MESON_MX_SDIO_CONF_WRITE_NWR_MASK GENMASK(28, 23)
+ #define MESON_MX_SDIO_CONF_WRITE_CRC_OK_STATUS_MASK GENMASK(31, 29)
+
+#define MESON_MX_SDIO_IRQS 0x0c
+ #define MESON_MX_SDIO_IRQS_STATUS_STATE_MACHINE_MASK GENMASK(3, 0)
+ #define MESON_MX_SDIO_IRQS_CMD_BUSY BIT(4)
+ #define MESON_MX_SDIO_IRQS_RESP_CRC7_OK BIT(5)
+ #define MESON_MX_SDIO_IRQS_DATA_READ_CRC16_OK BIT(6)
+ #define MESON_MX_SDIO_IRQS_DATA_WRITE_CRC16_OK BIT(7)
+ #define MESON_MX_SDIO_IRQS_IF_INT BIT(8)
+ #define MESON_MX_SDIO_IRQS_CMD_INT BIT(9)
+ #define MESON_MX_SDIO_IRQS_STATUS_INFO_MASK GENMASK(15, 12)
+ #define MESON_MX_SDIO_IRQS_TIMING_OUT_INT BIT(16)
+ #define MESON_MX_SDIO_IRQS_AMRISC_TIMING_OUT_INT_EN BIT(17)
+ #define MESON_MX_SDIO_IRQS_ARC_TIMING_OUT_INT_EN BIT(18)
+ #define MESON_MX_SDIO_IRQS_TIMING_OUT_COUNT_MASK GENMASK(31, 19)
+
+#define MESON_MX_SDIO_IRQC 0x10
+ #define MESON_MX_SDIO_IRQC_ARC_IF_INT_EN BIT(3)
+ #define MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN BIT(4)
+ #define MESON_MX_SDIO_IRQC_IF_CONFIG_MASK GENMASK(7, 6)
+ #define MESON_MX_SDIO_IRQC_SOFT_RESET BIT(15)
+ #define MESON_MX_SDIO_IRQC_FORCE_HALT BIT(30)
+ #define MESON_MX_SDIO_IRQC_HALT_HOLE BIT(31)
+
+#define MESON_MX_SDIO_MULT 0x14
+ #define MESON_MX_SDIO_MULT_PORT_SEL_MASK GENMASK(1, 0)
+ #define MESON_MX_SDIO_MULT_MEMORY_STICK_ENABLE BIT(2)
+ #define MESON_MX_SDIO_MULT_MEMORY_STICK_SCLK_ALWAYS BIT(3)
+ #define MESON_MX_SDIO_MULT_STREAM_ENABLE BIT(4)
+ #define MESON_MX_SDIO_MULT_STREAM_8BITS_MODE BIT(5)
+ #define MESON_MX_SDIO_MULT_WR_RD_OUT_INDEX BIT(8)
+ #define MESON_MX_SDIO_MULT_DAT0_DAT1_SWAPPED BIT(10)
+ #define MESON_MX_SDIO_MULT_DAT1_DAT0_SWAPPED BIT(11)
+ #define MESON_MX_SDIO_MULT_RESP_READ_INDEX_MASK GENMASK(15, 12)
+
+#define MESON_MX_SDIO_ADDR 0x18
+
+#define MESON_MX_SDIO_EXT 0x1c
+ #define MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK GENMASK(29, 16)
+
+#define MESON_MX_SDIO_BOUNCE_REQ_SIZE (128 * 1024)
+#define MESON_MX_SDIO_RESPONSE_CRC16_BITS (16 - 1)
+#define MESON_MX_SDIO_MAX_SLOTS 3
+
+enum meson_mx_mmc_host_status {
+ MESON_MX_MMC_STATUS_IDLE,
+ MESON_MX_MMC_STATUS_BUSY,
+ MESON_MX_MMC_STATUS_SHUTTING_DOWN,
+};
+
+struct meson_mx_mmc_slot {
+ struct mmc_host *mmc;
+ struct meson_mx_mmc_host *host;
+
+ struct mmc_request *mrq;
+ struct mmc_command *cmd;
+ int error;
+
+ unsigned int id;
+ struct list_head queue_node;
+};
+
+struct meson_mx_mmc_host {
+ struct device *dev;
+
+ struct clk *parent_clk;
+ struct clk *core_clk;
+ struct clk_divider cfg_div;
+ struct clk *cfg_div_clk;
+ struct clk_fixed_factor fixed_factor;
+ struct clk *fixed_factor_clk;
+
+ void __iomem *base;
+ int irq;
+ spinlock_t lock;
+ spinlock_t irq_lock;
Could you please explain what these locks are being used for?
Post by Martin Blumenstingl
+
+ enum meson_mx_mmc_host_status status;
+ struct list_head queue;
+ struct timer_list cmd_timeout;
+
+ struct device slot_devices[MESON_MX_SDIO_MAX_SLOTS];
Please convert to use an array of pointers instead.
I'll simply add a device pointer to meson_mx_mmc_slot so I don't have
to manage two arrays
Post by Ulf Hansson
The cavium mmc driver uses of_platform_device_create|destroy(), seem
like that should work in this case as well.
indeed, this makes things much easier - thanks for the hint (I didn't
know about of_platform_device_{create,destroy} yet)!
Post by Ulf Hansson
Post by Martin Blumenstingl
+ struct meson_mx_mmc_slot *slots[MESON_MX_SDIO_MAX_SLOTS];
+ struct meson_mx_mmc_slot *current_cmd_slot;
+ struct meson_mx_mmc_slot *sdio_irq_slot;
+};
+
+static u32 meson_mx_mmc_readl(struct mmc_host *mmc, char reg)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ return readl(slot->host->base + reg);
+}
+
+static void meson_mx_mmc_writel(struct mmc_host *mmc, u32 val, char reg)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ writel(val, slot->host->base + reg);
+}
Please remove these two wrapper functions, as those just make the code
less readable.
OK, these will be gone in RFCv2
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
+static void meson_mx_mmc_apply_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long clk_rate = ios->clock;
+ int ret;
+
+ switch (ios->bus_width) {
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH, 0);
+ break;
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH,
+ MESON_MX_SDIO_CONF_BUS_WIDTH);
+ break;
+
+ dev_err(mmc_dev(mmc), "unsupported bus width: %d\n",
+ ios->bus_width);
+ slot->error = -EINVAL;
+ return;
+ }
+
+ if (clk_rate) {
+ if (WARN_ON(clk_rate > mmc->f_max))
+ clk_rate = mmc->f_max;
+ else if (WARN_ON(clk_rate < mmc->f_min))
+ clk_rate = mmc->f_min;
+
+ ret = clk_set_rate(slot->host->cfg_div_clk, ios->clock);
+ if (ret) {
+ dev_warn(mmc_dev(mmc),
+ "failed to set MMC clock to %lu: %d\n",
+ clk_rate, ret);
+ slot->error = ret;
+ return;
+ }
+
+ mmc->actual_clock = clk_get_rate(slot->host->cfg_div_clk);
+ }
In some cases the mmc core request the clock rate to be zero (to gate
the clock) which is needed to for example switch to UHS speed mode. If
you intend to support that, you need to manage this at this point.
thanks for the explanation - unfortunately the lack of a datasheet
which properly describes the clocks makes this a bit hard to
implement.
the vendor driver simply ignores any set_ios request with clock = 0
it seems that there is also no dedicated "stop clock" bit (and we only
have a clock divider). all I could find is FORCE_HALT (bit 30 in the
IRQC register), where the vendor driver explains: "Force halt SDIO by
software. Halt in this sdio host controller means stop to transmit or
receive data from sd card. and then sd card clock will be shutdown.
Software can force to halt anytime, and hardware will automatically
halt the sdio when reading fifo is full or writing fifo is empty"

should I simply remove that if (...) and try to assign 0 to the clock anyways?
Post by Ulf Hansson
Post by Martin Blumenstingl
+}
+
+static void meson_mx_mmc_start_cmd(struct mmc_host *mmc,
+ struct mmc_command *cmd)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned int pack_size;
+ unsigned long irqflags, timeout;
+ u32 mult, send = 0, ext = 0;
+
+ slot->cmd = cmd;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ switch (mmc_resp_type(cmd)) {
+ /* 7 (CMD) + 32 (response) + 7 (CRC) -1 */
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK, 45);
+ break;
+ /* 7 (CMD) + 120 (response) + 7 (CRC) -1 */
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_CMD_RESP_BITS_MASK, 133);
+ send |= MESON_MX_SDIO_SEND_RESP_CRC7_FROM_8;
+ break;
+ break;
+ }
+
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Post by Ulf Hansson
Post by Martin Blumenstingl
+
+ if (cmd->data) {
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
+ (cmd->data->blocks - 1));
+
+ pack_size = cmd->data->blksz * BITS_PER_BYTE;
+ if (mmc->ios.bus_width == MMC_BUS_WIDTH_4)
+ pack_size += MESON_MX_SDIO_RESPONSE_CRC16_BITS * 4;
+ else
+ pack_size += MESON_MX_SDIO_RESPONSE_CRC16_BITS * 1;
+
+ ext |= FIELD_PREP(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
+ pack_size);
+
+ if (cmd->data->flags & MMC_DATA_WRITE)
+ send |= MESON_MX_SDIO_SEND_DATA;
+ else
+ send |= MESON_MX_SDIO_SEND_RESP_HAS_DATA;
+
+ cmd->data->bytes_xfered = 0;
+ }
+
+ send |= FIELD_PREP(MESON_MX_SDIO_SEND_COMMAND_INDEX_MASK,
+ (0x40 | cmd->opcode));
+
+ /*
+ * soft-reset the MMC core and re-apply the IOS to make sure that these
+ * are correct for the slot which is selected below (as the IOS
+ * registers are shared across all slots).
+ */
+ meson_mx_mmc_soft_reset(slot->host);
+ meson_mx_mmc_apply_ios(mmc, &mmc->ios);
The above function calls clk_set_rate(), which triggers the common
clock framework to take the mutex.
Doing that while disabled irqs via spin_lock_irqsave() earlier above,
seems like a bad idea.
OK, noted - see also my comment regarding the whole locking situation below
Post by Ulf Hansson
Post by Martin Blumenstingl
+
+ mult = meson_mx_mmc_readl(mmc, MESON_MX_SDIO_MULT);
+ mult &= ~MESON_MX_SDIO_MULT_PORT_SEL_MASK;
+ mult |= FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK, slot->id);
+ mult |= BIT(31);
+ meson_mx_mmc_writel(mmc, mult, MESON_MX_SDIO_MULT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN,
+ MESON_MX_SDIO_IRQC_ARC_CMD_INT_EN);
+
+ /* clear pending interrupts */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_CMD_INT,
+ MESON_MX_SDIO_IRQS_CMD_INT);
+
+ meson_mx_mmc_writel(mmc, cmd->arg, MESON_MX_SDIO_ARGU);
+ meson_mx_mmc_writel(mmc, ext, MESON_MX_SDIO_EXT);
+ meson_mx_mmc_writel(mmc, send, MESON_MX_SDIO_SEND);
+
+ spin_unlock_irqrestore(&slot->host->irq_lock, irqflags);
+
+ if (cmd->opcode == MMC_ERASE)
+ timeout = msecs_to_jiffies(30000);
+ else if (cmd->data)
+ timeout = msecs_to_jiffies(5000);
+ else
+ timeout = msecs_to_jiffies(1000);
Please don't hard-code these timeouts. Instead make use of the
cmd.busy_timeout, as that should contain a proper timeout value.
I successfully tested a reduced version of this, using
cmd.busy_timeout as suggested (and only fall back to a 1sec timeout if
there's no busy_timeout given): this still works fine on my hardware
(so again, thanks for the hint!)
Post by Ulf Hansson
Post by Martin Blumenstingl
+
+ mod_timer(&slot->host->cmd_timeout, jiffies + timeout);
+}
+
[...]
Post by Martin Blumenstingl
+static void meson_mx_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ if (spin_trylock(&slot->host->lock)) {
+ /*
+ * only apply the mmc_ios if we are idle to not break any
+ * ongoing transfer. in case we are busy meson_mx_mmc_start_cmd
+ * will take care of applying the mmc_ios later on.
+ */
+ if (slot->host->status == MESON_MX_MMC_STATUS_IDLE)
+ meson_mx_mmc_apply_ios(mmc, ios);
No this doesn't work!
In case the status != MESON_MX_MMC_STATUS_IDLE or if the attempt to
take the lock fails, you will just silently ignore to set the new ios
settings.
The mmc core implements the mmc/sd/sdio specifications, so when you
return from the ->set_ios() host ops, the mmc core relies on the host
to have applied the settings to conform the the specs. You can not
delay that to a later point.
OK, I did not know that this was part of the mmc/sd/sdio spec
the problem I (and the vendor driver) was trying to solve here is the
fact that we could end up with a call chain like this:
- slot0.set_ios(400 kHz)
- slot1.set_ios(50 MHz)
- slot0.request(cmd)
- slot1.request(cmd)
(or anything else, where the IOS change between the .set_ios and
.request invocation of the same slot)

the main problem here is that the clock divider register bits are
shared across all slots!

can I simply always apply the IOS here in .set_ios and then still
re-apply them in .request() if needed?
Post by Ulf Hansson
Post by Martin Blumenstingl
+
+ spin_unlock(&slot->host->lock);
+ }
+
+ switch (ios->power_mode) {
+ if (!IS_ERR(mmc->supply.vmmc))
+ mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
+ break;
+
+ if (!IS_ERR(mmc->supply.vmmc))
+ mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
+ break;
+ }
+}
+
+static void meson_mx_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_MULT,
+ MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ slot->id));
+
+ /* ACK pending interrupt */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_IF_INT,
+ MESON_MX_SDIO_IRQS_IF_INT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_IF_INT_EN,
+ enable ? MESON_MX_SDIO_IRQC_ARC_IF_INT_EN : 0);
+
+ if (enable)
+ slot->host->sdio_irq_slot = slot;
+ else
+ slot->host->sdio_irq_slot = NULL;
This looks weird. You support up to three slots per host, but only one
can do sdio_irq?
BTW, what happens if there are is a ongoing data transfer on an SD
card slot, while there is an SDIO irq raised on the SDIO card slot? Do
you cope with this correctly?
good question indeed. I guess I'll remove SDIO interrupt support for
now as I am not sure how this is supposed to work in the vendor driver
(which just uses the last active slot to figure out if there was an
SDIO interrupt)
Post by Ulf Hansson
Post by Martin Blumenstingl
+
+ spin_unlock_irqrestore(&slot->host->irq_lock, irqflags);
+}
+
[...]
Post by Martin Blumenstingl
+
+static void meson_mx_mmc_process_sdio_irq(struct meson_mx_mmc_slot *slot)
+{
+ /*
+ * ignore SDIO interrupts without corresponding slot as the SDIO
+ * interrupt seems to enable itself automatically - in this case we
+ * didn't assign a slot for this IRQ so we simply ignore it.
+ */
+ if (!slot)
+ return;
+
+ mmc_signal_sdio_irq(slot->mmc);
Please use the new sdio_signal_irq() API and method instead. This also
means you should implement the ->ack_sdio_irq() host ops.
I'll keep this in mind if decide that I will keep SDIO interrupt
support (see above)
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
+
+static int meson_mx_mmc_register_slot_device(struct device_node *np,
+ unsigned int id,
+ struct meson_mx_mmc_host *host)
+{
+ struct device *dev = &host->slot_devices[id];
+
+ dev->parent = host->dev;
+ dev->of_node = np;
+ dev->release = meson_mx_mmc_slot_device_release;
+ dev_set_name(dev, "%s.%d", dev_name(dev->parent), id);
+
+ return device_register(dev);
As stated, please try to use of_platform_device_create() instead.
Post by Martin Blumenstingl
+}
+
+static int meson_mx_mmc_add_slot(struct device_node *np,
+ struct meson_mx_mmc_host *host)
+{
+ struct meson_mx_mmc_slot *slot;
+ struct mmc_host *mmc;
+ unsigned int id;
+ int ret;
+
+ if (of_property_read_u32(np, "reg", &id)) {
+ dev_err(host->dev, "missing 'reg' property for %s\n",
+ of_node_full_name(np));
+ return -EINVAL;
+ }
+
+ if (id >= MESON_MX_SDIO_MAX_SLOTS) {
+ dev_err(host->dev,
+ "invalid 'reg' property value of %s\n",
+ of_node_full_name(np));
+ return -EINVAL;
+ }
+
+ ret = meson_mx_mmc_register_slot_device(np, id, host);
+ if (ret)
+ return ret;
+
+ mmc = mmc_alloc_host(sizeof(*slot), &host->slot_devices[id]);
+ if (!mmc) {
+ ret = -ENOMEM;
+ goto error_unregister_dev;
+ }
+
+ slot = mmc_priv(mmc);
+ slot->mmc = mmc;
+ slot->id = id;
+ slot->host = host;
+
+ host->slots[id] = slot;
+
+ /* Get regulators and the supported OCR mask */
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto error_free_host;
+
+ mmc->max_req_size = MESON_MX_SDIO_BOUNCE_REQ_SIZE;
+ mmc->max_seg_size = mmc->max_req_size;
+ mmc->max_blk_count =
+ FIELD_GET(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
+ 0xffffffff);
+ mmc->max_blk_size = FIELD_GET(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
+ 0xffffffff);
+ mmc->max_blk_size -= (4 * MESON_MX_SDIO_RESPONSE_CRC16_BITS);
+ mmc->max_blk_size /= BITS_PER_BYTE;
+
+ /* Get the min and max supported clock rates */
+ mmc->f_min = clk_round_rate(host->cfg_div_clk, 1);
+ mmc->f_max = clk_round_rate(host->cfg_div_clk,
+ clk_get_rate(host->parent_clk));
+
+ mmc->caps |= MMC_CAP_CMD23;
+ mmc->ops = &meson_mx_mmc_ops;
+
+ ret = mmc_of_parse(mmc);
+ if (ret)
+ goto error_free_host;
+
+ ret = mmc_add_host(mmc);
+ if (ret)
+ goto error_free_host;
+
+ return 0;
+
+ mmc_free_host(mmc);
+ device_unregister(&host->slot_devices[id]);
+ return ret;
+}
+
+static int meson_mx_mmc_probe_slots(struct meson_mx_mmc_host *host)
+{
+ struct device_node *slot_node, *controller_node;
+ int num_slots, ret;
+
+ controller_node = host->dev->of_node;
+
+ num_slots = of_get_available_child_count(controller_node);
According to the comment I had on the DT doc change in patch1, this
doesn't work as we are using child nodes to describe an embedded card
(embedded SDIO/eMMC) and SDIO func devices.
You need to also search for a compatible string, "mmc-slot".
it actually works (as I described the devices as child-nodes of the
slot nodes, so the graph is: SoC > MMC controller > slot N > device).
however, that doesn't mean it's correct :)
I added a check for "mmc-slot" compatibility. I'll also mention this
in the documentation
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (num_slots > MESON_MX_SDIO_MAX_SLOTS) {
+ dev_err(host->dev, "more slots configured than supported\n");
+ return -EINVAL;
+ }
+
+ for_each_available_child_of_node(controller_node, slot_node) {
+ ret = meson_mx_mmc_add_slot(slot_node, host);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static int meson_mx_mmc_register_clks(struct meson_mx_mmc_host *host)
+{
+ struct clk_init_data init;
+ const char *clk_div_parents[1], *clk_fixed_factor_parents[1];
It seems silly to use and array of size 1. Below should work as well, right!?
const char *clk_div_parents, *clk_fixed_factor_parents;
indeed, works fine as well
Post by Ulf Hansson
Post by Martin Blumenstingl
+
+ clk_fixed_factor_parents[0] = __clk_get_name(host->parent_clk);
+ init.name = devm_kasprintf(host->dev, GFP_KERNEL, "%s#fixed_factor",
+ dev_name(host->dev));
+ init.ops = &clk_fixed_factor_ops;
+ init.flags = 0;
+ init.parent_names = clk_fixed_factor_parents;
+ init.num_parents = 1;
+ host->fixed_factor.div = 2;
+ host->fixed_factor.mult = 1;
+ host->fixed_factor.hw.init = &init;
+
+ host->fixed_factor_clk = devm_clk_register(host->dev,
+ &host->fixed_factor.hw);
+ if (WARN_ON(PTR_ERR_OR_ZERO(host->fixed_factor_clk)))
+ return PTR_ERR(host->fixed_factor_clk);
+
+ clk_div_parents[0] = __clk_get_name(host->fixed_factor_clk);
+ init.name = devm_kasprintf(host->dev, GFP_KERNEL, "%s#div",
+ dev_name(host->dev));
+ init.ops = &clk_divider_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.parent_names = clk_div_parents;
+ init.num_parents = 1;
+ host->cfg_div.reg = host->base + MESON_MX_SDIO_CONF;
+ host->cfg_div.shift = MESON_MX_SDIO_CONF_CMD_CLK_DIV_SHIFT;
+ host->cfg_div.width = MESON_MX_SDIO_CONF_CMD_CLK_DIV_WIDTH;
+ host->cfg_div.hw.init = &init;
+ host->cfg_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+
+ host->cfg_div_clk = devm_clk_register(host->dev, &host->cfg_div.hw);
+ if (WARN_ON(PTR_ERR_OR_ZERO(host->cfg_div_clk)))
+ return PTR_ERR(host->fixed_factor_clk);
+
+ return 0;
+}
+
[...]
Another overall comment, which relates to the host locking mechanism
and the problem with ->set_ios(). Perhaps you can look into how the
cavium mmc driver has solved the similar problems as it also manages
several slots per host.
thank your for the hint with the cavium driver. using a semaphore to
serialize the requests of multiple slots seems a good idea too.
to avoid a "broken by design" RFC v2, can you please give me some more
details how the cavium driver code matches with the mmc/sd/sdio spec
and the mmc framework?

as I explained above the clock divider and bus-width register bits are
shared across all slots. so if one device operates in 1-bit mode while
the other uses 4-bit mode (or different clock rates, it just doesn't
matter) then we need to make sure that these settings stay the same
between .set_ios() and .request, until the request is completed
(regardless of whether that was successful or not/a timeout occurred).

from what I can read in the cavium driver (where the situation seems
to be the same), it uses acquire_bus() at the beginning of
.set_ios/.request and release_bus() at the end of it and keeping a
backup of the registers which are modified during .set_ios. once it
switches to a different slot it restores the register values for the
new slot (this can also happen in .request). this solves the problem
of keeping the correct IOS when switching slots at any point

however, what I don't understand so far is how it synchronizes the
access to the CMD response bits which are read in the interrupt. what
if slot0 sends a command, then right after that (before the the card
in slot0 replied) slot1 sends a command -> how is it going to know to
which slot the response belongs? (I am facing the same problem with
the the meson-mx-sdio driver, the solution is to allow only one
request at a time and queue all other requests -> I am not sure if
this is good solution though)


Regards,
Martin
Ulf Hansson
2017-06-07 16:15:36 UTC
Permalink
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
+static void meson_mx_mmc_apply_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long clk_rate = ios->clock;
+ int ret;
+
+ switch (ios->bus_width) {
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH, 0);
+ break;
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH,
+ MESON_MX_SDIO_CONF_BUS_WIDTH);
+ break;
+
+ dev_err(mmc_dev(mmc), "unsupported bus width: %d\n",
+ ios->bus_width);
+ slot->error = -EINVAL;
+ return;
+ }
+
+ if (clk_rate) {
+ if (WARN_ON(clk_rate > mmc->f_max))
+ clk_rate = mmc->f_max;
+ else if (WARN_ON(clk_rate < mmc->f_min))
+ clk_rate = mmc->f_min;
+
+ ret = clk_set_rate(slot->host->cfg_div_clk, ios->clock);
+ if (ret) {
+ dev_warn(mmc_dev(mmc),
+ "failed to set MMC clock to %lu: %d\n",
+ clk_rate, ret);
+ slot->error = ret;
+ return;
+ }
+
+ mmc->actual_clock = clk_get_rate(slot->host->cfg_div_clk);
+ }
In some cases the mmc core request the clock rate to be zero (to gate
the clock) which is needed to for example switch to UHS speed mode. If
you intend to support that, you need to manage this at this point.
thanks for the explanation - unfortunately the lack of a datasheet
which properly describes the clocks makes this a bit hard to
implement.
the vendor driver simply ignores any set_ios request with clock = 0
it seems that there is also no dedicated "stop clock" bit (and we only
have a clock divider). all I could find is FORCE_HALT (bit 30 in the
IRQC register), where the vendor driver explains: "Force halt SDIO by
software. Halt in this sdio host controller means stop to transmit or
receive data from sd card. and then sd card clock will be shutdown.
Software can force to halt anytime, and hardware will automatically
halt the sdio when reading fifo is full or writing fifo is empty"
should I simply remove that if (...) and try to assign 0 to the clock anyways?
I am not sure. Perhaps you are left with clk_disable_unprepare(), and
hope no other is using the clock.

Although, I suggest you address this as separate change on top.

[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Please don't just leave it as is. This is an important thing to get right.

You should be able to explore this area and see how the controller
behaves without too much of documentation. Regarding timeouts, it may
very well be that the controller don't have a timeout, which is why
you need a software timeout. That's not so uncommon actually.

[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+static void meson_mx_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ if (spin_trylock(&slot->host->lock)) {
+ /*
+ * only apply the mmc_ios if we are idle to not break any
+ * ongoing transfer. in case we are busy meson_mx_mmc_start_cmd
+ * will take care of applying the mmc_ios later on.
+ */
+ if (slot->host->status == MESON_MX_MMC_STATUS_IDLE)
+ meson_mx_mmc_apply_ios(mmc, ios);
No this doesn't work!
In case the status != MESON_MX_MMC_STATUS_IDLE or if the attempt to
take the lock fails, you will just silently ignore to set the new ios
settings.
The mmc core implements the mmc/sd/sdio specifications, so when you
return from the ->set_ios() host ops, the mmc core relies on the host
to have applied the settings to conform the the specs. You can not
delay that to a later point.
OK, I did not know that this was part of the mmc/sd/sdio spec
the problem I (and the vendor driver) was trying to solve here is the
- slot0.set_ios(400 kHz)
- slot1.set_ios(50 MHz)
- slot0.request(cmd)
- slot1.request(cmd)
(or anything else, where the IOS change between the .set_ios and
.request invocation of the same slot)
the main problem here is that the clock divider register bits are
shared across all slots!
can I simply always apply the IOS here in .set_ios and then still
re-apply them in .request() if needed?
Thinking more about this, and the answer is unfortunately *no*.

The mmc core uses mmc_claim|release_host() to get exclusive access to
operate the host via the host ops callbacks.

For some cases, that involves a sequence of commands/requests being
carried out via invoking to the host ops. During some of these
sequences, one can definitely not allow to change ios, because another
host/slot needs it in between. That will break the protocol - for
sure.

This also make me realize that the few other host drivers. which tries
to supports multiple slots are all broken. On the other side, that
just confirms my doubt about this; this is just a theoretical thing
one want to support or used in environments that doesn't requires
"product quality".

So to be able to support "multiple slots", we need to teach the mmc
core about hosts that supports multiple slots. This needs to be done
in a way that mmc_claim_host() gets exclusive right to run a host, but
without other hosts sharing the same controller are allowed to run
in-between.

It shouldn't be that hard to implement, although I strongly recommend
you to address this in a second step. In other words, I suggest you to
drop the entire multiple slot support in the first step, then we can
deal with that on top instead.

[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+static void meson_mx_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_MULT,
+ MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ slot->id));
+
+ /* ACK pending interrupt */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_IF_INT,
+ MESON_MX_SDIO_IRQS_IF_INT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_IF_INT_EN,
+ enable ? MESON_MX_SDIO_IRQC_ARC_IF_INT_EN : 0);
+
+ if (enable)
+ slot->host->sdio_irq_slot = slot;
+ else
+ slot->host->sdio_irq_slot = NULL;
This looks weird. You support up to three slots per host, but only one
can do sdio_irq?
BTW, what happens if there are is a ongoing data transfer on an SD
card slot, while there is an SDIO irq raised on the SDIO card slot? Do
you cope with this correctly?
good question indeed. I guess I'll remove SDIO interrupt support for
now as I am not sure how this is supposed to work in the vendor driver
(which just uses the last active slot to figure out if there was an
SDIO interrupt)
If you drop the multiple slot support, this should be easier to
verify. However, if you prefer adding it in step on top, I am of
course fine with that as well.

[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Another overall comment, which relates to the host locking mechanism
and the problem with ->set_ios(). Perhaps you can look into how the
cavium mmc driver has solved the similar problems as it also manages
several slots per host.
thank your for the hint with the cavium driver. using a semaphore to
serialize the requests of multiple slots seems a good idea too.
to avoid a "broken by design" RFC v2, can you please give me some more
details how the cavium driver code matches with the mmc/sd/sdio spec
and the mmc framework?
Unfortunate no, you will have to ask the cavium folkz about these
details. However, it may very well be that this is also broken,
according to my comments about multiple slot support.
Post by Martin Blumenstingl
as I explained above the clock divider and bus-width register bits are
shared across all slots. so if one device operates in 1-bit mode while
the other uses 4-bit mode (or different clock rates, it just doesn't
matter) then we need to make sure that these settings stay the same
between .set_ios() and .request, until the request is completed
(regardless of whether that was successful or not/a timeout occurred).
from what I can read in the cavium driver (where the situation seems
to be the same), it uses acquire_bus() at the beginning of
.set_ios/.request and release_bus() at the end of it and keeping a
backup of the registers which are modified during .set_ios. once it
switches to a different slot it restores the register values for the
new slot (this can also happen in .request). this solves the problem
of keeping the correct IOS when switching slots at any point
Yes, then it seems like also this driver is broken.
Post by Martin Blumenstingl
however, what I don't understand so far is how it synchronizes the
access to the CMD response bits which are read in the interrupt. what
if slot0 sends a command, then right after that (before the the card
in slot0 replied) slot1 sends a command -> how is it going to know to
which slot the response belongs? (I am facing the same problem with
the the meson-mx-sdio driver, the solution is to allow only one
request at a time and queue all other requests -> I am not sure if
this is good solution though)
I assume the lock is held throughout the entire period serving a
request. However that doesn't cover all scenarios, as explained above.

Kind regards
Uffe
Martin Blumenstingl
2017-06-09 21:51:36 UTC
Permalink
Hi Ulf,
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
+static void meson_mx_mmc_apply_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long clk_rate = ios->clock;
+ int ret;
+
+ switch (ios->bus_width) {
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH, 0);
+ break;
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_CONF,
+ MESON_MX_SDIO_CONF_BUS_WIDTH,
+ MESON_MX_SDIO_CONF_BUS_WIDTH);
+ break;
+
+ dev_err(mmc_dev(mmc), "unsupported bus width: %d\n",
+ ios->bus_width);
+ slot->error = -EINVAL;
+ return;
+ }
+
+ if (clk_rate) {
+ if (WARN_ON(clk_rate > mmc->f_max))
+ clk_rate = mmc->f_max;
+ else if (WARN_ON(clk_rate < mmc->f_min))
+ clk_rate = mmc->f_min;
+
+ ret = clk_set_rate(slot->host->cfg_div_clk, ios->clock);
+ if (ret) {
+ dev_warn(mmc_dev(mmc),
+ "failed to set MMC clock to %lu: %d\n",
+ clk_rate, ret);
+ slot->error = ret;
+ return;
+ }
+
+ mmc->actual_clock = clk_get_rate(slot->host->cfg_div_clk);
+ }
In some cases the mmc core request the clock rate to be zero (to gate
the clock) which is needed to for example switch to UHS speed mode. If
you intend to support that, you need to manage this at this point.
thanks for the explanation - unfortunately the lack of a datasheet
which properly describes the clocks makes this a bit hard to
implement.
the vendor driver simply ignores any set_ios request with clock = 0
it seems that there is also no dedicated "stop clock" bit (and we only
have a clock divider). all I could find is FORCE_HALT (bit 30 in the
IRQC register), where the vendor driver explains: "Force halt SDIO by
software. Halt in this sdio host controller means stop to transmit or
receive data from sd card. and then sd card clock will be shutdown.
Software can force to halt anytime, and hardware will automatically
halt the sdio when reading fifo is full or writing fifo is empty"
should I simply remove that if (...) and try to assign 0 to the clock anyways?
I am not sure. Perhaps you are left with clk_disable_unprepare(), and
hope no other is using the clock.
Although, I suggest you address this as separate change on top.
OK, getting the basic thing working first sounds like a good idea
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Please don't just leave it as is. This is an important thing to get right.
You should be able to explore this area and see how the controller
behaves without too much of documentation. Regarding timeouts, it may
very well be that the controller don't have a timeout, which is why
you need a software timeout. That's not so uncommon actually.
during my experiments I've never seen an interrupt when a command
timed out (nor could I find information about a timeout register in
the documentation). do you have any pointers (like a previous mail
where you've explained) how I can "explore the controller's timeout
behavior"?
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+static void meson_mx_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+
+ if (spin_trylock(&slot->host->lock)) {
+ /*
+ * only apply the mmc_ios if we are idle to not break any
+ * ongoing transfer. in case we are busy meson_mx_mmc_start_cmd
+ * will take care of applying the mmc_ios later on.
+ */
+ if (slot->host->status == MESON_MX_MMC_STATUS_IDLE)
+ meson_mx_mmc_apply_ios(mmc, ios);
No this doesn't work!
In case the status != MESON_MX_MMC_STATUS_IDLE or if the attempt to
take the lock fails, you will just silently ignore to set the new ios
settings.
The mmc core implements the mmc/sd/sdio specifications, so when you
return from the ->set_ios() host ops, the mmc core relies on the host
to have applied the settings to conform the the specs. You can not
delay that to a later point.
OK, I did not know that this was part of the mmc/sd/sdio spec
the problem I (and the vendor driver) was trying to solve here is the
- slot0.set_ios(400 kHz)
- slot1.set_ios(50 MHz)
- slot0.request(cmd)
- slot1.request(cmd)
(or anything else, where the IOS change between the .set_ios and
.request invocation of the same slot)
the main problem here is that the clock divider register bits are
shared across all slots!
can I simply always apply the IOS here in .set_ios and then still
re-apply them in .request() if needed?
Thinking more about this, and the answer is unfortunately *no*.
The mmc core uses mmc_claim|release_host() to get exclusive access to
operate the host via the host ops callbacks.
For some cases, that involves a sequence of commands/requests being
carried out via invoking to the host ops. During some of these
sequences, one can definitely not allow to change ios, because another
host/slot needs it in between. That will break the protocol - for
sure.
This also make me realize that the few other host drivers. which tries
to supports multiple slots are all broken. On the other side, that
just confirms my doubt about this; this is just a theoretical thing
one want to support or used in environments that doesn't requires
"product quality".
So to be able to support "multiple slots", we need to teach the mmc
core about hosts that supports multiple slots. This needs to be done
in a way that mmc_claim_host() gets exclusive right to run a host, but
without other hosts sharing the same controller are allowed to run
in-between.
It shouldn't be that hard to implement, although I strongly recommend
you to address this in a second step. In other words, I suggest you to
drop the entire multiple slot support in the first step, then we can
deal with that on top instead.
many thanks for the detailed explanation again!
I would be fine with dropping multiple slot support for the moment
*if* we can agree on the fact that the devicetree binding can support
multiple slots in theory (my idea here is: keep the child-nodes with
compatible = "mmc-slot" mandatory - but only allow one such child node
for now). I don't want to break DT compatibility after a few
weeks/months for a new driver. is that OK for you as well?
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+static void meson_mx_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ struct meson_mx_mmc_slot *slot = mmc_priv(mmc);
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&slot->host->irq_lock, irqflags);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_MULT,
+ MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ FIELD_PREP(MESON_MX_SDIO_MULT_PORT_SEL_MASK,
+ slot->id));
+
+ /* ACK pending interrupt */
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQS,
+ MESON_MX_SDIO_IRQS_IF_INT,
+ MESON_MX_SDIO_IRQS_IF_INT);
+
+ meson_mx_mmc_mask_bits(mmc, MESON_MX_SDIO_IRQC,
+ MESON_MX_SDIO_IRQC_ARC_IF_INT_EN,
+ enable ? MESON_MX_SDIO_IRQC_ARC_IF_INT_EN : 0);
+
+ if (enable)
+ slot->host->sdio_irq_slot = slot;
+ else
+ slot->host->sdio_irq_slot = NULL;
This looks weird. You support up to three slots per host, but only one
can do sdio_irq?
BTW, what happens if there are is a ongoing data transfer on an SD
card slot, while there is an SDIO irq raised on the SDIO card slot? Do
you cope with this correctly?
good question indeed. I guess I'll remove SDIO interrupt support for
now as I am not sure how this is supposed to work in the vendor driver
(which just uses the last active slot to figure out if there was an
SDIO interrupt)
If you drop the multiple slot support, this should be easier to
verify. However, if you prefer adding it in step on top, I am of
course fine with that as well.
I guess I'll postpone this until I have a working wifi driver (the
rtl8723bs driver doesn't seem to work for me due to whatever reason)
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Another overall comment, which relates to the host locking mechanism
and the problem with ->set_ios(). Perhaps you can look into how the
cavium mmc driver has solved the similar problems as it also manages
several slots per host.
thank your for the hint with the cavium driver. using a semaphore to
serialize the requests of multiple slots seems a good idea too.
to avoid a "broken by design" RFC v2, can you please give me some more
details how the cavium driver code matches with the mmc/sd/sdio spec
and the mmc framework?
Unfortunate no, you will have to ask the cavium folkz about these
details. However, it may very well be that this is also broken,
according to my comments about multiple slot support.
OK, I guess (like you explained above) it's easier to not look at any
maybe-broken (existing) implementations and try to add multi slot
support to the MMC core instead (I will open a separate topic for this
on linux-mmc once I have a bit more time)
Post by Ulf Hansson
Post by Martin Blumenstingl
as I explained above the clock divider and bus-width register bits are
shared across all slots. so if one device operates in 1-bit mode while
the other uses 4-bit mode (or different clock rates, it just doesn't
matter) then we need to make sure that these settings stay the same
between .set_ios() and .request, until the request is completed
(regardless of whether that was successful or not/a timeout occurred).
from what I can read in the cavium driver (where the situation seems
to be the same), it uses acquire_bus() at the beginning of
.set_ios/.request and release_bus() at the end of it and keeping a
backup of the registers which are modified during .set_ios. once it
switches to a different slot it restores the register values for the
new slot (this can also happen in .request). this solves the problem
of keeping the correct IOS when switching slots at any point
Yes, then it seems like also this driver is broken.
Post by Martin Blumenstingl
however, what I don't understand so far is how it synchronizes the
access to the CMD response bits which are read in the interrupt. what
if slot0 sends a command, then right after that (before the the card
in slot0 replied) slot1 sends a command -> how is it going to know to
which slot the response belongs? (I am facing the same problem with
the the meson-mx-sdio driver, the solution is to allow only one
request at a time and queue all other requests -> I am not sure if
this is good solution though)
I assume the lock is held throughout the entire period serving a
request. However that doesn't cover all scenarios, as explained above.
OK, I guess we're done with that topic now - let's skip multi slot
support for version one of my driver


Regards,
Martin
Ulf Hansson
2017-06-19 11:50:39 UTC
Permalink
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Please don't just leave it as is. This is an important thing to get right.
You should be able to explore this area and see how the controller
behaves without too much of documentation. Regarding timeouts, it may
very well be that the controller don't have a timeout, which is why
you need a software timeout. That's not so uncommon actually.
during my experiments I've never seen an interrupt when a command
timed out (nor could I find information about a timeout register in
the documentation). do you have any pointers (like a previous mail
where you've explained) how I can "explore the controller's timeout
behavior"?
Sorry, I don't have an pointers.

Anyway. If you do a big erase operation on an SD card, the card should
signal busy on DAT0 for a rather long time.

You could probably explore how long it takes for the card to respond
under those circumstances, and try both with and without
MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY.

Of course you also need to try with and without
MMC_CAP_WAIT_WHILE_BUSY, as the core may sometimes convert R1B to R1
responses depending on that cap.

[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
It shouldn't be that hard to implement, although I strongly recommend
you to address this in a second step. In other words, I suggest you to
drop the entire multiple slot support in the first step, then we can
deal with that on top instead.
many thanks for the detailed explanation again!
I would be fine with dropping multiple slot support for the moment
*if* we can agree on the fact that the devicetree binding can support
multiple slots in theory (my idea here is: keep the child-nodes with
compatible = "mmc-slot" mandatory - but only allow one such child node
for now). I don't want to break DT compatibility after a few
weeks/months for a new driver. is that OK for you as well?
That's fine! We already have such a binding available for some other
mmc controllers. We could even consider to make that binding a generic
mmc binding.

[...]

Kind regards
Uffe
Martin Blumenstingl
2017-07-11 21:23:44 UTC
Permalink
Hi Ulf,

sorry for the delay, I've been pretty busy.
however, I'll have more time to work in this driver this and next week
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Please don't just leave it as is. This is an important thing to get right.
You should be able to explore this area and see how the controller
behaves without too much of documentation. Regarding timeouts, it may
very well be that the controller don't have a timeout, which is why
you need a software timeout. That's not so uncommon actually.
during my experiments I've never seen an interrupt when a command
timed out (nor could I find information about a timeout register in
the documentation). do you have any pointers (like a previous mail
where you've explained) how I can "explore the controller's timeout
behavior"?
Sorry, I don't have an pointers.
Anyway. If you do a big erase operation on an SD card, the card should
signal busy on DAT0 for a rather long time.
erase operation = mount sd card; rm big_file.bin; sync ?
or is there some other way?
Post by Ulf Hansson
You could probably explore how long it takes for the card to respond
under those circumstances, and try both with and without
MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY.
so I simply read the card status through sysfs? or would I need to get
some of this information from the controller?
I found that the IRQC register contains a few interesting bits:
u32 sdio_force_data:6; /*[13:8]
* Write operation: Data forced by software
* Read operation: {CLK,CMD,DAT[3:0]}*/
I dumped these while transferring some data, the values seem to be
changing constantly (0x3f, 0x1f, 0x20, ...)
if I interpret those bits correctly then they are the status of the
corresponding pin
Post by Ulf Hansson
Of course you also need to try with and without
MMC_CAP_WAIT_WHILE_BUSY, as the core may sometimes convert R1B to R1
responses depending on that cap.
so MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY + MMC_CAP_WAIT_WHILE_BUSY should
go together?
and MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY should not be set when
MMC_CAP_WAIT_WHILE_BUSY isn't?
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
It shouldn't be that hard to implement, although I strongly recommend
you to address this in a second step. In other words, I suggest you to
drop the entire multiple slot support in the first step, then we can
deal with that on top instead.
many thanks for the detailed explanation again!
I would be fine with dropping multiple slot support for the moment
*if* we can agree on the fact that the devicetree binding can support
multiple slots in theory (my idea here is: keep the child-nodes with
compatible = "mmc-slot" mandatory - but only allow one such child node
for now). I don't want to break DT compatibility after a few
weeks/months for a new driver. is that OK for you as well?
That's fine! We already have such a binding available for some other
mmc controllers. We could even consider to make that binding a generic
mmc binding.
OK, perfect :)

Regards,
Martin
Ulf Hansson
2017-07-12 13:42:45 UTC
Permalink
On 11 July 2017 at 23:23, Martin Blumenstingl
Post by Martin Blumenstingl
Hi Ulf,
sorry for the delay, I've been pretty busy.
however, I'll have more time to work in this driver this and next week
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Please don't just leave it as is. This is an important thing to get right.
You should be able to explore this area and see how the controller
behaves without too much of documentation. Regarding timeouts, it may
very well be that the controller don't have a timeout, which is why
you need a software timeout. That's not so uncommon actually.
during my experiments I've never seen an interrupt when a command
timed out (nor could I find information about a timeout register in
the documentation). do you have any pointers (like a previous mail
where you've explained) how I can "explore the controller's timeout
behavior"?
Sorry, I don't have an pointers.
Anyway. If you do a big erase operation on an SD card, the card should
signal busy on DAT0 for a rather long time.
erase operation = mount sd card; rm big_file.bin; sync ?
or is there some other way?
There is tool called fstrim, maybe you can you that as well.

Don't forget that the mount options of the fs also matters of what
will happen for the so called discard operations (translates to erase
in SD).
Post by Martin Blumenstingl
Post by Ulf Hansson
You could probably explore how long it takes for the card to respond
under those circumstances, and try both with and without
MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY.
so I simply read the card status through sysfs? or would I need to get
some of this information from the controller?
u32 sdio_force_data:6; /*[13:8]
* Write operation: Data forced by software
* Read operation: {CLK,CMD,DAT[3:0]}*/
I dumped these while transferring some data, the values seem to be
changing constantly (0x3f, 0x1f, 0x20, ...)
if I interpret those bits correctly then they are the status of the
corresponding pin
Okay! So if the controller doesn't fully support HW busy detection,
you should at least be able to implement the ->card_busy() host ops,
which is used to poll the DAT0 line for busy detection.
Post by Martin Blumenstingl
Post by Ulf Hansson
Of course you also need to try with and without
MMC_CAP_WAIT_WHILE_BUSY, as the core may sometimes convert R1B to R1
responses depending on that cap.
so MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY + MMC_CAP_WAIT_WHILE_BUSY should
go together?
and MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY should not be set when
MMC_CAP_WAIT_WHILE_BUSY isn't?
Yeah, something like that.

So if you controller fully supports HW busy detection, the driver
should set MMC_CAP_WAIT_WHILE_BUSY and then act accordingly when R1B
responses are expected.

[...]

Kind regards
Uffe
Martin Blumenstingl
2017-07-18 11:17:21 UTC
Permalink
Hi Ulf,
Post by Ulf Hansson
On 11 July 2017 at 23:23, Martin Blumenstingl
Post by Martin Blumenstingl
Hi Ulf,
sorry for the delay, I've been pretty busy.
however, I'll have more time to work in this driver this and next week
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
[...]
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
+ if (!(cmd->flags & MMC_RSP_CRC))
+ send |= MESON_MX_SDIO_SEND_RESP_WITHOUT_CRC7;
+
+ if (cmd->flags & MMC_RSP_BUSY)
+ send |= MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY;
In case the controller has HW support of busy detection, please
consider to enable MMC_CAP_WAIT_WHILE_BUSY for this driver. Then also
assign host->max_busy_timeout a good value.
the IRQS register has bit 4 (CMD_BUSY) - but apart from that there is
no other documentation (about timeout values, etc.). the vendor driver
also neither uses MMC_CAP_WAIT_WHILE_BUSY nor host->max_busy_timeout
should I leave this as it is?
Please don't just leave it as is. This is an important thing to get right.
You should be able to explore this area and see how the controller
behaves without too much of documentation. Regarding timeouts, it may
very well be that the controller don't have a timeout, which is why
you need a software timeout. That's not so uncommon actually.
during my experiments I've never seen an interrupt when a command
timed out (nor could I find information about a timeout register in
the documentation). do you have any pointers (like a previous mail
where you've explained) how I can "explore the controller's timeout
behavior"?
Sorry, I don't have an pointers.
Anyway. If you do a big erase operation on an SD card, the card should
signal busy on DAT0 for a rather long time.
erase operation = mount sd card; rm big_file.bin; sync ?
or is there some other way?
There is tool called fstrim, maybe you can you that as well.
Don't forget that the mount options of the fs also matters of what
will happen for the so called discard operations (translates to erase
in SD).
I conducted a series of tests:

(the system boots with / mounted read-only):
mount -o remount,rw,discard
dd if=/dev/zero of=test.bin bs=1M count=1k

the actual test:
rm test.bin; sync

here are the results:

without MMC_CAP_WAIT_WHILE_BUSY, with MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY:
[ 98.891623] meson_mx_mmc_start_cmd: starting CMD38 with timeout = 33130
[ 99.341293] meson_mx_mmc_irq: CMD38 IRQS = 0x00000220 (timeout = 33125)

without MMC_CAP_WAIT_WHILE_BUSY, without MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY:
[ 197.591441] meson_mx_mmc_start_cmd: starting CMD38 with timeout = 4000
[ 197.591446] meson_mx_mmc_irq: CMD38 IRQS = 0x00000220 (timeout = 4000)
[ 202.881495] meson_mx_mmc_start_cmd: starting CMD38 with timeout = 30250
[ 202.881501] meson_mx_mmc_irq: CMD38 IRQS = 0x00000220 (timeout = 30250)

(I think the first one is is some other delete command, the actual rm
is probably the second one)

with MMC_CAP_WAIT_WHILE_BUSY (mmc->max_busy_timeout = ~0;), with
MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY:
[ 65.701441] meson_mx_mmc_start_cmd: starting CMD38 with timeout = 33125
[ 65.984235] meson_mx_mmc_irq: CMD38 IRQS = 0x00000220 (timeout = 33125)

so in all conditions CMD38 completes in less than 1 second
the card I'm using is a cheap "Intenso SDHC class 10" with 16GiB: [0]

an additional "fstrim /" didn't result in any CMD38 (because, just
like you said: the card is mounted with the discard flag)
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
You could probably explore how long it takes for the card to respond
under those circumstances, and try both with and without
MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY.
so I simply read the card status through sysfs? or would I need to get
some of this information from the controller?
u32 sdio_force_data:6; /*[13:8]
* Write operation: Data forced by software
* Read operation: {CLK,CMD,DAT[3:0]}*/
I dumped these while transferring some data, the values seem to be
changing constantly (0x3f, 0x1f, 0x20, ...)
if I interpret those bits correctly then they are the status of the
corresponding pin
Okay! So if the controller doesn't fully support HW busy detection,
you should at least be able to implement the ->card_busy() host ops,
which is used to poll the DAT0 line for busy detection.
only the DAT0 or DAT3:0?
the latter is what Amlogic does with their 8-bit bus capable drivers,
see [1] and [2]
(meson-gx-mmc.c is the upstream variant of [2] - where we don't
implement .card_busy or MMC_CAP_WAIT_WHILE_BUSY though)
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
Of course you also need to try with and without
MMC_CAP_WAIT_WHILE_BUSY, as the core may sometimes convert R1B to R1
responses depending on that cap.
so MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY + MMC_CAP_WAIT_WHILE_BUSY should
go together?
and MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY should not be set when
MMC_CAP_WAIT_WHILE_BUSY isn't?
Yeah, something like that.
So if you controller fully supports HW busy detection, the driver
should set MMC_CAP_WAIT_WHILE_BUSY and then act accordingly when R1B
responses are expected.
could you please have a look at my test results above and let me know
whether I should do more tests or which implementation you'd prefer
(from what I understood so far: if our hardware doesn't support
MMC_CAP_WAIT_WHILE_BUSY then I should implement .card_busy - but what
about MESON_MX_SDIO_SEND_CHECK_DAT0_BUSY then)?


Regards,
Martin


[0] http://intenso.de/multimedia/produktblatt/1343573205_en.pdf
[1] https://github.com/endlessm/linux-meson/blob/876ccc6ef48f406d8307dd741609c089e6e7e242/drivers/amlogic/mmc/aml_sdhc_m8.c#L2187
[2] https://github.com/endlessm/linux-s905x/blob/74ef17c59dab367c6040040940b77382a6098a58/drivers/amlogic/mmc/aml_sd_emmc.c#L3024
Ulf Hansson
2017-05-10 08:44:34 UTC
Permalink
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
This is the successor to Carlo Caione's "Add support for Amlogic Meson
MMC driver" series (v5) from [0].
- whether I've (ab)used the MMC framework properly (as this is my first
"larger" contribution to an MMC driver)
I take a look soonish.
Post by Martin Blumenstingl
- I think I have improved the locking compared to Carlo's version,
however I'd still like feedback on whether this looks sane now or if I
can improve that even further
- renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls
the driver "aml_sdio" as there is a second MMC controller in these SoCs
which they call the "SDHC controller"). do the same with our driver to
I don't like to renaming drivers, just because there are a reference
kernel using a different name.

What's is really the difference between controllers? Why do they have
two variants?

Can they be managed by the same driver?
Post by Martin Blumenstingl
avoid confusion once we add support for the second controller (which uses
a completely different register layout)
Besides that, do they behave differently in some other way?
Post by Martin Blumenstingl
- add support for the internal "mux" in this MMC controller (which allows
connecting up to three devices to the the controller - at the cost of
performance though since the controller can only process one request at
a time). The driver registers a new device for each sub-node, which is
then fed into the MMC framework to allow per-slot configuration using
devicetree (see the example in the documentation)
Unless there really is deployment for more than one slot on some
boards/SoCs, I would strongly suggest to *not* implement this.

Simply because of overhead and introduced complexity to the driver.
Post by Martin Blumenstingl
- use the common clock framework internally for managing the MMC clock
(there is a fixed-factor clock in the controller which takes clk81 as
input and divides it's clock by two and a divider clock which takes
the result from the fixed-factor clock as input)
- support the regulators provided by the MMC framework
- support for GPIO-based card-detection and read-only-detection through
the MMC framework
- use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it
make sense (and thus the code easier to read)
- re-worked locking (based on the locking in dw_mmc as that also provides
multiple "MMC slots")
Lots of changes!

Before even start to review (or someone else), you really need to make
this review-able.

So, please, one change per patch - and make sure to write good
changelogs. Then I can start to review.
Post by Martin Blumenstingl
- reading an OLD 256MiB SD card (which uses only a 1-bit bus) works fine
(sha1sum of the whole device matches with what I get on my PC's
card-reader)
- reading a somewhat more modern class 10 SD card and putting Arch Linux
ARM on it (and using that as root file system)
- it successfully detects the RTL8723BS SDIO wifi chip in my device (even
if the SD card is also enabled)
- reading a 128MiB file from the SD card while scanning wifi networks on
the RTL8723BS card does not seem to result in any corruption (sha1sum
of the read file seems to match)
- read speed of my class 10 SD card: ~15MiB/s
- (unfortunately I could NOT test downloading a file over wifi to the SD
card because the RTL8723BS driver refuses to see any wifi networks, but
that might be a problem on the RTL8723BS driver side since I don't get
any error and the driver has just landed a few weeks ago in staging)
[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/412136.html
[...]

Kind regards
Uffe
Martin Blumenstingl
2017-05-10 19:22:10 UTC
Permalink
Hi Ulf,
Post by Ulf Hansson
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
This is the successor to Carlo Caione's "Add support for Amlogic Meson
MMC driver" series (v5) from [0].
- whether I've (ab)used the MMC framework properly (as this is my first
"larger" contribution to an MMC driver)
I take a look soonish.
thank you!
Post by Ulf Hansson
Post by Martin Blumenstingl
- I think I have improved the locking compared to Carlo's version,
however I'd still like feedback on whether this looks sane now or if I
can improve that even further
- renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls
the driver "aml_sdio" as there is a second MMC controller in these SoCs
which they call the "SDHC controller"). do the same with our driver to
I don't like to renaming drivers, just because there are a reference
kernel using a different name.
What's is really the difference between controllers? Why do they have
two variants?
the driver from this thread is for the "SDSC/SDHC/SDXC card and SDIO
interface with 1-bit and 4-bit data bus width supporting spec version
2.x/3.x/4.x DS/HS modes up to UHS-I SDR50" (quote taken from the S805
datasheet: [0])
the "other" controller is an "eMMC and MMC card interface with
1/4/8-bit data bus width supporting spec version 4.4x/4.5x HS200 (up
to 100MHz clock), compatible with standard iNAND interface" (again,
quote taken from the S805 datasheet: [0])
Post by Ulf Hansson
Can they be managed by the same driver?
Post by Martin Blumenstingl
avoid confusion once we add support for the second controller (which uses
a completely different register layout)
Besides that, do they behave differently in some other way?
both drivers/controllers have a totally different register layout - I
don't see any way how they both could be handled by one driver (the
registers for the controllers from this series are not part of the
documentation, but the registers from the 8-bit capable controller
are, see page 76 and following from the S805 datasheet: [0])
Post by Ulf Hansson
Post by Martin Blumenstingl
- add support for the internal "mux" in this MMC controller (which allows
connecting up to three devices to the the controller - at the cost of
performance though since the controller can only process one request at
a time). The driver registers a new device for each sub-node, which is
then fed into the MMC framework to allow per-slot configuration using
devicetree (see the example in the documentation)
Unless there really is deployment for more than one slot on some
boards/SoCs, I would strongly suggest to *not* implement this.
Simply because of overhead and introduced complexity to the driver.
actually there are lots of devices out there which need to use two slots:
as mentioned above the S805 (Meson8b) SoC has two MMC controllers:
- one which is typically used for the SD card and the SDIO wireless
interface (the driver from this series handles this)
- the other one is typically connects to eMMC flash (as it supports
8-bit bus width - this controller is not related to the driver from
this series)

there are boards out there which use NAND flash instead of eMMC, but
the majority of the consumer devices (based on Amlogic SoCs) out there
uses eMMC flash.
we (unfortunately) have to support the internal mux since there are
three MMC devices (SD card, SDIO wifi and eMMC) but only two
controllers.

I agree with you that it adds extra complexity to the driver. I tried
to keep it as simple as possible - but I think we cannot remove it
(without "losing" access to one MMC devices on most boards)
Post by Ulf Hansson
Post by Martin Blumenstingl
- use the common clock framework internally for managing the MMC clock
(there is a fixed-factor clock in the controller which takes clk81 as
input and divides it's clock by two and a divider clock which takes
the result from the fixed-factor clock as input)
- support the regulators provided by the MMC framework
- support for GPIO-based card-detection and read-only-detection through
the MMC framework
- use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it
make sense (and thus the code easier to read)
- re-worked locking (based on the locking in dw_mmc as that also provides
multiple "MMC slots")
Lots of changes!
Before even start to review (or someone else), you really need to make
this review-able.
So, please, one change per patch - and make sure to write good
changelogs. Then I can start to review.
from the mainline tree's perspective this is a new driver:
Carlo (the original author) initially sent this driver for review more
than 14 months ago. unfortunately it was never merged since you
spotted some issues while reviewing that code, see [1].
I would have sent smaller patches for a driver which is already in mainline.

I also wanted to avoid extra complexity for the internal mux if it was
added later on (if we want to avoid breaking devicetree backwards
compatibility then the driver would have to support both: parsing from
the mmc node directly and parsing child "slot" nodes). we won't have
to change the DT bindings when the first version that will be
mainlined already has support for the mux

please let me know if there's anything I can do to make the code
easier to review.
Post by Ulf Hansson
Post by Martin Blumenstingl
- reading an OLD 256MiB SD card (which uses only a 1-bit bus) works fine
(sha1sum of the whole device matches with what I get on my PC's
card-reader)
- reading a somewhat more modern class 10 SD card and putting Arch Linux
ARM on it (and using that as root file system)
- it successfully detects the RTL8723BS SDIO wifi chip in my device (even
if the SD card is also enabled)
- reading a 128MiB file from the SD card while scanning wifi networks on
the RTL8723BS card does not seem to result in any corruption (sha1sum
of the read file seems to match)
- read speed of my class 10 SD card: ~15MiB/s
- (unfortunately I could NOT test downloading a file over wifi to the SD
card because the RTL8723BS driver refuses to see any wifi networks, but
that might be a problem on the RTL8723BS driver side since I don't get
any error and the driver has just landed a few weeks ago in staging)
[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/412136.html
[...]
Kind regards
Uffe
Regards,
Martin


[0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
[1] https://patchwork.kernel.org/patch/8444841/
Ulf Hansson
2017-05-11 09:39:21 UTC
Permalink
On 10 May 2017 at 21:22, Martin Blumenstingl
Post by Martin Blumenstingl
Hi Ulf,
Post by Ulf Hansson
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
This is the successor to Carlo Caione's "Add support for Amlogic Meson
MMC driver" series (v5) from [0].
- whether I've (ab)used the MMC framework properly (as this is my first
"larger" contribution to an MMC driver)
I take a look soonish.
thank you!
Post by Ulf Hansson
Post by Martin Blumenstingl
- I think I have improved the locking compared to Carlo's version,
however I'd still like feedback on whether this looks sane now or if I
can improve that even further
- renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls
the driver "aml_sdio" as there is a second MMC controller in these SoCs
which they call the "SDHC controller"). do the same with our driver to
I don't like to renaming drivers, just because there are a reference
kernel using a different name.
What's is really the difference between controllers? Why do they have
two variants?
the driver from this thread is for the "SDSC/SDHC/SDXC card and SDIO
interface with 1-bit and 4-bit data bus width supporting spec version
2.x/3.x/4.x DS/HS modes up to UHS-I SDR50" (quote taken from the S805
datasheet: [0])
the "other" controller is an "eMMC and MMC card interface with
1/4/8-bit data bus width supporting spec version 4.4x/4.5x HS200 (up
to 100MHz clock), compatible with standard iNAND interface" (again,
quote taken from the S805 datasheet: [0])
Post by Ulf Hansson
Can they be managed by the same driver?
Post by Martin Blumenstingl
avoid confusion once we add support for the second controller (which uses
a completely different register layout)
Besides that, do they behave differently in some other way?
both drivers/controllers have a totally different register layout - I
don't see any way how they both could be handled by one driver (the
registers for the controllers from this series are not part of the
documentation, but the registers from the 8-bit capable controller
are, see page 76 and following from the S805 datasheet: [0])
Okay, I am starting to understand. :-)

The spec in section 13 describes a controller supporting "MMC/SD/SDIO"
cards. However, there is yet another controller which @subject series
implement support for.
You don't happen to have a public datasheet for this controller as
well? If not, never mind.

Then, meson actually have three different MMC/SD/SDIO controllers, the
one in the S805 spec, the one supported by the recently up-streamed
meson-gx-mmc.c driver - and the one being supported in this series.
Right? And all of them are completely separate and non-compatible?
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
- add support for the internal "mux" in this MMC controller (which allows
connecting up to three devices to the the controller - at the cost of
performance though since the controller can only process one request at
a time). The driver registers a new device for each sub-node, which is
then fed into the MMC framework to allow per-slot configuration using
devicetree (see the example in the documentation)
Unless there really is deployment for more than one slot on some
boards/SoCs, I would strongly suggest to *not* implement this.
Simply because of overhead and introduced complexity to the driver.
- one which is typically used for the SD card and the SDIO wireless
interface (the driver from this series handles this)
- the other one is typically connects to eMMC flash (as it supports
8-bit bus width - this controller is not related to the driver from
this series)
Okay.
Post by Martin Blumenstingl
there are boards out there which use NAND flash instead of eMMC, but
the majority of the consumer devices (based on Amlogic SoCs) out there
uses eMMC flash.
we (unfortunately) have to support the internal mux since there are
three MMC devices (SD card, SDIO wifi and eMMC) but only two
controllers.
I see.
Post by Martin Blumenstingl
I agree with you that it adds extra complexity to the driver. I tried
to keep it as simple as possible - but I think we cannot remove it
(without "losing" access to one MMC devices on most boards)
Of course.
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
- use the common clock framework internally for managing the MMC clock
(there is a fixed-factor clock in the controller which takes clk81 as
input and divides it's clock by two and a divider clock which takes
the result from the fixed-factor clock as input)
- support the regulators provided by the MMC framework
- support for GPIO-based card-detection and read-only-detection through
the MMC framework
- use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it
make sense (and thus the code easier to read)
- re-worked locking (based on the locking in dw_mmc as that also provides
multiple "MMC slots")
Lots of changes!
Before even start to review (or someone else), you really need to make
this review-able.
So, please, one change per patch - and make sure to write good
changelogs. Then I can start to review.
Yes, I get it now.

I first thought this series was related to the recently up-streamed
meson-gx-mmc.c driver. Apologize for my ignorance.
Post by Martin Blumenstingl
Carlo (the original author) initially sent this driver for review more
than 14 months ago. unfortunately it was never merged since you
spotted some issues while reviewing that code, see [1].
Yes, I recall that now.
Post by Martin Blumenstingl
I would have sent smaller patches for a driver which is already in mainline.
Right.
Post by Martin Blumenstingl
I also wanted to avoid extra complexity for the internal mux if it was
added later on (if we want to avoid breaking devicetree backwards
compatibility then the driver would have to support both: parsing from
the mmc node directly and parsing child "slot" nodes). we won't have
to change the DT bindings when the first version that will be
mainlined already has support for the mux
please let me know if there's anything I can do to make the code
easier to review.
No worries, let me have a look as is!

[...]

Kind regards
Uffe
Martin Blumenstingl
2017-05-11 20:44:05 UTC
Permalink
Hi Ulf,
Post by Ulf Hansson
On 10 May 2017 at 21:22, Martin Blumenstingl
Post by Martin Blumenstingl
Hi Ulf,
Post by Ulf Hansson
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
This is the successor to Carlo Caione's "Add support for Amlogic Meson
MMC driver" series (v5) from [0].
- whether I've (ab)used the MMC framework properly (as this is my first
"larger" contribution to an MMC driver)
I take a look soonish.
thank you!
Post by Ulf Hansson
Post by Martin Blumenstingl
- I think I have improved the locking compared to Carlo's version,
however I'd still like feedback on whether this looks sane now or if I
can improve that even further
- renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls
the driver "aml_sdio" as there is a second MMC controller in these SoCs
which they call the "SDHC controller"). do the same with our driver to
I don't like to renaming drivers, just because there are a reference
kernel using a different name.
What's is really the difference between controllers? Why do they have
two variants?
the driver from this thread is for the "SDSC/SDHC/SDXC card and SDIO
interface with 1-bit and 4-bit data bus width supporting spec version
2.x/3.x/4.x DS/HS modes up to UHS-I SDR50" (quote taken from the S805
datasheet: [0])
the "other" controller is an "eMMC and MMC card interface with
1/4/8-bit data bus width supporting spec version 4.4x/4.5x HS200 (up
to 100MHz clock), compatible with standard iNAND interface" (again,
quote taken from the S805 datasheet: [0])
Post by Ulf Hansson
Can they be managed by the same driver?
Post by Martin Blumenstingl
avoid confusion once we add support for the second controller (which uses
a completely different register layout)
Besides that, do they behave differently in some other way?
both drivers/controllers have a totally different register layout - I
don't see any way how they both could be handled by one driver (the
registers for the controllers from this series are not part of the
documentation, but the registers from the 8-bit capable controller
are, see page 76 and following from the S805 datasheet: [0])
Okay, I am starting to understand. :-)
good to hear that my explanation made (at least some) sense!
Post by Ulf Hansson
The spec in section 13 describes a controller supporting "MMC/SD/SDIO"
implement support for.
You don't happen to have a public datasheet for this controller as
well? If not, never mind.
unfortunately the controller from @subject does not show up in any of
the public datasheets, but we have a header file from Amlogic which
documents it just as well as the datasheet (probably) would: [0]
Post by Ulf Hansson
Then, meson actually have three different MMC/SD/SDIO controllers, the
one in the S805 spec, the one supported by the recently up-streamed
meson-gx-mmc.c driver - and the one being supported in this series.
Right? And all of them are completely separate and non-compatible?
yes, unfortunately it seems that there are three different MMC
controller IP blocks from Amlogic

let me explain this a bit more in detail:
the code-names of the newer (64-bit) SoCs all start with GX (GXBB,
GXL, GXM), so we call these "Meson GX".
the code-names of the older (32-bit) SoCs (at least the ones which are
not too old) all start with Meson X (where X is a number: Meson6,
Meson8, Meson8b and Meson8m2), Amlogic's vendor code sometimes calls
these "MX" (or Meson MX)

the MMC IP blocks in Meson GX SoCs are easy: the same IP block is
embedded three times -> the driver for this is meson-gx-mmc.c (already
upstream)
the MMC IP blocks in Meson MX SoCs are more complicated: 1x 1/4-bit
which will be handled by the driver from @subject (meson-mx-sdio.c)
and 1x 1/4/8-bit for which there is only the vendor driver (plus the
description in the S805 datasheet) available.

so it looks like the older Meson MX SoCs may end up with two different
drivers, while there's a third driver (for a totally different IP
block) for the newer Meson GX SoCs.

[snip]
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
- use the common clock framework internally for managing the MMC clock
(there is a fixed-factor clock in the controller which takes clk81 as
input and divides it's clock by two and a divider clock which takes
the result from the fixed-factor clock as input)
- support the regulators provided by the MMC framework
- support for GPIO-based card-detection and read-only-detection through
the MMC framework
- use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it
make sense (and thus the code easier to read)
- re-worked locking (based on the locking in dw_mmc as that also provides
multiple "MMC slots")
Lots of changes!
Before even start to review (or someone else), you really need to make
this review-able.
So, please, one change per patch - and make sure to write good
changelogs. Then I can start to review.
Yes, I get it now.
I first thought this series was related to the recently up-streamed
meson-gx-mmc.c driver. Apologize for my ignorance.
should I put some more context/details in the description (maybe
something like: "this is a new driver/binding because the Meson MX
SoCs use a different IP block than the Meson GX SoCs (the IP block
from the latter is already supported by the meson-gx-mmc driver)"?
Post by Ulf Hansson
Post by Martin Blumenstingl
Carlo (the original author) initially sent this driver for review more
than 14 months ago. unfortunately it was never merged since you
spotted some issues while reviewing that code, see [1].
Yes, I recall that now.
the 14 month delay was one of the reasons why I decided to re-post it
only after all features were implemented
Post by Ulf Hansson
Post by Martin Blumenstingl
I would have sent smaller patches for a driver which is already in mainline.
Right.
Post by Martin Blumenstingl
I also wanted to avoid extra complexity for the internal mux if it was
added later on (if we want to avoid breaking devicetree backwards
compatibility then the driver would have to support both: parsing from
the mmc node directly and parsing child "slot" nodes). we won't have
to change the DT bindings when the first version that will be
mainlined already has support for the mux
please let me know if there's anything I can do to make the code
easier to review.
No worries, let me have a look as is!
okay, perfect! just take your time and let me know what needs to be changed.


Regards,
Martin


[0] https://github.com/endlessm/linux-meson/blob/master/arch/arm/mach-meson8/include/mach/sd.h#L231
Martin Blumenstingl
2017-05-24 20:37:40 UTC
Permalink
Hi Ulf,

On Thu, May 11, 2017 at 10:44 PM, Martin Blumenstingl
Post by Martin Blumenstingl
Hi Ulf,
Post by Ulf Hansson
On 10 May 2017 at 21:22, Martin Blumenstingl
Post by Martin Blumenstingl
Hi Ulf,
Post by Ulf Hansson
On 6 May 2017 at 19:18, Martin Blumenstingl
Post by Martin Blumenstingl
This is the successor to Carlo Caione's "Add support for Amlogic Meson
MMC driver" series (v5) from [0].
- whether I've (ab)used the MMC framework properly (as this is my first
"larger" contribution to an MMC driver)
I take a look soonish.
thank you!
Post by Ulf Hansson
Post by Martin Blumenstingl
- I think I have improved the locking compared to Carlo's version,
however I'd still like feedback on whether this looks sane now or if I
can improve that even further
- renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls
the driver "aml_sdio" as there is a second MMC controller in these SoCs
which they call the "SDHC controller"). do the same with our driver to
I don't like to renaming drivers, just because there are a reference
kernel using a different name.
What's is really the difference between controllers? Why do they have
two variants?
the driver from this thread is for the "SDSC/SDHC/SDXC card and SDIO
interface with 1-bit and 4-bit data bus width supporting spec version
2.x/3.x/4.x DS/HS modes up to UHS-I SDR50" (quote taken from the S805
datasheet: [0])
the "other" controller is an "eMMC and MMC card interface with
1/4/8-bit data bus width supporting spec version 4.4x/4.5x HS200 (up
to 100MHz clock), compatible with standard iNAND interface" (again,
quote taken from the S805 datasheet: [0])
Post by Ulf Hansson
Can they be managed by the same driver?
Post by Martin Blumenstingl
avoid confusion once we add support for the second controller (which uses
a completely different register layout)
Besides that, do they behave differently in some other way?
both drivers/controllers have a totally different register layout - I
don't see any way how they both could be handled by one driver (the
registers for the controllers from this series are not part of the
documentation, but the registers from the 8-bit capable controller
are, see page 76 and following from the S805 datasheet: [0])
Okay, I am starting to understand. :-)
good to hear that my explanation made (at least some) sense!
Post by Ulf Hansson
The spec in section 13 describes a controller supporting "MMC/SD/SDIO"
implement support for.
You don't happen to have a public datasheet for this controller as
well? If not, never mind.
the public datasheets, but we have a header file from Amlogic which
documents it just as well as the datasheet (probably) would: [0]
Post by Ulf Hansson
Then, meson actually have three different MMC/SD/SDIO controllers, the
one in the S805 spec, the one supported by the recently up-streamed
meson-gx-mmc.c driver - and the one being supported in this series.
Right? And all of them are completely separate and non-compatible?
yes, unfortunately it seems that there are three different MMC
controller IP blocks from Amlogic
the code-names of the newer (64-bit) SoCs all start with GX (GXBB,
GXL, GXM), so we call these "Meson GX".
the code-names of the older (32-bit) SoCs (at least the ones which are
not too old) all start with Meson X (where X is a number: Meson6,
Meson8, Meson8b and Meson8m2), Amlogic's vendor code sometimes calls
these "MX" (or Meson MX)
the MMC IP blocks in Meson GX SoCs are easy: the same IP block is
embedded three times -> the driver for this is meson-gx-mmc.c (already
upstream)
the MMC IP blocks in Meson MX SoCs are more complicated: 1x 1/4-bit
and 1x 1/4/8-bit for which there is only the vendor driver (plus the
description in the S805 datasheet) available.
so it looks like the older Meson MX SoCs may end up with two different
drivers, while there's a third driver (for a totally different IP
block) for the newer Meson GX SoCs.
[snip]
Post by Ulf Hansson
Post by Martin Blumenstingl
Post by Ulf Hansson
Post by Martin Blumenstingl
- use the common clock framework internally for managing the MMC clock
(there is a fixed-factor clock in the controller which takes clk81 as
input and divides it's clock by two and a divider clock which takes
the result from the fixed-factor clock as input)
- support the regulators provided by the MMC framework
- support for GPIO-based card-detection and read-only-detection through
the MMC framework
- use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it
make sense (and thus the code easier to read)
- re-worked locking (based on the locking in dw_mmc as that also provides
multiple "MMC slots")
Lots of changes!
Before even start to review (or someone else), you really need to make
this review-able.
So, please, one change per patch - and make sure to write good
changelogs. Then I can start to review.
Yes, I get it now.
I first thought this series was related to the recently up-streamed
meson-gx-mmc.c driver. Apologize for my ignorance.
should I put some more context/details in the description (maybe
something like: "this is a new driver/binding because the Meson MX
SoCs use a different IP block than the Meson GX SoCs (the IP block
from the latter is already supported by the meson-gx-mmc driver)"?
Post by Ulf Hansson
Post by Martin Blumenstingl
Carlo (the original author) initially sent this driver for review more
than 14 months ago. unfortunately it was never merged since you
spotted some issues while reviewing that code, see [1].
Yes, I recall that now.
the 14 month delay was one of the reasons why I decided to re-post it
only after all features were implemented
Post by Ulf Hansson
Post by Martin Blumenstingl
I would have sent smaller patches for a driver which is already in mainline.
Right.
Post by Martin Blumenstingl
I also wanted to avoid extra complexity for the internal mux if it was
added later on (if we want to avoid breaking devicetree backwards
compatibility then the driver would have to support both: parsing from
the mmc node directly and parsing child "slot" nodes). we won't have
to change the DT bindings when the first version that will be
mainlined already has support for the mux
please let me know if there's anything I can do to make the code
easier to review.
No worries, let me have a look as is!
okay, perfect! just take your time and let me know what needs to be changed.
I know that you're probably very busy, but I will still ask: did you
have time to look into this series yet (or do you have any ETA)?


Thank you!
Regards,
Martin
Daniel Drake
2017-05-27 02:48:57 UTC
Permalink
On Thu, May 11, 2017 at 2:44 PM, Martin Blumenstingl
Post by Martin Blumenstingl
the code-names of the older (32-bit) SoCs (at least the ones which are
not too old) all start with Meson X (where X is a number: Meson6,
Meson8, Meson8b and Meson8m2), Amlogic's vendor code sometimes calls
these "MX" (or Meson MX)
Just in case it helps you on any future adventures in the Amlogic
vendor code: I think this is not quite right. I believe that MX
specifically refers to Meson6 and no other SoC generation.

That said, if both MesonX (==Meson6) and Meson8/Meson8b all share the
same driver code, having this driver named "mx" makes sense to me,
especially because it helps distinguish from the GX* chips which
indeed have something rather different.

Daniel
Martin Blumenstingl
2017-05-27 14:25:51 UTC
Permalink
Hi Daniel,
Post by Daniel Drake
On Thu, May 11, 2017 at 2:44 PM, Martin Blumenstingl
Post by Martin Blumenstingl
the code-names of the older (32-bit) SoCs (at least the ones which are
not too old) all start with Meson X (where X is a number: Meson6,
Meson8, Meson8b and Meson8m2), Amlogic's vendor code sometimes calls
these "MX" (or Meson MX)
Just in case it helps you on any future adventures in the Amlogic
vendor code: I think this is not quite right. I believe that MX
specifically refers to Meson6 and no other SoC generation.
thanks for that hint - I guess that's due to the marketing name of the
Meson6 SoCs then (AML8726-MX). do you know if there's any codename
that includes the Meson6 and Meson8 (including Meson8b and Meson8m2)
families?
Post by Daniel Drake
That said, if both MesonX (==Meson6) and Meson8/Meson8b all share the
same driver code, having this driver named "mx" makes sense to me,
especially because it helps distinguish from the GX* chips which
indeed have something rather different.
from what I could read in Amlogic's GPL kernel sources the MMC
controller IP is also used in Meson6 SoCs (and possibly even older
generations). that said, I don't have any Meson6 hardware, which is
why I didn't add the "amlogic,meson6-sdio" binding.
so I guess using the "mx" name here should be OK (nevertheless, I'll
make sure that I don't use "MX" for anything that's
Meson8/Meson8b/Meson8m2 specific)


Regards,
Martin
Loading...