[1/7] ata: libahci_platform: comply to PHY framework
diff mbox series

Message ID 20181123101556.29888-2-miquel.raynal@bootlin.com
State New, archived
Headers show
Series
  • Bring suspend to RAM support to MVEBU SATA
Related show

Commit Message

Miquel Raynal Nov. 23, 2018, 10:15 a.m. UTC
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(+)

Comments

Hans de Goede Nov. 23, 2018, 10:33 a.m. UTC | #1
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;
>
Andrew Lunn Nov. 23, 2018, 3:18 p.m. UTC | #2
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
Russell King - ARM Linux admin Nov. 23, 2018, 3:27 p.m. UTC | #3
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.
Andrew Lunn Nov. 23, 2018, 3:36 p.m. UTC | #4
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
Miquel Raynal Nov. 30, 2018, 3:40 p.m. UTC | #5
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

Patch
diff mbox series

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;