| Message ID | 20170506171857.16492-3-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 6, 2017 at 7:18 PM, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > From: Carlo Caione <carlo@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 <carlo@endlessm.com> > Signed-off-by: Martin Blumenstingl <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 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 > + 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] -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 May 2017 at 19:18, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > From: Carlo Caione <carlo@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). 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. > > Signed-off-by: Carlo Caione <carlo@endlessm.com> > Signed-off-by: Martin Blumenstingl <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 <carlo@endlessm.com> > + * Copyright (C) 2017 Martin Blumenstingl <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; Could you please explain what these locks are being used for? > + > + 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. > + 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. [...] > +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); > + } 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. > +} > + > +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; 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. > + > + 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. > + > + 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. > + > + mod_timer(&slot->host->cmd_timeout, jiffies + timeout); > +} > + [...] > +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. > + > + 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; 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? > + > + spin_unlock_irqrestore(&slot->host->irq_lock, irqflags); > +} > + [...] > + > +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. [...] > + > +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. > +} > + > +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); 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". > + 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; > + > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! On Mon, May 29, 2017 at 4:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 6 May 2017 at 19:18, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> From: Carlo Caione <carlo@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). > > 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 >> >> Signed-off-by: Carlo Caione <carlo@endlessm.com> >> Signed-off-by: Martin Blumenstingl <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 <carlo@endlessm.com> >> + * Copyright (C) 2017 Martin Blumenstingl <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; > > Could you please explain what these locks are being used for? > >> + >> + 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 > 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)! >> + 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 > [...] > >> +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); >> + } > > 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? >> +} >> + >> +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; > > 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? >> + >> + 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 >> + >> + 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!) >> + >> + mod_timer(&slot->host->cmd_timeout, jiffies + timeout); >> +} >> + > > [...] > >> +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? >> + >> + 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; > > 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) >> + >> + spin_unlock_irqrestore(&slot->host->irq_lock, irqflags); >> +} >> + > > [...] > >> + >> +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) > [...] > >> + >> +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. > >> +} >> + >> +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); > > 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 >> + 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 >> + >> + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> [...] >> >>> +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); >>> + } >> >> 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. [...] >>> + 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. [...] >> >>> +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? 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. [...] >>> +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. [...] >> [...] >> >> 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. > > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On Wed, Jun 7, 2017 at 6:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> [...] >>> >>>> +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); >>>> + } >>> >>> 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 > [...] > >>>> + 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"? > [...] > >>> >>>> +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? > > 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? > [...] > >>>> +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) > [...] > >>> [...] >>> >>> 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) >> >> 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. > >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> [...] >> >>>>> + 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. [...] >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 On Mon, Jun 19, 2017 at 1:50 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >>> [...] >>> >>>>>> + 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? > 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 > 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? > [...] > >>> >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 July 2017 at 23:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > 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 > > On Mon, Jun 19, 2017 at 1:50 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> >>>> [...] >>>> >>>>>>> + 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). > >> 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 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. > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On Wed, Jul 12, 2017 at 3:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 11 July 2017 at 23:23, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> 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 >> >> On Mon, Jun 19, 2017 at 1:50 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> >>>>> [...] >>>>> >>>>>>>> + 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) >> >>> 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 > > 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) >> >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <carlo@endlessm.com> + * Copyright (C) 2017 Martin Blumenstingl <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 <carlo@endlessm.com>"); +MODULE_LICENSE("GPL v2");