On 17/08/16 19:09, Douglas Anderson wrote:
>>From empirical evidence (tested on Rockchip rk3399), it appears that the
> PHY intended to be used with the Arasan SDHCI 5.1 controller has trouble
> turning on when the card clock is slow or off. Strangely these problems
> appear to show up consistently on some boards while other boares work
boares -> boards
> fine, but on the boards where it shows up the problem reproduces 100% of
> the time and is quite consistent in its behavior.
>
> These problems can be fixed by always making sure that we power on the
> PHY (and turn on its DLL) when the card clock is faster than about 50
> MHz. Once on, we need to make sure that we never power down the PHY /
> turn off its DLL until the clock is faster again.
>
> We'll add logic for handling this into the sdhci-of-arasan driver. Note
> that right now the only user of a PHY in the sdhci-of-arasan driver is
> arasan,sdhci-5.1. It's presumed that all arasan,sdhci-5.1 PHY
> implementations need this workaround, so the logic is only contingent on
> having a PHY to control. If future Arasan controllers don't have this
> problem we can add code to decide if we want this flow or not.
>
> Also note that we check for slow clocks by checking for <= 400 kHz
> rather than checking for 50 MHz. This keeps things the most consistent
> and also means we can power the PHY on at max speed (where the DLL will
> lock fastest). Presumably anyone who intends to run with a card clock
> of < 50 MHz and > 400 Khz will be running on a device where this problem
Khz -> kHz
> is fixed anyway.
>
> I believe this brings some resolution to the problems reported before.
> See the commit 6fc09244d74d ("mmc: sdhci-of-arasan: Revert: Always power
> the PHY off/on when clock changes").
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
Apart from spelling and a define for 400000:
Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
> Tested on chromeos kernel-4.4 with backports.
>
> drivers/mmc/host/sdhci-of-arasan.c | 61 ++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index e0f193f7e3e5..ec2fd00a913d 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -77,6 +77,7 @@ struct sdhci_arasan_soc_ctl_map {
> * @host: Pointer to the main SDHCI host structure.
> * @clk_ahb: Pointer to the AHB clock
> * @phy: Pointer to the generic phy
> + * @is_phy_on: True if the PHY is on; false if not.
> * @sdcardclk_hw: Struct for the clock we might provide to a PHY.
> * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw.
> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers.
> @@ -86,6 +87,7 @@ struct sdhci_arasan_data {
> struct sdhci_host *host;
> struct clk *clk_ahb;
> struct phy *phy;
> + bool is_phy_on;
>
> struct clk_hw sdcardclk_hw;
> struct clk *sdcardclk;
> @@ -170,13 +172,47 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
> struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> bool ctrl_phy = false;
>
> - if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy)))
> - ctrl_phy = true;
> + if (!IS_ERR(sdhci_arasan->phy)) {
> + if (!sdhci_arasan->is_phy_on && clock <= 400000) {
Should probably use a #define for 400000
> + /*
> + * If PHY off, set clock to max speed and power PHY on.
> + *
> + * Although PHY docs apparently suggest power cycling
> + * when changing the clock the PHY doesn't like to be
> + * powered on while at low speeds like those used in ID
> + * mode. Even worse is powering the PHY on while the
> + * clock is off.
> + *
> + * To workaround the PHY limitatoins, the best we can
limitatoins -> limitations
> + * do is to power it on at a faster speed and then slam
> + * through low speeds without power cycling.
> + */
> + sdhci_set_clock(host, host->max_clk);
> + spin_unlock_irq(&host->lock);
> + phy_power_on(sdhci_arasan->phy);
> + spin_lock_irq(&host->lock);
> + sdhci_arasan->is_phy_on = true;
> +
> + /*
> + * We'll now fall through to the below case with
> + * ctrl_phy = false (so we won't turn off/on). The
> + * sdhci_set_clock() will set the real clock.
> + */
> + } else if (clock > 400000) {
> + /*
> + * At higher clock speeds the PHY is fine being power
> + * cycled and docs say you _should_ power cycle when
> + * changing clock speeds.
> + */
> + ctrl_phy = true;
> + }
> + }
>
> - if (ctrl_phy) {
> + if (ctrl_phy && sdhci_arasan->is_phy_on) {
> spin_unlock_irq(&host->lock);
> phy_power_off(sdhci_arasan->phy);
> spin_lock_irq(&host->lock);
> + sdhci_arasan->is_phy_on = false;
> }
>
> sdhci_set_clock(host, clock);
> @@ -185,6 +221,7 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
> spin_unlock_irq(&host->lock);
> phy_power_on(sdhci_arasan->phy);
> spin_lock_irq(&host->lock);
> + sdhci_arasan->is_phy_on = true;
> }
> }
>
> @@ -239,13 +276,14 @@ static int sdhci_arasan_suspend(struct device *dev)
> if (ret)
> return ret;
>
> - if (!IS_ERR(sdhci_arasan->phy)) {
> + if (!IS_ERR(sdhci_arasan->phy) && sdhci_arasan->is_phy_on) {
> ret = phy_power_off(sdhci_arasan->phy);
> if (ret) {
> dev_err(dev, "Cannot power off phy.\n");
> sdhci_resume_host(host);
> return ret;
> }
> + sdhci_arasan->is_phy_on = false;
> }
>
> clk_disable(pltfm_host->clk);
> @@ -281,12 +319,13 @@ static int sdhci_arasan_resume(struct device *dev)
> return ret;
> }
>
> - if (!IS_ERR(sdhci_arasan->phy)) {
> + if (!IS_ERR(sdhci_arasan->phy) && host->mmc->actual_clock) {
> ret = phy_power_on(sdhci_arasan->phy);
> if (ret) {
> dev_err(dev, "Cannot power on phy.\n");
> return ret;
> }
> + sdhci_arasan->is_phy_on = true;
> }
>
> return sdhci_resume_host(host);
> @@ -547,12 +586,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
> goto unreg_clk;
> }
>
> - ret = phy_power_on(sdhci_arasan->phy);
> - if (ret < 0) {
> - dev_err(&pdev->dev, "phy_power_on err.\n");
> - goto err_phy_power;
> - }
> -
> host->mmc_host_ops.hs400_enhanced_strobe =
> sdhci_arasan_hs400_enhanced_strobe;
> }
> @@ -565,9 +598,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>
> err_add_host:
> if (!IS_ERR(sdhci_arasan->phy))
> - phy_power_off(sdhci_arasan->phy);
> -err_phy_power:
> - if (!IS_ERR(sdhci_arasan->phy))
> phy_exit(sdhci_arasan->phy);
> unreg_clk:
> sdhci_arasan_unregister_sdclk(&pdev->dev);
> @@ -589,7 +619,8 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
> struct clk *clk_ahb = sdhci_arasan->clk_ahb;
>
> if (!IS_ERR(sdhci_arasan->phy)) {
> - phy_power_off(sdhci_arasan->phy);
> + if (sdhci_arasan->is_phy_on)
> + phy_power_off(sdhci_arasan->phy);
> phy_exit(sdhci_arasan->phy);
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
