[U-Boot] [PATCH 08/14] mmc: Add JZ47xx SD/MMC controller driver

[Marek Vasut]

On 11/28/2016 03:58 AM, Jaehoon Chung wrote:
> Hi Marek,
> 
> On 11/26/2016 07:32 AM, Marek Vasut wrote:
>> From: Paul Burton <paul.burton at imgtec.com>
>>
>> Add driver for the JZ47xx MSC controller.
> 
> There are some checkpatch error and warings. Could you fix them?

Yeah

> And i don't know what means MSC?

Me neither, probably MMC SD Controller .

>> Signed-off-by: Marek Vasut <marex at denx.de>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> Cc: Paul Burton <paul.burton at imgtec.com>
>> ---
>>  drivers/mmc/Kconfig  |   6 +
>>  drivers/mmc/Makefile |   1 +
>>  drivers/mmc/jz_mmc.c | 443 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 450 insertions(+)
>>  create mode 100644 drivers/mmc/jz_mmc.c
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index aca438b8..761b886 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -102,6 +102,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..213fe63
>> --- /dev/null
>> +++ b/drivers/mmc/jz_mmc.c
>> @@ -0,0 +1,443 @@
>> +/*
>> + * Ingenic JZ MMC driver
>> + *
>> + * Copyright (c) 2013 Imagination Technologies
>> + * Author: Paul Burton <paul.burton at 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>
>> +
>> +/* 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)
>> +#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 << 0)
>> +#define MSC_CMDAT_RESPONSE_NONE		(0x0 << 0) /* No response */
>> +#define MSC_CMDAT_RESPONSE_R1		(0x1 << 0) /* Format R1 and R1b */
>> +#define MSC_CMDAT_RESPONSE_R2		(0x2 << 0) /* Format R2 */
>> +#define MSC_CMDAT_RESPONSE_R3		(0x3 << 0) /* Format R3 */
>> +#define MSC_CMDAT_RESPONSE_R4		(0x4 << 0) /* Format R4 */
>> +#define MSC_CMDAT_RESPONSE_R5		(0x5 << 0) /* Format R5 */
>> +#define MSC_CMDAT_RESPONSE_R6		(0x6 << 0) /* Format R6 */
> 
> It doesn't need to shift 0.
> 
>> +
>> +/* 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;	/* FIXME */
> 
> What FIXME?

Dunno, removed.

>> +/* 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;
>> +}
> 
> I don't know this function is really needs..

Ev. this clock rate should come from DT, so this function should be
here. But, we don't support DT in SPL due to size concerns (the SPL
has to be below 14 kiB, see previous patch), so there will eventually be
come more code here I think.

>> +
>> +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);
>> +	/* FIXME -- wait_bit() */
> 
> Fixme -- wait bit()?

Seems like wait_for_bit now fits, so fixed.

>> +	while (readl(priv->regs + MSC_STAT) & MSC_STAT_CLK_EN);
>> +
>> +	writel(0, priv->regs + MSC_DMAC);

[...]

>> +	} 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 -ECOMM;
> 
> Not ECOMM..It's CRC error..then use the "-EILSEQ 84"
> 
>> +			if (stat & MSC_STAT_DATA_FIFO_EMPTY) {
>> +				udelay(10);
> 
> Why put udelay at here?

Wait until more data get loaded, the hardware is crap and sensitive to
me polling this register too often (sigh).

>> +				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));
>> +		} while (!(stat & MSC_STAT_DATA_TRAN_DONE));
>> +	}
>> +
>> +	return 0;
>> +}

[...]

>> +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);
> 
> I think that almost all mmc can support 1bit buswidth..

True

>> +	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;
>> +		break;
>> +	default:
>> +		printf("error: invalid bus-width property\n");
> 
> Dosen't consider 1bit-buswidth?

Oh, good catch.

>> +		return -ENOENT;
>> +	}
>> +
>> +	cfg->f_min = 400000;
>> +	cfg->f_max = fdtdec_get_int(fdt, node, "max-frequency", 52000000);
>> +
>> +
>> +

[...]


-- 
Best regards,
Marek Vasut