| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <288b58dc-cce7-11c3-e946-7bf0ea5a4863@marvell.com>
Date: Wed, 15 Mar 2017 22:31:53 +0800
From: Ziji Hu <huziji@...vell.com>
To: Ulf Hansson <ulf.hansson@...aro.org>,
Gregory CLEMENT <gregory.clement@...e-electrons.com>
CC: Adrian Hunter <adrian.hunter@...el.com>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
Jason Cooper <jason@...edaemon.net>,
Andrew Lunn <andrew@...n.ch>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Mike Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...eaurora.org>,
linux-clk <linux-clk@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Jimmy Xu <zmxu@...vell.com>,
Jisheng Zhang <jszhang@...vell.com>,
Nadav Haklai <nadavh@...vell.com>, Ryan Gao <ygao@...vell.com>,
Doug Jones <dougj@...vell.com>, Victor Gu <xigu@...vell.com>,
"Wei(SOCP) Liu" <liuw@...vell.com>,
Wilson Ding <dingwei@...vell.com>,
Yehuda Yitschak <yehuday@...vell.com>,
Marcin Wojtas <mw@...ihalf.com>,
Hanna Hawa <hannah@...vell.com>,
Kostya Porotchkin <kostap@...vell.com>
Subject: Re: [PATCH v6 09/14] mmc: sdhci-xenon: Add support to PHYs of Marvell
Xenon SDHC
Hi Ulf,
On 2017/3/15 21:39, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
> <gregory.clement@...e-electrons.com> wrote:
>> From: Hu Ziji <huziji@...vell.com>
>>
<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
>
Powered by blists - more mailing lists