lkml.org 
[lkml]   [2014]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
    On 01/30, Georgi Djakov wrote:
    > @@ -75,17 +110,389 @@ struct sdhci_msm_host {
    > };
    >
    > /* MSM platform specific tuning */
    > -int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
    > +static inline int msm_dll_poll_ck_out_en(struct sdhci_host *host, u8 poll)
    > +{
    > + u32 wait_cnt = 50;
    > + u8 ck_out_en = 0;

    Looks like a useless assignment.

    > + struct mmc_host *mmc = host->mmc;
    > +
    > + /* poll for CK_OUT_EN bit. max. poll time = 50us */
    > + ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
    > + CORE_CK_OUT_EN);
    > +
    > + while (ck_out_en != poll) {
    > + if (--wait_cnt == 0) {
    > + dev_err(mmc_dev(mmc), "%s: CK_OUT_EN bit is not %d\n",
    > + mmc_hostname(mmc), poll);
    > + return -ETIMEDOUT;
    > + }
    > + udelay(1);
    > +
    > + ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
    > + CORE_CK_OUT_EN);
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +static int msm_config_cm_dll_phase(struct sdhci_host *host, u8 phase)
    > +{
    > + int rc = 0;

    Looks like a useless assignment.

    > + u8 grey_coded_phase_table[] = {
    > + 0x0, 0x1, 0x3, 0x2, 0x6, 0x7, 0x5, 0x4,
    > + 0xc, 0xd, 0xf, 0xe, 0xa, 0xb, 0x9, 0x8
    > + };

    This could be static const?

    > + unsigned long flags;
    > + u32 config;
    > + struct mmc_host *mmc = host->mmc;
    > +
    > + spin_lock_irqsave(&host->lock, flags);
    > +
    > + config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
    > + config &= ~(CORE_CDR_EN | CORE_CK_OUT_EN);
    > + config |= (CORE_CDR_EXT_EN | CORE_DLL_EN);
    > + writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
    > +
    > + /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '0' */
    > + rc = msm_dll_poll_ck_out_en(host, 0);
    > + if (rc)
    > + goto err_out;
    > +
    > + /*
    > + * Write the selected DLL clock output phase (0 ... 15)
    > + * to CDR_SELEXT bit field of DLL_CONFIG register.
    > + */
    > + writel_relaxed(((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
    > + & ~(0xF << 20))
    > + | (grey_coded_phase_table[phase] << 20)),
    > + host->ioaddr + CORE_DLL_CONFIG);

    Wow this is complicated. Can we please break this up into
    multiple lines? What does 0xf << 20 mean?

    > +
    > + /* Set CK_OUT_EN bit of DLL_CONFIG register to 1. */
    > + writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
    > + | CORE_CK_OUT_EN), host->ioaddr + CORE_DLL_CONFIG);
    > +
    [...]
    > +}
    > +
    > +static int msm_find_most_appropriate_phase(struct sdhci_host *host,
    > + u8 *phase_table, u8 total_phases)
    > +{
    [...]
    > +
    > + for (cnt = 0; cnt <= row_index; cnt++) {
    > + if (phases_per_row[cnt] > curr_max) {
    > + curr_max = phases_per_row[cnt];
    > + selected_row_index = cnt;
    > + }
    > + }
    > +
    > + i = ((curr_max * 3) / 4);

    Unnecessary extra parentheses here.

    > + if (i)
    > + i--;
    > +
    > + ret = (int)ranges[selected_row_index][i];

    Is this cast necessary?

    > +
    > + if (ret >= MAX_PHASES) {
    > + ret = -EINVAL;
    > + dev_err(mmc_dev(mmc), "%s: invalid phase selected=%d\n",
    > + mmc_hostname(mmc), ret);
    > + }
    > +
    > + return ret;
    > +}
    > +
    > +static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
    > +{
    > + u32 mclk_freq = 0;
    > +
    > + /* Program the MCLK value to MCLK_FREQ bit field */
    > + if (host->clock <= 112000000)
    > + mclk_freq = 0;
    > + else if (host->clock <= 125000000)
    > + mclk_freq = 1;
    > + else if (host->clock <= 137000000)
    > + mclk_freq = 2;
    > + else if (host->clock <= 150000000)
    > + mclk_freq = 3;
    > + else if (host->clock <= 162000000)
    > + mclk_freq = 4;
    > + else if (host->clock <= 175000000)
    > + mclk_freq = 5;
    > + else if (host->clock <= 187000000)
    > + mclk_freq = 6;
    > + else if (host->clock <= 200000000)
    > + mclk_freq = 7;

    This could be a case statement but I'm not sure it's any clearer.
    At least the range is specified.

    switch (host->clock) {
    case 0 ... 112000000:
    mclk_freq = 0;
    break;
    case 112000001 ... 125000000:
    mclk_freq = 1;
    break;
    ...

    > +
    > + writel_relaxed(((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
    > + & ~(7 << 24)) | (mclk_freq << 24)),
    > + host->ioaddr + CORE_DLL_CONFIG);

    This is also complicated. Can you split this up into multiple lines?

    > +int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
    > +{
    [...]
    > + do {
    > + struct mmc_command cmd = { 0 };
    > + struct mmc_data data = { 0 };
    > + struct mmc_request mrq = {
    > + .cmd = &cmd,
    > + .data = &data
    > + };
    > + struct scatterlist sg;
    > +
    > + /* set the phase in delay line hw block */
    > + rc = msm_config_cm_dll_phase(host, phase);
    > + if (rc)
    > + goto out;
    > +
    > + cmd.opcode = opcode;
    > + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
    > +
    > + data.blksz = size;
    > + data.blocks = 1;
    > + data.flags = MMC_DATA_READ;
    > + data.timeout_ns = 1000 * 1000 * 1000; /* 1 sec */

    NSEC_PER_SEC?

    > +
    > + data.sg = &sg;
    > + data.sg_len = 1;
    > + sg_init_one(&sg, data_buf, sizeof(data_buf));
    > + memset(data_buf, 0, sizeof(data_buf));
    > + mmc_wait_for_req(mmc, &mrq);
    > +
    > + if (!cmd.error && !data.error &&
    > + !memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
    > + /* tuning is successful at this tuning point */
    > + tuned_phases[tuned_phase_cnt++] = phase;
    > + dev_dbg(mmc_dev(mmc), "%s: found good phase = %d\n",
    > + mmc_hostname(mmc), phase);
    > + }
    > + } while (++phase < 16);

    ++phase < ARRAY_SIZE(tuned_phases) ?

    > +
    > + if (tuned_phase_cnt) {
    > + rc = msm_find_most_appropriate_phase(host, tuned_phases,
    > + tuned_phase_cnt);
    > + if (rc < 0)
    > + goto out;
    > + else
    > + phase = (u8) rc;

    Unnecessary cast?

    > +
    > + /*
    > + * Finally set the selected phase in delay
    > + * line hw block.
    > + */
    > + rc = msm_config_cm_dll_phase(host, phase);
    > + if (rc)
    > + goto out;
    > + dev_dbg(mmc_dev(mmc), "%s: setting the tuning phase to %d\n",
    > + mmc_hostname(mmc), phase);
    > + } else {
    > + if (--tuning_seq_cnt)
    > + goto retry;
    > + /* tuning failed */
    > + dev_dbg(mmc_dev(mmc), "%s: no tuning point found\n",
    > + mmc_hostname(mmc));
    > + rc = -EIO;
    > + }
    > +
    > +out:
    > + kfree(data_buf);
    > + return rc;
    > +}
    > +
    > #define MAX_PROP_SIZE 32
    > static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
    > struct sdhci_msm_reg_data *vreg, const char *vreg_name)

    --
    Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
    hosted by The Linux Foundation


    \
     
     \ /
      Last update: 2014-02-08 04:41    [W:6.326 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site