[U-Boot,1/2] mmc: Add Amlogic Meson driver

Jaehoon Chung jh80.chung at samsung.com
Wed Aug 3 23:48:50 PDT 2016


Hi Carlo

On 05/12/2016 09:41 PM, Carlo Caione wrote:
> From: Carlo Caione <carlo at endlessm.com>
> 
> This is a port / rewrite of the Amlogic driver shipped whithin the
> Amlogic SDK for the Meson GXBaby (S905) SoC.

s/whithin/within

And If you want to apply this patch, could you resend the patch on lates u-boot?

> 
> Signed-off-by: Carlo Caione <carlo at endlessm.com>
> ---
>  arch/arm/include/asm/arch-meson/sd_emmc.h | 109 +++++++++++
>  drivers/mmc/Makefile                      |   1 +
>  drivers/mmc/meson_mmc.c                   | 305 ++++++++++++++++++++++++++++++
>  3 files changed, 415 insertions(+)
>  create mode 100644 arch/arm/include/asm/arch-meson/sd_emmc.h
>  create mode 100644 drivers/mmc/meson_mmc.c
> 
> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h
> new file mode 100644
> index 0000000..6781dca
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h
> @@ -0,0 +1,109 @@
> +/*
> + * (C) Copyright 2016 Carlo Caione <carlo at caione.org>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __SD_EMMC_H__
> +#define __SD_EMMC_H__
> +
> +#include <mmc.h>
> +
> +#define SDIO_PORT_A			0
> +#define SDIO_PORT_B			1
> +#define SDIO_PORT_C			2
> +
> +#define SD_EMMC_BASE_A			0xd0070000
> +#define SD_EMMC_BASE_B			0xd0072000
> +#define SD_EMMC_BASE_C			0xd0074000
> +
> +#define SD_IRQ_ALL			0x3fff
> +
> +#define SD_EMMC_CLKSRC_24M		24000000
> +#define SD_EMMC_CLKSRC_DIV2		1000000000

Is it right? 1GHz?

> +
> +#define CLK_DIV				0
> +#define CLK_SRC				6
> +#define CLK_CO_PHASE			8
> +#define CLK_ALWAYS_ON			24

Are these for shifting?

> +
> +#define ADDR_USE_PING_BUF		BIT(1)
> +
> +#define SD_EMMC_RXD_ERROR		BIT(0)
> +#define SD_EMMC_TXD_ERROR		BIT(1)
> +#define SD_EMMC_DESC_ERROR		BIT(2)
> +#define SD_EMMC_RESP_CRC_ERROR		BIT(3)
> +#define SD_EMMC_RESP_TIMEOUT_ERROR	BIT(4)
> +#define SD_EMMC_DESC_TIMEOUT_ERROR	BIT(5)
> +
> +#define CFG_BUS_WIDTH			0
> +#define CFG_BUS_WIDTH_MASK		(0x3 << 0)
> +#define CFG_BL_LEN			4
> +#define CFG_BL_LEN_MASK			(0xf << 4)
> +#define CFG_RESP_TIMEOUT		8
> +#define CFG_RESP_TIMEOUT_MASK		(0xf << 8)
> +#define CFG_RC_CC			12
> +#define CFG_RC_CC_MASK			(0xf << 12)
> +
> +#define STATUS_RXD_ERR_MASK		0xff
> +#define STATUS_TXD_ERR			BIT(8)
> +#define STATUS_DESC_ERR			BIT(9)
> +#define STATUS_RESP_ERR			BIT(10)
> +#define STATUS_RESP_TIMEOUT		BIT(11)
> +#define STATUS_DESC_TIMEOUT		BIT(12)
> +#define STATUS_END_OF_CHAIN		BIT(13)
> +
> +#define CMD_CFG_LENGTH_MASK		0x1ff
> +#define CMD_CFG_CMD_INDEX		24
> +#define CMD_CFG_BLOCK_MODE		BIT(9)
> +#define CMD_CFG_R1B			BIT(10)
> +#define CMD_CFG_END_OF_CHAIN		BIT(11)
> +#define CMD_CFG_NO_RESP			BIT(16)
> +#define CMD_CFG_DATA_IO			BIT(18)
> +#define CMD_CFG_DATA_WR			BIT(19)
> +#define CMD_CFG_RESP_NOCRC		BIT(20)
> +#define CMD_CFG_RESP_128		BIT(21)
> +#define CMD_CFG_OWNER			BIT(31)

Add the description for which register.
e,g
/* GCLOCK register */
#define ...

