Stephen Boyd <sboyd@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> >> + * >> + * 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@xxxxxxxxxxxx> >> + * >> + * 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 -- 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
