The Exynos host controller driver is clearly a PHY consumer (gets the ufs->phy using devm_phy_get()), but pokes into the guts of struct phy to get the generic_phy->power_count.
The UFS core (specifically ufshcd_link_startup()) may call the variant operation exynos_ufs_pre_link() -> exynos_ufs_phy_init() multiple times if the link startup fails and needs to be retried. However ufs-exynos shouldn't be doing what it's doing, i.e. looking at the generic_phy->power_count, because in the general sense of the API, a single Generic PHY may have multiple consumers. If ufs-exynos looks at generic_phy->power_count, there's no guarantee that *ufs-exynos* is the one who previously bumped that power count. So it may be powering down the PHY on behalf of another consumer. The correct way in which this should be handled is ufs-exynos should *remember* whether it has initialized and powered up the PHY before, and power it down during link retries. Not rely on the power_count (which, btw, on the writer side is modified under &phy->mutex, but on the reader side is accessed unlocked). This is a discouraged pattern even if here it doesn't cause functional problems. Signed-off-by: Vladimir Oltean <[email protected]> --- Cc: Alim Akhtar <[email protected]> Cc: Peter Griffin <[email protected]> Cc: "James E.J. Bottomley" <[email protected]> Cc: "Martin K. Petersen" <[email protected]> Cc: Krzysztof Kozlowski <[email protected]> --- drivers/ufs/host/ufs-exynos.c | 17 ++++++++++++++--- drivers/ufs/host/ufs-exynos.h | 1 + 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c index 76fee3a79c77..eff2abbf9a9b 100644 --- a/drivers/ufs/host/ufs-exynos.c +++ b/drivers/ufs/host/ufs-exynos.c @@ -963,9 +963,10 @@ static int exynos_ufs_phy_init(struct exynos_ufs *ufs) phy_set_bus_width(generic_phy, ufs->avail_ln_rx); - if (generic_phy->power_count) { + if (ufs->phy_powered_on) { phy_power_off(generic_phy); phy_exit(generic_phy); + ufs->phy_powered_on = false; } ret = phy_init(generic_phy); @@ -979,6 +980,8 @@ static int exynos_ufs_phy_init(struct exynos_ufs *ufs) if (ret) goto out_exit_phy; + ufs->phy_powered_on = true; + return 0; out_exit_phy: @@ -1737,9 +1740,17 @@ static int exynos_ufs_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, static int exynos_ufs_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) { struct exynos_ufs *ufs = ufshcd_get_variant(hba); + int err; - if (!ufshcd_is_link_active(hba)) - phy_power_on(ufs->phy); + if (!ufshcd_is_link_active(hba)) { + err = phy_power_on(ufs->phy); + if (err) { + dev_err(hba->dev, "Failed to power on PHY: %pe\n", + ERR_PTR(err)); + } else { + ufs->phy_powered_on = true; + } + } exynos_ufs_config_smu(ufs); exynos_ufs_fmp_resume(hba); diff --git a/drivers/ufs/host/ufs-exynos.h b/drivers/ufs/host/ufs-exynos.h index abe7e472759e..683b9150e2ba 100644 --- a/drivers/ufs/host/ufs-exynos.h +++ b/drivers/ufs/host/ufs-exynos.h @@ -227,6 +227,7 @@ struct exynos_ufs { int avail_ln_rx; int avail_ln_tx; int rx_sel_idx; + bool phy_powered_on; struct ufs_pa_layer_attr dev_req_params; struct ufs_phy_time_cfg t_cfg; ktime_t entry_hibern8_t; -- 2.43.0
