[U-Boot] [PATCH v2 1/4] Tegra2: mmc: define register field values in tegra2_mmc.h

Anton Staaf robotboy at chromium.org
Thu Nov 10 18:38:58 CET 2011


On Thu, Nov 10, 2011 at 8:28 AM, Tom Warren <TWarren at nvidia.com> wrote:
> Anton,
>
> -----Original Message-----
> From: Anton Staaf [mailto:robotboy at chromium.org]
> Sent: Wednesday, November 09, 2011 12:46 PM
> To: u-boot at 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 <robotboy at chromium.org>
> Cc: Andy Fleming <afleming at gmail.com>
> Cc: Tom Warren <twarren at nvidia.com>
> Cc: Stephen Warren <swarren at nvidia.com>
> Cc: Albert Aribaud <albert.u.boot at 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.
> -----------------------------------------------------------------------------------
>


More information about the U-Boot mailing list