| Message ID | 20181123101556.29888-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Bring suspend to RAM support to MVEBU SATA | expand |
Hi Miquel, On 23-11-18 11:15, Miquel Raynal wrote: > Current implementation of the libahci does not take into account the > new PHY framework. Correct the situation by adding a call to > phy_set_mode() before phy_power_on() and by adding calls to > ahci_platform_enable/disable_phys() at suspend/resume_host() time. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/ata/libahci_platform.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 4b900fc659f7..9f33f72b674b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) > if (rc) > goto disable_phys; > > + rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA); > + if (rc) { > + phy_exit(hpriv->phys[i]); > + goto disable_phys; > + } > + I see that phy_set_mode returns 0 for drivers which do not implement it, so this should be fine. > rc = phy_power_on(hpriv->phys[i]); > if (rc) { > phy_exit(hpriv->phys[i]); > @@ -738,6 +744,8 @@ int ahci_platform_suspend_host(struct device *dev) > writel(ctl, mmio + HOST_CTL); > readl(mmio + HOST_CTL); /* flush */ > > + ahci_platform_disable_phys(hpriv); > + > return ata_host_suspend(host, PMSG_SUSPEND); > } > EXPORT_SYMBOL_GPL(ahci_platform_suspend_host); I'm afraid that this and the matching change in ahci_platform_suspend_host needs to be guarded by a flag, there are quite a few sata drivers using the libahci_platform functions as well as quite a few sata drivers combining this with using phy drivers. I'm worried that doing this unconditionally on drivers which have not been tested with this change my break things. I think it might be cleanest to extend the existing flags passed to ahci_platform_get_resources with a flag for this and storing them somewhere in ahci_host_priv so that the suspend/resume functions can get to them. Regards, Hans > @@ -756,6 +764,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend_host); > int ahci_platform_resume_host(struct device *dev) > { > struct ata_host *host = dev_get_drvdata(dev); > + struct ahci_host_priv *hpriv = host->private_data; > int rc; > > if (dev->power.power_state.event == PM_EVENT_SUSPEND) { > @@ -766,6 +775,8 @@ int ahci_platform_resume_host(struct device *dev) > ahci_init_controller(host); > } > > + ahci_platform_enable_phys(hpriv); > + > ata_host_resume(host); > > return 0; >
On Fri, Nov 23, 2018 at 11:15:50AM +0100, Miquel Raynal wrote: > Current implementation of the libahci does not take into account the > new PHY framework. Correct the situation by adding a call to > phy_set_mode() before phy_power_on() and by adding calls to > ahci_platform_enable/disable_phys() at suspend/resume_host() time. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/ata/libahci_platform.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 4b900fc659f7..9f33f72b674b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) > if (rc) > goto disable_phys; > > + rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA); > + if (rc) { > + phy_exit(hpriv->phys[i]); > + goto disable_phys; > + } Hi Miquel Russell King wrote a comphy driver for the Armada 3XX family. It only supports network PHYs. Did you check it does the right thing when passed PHY_MODE_SATA? This is slightly different to Hans's comment, in that i expect phy_set_mode() is implemented for the comphy driver, but it might not understand PHY_MODE_SATA and return an error? Maybe you should look at rc and keep going if EOPNOTSUPP is returned? Andrew
On Fri, Nov 23, 2018 at 04:18:06PM +0100, Andrew Lunn wrote: > Russell King wrote a comphy driver for the Armada 3XX family. It only > supports network PHYs. Did you check it does the right thing when > passed PHY_MODE_SATA? This is slightly different to Hans's comment, in > that i expect phy_set_mode() is implemented for the comphy driver, but > it might not understand PHY_MODE_SATA and return an error? It makes no difference until the comphy is wired up to the SATA device in the appropriate DT files. Until then, there's no way that the comphy will ever see PHY_MODE_SATA.
On Fri, Nov 23, 2018 at 03:27:30PM +0000, Russell King - ARM Linux wrote: > On Fri, Nov 23, 2018 at 04:18:06PM +0100, Andrew Lunn wrote: > > Russell King wrote a comphy driver for the Armada 3XX family. It only > > supports network PHYs. Did you check it does the right thing when > > passed PHY_MODE_SATA? This is slightly different to Hans's comment, in > > that i expect phy_set_mode() is implemented for the comphy driver, but > > it might not understand PHY_MODE_SATA and return an error? > > It makes no difference until the comphy is wired up to the SATA > device in the appropriate DT files. Until then, there's no way > that the comphy will ever see PHY_MODE_SATA. O.K, great. Thanks Andrew
Hi Hans, Thanks for the review. Hans de Goede <hdegoede@redhat.com> wrote on Fri, 23 Nov 2018 11:33:15 +0100: > Hi Miquel, > > On 23-11-18 11:15, Miquel Raynal wrote: > > Current implementation of the libahci does not take into account the > > new PHY framework. Correct the situation by adding a call to > > phy_set_mode() before phy_power_on() and by adding calls to > > ahci_platform_enable/disable_phys() at suspend/resume_host() time. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/ata/libahci_platform.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > > index 4b900fc659f7..9f33f72b674b 100644 > > --- a/drivers/ata/libahci_platform.c > > +++ b/drivers/ata/libahci_platform.c > > @@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) > > if (rc) > > goto disable_phys; > > > + rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA); > > + if (rc) { > > + phy_exit(hpriv->phys[i]); > > + goto disable_phys; > > + } > > + > > I see that phy_set_mode returns 0 for drivers which do not implement it, > so this should be fine. > > > > rc = phy_power_on(hpriv->phys[i]); > > if (rc) { > > phy_exit(hpriv->phys[i]); > > @@ -738,6 +744,8 @@ int ahci_platform_suspend_host(struct device *dev) > > writel(ctl, mmio + HOST_CTL); > > readl(mmio + HOST_CTL); /* flush */ > > > + ahci_platform_disable_phys(hpriv); > > + > > return ata_host_suspend(host, PMSG_SUSPEND); > > } > > EXPORT_SYMBOL_GPL(ahci_platform_suspend_host); > > I'm afraid that this and the matching change in ahci_platform_suspend_host > needs to be guarded by a flag, there are quite a few sata drivers > using the libahci_platform functions as well as quite a few sata drivers > combining this with using phy drivers. > > I'm worried that doing this unconditionally on drivers which have > not been tested with this change my break things. > > I think it might be cleanest to extend the existing flags passed > to ahci_platform_get_resources with a flag for this and storing them > somewhere in ahci_host_priv so that the suspend/resume functions can > get to them. I understand your concern, please have a look at the v2 which addresses this the way you suggested. Thanks, Miquèl
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 4b900fc659f7..9f33f72b674b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) if (rc) goto disable_phys; + rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA); + if (rc) { + phy_exit(hpriv->phys[i]); + goto disable_phys; + } + rc = phy_power_on(hpriv->phys[i]); if (rc) { phy_exit(hpriv->phys[i]); @@ -738,6 +744,8 @@ int ahci_platform_suspend_host(struct device *dev) writel(ctl, mmio + HOST_CTL); readl(mmio + HOST_CTL); /* flush */ + ahci_platform_disable_phys(hpriv); + return ata_host_suspend(host, PMSG_SUSPEND); } EXPORT_SYMBOL_GPL(ahci_platform_suspend_host); @@ -756,6 +764,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend_host); int ahci_platform_resume_host(struct device *dev) { struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; int rc; if (dev->power.power_state.event == PM_EVENT_SUSPEND) { @@ -766,6 +775,8 @@ int ahci_platform_resume_host(struct device *dev) ahci_init_controller(host); } + ahci_platform_enable_phys(hpriv); + ata_host_resume(host); return 0;
Current implementation of the libahci does not take into account the new PHY framework. Correct the situation by adding a call to phy_set_mode() before phy_power_on() and by adding calls to ahci_platform_enable/disable_phys() at suspend/resume_host() time. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/ata/libahci_platform.c | 11 +++++++++++ 1 file changed, 11 insertions(+)