| 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: <CAPDyKFpx23wio-QmDrx277+YO2Jt2S0ZrokZ2E4K+oVURSR1Hg@mail.gmail.com>
Date: Thu, 26 Jan 2017 12:29:33 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: 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>,
Ziji Hu <huziji@...vell.com>, 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 v5 07/12] mmc: sdhci-xenon: Add support to PHYs of Marvell
Xenon SDHC.
[...]
> +
> +static inline bool temp_stage_hs200_to_hs400(struct sdhci_host *host,
> + struct sdhci_xenon_priv *priv)
> +{
> + /*
> + * Tmep stages from HS200 to HS400
> + * from HS200 to HS in 200MHz
> + * from 200MHz to 52MHz
> + */
> + if (((priv->timing == MMC_TIMING_MMC_HS200) &&
> + (host->timing == MMC_TIMING_MMC_HS)) ||
> + ((host->timing == MMC_TIMING_MMC_HS) &&
> + (priv->clock > host->clock)))
> + return true;
> +
> + return false;
> +}
> +
> +static inline bool temp_stage_hs400_to_h200(struct sdhci_host *host,
> + struct sdhci_xenon_priv *priv)
> +{
> + /*
> + * Temp stages from HS400 t0 HS200:
> + * from 200MHz to 52MHz in HS400
> + * from HS400 to HS DDR in 52MHz
> + * from HS DDR to HS in 52MHz
> + * from HS to HS200 in 52MHz
> + */
> + if (((priv->timing == MMC_TIMING_MMC_HS400) &&
> + ((host->clock == MMC_HIGH_52_MAX_DTR) ||
> + (host->timing == MMC_TIMING_MMC_DDR52))) ||
> + ((priv->timing == MMC_TIMING_MMC_DDR52) &&
> + (host->timing == MMC_TIMING_MMC_HS)) ||
> + ((host->timing == MMC_TIMING_MMC_HS200) &&
> + (host->clock == MMC_HIGH_52_MAX_DTR)))
> + return true;
> +
> + return false;
> +}
Both the above functions seems to be really fragile to use. If
anything changes in the core layer related to these sequences, you may
end up getting a wrong validated expression.
Perhaps an option would be to add one or two new mmc host ops, which
could be called during the related hs200/400 sequences that could make
this less fragile for you? What do you think?
> +
> +/*
> + * If eMMC PHY Slow Mode is required in lower speed mode in SDR mode
> + * (SDLCK < 55MHz), enable Slow Mode to bypass eMMC PHY.
> + * SDIO slower SDR mode also requires Slow Mode.
> + *
> + * If Slow Mode is enabled, return true.
> + * Otherwise, return false.
> + */
> +static bool emmc_phy_slow_mode(struct sdhci_host *host,
> + unsigned char timing)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + struct emmc_phy_params *params = priv->phy_params;
> + struct xenon_emmc_phy_regs *phy_regs = priv->emmc_phy_regs;
> + u32 reg;
> +
> + /* Skip temp stages from HS200 to HS400 */
> + if (temp_stage_hs200_to_hs400(host, priv))
> + return false;
> +
> + /* Skip temp stages from HS400 t0 HS200 */
> + if (temp_stage_hs400_to_h200(host, priv))
> + return false;
> +
> + reg = sdhci_readl(host, phy_regs->timing_adj);
> + /* Enable Slow Mode for SDIO in slower SDR mode */
> + if ((priv->init_card_type == MMC_TYPE_SDIO) &&
Ah, noticed that there is some specific things going on here for SDIO
here as well. So perhaps you do need to implement the ->init_card()
ops anyway. Which I suggested not to, for patch6. :-)
Although, there is one problem with using ->init_card(). That is
particularly for removable cards, as mmc_rescan() doesn't invoke
->init_card() when it removes a card, which means, you may use old and
thus wrong information about what kind of card in the next card
detection attempt. So perhaps ->init_card() needs to be called from
the core before it even figured out what kind of card it is trying to
detect, as to allow the callbacks to reset some data.
Or perhaps you can think of another clever way!?
> + ((timing == MMC_TIMING_UHS_SDR25) ||
> + (timing == MMC_TIMING_UHS_SDR12) ||
> + (timing == MMC_TIMING_SD_HS) ||
> + (timing == MMC_TIMING_LEGACY))) {
> + reg |= XENON_TIMING_ADJUST_SLOW_MODE;
> + sdhci_writel(host, reg, phy_regs->timing_adj);
> + return true;
> + }
> +
> + /* Check if Slow Mode is required in lower speed mode in SDR mode */
> + if (((timing == MMC_TIMING_UHS_SDR50) ||
> + (timing == MMC_TIMING_UHS_SDR25) ||
> + (timing == MMC_TIMING_UHS_SDR12) ||
> + (timing == MMC_TIMING_SD_HS) ||
> + (timing == MMC_TIMING_MMC_HS) ||
> + (timing == MMC_TIMING_LEGACY)) && params->slow_mode) {
> + reg |= XENON_TIMING_ADJUST_SLOW_MODE;
> + sdhci_writel(host, reg, phy_regs->timing_adj);
> + return true;
> + }
> +
> + reg &= ~XENON_TIMING_ADJUST_SLOW_MODE;
> + sdhci_writel(host, reg, phy_regs->timing_adj);
> + return false;
> +}
> +
[...]
Kind regards
Uffe
Powered by blists - more mailing lists