> +
> +struct meson_mmc_regs {
> +	uint32_t gclock;
> +	uint32_t gdelay;
> +	uint32_t gadjust;
> +	uint32_t reserved_0c;
> +	uint32_t gcalout;
> +	uint32_t reserved_14[11];
> +	uint32_t gstart;
> +	uint32_t gcfg;
> +	uint32_t gstatus;
> +	uint32_t girq_en;
> +	uint32_t gcmd_cfg;
> +	uint32_t gcmd_arg;
> +	uint32_t gcmd_dat;
> +	uint32_t gcmd_rsp0;
> +	uint32_t gcmd_rsp1;
> +	uint32_t gcmd_rsp2;
> +	uint32_t gcmd_rsp3;
> +	uint32_t reserved_6c;
> +	uint32_t gcurr_cfg;
> +	uint32_t gcurr_arg;
> +	uint32_t gcurr_dat;
> +	uint32_t gcurr_rsp;
> +	uint32_t gnext_cfg;
> +	uint32_t gnext_arg;
> +	uint32_t gnext_dat;
> +	uint32_t gnext_rsp;
> +	uint32_t grxd;
> +	uint32_t gtxd;
> +	uint32_t reserved_98[90];
> +	uint32_t gdesc[128];
> +	uint32_t gping[128];
> +	uint32_t gpong[128];
> +};

Don't use this style..Use the define.
It's difficult to debug..devloper don't know which offset is used before calculating.
And add the descriptions..

> +
> +struct meson_mmc_platdata {
> +	struct mmc_config cfg;
> +	struct meson_mmc_regs *sd_emmc_reg;
> +	char *w_buf;
> +};
> +
> +#endif
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 585aaf3..08ac9ba 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_FTSDC021) += ftsdc021_sdhci.o
>  obj-$(CONFIG_GENERIC_MMC) += mmc.o
>  obj-$(CONFIG_GENERIC_ATMEL_MCI) += gen_atmel_mci.o
>  obj-$(CONFIG_KONA_SDHCI) += kona_sdhci.o
> +obj-$(CONFIG_MMC_MESON) += meson_mmc.o
>  obj-$(CONFIG_MMC_SPI) += mmc_spi.o
>  obj-$(CONFIG_MMC_SUNXI) += sunxi_mmc.o
>  obj-$(CONFIG_MV_SDHCI) += mv_sdhci.o
> diff --git a/drivers/mmc/meson_mmc.c b/drivers/mmc/meson_mmc.c
> new file mode 100644
> index 0000000..af224ec
> --- /dev/null
> +++ b/drivers/mmc/meson_mmc.c
> @@ -0,0 +1,305 @@
> +/*
> + * (C) Copyright 2016 Carlo Caione <carlo at caione.org>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <mmc.h>
> +#include <asm/io.h>
> +#include <asm/arch/sd_emmc.h>
> +#include <dm/device.h>
> +
> +static void meson_mmc_config_clock(struct mmc *mmc,
> +				   struct meson_mmc_regs *meson_mmc_reg)
> +{
> +	uint32_t meson_mmc_clk = 0;
> +	unsigned int clk, clk_src, clk_div;
> +
> +	if (mmc->clock > 12000000) {
> +		clk = SD_EMMC_CLKSRC_DIV2;
> +		clk_src = 1;
> +	} else {
> +		clk = SD_EMMC_CLKSRC_24M;
> +		clk_src = 0;
> +	}
> +	clk_div = clk / mmc->clock;
> +
> +	if (mmc->clock < mmc->cfg->f_min)
> +		mmc->clock = mmc->cfg->f_min;
> +	if (mmc->clock > mmc->cfg->f_max)
> +		mmc->clock = mmc->cfg->f_max;

In mmc_set_clock(), mmc->clock is already assigned..
Doesn't need this.

> +
> +	/* Keep the clock always on */
> +	meson_mmc_clk |= (1 << CLK_ALWAYS_ON);
> +
> +	/* 180 phase */
> +	meson_mmc_clk |= (2 << CLK_CO_PHASE);

