Hi Ulf,
On 2017/3/15 21:39, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
> <gregory.clement@xxxxxxxxxxxxxxxxxx> wrote:
>> From: Hu Ziji <huziji@xxxxxxxxxxx>
>>
<snip>
>> +
>> + /*
>> + * FIXME: should depends on the specific board timing.
>> + */
>> + if ((timing == MMC_TIMING_MMC_HS400) ||
>> + (timing == MMC_TIMING_MMC_HS200) ||
>> + (timing == MMC_TIMING_UHS_SDR50) ||
>> + (timing == MMC_TIMING_UHS_SDR104) ||
>> + (timing == MMC_TIMING_UHS_DDR50) ||
>> + (timing == MMC_TIMING_UHS_SDR25) ||
>> + (timing == MMC_TIMING_MMC_DDR52)) {
>> + reg = sdhci_readl(host, phy_regs->timing_adj);
>> + reg &= ~XENON_OUTPUT_QSN_PHASE_SELECT;
>> + sdhci_writel(host, reg, phy_regs->timing_adj);
>> + }
>> +
<snip>
>> +
>> + reg = sdhci_readl(host, phy_regs->func_ctrl);
>> + if ((timing == MMC_TIMING_UHS_DDR50) ||
>> + (timing == MMC_TIMING_MMC_HS400) ||
>> + (timing == MMC_TIMING_MMC_DDR52))
>> + reg |= (XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) |
>> + XENON_CMD_DDR_MODE;
>> + else
>> + reg &= ~((XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) |
>> + XENON_CMD_DDR_MODE);
<snip>
>
> Again you are checking the MMC_TIMING_* in several if statements.
> Perhaps you can use switch statement instead and build a couple of
> different register variables, that you use when reading/writing
> afterwards.
>
Sorry. I don't quite understand your comment.
Do you mean combine all the related register settings
under a specific MMC_TIMING_*?
swhich (timing) {
case MMC_TIMING_HS400:
config reg_1;
config reg_2;
...
case MMC_TIMING_HS200:
config reg_2;
config reg_3
..
case ...
};
I have a little concern that there might be a lot
of duplicate register settings. Some PHY operations are
irrelevant to the speed mode. They have to be executed in
a particular sequence in all speed modes.
>> +}
>> +
>> +static int emmc_phy_parse_param_dt(struct sdhci_host *host,
>> + struct device_node *np,
>> + struct emmc_phy_params *params)
>> +{
>> + u32 value;
>> +
>> + if (of_property_read_bool(np, "marvell,xenon-phy-slow-mode"))
>> + params->slow_mode = true;
>> + else
>> + params->slow_mode = false;
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-phy-znr", &value))
>> + params->znr = value & XENON_ZNR_MASK;
>> + else
>> + params->znr = XENON_ZNR_DEF_VALUE;
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-phy-zpr", &value))
>> + params->zpr = value & XENON_ZPR_MASK;
>> + else
>> + params->zpr = XENON_ZPR_DEF_VALUE;
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-phy-nr-success-tun",
>> + &value))
>> + params->nr_tun_times = value & XENON_TUN_CONSECUTIVE_TIMES_MASK;
>> + else
>> + params->nr_tun_times = XENON_TUN_CONSECUTIVE_TIMES;
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-phy-tun-step-divider",
>> + &value))
>> + params->tun_step_divider = value & 0xFF;
>> + else
>> + params->tun_step_divider = XENON_TUNING_STEP_DIVIDER;
>> +
>> + return 0;
>
> Instead of all these if-else, let's start by assigning default values
> instead. This makes the code more readable.
>
Sure. I'll change it.
>> +}
>> +
>> +/*
>> + * Setting PHY when card is working in High Speed Mode.
>> + * HS400 set data strobe line.
>> + * HS200/SDR104 set tuning config to prepare for tuning.
>> + */
>> +static int xenon_hs_delay_adj(struct sdhci_host *host)
>> +{
>> + int ret = 0;
>> +
>> + if (WARN_ON(host->clock <= XENON_DEFAULT_SDCLK_FREQ))
>> + return -EINVAL;
>> +
>> + if (host->timing == MMC_TIMING_MMC_HS400) {
>> + emmc_phy_strobe_delay_adj(host);
>> + return 0;
>> + }
>> +
>> + if ((host->timing == MMC_TIMING_MMC_HS200) ||
>> + (host->timing == MMC_TIMING_UHS_SDR104))
>> + return emmc_phy_config_tuning(host);
>> +
>> + /*
>> + * DDR Mode requires driver to scan Sampling Fixed Delay Line,
>> + * to find out a perfect operation sampling point.
>> + * It is hard to implement such a scan in host driver since initiating
>> + * commands by host driver is not safe.
>> + * Thus so far just keep PHY Sampling Fixed Delay in default value
>> + * in DDR mode.
>> + *
>> + * If any timing issue occurs in DDR mode on Marvell products,
>> + * please contact maintainer to ask for internal support in Marvell.
>> + */
>> + if ((host->timing == MMC_TIMING_MMC_DDR52) ||
>> + (host->timing == MMC_TIMING_UHS_DDR50))
>> + dev_warn_once(mmc_dev(host->mmc), "Timing issue might occur in DDR mode\n");
>
> Please consider a to convert to use a switch statment instead of the
> several if statements.
>
It is a good idea to use switch here.
>> + return ret;
>> +}
>> +
>
<snip>
>> +int xenon_phy_parse_dt(struct device_node *np, struct sdhci_host *host)
>> +{
>> + const char *phy_type = NULL;
>> +
>> + if (!of_property_read_string(np, "marvell,xenon-phy-type", &phy_type))
>> + return add_xenon_phy(np, host, phy_type);
>> +
>> + dev_info(mmc_dev(host->mmc), "Fail to get Xenon PHY type. Use default eMMC 5.1 PHY\n");
>
> Isn't there already a dev_err() being printed for this case. You
> probably only need one.
By default, "emmc 5.1 phy" is used on most of our products.
Thus this dt property is an optional one.
I can remove this dev_info.
>
>> + return add_xenon_phy(np, host, "emmc 5.1 phy");
>> +}
>
> [...]
>
> Kind regards
> Uffe
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
