On Thu, Nov 10, 2011 at 8:28 AM, Tom Warren <twar...@nvidia.com> wrote:
> Anton,
>
> -----Original Message-----
> From: Anton Staaf [mailto:robot...@chromium.org]
> Sent: Wednesday, November 09, 2011 12:46 PM
> To: u-boot@lists.denx.de
> Cc: Anton Staaf; Andy Fleming; Tom Warren; Stephen Warren; Albert Aribaud
> Subject: [PATCH v2 1/4] Tegra2: mmc: define register field values in
> tegra2_mmc.h
>
> This moves the magic numbers sprinkled about the MMC driver to a single
> location in the header file and gives them meaningful names.
>
> Signed-off-by: Anton Staaf <robot...@chromium.org>
> Cc: Andy Fleming <aflem...@gmail.com>
> Cc: Tom Warren <twar...@nvidia.com>
> Cc: Stephen Warren <swar...@nvidia.com>
> Cc: Albert Aribaud <albert.u.b...@aribaud.net>
> ---
> drivers/mmc/tegra2_mmc.c | 126
> ++++++++++++++++------------------------------
> drivers/mmc/tegra2_mmc.h | 80 +++++++++++++++++++++++++++++
> 2 files changed, 123 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/mmc/tegra2_mmc.c b/drivers/mmc/tegra2_mmc.c index
> 78b1190..2e7746d 100644
> --- a/drivers/mmc/tegra2_mmc.c
> +++ b/drivers/mmc/tegra2_mmc.c
> @@ -73,15 +73,10 @@ static void mmc_prepare_data(struct mmc_host *host,
> struct mmc_data *data)
> (u32)data->dest, data->blocks, data->blocksize);
>
> writel((u32)data->dest, &host->reg->sysad);
> - /*
> - * DMASEL[4:3]
> - * 00 = Selects SDMA
> - * 01 = Reserved
> - * 10 = Selects 32-bit Address ADMA2
> - * 11 = Selects 64-bit Address ADMA2
> - */
>
> Why remove the comments about the bit fields? Helps make it obvious what's
> being set.
Fair, I'll add them back in.
> +
> + /* Select SDMA */
> ctrl = readb(&host->reg->hostctl);
> - ctrl &= ~(3 << 3); /* SDMA */
> + ctrl &= ~TEGRA2_MMC_HOSTCTL_DMASEL_MASK;
>
> It's no longer clear that SDMA mode is being chosen here (especially w/o the
> DMASEL field comment, above). Maybe add a comment?
Yah, I wasn't certain about this either. Perhaps I'll change it to
mask out the bits and then or in the SDMA define, even though it's
zero. It will make it clear.
> writeb(ctrl, &host->reg->hostctl);
>
> /* We do not handle DMA boundaries, so set it to max (512 KiB) */ @@
> -93,21 +88,15 @@ static void mmc_set_transfer_mode(struct mmc_host *host,
> struct mmc_data *data) {
> unsigned short mode;
> debug(" mmc_set_transfer_mode called\n");
> - /*
> - * TRNMOD
> - * MUL1SIN0[5] : Multi/Single Block Select
> - * RD1WT0[4] : Data Transfer Direction Select
> - * 1 = read
> - * 0 = write
> - * ENACMD12[2] : Auto CMD12 Enable
> - * ENBLKCNT[1] : Block Count Enable
> - * ENDMA[0] : DMA Enable
> - */
> - mode = (1 << 1) | (1 << 0);
> +
> + mode = (TEGRA2_MMC_TRNMOD_DMA_EN_ENABLE |
> + TEGRA2_MMC_TRNMOD_BLOCK_COUNT_EN_ENABLE);
> +
>
> I know I work for NVIDIA and probably shouldn't say this, but I hate the
> _EN_ENABLE and _EN_DISABLE
> bit names in our HW docs. Can you use more natural names? Like
> "TRNMOD_DMA_ENABLE, etc.?
Heh. :) Yah, I'll come up with something better.
> if (data->blocks > 1)
> - mode |= (1 << 5);
> + mode |= TEGRA2_MMC_TRNMOD_MULTI_BLOCK_SELECT_ENABLE;
> +
> if (data->flags & MMC_DATA_READ)
> - mode |= (1 << 4);
> + mode |= TEGRA2_MMC_TRNMOD_DATA_XFER_DIR_SEL_READ;
>
> writew(mode, &host->reg->trnmod);
> }
> @@ -125,21 +114,16 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd
> *cmd,
> /* Wait max 10 ms */
> timeout = 10;
>
> - /*
> - * PRNSTS
> - * CMDINHDAT[1] : Command Inhibit (DAT)
> - * CMDINHCMD[0] : Command Inhibit (CMD)
> - */
> - mask = (1 << 0);
> + mask = TEGRA2_MMC_PRNSTS_CMD_INHIBIT_CMD_ACTIVE;
> if ((data != NULL) || (cmd->resp_type & MMC_RSP_BUSY))
> - mask |= (1 << 1);
> + mask |= TEGRA2_MMC_PRNSTS_CMD_INHIBIT_DAT_ACTIVE;
>
> /*
> * We shouldn't wait for data inhibit for stop commands, even
> * though they might use busy signaling
> */
> if (data)
> - mask &= ~(1 << 1);
> + mask &= ~TEGRA2_MMC_PRNSTS_CMD_INHIBIT_DAT_ACTIVE;
>
> while (readl(&host->reg->prnsts) & mask) {
> if (timeout == 0) {
> @@ -162,33 +146,23 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd
> *cmd,
> if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
> return -1;
>
> - /*
> - * CMDREG
> - * CMDIDX[13:8] : Command index
> - * DATAPRNT[5] : Data Present Select
> - * ENCMDIDX[4] : Command Index Check Enable
> - * ENCMDCRC[3] : Command CRC Check Enable
> - * RSPTYP[1:0]
> - * 00 = No Response
> - * 01 = Length 136
> - * 10 = Length 48
> - * 11 = Length 48 Check busy after response
> - */
> if (!(cmd->resp_type & MMC_RSP_PRESENT))
> - flags = 0;
> + flags = TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_NO_RESPONSE;
> else if (cmd->resp_type & MMC_RSP_136)
> - flags = (1 << 0);
> + flags = TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_LENGTH_136;
> else if (cmd->resp_type & MMC_RSP_BUSY)
> - flags = (3 << 0);
> + flags = TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_LENGTH_48_BUSY;
> else
> - flags = (2 << 0);
> + flags = TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_LENGTH_48;
>
> if (cmd->resp_type & MMC_RSP_CRC)
> - flags |= (1 << 3);
> + flags |= TEGRA2_MMC_TRNMOD_CMD_CRC_CHECK_EN_ENABLE;
> +
> if (cmd->resp_type & MMC_RSP_OPCODE)
> - flags |= (1 << 4);
> + flags |= TEGRA2_MMC_TRNMOD_CMD_INDEX_CHECK_EN_ENABLE;
> +
> if (data)
> - flags |= (1 << 5);
> + flags |= TEGRA2_MMC_TRNMOD_DATA_PRESENT_SELECT_DATA_TRANSFER;
>
> debug("cmd: %d\n", cmd->cmdidx);
>
> @@ -197,7 +171,7 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd
> *cmd,
> for (i = 0; i < retry; i++) {
> mask = readl(&host->reg->norintsts);
> /* Command Complete */
> - if (mask & (1 << 0)) {
> + if (mask & TEGRA2_MMC_NORINTSTS_CMD_COMPLETE) {
> if (!data)
> writel(mask, &host->reg->norintsts);
> break;
> @@ -209,11 +183,11 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd
> *cmd,
> return TIMEOUT;
> }
>
> - if (mask & (1 << 16)) {
> + if (mask & TEGRA2_MMC_NORINTSTS_CMD_TIMEOUT_ERR) {
> /* Timeout Error */
> debug("timeout: %08x cmd %d\n", mask, cmd->cmdidx);
> return TIMEOUT;
> - } else if (mask & (1 << 15)) {
> + } else if (mask & TEGRA2_MMC_NORINTSTS_ERR_INTERRUPT) {
> /* Error Interrupt */
> debug("error: %08x cmd %d\n", mask, cmd->cmdidx);
> return -1;
> @@ -259,17 +233,17 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd
> *cmd,
> while (1) {
> mask = readl(&host->reg->norintsts);
>
> - if (mask & (1 << 15)) {
> + if (mask & TEGRA2_MMC_NORINTSTS_ERR_INTERRUPT) {
> /* Error Interrupt */
> writel(mask, &host->reg->norintsts);
> printf("%s: error during transfer: 0x%08x\n",
> __func__, mask);
> return -1;
> - } else if (mask & (1 << 3)) {
> + } else if (mask & TEGRA2_MMC_NORINTSTS_DMA_INTERRUPT)
> {
> /* DMA Interrupt */
> debug("DMA end\n");
> break;
> - } else if (mask & (1 << 1)) {
> + } else if (mask & TEGRA2_MMC_NORINTSTS_XFER_COMPLETE)
> {
> /* Transfer Complete */
> debug("r/w is done\n");
> break;
> @@ -302,20 +276,15 @@ static void mmc_change_clock(struct mmc_host *host,
> uint clock)
>
> writew(0, &host->reg->clkcon);
>
> - /*
> - * CLKCON
> - * SELFREQ[15:8] : base clock divided by value
> - * ENSDCLK[2] : SD Clock Enable
> - * STBLINTCLK[1] : Internal Clock Stable
> - * ENINTCLK[0] : Internal Clock Enable
> - */
> div >>= 1;
> - clk = (div << 8) | (1 << 0);
> + clk = ((div << TEGRA2_MMC_CLKCON_SDCLK_FREQ_SEL_SHIFT) |
> + TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_EN_ENABLE);
> writew(clk, &host->reg->clkcon);
>
> /* Wait max 10 ms */
> timeout = 10;
> - while (!(readw(&host->reg->clkcon) & (1 << 1))) {
> + while (!(readw(&host->reg->clkcon) &
> + TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_STABLE_READY)) {
> if (timeout == 0) {
> printf("%s: timeout error\n", __func__);
> return;
> @@ -324,7 +293,7 @@ static void mmc_change_clock(struct mmc_host *host, uint
> clock)
> udelay(1000);
> }
>
> - clk |= (1 << 2);
> + clk |= TEGRA2_MMC_CLKCON_SD_CLOCK_EN_ENABLE;
> writew(clk, &host->reg->clkcon);
>
> debug("mmc_change_clock: clkcon = %08X\n", clk); @@ -370,12 +339,7 @@
> static void mmc_reset(struct mmc_host *host)
> unsigned int timeout;
> debug(" mmc_reset called\n");
>
> - /*
> - * RSTALL[0] : Software reset for all
> - * 1 = reset
> - * 0 = work
> - */
> - writeb((1 << 0), &host->reg->swrst);
> + writeb(TEGRA2_MMC_SWRST_SW_RESET_FOR_ALL, &host->reg->swrst);
>
> host->clock = 0;
>
> @@ -383,7 +347,7 @@ static void mmc_reset(struct mmc_host *host)
> timeout = 100;
>
> /* hw clears the bit when it's done */
> - while (readb(&host->reg->swrst) & (1 << 0)) {
> + while (readb(&host->reg->swrst) & TEGRA2_MMC_SWRST_SW_RESET_FOR_ALL) {
> if (timeout == 0) {
> printf("%s: timeout error\n", __func__);
> return;
> @@ -409,25 +373,21 @@ static int mmc_core_init(struct mmc *mmc)
> writel(0xffffffff, &host->reg->norintsigen);
>
> writeb(0xe, &host->reg->timeoutcon); /* TMCLK * 2^27 */
> - /*
> - * NORMAL Interrupt Status Enable Register init
> - * [5] ENSTABUFRDRDY : Buffer Read Ready Status Enable
> - * [4] ENSTABUFWTRDY : Buffer write Ready Status Enable
> - * [1] ENSTASTANSCMPLT : Transfre Complete Status Enable
> - * [0] ENSTACMDCMPLT : Command Complete Status Enable
> - */
> +
> + /* * NORMAL Interrupt Status Enable Register init */
> mask = readl(&host->reg->norintstsen);
> mask &= ~(0xffff);
> mask |= (1 << 5) | (1 << 4) | (1 << 1) | (1 << 0);
> + mask |= (TEGRA2_MMC_NORINTSTSEN_CMD_COMPLETE |
> + TEGRA2_MMC_NORINTSTSEN_XFER_COMPLETE |
> + TEGRA2_MMC_NORINTSTSEN_BUFFER_WRITE_READY |
> + TEGRA2_MMC_NORINTSTSEN_BUFFER_READ_READY);
> writel(mask, &host->reg->norintstsen);
>
> - /*
> - * NORMAL Interrupt Signal Enable Register init
> - * [1] ENSTACMDCMPLT : Transfer Complete Signal Enable
> - */
> + /* NORMAL Interrupt Signal Enable Register init */
> mask = readl(&host->reg->norintsigen);
> mask &= ~(0xffff);
> - mask |= (1 << 1);
> + mask |= TEGRA2_MMC_NORINTSIGEN_XFER_COMPLETE;
> writel(mask, &host->reg->norintsigen);
>
> return 0;
> diff --git a/drivers/mmc/tegra2_mmc.h b/drivers/mmc/tegra2_mmc.h index
> 28698e0..747aaa9 100644
> --- a/drivers/mmc/tegra2_mmc.h
> +++ b/drivers/mmc/tegra2_mmc.h
> @@ -68,6 +68,86 @@ struct tegra2_mmc {
> unsigned char res6[0x100]; /* RESERVED, offset 100h-1FFh */
> };
>
> +#define TEGRA2_MMC_HOSTCTL_DMASEL_MASK (3 << 3)
> +#define TEGRA2_MMC_HOSTCTL_DMASEL_SDMA (0 << 3)
> +#define TEGRA2_MMC_HOSTCTL_DMASEL_ADMA2_32BIT (2 << 3)
> +#define TEGRA2_MMC_HOSTCTL_DMASEL_ADMA2_64BIT (3 << 3)
> +
>
> These are awfully long (and hard to parse with the _EN_ENABLE & _EN_DISABLE
> NV nomenclature).
> Can you shorten them? The TEGRA2_MMC_ isn't really necessary, or at least s/b
> changed to TEGRA_MMC_,
> since T30 won't redefine these bits.
Yup, I don't want to shorten them to the point of aliasing with other
drivers. I'll at least remove the 2.
Thanks,
Anton
> +#define TEGRA2_MMC_TRNMOD_DMA_EN_MASK (1 << 0)
> +#define TEGRA2_MMC_TRNMOD_DMA_EN_DISABLE (0 << 0)
> +#define TEGRA2_MMC_TRNMOD_DMA_EN_ENABLE (1 <<
> 0)
> +
> +#define TEGRA2_MMC_TRNMOD_BLOCK_COUNT_EN_MASK (1 << 1)
> +#define TEGRA2_MMC_TRNMOD_BLOCK_COUNT_EN_DISABLE (0 << 1)
> +#define TEGRA2_MMC_TRNMOD_BLOCK_COUNT_EN_ENABLE (1 <<
> 1)
> +
> +#define TEGRA2_MMC_TRNMOD_DATA_XFER_DIR_SEL_MASK (1 << 4)
> +#define TEGRA2_MMC_TRNMOD_DATA_XFER_DIR_SEL_WRITE (0 << 4)
> +#define TEGRA2_MMC_TRNMOD_DATA_XFER_DIR_SEL_READ (1 << 4)
> +
> +#define TEGRA2_MMC_TRNMOD_MULTI_BLOCK_SELECT_MASK (1 << 5)
> +#define TEGRA2_MMC_TRNMOD_MULTI_BLOCK_SELECT_DISABLE (0 << 5)
> +#define TEGRA2_MMC_TRNMOD_MULTI_BLOCK_SELECT_ENABLE (1 << 5)
> +
> +#define TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_MASK (3 <<
> 0)
> +#define TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_NO_RESPONSE (0 << 0)
> +#define TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_LENGTH_136 (1 << 0)
> +#define TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_LENGTH_48 (2 << 0)
> +#define TEGRA2_MMC_CMDREG_RESP_TYPE_SELECT_LENGTH_48_BUSY (3 << 0)
> +
> +#define TEGRA2_MMC_TRNMOD_CMD_CRC_CHECK_EN_MASK (1 <<
> 3)
> +#define TEGRA2_MMC_TRNMOD_CMD_CRC_CHECK_EN_DISABLE (0 << 3)
> +#define TEGRA2_MMC_TRNMOD_CMD_CRC_CHECK_EN_ENABLE (1 << 3)
> +
> +#define TEGRA2_MMC_TRNMOD_CMD_INDEX_CHECK_EN_MASK (1 << 4)
> +#define TEGRA2_MMC_TRNMOD_CMD_INDEX_CHECK_EN_DISABLE (0 << 4)
> +#define TEGRA2_MMC_TRNMOD_CMD_INDEX_CHECK_EN_ENABLE (1 << 4)
> +
> +#define TEGRA2_MMC_TRNMOD_DATA_PRESENT_SELECT_MASK (1 << 5)
> +#define TEGRA2_MMC_TRNMOD_DATA_PRESENT_SELECT_NO_DATA_TRANSFER (0 << 5)
> +#define TEGRA2_MMC_TRNMOD_DATA_PRESENT_SELECT_DATA_TRANSFER (1 << 5)
> +
> +#define TEGRA2_MMC_PRNSTS_CMD_INHIBIT_CMD_MASK (1 << 0)
> +#define TEGRA2_MMC_PRNSTS_CMD_INHIBIT_CMD_INACTIVE (0 << 0)
> +#define TEGRA2_MMC_PRNSTS_CMD_INHIBIT_CMD_ACTIVE (1 << 0)
> +
> +#define TEGRA2_MMC_PRNSTS_CMD_INHIBIT_DAT_MASK (1 << 1)
> +#define TEGRA2_MMC_PRNSTS_CMD_INHIBIT_DAT_INACTIVE (0 << 1)
> +#define TEGRA2_MMC_PRNSTS_CMD_INHIBIT_DAT_ACTIVE (1 << 1)
> +
> +#define TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_EN_MASK (1 << 0)
> +#define TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_EN_DISABLE (0 << 0)
> +#define TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_EN_ENABLE (1 << 0)
> +
> +#define TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_STABLE_MASK (1 << 1)
> +#define TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_STABLE_NOT_READY (0 << 1)
> +#define TEGRA2_MMC_CLKCON_INTERNAL_CLOCK_STABLE_READY (1 << 1)
> +
> +#define TEGRA2_MMC_CLKCON_SD_CLOCK_EN_MASK (1 << 2)
> +#define TEGRA2_MMC_CLKCON_SD_CLOCK_EN_DISABLE (0 << 2)
> +#define TEGRA2_MMC_CLKCON_SD_CLOCK_EN_ENABLE (1 << 2)
> +
> +#define TEGRA2_MMC_CLKCON_SDCLK_FREQ_SEL_SHIFT 8
> +#define TEGRA2_MMC_CLKCON_SDCLK_FREQ_SEL_MASK (0xff << 8)
> +
> +#define TEGRA2_MMC_SWRST_SW_RESET_FOR_ALL (1 << 0)
> +#define TEGRA2_MMC_SWRST_SW_RESET_FOR_CMD_LINE (1 << 1)
> +#define TEGRA2_MMC_SWRST_SW_RESET_FOR_DAT_LINE (1 << 2)
> +
> +#define TEGRA2_MMC_NORINTSTS_CMD_COMPLETE (1 << 0)
> +#define TEGRA2_MMC_NORINTSTS_XFER_COMPLETE (1 << 1)
> +#define TEGRA2_MMC_NORINTSTS_DMA_INTERRUPT (1 << 3)
> +#define TEGRA2_MMC_NORINTSTS_ERR_INTERRUPT (1 << 15)
> +#define TEGRA2_MMC_NORINTSTS_CMD_TIMEOUT_ERR (1 << 16)
> +
> +#define TEGRA2_MMC_NORINTSTSEN_CMD_COMPLETE (1 << 0)
> +#define TEGRA2_MMC_NORINTSTSEN_XFER_COMPLETE (1 << 1)
> +#define TEGRA2_MMC_NORINTSTSEN_DMA_INTERRUPT (1 << 3)
> +#define TEGRA2_MMC_NORINTSTSEN_BUFFER_WRITE_READY (1 << 4)
> +#define TEGRA2_MMC_NORINTSTSEN_BUFFER_READ_READY (1 << 5)
> +
> +#define TEGRA2_MMC_NORINTSIGEN_XFER_COMPLETE (1 << 1)
> +
> struct mmc_host {
> struct tegra2_mmc *reg;
> unsigned int version; /* SDHCI spec. version */
> Tom
>
> --
> 1.7.3.1
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information. Any unauthorized review, use, disclosure or
> distribution
> is prohibited. If you are not the intended recipient, please contact the
> sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
