Re: [PATCH v3] mmc: meson: Add driver for the SD/MMC host found on Amlogic MesonX SoCs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Carlo,

Let me start by apologizing for the delayed reply!

On 20 June 2015 at 11:22, Carlo Caione <carlo@xxxxxxxxxx> wrote:
> From: Carlo Caione <carlo@xxxxxxxxxxxx>
>
> Add a driver for the SD/MMC host found on the Amlogic MesonX SoCs. It
> 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. It also supports
> SDSC, SDHC and SDXC memory card default speed.
>
> This patch adds also the binding documentation.
>
> Signed-off-by: Carlo Caione <carlo@xxxxxxxxxxxx>
> ---
>
> Changelog:
>
> v2:
>     * fix typo in commit message
>     * avoid the bounce buffer (max_segs == 1)
>
> v3:
>     * simplify meson_mmc_map_dma() and fix memory leak
> ---
>  .../devicetree/bindings/mmc/meson-mmc.txt          |  38 ++
>  drivers/mmc/host/Kconfig                           |   7 +
>  drivers/mmc/host/Makefile                          |   1 +
>  drivers/mmc/host/meson-mmc.c                       | 626 +++++++++++++++++++++
>  4 files changed, 672 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/meson-mmc.txt
>  create mode 100644 drivers/mmc/host/meson-mmc.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/meson-mmc.txt b/Documentation/devicetree/bindings/mmc/meson-mmc.txt
> new file mode 100644
> index 0000000..839569a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/meson-mmc.txt
> @@ -0,0 +1,38 @@
> +* Amlogic MesonX MMC controller
> +
> +The highspeed MMC host controller on Amlogic SoCs provides an interface
> +for MMC, SD, SDIO and SDHC types of memory cards.
> +
> +Supported maximum speeds are the ones of the eMMC standard 4.41 as well
> +as the speed of SD standard 2.0.
> +
> +Required properties:
> + - compatible : "amlogic,meson-mmc"
> + - reg : mmc controller base registers
> + - interrupts : mmc controller interrupt
> + - clocks : phandle to SDIO clock provider

SDIO clock? Specific for the SDIO as in the SDIO spec?

> +
> +Optional properties:
> + - meson,sdio-port : 0 for SDIO port A, 1 for SDIO port B. (default: 0)

Could you elaborate a bit on this. Will you ever use both ports
simultaneously? And why do you call them SDIO ports?

> + - for cd, bus-width and additional generic mmc parameters
> +   please refer to mmc.txt within this directory
> +
> +Examples:
> +       - Within .dtsi:

If some configuration are related to the SoC/MMC-controller and some
are board specific, let's describe that for each binding. Then you
don't need to state which kind of DT file those should remain in.

> +       mmc0: mmc@c1108c20 {
> +               compatible = "amlogic,meson-mmc";
> +               reg = <0xc1108c20 0x20>;
> +               interrupts = <0 28 1>;
> +               clocks = <&clkc CLKID_CLK81>;
> +               status = "disabled";
> +       };
> +
> +       - Within .dts:

Same comment as above.

