| Message ID | 1442212161-23234-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shawn, overall, it looks good to me. I have some questions though. On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin 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. > Currently, it's just mandatory for arasan,sdhci-5.1. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > Serise-changes: 2 > - Keep phy as a mandatory requirement for arasan,sdhci-5.1 > > --- > > Changes in v2: None > > drivers/mmc/host/sdhci-of-arasan.c | 97 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 75379cb..2c13ef8 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) > @@ -70,6 +72,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); Just to confirm, are these actions in the error path correct? E.g. if the power_off() call fails, is it safe to call power_on()? Isn't the phy still powered on? (this would apply to other error paths too) > + 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 > @@ -91,6 +149,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; > } > > @@ -122,6 +186,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 */ > @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > goto clk_dis_ahb; > } > > + sdhci_arasan->phy = NULL; > + if (of_device_is_compatible(pdev->dev.of_node, > + "arasan,sdhci-5.1")) { > + sdhci_arasan->phy = devm_phy_get(&pdev->dev, > + "phy_arasan"); > + if (IS_ERR(sdhci_arasan->phy)) { > + ret = -ENODEV; > + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n"); > + goto clk_dis_ahb; > + } > + > + 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; > + } > + } I assume you looked at options for having the error paths in a consolidated location? I guess this may be the nicest solution since all of this is in this conditional block? Feel free to add my Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com> Sören -- 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
On 2015/9/14 23:07, Sören Brinkmann wrote: > Hi Shawn, > > overall, it looks good to me. I have some questions though. > > On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote: [...] >> +err_phy_exit: >> + phy_init(phy); > > Just to confirm, are these actions in the error path correct? E.g. > if the power_off() call fails, is it safe to call power_on()? Isn't > the phy still powered on? (this would apply to other error paths too) > Cool question! While writing this, I had read generic phy stuffs deliberately to find a solution for a case: how to deal with ping-pong fails? In another word, if power_off call fails, then we should call power_on, but unfortunately it fails again then we call power_off... so endless nested err handling... No answer yet. So, I assumed two cases happened when power_off call fails: (1) *real power_off* is done, but some other stuffs in the calling path fail, so phy is really power_off in theory. We need to power_on it again, but if it fail, we don't know PHY is on or off since we don't know power_on fails for what? *real power on* ? some other stuffs? (2) *real power_off* isn't completed, so indeed it's *still* in power_on state. The reason we never need to check the return value of power_on cross the err handling is that whether power_on call successfully or not, it's always make phy in power_on state. Now, let's think about case(1). After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt generic phy framework for PHY management, real thing should be like that: they NEVER deal with case(1). It's a trick of sub-phy drivers themself. power_on/off calling path return err for two case: <1> phy_runtime callback fails. It's after *real power on/off*, so definitely *real power on/off* is conpleted. That is the case(2) I mentioned above. <2> sub-phy drivers return err for phy->ops->power_off(phy); Look into all the sub-phy drivers twice, we find that they always return success for phy->ops->power_off hook. Why? Because all of them write registers to enable/disable something, always consider things going well. Actually if we write value into a register, we have to think that it's functional. Anyway, back to this patch. Indeed we also write value into arasan phy's register to do phy_power_on/off/init/exit to make things work. Right, we return success state for all of these them just as all the other sub-phy drivers do. Feel free to let me know if I make mistakes or misunderstanding above. >> + return ret; >> +} [...] >> + } >> + } > > I assume you looked at options for having the error paths in a > consolidated location? I guess this may be the nicest solution since all > of this is in this conditional block? > yep, otherwise we should add some *if* statements to check sdhci_arasan->phy cross the err handles. And I intent to strictly limit the phy stuffs under the scope of arasan,sdhci-5.1 currently. > Feel free to add my > Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com> > Thanks, Sören. :) > Sören > > >
On 14 September 2015 at 08:29, 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. > Currently, it's just mandatory for arasan,sdhci-5.1. I am trying to understand how a PHY can be used together with a MMC/SD/SDIO controller. Normally the card connector doesn't hold any intelligence, so I wonder if PHY is correctly used here. Could you try to explain, HW-wise, what role the PHY has for you? Kind regards Uffe > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > Serise-changes: 2 > - Keep phy as a mandatory requirement for arasan,sdhci-5.1 > > --- > > Changes in v2: None > > drivers/mmc/host/sdhci-of-arasan.c | 97 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 75379cb..2c13ef8 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) > @@ -70,6 +72,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 > @@ -91,6 +149,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; > } > > @@ -122,6 +186,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 */ > @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > goto clk_dis_ahb; > } > > + sdhci_arasan->phy = NULL; > + if (of_device_is_compatible(pdev->dev.of_node, > + "arasan,sdhci-5.1")) { > + sdhci_arasan->phy = devm_phy_get(&pdev->dev, > + "phy_arasan"); > + if (IS_ERR(sdhci_arasan->phy)) { > + ret = -ENODEV; > + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n"); > + goto clk_dis_ahb; > + } > + > + 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; > + } > + } > + > host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0); > if (IS_ERR(host)) { > ret = PTR_ERR(host); > -- > 2.3.7 > > > -- > 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 -- 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
On 2015/9/17 19:44, Ulf Hansson wrote: > On 14 September 2015 at 08:29, 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. >> Currently, it's just mandatory for arasan,sdhci-5.1. > > I am trying to understand how a PHY can be used together with a > MMC/SD/SDIO controller. Normally the card connector doesn't hold any > intelligence, so I wonder if PHY is correctly used here. > > Could you try to explain, HW-wise, what role the PHY has for you? > Thanks for comment, Ulf. It's the first time we introduce phy into mmc subsystem,also it's my first time to use phy for emmc in real Soc. so definitely we need to discuss it deliberately. From my view, phy is an active-power, differential sampling and more well-designed analog *IO component* for ultra-highspeed device to guarantee its signal integrity, like USB, UFS, DDR-RAM,MIPI and so on. (1)Firstly it contains DLL to generate delayline and phase for sampling data from mmc devices. When sdhci controller executes tuning and sends out tuning sequence, and PHY can auto change its delayline and phase in order to test if this "sample timing" is okay. If not, CRC err is generate back to the controller. Then after SDHCI tune timing for 40 times, also PHY have scanned all the "sample windows" by trying different delayline and phase combination, and finally auto-select the best sample timing for the "sample windows". And sdhci HOST CONTROL 2 Register[6:7] is controlled by PHY in my case. (2)Then phy can programming source/sink impedance to avoid signal reflex. Given that JEDEC has come to HS533(highspeed 533MHz), this function is imperative to be implemented just like the role of ODT for DDR-RAM's PHY. (3)Phy is integrated with diff pull-up/down resistance value and open-drain type for HW design. (4)Phy can also have some enable-bit to decide whether all mmc signal(clk/data/cmd/strobe) can output to the devices or not. For the card, even for host controller itself, phy can just be regarded as a need-to-do-something-before-used GPIO. So, before we start initializing card, we need to power up PHY, enable clk for it and configure all the stuffs. Also we need to power down it for power-saving when into suspend. That's all we need to do and that's all the generic phy framework had provided by four interface: phy_init/phy_exit/phy_power_on/phy_power_off. Sincerely welcome any comments here to make things better. :) > Kind regards > Uffe > >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> >> Serise-changes: 2 >> - Keep phy as a mandatory requirement for arasan,sdhci-5.1 >> >> --- >> >> Changes in v2: None >> >> drivers/mmc/host/sdhci-of-arasan.c | 97 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 97 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c >> index 75379cb..2c13ef8 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) >> @@ -70,6 +72,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 >> @@ -91,6 +149,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; >> } >> >> @@ -122,6 +186,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 */ >> @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev) >> goto clk_dis_ahb; >> } >> >> + sdhci_arasan->phy = NULL; >> + if (of_device_is_compatible(pdev->dev.of_node, >> + "arasan,sdhci-5.1")) { >> + sdhci_arasan->phy = devm_phy_get(&pdev->dev, >> + "phy_arasan"); >> + if (IS_ERR(sdhci_arasan->phy)) { >> + ret = -ENODEV; >> + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n"); >> + goto clk_dis_ahb; >> + } >> + >> + 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; >> + } >> + } >> + >> host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0); >> if (IS_ERR(host)) { >> ret = PTR_ERR(host); >> -- >> 2.3.7 >> >> >> -- >> 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 > > >
On 17 September 2015 at 17:20, Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2015/9/17 19:44, Ulf Hansson wrote: >> >> On 14 September 2015 at 08:29, 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. >>> Currently, it's just mandatory for arasan,sdhci-5.1. >> >> >> I am trying to understand how a PHY can be used together with a >> MMC/SD/SDIO controller. Normally the card connector doesn't hold any >> intelligence, so I wonder if PHY is correctly used here. >> >> Could you try to explain, HW-wise, what role the PHY has for you? >> > > Thanks for comment, Ulf. > > It's the first time we introduce phy into mmc subsystem,also it's my first > time to use phy for emmc in real Soc. > so definitely we need to discuss it deliberately. > > From my view, phy is an active-power, differential sampling and more > well-designed analog *IO component* for ultra-highspeed device to guarantee > its signal integrity, like USB, UFS, DDR-RAM,MIPI and so on. > > (1)Firstly it contains DLL to generate delayline and phase for sampling data > from mmc devices. When sdhci controller executes tuning and sends out tuning > sequence, and PHY can auto change its delayline and phase > in order to test if this "sample timing" is okay. If not, CRC err is > generate back to the controller. Then after SDHCI tune timing for 40 times, > also PHY have scanned all the "sample windows" by trying different delayline > and phase combination, and finally auto-select the best sample timing for > the "sample windows". And sdhci HOST CONTROL > 2 Register[6:7] is controlled by PHY in my case. > > (2)Then phy can programming source/sink impedance to avoid signal reflex. > Given that JEDEC has come to HS533(highspeed 533MHz), this function is > imperative to be implemented just like the role of ODT for DDR-RAM's PHY. > > (3)Phy is integrated with diff pull-up/down resistance value and open-drain > type for HW design. > > (4)Phy can also have some enable-bit to decide whether all mmc > signal(clk/data/cmd/strobe) can output to the devices or not. Thanks for elaborating. It seems like using phy is justified in your case. > > > For the card, even for host controller itself, phy can just be regarded as a > need-to-do-something-before-used GPIO. > So, before we start initializing card, we need to power up PHY, enable clk > for it and configure all the stuffs. Also we need to power down it for > power-saving when into suspend. That's all we need to do and that's all the > generic phy framework had provided by four interface: > phy_init/phy_exit/phy_power_on/phy_power_off. > > > Sincerely welcome any comments here to make things better. :) > > I will get back to your patch and review it in detail as soon as can. 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
On Tue, 2015-09-15 at 09:07AM +0800, Shawn Lin wrote: > On 2015/9/14 23:07, Sören Brinkmann wrote: > >Hi Shawn, > > > >overall, it looks good to me. I have some questions though. > > > >On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote: > > [...] > > >>+err_phy_exit: > >>+ phy_init(phy); > > > >Just to confirm, are these actions in the error path correct? E.g. > >if the power_off() call fails, is it safe to call power_on()? Isn't > >the phy still powered on? (this would apply to other error paths too) > > > > Cool question! > > While writing this, I had read generic phy stuffs deliberately to find a > solution for a case: how to deal with ping-pong fails? In another word, if > power_off call fails, then we should call power_on, but unfortunately it > fails again then we call power_off... so endless nested err handling... No > answer yet. > > So, I assumed two cases happened when power_off call fails: > (1) *real power_off* is done, but some other stuffs in the calling path > fail, so phy is really power_off in theory. We need to power_on it again, > but if it fail, we don't know PHY is on or off since we don't know power_on > fails for what? *real power on* ? some other stuffs? > > (2) *real power_off* isn't completed, so indeed it's *still* in power_on > state. The reason we never need to check the return value of power_on cross > the err handling is that whether power_on call successfully or not, it's > always make phy in power_on state. > > Now, let's think about case(1). > After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt > generic phy framework for PHY management, real thing should be like that: > they NEVER deal with case(1). > > It's a trick of sub-phy drivers themself. power_on/off calling path return > err for two case: > <1> phy_runtime callback fails. It's after *real power on/off*, so > definitely *real power on/off* is conpleted. That is the case(2) I mentioned > above. > <2> sub-phy drivers return err for phy->ops->power_off(phy); Look > into all the sub-phy drivers twice, we find that they always return success > for phy->ops->power_off hook. Why? Because all of them > write registers to enable/disable something, always consider things going > well. Actually if we write value into a register, we have to think that it's > functional. > > Anyway, back to this patch. > Indeed we also write value into arasan phy's register to do > phy_power_on/off/init/exit to make things work. Right, we return success > state for all of these them just as all the other sub-phy drivers do. > > Feel free to let me know if I make mistakes or misunderstanding above. > > >>+ return ret; > >>+} > > [...] > > >>+ } > >>+ } > > > >I assume you looked at options for having the error paths in a > >consolidated location? I guess this may be the nicest solution since all > >of this is in this conditional block? > > > > yep, otherwise we should add some *if* statements to check sdhci_arasan->phy > cross the err handles. And I intent to strictly limit > the phy stuffs under the scope of arasan,sdhci-5.1 currently. > > >Feel free to add my > >Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com> > > > > Thanks, Sören. :) Makes all sense to me. Thanks for all the details. Sören -- 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 75379cb..2c13ef8 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) @@ -70,6 +72,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 @@ -91,6 +149,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; } @@ -122,6 +186,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 */ @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev) goto clk_dis_ahb; } + sdhci_arasan->phy = NULL; + if (of_device_is_compatible(pdev->dev.of_node, + "arasan,sdhci-5.1")) { + sdhci_arasan->phy = devm_phy_get(&pdev->dev, + "phy_arasan"); + if (IS_ERR(sdhci_arasan->phy)) { + ret = -ENODEV; + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n"); + goto clk_dis_ahb; + } + + 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; + } + } + 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. Currently, it's just mandatory for arasan,sdhci-5.1. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> Serise-changes: 2 - Keep phy as a mandatory requirement for arasan,sdhci-5.1 --- Changes in v2: None drivers/mmc/host/sdhci-of-arasan.c | 97 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)