[U-Boot] [PATCH 1/2] PXA: Add MMC driver using the generic MMC framework

[Igor Grinberg <grinberg@compulab.co.il>]

On 11/01/11 14:20, Marek Vasut wrote:
>> Hi Marek,
> 
> Hi Igor,
>>
>> On 08/29/11 01:02, Marek Vasut wrote:
>>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>>> ---
>>>
>>>  arch/arm/include/asm/arch-pxa/regs-mmc.h |  155 +++++++++++
>>>  drivers/mmc/Makefile                     |    1 +
>>>  drivers/mmc/pxa_mmc_gen.c                |  442
>>>  ++++++++++++++++++++++++++++++ 3 files changed, 598 insertions(+), 0
>>>  deletions(-)
>>>  create mode 100644 arch/arm/include/asm/arch-pxa/regs-mmc.h
>>>  create mode 100644 drivers/mmc/pxa_mmc_gen.c
>>>
>>> diff --git a/arch/arm/include/asm/arch-pxa/regs-mmc.h
>>> b/arch/arm/include/asm/arch-pxa/regs-mmc.h new file mode 100644
>>> index 0000000..fd1eb1e
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/arch-pxa/regs-mmc.h
>>> @@ -0,0 +1,155 @@
>>> +/*
>>> + * Copyright (C) 2011 Marek Vasut <marek.vasut@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation; either version 2 of
>>> + * the License, or (at your option) any later version.
>>> + *
>>> + * 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, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>> + */
>>> +
>>> +#ifndef	__REGS_MMC_H__
>>> +#define	__REGS_MMC_H__
>>> +
>>> +#define	MMC0_BASE	0x41100000
>>> +#define	MMC1_BASE	0x42000000
>>> +
>>> +int pxa_mmc_register(int card_index);
>>> +
>>> +struct pxa_mmc_regs {
>>> +	uint32_t	strpcl;
>>> +	uint32_t	stat;
>>> +	uint32_t	clkrt;
>>> +	uint32_t	spi;
>>> +	uint32_t	cmdat;
>>> +	uint32_t	resto;
>>> +	uint32_t	rdto;
>>> +	uint32_t	blklen;
>>> +	uint32_t	nob;
>>> +	uint32_t	prtbuf;
>>> +	uint32_t	i_mask;
>>> +	uint32_t	i_reg;
>>> +	uint32_t	cmd;
>>> +	uint32_t	argh;
>>> +	uint32_t	argl;
>>> +	uint32_t	res;
>>> +	uint32_t	rxfifo;
>>> +	uint32_t	txfifo;
>>
>> Isn't space would be enough?
>> I mean, they all the same type, so no alignment issues...
> 
> It's more readable like this, though that's likely an issue of personal taste.

Agreed, it is personal taste.

> 
>>
>>> +};
>>> +
>>> +/* MMC_STRPCL */
>>> +#define	MMC_STRPCL_STOP_CLK		(1 << 0)
>>> +#define	MMC_STRPCL_START_CLK		(1 << 1)
>>> +
>>> +/* MMC_STAT */
>>> +#define	MMC_STAT_END_CMD_RES		(1 << 13)
>>> +#define	MMC_STAT_PRG_DONE		(1 << 12)
>>> +#define	MMC_STAT_DATA_TRAN_DONE		(1 << 11)
>>> +#define	MMC_STAT_CLK_EN			(1 << 8)
>>> +#define	MMC_STAT_RECV_FIFO_FULL		(1 << 7)
>>> +#define	MMC_STAT_XMIT_FIFO_EMPTY	(1 << 6)
>>> +#define	MMC_STAT_RES_CRC_ERROR		(1 << 5)
>>> +#define	MMC_STAT_SPI_READ_ERROR_TOKEN   (1 << 4)
>>> +#define	MMC_STAT_CRC_READ_ERROR		(1 << 3)
>>> +#define	MMC_STAT_CRC_WRITE_ERROR	(1 << 2)
>>> +#define	MMC_STAT_TIME_OUT_RESPONSE	(1 << 1)
>>> +#define	MMC_STAT_READ_TIME_OUT		(1 << 0)
>>> +
>>> +/* MMC_CLKRT */
>>> +#define	MMC_CLKRT_20MHZ			0
>>> +#define	MMC_CLKRT_10MHZ			1
>>> +#define	MMC_CLKRT_5MHZ			2
>>> +#define	MMC_CLKRT_2_5MHZ		3
>>> +#define	MMC_CLKRT_1_25MHZ		4
>>> +#define	MMC_CLKRT_0_625MHZ		5
>>> +#define	MMC_CLKRT_0_3125MHZ		6
>>> +
>>> +/* MMC_SPI */
>>> +#define	MMC_SPI_EN			(1 << 0)
>>> +#define	MMC_SPI_CS_EN			(1 << 2)
>>> +#define	MMC_SPI_CS_ADDRESS		(1 << 3)
>>> +#define	MMC_SPI_CRC_ON			(1 << 1)
>>> +
>>> +/* MMC_CMDAT */
>>> +#define	MMC_CMDAT_SD_4DAT		(1 << 8)
>>> +#define	MMC_CMDAT_MMC_DMA_EN		(1 << 7)
>>> +#define	MMC_CMDAT_INIT			(1 << 6)
>>> +#define	MMC_CMDAT_BUSY			(1 << 5)
>>> +#define	MMC_CMDAT_BCR			(MMC_CMDAT_BUSY | 
> MMC_CMDAT_INIT)
>>> +#define	MMC_CMDAT_STREAM		(1 << 4)
>>> +#define	MMC_CMDAT_WRITE			(1 << 3)
>>> +#define	MMC_CMDAT_DATA_EN		(1 << 2)
>>> +#define	MMC_CMDAT_R0			0
>>> +#define	MMC_CMDAT_R1			1
>>> +#define	MMC_CMDAT_R2			2
>>> +#define	MMC_CMDAT_R3			3
>>> +
>>> +/* MMC_RESTO */
>>> +#define	MMC_RES_TO_MAX_MASK		0x7f
>>> +
>>> +/* MMC_RDTO */
>>> +#define	MMC_READ_TO_MAX_MASK		0xffff
>>> +
>>> +/* MMC_BLKLEN */
>>> +#define	MMC_BLK_LEN_MAX_MASK		0x3ff
>>> +
>>> +/* MMC_PRTBUF */
>>> +#define	MMC_PRTBUF_BUF_PART_FULL	(1 << 0)
>>> +
>>> +/* MMC_I_MASK */
>>> +#define	MMC_I_MASK_TXFIFO_WR_REQ	(1 << 6)
>>> +#define	MMC_I_MASK_RXFIFO_RD_REQ	(1 << 5)
>>> +#define	MMC_I_MASK_CLK_IS_OFF		(1 << 4)
>>> +#define	MMC_I_MASK_STOP_CMD		(1 << 3)
>>> +#define	MMC_I_MASK_END_CMD_RES		(1 << 2)
>>> +#define	MMC_I_MASK_PRG_DONE		(1 << 1)
>>> +#define	MMC_I_MASK_DATA_TRAN_DONE       (1 << 0)
>>> +#define	MMC_I_MASK_ALL			0x7f
>>> +
>>> +
>>> +/* MMC_I_REG */
>>> +#define	MMC_I_REG_TXFIFO_WR_REQ		(1 << 6)
>>> +#define	MMC_I_REG_RXFIFO_RD_REQ		(1 << 5)
>>> +#define	MMC_I_REG_CLK_IS_OFF		(1 << 4)
>>> +#define	MMC_I_REG_STOP_CMD		(1 << 3)
>>> +#define	MMC_I_REG_END_CMD_RES		(1 << 2)
>>> +#define	MMC_I_REG_PRG_DONE		(1 << 1)
>>> +#define	MMC_I_REG_DATA_TRAN_DONE	(1 << 0)
>>> +
>>> +/* MMC_CMD */
>>> +#define	MMC_CMD_INDEX_MAX		0x6f
>>> +#define	CMD(x)  (x)
>>
>> You've missed this one
> 
> Thanks
> 
>>
>>> +
>>> +#define	MMC_R1_IDLE_STATE		0x01
>>> +#define	MMC_R1_ERASE_STATE		0x02
>>> +#define	MMC_R1_ILLEGAL_CMD		0x04
>>> +#define	MMC_R1_COM_CRC_ERR		0x08
>>> +#define	MMC_R1_ERASE_SEQ_ERR		0x01
>>> +#define	MMC_R1_ADDR_ERR			0x02
>>> +#define	MMC_R1_PARAM_ERR		0x04
>>> +
>>> +#define	MMC_R1B_WP_ERASE_SKIP		0x0002
>>> +#define	MMC_R1B_ERR			0x0004
>>> +#define	MMC_R1B_CC_ERR			0x0008
>>> +#define	MMC_R1B_CARD_ECC_ERR		0x0010
>>> +#define	MMC_R1B_WP_VIOLATION		0x0020
>>> +#define	MMC_R1B_ERASE_PARAM		0x0040
>>> +#define	MMC_R1B_OOR			0x0080
>>> +#define	MMC_R1B_IDLE_STATE		0x0100
>>> +#define	MMC_R1B_ERASE_RESET		0x0200
>>> +#define	MMC_R1B_ILLEGAL_CMD		0x0400
>>> +#define	MMC_R1B_COM_CRC_ERR		0x0800
>>> +#define	MMC_R1B_ERASE_SEQ_ERR		0x1000
>>> +#define	MMC_R1B_ADDR_ERR		0x2000
>>> +#define	MMC_R1B_PARAM_ERR		0x4000
>>
>> All the above defines are really unnecessarily "tabbed"...
>> The values are fine, but why the names?
> 
> I'm used to put tabs there ... is there actually any rule against that ? 
> Checkpatch doesn't complain.

There is no rule, but it seems redundant,
especially with the 80 characters limit (though it is not a hard limit).

> 
>>
>>> +
>>> +#endif	/* __REGS_MMC_H__ */
>>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>>> index 3968c14..59bda49 100644
>>> --- a/drivers/mmc/Makefile
>>> +++ b/drivers/mmc/Makefile
>>> @@ -38,6 +38,7 @@ COBJS-$(CONFIG_MXC_MMC) += mxcmmc.o
>>>
>>>  COBJS-$(CONFIG_OMAP3_MMC) += omap3_mmc.o
>>>  COBJS-$(CONFIG_OMAP_HSMMC) += omap_hsmmc.o
>>>  COBJS-$(CONFIG_PXA_MMC) += pxa_mmc.o
>>>
>>> +COBJS-$(CONFIG_PXA_MMC_GENERIC) += pxa_mmc_gen.o
>>>
>>>  COBJS-$(CONFIG_S5P_MMC) += s5p_mmc.o
>>>  COBJS-$(CONFIG_SDHCI) += sdhci.o
>>>  COBJS-$(CONFIG_SH_MMCIF) += sh_mmcif.o
>>>
> [...]
> 
> Cheers
> 

-- 
Regards,
Igor.