> +       &mmc0 {
> +               status = "okay";
> +               pinctrl-0 = <&mmc0_sd_b_pins>;
> +               pinctrl-names = "default";
> +               meson,sdio-port = <1>;
> +               cd-gpios = <&gpio CARD_6 0>;
> +               cd-inverted;
> +       };
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index b1f837e..e61b8d6 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -764,6 +764,13 @@ config MMC_REALTEK_USB
>           Say Y here to include driver code to support SD/MMC card interface
>           of Realtek RTS5129/39 series card reader
>
> +config MMC_MESON
> +       tristate "Amlogic MesonX SD/MMC Host Controller support"
> +       depends on ARCH_MESON
> +       help
> +         This selects support for the SD/MMC Host Controller on
> +         Amlogic MesonX SoCs.
> +
>  config MMC_SUNXI
>         tristate "Allwinner sunxi SD/MMC Host Controller support"
>         depends on ARCH_SUNXI
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index e3ab5b9..c0455e7 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_MMC_VUB300)      += vub300.o
>  obj-$(CONFIG_MMC_USHC)         += ushc.o
>  obj-$(CONFIG_MMC_WMT)          += wmt-sdmmc.o
>  obj-$(CONFIG_MMC_MOXART)       += moxart-mmc.o
> +obj-$(CONFIG_MMC_MESON)                += meson-mmc.o
>  obj-$(CONFIG_MMC_SUNXI)                += sunxi-mmc.o
>  obj-$(CONFIG_MMC_USDHI6ROL0)   += usdhi6rol0.o
>  obj-$(CONFIG_MMC_TOSHIBA_PCI)  += toshsd.o
> diff --git a/drivers/mmc/host/meson-mmc.c b/drivers/mmc/host/meson-mmc.c
> new file mode 100644
> index 0000000..81a050e
> --- /dev/null
> +++ b/drivers/mmc/host/meson-mmc.c
> @@ -0,0 +1,626 @@
> +/*
> + * mesonsd.c - Meson SDH Controller
> + *
> + * Copyright (C) 2015 Endless Mobile, Inc.
> + * Author: Carlo Caione <carlo@xxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#define DRIVER_NAME    "meson-mmc"
> +
> +#define DEBUG
> +
> +#include <linux/clk.h>
> +#include <linux/delay.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/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sdio.h>
> +#include <linux/mmc/slot-gpio.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +
> +#define SDIO_ARGU              (0x00)
> +#define SDIO_SEND              (0x04)
> +#define SDIO_CONF              (0x08)
> +#define SDIO_IRQS              (0x0c)
> +#define SDIO_IRQC              (0x10)
> +#define SDIO_MULT              (0x14)
> +#define SDIO_ADDR              (0x18)
> +#define SDIO_EXT               (0x1c)
> +
> +#define REG_IRQS_RESP_CRC7     BIT(5)
> +#define REG_IRQS_RD_CRC16      BIT(6)
> +#define REG_IRQS_WR_CRC16      BIT(7)
> +#define REG_IRQS_CMD_INT       BIT(9)
> +
> +#define REG_IRQC_ARC_CMD_INT   BIT(4)
> +#define REG_IRQC_SOFT_RESET    BIT(15)
> +
> +#define REG_CONF_WR_CRC_S      (29)
> +#define REG_CONF_WR_NWR_S      (23)
> +#define REG_CONF_M_END_S       (21)
> +#define REG_CONF_ARGU_BITS_S   (12)
> +#define REG_CONF_CLK_DIV_M     (0x3ff)
> +#define REG_CONF_BUS_WIDTH     BIT(20)
> +
> +#define REG_MULT_PORT_SEL_M    (0x3)
> +#define REG_MULT_RD_INDEX_M    (0x0f)
> +#define REG_MULT_RD_INDEX_S    (12)
> +#define REG_MULT_WR_RD_OUT_IND BIT(8)
> +
> +#define REG_SEND_CMD_COMMAND_M (0xff)
> +#define REG_SEND_CMD_RESP_S    (8)
> +#define REG_SEND_RESP_NO_CRC7  BIT(16)
> +#define REG_SEND_RESP_HAVE_DATA        BIT(17)
> +#define REG_SEND_RESP_CRC7_F_8 BIT(18)
> +#define REG_SEND_CHECK_BUSY_D0 BIT(19)
> +#define REG_SEND_CMD_SEND_DATA BIT(20)
> +#define REG_SEND_REP_PACK_N_M  (0xff)
> +#define REG_SEND_REP_PACK_N_S  (24)
> +
> +#define REG_EXT_DAT_RW_NUM_M   (0x3fff)
> +#define REG_EXT_DAT_RW_NUM_S   (16)
> +
> +#define CLK_DIV                        (0x1f4)
> +
> +#define SDIO_BOUNCE_REQ_SIZE   (128 * 1024)
> +
> +enum mmcif_state {
> +       STATE_IDLE,
> +       STATE_REQUEST,
> +       STATE_IOS,
> +       STATE_TIMEOUT,
> +       STATE_STOP,
> +};

Hmm, I wonder if these states are really needed?

Have you considered if protection of the host->mrq pointer within
locks, would be sufficient instead of having a state machine?

> +
> +struct meson_mmc_host {
> +       struct mmc_host         *mmc;
> +       struct mmc_request      *mrq;
> +       struct delayed_work     timeout_work;
> +       struct clk              *clk_sdio;

Do you have a specific clock for sdio?

Moreover I think you are using "SDIO" and "sdio" from many places in
the driver. It's a bit confusing as I guess you don't refer to the
SDIO spec!? Please consider to use a different naming when applicable.

> +       spinlock_t              lock;
> +       void __iomem            *base;
> +       long                    timeout;
> +       int                     irq;
> +       int                     ferror;

"ferror" why not "error"?

> +       unsigned int            bus_width;
> +       unsigned int            port;
> +       enum mmcif_state        state;
> +};
> +
> +static int meson_mmc_clk_set_rate(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       struct meson_mmc_host *host = mmc_priv(mmc);
> +       unsigned long clk_rate;
> +       unsigned int clk_ios = ios->clock;
> +       unsigned int clk_div;
> +       u32 conf_reg;
> +
> +       clk_rate = clk_get_rate(host->clk_sdio) / 2;
> +       if (clk_rate < 0) {
> +               dev_err(mmc_dev(mmc), "cannot get clock rate\n");
> +               return -EINVAL;
> +       }
> +
> +       clk_div = clk_rate / clk_ios - !(clk_rate % clk_ios);
> +
> +       conf_reg = readl(host->base + SDIO_CONF);
> +       conf_reg &= ~REG_CONF_CLK_DIV_M;
> +       conf_reg |= clk_div;
> +       writel(conf_reg, host->base + SDIO_CONF);
> +
> +       dev_dbg(mmc_dev(mmc), "clk_ios: %d, clk_div: %d, clk_rate: %ld\n",
> +                       clk_ios, clk_div, clk_rate);
> +
> +       return 0;
> +}
> +
> +static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       struct meson_mmc_host *host = mmc_priv(mmc);
> +       u32 reg;
> +
> +       if (host->state != STATE_IDLE) {
> +               dev_dbg(mmc_dev(mmc), "%s() rejected, state %u\n",
> +                       __func__, host->state);
> +               return;
> +       }
> +
> +       host->state = STATE_IOS;
> +
> +       reg = readl(host->base + SDIO_CONF);
> +
> +       switch (ios->bus_width) {
> +       case MMC_BUS_WIDTH_1:
> +               host->bus_width = 0;

bus_width is already stored in the struct mmc_ios, could you use that instead?

> +               reg &= ~REG_CONF_BUS_WIDTH;
> +               dev_dbg(mmc_dev(mmc), "bus width: 1bit\n");

The mmc core provides information of the currently used ios settings
via debugfs. I don't think you need to log this here as well.

> +               break;
> +       case MMC_BUS_WIDTH_4:
> +               host->bus_width = 1;
> +               reg |= REG_CONF_BUS_WIDTH;
> +               dev_dbg(mmc_dev(mmc), "bus width: 4bit\n");
> +               break;
> +       case MMC_BUS_WIDTH_8:
> +       default:
> +               dev_err(mmc_dev(mmc), "SDIO controller doesn't support 8bit data bus\n");

SDIO controller? Should this be something like "the meson controller..."

> +               host->ferror = -EINVAL;
> +               return;
> +       }
> +
> +       writel(reg, host->base + SDIO_CONF);
> +
> +       if (ios->clock && ios->power_mode)
> +               host->ferror = meson_mmc_clk_set_rate(mmc, ios);
> +
> +       host->state = STATE_IDLE;
> +}
> +
> +static void meson_mmc_soft_reset(struct meson_mmc_host *host)
> +{
> +       u32 irqc;
> +
> +       irqc = readl(host->base + SDIO_IRQC);
> +       irqc |= REG_IRQC_SOFT_RESET;
> +       writel(irqc, host->base + SDIO_IRQC);
> +       udelay(2);
> +}
> +
> +static void meson_mmc_start_cmd(struct mmc_host *mmc,
> +                               struct mmc_request *mrq)
> +{
> +       struct meson_mmc_host *host = mmc_priv(mmc);
> +       unsigned int pack_size;
> +       u32 irqc, irqs, mult;
> +       u32 send = 0;
> +       u32 ext = 0;
> +
> +       switch (mmc_resp_type(mrq->cmd)) {
> +       case MMC_RSP_R1:
> +       case MMC_RSP_R1B:
> +       case MMC_RSP_R3:
> +               send |= (45 << REG_SEND_CMD_RESP_S);
> +               break;
> +       case MMC_RSP_R2:
> +               send |= (133 << REG_SEND_CMD_RESP_S);
> +               send |= REG_SEND_RESP_CRC7_F_8;
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       if (!(mrq->cmd->flags & MMC_RSP_CRC))
> +               send |= REG_SEND_RESP_NO_CRC7;
> +
> +       if (mrq->cmd->flags & MMC_RSP_BUSY)
> +               send |= REG_SEND_CHECK_BUSY_D0;
> +
> +       if (mrq->data) {
> +               send &= ~(REG_SEND_REP_PACK_N_M << REG_SEND_REP_PACK_N_S);
> +               send |= ((mrq->data->blocks - 1) << REG_SEND_REP_PACK_N_S);
> +
> +               ext &= ~(REG_EXT_DAT_RW_NUM_M << REG_EXT_DAT_RW_NUM_S);
> +               if (host->bus_width)
> +                       pack_size = mrq->data->blksz * 8 + (16 - 1) * 4;
> +               else
> +                       pack_size = mrq->data->blksz * 8 + (16 - 1);
> +               ext |= (pack_size << REG_EXT_DAT_RW_NUM_S);
> +
> +               if (mrq->data->flags & MMC_DATA_WRITE)
> +                       send |= REG_SEND_CMD_SEND_DATA;
> +               else
> +                       send |= REG_SEND_RESP_HAVE_DATA;
> +       }
> +
> +       send &= ~REG_SEND_CMD_COMMAND_M;
> +       send |= (0x40 | mrq->cmd->opcode);
> +
> +       meson_mmc_soft_reset(host);
> +
> +       irqc = readl(host->base + SDIO_IRQC);
> +       irqc |= REG_IRQC_ARC_CMD_INT;
> +
> +       irqs = readl(host->base + SDIO_IRQS);
> +       irqs |= REG_IRQS_CMD_INT;
> +
> +       mult = readl(host->base + SDIO_MULT);
> +       mult &= ~REG_MULT_PORT_SEL_M;
> +       mult |= host->port;
> +       mult |= (1 << 31);
> +       writel(mult, host->base + SDIO_MULT);
> +       writel(irqs, host->base + SDIO_IRQS);
> +       writel(irqc, host->base + SDIO_IRQC);
> +
> +       writel(mrq->cmd->arg, host->base + SDIO_ARGU);
> +       writel(ext, host->base + SDIO_EXT);
> +       writel(send, host->base + SDIO_SEND);
> +}
> +
> +static int meson_mmc_map_dma(struct meson_mmc_host *host,
> +                            struct mmc_data *data,
> +                            unsigned int flags)
> +{      u32 i, dma_len;
> +       struct scatterlist *sg = data->sg;
> +
> +       if (sg->offset & 3 || sg->length & 3) {
> +               dev_err(mmc_dev(host->mmc),
> +                       "unaligned scatterlist: os %x length %d\n",
> +                       sg->offset, sg->length);
> +               return -EINVAL;
> +       }
> +
> +       dma_len = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> +                            ((data->flags & MMC_DATA_READ) ?
> +                            DMA_FROM_DEVICE : DMA_TO_DEVICE));
> +       if (dma_len == 0) {
> +               dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> +       struct meson_mmc_host *host = mmc_priv(mmc);
> +       struct mmc_command *cmd = mrq->cmd;
> +       struct mmc_data *data = mrq->data;
> +       unsigned long flags;
> +       int ret;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +
> +       if (host->state != STATE_IDLE) {
> +               dev_dbg(mmc_dev(mmc), "%s() rejected, state %u\n",
> +                       __func__, host->state);
> +               mrq->cmd->error = -EAGAIN;
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               mmc_request_done(mmc, mrq);
> +               return;
> +       }
> +
> +       if (host->ferror) {
> +               cmd->error = host->ferror;
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               mmc_request_done(mmc, mrq);
> +               return;
> +       }
> +
> +       dev_dbg(mmc_dev(mmc), "CMD%d(%08x) arg %x len %d flags %08x\n",
> +               cmd->opcode & 0x3f, cmd->opcode, cmd->arg,
> +               mrq->data ? mrq->data->blksz * mrq->data->blocks : 0,
> +               mrq->cmd->flags);
> +
> +       /* Filter out CMD 5/52/53 */
> +       if (cmd->opcode == SD_IO_SEND_OP_COND ||
> +           cmd->opcode == SD_IO_RW_DIRECT ||
> +           cmd->opcode == SD_IO_RW_EXTENDED) {
> +               dev_dbg(mmc_dev(host->mmc), "CMD%d not supported\n",
> +                       cmd->opcode);
> +               cmd->error = -EINVAL;
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               mmc_request_done(mmc, mrq);
> +               return;
> +       }

