[U-Boot,06/13] mmc: Add JZ47xx SD/MMC controller driver

From: Paul Burton <paul.burton@imgtec.com>

Add driver for the JZ47xx MSC controller.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
---
 drivers/mmc/Kconfig  |   6 +
 drivers/mmc/Makefile |   1 +
 drivers/mmc/jz_mmc.c | 445 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 452 insertions(+)
 create mode 100644 drivers/mmc/jz_mmc.c

Dear Marek,

On 12/01/2016 10:06 AM, Marek Vasut wrote:
> From: Paul Burton <paul.burton@imgtec.com>
> 
> Add driver for the JZ47xx MSC controller.

I added more comment..Could you check my comments?

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> ---
>  drivers/mmc/Kconfig  |   6 +
>  drivers/mmc/Makefile |   1 +
>  drivers/mmc/jz_mmc.c | 445 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 452 insertions(+)
>  create mode 100644 drivers/mmc/jz_mmc.c
> 
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index 5e84a41..da26743 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -101,6 +101,12 @@ config MMC_UNIPHIER
>  	help
>  	  This selects support for the SD/MMC Host Controller on UniPhier SoCs.
>  
> +config JZ47XX_MMC
> +	bool "Ingenic JZ47xx SD/MMC Host Controller support"
> +	depends on ARCH_JZ47XX
> +	help
> +	  This selects support for the SD Card Controller on Ingenic JZ47xx SoCs.
> +
>  config SANDBOX_MMC
>  	bool "Sandbox MMC support"
>  	depends on MMC && SANDBOX
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index d850758..5f7cca3 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_TEGRA_MMC) += tegra_mmc.o
>  obj-$(CONFIG_MMC_UNIPHIER) += uniphier-sd.o
>  obj-$(CONFIG_ZYNQ_SDHCI) += zynq_sdhci.o
>  obj-$(CONFIG_ROCKCHIP_SDHCI) += rockchip_sdhci.o
> +obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
>  
>  ifdef CONFIG_SPL_BUILD
>  obj-$(CONFIG_SPL_MMC_BOOT) += fsl_esdhc_spl.o
> diff --git a/drivers/mmc/jz_mmc.c b/drivers/mmc/jz_mmc.c
> new file mode 100644
> index 0000000..95b3367
> --- /dev/null
> +++ b/drivers/mmc/jz_mmc.c
> @@ -0,0 +1,445 @@
> +/*
> + * Ingenic JZ MMC driver
> + *
> + * Copyright (c) 2013 Imagination Technologies
> + * Author: Paul Burton <paul.burton@imgtec.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <mmc.h>
> +#include <errno.h>
> +#include <asm/io.h>
> +#include <asm/unaligned.h>
> +#include <mach/jz4780.h>
> +#include <wait_bit.h>
> +
> +/* Registers */
> +#define MSC_STRPCL			0x000
> +#define MSC_STAT			0x004
> +#define MSC_CLKRT			0x008
> +#define MSC_CMDAT			0x00c
> +#define MSC_RESTO			0x010
> +#define MSC_RDTO			0x014
> +#define MSC_BLKLEN			0x018
> +#define MSC_NOB				0x01c
> +#define MSC_SNOB			0x020
> +#define MSC_IMASK			0x024
> +#define MSC_IREG			0x028
> +#define MSC_CMD				0x02c
> +#define MSC_ARG				0x030
> +#define MSC_RES				0x034
> +#define MSC_RXFIFO			0x038
> +#define MSC_TXFIFO			0x03c
> +#define MSC_LPM				0x040
> +#define MSC_DMAC			0x044
> +#define MSC_DMANDA			0x048
> +#define MSC_DMADA			0x04c
> +#define MSC_DMALEN			0x050
> +#define MSC_DMACMD			0x054
> +#define MSC_CTRL2			0x058
> +#define MSC_RTCNT			0x05c
> +#define MSC_DBG				0x0fc
> +
> +/* MSC Clock and Control Register (MSC_STRPCL) */
> +#define MSC_STRPCL_EXIT_MULTIPLE	BIT(7)
> +#define MSC_STRPCL_EXIT_TRANSFER	BIT(6)
> +#define MSC_STRPCL_START_READWAIT	BIT(5)
> +#define MSC_STRPCL_STOP_READWAIT	BIT(4)
> +#define MSC_STRPCL_RESET		BIT(3)
> +#define MSC_STRPCL_START_OP		BIT(2)
> +#define MSC_STRPCL_CLOCK_CONTROL_STOP	BIT(0)
> +#define MSC_STRPCL_CLOCK_CONTROL_START	BIT(1)
> +
> +/* MSC Status Register (MSC_STAT) */
> +#define MSC_STAT_AUTO_CMD_DONE		BIT(31)
> +#define MSC_STAT_IS_RESETTING		BIT(15)
> +#define MSC_STAT_SDIO_INT_ACTIVE	BIT(14)
> +#define MSC_STAT_PRG_DONE		BIT(13)
> +#define MSC_STAT_DATA_TRAN_DONE		BIT(12)
> +#define MSC_STAT_END_CMD_RES		BIT(11)
> +#define MSC_STAT_DATA_FIFO_AFULL	BIT(10)
> +#define MSC_STAT_IS_READWAIT		BIT(9)
> +#define MSC_STAT_CLK_EN			BIT(8)
> +#define MSC_STAT_DATA_FIFO_FULL		BIT(7)
> +#define MSC_STAT_DATA_FIFO_EMPTY	BIT(6)
> +#define MSC_STAT_CRC_RES_ERR		BIT(5)
> +#define MSC_STAT_CRC_READ_ERROR		BIT(4)
> +#define MSC_STAT_CRC_WRITE_ERROR	BIT(2)
> +#define MSC_STAT_CRC_WRITE_ERROR_NOSTS	BIT(4)