Don't use the magic number, what do 1 and 2 mean?

> +
> +	/* clock settings */
> +	meson_mmc_clk |= (clk_src << CLK_SRC);
> +	meson_mmc_clk |= (clk_div << CLK_DIV);

How many bits do use for clk_div? bit[0:11]?

> +
> +	writel(meson_mmc_clk, &meson_mmc_reg->gclock);
> +}
> +
> +static void meson_mmc_set_ios(struct mmc *mmc)
> +{
> +	struct meson_mmc_platdata *pdata;
> +	struct meson_mmc_regs *meson_mmc_reg;
> +	unsigned int bus_width;
> +	uint32_t meson_mmc_cfg = 0;
> +
> +	pdata = mmc->priv;
> +	meson_mmc_reg = pdata->sd_emmc_reg;
> +
> +	meson_mmc_config_clock(mmc, meson_mmc_reg);
> +
> +	meson_mmc_cfg = readl(&meson_mmc_reg->gcfg);
> +
> +	if (mmc->bus_width == 1)
> +		bus_width = 0;
> +	else
> +		bus_width = mmc->bus_width / 4;
> +
> +	/* 1-bit mode */
> +	meson_mmc_cfg &= ~CFG_BUS_WIDTH_MASK;
> +	meson_mmc_cfg |= (bus_width << CFG_BUS_WIDTH);
> +
> +	/* 512 bytes block lenght */
> +	meson_mmc_cfg &= ~CFG_BL_LEN_MASK;
> +	meson_mmc_cfg |= (9 << CFG_BL_LEN);
> +
> +	/* Response timeout 256 clk */
> +	meson_mmc_cfg &= ~CFG_RESP_TIMEOUT_MASK;
> +	meson_mmc_cfg |= (7 << CFG_RESP_TIMEOUT);
> +
> +	/* Command-command gap 1024 clk */
> +	meson_mmc_cfg &= ~CFG_RC_CC_MASK;
> +	meson_mmc_cfg |= (4 << CFG_RC_CC);

Ditto...what are 9, 7, 4?
Don't use the magic number in entire your codes.

> +
> +	writel(meson_mmc_cfg, &meson_mmc_reg->gcfg);
> +
> +	return;

not needs "return"

> +}
> +
> +static int meson_mmc_init(struct mmc *mmc)
> +{
> +	mmc_set_clock(mmc, 400000);
> +
> +	return 0;
> +}
> +
> +static void meson_mmc_setup_cmd(struct mmc *mmc, struct mmc_data *data,
> +				struct mmc_cmd *cmd,
> +				struct meson_mmc_regs *meson_mmc_reg)
> +{
> +	uint32_t meson_mmc_cmd = 0;
> +
> +	meson_mmc_cmd = ((0x80 | cmd->cmdidx) << CMD_CFG_CMD_INDEX);
> +
> +	if (cmd->resp_type & MMC_RSP_PRESENT) {
> +		if (cmd->resp_type & MMC_RSP_136)
> +			meson_mmc_cmd |= CMD_CFG_RESP_128;
> +
> +		if (cmd->resp_type & MMC_RSP_BUSY)
> +			meson_mmc_cmd |= CMD_CFG_R1B;

There is no case with MMC_RSP_136 and MMC_RSP_BUSY together.

> +
> +		if (!(cmd->resp_type & MMC_RSP_CRC))
> +			meson_mmc_cmd |= CMD_CFG_RESP_NOCRC;
> +	} else {
> +		meson_mmc_cmd |= CMD_CFG_NO_RESP;
> +	}
> +
> +	if (data) {
> +		meson_mmc_cmd |= CMD_CFG_DATA_IO;
> +
> +		if (data->flags == MMC_DATA_WRITE)
> +			meson_mmc_cmd |= CMD_CFG_DATA_WR;
> +
> +		if (data->blocks > 1) {
> +			meson_mmc_cmd |= CMD_CFG_BLOCK_MODE;
> +			meson_mmc_cmd |= data->blocks;
> +		} else {
> +			meson_mmc_cmd |= (data->blocksize & CMD_CFG_LENGTH_MASK);
> +		}
> +	}
> +
> +	meson_mmc_cmd |= CMD_CFG_OWNER;
> +	meson_mmc_cmd |= CMD_CFG_END_OF_CHAIN;
> +
> +	writel(meson_mmc_cmd, &meson_mmc_reg->gcmd_cfg);
> +
> +	return;
> +}
> +
> +static void meson_mmc_setup_addr(struct mmc *mmc, struct mmc_data *data,
> +				 struct meson_mmc_regs *meson_mmc_reg)
> +{
> +	struct meson_mmc_platdata *pdata;
> +	unsigned int data_size = 0;
> +	uint32_t meson_mmc_data_addr = 0;
> +
> +	pdata = mmc->priv;
> +
> +	if (data) {
> +		data_size = data->blocks * data->blocksize;
> +
> +		if (data->flags == MMC_DATA_READ) {
> +			if (data_size < 0x200) {
> +				meson_mmc_data_addr = (ulong) meson_mmc_reg->gping;
> +				meson_mmc_data_addr |= ADDR_USE_PING_BUF;
> +			} else {
> +				invalidate_dcache_range((ulong) data->dest,
> +							(ulong) (data->dest + data_size));
> +				meson_mmc_data_addr = (ulong) data->dest;
> +			}
> +		}
> +
> +		if (data->flags == MMC_DATA_WRITE) {

data->flags is only two..MMC_DATA_WRITE ..otherwise MMC_DATA_READ.

> +			pdata->w_buf = calloc(data_size, sizeof(char));
> +			memcpy(pdata->w_buf, data->src, data_size);
> +			flush_dcache_range((ulong) pdata->w_buf,
> +					   (ulong) (pdata->w_buf + data_size));
> +			meson_mmc_data_addr = (ulong) pdata->w_buf;
> +		}
> +	}
> +
> +	writel(meson_mmc_data_addr, &meson_mmc_reg->gcmd_dat);
> +
> +	return;
> +}
> +
> +static void meson_mmc_read_response(struct mmc *mmc, struct mmc_data *data,
> +				    struct mmc_cmd *cmd,
> +				    struct meson_mmc_regs *meson_mmc_reg)
> +{
> +	unsigned int data_size = 0;
> +
> +	if (data) {
> +		data_size = data->blocks * data->blocksize;
> +		if ((data_size < 0x200) && (data->flags == MMC_DATA_READ))
> +			memcpy(data->dest, (const void *)meson_mmc_reg->gping, data_size);
> +	}
> +
> +	if (cmd->resp_type & MMC_RSP_136) {
> +		cmd->response[0] = readl(&meson_mmc_reg->gcmd_rsp3);
> +		cmd->response[1] = readl(&meson_mmc_reg->gcmd_rsp2);
> +		cmd->response[2] = readl(&meson_mmc_reg->gcmd_rsp1);
> +		cmd->response[3] = readl(&meson_mmc_reg->gcmd_rsp0);
> +	} else {
> +		cmd->response[0] = readl(&meson_mmc_reg->gcmd_rsp0);
> +	}
> +}
> +
> +static int meson_mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> +			      struct mmc_data *data)
> +{
> +	struct meson_mmc_platdata *pdata;
> +	struct meson_mmc_regs *meson_mmc_reg;
> +	uint32_t meson_mmc_irq = 0;
> +	int ret = 0;
> +
> +	pdata = mmc->priv;
> +	meson_mmc_reg = pdata->sd_emmc_reg;
> +
> +	meson_mmc_setup_cmd(mmc, data, cmd, meson_mmc_reg);
> +	meson_mmc_setup_addr(mmc, data, meson_mmc_reg);
> +
> +	writel(SD_IRQ_ALL, &meson_mmc_reg->gstatus);
> +	writel(cmd->cmdarg, &meson_mmc_reg->gcmd_arg);
> +
> +	while (1) {
> +		meson_mmc_irq = readl(&meson_mmc_reg->gstatus);
> +		if (meson_mmc_irq & STATUS_END_OF_CHAIN)
> +			break;
> +	}

I don't like this style...potential infinte loop.

> +
> +	if (meson_mmc_irq & STATUS_RXD_ERR_MASK)
> +		ret |= SD_EMMC_RXD_ERROR;
> +	if (meson_mmc_irq & STATUS_TXD_ERR)
> +		ret |= SD_EMMC_TXD_ERROR;
> +	if (meson_mmc_irq & STATUS_DESC_ERR)
> +		ret |= SD_EMMC_DESC_ERROR;
> +	if (meson_mmc_irq & STATUS_RESP_ERR)
> +		ret |= SD_EMMC_RESP_CRC_ERROR;
> +	if (meson_mmc_irq & STATUS_RESP_TIMEOUT)
> +		ret |= SD_EMMC_RESP_TIMEOUT_ERROR;
> +	if (meson_mmc_irq & STATUS_DESC_TIMEOUT)
> +		ret |= SD_EMMC_DESC_TIMEOUT_ERROR;

What are these ret values? Do you use the your specific error values?

> +
> +	meson_mmc_read_response(mmc, data, cmd, meson_mmc_reg);
> +
> +	if (data && data->flags == MMC_DATA_WRITE)
> +		free(pdata->w_buf);
> +
> +	if (ret) {
> +		if (meson_mmc_irq & STATUS_RESP_TIMEOUT)
> +			return TIMEOUT;

Strange this..why return at here...not above?

Best Regards,
Jaehoon Chung

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct mmc_ops meson_mmc_ops = {
> +	.send_cmd	= meson_mmc_send_cmd,
> +	.set_ios	= meson_mmc_set_ios,
> +	.init		= meson_mmc_init,
> +};
> +
> +static int meson_mmc_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct meson_mmc_platdata *pdata = dev->platdata;
> +	fdt_addr_t addr;
> +
> +	addr = dev_get_addr(dev);
> +	if (addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;
> +
> +	pdata->sd_emmc_reg = (struct meson_mmc_regs *)addr;
> +
> +	return 0;
> +}
> +
> +static int meson_mmc_probe(struct udevice *dev)
> +{
> +	struct meson_mmc_platdata *pdata = dev->platdata;
> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> +	struct mmc *mmc;
> +	struct mmc_config *cfg;
> +
> +	cfg = &pdata->cfg;
> +	cfg->ops = &meson_mmc_ops;
> +
> +	cfg->voltages = MMC_VDD_33_34 | MMC_VDD_32_33 |
> +			MMC_VDD_31_32 | MMC_VDD_165_195;
> +	cfg->host_caps = MMC_MODE_8BIT | MMC_MODE_4BIT |
> +			 MMC_MODE_HS_52MHz | MMC_MODE_HS;
> +	cfg->f_min = 400000;
> +	cfg->f_max = 50000000;
> +	cfg->b_max = 256;
> +
> +	mmc = mmc_create(cfg, pdata);
> +	if (!mmc)
> +		return -ENOMEM;
> +
> +	upriv->mmc = mmc;
> +	return 0;
> +}
> +
> +static const struct udevice_id meson_mmc_match[] = {
> +	{ .compatible = "amlogic,meson-mmc" },
> +	{ /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(meson_mmc) = {
> +	.name = "meson_mmc",
> +	.id = UCLASS_MMC,
> +	.of_match = meson_mmc_match,
> +	.probe = meson_mmc_probe,
> +	.ofdata_to_platdata = meson_mmc_ofdata_to_platdata,
> +	.platdata_auto_alloc_size = sizeof(struct meson_mmc_platdata),
> +};
> 




More information about the linux-amlogic mailing list