I think it's time that we invent a new mmc host cap bit, which
indicates whether the host supports the SDIO spec or not. Something
like MMC_CAP_NO_SDIO, which then would tell the mmc core to not send
SDIO specfic commands during initialization. I think it would speed up
initialization as well.

Typically your driver will set this new cap and thus you don't need
the above validation. Does it make sense?

> +
> +       host->state = STATE_REQUEST;
> +
> +       if (data) {
> +               ret = meson_mmc_map_dma(host, data, data->flags);
> +               if (ret < 0) {
> +                       dev_err(mmc_dev(mmc), "map DMA failed\n");
> +                       cmd->error = ret;
> +                       data->error = ret;
> +                       host->state = STATE_IDLE;
> +                       spin_unlock_irqrestore(&host->lock, flags);
> +                       mmc_request_done(mmc, mrq);
> +                       return;
> +               }
> +               writel(sg_dma_address(data->sg), host->base + SDIO_ADDR);
> +       }
> +
> +       host->mrq = mrq;
> +       schedule_delayed_work(&host->timeout_work, host->timeout);

Perhaps you should schedule the timeout_work outside the spinlocks and
after the meson_mmc_start_cmd() has been invoked!?

> +       meson_mmc_start_cmd(mmc, mrq);
> +
> +       spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +static irqreturn_t meson_mmc_irq(int irq, void *data)
> +{
> +       struct meson_mmc_host *host = (void *) data;
> +       struct mmc_request *mrq = host->mrq;
> +       u32 irqs;
> +
> +       irqs = readl(host->base + SDIO_IRQS);
> +       if (mrq && (irqs & REG_IRQS_CMD_INT))
> +               return IRQ_WAKE_THREAD;
> +
> +       return IRQ_HANDLED;
> +}
> +
> +void meson_mmc_read_response(struct meson_mmc_host *host)
> +{
> +       struct mmc_command *cmd = host->mrq->cmd;
> +       u32 mult;
> +       int i, resp[4] = { 0 };
> +
> +       mult = readl(host->base + SDIO_MULT);
> +       mult |= REG_MULT_WR_RD_OUT_IND;
> +       mult &= ~(REG_MULT_RD_INDEX_M << REG_MULT_RD_INDEX_S);
> +       writel(mult, host->base + SDIO_MULT);
> +
> +       if (cmd->flags & MMC_RSP_136) {
> +               for (i = 0; i <= 3; i++)
> +                       resp[3 - i] = readl(host->base + 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] = readl(host->base + SDIO_ARGU);
> +       }
> +}
> +
> +struct mmc_command meson_mmc_cmd = {
> +       .opcode = MMC_STOP_TRANSMISSION,
> +       .flags  = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC,
> +};
> +
> +struct mmc_request meson_mmc_stop = {
> +       .cmd = &meson_mmc_cmd,
> +};
> +