Does it use the same bit with MSC_STAT_CRC_READ_ERROR?

> +#define MSC_STAT_TIME_OUT_RES		BIT(1)
> +#define MSC_STAT_TIME_OUT_READ		BIT(0)
> +
> +/* MSC Bus Clock Control Register (MSC_CLKRT) */
> +#define MSC_CLKRT_CLK_RATE_MASK		0x7
> +
> +/* MSC Command Sequence Control Register (MSC_CMDAT) */
> +#define MSC_CMDAT_IO_ABORT		BIT(11)
> +#define MSC_CMDAT_BUS_WIDTH_1BIT	(0x0 << 9)
> +#define MSC_CMDAT_BUS_WIDTH_4BIT	(0x2 << 9)
> +#define MSC_CMDAT_DMA_EN		BIT(8)
> +#define MSC_CMDAT_INIT			BIT(7)
> +#define MSC_CMDAT_BUSY			BIT(6)
> +#define MSC_CMDAT_STREAM_BLOCK		BIT(5)
> +#define MSC_CMDAT_WRITE			BIT(4)
> +#define MSC_CMDAT_DATA_EN		BIT(3)
> +#define MSC_CMDAT_RESPONSE_MASK		0x7
> +#define MSC_CMDAT_RESPONSE_NONE		0x0 /* No response */
> +#define MSC_CMDAT_RESPONSE_R1		0x1 /* Format R1 and R1b */
> +#define MSC_CMDAT_RESPONSE_R2		0x2 /* Format R2 */
> +#define MSC_CMDAT_RESPONSE_R3		0x3 /* Format R3 */
> +#define MSC_CMDAT_RESPONSE_R4		0x4 /* Format R4 */
> +#define MSC_CMDAT_RESPONSE_R5		0x5 /* Format R5 */
> +#define MSC_CMDAT_RESPONSE_R6		0x6 /* Format R6 */
> +
> +/* MSC Interrupts Mask Register (MSC_IMASK) */
> +#define MSC_IMASK_TIME_OUT_RES		BIT(9)
> +#define MSC_IMASK_TIME_OUT_READ		BIT(8)
> +#define MSC_IMASK_SDIO			BIT(7)
> +#define MSC_IMASK_TXFIFO_WR_REQ		BIT(6)
> +#define MSC_IMASK_RXFIFO_RD_REQ		BIT(5)
> +#define MSC_IMASK_END_CMD_RES		BIT(2)
> +#define MSC_IMASK_PRG_DONE		BIT(1)
> +#define MSC_IMASK_DATA_TRAN_DONE	BIT(0)
> +
> +
> +/* MSC Interrupts Status Register (MSC_IREG) */
> +#define MSC_IREG_TIME_OUT_RES		BIT(9)
> +#define MSC_IREG_TIME_OUT_READ		BIT(8)
> +#define MSC_IREG_SDIO			BIT(7)
> +#define MSC_IREG_TXFIFO_WR_REQ		BIT(6)
> +#define MSC_IREG_RXFIFO_RD_REQ		BIT(5)
> +#define MSC_IREG_END_CMD_RES		BIT(2)
> +#define MSC_IREG_PRG_DONE		BIT(1)
> +#define MSC_IREG_DATA_TRAN_DONE		BIT(0)
> +
> +struct jz_mmc_priv {
> +	struct mmc_config	cfg;
> +	void __iomem		*regs;
> +	u32			flags;
> +/* priv flags */
> +#define JZ_MMC_BUS_WIDTH_MASK	0x3
> +#define JZ_MMC_BUS_WIDTH_1	0x0
> +#define JZ_MMC_BUS_WIDTH_4	0x2
> +#define JZ_MMC_BUS_WIDTH_8	0x3
> +#define JZ_MMC_SENT_INIT	BIT(2)
> +};
> +
> +static int jz_mmc_clock_rate(void)
> +{
> +	return 24000000;
> +}
> +
> +static int jz_mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> +						struct mmc_data *data)
> +{
> +	struct jz_mmc_priv *priv = mmc->priv;
> +	u32 stat, mask, cmdat = 0;
> +
> +	/* stop the clock */
> +	writel(MSC_STRPCL_CLOCK_CONTROL_STOP, priv->regs + MSC_STRPCL);
> +
> +	wait_for_bit("jzmmc", priv->regs + MSC_STAT,
> +		     MSC_STAT_CLK_EN, 0, 10, 0);
> +
> +	writel(0, priv->regs + MSC_DMAC);
> +
> +	/* setup command */
> +	writel(cmd->cmdidx, priv->regs + MSC_CMD);
> +	writel(cmd->cmdarg, priv->regs + MSC_ARG);
> +
> +	if (data) {
> +		/* setup data */
> +		cmdat |= MSC_CMDAT_DATA_EN;
> +		if (data->flags & MMC_DATA_WRITE)
> +			cmdat |= MSC_CMDAT_WRITE;
> +
> +		writel(data->blocks, priv->regs + MSC_NOB);
> +		writel(data->blocksize, priv->regs + MSC_BLKLEN);
> +	} else {
> +		writel(0, priv->regs + MSC_NOB);
> +		writel(0, priv->regs + MSC_BLKLEN);
> +	}
> +
> +	/* setup response */
> +	switch (cmd->resp_type) {
> +	case MMC_RSP_NONE:
> +		break;
> +	case MMC_RSP_R1:
> +	case MMC_RSP_R1b:
> +		cmdat |= MSC_CMDAT_RESPONSE_R1;
> +		break;
> +	case MMC_RSP_R2:
> +		cmdat |= MSC_CMDAT_RESPONSE_R2;
> +		break;
> +	case MMC_RSP_R3:
> +		cmdat |= MSC_CMDAT_RESPONSE_R3;
> +		break;

Can be put MMC_RSP_NONE at here?

> +	default:
> +		break;
> +	}
> +
> +	if (cmd->resp_type & MMC_RSP_BUSY)
> +		cmdat |= MSC_CMDAT_BUSY;
> +
> +	/* set init for the first command only */
> +	if (!(priv->flags & JZ_MMC_SENT_INIT)) {
> +		cmdat |= MSC_CMDAT_INIT;
> +		priv->flags |= JZ_MMC_SENT_INIT;

Is it right?

> +	}
> +
> +	cmdat |= (priv->flags & JZ_MMC_BUS_WIDTH_MASK) << 9;

After priv->flags is set to JZ_MMC_SENT_INIT (BIT(2)), immediately priv->flags & JZ_MMC_BUS_WIDTH_MASK (0x3)
It's meaningless, doesn't?

> +
> +	/* write the data setup */
> +	writel(cmdat, priv->regs + MSC_CMDAT);
> +
> +	/* unmask interrupts */
> +	mask = 0xffffffff & ~(MSC_IMASK_END_CMD_RES | MSC_IMASK_TIME_OUT_RES);

No..it's not readable..mask can be 0.
It seems that first set to all mask without CMD_RES and OUT_RES.
And at below sequence, clearing the bit after checking flags.

Why don't use the setting step, not clearing step?

> +	if (data) {
> +		mask &= ~MSC_IMASK_DATA_TRAN_DONE;
> +		if (data->flags & MMC_DATA_WRITE) {
> +			mask &= ~MSC_IMASK_TXFIFO_WR_REQ;

Then it can be "mask |= MSC_IMASK_TXFIFO_WR_REQ;" 

> +		} else {
> +			mask &= ~(MSC_IMASK_RXFIFO_RD_REQ |
> +				  MSC_IMASK_TIME_OUT_READ);

Ditto.

> +		}
> +	}
> +	writel(mask, priv->regs + MSC_IMASK);
> +
> +	/* clear interrupts */
> +	writel(0xffffffff, priv->regs + MSC_IREG);
> +
> +	/* start the command (& the clock) */
> +	writel(MSC_STRPCL_START_OP | MSC_STRPCL_CLOCK_CONTROL_START,
> +	       priv->regs + MSC_STRPCL);
> +
> +	/* wait for completion */
> +	wait_for_bit("jzmmc", priv->regs + MSC_IREG,
> +		     MSC_IREG_END_CMD_RES | MSC_IREG_TIME_OUT_RES, 1, 1, 0);
> +	stat = readl(priv->regs + MSC_IREG);
> +	stat &= MSC_IREG_END_CMD_RES | MSC_IREG_TIME_OUT_RES;
> +	writel(stat, priv->regs + MSC_IREG);
> +	if (stat & MSC_IREG_TIME_OUT_RES)
> +		return -ETIMEDOUT;
> +
> +	if (cmd->resp_type & MMC_RSP_PRESENT) {
> +		/* read the response */
> +		if (cmd->resp_type & MMC_RSP_136) {
> +			u16 a, b, c, i;
> +			a = readw(priv->regs + MSC_RES);
> +			for (i = 0; i < 4; i++) {
> +				b = readw(priv->regs + MSC_RES);
> +				c = readw(priv->regs + MSC_RES);

a and b and c are same? Why read MSC_RES twice?
Is register value changed?

> +				cmd->response[i] = (a << 24) | (b << 8) |
> +						   (c >> 8);
> +				a = c;
> +			}
> +		} else {
> +			cmd->response[0] = readw(priv->regs + MSC_RES) << 24;
> +			cmd->response[0] |= readw(priv->regs + MSC_RES) << 8;
> +			cmd->response[0] |= readw(priv->regs + MSC_RES) & 0xff;

Hmm..I didn't understand this..MSC_RES is 16bit register?
It seems strange..

> +		}
> +	}
> +
> +	if (data && (data->flags & MMC_DATA_WRITE)) {
> +		/* write the data */
> +		int sz = DIV_ROUND_UP(data->blocks * data->blocksize, 4);
> +		const void *buf = data->src;
> +
> +		while (sz--) {
> +			u32 val = get_unaligned_le32(buf);
> +			wait_for_bit("jzmmc", priv->regs + MSC_IREG,
> +				     MSC_IREG_TXFIFO_WR_REQ, 1, 50, 0);
> +			writel(val, priv->regs + MSC_TXFIFO);
> +			buf += 4;
> +		}
> +	} else if (data && (data->flags & MMC_DATA_READ)) {
> +		/* read the data */
> +		int sz = data->blocks * data->blocksize;
> +		void *buf = data->dest;
> +
> +		do {
> +			stat = readl(priv->regs + MSC_STAT);
> +
> +			if (stat & MSC_STAT_TIME_OUT_READ)
> +				return -ETIMEDOUT;
> +			if (stat & MSC_STAT_CRC_READ_ERROR)
> +				return -EILSEQ;
> +			if (stat & MSC_STAT_DATA_FIFO_EMPTY) {
> +				udelay(10);
> +				continue;
> +			}
> +			do {
> +				u32 val = readl(priv->regs + MSC_RXFIFO);
> +
> +				if (sz == 1)
> +					*(u8 *)buf = (u8)val;
> +				else if (sz == 2)
> +					put_unaligned_le16(val, buf);
> +				else if (sz >= 4)
> +					put_unaligned_le32(val, buf);
> +				buf += 4;
> +				sz -= 4;
> +				stat = readl(priv->regs + MSC_STAT);
> +			} while (!(stat & MSC_STAT_DATA_FIFO_EMPTY));

Doesn't need to check whether size is zero or not?
And error bit checking for stat?

> +		} while (!(stat & MSC_STAT_DATA_TRAN_DONE));
> +	}
> +
> +	return 0;
> +}
> +
> +static void jz_mmc_set_ios(struct mmc *mmc)
> +{
> +	struct jz_mmc_priv *priv = mmc->priv;
> +	u32 real_rate = jz_mmc_clock_rate();
> +	u8 clk_div = 0;
> +
> +	/* calculate clock divide */
> +	while ((real_rate > mmc->clock) && (clk_div < 7)) {
> +		real_rate >>= 1;
> +		clk_div++;
> +	}
> +	writel(clk_div & MSC_CLKRT_CLK_RATE_MASK, priv->regs + MSC_CLKRT);
> +
> +	/* set the bus width for the next command */
> +	priv->flags &= ~JZ_MMC_BUS_WIDTH_MASK;
> +	if (mmc->bus_width == 8)
> +		priv->flags |= JZ_MMC_BUS_WIDTH_8;
> +	else if (mmc->bus_width == 4)
> +		priv->flags |= JZ_MMC_BUS_WIDTH_4;
> +	else
> +		priv->flags |= JZ_MMC_BUS_WIDTH_1;
> +}
> +
> +static int jz_mmc_core_init(struct mmc *mmc)
> +{
> +	struct jz_mmc_priv *priv = mmc->priv;
> +
> +	/* Reset */
> +	writel(MSC_STRPCL_RESET, priv->regs + MSC_STRPCL);
> +
> +	wait_for_bit("jzmmc", priv->regs + MSC_STAT,
> +		     MSC_STAT_IS_RESETTING, 0, 10, 0);
> +
> +	/* Maximum timeouts */
> +	writel(0xffff, priv->regs + MSC_RESTO);
> +	writel(0xffffffff, priv->regs + MSC_RDTO);
> +
> +	/* Enable low power mode */
> +	writel(0x1, priv->regs + MSC_LPM);
> +
> +	return 0;
> +}
> +
> +static const struct mmc_ops jz_msc_ops = {
> +	.send_cmd	= jz_mmc_send_cmd,
> +	.set_ios	= jz_mmc_set_ios,
> +	.init		= jz_mmc_core_init,
> +};
> +
> +#ifdef CONFIG_MMC_TINY
> +static struct jz_mmc_priv jz_mmc_priv_static = {
> +	.cfg = {
> +		.name = "MSC",
> +		.ops = &jz_msc_ops,
> +
> +		.voltages = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
> +				MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
> +				MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36,

Really need to set all VDD?

> +		.host_caps = MMC_MODE_4BIT | MMC_MODE_HS_52MHz | MMC_MODE_HS,
> +
> +		.f_min = 375000,
> +		.f_max = 48000000,
> +		.b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT,
> +	},
> +};
> +
> +int jz_mmc_init(void __iomem *base)
> +{
> +	struct mmc *mmc;
> +
> +	jz_mmc_priv_static.regs = base;
> +
> +	mmc = mmc_create(&jz_mmc_priv_static.cfg, &jz_mmc_priv_static);
> +
> +	return mmc ? 0 : -ENODEV;
> +}
> +#endif
> +
> +#ifdef CONFIG_DM_MMC
> +#include <dm.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int jz_mmc_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct jz_mmc_priv *priv = dev_get_priv(dev);
> +	const void *fdt = gd->fdt_blob;
> +	int node = dev->of_offset;
> +	struct mmc_config *cfg;
> +	int val;
> +
> +	priv->regs = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
> +	cfg = &priv->cfg;
> +
> +	cfg->host_caps = MMC_MODE_HS_52MHz | MMC_MODE_HS;
> +	val = fdtdec_get_int(fdt, node, "bus-width", 1);
> +	if (val < 0) {
> +		printf("error: bus-width property missing\n");
> +		return -ENOENT;
> +	}

Maybe i think this condition doesn't need..
because if buswidth is wrong, it should be returned error at below switch statement.

> +
> +	switch (val) {
> +	case 0x8:
> +		cfg->host_caps |= MMC_MODE_8BIT;
> +	case 0x4:
> +		cfg->host_caps |= MMC_MODE_4BIT;
> +	case 0x1:
> +		break;
> +	default:
> +		printf("error: invalid bus-width property\n");
> +		return -ENOENT;
> +	}
> +
> +	cfg->f_min = 400000;
> +	cfg->f_max = fdtdec_get_int(fdt, node, "max-frequency", 52000000);
> +	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
> +	cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
> +
> +	return 0;
> +}
> +
> +static int jz_mmc_probe(struct udevice *dev)
> +{
> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> +	struct jz_mmc_priv *priv = dev_get_priv(dev);
> +	struct mmc_config *cfg = &priv->cfg;
> +	struct mmc *mmc;
> +
> +	cfg->name = "MSC";

I guess...MSC is "Mobile Storage Controller"? Is it new IP for designware?
Just wondering.. :)

Best Regards,
Jaehoon Chung

> +	cfg->ops = &jz_msc_ops;
> +
> +	mmc = mmc_create(cfg, priv);
> +	if (!mmc)
> +		return -ENODEV;
> +
> +	mmc->dev = dev;
> +	upriv->mmc = mmc;
> +
> +	return 0;
> +}
> +static const struct udevice_id jz_mmc_ids[] = {
> +	{ .compatible = "ingenic,jz4780-mmc" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(jz_mmc_drv) = {
> +	.name			= "jz_mmc",
> +	.id			= UCLASS_MMC,
> +	.of_match		= jz_mmc_ids,
> +	.ofdata_to_platdata	= jz_mmc_ofdata_to_platdata,
> +	.probe			= jz_mmc_probe,
> +	.priv_auto_alloc_size	= sizeof(struct jz_mmc_priv),
> +};
> +#endif
>

Am 01.12.2016 um 02:06 schrieb Marek Vasut:
> diff --git a/drivers/mmc/jz_mmc.c b/drivers/mmc/jz_mmc.c
> new file mode 100644
> index 0000000..95b3367
> --- /dev/null
> +++ b/drivers/mmc/jz_mmc.c
[...]
> +static void jz_mmc_set_ios(struct mmc *mmc)

This needs to return int by now.

> +{
> +	struct jz_mmc_priv *priv = mmc->priv;
> +	u32 real_rate = jz_mmc_clock_rate();
> +	u8 clk_div = 0;
> +
> +	/* calculate clock divide */
> +	while ((real_rate > mmc->clock) && (clk_div < 7)) {
> +		real_rate >>= 1;
> +		clk_div++;
> +	}
> +	writel(clk_div & MSC_CLKRT_CLK_RATE_MASK, priv->regs + MSC_CLKRT);
> +
> +	/* set the bus width for the next command */
> +	priv->flags &= ~JZ_MMC_BUS_WIDTH_MASK;
> +	if (mmc->bus_width == 8)
> +		priv->flags |= JZ_MMC_BUS_WIDTH_8;
> +	else if (mmc->bus_width == 4)
> +		priv->flags |= JZ_MMC_BUS_WIDTH_4;
> +	else
> +		priv->flags |= JZ_MMC_BUS_WIDTH_1;

return 0;

> +}
[...]
> +#ifdef CONFIG_MMC_TINY

Shouldn't this rather use something like #if defined(CONFIG_SPL_BUILD)
&& defined(CONFIG_SPL_MMC_TINY) so that we can drop the #define from ci20.h?

> +static struct jz_mmc_priv jz_mmc_priv_static = {
> +	.cfg = {
> +		.name = "MSC",
> +		.ops = &jz_msc_ops,
> +
> +		.voltages = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
> +				MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
> +				MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36,
> +		.host_caps = MMC_MODE_4BIT | MMC_MODE_HS_52MHz | MMC_MODE_HS,
> +
> +		.f_min = 375000,
> +		.f_max = 48000000,
> +		.b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT,
> +	},
> +};
> +
> +int jz_mmc_init(void __iomem *base)
> +{
> +	struct mmc *mmc;
> +
> +	jz_mmc_priv_static.regs = base;
> +
> +	mmc = mmc_create(&jz_mmc_priv_static.cfg, &jz_mmc_priv_static);
> +
> +	return mmc ? 0 : -ENODEV;
> +}
> +#endif
> +
> +#ifdef CONFIG_DM_MMC

Similarly here !defined(CONFIG_SPL_BUILD) && defined(CONFIG_DM_MMC)?

> +#include <dm.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int jz_mmc_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct jz_mmc_priv *priv = dev_get_priv(dev);
> +	const void *fdt = gd->fdt_blob;
> +	int node = dev->of_offset;
> +	struct mmc_config *cfg;
> +	int val;
> +
> +	priv->regs = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
> +	cfg = &priv->cfg;
> +
> +	cfg->host_caps = MMC_MODE_HS_52MHz | MMC_MODE_HS;
> +	val = fdtdec_get_int(fdt, node, "bus-width", 1);
> +	if (val < 0) {
> +		printf("error: bus-width property missing\n");
> +		return -ENOENT;
> +	}
> +
> +	switch (val) {
> +	case 0x8:
> +		cfg->host_caps |= MMC_MODE_8BIT;
> +	case 0x4:
> +		cfg->host_caps |= MMC_MODE_4BIT;
> +	case 0x1:
> +		break;
> +	default:
> +		printf("error: invalid bus-width property\n");
> +		return -ENOENT;
> +	}
> +
> +	cfg->f_min = 400000;
> +	cfg->f_max = fdtdec_get_int(fdt, node, "max-frequency", 52000000);
> +	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
> +	cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
> +
> +	return 0;
> +}
> +
> +static int jz_mmc_probe(struct udevice *dev)
> +{
> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> +	struct jz_mmc_priv *priv = dev_get_priv(dev);
> +	struct mmc_config *cfg = &priv->cfg;
> +	struct mmc *mmc;
> +
> +	cfg->name = "MSC";
> +	cfg->ops = &jz_msc_ops;
> +
> +	mmc = mmc_create(cfg, priv);
> +	if (!mmc)
> +		return -ENODEV;
> +
> +	mmc->dev = dev;
> +	upriv->mmc = mmc;
> +
> +	return 0;
> +}
> +static const struct udevice_id jz_mmc_ids[] = {
> +	{ .compatible = "ingenic,jz4780-mmc" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(jz_mmc_drv) = {
> +	.name			= "jz_mmc",
> +	.id			= UCLASS_MMC,
> +	.of_match		= jz_mmc_ids,
> +	.ofdata_to_platdata	= jz_mmc_ofdata_to_platdata,
> +	.probe			= jz_mmc_probe,
> +	.priv_auto_alloc_size	= sizeof(struct jz_mmc_priv),
> +};
> +#endif

Regards,
Andreas

On 02/12/2017 03:20 PM, Andreas Färber wrote:
> Am 01.12.2016 um 02:06 schrieb Marek Vasut:
>> diff --git a/drivers/mmc/jz_mmc.c b/drivers/mmc/jz_mmc.c
>> new file mode 100644
>> index 0000000..95b3367
>> --- /dev/null
>> +++ b/drivers/mmc/jz_mmc.c
> [...]
>> +static void jz_mmc_set_ios(struct mmc *mmc)
> 
> This needs to return int by now.

Feel free to fix this up and resubmit :)

>> +{
>> +	struct jz_mmc_priv *priv = mmc->priv;
>> +	u32 real_rate = jz_mmc_clock_rate();
>> +	u8 clk_div = 0;
>> +
>> +	/* calculate clock divide */
>> +	while ((real_rate > mmc->clock) && (clk_div < 7)) {
>> +		real_rate >>= 1;
>> +		clk_div++;
>> +	}
>> +	writel(clk_div & MSC_CLKRT_CLK_RATE_MASK, priv->regs + MSC_CLKRT);
>> +
>> +	/* set the bus width for the next command */
>> +	priv->flags &= ~JZ_MMC_BUS_WIDTH_MASK;
>> +	if (mmc->bus_width == 8)
>> +		priv->flags |= JZ_MMC_BUS_WIDTH_8;
>> +	else if (mmc->bus_width == 4)
>> +		priv->flags |= JZ_MMC_BUS_WIDTH_4;
>> +	else
>> +		priv->flags |= JZ_MMC_BUS_WIDTH_1;
> 
> return 0;
> 
>> +}
> [...]
>> +#ifdef CONFIG_MMC_TINY
> 
> Shouldn't this rather use something like #if defined(CONFIG_SPL_BUILD)
> && defined(CONFIG_SPL_MMC_TINY) so that we can drop the #define from ci20.h?
> 
>> +static struct jz_mmc_priv jz_mmc_priv_static = {
>> +	.cfg = {
>> +		.name = "MSC",
>> +		.ops = &jz_msc_ops,
>> +
>> +		.voltages = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
>> +				MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
>> +				MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36,
>> +		.host_caps = MMC_MODE_4BIT | MMC_MODE_HS_52MHz | MMC_MODE_HS,
>> +
>> +		.f_min = 375000,
>> +		.f_max = 48000000,
>> +		.b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT,
>> +	},
>> +};
>> +
>> +int jz_mmc_init(void __iomem *base)
>> +{
>> +	struct mmc *mmc;
>> +
>> +	jz_mmc_priv_static.regs = base;
>> +
>> +	mmc = mmc_create(&jz_mmc_priv_static.cfg, &jz_mmc_priv_static);
>> +
>> +	return mmc ? 0 : -ENODEV;
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_DM_MMC
> 
> Similarly here !defined(CONFIG_SPL_BUILD) && defined(CONFIG_DM_MMC)?
> 
>> +#include <dm.h>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static int jz_mmc_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +	struct jz_mmc_priv *priv = dev_get_priv(dev);
>> +	const void *fdt = gd->fdt_blob;
>> +	int node = dev->of_offset;
>> +	struct mmc_config *cfg;
>> +	int val;
>> +
>> +	priv->regs = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>> +	cfg = &priv->cfg;
>> +
>> +	cfg->host_caps = MMC_MODE_HS_52MHz | MMC_MODE_HS;
>> +	val = fdtdec_get_int(fdt, node, "bus-width", 1);
>> +	if (val < 0) {
>> +		printf("error: bus-width property missing\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	switch (val) {
>> +	case 0x8:
>> +		cfg->host_caps |= MMC_MODE_8BIT;
>> +	case 0x4:
>> +		cfg->host_caps |= MMC_MODE_4BIT;
>> +	case 0x1:
>> +		break;
>> +	default:
>> +		printf("error: invalid bus-width property\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	cfg->f_min = 400000;
>> +	cfg->f_max = fdtdec_get_int(fdt, node, "max-frequency", 52000000);
>> +	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
>> +	cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
>> +
>> +	return 0;
>> +}
>> +
>> +static int jz_mmc_probe(struct udevice *dev)
>> +{
>> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>> +	struct jz_mmc_priv *priv = dev_get_priv(dev);
>> +	struct mmc_config *cfg = &priv->cfg;
>> +	struct mmc *mmc;
>> +
>> +	cfg->name = "MSC";
>> +	cfg->ops = &jz_msc_ops;
>> +
>> +	mmc = mmc_create(cfg, priv);
>> +	if (!mmc)
>> +		return -ENODEV;
>> +
>> +	mmc->dev = dev;
>> +	upriv->mmc = mmc;
>> +
>> +	return 0;
>> +}
>> +static const struct udevice_id jz_mmc_ids[] = {
>> +	{ .compatible = "ingenic,jz4780-mmc" },
>> +	{ }
>> +};
>> +
>> +U_BOOT_DRIVER(jz_mmc_drv) = {
>> +	.name			= "jz_mmc",
>> +	.id			= UCLASS_MMC,
>> +	.of_match		= jz_mmc_ids,
>> +	.ofdata_to_platdata	= jz_mmc_ofdata_to_platdata,
>> +	.probe			= jz_mmc_probe,
>> +	.priv_auto_alloc_size	= sizeof(struct jz_mmc_priv),
>> +};
>> +#endif
> 
> Regards,
> Andreas
>

Am 12.02.2017 um 15:29 schrieb Marek Vasut:
> On 02/12/2017 03:20 PM, Andreas Färber wrote:
>> Am 01.12.2016 um 02:06 schrieb Marek Vasut:
>>> diff --git a/drivers/mmc/jz_mmc.c b/drivers/mmc/jz_mmc.c
>>> new file mode 100644
>>> index 0000000..95b3367
>>> --- /dev/null
>>> +++ b/drivers/mmc/jz_mmc.c
>> [...]
>>> +static void jz_mmc_set_ios(struct mmc *mmc)
>>
>> This needs to return int by now.
> 
> Feel free to fix this up and resubmit :)

Resubmitting with this trivial change wouldn't resolve Jaehoon's
unanswered code questions from Dec 1 though. ;)

As mentioned elsewhere, my trivial fixup is available for anyone who
wants to test or squash&resubmit:

https://github.com/afaerber/u-boot/commits/ci20

I don't know the Ingenic registers myself, sorry, and am already failing
to resubmit my own unfinished RK3368 series...

Regards,
Andreas