[U-Boot] [PATCH v2 4/5] Add Support for qc_mmc MMC Controller
Andy Fleming
afleming at gmail.com
Fri May 4 00:05:18 CEST 2012
NAK, allow me to explain below:
On Mon, Mar 5, 2012 at 8:40 AM, Mohamed Haneef
<mohamed.haneef at lntinfotech.com> wrote:
> * Support for qc_mmc MMC Controller
>
> Signed-off-by: Mohamed Haneef <mohamed.haneef at lntinfotech.com>
> ---
> +#ifndef __MMC_H__
> +#define __MMC_H__
> +
> +#ifndef MMC_SLOT
> +#define MMC_SLOT 0
> +#endif
This sort of thing should probably be passed in by the board code.
> +
> +#if 0
> +#define MSM_SDC1_BASE 0x12400000
> +#define MSM_SDC2_BASE 0x12140000
> +#define MSM_SDC3_BASE 0x12180000
> +#define MSM_SDC4_BASE 0x121C0000
> +#endif
Why's this still here?
> diff --git a/drivers/mmc/qc_mmc.c b/drivers/mmc/qc_mmc.c
> new file mode 100644
> index 0000000..7b7a1ed
> --- /dev/null
> +++ b/drivers/mmc/qc_mmc.c
> @@ -0,0 +1,584 @@
> +#if MMC_BOOT_ADM
> +#include "adm.h"
> +#endif
> +
> +#ifndef NULL
> +#define NULL 0
> +#endif
?? Is NULL ever undefined in C?
> +
> +#define MMC_BOOT_DATA_READ 0
> +#define MMC_BOOT_DATA_WRITE 1
> +/*
> + * Calculates the address of registers according to the base address
> + */
> +unsigned long int mmc_boot_mci_reg(unsigned long base, unsigned long offset)
> +{
> + return base + offset;
> +}
> +
> +
> +/*
> + * Sets a timeout for read operation.
> + */
> +static unsigned int mmc_boot_set_read_timeout(struct mmc *mmc)
> +{
> + if (mmc == NULL)
> + return MMC_BOOT_E_INVAL;
> +
> + /* todo: Add a check for HC, only if HC do this.
> + * If not, taac and nsac must be taken into account
> + */
> + ((struct mmc_priv *)(mmc->priv))->rd_timeout_ns = 100000000;
> + debug(" Read timeout set: %d ns\n",
> + ((struct mmc_priv *)(mmc->priv))->rd_timeout_ns);
I think it's preferable to do:
struct mmc_priv *priv = mmc->priv;
priv->rd_timeout_ns = 100000000;
> +
> + return MMC_BOOT_E_SUCCESS;
> +}
> +
> +/*
> + * Check to ensure that there is no alignment or data length errors
> + */
> +static unsigned int mmc_boot_check_read_data(struct mmc_cmd *cmd)
> +{
> +
> + if (cmd->response[0] & MMC_BOOT_R1_BLOCK_LEN_ERR)
> + return MMC_BOOT_E_BLOCKLEN_ERR;
> +
> + /* Misaligned address not matching block length */
> + if (cmd->response[0] & MMC_BOOT_R1_ADDR_ERR)
> + return MMC_BOOT_E_ADDRESS_ERR;
> +
> + return MMC_BOOT_E_SUCCESS;
> +}
> +
> +/*
> + * Decode type of error caused during read and write
> + */
> +static unsigned int mmc_boot_status_error(unsigned mmc_status)
> +{
> + unsigned int mmc_ret = MMC_BOOT_E_SUCCESS;
> +
> + /* If DATA_CRC_FAIL bit is set to 1 then CRC error
> + * was detected by card/device during the data transfer
> + */
> + if (mmc_status & MMC_BOOT_MCI_STAT_DATA_CRC_FAIL)
> + mmc_ret = MMC_BOOT_E_DATA_CRC_FAIL;
> + /* If DATA_TIMEOUT bit is set to 1 then the data transfer time
> + * exceeded the data timeout period without completing the
> + * transfer
> + */
> + else if (mmc_status & MMC_BOOT_MCI_STAT_DATA_TIMEOUT)
> + mmc_ret = MMC_BOOT_E_DATA_TIMEOUT;
> + /* If RX_OVERRUN bit is set to 1 then SDCC2 tried to
> + * receive data from the card before empty storage for new
> + * received data was available.
> + * Verify that bit FLOW_ENA in MCI_CLK is set to 1 during
> + * the data xfer.
> + */
> + else if (mmc_status & MMC_BOOT_MCI_STAT_RX_OVRRUN)
> + /* Note: We've set FLOW_ENA bit in MCI_CLK to 1.
> + * so no need to verify for now
> + */
> + mmc_ret = MMC_BOOT_E_RX_OVRRUN;
> + /* If TX_UNDERRUN bit is set to 1 then SDCC2 tried to send
> + * data to the card before new data for sending was available.
> + * Verify that bit FLOW_ENA in MCI_CLK
> + * is set to 1 during the data xfer.
> + */
> + else if (mmc_status & MMC_BOOT_MCI_STAT_TX_UNDRUN)
> + /* Note: We've set FLOW_ENA bit in MCI_CLK to 1.
> + * so skipping it now
> + */
> + mmc_ret = MMC_BOOT_E_RX_OVRRUN;
> +
> + return mmc_ret;
> +}
> +
> +/*
> + * Read data to SDC FIFO.
> + */
> +static unsigned int mmc_boot_fifo_read(unsigned int *mmc_ptr,
> + unsigned int data_len,
> + struct mmc *mmc)
> +{
Why is mmc_ptr an unsigned int *?
> + unsigned int mmc_ret = MMC_BOOT_E_SUCCESS;
> + unsigned int mmc_status = 0;
> + unsigned int mmc_count = 0;
> + unsigned int read_error = MMC_BOOT_MCI_STAT_DATA_CRC_FAIL | \
> + MMC_BOOT_MCI_STAT_DATA_TIMEOUT | \
> + MMC_BOOT_MCI_STAT_RX_OVRRUN;
> + unsigned int i;
> + struct mmc_priv *priv = (struct mmc_priv *)mmc->priv;
Unnecessary cast
> + unsigned long reg_status, reg_fifo;
> +
> + reg_status = mmc_boot_mci_reg(priv->base, MMC_BOOT_MCI_STATUS);
> + reg_fifo = mmc_boot_mci_reg(priv->base, MMC_BOOT_MCI_FIFO);
> +
> + /* Read the data from the MCI_FIFO register as long as
> + * RXDATA_AVLBL bit of MCI_STATUS register is set to 1 and bits
> + * DATA_CRC_FAIL, DATA_TIMEOUT, RX_OVERRUN of MCI_STATUS
> + * register are cleared to 0.
> + * Continue the reads until the whole transfer data is received
> + */
> +
> + do {
> + mmc_ret = MMC_BOOT_E_SUCCESS;
> + mmc_status = readl(reg_status);
> +
> + if (mmc_status & read_error) {
> + mmc_ret = mmc_boot_status_error(mmc_status);
> + break;
> + }
> +
> + if (mmc_status & MMC_BOOT_MCI_STAT_RX_DATA_AVLBL) {
> + unsigned read_count = 1;
> + if (mmc_status & MMC_BOOT_MCI_STAT_RX_FIFO_HFULL)
> + read_count = MMC_BOOT_MCI_HFIFO_COUNT;
> +
> + for (i = 0; i < read_count; i++) {
> + /* FIFO contains 16 32-bit data buffer
> + * on 16 sequential addresses
> + */
> + *mmc_ptr = readl(reg_fifo +
> + (mmc_count % MMC_BOOT_MCI_FIFO_SIZE));
> + mmc_ptr++;
> + /* increase mmc_count by word size */
> + mmc_count += sizeof(unsigned int);
I don't remember if readl is explicitly sized, but it seems like there
are some implicit and explicit size dependencies, and they aren't
currently guaranteed to line up. If you're going to do this, I think
mmc_ptr should be u32*. And you should update mmc_count by
sizeof(*mmc_ptr) to lock in the connection between the increase in
mmc_count and the amount of data read.
Out of curiosity, I notice that you will read from the start of the
FIFO each time this function is called. Is there something that resets
the FIFO to put the oldest data at the first address? Or is this only
called once?
> +static void mmc_boot_init_data(struct mmc *mmc, struct mmc_cmd *cmd,
> + struct mmc_data *data)
> +{
> + uint32_t data_ctrl, mmc_reg, data_len;
> + struct mmc_priv *priv = (struct mmc_priv *)mmc->priv;
Unnecessary cast
> + unsigned long reg;
> +
> + /* Write data timeout period to MCI_DATA_TIMER register. */
> + /* Data timeout period should be in card bus clock periods */
> + mmc_reg = (unsigned long)(((struct mmc_priv *)
> + (mmc->priv))->rd_timeout_ns /
> + 1000000) * (mmc->clock / 1000);
You've got priv already, why access it via complicated cast?
> + /* add some extra clock cycles to be safe */
> + mmc_reg += 1000;
> + mmc_reg = mmc_reg/2;
I'd prefer that the variables have names that indicate their purpose
(e.g. - mmc_timeout). It took me a few times looking at this to
realize you were updating a variable that will be written to a reg,
rather than a pointer to your mmc register space.
Also, I'm assuming that the register defines the timeout field so that
you need ns/2, but you might want to document that as why you divide
by 2.
> +
> + reg = mmc_boot_mci_reg(priv->base, MMC_BOOT_MCI_DATA_TIMER);
> + writel(mmc_reg, reg);
If you're going to repeat a sequence like this, frequently, it might
be better to create a qc_mmc_write() function which takes priv, reg,
and value as arguments:
qc_mmc_write(priv, MMC_BOOT_MCI_DATA_TIMER, mmc_reg);
BTW, are all of these prefixed with MMC_BOOT because this is only used
at boot time? Seems limiting. And it makes the constant names very
long.
> +
> + /* Write the total size of the transfer data to MCI_DATA_LENGTH
> + * register. For block xfer it must be multiple of the block
> + * size.
> + */
> +
> + data_len = data->blocks*data->blocksize;
> + reg = mmc_boot_mci_reg(priv->base, MMC_BOOT_MCI_DATA_LENGTH);
> + writel(data_len, reg);
> +
> + /* Writes to MCI port are not effective for 3 ticks of PCLK.
> + * ticks calculated using 400KHz clock speed
> + */
> + udelay(8);
> +
> + data_ctrl = MMC_BOOT_MCI_DATA_ENABLE |
> + (data->blocksize << MMC_BOOT_MCI_BLKSIZE_POS);
> +
> +
> + if (data->flags == MMC_DATA_READ)
> + data_ctrl |= MMC_BOOT_MCI_DATA_DIR;
> +
> + reg = mmc_boot_mci_reg(priv->base, MMC_BOOT_MCI_DATA_CTL);
> + writel(data_ctrl, reg);
> +
> + /* Writes to MCI port are not effective for 3 ticks of PCLK.
> + * ticks calculated using 400KHz clock speed
> + */
> + udelay(8);
> +
> +}
> +
> +/*
> + * Function to map mmc send command to board send command
> + */
This sounds...odd.
> +int mmc_boot_send_command_map(struct mmc *mmc,
> + struct mmc_cmd *cmd,
> + struct mmc_data *data)
> +{
> + unsigned int mmc_ret = MMC_BOOT_E_SUCCESS;
> +
> + /* todo: do we need to fill in command type ?? */
> +
> + if (cmd->cmdidx == MMC_CMD_SEND_STATUS) {
> + /* u-boot doesn't fill in the correct argument value */
> + cmd->cmdarg = mmc->rca << 16;
I'm not sure what this means. But if you think U-Boot should be
filling in a different value, we should change it in mmc.c, not
one-at-a-time in each driver...
> + /* this is to explicitly disable the prg enabled flag */
> + cmd->flags &= ~MMC_BOOT_PROGRAM_ENABLED;
1) I'm not sure this field is ever used for anything
2) You shouldn't define your own constants for inspecting/setting data
in fields that haven't been explicitly set aside for driver-specific
implementation. While it doesn't look like any of the mmc code ever
sets this field, it could one day, and break your driver.
> +
> + } else if (cmd->cmdidx == MMC_CMD_READ_SINGLE_BLOCK) {
> + if (mmc->read_bl_len != MMC_BOOT_RD_BLOCK_LEN &&
> + mmc->version & SD_VERSION_SD) {
> + mmc->read_bl_len = MMC_BOOT_RD_BLOCK_LEN;
> + data->blocksize = MMC_BOOT_RD_BLOCK_LEN;
> + }
> + } else if (cmd->cmdidx == MMC_CMD_READ_MULTIPLE_BLOCK) {
> + if (mmc->read_bl_len != MMC_BOOT_RD_BLOCK_LEN &&
> + mmc->version & SD_VERSION_SD) {
> + mmc->read_bl_len = MMC_BOOT_RD_BLOCK_LEN;
> + data->blocksize = MMC_BOOT_RD_BLOCK_LEN;
> + }
> + } else if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION) {
> + /* explicitly disable the prg enabled flag */
> + cmd->flags &= ~MMC_BOOT_PROGRAM_ENABLED;
> + /* set the XFER mode */
> + cmd->flags |= MMC_BOOT_XFER_MODE_BLOCK;
> + }
> +
> +
> + /* For Data cmd's */
nit: no apostrophe needed
> +/*
> + * Initialize host structure, set and enable clock-rate and power mode.
> + */
> +unsigned int mmc_boot_init(struct mmc *mmc)
> +{
> + unsigned int mmc_pwr = 0;
> + struct mmc_priv *priv = (struct mmc_priv *) mmc->priv;
No cast
> + int mmc_slot = priv->instance;
Whenever a driver has to be told which instance it is, a kitten
dies.... Ok, after reading through I see that it's only used to set
clocking. That's probably fine. More on this later.
> +/*
> + * Board specific initializations
> + */
> +unsigned int mmc_boot_main(struct mmc *mmc)
> +{
> + unsigned int mmc_ret = MMC_BOOT_E_SUCCESS;
> +
> + /* Initialize necessary data structure and enable/set clock and power */
> + debug(" Initializing MMC host data structure and clock!\n");
> + mmc_ret = mmc_boot_init(mmc);
> + if (mmc_ret != MMC_BOOT_E_SUCCESS) {
> + printf("MMC Boot: Error Initializing MMC Card!!!\n");
> + return MMC_BOOT_E_FAILURE;
> + }
> +
> + return MMC_BOOT_E_SUCCESS;
> +}
Why not put the actual initialization function here? We usually want
to contain the driver information, here. It looks like you have two
different initialization paths, but there should be some way to codify
that so that future readers/writers can understand which parts of the
driver are being used under a given set of conditions. As it currently
stands, they will have to investigate the board file to figure it out.
Use the board init function to pass in parameters the driver can use
to make those choices. For instance, the
>
> The contents of this e-mail and any attachment(s) may contain confidential or privileged information for the intended recipient(s). Unintended recipients are prohibited from taking action on the basis of information in this e-mail and using or disseminating the information, and must notify the sender and delete it from their system. L&T Infotech will not accept responsibility or liability for the accuracy or completeness of, or the presence of any virus or disabling code in this e-mail"
These sorts of disclaimers really serve to irritate open source
developers. Also, it puts me in the legal void, as you neglected to CC
me, which means I might be an unintended recipient. In defiance of
your disclaimer, I decided to take action, anyway. Feel free to sue
me. :P
Andy
More information about the U-Boot
mailing list