Re: [PATCH v2 2/2] MMC: meson: initial support for GXBB platforms

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

 



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



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

  Powered by Linux