| Message ID | 1441961698-10516-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11 September 2015 at 10:54, Shawn Lin <shawn.lin@rock-chips.com> wrote: > This patch adds Generic PHY access for sdhci-of-arasan. Driver > can get PHY handler from dt-binding, and power-on/init the PHY. > Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/host/sdhci-of-arasan.c | 90 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 621c3f4..fdd71c7 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -21,6 +21,7 @@ > > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/phy/phy.h> > #include "sdhci-pltfm.h" > > #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c > @@ -35,6 +36,7 @@ > */ > struct sdhci_arasan_data { > struct clk *clk_ahb; > + struct phy *phy; > }; > > static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) > @@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = { > > #ifdef CONFIG_PM_SLEEP > /** > + * sdhci_arasan_suspend_phy - Suspend phy method for the driver > + * @phy: Handler of phy structure > + * Returns 0 on success and error value on error > + * > + * Put the phy in a deactive state. > + */ > +static int sdhci_arasan_suspend_phy(struct phy *phy) > +{ > + int ret; > + > + ret = phy_exit(phy); > + if (ret < 0) > + goto err_phy_exit; This odd to me. First you do phy_exit() then phy_power_off(). It seems like it should be in the opposite order. Moreover I wonder why phy_exit() is needed, I expected that to be called at ->remove() only!? > + > + ret = phy_power_off(phy); > + if (ret < 0) > + goto err_phy_pwr_off; > + > + return 0; > + > +err_phy_pwr_off: > + phy_power_on(phy); > +err_phy_exit: > + phy_init(phy); > + return ret; > +} > + > +/** > + * sdhci_arasan_resume_phy - Resume phy method for the driver > + * @phy: Handler of phy structure > + * Returns 0 on success and error value on error > + * > + * Put the phy in a active state. > + */ > +static int sdhci_arasan_resume_phy(struct phy *phy) > +{ > + int ret; > + > + ret = phy_power_on(phy); > + if (ret < 0) > + goto err_phy_pwr_on; > + > + ret = phy_init(phy); > + if (ret < 0) > + goto err_phy_init; > + Similar comment as above. > + return 0; > + > +err_phy_init: > + phy_exit(phy); > +err_phy_pwr_on: > + phy_power_off(phy); > + return ret; > +} > + > +/** > * sdhci_arasan_suspend - Suspend method for the driver > * @dev: Address of the device structure > * Returns 0 on success and error value on error > @@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev) > clk_disable(pltfm_host->clk); > clk_disable(sdhci_arasan->clk_ahb); > > + if (sdhci_arasan->phy) { > + ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy); > + if (ret < 0) > + return ret; > + } This means you will suspend the phy after you have disabled the clocks. Of course I can't tell whether that okay, but it doesn't follow the same sequence as in ->probe(). To me that indicates that either probe or suspend/resume could be broken. > + > return 0; > } > > @@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev) > return ret; > } > > + if (sdhci_arasan->phy) { > + ret = sdhci_arasan_resume_phy(sdhci_arasan->phy); > + if (ret < 0) > + return ret; > + } > + > return sdhci_resume_host(host); > } > #endif /* ! CONFIG_PM_SLEEP */ > @@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > goto clk_dis_ahb; > } > > + sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan"); > + if (!IS_ERR(sdhci_arasan->phy)) { I understand the phy is optional, but you still need to handle the EPROBE_DEFER case. Perhaps you should also use devm_phy_optional_get() instead!? > + ret = phy_power_on(sdhci_arasan->phy); This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()? Similar comment applies to phy_exit() and phy_power_off(). > + if (ret < 0) { > + dev_err(&pdev->dev, "phy_power_on err.\n"); > + phy_power_off(sdhci_arasan->phy); > + goto clk_dis_ahb; > + } > + > + ret = phy_init(sdhci_arasan->phy); > + if (ret < 0) { > + dev_err(&pdev->dev, "phy_init err.\n"); > + phy_exit(sdhci_arasan->phy); > + phy_power_off(sdhci_arasan->phy); > + goto clk_dis_ahb; > + } > + } else { This else isn't needed. When you are about to access the phy you can check the cookie of it by "!IS_ERR(sdhci_arasan->phy)". > + sdhci_arasan->phy = NULL; > + } > + > host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0); > if (IS_ERR(host)) { > ret = PTR_ERR(host); > -- > 2.3.7 > > Kind regards Uffe
On 2015/10/16 20:35, Ulf Hansson wrote: > On 11 September 2015 at 10:54, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> This patch adds Generic PHY access for sdhci-of-arasan. Driver >> can get PHY handler from dt-binding, and power-on/init the PHY. >> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/host/sdhci-of-arasan.c | 90 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 90 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c >> index 621c3f4..fdd71c7 100644 >> --- a/drivers/mmc/host/sdhci-of-arasan.c >> +++ b/drivers/mmc/host/sdhci-of-arasan.c >> @@ -21,6 +21,7 @@ >> >> #include <linux/module.h> >> #include <linux/of_device.h> >> +#include <linux/phy/phy.h> >> #include "sdhci-pltfm.h" >> >> #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c >> @@ -35,6 +36,7 @@ >> */ >> struct sdhci_arasan_data { >> struct clk *clk_ahb; >> + struct phy *phy; >> }; >> >> static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) >> @@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = { >> >> #ifdef CONFIG_PM_SLEEP >> /** >> + * sdhci_arasan_suspend_phy - Suspend phy method for the driver >> + * @phy: Handler of phy structure >> + * Returns 0 on success and error value on error >> + * >> + * Put the phy in a deactive state. >> + */ >> +static int sdhci_arasan_suspend_phy(struct phy *phy) >> +{ >> + int ret; >> + >> + ret = phy_exit(phy); >> + if (ret < 0) >> + goto err_phy_exit; > > This odd to me. First you do phy_exit() then phy_power_off(). It seems > like it should be in the opposite order. > yep, there is no need to use phy_exit/init for suspend/resume. I will do it. > Moreover I wonder why phy_exit() is needed, I expected that to be > called at ->remove() only!? > >> + >> + ret = phy_power_off(phy); >> + if (ret < 0) >> + goto err_phy_pwr_off; >> + >> + return 0; >> + >> +err_phy_pwr_off: >> + phy_power_on(phy); >> +err_phy_exit: >> + phy_init(phy); >> + return ret; >> +} >> + >> +/** >> + * sdhci_arasan_resume_phy - Resume phy method for the driver >> + * @phy: Handler of phy structure >> + * Returns 0 on success and error value on error >> + * >> + * Put the phy in a active state. >> + */ >> +static int sdhci_arasan_resume_phy(struct phy *phy) >> +{ >> + int ret; >> + >> + ret = phy_power_on(phy); >> + if (ret < 0) >> + goto err_phy_pwr_on; >> + >> + ret = phy_init(phy); >> + if (ret < 0) >> + goto err_phy_init; >> + > > Similar comment as above. > >> + return 0; >> + >> +err_phy_init: >> + phy_exit(phy); >> +err_phy_pwr_on: >> + phy_power_off(phy); >> + return ret; >> +} >> + >> +/** >> * sdhci_arasan_suspend - Suspend method for the driver >> * @dev: Address of the device structure >> * Returns 0 on success and error value on error >> @@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev) >> clk_disable(pltfm_host->clk); >> clk_disable(sdhci_arasan->clk_ahb); >> >> + if (sdhci_arasan->phy) { >> + ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy); >> + if (ret < 0) >> + return ret; >> + } > > This means you will suspend the phy after you have disabled the > clocks. Of course I can't tell whether that okay, but it doesn't > follow the same sequence as in ->probe(). To me that indicates that > either probe or suspend/resume could be broken. > phy has a seperate clk for itself, and it's controlled by phy driver. So, there is no any relationship between controller's clk and phy's clk. We disable or enable phy's clk internally in phy_int/exit/power_off/power_on. Of course if it makes odd to you, I would put suspend_phy before clk_disable. :) >> + >> return 0; >> } >> >> @@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev) >> return ret; >> } >> >> + if (sdhci_arasan->phy) { >> + ret = sdhci_arasan_resume_phy(sdhci_arasan->phy); >> + if (ret < 0) >> + return ret; >> + } >> + >> return sdhci_resume_host(host); >> } >> #endif /* ! CONFIG_PM_SLEEP */ >> @@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device *pdev) >> goto clk_dis_ahb; >> } >> >> + sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan"); >> + if (!IS_ERR(sdhci_arasan->phy)) { > > I understand the phy is optional, but you still need to handle the > EPROBE_DEFER case. > > Perhaps you should also use devm_phy_optional_get() instead!? I already changed it in version-2 [1]. :) phy is mandatory for sdhci-arasan,5.1. [1]: https://patchwork.kernel.org/patch/7173251/ > >> + ret = phy_power_on(sdhci_arasan->phy); > > This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()? > > Similar comment applies to phy_exit() and phy_power_off(). Both are okay. It depends how the phy-driver implement the four interfaces. From my case, power_on for arasan's phy driver firstly enable phy's clk and open power-domain, then programme phy internal registers to activate phy. Without enabling phy's clk and power-domain, we cannot do phy_init since phy can't be accessed by CPU. But here, I think you are right. It does look a bit weird. I think the better way is to remove phy_power_on here, and let phy-driver do power_on in phy_init internally. Similarly in the remove call. How about? > >> + if (ret < 0) { >> + dev_err(&pdev->dev, "phy_power_on err.\n"); >> + phy_power_off(sdhci_arasan->phy); >> + goto clk_dis_ahb; >> + } >> + >> + ret = phy_init(sdhci_arasan->phy); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "phy_init err.\n"); >> + phy_exit(sdhci_arasan->phy); >> + phy_power_off(sdhci_arasan->phy); >> + goto clk_dis_ahb; >> + } >> + } else { > > This else isn't needed. When you are about to access the phy you can > check the cookie of it by "!IS_ERR(sdhci_arasan->phy)". > similarly, I had removed it in v2. >> + sdhci_arasan->phy = NULL; >> + } >> + >> host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0); >> if (IS_ERR(host)) { >> ret = PTR_ERR(host); >> -- >> 2.3.7 >> >> > > Kind regards > Uffe > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
[...] >> I understand the phy is optional, but you still need to handle the >> EPROBE_DEFER case. >> >> Perhaps you should also use devm_phy_optional_get() instead!? > > > I already changed it in version-2 [1]. :) > phy is mandatory for sdhci-arasan,5.1. > > [1]: https://patchwork.kernel.org/patch/7173251/ Oh, apologize for reviewing the old version! > >> >>> + ret = phy_power_on(sdhci_arasan->phy); >> >> >> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()? >> >> Similar comment applies to phy_exit() and phy_power_off(). > > > Both are okay. It depends how the phy-driver implement the four interfaces. > From my case, power_on for arasan's phy driver firstly enable phy's clk and > open power-domain, then programme phy internal registers to activate phy. > Without enabling phy's clk and power-domain, we cannot do phy_init since phy > can't be accessed by CPU. > > But here, I think you are right. It does look a bit weird. > I think the better way is to remove phy_power_on here, and let phy-driver do > power_on in phy_init internally. Similarly in the remove call. That makes sense to me! I think it would also follow other phy clients use of the phy API. What makes me wonder though is from a power management point of view. *When* do you need to have phy initialized and powered? 1) For a removable card can leave the phy uninitialised or powered off, but still detect card insertion/removal via GPIO? Is that a valid scenario for you? 2) Considering the runtime PM case for the sdhci device. Typically you can gate clocks etc at runtime suspend to save power, but what about the phy? Can you power off it in runtime suspend? [...] Kind regards Uffe
On 2015/10/19 15:50, Ulf Hansson wrote: > [...] > >>> I understand the phy is optional, but you still need to handle the >>> EPROBE_DEFER case. >>> >>> Perhaps you should also use devm_phy_optional_get() instead!? >> >> >> I already changed it in version-2 [1]. :) >> phy is mandatory for sdhci-arasan,5.1. >> >> [1]: https://patchwork.kernel.org/patch/7173251/ > > Oh, apologize for reviewing the old version! > >> >>> >>>> + ret = phy_power_on(sdhci_arasan->phy); >>> >>> >>> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()? >>> >>> Similar comment applies to phy_exit() and phy_power_off(). >> >> >> Both are okay. It depends how the phy-driver implement the four interfaces. >> From my case, power_on for arasan's phy driver firstly enable phy's clk and >> open power-domain, then programme phy internal registers to activate phy. >> Without enabling phy's clk and power-domain, we cannot do phy_init since phy >> can't be accessed by CPU. >> >> But here, I think you are right. It does look a bit weird. >> I think the better way is to remove phy_power_on here, and let phy-driver do >> power_on in phy_init internally. Similarly in the remove call. > > That makes sense to me! I think it would also follow other phy clients > use of the phy API. > > What makes me wonder though is from a power management point of view. > *When* do you need to have phy initialized and powered? > Whenever controller need to communicate with card, must init/power_on phy firstly. > 1) For a removable card can leave the phy uninitialised or powered > off, but still detect card insertion/removal via GPIO? Is that a valid > scenario for you? > Theoretically, it is. Although my soc don't use phy for removable card, I also consider how to handle this case. Should we add a hook for sdhci_get_cd, and initialize phy if it's non-removeable device or removeable card in slot ? Doesn't seem like a good idea. Also seems not always work if we just use SDHCI_PRESENT_STATE to detect card insertion/removal without phy in active state. > 2) Considering the runtime PM case for the sdhci device. Typically you > can gate clocks etc at runtime suspend to save power, but what about > the phy? Can you power off it in runtime suspend? yes, we can power off it in runtime suspend. So we can append some patches later to introduce runtime pm for sdhci-of-arasan? > > [...] > > Kind regards > Uffe > > >
[...] >>> >>>> >>>>> + ret = phy_power_on(sdhci_arasan->phy); >>>> >>>> >>>> >>>> This looks a bit weird. Shouldn't you do phy_init() prior >>>> phy_power_on()? >>>> >>>> Similar comment applies to phy_exit() and phy_power_off(). >>> >>> >>> >>> Both are okay. It depends how the phy-driver implement the four >>> interfaces. >>> From my case, power_on for arasan's phy driver firstly enable phy's clk >>> and >>> open power-domain, then programme phy internal registers to activate phy. >>> Without enabling phy's clk and power-domain, we cannot do phy_init since >>> phy >>> can't be accessed by CPU. >>> >>> But here, I think you are right. It does look a bit weird. >>> I think the better way is to remove phy_power_on here, and let phy-driver >>> do >>> power_on in phy_init internally. Similarly in the remove call. >> >> >> That makes sense to me! I think it would also follow other phy clients >> use of the phy API. >> >> What makes me wonder though is from a power management point of view. >> *When* do you need to have phy initialized and powered? >> > > Whenever controller need to communicate with card, must init/power_on phy > firstly. > >> 1) For a removable card can leave the phy uninitialised or powered >> off, but still detect card insertion/removal via GPIO? Is that a valid >> scenario for you? >> > > Theoretically, it is. Although my soc don't use phy for removable card, I > also consider how to handle this case. Should we add a hook > for sdhci_get_cd, and initialize phy if it's non-removeable device or > removeable card in slot ? Doesn't seem like a good idea. > > Also seems not always work if we just use SDHCI_PRESENT_STATE to detect card > insertion/removal without phy in active state. As you SoC don't use removable card, let's just leave this for now. Thanks for sharing your thoughts. > >> 2) Considering the runtime PM case for the sdhci device. Typically you >> can gate clocks etc at runtime suspend to save power, but what about >> the phy? Can you power off it in runtime suspend? > > > yes, we can power off it in runtime suspend. So we can append some patches > later to introduce runtime pm for sdhci-of-arasan? Yes, that's fine. I would thus expect that you want to do phy power off/on from the runtime PM callbacks, right!? If that's the case I think $subject patch should deal with *both* phy init and phy power on during ->probe(). Kind regards Uffe
On 2015/10/19 18:54, Ulf Hansson wrote: > [...] > [...] > >> >>> 2) Considering the runtime PM case for the sdhci device. Typically you >>> can gate clocks etc at runtime suspend to save power, but what about >>> the phy? Can you power off it in runtime suspend? >> >> >> yes, we can power off it in runtime suspend. So we can append some patches >> later to introduce runtime pm for sdhci-of-arasan? > > Yes, that's fine. I would thus expect that you want to do phy power > off/on from the runtime PM callbacks, right!? > > If that's the case I think $subject patch should deal with *both* phy > init and phy power on during ->probe(). > yes. I will do phy power_off/on from runtime pm for the next version of $subject patchset. Thanks. > Kind regards > Uffe > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 621c3f4..fdd71c7 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -21,6 +21,7 @@ #include <linux/module.h> #include <linux/of_device.h> +#include <linux/phy/phy.h> #include "sdhci-pltfm.h" #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c @@ -35,6 +36,7 @@ */ struct sdhci_arasan_data { struct clk *clk_ahb; + struct phy *phy; }; static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) @@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = { #ifdef CONFIG_PM_SLEEP /** + * sdhci_arasan_suspend_phy - Suspend phy method for the driver + * @phy: Handler of phy structure + * Returns 0 on success and error value on error + * + * Put the phy in a deactive state. + */ +static int sdhci_arasan_suspend_phy(struct phy *phy) +{ + int ret; + + ret = phy_exit(phy); + if (ret < 0) + goto err_phy_exit; + + ret = phy_power_off(phy); + if (ret < 0) + goto err_phy_pwr_off; + + return 0; + +err_phy_pwr_off: + phy_power_on(phy); +err_phy_exit: + phy_init(phy); + return ret; +} + +/** + * sdhci_arasan_resume_phy - Resume phy method for the driver + * @phy: Handler of phy structure + * Returns 0 on success and error value on error + * + * Put the phy in a active state. + */ +static int sdhci_arasan_resume_phy(struct phy *phy) +{ + int ret; + + ret = phy_power_on(phy); + if (ret < 0) + goto err_phy_pwr_on; + + ret = phy_init(phy); + if (ret < 0) + goto err_phy_init; + + return 0; + +err_phy_init: + phy_exit(phy); +err_phy_pwr_on: + phy_power_off(phy); + return ret; +} + +/** * sdhci_arasan_suspend - Suspend method for the driver * @dev: Address of the device structure * Returns 0 on success and error value on error @@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev) clk_disable(pltfm_host->clk); clk_disable(sdhci_arasan->clk_ahb); + if (sdhci_arasan->phy) { + ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy); + if (ret < 0) + return ret; + } + return 0; } @@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev) return ret; } + if (sdhci_arasan->phy) { + ret = sdhci_arasan_resume_phy(sdhci_arasan->phy); + if (ret < 0) + return ret; + } + return sdhci_resume_host(host); } #endif /* ! CONFIG_PM_SLEEP */ @@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device *pdev) goto clk_dis_ahb; } + sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan"); + if (!IS_ERR(sdhci_arasan->phy)) { + ret = phy_power_on(sdhci_arasan->phy); + if (ret < 0) { + dev_err(&pdev->dev, "phy_power_on err.\n"); + phy_power_off(sdhci_arasan->phy); + goto clk_dis_ahb; + } + + ret = phy_init(sdhci_arasan->phy); + if (ret < 0) { + dev_err(&pdev->dev, "phy_init err.\n"); + phy_exit(sdhci_arasan->phy); + phy_power_off(sdhci_arasan->phy); + goto clk_dis_ahb; + } + } else { + sdhci_arasan->phy = NULL; + } + host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0); if (IS_ERR(host)) { ret = PTR_ERR(host);
This patch adds Generic PHY access for sdhci-of-arasan. Driver can get PHY handler from dt-binding, and power-on/init the PHY. Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/host/sdhci-of-arasan.c | 90 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)