[PATCH v2 2/2] MMC: meson: initial support for GXBB platforms
Kevin Hilman
khilman at baylibre.com
Wed Sep 7 16:02:15 PDT 2016
Stephen Boyd <sboyd at codeaurora.org> writes:
> On 08/03, Kevin Hilman wrote:
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7304d2e37a98..3762e46811f3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -972,6 +972,7 @@ F: arch/arm/mach-meson/
>> F: arch/arm/boot/dts/meson*
>> F: arch/arm64/boot/dts/amlogic/
>> F: drivers/pinctrl/meson/
>> +F: drivers/mmc/host/meson*
>
> Weird spacing here?
>
>> N: meson
>>
>> ARM/Annapurna Labs ALPINE ARCHITECTURE
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 0aa484c10c0a..4c3d091f7548 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -332,6 +332,16 @@ config MMC_SDHCI_IPROC
>>
>> If unsure, say N.
>>
>> +config MMC_MESON_GXBB
>> + tristate "Amlogic S905/GXBB SD/MMC Host Controller support"
>> + depends on ARCH_MESON && MMC
>> + help
>> + This selects support for the Amlogic SD/MMC Host Controller
>> + found on the S905/GXBB family of SoCs. This controller is
>> + MMC 5.1 compliant and support SD, eMMC and SDIO interfaces.
>
> s/support/supports/
>
>> +
>> + If you have a controller with this interface, say Y here.
>> +
>> config MMC_MOXART
>> tristate "MOXART SD/MMC Host Controller support"
>> depends on ARCH_MOXART && MMC
>> diff --git a/drivers/mmc/host/meson-gxbb.c b/drivers/mmc/host/meson-gxbb.c
>> new file mode 100644
>> index 000000000000..10eac41cf31a
>> --- /dev/null
>> +++ b/drivers/mmc/host/meson-gxbb.c
>> @@ -0,0 +1,918 @@
>> +/*
>> + * This file is provided under a dual BSD/GPLv2 license. When using or
>> + * redistributing this file, you may do so under either license.
>> + *
>> + * GPL LICENSE SUMMARY
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Kevin Hilman <khilman at baylibre.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of version 2 of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + * The full GNU General Public License is included in this distribution
>> + * in the file called COPYING.
>> + *
>> + * BSD LICENSE
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Kevin Hilman <khilman at baylibre.com>
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * * Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * * Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in
>> + * the documentation and/or other materials provided with the
>> + * distribution.
>> + * * Neither the name of Intel Corporation nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>
> Unused?
>
>> +#include <linux/init.h>
>> +#include <linux/device.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/ioport.h>
>> +#include <linux/regmap.h>
>
> Unused?
>
>> +#include <linux/spinlock.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/mmc/mmc.h>
>> +#include <linux/mmc/sdio.h>
>> +#include <linux/mmc/slot-gpio.h>
>> +#include <linux/io.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +
>> +#define DRIVER_NAME "meson-gxbb-mmc"
>> +
>> +#define SD_EMMC_CLOCK 0x0
>> +#define CLK_DIV_SHIFT 0
>> +#define CLK_DIV_WIDTH 6
>> +#define CLK_DIV_MASK 0x3f
>> +#define CLK_DIV_MAX 63
>> +#define CLK_SRC_SHIFT 6
>> +#define CLK_SRC_WIDTH 2
>> +#define CLK_SRC_MASK 0x3
>> +#define CLK_SRC_XTAL 0 /* external crystal */
>> +#define CLK_SRC_XTAL_RATE 24000000
>> +#define CLK_SRC_PLL 1 /* FCLK_DIV2 */
>> +#define CLK_SRC_PLL_RATE 1000000000
>> +#define CLK_PHASE_SHIFT 8
>> +#define CLK_PHASE_MASK 0x3
>> +#define CLK_PHASE_0 0
>> +#define CLK_PHASE_90 1
>> +#define CLK_PHASE_180 2
>> +#define CLK_PHASE_270 3
>> +#define CLK_ALWAYS_ON BIT(24)
>> +
>> +#define SD_EMMC_DElAY 0x4
>> +#define SD_EMMC_ADJUST 0x8
>> +#define SD_EMMC_CALOUT 0x10
>> +#define SD_EMMC_START 0x40
>> +#define START_DESC_INIT BIT(0)
>> +#define START_DESC_BUSY BIT(1)
>> +#define START_DESC_ADDR_SHIFT 2
>> +#define START_DESC_ADDR_MASK (~0x3)
>> +
>> +#define SD_EMMC_CFG 0x44
>> +#define CFG_BUS_WIDTH_SHIFT 0
>> +#define CFG_BUS_WIDTH_MASK 0x3
>> +#define CFG_BUS_WIDTH_1 0x0
>> +#define CFG_BUS_WIDTH_4 0x1
>> +#define CFG_BUS_WIDTH_8 0x2
>> +#define CFG_DDR BIT(2)
>> +#define CFG_BLK_LEN_SHIFT 4
>> +#define CFG_BLK_LEN_MASK 0xf
>> +#define CFG_RESP_TIMEOUT_SHIFT 8
>> +#define CFG_RESP_TIMEOUT_MASK 0xf
>> +#define CFG_RC_CC_SHIFT 12
>> +#define CFG_RC_CC_MASK 0xf
>> +#define CFG_STOP_CLOCK BIT(22)
>> +#define CFG_CLK_ALWAYS_ON BIT(18)
>> +#define CFG_AUTO_CLK BIT(23)
>> +
>> +#define SD_EMMC_STATUS 0x48
>> +#define STATUS_BUSY BIT(31)
>> +
>> +#define SD_EMMC_IRQ_EN 0x4c
>> +#define IRQ_EN_MASK 0x3fff
>> +#define IRQ_RXD_ERR_SHIFT 0
>> +#define IRQ_RXD_ERR_MASK 0xff
>> +#define IRQ_TXD_ERR BIT(8)
>> +#define IRQ_DESC_ERR BIT(9)
>> +#define IRQ_RESP_ERR BIT(10)
>> +#define IRQ_RESP_TIMEOUT BIT(11)
>> +#define IRQ_DESC_TIMEOUT BIT(12)
>> +#define IRQ_END_OF_CHAIN BIT(13)
>> +#define IRQ_RESP_STATUS BIT(14)
>> +#define IRQ_SDIO BIT(15)
>> +
>> +#define SD_EMMC_CMD_CFG 0x50
>> +#define SD_EMMC_CMD_ARG 0x54
>> +#define SD_EMMC_CMD_DAT 0x58
>> +#define SD_EMMC_CMD_RSP 0x5c
>> +#define SD_EMMC_CMD_RSP1 0x60
>> +#define SD_EMMC_CMD_RSP2 0x64
>> +#define SD_EMMC_CMD_RSP3 0x68
>> +
>> +#define SD_EMMC_RXD 0x94
>> +#define SD_EMMC_TXD 0x94
>> +#define SD_EMMC_LAST_REG SD_EMMC_TXD
>> +
>> +#define SD_EMMC_CFG_BLK_SIZE 512 /* internal buffer max: 512 bytes */
>> +#define SD_EMMC_CFG_RESP_TIMEOUT 256 /* in clock cycles */
>> +#define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */
>> +#define MUX_CLK_NUM_PARENTS 2
>> +
>> +struct meson_host {
>> + struct device *dev;
>> + struct mmc_host *mmc;
>> + struct mmc_request *mrq;
>> + struct mmc_command *cmd;
>> +
>> + spinlock_t lock;
>> + void __iomem *regs;
>> +#ifdef USE_REGMAP
>> + struct regmap *regmap;
>> +#endif
>
> Dead code?
>
>> + int irq;
>> + u32 ocr_mask;
>> + struct clk *core_clk;
>> + struct clk_mux mux;
>> + struct clk *mux_clk;
>> + struct clk *mux_parent[MUX_CLK_NUM_PARENTS];
>> + unsigned long mux_parent_rate[MUX_CLK_NUM_PARENTS];
>> +
>> + struct clk_divider cfg_div;
>> + struct clk *cfg_div_clk;
>> +
>> + unsigned int bounce_buf_size;
>> + void *bounce_buf;
>> + dma_addr_t bounce_dma_addr;
>> +
>> + unsigned long clk_rate;
>> + unsigned long clk_src_rate;
>> + unsigned short clk_src_div;
>> +};
>> +
>> +#define reg_read(host, offset) readl(host->regs + offset)
>> +#define reg_write(host, offset, val) writel(val, host->regs + offset)
>> +
>> +struct cmd_cfg {
>> + union {
>> + struct {
>> + u32 length:9;
>> + u32 block_mode:1;
>> + u32 r1b:1;
>> + u32 end_of_chain:1;
>> + u32 timeout:4; /* 2^timeout msec */
>> + u32 no_resp:1;
>> + u32 no_cmd:1;
>> + u32 data_io:1;
>> + u32 data_wr:1;
>> + u32 resp_nocrc:1;
>> + u32 resp_128:1;
>> + u32 resp_num:1;
>> + u32 data_num:1;
>> + u32 cmd_index:6;
>> + u32 error:1;
>> + u32 owner:1;
>
>> +#define CFG_OWNER_CPU 1
>> +#define CFG_OWNER_MMC 0
>> + };
>> + u32 val;
>
> Does this need to be __le32 to handle endian swaps? I thought
> bitfields weren't safe for things that are written to hardware,
> but I can't seem to find any definitive email about that
> anywhere.
Hmm, yeah. I need to rework and test this for CONFIG_CPU_BIG_ENDIAN=y.
I think I'll completely get rid of the bitfields.
>> + };
>> +};
>> +
>> +struct sd_emmc_desc {
>> + struct cmd_cfg cmd_cfg;
>> + u32 cmd_arg;
>> + u32 cmd_data;
>> + u32 cmd_resp;
>
> __le32?
Ack.
>> +};
>> +#define CMD_DATA_MASK (~0x3)
>> +#define CMD_DATA_BIG_ENDIAN BIT(1)
>> +#define CMD_DATA_SRAM BIT(0)
>> +#define CMD_RESP_MASK (~0x1)
>> +#define CMD_RESP_SRAM BIT(0)
>> +
>> +static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
>> +{
>> + struct mmc_host *mmc = host->mmc;
>> + int ret = 0;
>> + u32 cfg;
>> +
>> + 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;
>> + }
>> +
>> + if (clk_rate == host->clk_rate)
>> + return 0;
>> +
>> + /* stop clock */
>> + cfg = reg_read(host, SD_EMMC_CFG);
>> + if (!(cfg & CFG_STOP_CLOCK)) {
>> + cfg |= CFG_STOP_CLOCK;
>> + reg_write(host, SD_EMMC_CFG, cfg);
>> + }
>> +
>> + dev_dbg(host->dev, "change clock rate %lu -> %lu\n",
>> + host->clk_rate, clk_rate);
>> + ret = clk_set_rate(host->cfg_div_clk, clk_rate);
>> + if (clk_rate && clk_rate != clk_get_rate(host->cfg_div_clk))
>> + dev_warn(host->dev, "divider requested rate %lu != actual rate %lu: ret=%d\n",
>> + clk_rate, clk_get_rate(host->cfg_div_clk), ret);
>> + else
>> + host->clk_rate = clk_rate;
>> +
>> + /* (re)start clock, if non-zero */
>> + if (clk_rate) {
>> + cfg = reg_read(host, SD_EMMC_CFG);
>> + cfg &= ~CFG_STOP_CLOCK;
>> + reg_write(host, SD_EMMC_CFG, cfg);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int meson_mmc_clk_init(struct meson_host *host)
>> +{
>> + struct clk_init_data init;
>> + char clk_name[32];
>> + int i, ret = 0;
>> + const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
>> + unsigned int mux_parent_count = 0;
>> + const char *clk_div_parents[1];
>> + unsigned int f_min = UINT_MAX;
>> + u32 clk_reg, cfg;
>> +
>> + /* get the mux parents from DT */
>
> None of this looks DT specific (good).
>
Ok, will drop the "from DT" part of the comment.
>> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> + char name[16];
>> +
>> + snprintf(name, sizeof(name), "clkin%d", i);
>> + host->mux_parent[i] = devm_clk_get(host->dev, name);
>> + if (IS_ERR(host->mux_parent[i])) {
>> + ret = PTR_ERR(host->mux_parent[i]);
>> + if (PTR_ERR(host->mux_parent[i]) != -EPROBE_DEFER)
>> + dev_err(host->dev, "Missing clock %s\n", name);
>> + host->mux_parent[i] = NULL;
>> + return ret;
>> + }
>> +
>> + host->mux_parent_rate[i] = clk_get_rate(host->mux_parent[i]);
>> + mux_parent_names[i] = __clk_get_name(host->mux_parent[i]);
>> + mux_parent_count++;
>> + if (host->mux_parent_rate[i] < f_min)
>> + f_min = host->mux_parent_rate[i];
>> + }
>> +
>> + /* cacluate f_min based on input clocks, and max divider value */
>> + if (f_min != UINT_MAX)
>> + f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX);
>> + else
>> + f_min = 4000000; /* default min: 400 MHz */
>> + host->mmc->f_min = f_min;
>> +
>> + /* create the mux */
>> + snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev));
>> + init.name = clk_name;
>> + init.ops = &clk_mux_ops;
>> + init.flags = CLK_IS_BASIC;
>
> Do you need CLK_IS_BASIC? If not please drop it.
>
Dropped.
>> + init.parent_names = mux_parent_names;
>> + init.num_parents = mux_parent_count;
>> +
>> + host->mux.reg = host->regs + SD_EMMC_CLOCK;
>> + host->mux.shift = CLK_SRC_SHIFT;
>> + host->mux.mask = CLK_SRC_MASK;
>> + host->mux.flags = 0;
>> + host->mux.table = NULL;
>> + host->mux.hw.init = &init;
>> +
>> + host->mux_clk = devm_clk_register(host->dev, &host->mux.hw);
>
> Hmm I was hoping to get rid of devm_clk_register() and replace it
> with devm_clk_hw_register() instead. All part of my grand plan to
> split providers from consumers, but this driver is different in
> the sense that it registers clks to provide to itself. Maybe we
> should make __clk_create_clk() into a real clk provider API so
> that we can use devm_clk_hw_register() here and then generate a
> clk for this device from the hw structure.
So is there something else I should do here, or can we rework this later
after you rework the framework a bit?
>> + if (WARN_ON(PTR_ERR_OR_ZERO(host->mux_clk)))
>
> NULL is a valid clk so I would expect to only see
> WARN_ON(IS_ERR(host->mux_clk)) here.
OK.
>> + return PTR_ERR(host->mux_clk);
>> +
>> + /* create the divider */
>> + snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev));
>> + init.name = devm_kstrdup(host->dev, clk_name, GFP_KERNEL);
>> + init.ops = &clk_divider_ops;
>> + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
>> + clk_div_parents[0] = __clk_get_name(host->mux_clk);
>> + init.parent_names = clk_div_parents;
>> + init.num_parents = ARRAY_SIZE(clk_div_parents);
>> +
>> + host->cfg_div.reg = host->regs + SD_EMMC_CLOCK;
>> + host->cfg_div.shift = CLK_DIV_SHIFT;
>> + host->cfg_div.width = CLK_DIV_WIDTH;
>> + host->cfg_div.hw.init = &init;
>> + host->cfg_div.flags = CLK_DIVIDER_ONE_BASED |
>> + CLK_DIVIDER_ROUND_CLOSEST | 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->cfg_div_clk);
>> +
>> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> + clk_reg = 0;
>> + clk_reg |= CLK_PHASE_180 << CLK_PHASE_SHIFT;
>> + clk_reg |= CLK_SRC_XTAL << CLK_SRC_SHIFT;
>> + clk_reg |= CLK_DIV_MAX << CLK_DIV_SHIFT;
>> + clk_reg &= ~CLK_ALWAYS_ON;
>> + reg_write(host, SD_EMMC_CLOCK, clk_reg);
>> +
>> + clk_prepare_enable(host->cfg_div_clk);
>
> This can fail...
>
>> +
>> + /* Ensure clock starts in "auto" mode, not "always on" */
>> + cfg = reg_read(host, SD_EMMC_CFG);
>> + cfg &= ~CFG_CLK_ALWAYS_ON;
>> + cfg |= CFG_AUTO_CLK;
>> + reg_write(host, SD_EMMC_CFG, cfg);
>> +
>> + meson_mmc_clk_set(host, f_min);
>
> This can fail too but we don't handle the error?
>
Ok, fixing the error checking.
>> +
>> + return ret;
>> +}
>> +
>> +static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> + struct meson_host *host = mmc_priv(mmc);
>> + u32 bus_width;
>> + u32 val, orig;
>> +
>> + /*
>> + * GPIO regulator, only controls switching between 1v8 and
>> + * 3v3, doesn't support MMC_POWER_OFF, MMC_POWER_ON.
>> + */
>> + switch (ios->power_mode) {
>> + case MMC_POWER_UP:
>> + if (!IS_ERR(mmc->supply.vmmc))
>> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
>> + }
>> +
>> + meson_mmc_clk_set(host, ios->clock);
>> +
>> + /* Bus width */
>> + val = reg_read(host, SD_EMMC_CFG);
>> + switch (ios->bus_width) {
>> + case MMC_BUS_WIDTH_1:
>> + bus_width = CFG_BUS_WIDTH_1;
>> + break;
>> + case MMC_BUS_WIDTH_4:
>> + bus_width = CFG_BUS_WIDTH_4;
>> + break;
>> + case MMC_BUS_WIDTH_8:
>> + bus_width = CFG_BUS_WIDTH_8;
>> + break;
>> + default:
>> + dev_err(host->dev, "Invalid ios->bus_width: %u. Setting to 4.\n",
>> + ios->bus_width);
>> + bus_width = CFG_BUS_WIDTH_4;
>> + return;
>> + }
>> +
>> + val = reg_read(host, SD_EMMC_CFG);
>> + orig = val;
>> +
>> + val &= ~(CFG_BUS_WIDTH_MASK << CFG_BUS_WIDTH_SHIFT);
>> + val |= bus_width << CFG_BUS_WIDTH_SHIFT;
>> +
>> + val &= ~(CFG_BLK_LEN_MASK << CFG_BLK_LEN_SHIFT);
>> + val |= ilog2(SD_EMMC_CFG_BLK_SIZE) << CFG_BLK_LEN_SHIFT;
>> +
>> + val &= ~(CFG_RESP_TIMEOUT_MASK << CFG_RESP_TIMEOUT_SHIFT);
>> + val |= ilog2(SD_EMMC_CFG_RESP_TIMEOUT) << CFG_RESP_TIMEOUT_SHIFT;
>> +
>> + val &= ~(CFG_RC_CC_MASK << CFG_RC_CC_SHIFT);
>> + val |= ilog2(SD_EMMC_CFG_CMD_GAP) << CFG_RC_CC_SHIFT;
>> +
>> + reg_write(host, SD_EMMC_CFG, val);
>> +
>> + if (val != orig)
>> + dev_dbg(host->dev, "%s: SD_EMMC_CFG: 0x%08x -> 0x%08x\n",
>> + __func__, orig, val);
>> +}
>> +
>> +static int meson_mmc_wait_busy(struct mmc_host *mmc, unsigned int timeout)
>> +{
>> + struct meson_host *host = mmc_priv(mmc);
>> + u32 status;
>> + int i;
>> +
>> + for (i = timeout; i > 0; i--) {
>> + status = reg_read(host, SD_EMMC_STATUS);
>> + if (!(status & STATUS_BUSY))
>> + break;
>> + };
>> +
>> + return (timeout == 0);
>> +}
>> +
>> +static int meson_mmc_request_done(struct mmc_host *mmc, struct mmc_request *mrq)
>> +{
>> + struct meson_host *host = mmc_priv(mmc);
>> + struct mmc_command *cmd = host->cmd;
>> + unsigned int loops = 0xfffff;
>
> Perhaps this should be a value of time instead of raw loops? Who
> knows how fast the CPU can run all those loops?
Ack.
>> +
>> + WARN_ON(host->mrq != mrq);
>> + if (cmd && !cmd->error)
>> + if (meson_mmc_wait_busy(mmc, loops))
>> + dev_warn(host->dev, "%s: timeout busy.\n", __func__);
>> +
>> + host->mrq = NULL;
>> + host->cmd = NULL;
>> + mmc_request_done(host->mmc, mrq);
>> +
>> + return 0;
>> +}
>> +
> [...]
>> +
>> +static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> +{
>> + struct meson_host *host = mmc_priv(mmc);
>> +
>> + WARN_ON(host->mrq != NULL);
>> +
>> + /* Stop execution */
>> + reg_write(host, SD_EMMC_START, 0);
>> +
>> + /* clear, ack, enable all interrupts */
>> + reg_write(host, SD_EMMC_IRQ_EN, 0);
>> + reg_write(host, SD_EMMC_STATUS, IRQ_EN_MASK);
>> + reg_write(host, SD_EMMC_IRQ_EN, IRQ_EN_MASK);
>> +
>> + host->mrq = mrq;
>> +
>> + if (meson_mmc_check_cmd(mmc, mrq->cmd))
>> + return;
>> +
>> + if (mrq->sbc)
>> + meson_mmc_start_cmd(mmc, mrq->sbc);
>> + else
>> + meson_mmc_start_cmd(mmc, mrq->cmd);
>> +}
>> +
>> +static int meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
>> +{
>> + struct meson_host *host = mmc_priv(mmc);
>> +
>> + if (!cmd) {
>
> This happens?
Hmm, nope, it's prevented elsewhere. I'll remove.
>> + dev_dbg(host->dev, "%s: NULL command.\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + if (cmd->flags & MMC_RSP_136) {
>> + cmd->resp[0] = reg_read(host, SD_EMMC_CMD_RSP3);
>> + cmd->resp[1] = reg_read(host, SD_EMMC_CMD_RSP2);
>> + cmd->resp[2] = reg_read(host, SD_EMMC_CMD_RSP1);
>> + cmd->resp[3] = reg_read(host, SD_EMMC_CMD_RSP);
>> + } else if (cmd->flags & MMC_RSP_PRESENT) {
>> + cmd->resp[0] = reg_read(host, SD_EMMC_CMD_RSP);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
>> +{
>> + struct meson_host *host = dev_id;
>> + struct mmc_request *mrq = host->mrq;
>> + struct mmc_command *cmd = host->cmd;
>> + u32 irq_en, status, raw_status;
>> + irqreturn_t ret = IRQ_HANDLED;
>> +
>> + if (WARN_ON(!host))
>> + return IRQ_NONE;
>> +
>> + if (WARN_ON(!mrq))
>> + return IRQ_NONE;
>> +
>> + if (WARN_ON(!cmd))
>> + return IRQ_NONE;
>
> These can happen? Spurious interrupts abound? Ugh.
They do when you mis-program the hardware, so I want to catch that here.
>> +
>> + spin_lock(&host->lock);
>> + irq_en = reg_read(host, SD_EMMC_IRQ_EN);
>> + raw_status = reg_read(host, SD_EMMC_STATUS);
>> + status = raw_status & irq_en;
>> +
>> + if (!status) {
>> + dev_warn(host->dev, "Spurious IRQ! status=0x%08x, irq_en=0x%08x\n",
>> + raw_status, irq_en);
>> + ret = IRQ_NONE;
>> + goto out;
>> + }
>> +
>> + cmd->error = 0;
>> + if (status & IRQ_RXD_ERR_MASK) {
>> + dev_dbg(host->dev, "Unhandled IRQ: RXD error\n");
>> + cmd->error = -EILSEQ;
>> + }
>> + if (status & IRQ_TXD_ERR) {
>> + dev_dbg(host->dev, "Unhandled IRQ: TXD error\n");
>> + cmd->error = -EILSEQ;
>> + }
>> + if (status & IRQ_DESC_ERR)
>> + dev_dbg(host->dev, "Unhandled IRQ: Descriptor error\n");
>> + if (status & IRQ_RESP_ERR) {
>> + dev_dbg(host->dev, "Unhandled IRQ: Response error\n");
>> + cmd->error = -EILSEQ;
>> + }
>> + if (status & IRQ_RESP_TIMEOUT) {
>> + dev_dbg(host->dev, "Unhandled IRQ: Response timeout\n");
>> + cmd->error = -ETIMEDOUT;
>> + }
>> + if (status & IRQ_DESC_TIMEOUT) {
>> + dev_dbg(host->dev, "Unhandled IRQ: Descriptor timeout\n");
>> + cmd->error = -ETIMEDOUT;
>> + }
>> + if (status & IRQ_SDIO)
>> + dev_dbg(host->dev, "Unhandled IRQ: SDIO.\n");
>> +
>> + if (status & (IRQ_END_OF_CHAIN | IRQ_RESP_STATUS))
>> + ret = IRQ_WAKE_THREAD;
>> + else {
>> + dev_warn(host->dev, "Unknown IRQ! status=0x%04x: MMC CMD%u arg=0x%08x flags=0x%08x stop=%d\n",
>> + status, cmd->opcode, cmd->arg,
>> + cmd->flags, mrq->stop ? 1 : 0);
>> + if (cmd->data) {
>> + struct mmc_data *data = cmd->data;
>> +
>> + dev_warn(host->dev, "\tblksz %u blocks %u flags 0x%08x (%s%s)",
>> + data->blksz, data->blocks, data->flags,
>> + data->flags & MMC_DATA_WRITE ? "write" : "",
>> + data->flags & MMC_DATA_READ ? "read" : "");
>> + }
>> + }
>> +
>> +out:
>> + /* ack all (enabled) interrupts */
>> + reg_write(host, SD_EMMC_STATUS, status);
>> +
>> + if (ret == IRQ_HANDLED) {
>> + meson_mmc_read_resp(host->mmc, cmd);
>> + meson_mmc_request_done(host->mmc, cmd->mrq);
>> + }
>> +
>> + spin_unlock(&host->lock);
>> + return ret;
>> +}
>> +
>> +static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
>> +{
>> + struct meson_host *host = dev_id;
>> + struct mmc_request *mrq = host->mrq;
>> + struct mmc_command *cmd = host->cmd;
>> + struct mmc_data *data;
>> +
>
> Drop that newline?
>
>> + unsigned int xfer_bytes;
>> + int ret = IRQ_HANDLED;
>> +
>> + if (WARN_ON(!mrq))
>> + ret = IRQ_NONE;
>> +
>> + if (WARN_ON(!cmd))
>> + ret = IRQ_NONE;
>> +
>> + data = cmd->data;
>> + if (data) {
>> + xfer_bytes = data->blksz * data->blocks;
>> + if (data->flags & MMC_DATA_READ) {
>> + WARN_ON(xfer_bytes > host->bounce_buf_size);
>> + sg_copy_from_buffer(data->sg, data->sg_len,
>> + host->bounce_buf, xfer_bytes);
>> + data->bytes_xfered = xfer_bytes;
>> + }
>> + }
>> +
>> + meson_mmc_read_resp(host->mmc, cmd);
>> + if (!data || !data->stop || mrq->sbc)
>> + meson_mmc_request_done(host->mmc, mrq);
>> + else
>> + meson_mmc_start_cmd(host->mmc, data->stop);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * NOTE: we only need this until the GPIO/pinctrl driver can handle
>> + * interrupts. For now, the MMC core will use this for polling.
>> + */
>> +static int meson_mmc_get_cd(struct mmc_host *mmc)
>> +{
>> + int status = mmc_gpio_get_cd(mmc);
>> +
>> + if (status == -ENOSYS)
>> + return 1; /* assume present */
>> +
>> + return status;
>> +}
>> +
>> +static struct mmc_host_ops meson_mmc_ops = {
>
> const?
>
>> + .request = meson_mmc_request,
>> + .set_ios = meson_mmc_set_ios,
>> + .get_cd = meson_mmc_get_cd,
>> +};
>> +
>> +static int meson_mmc_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct resource *res;
>> + struct meson_host *host;
>> + struct mmc_host *mmc;
>> + int ret;
>> +
>> + mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev);
>> + if (!mmc)
>> + return -ENOMEM;
>> + host = mmc_priv(mmc);
>> + host->mmc = mmc;
>> + host->dev = &pdev->dev;
>> + dev_set_drvdata(&pdev->dev, host);
>> +
>> + spin_lock_init(&host->lock);
>> +
>> + host->core_clk = devm_clk_get(&pdev->dev, "core");
>> + if (IS_ERR(host->core_clk)) {
>> + ret = PTR_ERR(host->core_clk);
>> + if (ret == -EPROBE_DEFER)
>> + dev_dbg(&pdev->dev,
>> + "Missing core clock. EPROBE_DEFER\n");
>> + else
>> + dev_err(&pdev->dev,
>> + "Unable to get core clk (ret=%d).\n", ret);
>> + goto free_host;
>> + }
>> +
>> + /* Voltage supply */
>> + mmc_of_parse_voltage(np, &host->ocr_mask);
>> + ret = mmc_regulator_get_supply(mmc);
>> + if (ret == -EPROBE_DEFER) {
>> + dev_dbg(&pdev->dev, "Missing regulator: EPROBE_DEFER");
>> + goto free_host;
>> + }
>> +
>> + if (!mmc->ocr_avail)
>> + mmc->ocr_avail = host->ocr_mask;
>> +
>> + ret = mmc_of_parse(mmc);
>> + if (ret) {
>> + dev_warn(&pdev->dev, "error parsing DT: %d\n", ret);
>> + goto free_host;
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + ret = -ENODEV;
>> + goto free_host;
>> + }
>
> This if can be dropped and go right into devm_ioremap_resource().
>
Yup, somone already submitted a patch to remove that from linux-next.
>> + host->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(host->regs)) {
>> + ret = PTR_ERR(host->regs);
>> + goto free_host;
>> + }
>> +
>> + host->irq = platform_get_irq(pdev, 0);
>> + if (host->irq == 0) {
>> + dev_err(&pdev->dev, "failed to get interrupt resource.\n");
>> + ret = -EINVAL;
>> + goto free_host;
>> + }
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, host->irq,
>> + meson_mmc_irq, meson_mmc_irq_thread,
>> + IRQF_SHARED, DRIVER_NAME, host);
>> + if (ret)
>> + goto free_host;
>> +
>> + /* data bounce buffer */
>> + host->bounce_buf_size = SZ_512K;
>> + host->bounce_buf =
>> + dma_alloc_coherent(host->dev, host->bounce_buf_size,
>> + &host->bounce_dma_addr, GFP_KERNEL);
>> + if (host->bounce_buf == NULL) {
>> + dev_err(host->dev, "Unable to map allocate DMA bounce buffer.\n");
>> + ret = -ENOMEM;
>> + goto free_host;
>> + }
>> +
>> + clk_prepare_enable(host->core_clk);
>
> This can fail.
>
>> +
>> + ret = meson_mmc_clk_init(host);
>> + if (ret)
>> + goto free_host;
>> +
>> + /* Stop execution */
>> + reg_write(host, SD_EMMC_START, 0);
>> +
>> + /* clear, ack, enable all interrupts */
>> + reg_write(host, SD_EMMC_IRQ_EN, 0);
>> + reg_write(host, SD_EMMC_STATUS, IRQ_EN_MASK);
>
> Shouldn't we do this before requesting the interrupt?
>
Yes.
>> +
>> + mmc->ops = &meson_mmc_ops;
>> + mmc_add_host(mmc);
>> +
>> + return 0;
>> +
>> +free_host:
>> + dev_dbg(host->dev, "Failed to probe: ret=%d\n", ret);
>> + if (host->core_clk)
>> + clk_disable_unprepare(host->core_clk);
>> + mmc_free_host(mmc);
>> + return ret;
>> +}
>> +
I'll clean up all the other minor comments for v2 also.
Thanks for the review!
Kevin
More information about the linux-arm-kernel
mailing list