From: Douglas Anderson <dianders@chromium.org> Date: 2016-06-27 17:39:56
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
From: Douglas Anderson <dianders@chromium.org> Date: 2016-06-27 17:39:59
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(-)
From: Douglas Anderson <dianders@chromium.org> Date: 2016-06-27 17:40:23
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(-)
From: Douglas Anderson <dianders@chromium.org> Date: 2016-06-27 17:40:25
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(-)
From: Kishon Vijay Abraham I <hidden> Date: 2016-06-29 13:50:38
Hi,
On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote:
quoted hunk
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(-)
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
From: Kishon Vijay Abraham I <hidden> Date: 2016-06-29 13:51:37
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>
From: Doug Anderson <dianders@chromium.org> Date: 2016-06-29 15:19:21
Kishon,
On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I [off-list ref] wrote:
Hi,
On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote:
quoted
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(-)
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
From: Adrian Hunter <adrian.hunter@intel.com> Date: 2016-07-21 10:13:42
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>
On 29 June 2016 at 17:18, Doug Anderson [off-list ref] wrote:
Kishon,
On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I [off-list ref] wrote:
quoted
Hi,
On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote:
quoted
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(-)
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
From: Kishon Vijay Abraham I <hidden> Date: 2016-07-25 05:58:14
Ulf,
On Saturday 23 July 2016 03:09 PM, Ulf Hansson wrote:
On 29 June 2016 at 17:18, Doug Anderson [off-list ref] wrote:
quoted
Kishon,
On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I [off-list ref] wrote:
quoted
Hi,
On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote:
quoted
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(-)
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 <redacted>
Thanks
Kishon
On 25 July 2016 at 07:57, Kishon Vijay Abraham I [off-list ref] wrote:
Ulf,
On Saturday 23 July 2016 03:09 PM, Ulf Hansson wrote:
quoted
On 29 June 2016 at 17:18, Doug Anderson [off-list ref] wrote:
quoted
Kishon,
On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I [off-list ref] wrote:
quoted
Hi,
On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote:
quoted
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(-)
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 <redacted>
I assume it's okay that I queue this via my mmc tree then. If not, please tell!
Kind regards
Uffe
On 27 June 2016 at 19:39, Douglas Anderson [off-list ref] 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
From: Doug Anderson <dianders@chromium.org> Date: 2016-07-25 14:19:20
Hi,
On Sun, Jul 24, 2016 at 10:57 PM, Kishon Vijay Abraham I [off-list ref] wrote:
quoted
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 <redacted>
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