Both the above declartions isn't needed. See further comment below,
where the "meson_mmc_stop" is used.

> +static irqreturn_t meson_mmc_irq_thread(int irq, void *irq_data)
> +{
> +       struct meson_mmc_host *host = (void *) irq_data;
> +       struct mmc_data *data;
> +       unsigned long flags;
> +       struct mmc_request *mrq;
> +       u32 irqs, send;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +
> +       cancel_delayed_work_sync(&host->timeout_work);

I don't think this a good idea as you are within spin_lock_irqsave().

> +       mrq = host->mrq;
> +       data = mrq->data;
> +
> +       if (!mrq) {
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               return IRQ_HANDLED;
> +       }
> +
> +       if (host->state == STATE_STOP)
> +               goto out;
> +
> +       if (host->state != STATE_REQUEST) {
> +               dev_dbg(mmc_dev(host->mmc), "%s() rejected, state %u\n",
> +                       __func__, host->state);
> +               mrq->cmd->error = -EAGAIN;
> +               goto out;
> +       }
> +
> +       irqs = readl(host->base + SDIO_IRQS);
> +       send = readl(host->base + SDIO_SEND);
> +
> +       mrq->cmd->error = 0;
> +
> +       if (!data) {
> +               if (!((irqs & REG_IRQS_RESP_CRC7) ||
> +                     (send & REG_SEND_RESP_NO_CRC7)))
> +                       mrq->cmd->error = -EILSEQ;
> +               else
> +                       meson_mmc_read_response(host);
> +       } else {
> +               if (!((irqs & REG_IRQS_RD_CRC16) ||
> +                     (irqs & REG_IRQS_WR_CRC16))) {
> +                       mrq->cmd->error = -EILSEQ;
> +               } else {
> +                       data->bytes_xfered = data->blksz * data->blocks;
> +                       dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> +                                    ((data->flags & MMC_DATA_READ) ?
> +                                    DMA_FROM_DEVICE : DMA_TO_DEVICE));
> +
> +                       if (mrq->stop) {
> +                               host->state = STATE_STOP;
> +                               meson_mmc_start_cmd(host->mmc, &meson_mmc_stop);

mrq->stop already contains a struct mmc_command. I think you shall use
that instead of you local copy in meson_mmc_stop.

> +                               spin_unlock_irqrestore(&host->lock, flags);
> +                               return IRQ_HANDLED;
> +                       }
> +               }
> +       }
> +
> +out:
> +       host->state = STATE_IDLE;
> +       host->mrq = NULL;
> +       spin_unlock_irqrestore(&host->lock, flags);
> +       mmc_request_done(host->mmc, mrq);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void meson_mmc_timeout(struct work_struct *work)
> +{
> +       struct meson_mmc_host *host = container_of(work,
> +                                                  struct meson_mmc_host,
> +                                                  timeout_work.work);
> +       struct mmc_request *mrq = host->mrq;
> +       unsigned long flags;
> +       u32 irqc;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +
> +       if (host->state == STATE_IDLE) {
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               return;
> +       }
> +
> +       dev_err(mmc_dev(host->mmc), "Timeout on CMD%u\n", mrq->cmd->opcode);
> +
> +       host->state = STATE_TIMEOUT;
> +
> +       irqc = readl(host->base + SDIO_IRQC);
> +       irqc &= ~REG_IRQC_ARC_CMD_INT;
> +       writel(irqc, host->base + SDIO_IRQC);
> +
> +       mrq->cmd->error = -ETIMEDOUT;
> +
> +       host->state = STATE_IDLE;
> +       host->mrq = NULL;
> +       spin_unlock_irqrestore(&host->lock, flags);
> +
> +       mmc_request_done(host->mmc, mrq);
> +}
> +
> +static void meson_mmc_reset(struct mmc_host *mmc)
> +{
> +       struct meson_mmc_host *host = mmc_priv(mmc);
> +       u32 reg = 0;
> +
> +       dev_dbg(mmc_dev(mmc), "resetting mmc controller\n");
> +
> +       reg =  (2 << REG_CONF_WR_CRC_S);
> +       reg |= (2 << REG_CONF_WR_NWR_S);
> +       reg |= (3 << REG_CONF_M_END_S);
> +       reg |= (39 << REG_CONF_ARGU_BITS_S);
> +
> +       reg |= CLK_DIV;
> +       writel(reg, host->base + SDIO_CONF);
> +
> +       reg = readl(host->base + SDIO_IRQS);
> +       reg |= (REG_IRQS_CMD_INT);
> +       writel(reg, host->base + SDIO_IRQS);
> +}
> +
> +static struct mmc_host_ops meson_mmc_ops = {
> +       .request         = meson_mmc_request,
> +       .set_ios         = meson_mmc_set_ios,
> +       .get_cd          = mmc_gpio_get_cd,
> +};
> +
> +static int meson_mmc_probe(struct platform_device *pdev)
> +{
> +       struct mmc_host *mmc;
> +       struct meson_mmc_host *host;
> +       struct device_node *node = pdev->dev.of_node;
> +       int ret, sdio_port;
> +
> +       mmc = mmc_alloc_host(sizeof(struct meson_mmc_host), &pdev->dev);
> +       if (!mmc) {
> +               dev_err(&pdev->dev, "mmc alloc host failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       host = mmc_priv(mmc);
> +       host->mmc = mmc;
> +       spin_lock_init(&host->lock);
> +
> +       host->base = devm_ioremap_resource(&pdev->dev,
> +                               platform_get_resource(pdev, IORESOURCE_MEM, 0));
> +       if (IS_ERR(host->base)) {
> +               ret = PTR_ERR(host->base);
> +               goto error_free_host;
> +       }
> +
> +       host->irq = platform_get_irq(pdev, 0);
> +       ret = devm_request_threaded_irq(&pdev->dev, host->irq, meson_mmc_irq,
> +                       meson_mmc_irq_thread, 0, "meson_mmc", host);
> +       if (ret)
> +               goto error_free_host;
> +
> +       host->clk_sdio = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(host->clk_sdio)) {
> +               dev_err(&pdev->dev, "Could not get clk81 clock\n");

No need to log this, it's handled by the clock framework.

> +               ret = PTR_ERR(host->clk_sdio);
> +               goto error_free_host;
> +       }
> +
> +       mmc->ops = &meson_mmc_ops;
> +
> +       /* we do not support scatter lists in hardware */
> +       mmc->max_segs = 1;
> +       mmc->max_req_size = SDIO_BOUNCE_REQ_SIZE;
> +       mmc->max_seg_size = mmc->max_req_size;
> +       mmc->max_blk_count = 256;
> +       mmc->max_blk_size = mmc->max_req_size / mmc->max_blk_count;
> +       mmc->f_min = 300000;
> +       mmc->f_max = 50000000;
> +       mmc->caps |= MMC_CAP_4_BIT_DATA;
> +       mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED;
> +       mmc->ocr_avail = MMC_VDD_33_34;
> +
> +       INIT_DELAYED_WORK(&host->timeout_work, meson_mmc_timeout);
> +       host->timeout = msecs_to_jiffies(2000);
> +       host->port = 0;
> +
> +       if (!of_property_read_u32(node, "meson,sdio-port", &sdio_port))
> +               host->port = sdio_port;
> +
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               goto error_free_host;
> +
> +       platform_set_drvdata(pdev, mmc);
> +
> +       meson_mmc_reset(mmc);
> +       mmc_add_host(mmc);
> +
> +       dev_info(&pdev->dev, "base:0x%p irq:%u port:%u\n",
> +                host->base, host->irq, host->port);
> +
> +       return 0;
> +
> +error_free_host:
> +       mmc_free_host(mmc);
> +
> +       return ret;
> +}
> +
> +static int meson_mmc_remove(struct platform_device *pdev)
> +{
> +       struct mmc_host *mmc = platform_get_drvdata(pdev);
> +       struct meson_mmc_host *host = mmc_priv(mmc);
> +
> +       mmc_remove_host(mmc);
> +       disable_irq(host->irq);
> +
> +       mmc_free_host(mmc);
> +
> +       cancel_delayed_work_sync(&host->timeout_work);

This doesn't seem right, espeically since the work function are
accessing the struct mmc_host. Somehow you need to protect this from
happen, but I don't think you are doing that.

> +
> +       return 0;
> +}
> +
> +static const struct of_device_id meson_mmc_of_match[] = {
> +       { .compatible = "amlogic,meson-mmc", },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, meson_mmc_of_match);
> +
> +static struct platform_driver meson_mmc_driver = {
> +       .probe   = meson_mmc_probe,
> +       .remove  = meson_mmc_remove,
> +       .driver  = {
> +               .name = DRIVER_NAME,
> +               .of_match_table = of_match_ptr(meson_mmc_of_match),
> +       },
> +};
> +
> +module_platform_driver(meson_mmc_driver);
> +
> +MODULE_DESCRIPTION("Meson Secure Digital Host Driver");
> +MODULE_AUTHOR("Carlo Caione <carlo@xxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.1.4
>

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux