| Message ID | 1387311713-1926-3-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tejun, On Wednesday 18 December 2013 01:51 AM, Andrew Lunn wrote: > Some Marvell SoCs have a SATA PHY which can be powered off, in order > to save power. Make use of the generic phy framework to control these > phys. I can queue this patch along with the other patches in this series if you don't expect any merge issues. If you want to take this patch by yourself, you can add my Acked-by: Kishon Vijay Abraham I <kishon@ti.com> Either ways let me know. Cheers Kishon > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > v2->v3 > Look for phys using name "port0", "port1", etc. > --- > drivers/ata/sata_mv.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c > index 56be31819897..896725bb5e34 100644 > --- a/drivers/ata/sata_mv.c > +++ b/drivers/ata/sata_mv.c > @@ -60,6 +60,7 @@ > #include <linux/dma-mapping.h> > #include <linux/device.h> > #include <linux/clk.h> > +#include <linux/phy/phy.h> > #include <linux/platform_device.h> > #include <linux/ata_platform.h> > #include <linux/mbus.h> > @@ -563,6 +564,12 @@ struct mv_host_priv { > struct clk *clk; > struct clk **port_clks; > /* > + * Some devices have a SATA PHY which can be enabled/disabled > + * in order to save power. These are optional: if the platform > + * devices does not have any phy, they won't be used. > + */ > + struct phy **port_phys; > + /* > * These consistent DMA memory pools give us guaranteed > * alignment for hardware-accessed data structures, > * and less memory waste in accomplishing the alignment. > @@ -4076,6 +4083,11 @@ static int mv_platform_probe(struct platform_device *pdev) > GFP_KERNEL); > if (!hpriv->port_clks) > return -ENOMEM; > + hpriv->port_phys = devm_kzalloc(&pdev->dev, > + sizeof(struct phy *) * n_ports, > + GFP_KERNEL); > + if (!hpriv->port_phys) > + return -ENOMEM; > host->private_data = hpriv; > hpriv->n_ports = n_ports; > hpriv->board_idx = chip_soc; > @@ -4097,6 +4109,10 @@ static int mv_platform_probe(struct platform_device *pdev) > hpriv->port_clks[port] = clk_get(&pdev->dev, port_number); > if (!IS_ERR(hpriv->port_clks[port])) > clk_prepare_enable(hpriv->port_clks[port]); > + sprintf(port_number, "port%d", port); > + hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number); > + if (!IS_ERR(hpriv->port_phys[port])) > + phy_power_on(hpriv->port_phys[port]); > } > > /* > @@ -4132,6 +4148,8 @@ err: > clk_disable_unprepare(hpriv->port_clks[port]); > clk_put(hpriv->port_clks[port]); > } > + if (!IS_ERR(hpriv->port_phys[port])) > + phy_power_off(hpriv->port_phys[port]); > } > > return rc; > @@ -4161,6 +4179,8 @@ static int mv_platform_remove(struct platform_device *pdev) > clk_disable_unprepare(hpriv->port_clks[port]); > clk_put(hpriv->port_clks[port]); > } > + if (!IS_ERR(hpriv->port_phys[port])) > + phy_power_off(hpriv->port_phys[port]); > } > return 0; > } >
Hello, On Wed, Dec 18, 2013 at 12:00:10PM +0530, Kishon Vijay Abraham I wrote: > > @@ -4097,6 +4109,10 @@ static int mv_platform_probe(struct platform_device *pdev) > > hpriv->port_clks[port] = clk_get(&pdev->dev, port_number); > > if (!IS_ERR(hpriv->port_clks[port])) > > clk_prepare_enable(hpriv->port_clks[port]); > > + sprintf(port_number, "port%d", port); > > + hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number); > > + if (!IS_ERR(hpriv->port_phys[port])) > > + phy_power_on(hpriv->port_phys[port]); Shouldn't it distinguish between failures and at least produce warning? ie. phy not available and phy init failed due to memory pressure or whatnot shouldn't be handled the same. > > @@ -4132,6 +4148,8 @@ err: > > clk_disable_unprepare(hpriv->port_clks[port]); > > clk_put(hpriv->port_clks[port]); > > } > > + if (!IS_ERR(hpriv->port_phys[port])) > > + phy_power_off(hpriv->port_phys[port]); And I'd much prefer the array holds either NULL or valid pointer. Thanks.
On Wed, Dec 18, 2013 at 12:00:10PM +0530, Kishon Vijay Abraham I wrote: > Hi Tejun, > > On Wednesday 18 December 2013 01:51 AM, Andrew Lunn wrote: > > Some Marvell SoCs have a SATA PHY which can be powered off, in order > > to save power. Make use of the generic phy framework to control these > > phys. > > I can queue this patch along with the other patches in this series if you don't > expect any merge issues. Hi Kishon I expect merge issues with the DT and the DT binding documentation. It would be best if Jason took those parts. Andrew
On Wed, Dec 18, 2013 at 07:13:13AM -0500, Tejun Heo wrote: > Hello, > > On Wed, Dec 18, 2013 at 12:00:10PM +0530, Kishon Vijay Abraham I wrote: > > > @@ -4097,6 +4109,10 @@ static int mv_platform_probe(struct platform_device *pdev) > > > hpriv->port_clks[port] = clk_get(&pdev->dev, port_number); > > > if (!IS_ERR(hpriv->port_clks[port])) > > > clk_prepare_enable(hpriv->port_clks[port]); > > > + sprintf(port_number, "port%d", port); > > > + hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number); > > > + if (!IS_ERR(hpriv->port_phys[port])) > > > + phy_power_on(hpriv->port_phys[port]); > > Shouldn't it distinguish between failures and at least produce > warning? ie. phy not available and phy init failed due to memory > pressure or whatnot shouldn't be handled the same. Phy not available is not an error, since not all variants of the SATA IP block have the ability to control the phy. I can however add a warning for real errors. > > > @@ -4132,6 +4148,8 @@ err: > > > clk_disable_unprepare(hpriv->port_clks[port]); > > > clk_put(hpriv->port_clks[port]); > > > } > > > + if (!IS_ERR(hpriv->port_phys[port])) > > > + phy_power_off(hpriv->port_phys[port]); > > And I'd much prefer the array holds either NULL or valid pointer. I was trying to keep the code similar to the clk handling. However now that it is diverging more and more from how clk is handled, i can add yet more divergence and overwrite the error with a NULL. Andrew
Hello, On Thu, Dec 19, 2013 at 08:10:27PM +0100, Andrew Lunn wrote: > > Shouldn't it distinguish between failures and at least produce > > warning? ie. phy not available and phy init failed due to memory > > pressure or whatnot shouldn't be handled the same. > > Phy not available is not an error, since not all variants of the SATA ^ always > IP block have the ability to control the phy. I can however add a > warning for real errors. Yes, please. > > > > @@ -4132,6 +4148,8 @@ err: > > > > clk_disable_unprepare(hpriv->port_clks[port]); > > > > clk_put(hpriv->port_clks[port]); > > > > } > > > > + if (!IS_ERR(hpriv->port_phys[port])) > > > > + phy_power_off(hpriv->port_phys[port]); > > > > And I'd much prefer the array holds either NULL or valid pointer. > > I was trying to keep the code similar to the clk handling. However now > that it is diverging more and more from how clk is handled, i can add > yet more divergence and overwrite the error with a NULL. Thanks.
On Thu, Dec 19, 2013 at 08:03:29PM +0100, Andrew Lunn wrote: > On Wed, Dec 18, 2013 at 12:00:10PM +0530, Kishon Vijay Abraham I wrote: > > Hi Tejun, > > > > On Wednesday 18 December 2013 01:51 AM, Andrew Lunn wrote: > > > Some Marvell SoCs have a SATA PHY which can be powered off, in order > > > to save power. Make use of the generic phy framework to control these > > > phys. > > > > I can queue this patch along with the other patches in this series if you don't > > expect any merge issues. > > Hi Kishon > > I expect merge issues with the DT and the DT binding documentation. It > would be best if Jason took those parts. I'd prefer to keep the DT binding docs with the driver patches. thx, Jason.
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index 56be31819897..896725bb5e34 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -60,6 +60,7 @@ #include <linux/dma-mapping.h> #include <linux/device.h> #include <linux/clk.h> +#include <linux/phy/phy.h> #include <linux/platform_device.h> #include <linux/ata_platform.h> #include <linux/mbus.h> @@ -563,6 +564,12 @@ struct mv_host_priv { struct clk *clk; struct clk **port_clks; /* + * Some devices have a SATA PHY which can be enabled/disabled + * in order to save power. These are optional: if the platform + * devices does not have any phy, they won't be used. + */ + struct phy **port_phys; + /* * These consistent DMA memory pools give us guaranteed * alignment for hardware-accessed data structures, * and less memory waste in accomplishing the alignment. @@ -4076,6 +4083,11 @@ static int mv_platform_probe(struct platform_device *pdev) GFP_KERNEL); if (!hpriv->port_clks) return -ENOMEM; + hpriv->port_phys = devm_kzalloc(&pdev->dev, + sizeof(struct phy *) * n_ports, + GFP_KERNEL); + if (!hpriv->port_phys) + return -ENOMEM; host->private_data = hpriv; hpriv->n_ports = n_ports; hpriv->board_idx = chip_soc; @@ -4097,6 +4109,10 @@ static int mv_platform_probe(struct platform_device *pdev) hpriv->port_clks[port] = clk_get(&pdev->dev, port_number); if (!IS_ERR(hpriv->port_clks[port])) clk_prepare_enable(hpriv->port_clks[port]); + sprintf(port_number, "port%d", port); + hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number); + if (!IS_ERR(hpriv->port_phys[port])) + phy_power_on(hpriv->port_phys[port]); } /* @@ -4132,6 +4148,8 @@ err: clk_disable_unprepare(hpriv->port_clks[port]); clk_put(hpriv->port_clks[port]); } + if (!IS_ERR(hpriv->port_phys[port])) + phy_power_off(hpriv->port_phys[port]); } return rc; @@ -4161,6 +4179,8 @@ static int mv_platform_remove(struct platform_device *pdev) clk_disable_unprepare(hpriv->port_clks[port]); clk_put(hpriv->port_clks[port]); } + if (!IS_ERR(hpriv->port_phys[port])) + phy_power_off(hpriv->port_phys[port]); } return 0; }
Some Marvell SoCs have a SATA PHY which can be powered off, in order to save power. Make use of the generic phy framework to control these phys. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- v2->v3 Look for phys using name "port0", "port1", etc. --- drivers/ata/sata_mv.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)