* [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series
@ 2016-06-27 17:39 Douglas Anderson
2016-06-27 17:39 ` [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes Douglas Anderson
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Douglas Anderson @ 2016-06-27 17:39 UTC (permalink / raw)
To: linux-arm-kernel
Despite positive testing across a number of machines and a number of
people, last week I got a report that my 150 MHz series was breaking
things on a handful of boards.
It turns out that reverting the patch to always power cycle across clock
changes fixes things and reverting that patch also doesn't actually
cause any known problems (and 150 MHz should even continue to work).
Although we really need to get to the bottom of things, it seems
expedient to revert while we're waiting for a better solution, hence
this series. Also in this series is further increase in the time we'll
wait for the DLL to stabilize (found during reboot stress testing) and a
fix to the way we handle the card clock being reported as 0 (needed for
suspend/resume because of the revert we just did).
It's expected that this series could go through Ulf's mmc tree (like the
previous) with Kishon's Ack as appropriate.
Douglas Anderson (3):
mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock
changes
phy: rockchip-emmc: Be tolerant to card clock of 0 in power on
phy: rockchip-emmc: Wait even longer for the DLL to lock
drivers/mmc/host/sdhci-of-arasan.c | 21 +++++++----
drivers/phy/phy-rockchip-emmc.c | 71 ++++++++++++++++++++++++++------------
2 files changed, 62 insertions(+), 30 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes 2016-06-27 17:39 [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Douglas Anderson @ 2016-06-27 17:39 ` Douglas Anderson 2016-07-21 10:09 ` Adrian Hunter 2016-06-27 17:39 ` [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on Douglas Anderson ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Douglas Anderson @ 2016-06-27 17:39 UTC (permalink / raw) To: linux-arm-kernel This reverts commit 4ac0d5f245e1 ("mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes"), resolving conflicts with other patches that have come after. It appears that on some boards / with some eMMC devices that the patch is causing problems. Presumably turning the phy off and on again at the wrong time while initially setting up the card is confusing the card, the host, or the PHY. We have lots of power cycles while initially setting up the card because the main sdhci driver often turns off the clock by clearing SDHCI_CLOCK_CARD_EN and then calls host->ops->set_clock() to set the clock again. With all of those, we ended up with lots of power cycles. Presumably the arguments made in the original patch still hold. That is, whenever the card clock is turned off and on again (or changed) we really should wait for the DLL to lock again. However, perhaps it's really not that critical for the lower speeds. It's possible that the right answer here is: * Whenever set_clock() is called we should double-check that the DLL is locked. * Whenever set_clock() is called and we're actually changing clocks we should do a power cycle around that. * When we're doing a power cycle just because the clock changed, we probably shouldn't do quite as many things (maybe don't need to recalibarate, etc). Unfortunately the interaction between SDHCI and the PHY is extremely limited because of the limited PHY API. The PHY does have a reference to the card clock and could theoretically register for notifications, except that our clock is query only (it uses CLK_GET_RATE_NOCACHE) and so can't really be notified about updates. I believe we would need a major redesign of clock handling in SDHCI core to do better than that, or we would need to make our one fake notifications. :( Let's hope that we can eventually get more information from Arasan on how all this should be handled before doing tons more work. Until then, let's get back to a known working state. Note that the rest of the patches in the 150 MHz series should still work fine even without this one. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/mmc/host/sdhci-of-arasan.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 678f316702e0..e0f193f7e3e5 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -77,7 +77,6 @@ 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 - * @phy_on: True if the PHY is turned on. * @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. @@ -87,7 +86,6 @@ struct sdhci_arasan_data { struct sdhci_host *host; struct clk *clk_ahb; struct phy *phy; - bool phy_on; struct clk_hw sdcardclk_hw; struct clk *sdcardclk; @@ -170,10 +168,12 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + bool ctrl_phy = false; - if (sdhci_arasan->phy_on && !IS_ERR(sdhci_arasan->phy)) { - sdhci_arasan->phy_on = false; + if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy))) + ctrl_phy = true; + if (ctrl_phy) { spin_unlock_irq(&host->lock); phy_power_off(sdhci_arasan->phy); spin_lock_irq(&host->lock); @@ -181,9 +181,7 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock) sdhci_set_clock(host, clock); - if (host->mmc->actual_clock && !IS_ERR(sdhci_arasan->phy)) { - sdhci_arasan->phy_on = true; - + if (ctrl_phy) { spin_unlock_irq(&host->lock); phy_power_on(sdhci_arasan->phy); spin_lock_irq(&host->lock); @@ -549,6 +547,12 @@ 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; } @@ -561,6 +565,9 @@ 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); -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes 2016-06-27 17:39 ` [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes Douglas Anderson @ 2016-07-21 10:09 ` Adrian Hunter 0 siblings, 0 replies; 13+ messages in thread From: Adrian Hunter @ 2016-07-21 10:09 UTC (permalink / raw) To: linux-arm-kernel On 27/06/16 20:39, Douglas Anderson wrote: > This reverts commit 4ac0d5f245e1 ("mmc: sdhci-of-arasan: Always power > the PHY off/on when clock changes"), resolving conflicts with other > patches that have come after. It appears that on some boards / with > some eMMC devices that the patch is causing problems. > > Presumably turning the phy off and on again at the wrong time while > initially setting up the card is confusing the card, the host, or the > PHY. We have lots of power cycles while initially setting up the card > because the main sdhci driver often turns off the clock by clearing > SDHCI_CLOCK_CARD_EN and then calls host->ops->set_clock() to set the > clock again. With all of those, we ended up with lots of power cycles. > > Presumably the arguments made in the original patch still hold. That > is, whenever the card clock is turned off and on again (or changed) we > really should wait for the DLL to lock again. However, perhaps it's > really not that critical for the lower speeds. > > It's possible that the right answer here is: > * Whenever set_clock() is called we should double-check that the DLL is > locked. > * Whenever set_clock() is called and we're actually changing clocks we > should do a power cycle around that. > * When we're doing a power cycle just because the clock changed, we > probably shouldn't do quite as many things (maybe don't need to > recalibarate, etc). > > Unfortunately the interaction between SDHCI and the PHY is extremely > limited because of the limited PHY API. The PHY does have a reference > to the card clock and could theoretically register for notifications, > except that our clock is query only (it uses CLK_GET_RATE_NOCACHE) and > so can't really be notified about updates. I believe we would need a > major redesign of clock handling in SDHCI core to do better than that, > or we would need to make our one fake notifications. :( > > Let's hope that we can eventually get more information from Arasan on > how all this should be handled before doing tons more work. Until then, > let's get back to a known working state. Note that the rest of the > patches in the 150 MHz series should still work fine even without this > one. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-of-arasan.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 678f316702e0..e0f193f7e3e5 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -77,7 +77,6 @@ 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 > - * @phy_on: True if the PHY is turned on. > * @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. > @@ -87,7 +86,6 @@ struct sdhci_arasan_data { > struct sdhci_host *host; > struct clk *clk_ahb; > struct phy *phy; > - bool phy_on; > > struct clk_hw sdcardclk_hw; > struct clk *sdcardclk; > @@ -170,10 +168,12 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > + bool ctrl_phy = false; > > - if (sdhci_arasan->phy_on && !IS_ERR(sdhci_arasan->phy)) { > - sdhci_arasan->phy_on = false; > + if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy))) > + ctrl_phy = true; > > + if (ctrl_phy) { > spin_unlock_irq(&host->lock); > phy_power_off(sdhci_arasan->phy); > spin_lock_irq(&host->lock); > @@ -181,9 +181,7 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock) > > sdhci_set_clock(host, clock); > > - if (host->mmc->actual_clock && !IS_ERR(sdhci_arasan->phy)) { > - sdhci_arasan->phy_on = true; > - > + if (ctrl_phy) { > spin_unlock_irq(&host->lock); > phy_power_on(sdhci_arasan->phy); > spin_lock_irq(&host->lock); > @@ -549,6 +547,12 @@ 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; > } > @@ -561,6 +565,9 @@ 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); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-06-27 17:39 [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Douglas Anderson 2016-06-27 17:39 ` [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes Douglas Anderson @ 2016-06-27 17:39 ` Douglas Anderson 2016-06-29 13:49 ` Kishon Vijay Abraham I 2016-06-27 17:39 ` [PATCH 3/3] phy: rockchip-emmc: Wait even longer for the DLL to lock Douglas Anderson 2016-07-25 8:49 ` [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Ulf Hansson 3 siblings, 1 reply; 13+ messages in thread From: Douglas Anderson @ 2016-06-27 17:39 UTC (permalink / raw) To: linux-arm-kernel It's possible that there are some reasons to turn the PHY on while the clock is 0. In this case we just won't wait for the DLL to lock. This is a bit of a stopgap until we figure out exactly when we're supposed to wait for the DLL to lock and when we're supposed to power cycle the PHY. Note: this patch should help with suspend/resume where the system will try to turn the PHY back on when the clock is 0. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c index 9dce958233a0..a2aa6aca7dec 100644 --- a/drivers/phy/phy-rockchip-emmc.c +++ b/drivers/phy/phy-rockchip-emmc.c @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) unsigned int caldone; unsigned int dllrdy; unsigned int freqsel = PHYCTRL_FREQSEL_200M; + unsigned long rate; unsigned long timeout; - if (rk_phy->emmcclk != NULL) { - unsigned long rate = clk_get_rate(rk_phy->emmcclk); + /* + * Keep phyctrl_pdb and phyctrl_endll low to allow + * initialization of CALIO state M/C DFFs + */ + regmap_write(rk_phy->reg_base, + rk_phy->reg_offset + GRF_EMMCPHY_CON6, + HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, + PHYCTRL_PDB_MASK, + PHYCTRL_PDB_SHIFT)); + regmap_write(rk_phy->reg_base, + rk_phy->reg_offset + GRF_EMMCPHY_CON6, + HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, + PHYCTRL_ENDLL_MASK, + PHYCTRL_ENDLL_SHIFT)); + + /* Already finish power_off above */ + if (on_off == PHYCTRL_PDB_PWR_OFF) + return 0; + + rate = clk_get_rate(rk_phy->emmcclk); + + if (rate != 0) { unsigned long ideal_rate; unsigned long diff; switch (rate) { - case 0 ... 74999999: + case 1 ... 74999999: ideal_rate = 50000000; freqsel = PHYCTRL_FREQSEL_50M; break; @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) } /* - * Keep phyctrl_pdb and phyctrl_endll low to allow - * initialization of CALIO state M/C DFFs - */ - regmap_write(rk_phy->reg_base, - rk_phy->reg_offset + GRF_EMMCPHY_CON6, - HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, - PHYCTRL_PDB_MASK, - PHYCTRL_PDB_SHIFT)); - regmap_write(rk_phy->reg_base, - rk_phy->reg_offset + GRF_EMMCPHY_CON6, - HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, - PHYCTRL_ENDLL_MASK, - PHYCTRL_ENDLL_SHIFT)); - - /* Already finish power_off above */ - if (on_off == PHYCTRL_PDB_PWR_OFF) - return 0; - - /* * According to the user manual, calpad calibration * cycle takes more than 2us without the minimal recommended * value, so we may need a little margin here @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE, PHYCTRL_ENDLL_MASK, PHYCTRL_ENDLL_SHIFT)); + + /* + * We turned on the DLL even though the rate was 0 because we the + * clock might be turned on later. ...but we can't wait for the DLL + * to lock when the rate is 0 because it will never lock with no + * input clock. + * + * Technically we should be checking the lock later when the clock + * is turned on, but for now we won't. + */ + if (rate == 0) + return 0; + /* * After enabling analog DLL circuits docs say that we need 10.2 us if * our source clock is at 50 MHz and that lock time scales linearly -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-06-27 17:39 ` [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on Douglas Anderson @ 2016-06-29 13:49 ` Kishon Vijay Abraham I 2016-06-29 15:18 ` Doug Anderson 0 siblings, 1 reply; 13+ messages in thread From: Kishon Vijay Abraham I @ 2016-06-29 13:49 UTC (permalink / raw) To: linux-arm-kernel Hi, On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote: > It's possible that there are some reasons to turn the PHY on while the > clock is 0. In this case we just won't wait for the DLL to lock. > > This is a bit of a stopgap until we figure out exactly when we're > supposed to wait for the DLL to lock and when we're supposed to power > cycle the PHY. > > Note: this patch should help with suspend/resume where the system will > try to turn the PHY back on when the clock is 0. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++--------------- > 1 file changed, 37 insertions(+), 22 deletions(-) > > diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c > index 9dce958233a0..a2aa6aca7dec 100644 > --- a/drivers/phy/phy-rockchip-emmc.c > +++ b/drivers/phy/phy-rockchip-emmc.c > @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) > unsigned int caldone; > unsigned int dllrdy; > unsigned int freqsel = PHYCTRL_FREQSEL_200M; > + unsigned long rate; > unsigned long timeout; > > - if (rk_phy->emmcclk != NULL) { > - unsigned long rate = clk_get_rate(rk_phy->emmcclk); > + /* > + * Keep phyctrl_pdb and phyctrl_endll low to allow > + * initialization of CALIO state M/C DFFs > + */ > + regmap_write(rk_phy->reg_base, > + rk_phy->reg_offset + GRF_EMMCPHY_CON6, > + HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, > + PHYCTRL_PDB_MASK, > + PHYCTRL_PDB_SHIFT)); > + regmap_write(rk_phy->reg_base, > + rk_phy->reg_offset + GRF_EMMCPHY_CON6, > + HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, > + PHYCTRL_ENDLL_MASK, > + PHYCTRL_ENDLL_SHIFT)); > + > + /* Already finish power_off above */ > + if (on_off == PHYCTRL_PDB_PWR_OFF) > + return 0; > + > + rate = clk_get_rate(rk_phy->emmcclk); > + > + if (rate != 0) { > unsigned long ideal_rate; > unsigned long diff; > > switch (rate) { > - case 0 ... 74999999: > + case 1 ... 74999999: > ideal_rate = 50000000; > freqsel = PHYCTRL_FREQSEL_50M; > break; > @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) > } > > /* > - * Keep phyctrl_pdb and phyctrl_endll low to allow > - * initialization of CALIO state M/C DFFs > - */ > - regmap_write(rk_phy->reg_base, > - rk_phy->reg_offset + GRF_EMMCPHY_CON6, > - HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, > - PHYCTRL_PDB_MASK, > - PHYCTRL_PDB_SHIFT)); > - regmap_write(rk_phy->reg_base, > - rk_phy->reg_offset + GRF_EMMCPHY_CON6, > - HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, > - PHYCTRL_ENDLL_MASK, > - PHYCTRL_ENDLL_SHIFT)); > - > - /* Already finish power_off above */ > - if (on_off == PHYCTRL_PDB_PWR_OFF) > - return 0; > - > - /* > * According to the user manual, calpad calibration > * cycle takes more than 2us without the minimal recommended > * value, so we may need a little margin here > @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) > HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE, > PHYCTRL_ENDLL_MASK, > PHYCTRL_ENDLL_SHIFT)); > + > + /* > + * We turned on the DLL even though the rate was 0 because we the > + * clock might be turned on later. ...but we can't wait for the DLL > + * to lock when the rate is 0 because it will never lock with no > + * input clock. > + * > + * Technically we should be checking the lock later when the clock > + * is turned on, but for now we won't. > + */ > + if (rate == 0) > + return 0; Why not return initially from rockchip_emmc_phy_power if the clock rate is '0'. Are there other functions to lock the DLL apart from phy_power? Thanks Kishon ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-06-29 13:49 ` Kishon Vijay Abraham I @ 2016-06-29 15:18 ` Doug Anderson 2016-07-23 9:39 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Doug Anderson @ 2016-06-29 15:18 UTC (permalink / raw) To: linux-arm-kernel Kishon, On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote: > Hi, > > On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote: >> It's possible that there are some reasons to turn the PHY on while the >> clock is 0. In this case we just won't wait for the DLL to lock. >> >> This is a bit of a stopgap until we figure out exactly when we're >> supposed to wait for the DLL to lock and when we're supposed to power >> cycle the PHY. >> >> Note: this patch should help with suspend/resume where the system will >> try to turn the PHY back on when the clock is 0. >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- >> drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++--------------- >> 1 file changed, 37 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c >> index 9dce958233a0..a2aa6aca7dec 100644 >> --- a/drivers/phy/phy-rockchip-emmc.c >> +++ b/drivers/phy/phy-rockchip-emmc.c >> @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >> unsigned int caldone; >> unsigned int dllrdy; >> unsigned int freqsel = PHYCTRL_FREQSEL_200M; >> + unsigned long rate; >> unsigned long timeout; >> >> - if (rk_phy->emmcclk != NULL) { >> - unsigned long rate = clk_get_rate(rk_phy->emmcclk); >> + /* >> + * Keep phyctrl_pdb and phyctrl_endll low to allow >> + * initialization of CALIO state M/C DFFs >> + */ >> + regmap_write(rk_phy->reg_base, >> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >> + HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >> + PHYCTRL_PDB_MASK, >> + PHYCTRL_PDB_SHIFT)); >> + regmap_write(rk_phy->reg_base, >> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >> + HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >> + PHYCTRL_ENDLL_MASK, >> + PHYCTRL_ENDLL_SHIFT)); >> + >> + /* Already finish power_off above */ >> + if (on_off == PHYCTRL_PDB_PWR_OFF) >> + return 0; >> + >> + rate = clk_get_rate(rk_phy->emmcclk); >> + >> + if (rate != 0) { >> unsigned long ideal_rate; >> unsigned long diff; >> >> switch (rate) { >> - case 0 ... 74999999: >> + case 1 ... 74999999: >> ideal_rate = 50000000; >> freqsel = PHYCTRL_FREQSEL_50M; >> break; >> @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >> } >> >> /* >> - * Keep phyctrl_pdb and phyctrl_endll low to allow >> - * initialization of CALIO state M/C DFFs >> - */ >> - regmap_write(rk_phy->reg_base, >> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >> - HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >> - PHYCTRL_PDB_MASK, >> - PHYCTRL_PDB_SHIFT)); >> - regmap_write(rk_phy->reg_base, >> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >> - HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >> - PHYCTRL_ENDLL_MASK, >> - PHYCTRL_ENDLL_SHIFT)); >> - >> - /* Already finish power_off above */ >> - if (on_off == PHYCTRL_PDB_PWR_OFF) >> - return 0; >> - >> - /* >> * According to the user manual, calpad calibration >> * cycle takes more than 2us without the minimal recommended >> * value, so we may need a little margin here >> @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >> HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE, >> PHYCTRL_ENDLL_MASK, >> PHYCTRL_ENDLL_SHIFT)); >> + >> + /* >> + * We turned on the DLL even though the rate was 0 because we the >> + * clock might be turned on later. ...but we can't wait for the DLL >> + * to lock when the rate is 0 because it will never lock with no >> + * input clock. >> + * >> + * Technically we should be checking the lock later when the clock >> + * is turned on, but for now we won't. >> + */ >> + if (rate == 0) >> + return 0; > > Why not return initially from rockchip_emmc_phy_power if the clock rate is '0'. > Are there other functions to lock the DLL apart from phy_power? Yeah, it's a big ugly right now. This ugliness is really needed because of <https://patchwork.kernel.org/patch/9201035/> because: 1. We power on the PHY at probe time and the card clock is in an unknown state at that time. It will be reported as 0 right now, but it may or may not actually be 0. 2. We don't have an easy way to call back into the PHY when we actually set the clock to a low rate (like 400kHz) for ID mode. Before this series I tried to power the PHY off and on for every clock change, but apparently that was causing problems. As talked about in <https://patchwork.kernel.org/patch/9201035/>, I think the right answer is to figure out how to get the common clock framework notifications to happen for the card clock and then remove the wholesale PHY power off / power on for every clock change. The PHY itself can register for the clock change notifications and figure out how much or how little to do on every clock change. Unfortunately, as also discussed in the other patch, it's not trivial to do this because I think it requires surgery on the main SDHCI driver to change the way it deals with the card clock. I'm not sure I have time for this delicate surgery right now and I'm hoping that perhaps Shawn will be able to help figure something out (maybe?) or I can try coming back to it later. In any case, I think a wholesale revert of my previous 150 MHz series probably puts us in a worse state than we started with, so I was just proposing reverting the one patch. Once we do that, this PHY patch helps keep us in a sane state (keeps suspend/resume working). -Doug ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-06-29 15:18 ` Doug Anderson @ 2016-07-23 9:39 ` Ulf Hansson 2016-07-25 5:57 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2016-07-23 9:39 UTC (permalink / raw) To: linux-arm-kernel On 29 June 2016 at 17:18, Doug Anderson <dianders@chromium.org> wrote: > Kishon, > > On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> Hi, >> >> On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote: >>> It's possible that there are some reasons to turn the PHY on while the >>> clock is 0. In this case we just won't wait for the DLL to lock. >>> >>> This is a bit of a stopgap until we figure out exactly when we're >>> supposed to wait for the DLL to lock and when we're supposed to power >>> cycle the PHY. >>> >>> Note: this patch should help with suspend/resume where the system will >>> try to turn the PHY back on when the clock is 0. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++--------------- >>> 1 file changed, 37 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c >>> index 9dce958233a0..a2aa6aca7dec 100644 >>> --- a/drivers/phy/phy-rockchip-emmc.c >>> +++ b/drivers/phy/phy-rockchip-emmc.c >>> @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>> unsigned int caldone; >>> unsigned int dllrdy; >>> unsigned int freqsel = PHYCTRL_FREQSEL_200M; >>> + unsigned long rate; >>> unsigned long timeout; >>> >>> - if (rk_phy->emmcclk != NULL) { >>> - unsigned long rate = clk_get_rate(rk_phy->emmcclk); >>> + /* >>> + * Keep phyctrl_pdb and phyctrl_endll low to allow >>> + * initialization of CALIO state M/C DFFs >>> + */ >>> + regmap_write(rk_phy->reg_base, >>> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>> + HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >>> + PHYCTRL_PDB_MASK, >>> + PHYCTRL_PDB_SHIFT)); >>> + regmap_write(rk_phy->reg_base, >>> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>> + HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >>> + PHYCTRL_ENDLL_MASK, >>> + PHYCTRL_ENDLL_SHIFT)); >>> + >>> + /* Already finish power_off above */ >>> + if (on_off == PHYCTRL_PDB_PWR_OFF) >>> + return 0; >>> + >>> + rate = clk_get_rate(rk_phy->emmcclk); >>> + >>> + if (rate != 0) { >>> unsigned long ideal_rate; >>> unsigned long diff; >>> >>> switch (rate) { >>> - case 0 ... 74999999: >>> + case 1 ... 74999999: >>> ideal_rate = 50000000; >>> freqsel = PHYCTRL_FREQSEL_50M; >>> break; >>> @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>> } >>> >>> /* >>> - * Keep phyctrl_pdb and phyctrl_endll low to allow >>> - * initialization of CALIO state M/C DFFs >>> - */ >>> - regmap_write(rk_phy->reg_base, >>> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>> - HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >>> - PHYCTRL_PDB_MASK, >>> - PHYCTRL_PDB_SHIFT)); >>> - regmap_write(rk_phy->reg_base, >>> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>> - HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >>> - PHYCTRL_ENDLL_MASK, >>> - PHYCTRL_ENDLL_SHIFT)); >>> - >>> - /* Already finish power_off above */ >>> - if (on_off == PHYCTRL_PDB_PWR_OFF) >>> - return 0; >>> - >>> - /* >>> * According to the user manual, calpad calibration >>> * cycle takes more than 2us without the minimal recommended >>> * value, so we may need a little margin here >>> @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>> HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE, >>> PHYCTRL_ENDLL_MASK, >>> PHYCTRL_ENDLL_SHIFT)); >>> + >>> + /* >>> + * We turned on the DLL even though the rate was 0 because we the >>> + * clock might be turned on later. ...but we can't wait for the DLL >>> + * to lock when the rate is 0 because it will never lock with no >>> + * input clock. >>> + * >>> + * Technically we should be checking the lock later when the clock >>> + * is turned on, but for now we won't. >>> + */ >>> + if (rate == 0) >>> + return 0; >> >> Why not return initially from rockchip_emmc_phy_power if the clock rate is '0'. >> Are there other functions to lock the DLL apart from phy_power? > > Yeah, it's a big ugly right now. This ugliness is really needed > because of <https://patchwork.kernel.org/patch/9201035/> because: > > 1. We power on the PHY at probe time and the card clock is in an > unknown state at that time. It will be reported as 0 right now, but > it may or may not actually be 0. > > 2. We don't have an easy way to call back into the PHY when we > actually set the clock to a low rate (like 400kHz) for ID mode. > Before this series I tried to power the PHY off and on for every clock > change, but apparently that was causing problems. > > > As talked about in <https://patchwork.kernel.org/patch/9201035/>, I > think the right answer is to figure out how to get the common clock > framework notifications to happen for the card clock and then remove > the wholesale PHY power off / power on for every clock change. The > PHY itself can register for the clock change notifications and figure > out how much or how little to do on every clock change. > > Unfortunately, as also discussed in the other patch, it's not trivial > to do this because I think it requires surgery on the main SDHCI > driver to change the way it deals with the card clock. I'm not sure I > have time for this delicate surgery right now and I'm hoping that > perhaps Shawn will be able to help figure something out (maybe?) or I > can try coming back to it later. > > > In any case, I think a wholesale revert of my previous 150 MHz series > probably puts us in a worse state than we started with, so I was just > proposing reverting the one patch. Once we do that, this PHY patch > helps keep us in a sane state (keeps suspend/resume working). > > > -Doug Doug, Kishon, Did you agree on how to move forward with this change? Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-07-23 9:39 ` Ulf Hansson @ 2016-07-25 5:57 ` Kishon Vijay Abraham I 2016-07-25 7:37 ` Ulf Hansson 2016-07-25 14:19 ` Doug Anderson 0 siblings, 2 replies; 13+ messages in thread From: Kishon Vijay Abraham I @ 2016-07-25 5:57 UTC (permalink / raw) To: linux-arm-kernel Ulf, On Saturday 23 July 2016 03:09 PM, Ulf Hansson wrote: > On 29 June 2016 at 17:18, Doug Anderson <dianders@chromium.org> wrote: >> Kishon, >> >> On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote: >>> Hi, >>> >>> On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote: >>>> It's possible that there are some reasons to turn the PHY on while the >>>> clock is 0. In this case we just won't wait for the DLL to lock. >>>> >>>> This is a bit of a stopgap until we figure out exactly when we're >>>> supposed to wait for the DLL to lock and when we're supposed to power >>>> cycle the PHY. >>>> >>>> Note: this patch should help with suspend/resume where the system will >>>> try to turn the PHY back on when the clock is 0. >>>> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>> --- >>>> drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++--------------- >>>> 1 file changed, 37 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c >>>> index 9dce958233a0..a2aa6aca7dec 100644 >>>> --- a/drivers/phy/phy-rockchip-emmc.c >>>> +++ b/drivers/phy/phy-rockchip-emmc.c >>>> @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>>> unsigned int caldone; >>>> unsigned int dllrdy; >>>> unsigned int freqsel = PHYCTRL_FREQSEL_200M; >>>> + unsigned long rate; >>>> unsigned long timeout; >>>> >>>> - if (rk_phy->emmcclk != NULL) { >>>> - unsigned long rate = clk_get_rate(rk_phy->emmcclk); >>>> + /* >>>> + * Keep phyctrl_pdb and phyctrl_endll low to allow >>>> + * initialization of CALIO state M/C DFFs >>>> + */ >>>> + regmap_write(rk_phy->reg_base, >>>> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>> + HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >>>> + PHYCTRL_PDB_MASK, >>>> + PHYCTRL_PDB_SHIFT)); >>>> + regmap_write(rk_phy->reg_base, >>>> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>> + HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >>>> + PHYCTRL_ENDLL_MASK, >>>> + PHYCTRL_ENDLL_SHIFT)); >>>> + >>>> + /* Already finish power_off above */ >>>> + if (on_off == PHYCTRL_PDB_PWR_OFF) >>>> + return 0; >>>> + >>>> + rate = clk_get_rate(rk_phy->emmcclk); >>>> + >>>> + if (rate != 0) { >>>> unsigned long ideal_rate; >>>> unsigned long diff; >>>> >>>> switch (rate) { >>>> - case 0 ... 74999999: >>>> + case 1 ... 74999999: >>>> ideal_rate = 50000000; >>>> freqsel = PHYCTRL_FREQSEL_50M; >>>> break; >>>> @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>>> } >>>> >>>> /* >>>> - * Keep phyctrl_pdb and phyctrl_endll low to allow >>>> - * initialization of CALIO state M/C DFFs >>>> - */ >>>> - regmap_write(rk_phy->reg_base, >>>> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>> - HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >>>> - PHYCTRL_PDB_MASK, >>>> - PHYCTRL_PDB_SHIFT)); >>>> - regmap_write(rk_phy->reg_base, >>>> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>> - HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >>>> - PHYCTRL_ENDLL_MASK, >>>> - PHYCTRL_ENDLL_SHIFT)); >>>> - >>>> - /* Already finish power_off above */ >>>> - if (on_off == PHYCTRL_PDB_PWR_OFF) >>>> - return 0; >>>> - >>>> - /* >>>> * According to the user manual, calpad calibration >>>> * cycle takes more than 2us without the minimal recommended >>>> * value, so we may need a little margin here >>>> @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>>> HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE, >>>> PHYCTRL_ENDLL_MASK, >>>> PHYCTRL_ENDLL_SHIFT)); >>>> + >>>> + /* >>>> + * We turned on the DLL even though the rate was 0 because we the >>>> + * clock might be turned on later. ...but we can't wait for the DLL >>>> + * to lock when the rate is 0 because it will never lock with no >>>> + * input clock. >>>> + * >>>> + * Technically we should be checking the lock later when the clock >>>> + * is turned on, but for now we won't. >>>> + */ >>>> + if (rate == 0) >>>> + return 0; >>> >>> Why not return initially from rockchip_emmc_phy_power if the clock rate is '0'. >>> Are there other functions to lock the DLL apart from phy_power? >> >> Yeah, it's a big ugly right now. This ugliness is really needed >> because of <https://patchwork.kernel.org/patch/9201035/> because: >> >> 1. We power on the PHY at probe time and the card clock is in an >> unknown state at that time. It will be reported as 0 right now, but >> it may or may not actually be 0. >> >> 2. We don't have an easy way to call back into the PHY when we >> actually set the clock to a low rate (like 400kHz) for ID mode. >> Before this series I tried to power the PHY off and on for every clock >> change, but apparently that was causing problems. >> >> >> As talked about in <https://patchwork.kernel.org/patch/9201035/>, I >> think the right answer is to figure out how to get the common clock >> framework notifications to happen for the card clock and then remove >> the wholesale PHY power off / power on for every clock change. The >> PHY itself can register for the clock change notifications and figure >> out how much or how little to do on every clock change. >> >> Unfortunately, as also discussed in the other patch, it's not trivial >> to do this because I think it requires surgery on the main SDHCI >> driver to change the way it deals with the card clock. I'm not sure I >> have time for this delicate surgery right now and I'm hoping that >> perhaps Shawn will be able to help figure something out (maybe?) or I >> can try coming back to it later. >> >> >> In any case, I think a wholesale revert of my previous 150 MHz series >> probably puts us in a worse state than we started with, so I was just >> proposing reverting the one patch. Once we do that, this PHY patch >> helps keep us in a sane state (keeps suspend/resume working). >> >> >> -Doug > > Doug, Kishon, > > Did you agree on how to move forward with this change? I think still few things are not very clear especially on what should be handled in PHY when the clock rate changes or if any new PHY APIs are required to handle any clock changes etc.. But since this actually gets MMC working in rockchip, I'd be okay to merge this now. Though I'd expect this to be refined in the future release cycles. FWIW: Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com> Thanks Kishon ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-07-25 5:57 ` Kishon Vijay Abraham I @ 2016-07-25 7:37 ` Ulf Hansson 2016-07-25 14:19 ` Doug Anderson 1 sibling, 0 replies; 13+ messages in thread From: Ulf Hansson @ 2016-07-25 7:37 UTC (permalink / raw) To: linux-arm-kernel On 25 July 2016 at 07:57, Kishon Vijay Abraham I <kishon@ti.com> wrote: > Ulf, > > On Saturday 23 July 2016 03:09 PM, Ulf Hansson wrote: >> On 29 June 2016 at 17:18, Doug Anderson <dianders@chromium.org> wrote: >>> Kishon, >>> >>> On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote: >>>> Hi, >>>> >>>> On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote: >>>>> It's possible that there are some reasons to turn the PHY on while the >>>>> clock is 0. In this case we just won't wait for the DLL to lock. >>>>> >>>>> This is a bit of a stopgap until we figure out exactly when we're >>>>> supposed to wait for the DLL to lock and when we're supposed to power >>>>> cycle the PHY. >>>>> >>>>> Note: this patch should help with suspend/resume where the system will >>>>> try to turn the PHY back on when the clock is 0. >>>>> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>>> --- >>>>> drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++--------------- >>>>> 1 file changed, 37 insertions(+), 22 deletions(-) >>>>> >>>>> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c >>>>> index 9dce958233a0..a2aa6aca7dec 100644 >>>>> --- a/drivers/phy/phy-rockchip-emmc.c >>>>> +++ b/drivers/phy/phy-rockchip-emmc.c >>>>> @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>>>> unsigned int caldone; >>>>> unsigned int dllrdy; >>>>> unsigned int freqsel = PHYCTRL_FREQSEL_200M; >>>>> + unsigned long rate; >>>>> unsigned long timeout; >>>>> >>>>> - if (rk_phy->emmcclk != NULL) { >>>>> - unsigned long rate = clk_get_rate(rk_phy->emmcclk); >>>>> + /* >>>>> + * Keep phyctrl_pdb and phyctrl_endll low to allow >>>>> + * initialization of CALIO state M/C DFFs >>>>> + */ >>>>> + regmap_write(rk_phy->reg_base, >>>>> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>>> + HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >>>>> + PHYCTRL_PDB_MASK, >>>>> + PHYCTRL_PDB_SHIFT)); >>>>> + regmap_write(rk_phy->reg_base, >>>>> + rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>>> + HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >>>>> + PHYCTRL_ENDLL_MASK, >>>>> + PHYCTRL_ENDLL_SHIFT)); >>>>> + >>>>> + /* Already finish power_off above */ >>>>> + if (on_off == PHYCTRL_PDB_PWR_OFF) >>>>> + return 0; >>>>> + >>>>> + rate = clk_get_rate(rk_phy->emmcclk); >>>>> + >>>>> + if (rate != 0) { >>>>> unsigned long ideal_rate; >>>>> unsigned long diff; >>>>> >>>>> switch (rate) { >>>>> - case 0 ... 74999999: >>>>> + case 1 ... 74999999: >>>>> ideal_rate = 50000000; >>>>> freqsel = PHYCTRL_FREQSEL_50M; >>>>> break; >>>>> @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>>>> } >>>>> >>>>> /* >>>>> - * Keep phyctrl_pdb and phyctrl_endll low to allow >>>>> - * initialization of CALIO state M/C DFFs >>>>> - */ >>>>> - regmap_write(rk_phy->reg_base, >>>>> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>>> - HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF, >>>>> - PHYCTRL_PDB_MASK, >>>>> - PHYCTRL_PDB_SHIFT)); >>>>> - regmap_write(rk_phy->reg_base, >>>>> - rk_phy->reg_offset + GRF_EMMCPHY_CON6, >>>>> - HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE, >>>>> - PHYCTRL_ENDLL_MASK, >>>>> - PHYCTRL_ENDLL_SHIFT)); >>>>> - >>>>> - /* Already finish power_off above */ >>>>> - if (on_off == PHYCTRL_PDB_PWR_OFF) >>>>> - return 0; >>>>> - >>>>> - /* >>>>> * According to the user manual, calpad calibration >>>>> * cycle takes more than 2us without the minimal recommended >>>>> * value, so we may need a little margin here >>>>> @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) >>>>> HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE, >>>>> PHYCTRL_ENDLL_MASK, >>>>> PHYCTRL_ENDLL_SHIFT)); >>>>> + >>>>> + /* >>>>> + * We turned on the DLL even though the rate was 0 because we the >>>>> + * clock might be turned on later. ...but we can't wait for the DLL >>>>> + * to lock when the rate is 0 because it will never lock with no >>>>> + * input clock. >>>>> + * >>>>> + * Technically we should be checking the lock later when the clock >>>>> + * is turned on, but for now we won't. >>>>> + */ >>>>> + if (rate == 0) >>>>> + return 0; >>>> >>>> Why not return initially from rockchip_emmc_phy_power if the clock rate is '0'. >>>> Are there other functions to lock the DLL apart from phy_power? >>> >>> Yeah, it's a big ugly right now. This ugliness is really needed >>> because of <https://patchwork.kernel.org/patch/9201035/> because: >>> >>> 1. We power on the PHY at probe time and the card clock is in an >>> unknown state at that time. It will be reported as 0 right now, but >>> it may or may not actually be 0. >>> >>> 2. We don't have an easy way to call back into the PHY when we >>> actually set the clock to a low rate (like 400kHz) for ID mode. >>> Before this series I tried to power the PHY off and on for every clock >>> change, but apparently that was causing problems. >>> >>> >>> As talked about in <https://patchwork.kernel.org/patch/9201035/>, I >>> think the right answer is to figure out how to get the common clock >>> framework notifications to happen for the card clock and then remove >>> the wholesale PHY power off / power on for every clock change. The >>> PHY itself can register for the clock change notifications and figure >>> out how much or how little to do on every clock change. >>> >>> Unfortunately, as also discussed in the other patch, it's not trivial >>> to do this because I think it requires surgery on the main SDHCI >>> driver to change the way it deals with the card clock. I'm not sure I >>> have time for this delicate surgery right now and I'm hoping that >>> perhaps Shawn will be able to help figure something out (maybe?) or I >>> can try coming back to it later. >>> >>> >>> In any case, I think a wholesale revert of my previous 150 MHz series >>> probably puts us in a worse state than we started with, so I was just >>> proposing reverting the one patch. Once we do that, this PHY patch >>> helps keep us in a sane state (keeps suspend/resume working). >>> >>> >>> -Doug >> >> Doug, Kishon, >> >> Did you agree on how to move forward with this change? > > I think still few things are not very clear especially on what should be > handled in PHY when the clock rate changes or if any new PHY APIs are required > to handle any clock changes etc.. > But since this actually gets MMC working in rockchip, I'd be okay to merge this > now. Though I'd expect this to be refined in the future release cycles. Okay. > > FWIW: > Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com> I assume it's okay that I queue this via my mmc tree then. If not, please tell! Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on 2016-07-25 5:57 ` Kishon Vijay Abraham I 2016-07-25 7:37 ` Ulf Hansson @ 2016-07-25 14:19 ` Doug Anderson 1 sibling, 0 replies; 13+ messages in thread From: Doug Anderson @ 2016-07-25 14:19 UTC (permalink / raw) To: linux-arm-kernel Hi, On Sun, Jul 24, 2016 at 10:57 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> Did you agree on how to move forward with this change? > > I think still few things are not very clear especially on what should be > handled in PHY when the clock rate changes or if any new PHY APIs are required > to handle any clock changes etc.. > But since this actually gets MMC working in rockchip, I'd be okay to merge this > now. Though I'd expect this to be refined in the future release cycles. > > FWIW: > Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com> Thank you. Agreed that I don't particularly like the revert and last I checked with Shawn at Rockchip he was trying to find a better solution. ...but I also agree that landing the revert is better than not landing it while we wait for a better solution. IMHO in this particular case PHY API changes shouldn't be needed since we should be able to use the CCF notifications. ...but we need to figure out how to do that in the case of the current card clock since it's not really a fully fledged clock and doesn't support notifications. We might need to get advice from CCF guys... -Doug ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] phy: rockchip-emmc: Wait even longer for the DLL to lock 2016-06-27 17:39 [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Douglas Anderson 2016-06-27 17:39 ` [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes Douglas Anderson 2016-06-27 17:39 ` [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on Douglas Anderson @ 2016-06-27 17:39 ` Douglas Anderson 2016-06-29 13:50 ` Kishon Vijay Abraham I 2016-07-25 8:49 ` [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Ulf Hansson 3 siblings, 1 reply; 13+ messages in thread From: Douglas Anderson @ 2016-06-27 17:39 UTC (permalink / raw) To: linux-arm-kernel Two times out of 2000 reboots I ran into the error message "rockchip_emmc_phy_power: dllrdy timeout". Presumably there is some corner case where the DLL just takes a little longer to timeout. Let's give it even more time to handle these corner cases. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/phy/phy-rockchip-emmc.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c index a2aa6aca7dec..fd57345ffed2 100644 --- a/drivers/phy/phy-rockchip-emmc.c +++ b/drivers/phy/phy-rockchip-emmc.c @@ -206,8 +206,18 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) * per the math: 10.2 us * (50000000 Hz / 100000 Hz) => 5.1 ms * Hopefully we won't be running at 100 kHz, but we should still make * sure we wait long enough. + * + * NOTE: There appear to be corner cases where the DLL seems to take + * extra long to lock for reasons that aren't understood. In some + * extreme cases we've seen it take up to over 10ms (!). We'll be + * generous and give it 50ms. We still busy wait here because: + * - In most cases it should be super fast. + * - This is not called lots during normal operation so it shouldn't + * be a power or performance problem to busy wait. We expect it + * only at boot / resume. In both cases, eMMC is probably on the + * critical path so busy waiting a little extra time should be OK. */ - timeout = jiffies + msecs_to_jiffies(10); + timeout = jiffies + msecs_to_jiffies(50); do { udelay(1); -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] phy: rockchip-emmc: Wait even longer for the DLL to lock 2016-06-27 17:39 ` [PATCH 3/3] phy: rockchip-emmc: Wait even longer for the DLL to lock Douglas Anderson @ 2016-06-29 13:50 ` Kishon Vijay Abraham I 0 siblings, 0 replies; 13+ messages in thread From: Kishon Vijay Abraham I @ 2016-06-29 13:50 UTC (permalink / raw) To: linux-arm-kernel On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote: > Two times out of 2000 reboots I ran into the error message > "rockchip_emmc_phy_power: dllrdy timeout". Presumably there is some > corner case where the DLL just takes a little longer to timeout. Let's > give it even more time to handle these corner cases. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/phy/phy-rockchip-emmc.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c > index a2aa6aca7dec..fd57345ffed2 100644 > --- a/drivers/phy/phy-rockchip-emmc.c > +++ b/drivers/phy/phy-rockchip-emmc.c > @@ -206,8 +206,18 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) > * per the math: 10.2 us * (50000000 Hz / 100000 Hz) => 5.1 ms > * Hopefully we won't be running at 100 kHz, but we should still make > * sure we wait long enough. > + * > + * NOTE: There appear to be corner cases where the DLL seems to take > + * extra long to lock for reasons that aren't understood. In some > + * extreme cases we've seen it take up to over 10ms (!). We'll be > + * generous and give it 50ms. We still busy wait here because: > + * - In most cases it should be super fast. > + * - This is not called lots during normal operation so it shouldn't > + * be a power or performance problem to busy wait. We expect it > + * only at boot / resume. In both cases, eMMC is probably on the > + * critical path so busy waiting a little extra time should be OK. > */ > - timeout = jiffies + msecs_to_jiffies(10); > + timeout = jiffies + msecs_to_jiffies(50); > do { > udelay(1); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series 2016-06-27 17:39 [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Douglas Anderson ` (2 preceding siblings ...) 2016-06-27 17:39 ` [PATCH 3/3] phy: rockchip-emmc: Wait even longer for the DLL to lock Douglas Anderson @ 2016-07-25 8:49 ` Ulf Hansson 3 siblings, 0 replies; 13+ messages in thread From: Ulf Hansson @ 2016-07-25 8:49 UTC (permalink / raw) To: linux-arm-kernel On 27 June 2016 at 19:39, Douglas Anderson <dianders@chromium.org> wrote: > Despite positive testing across a number of machines and a number of > people, last week I got a report that my 150 MHz series was breaking > things on a handful of boards. > > It turns out that reverting the patch to always power cycle across clock > changes fixes things and reverting that patch also doesn't actually > cause any known problems (and 150 MHz should even continue to work). > > Although we really need to get to the bottom of things, it seems > expedient to revert while we're waiting for a better solution, hence > this series. Also in this series is further increase in the time we'll > wait for the DLL to stabilize (found during reboot stress testing) and a > fix to the way we handle the card clock being reported as 0 (needed for > suspend/resume because of the revert we just did). > > It's expected that this series could go through Ulf's mmc tree (like the > previous) with Kishon's Ack as appropriate. > > > Douglas Anderson (3): > mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock > changes > phy: rockchip-emmc: Be tolerant to card clock of 0 in power on > phy: rockchip-emmc: Wait even longer for the DLL to lock > > drivers/mmc/host/sdhci-of-arasan.c | 21 +++++++---- > drivers/phy/phy-rockchip-emmc.c | 71 ++++++++++++++++++++++++++------------ > 2 files changed, 62 insertions(+), 30 deletions(-) > > -- > 2.8.0.rc3.226.g39d4020 > Thanks, applied for next! Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-07-25 14:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-27 17:39 [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Douglas Anderson 2016-06-27 17:39 ` [PATCH 1/3] mmc: sdhci-of-arasan: Revert: Always power the PHY off/on when clock changes Douglas Anderson 2016-07-21 10:09 ` Adrian Hunter 2016-06-27 17:39 ` [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on Douglas Anderson 2016-06-29 13:49 ` Kishon Vijay Abraham I 2016-06-29 15:18 ` Doug Anderson 2016-07-23 9:39 ` Ulf Hansson 2016-07-25 5:57 ` Kishon Vijay Abraham I 2016-07-25 7:37 ` Ulf Hansson 2016-07-25 14:19 ` Doug Anderson 2016-06-27 17:39 ` [PATCH 3/3] phy: rockchip-emmc: Wait even longer for the DLL to lock Douglas Anderson 2016-06-29 13:50 ` Kishon Vijay Abraham I 2016-07-25 8:49 ` [PATCH 0/3] mmc: Fixes for 150 MHz Rockchip eMMC series Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).