| Message ID | 1379595943-14622-9-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 19, 2013 at 04:05:36PM +0300, Roger Quadros wrote: > From: Balaji T K <balajitk@ti.com> > > Some platforms have a PHY hooked up to the > SATA controller. The PHY needs to be initialized > and powered up for SATA to work. We do that > using the PHY framework. > > [Roger Q] Cleaned up. > > CC: Tejun Heo <tj@kernel.org> > Signed-off-by: Balaji T K <balajitk@ti.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> Looks okay to me although I don't know whether everyone using ahci_platform would be happy with requiring phy. Sergei, does this look good to you? Thanks.
Hello. On 09/19/2013 05:05 PM, Roger Quadros wrote: > From: Balaji T K <balajitk@ti.com> > Some platforms have a PHY hooked up to the > SATA controller. The PHY needs to be initialized > and powered up for SATA to work. We do that > using the PHY framework. > [Roger Q] Cleaned up. > CC: Tejun Heo <tj@kernel.org> > Signed-off-by: Balaji T K <balajitk@ti.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> [...] > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 1145637..94484cb 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -37,6 +37,7 @@ > > #include <linux/clk.h> > #include <linux/libata.h> > +#include <linux/phy/phy.h> struct phy; should suffice. > @@ -322,6 +323,7 @@ struct ahci_host_priv { > u32 em_buf_sz; /* EM buffer size in byte */ > u32 em_msg_type; /* EM message type */ > struct clk *clk; /* Only for platforms supporting clk */ > + struct phy *phy; /* If platforms use phy */ > void *plat_data; /* Other platform data */ > }; > > diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c > index 2daaee0..f812ffa 100644 > --- a/drivers/ata/ahci_platform.c > +++ b/drivers/ata/ahci_platform.c > @@ -23,6 +23,7 @@ > #include <linux/platform_device.h> > #include <linux/libata.h> > #include <linux/ahci_platform.h> > +#include <linux/phy/phy.h> Why include it from both ahci.h and here? > #include "ahci.h" > > static void ahci_host_stop(struct ata_host *host); > @@ -141,16 +142,32 @@ static int ahci_probe(struct platform_device *pdev) > } > } > > + hpriv->phy = devm_phy_get(dev, "sata-phy"); > + if (IS_ERR(hpriv->phy)) { > + dev_err(dev, "can't get phy\n"); Don't think it's a good idea to complain about missing PHY when the driver doesn't even use it. > + /* return only if -EPROBE_DEFER */ > + if (PTR_ERR(hpriv->phy) == -EPROBE_DEFER) { > + rc = -EPROBE_DEFER; > + goto disable_unprepare_clk; > + } > + } > + > /* > * Some platforms might need to prepare for mmio region access, > * which could be done in the following init call. So, the mmio > * region shouldn't be accessed before init (if provided) has > * returned successfully. > */ > + > + if (!(IS_ERR(hpriv->phy))) { () not needed around IS_ERR() invocation. > + phy_init(hpriv->phy); > + phy_power_on(hpriv->phy); > + } > + I think this is misplaced, i.e. it should precede the comment. > if (pdata && pdata->init) { > rc = pdata->init(dev, hpriv->mmio); > if (rc) > - goto disable_unprepare_clk; > + goto disable_phy; > } > > ahci_save_initial_config(dev, hpriv, [...] > @@ -328,6 +356,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume); > static const struct of_device_id ahci_of_match[] = { > { .compatible = "snps,spear-ahci", }, > { .compatible = "snps,exynos5440-ahci", }, > + { .compatible = "snps,dwc-ahci", }, Looks like the binding documentation is incomplete -- it doesn't list "snps,exynos5440-ahci"... WBR, Sergei
Hello. On 09/22/2013 08:58 PM, Tejun Heo wrote: >> From: Balaji T K <balajitk@ti.com> >> Some platforms have a PHY hooked up to the >> SATA controller. The PHY needs to be initialized >> and powered up for SATA to work. We do that >> using the PHY framework. >> [Roger Q] Cleaned up. >> CC: Tejun Heo <tj@kernel.org> >> Signed-off-by: Balaji T K <balajitk@ti.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> > Looks okay to me although I don't know whether everyone using > ahci_platform would be happy with requiring phy. Sergei, does this > look good to you? Not sure why you asked -- I'm not using this driver, neither I'm the author of it (former MV's Anton Vorontsov is IIRC). I've commented on the patch anyway though... > Thanks. WBR, Sergei
Hello, On Sun, Sep 22, 2013 at 10:22:31PM +0400, Sergei Shtylyov wrote: > >@@ -37,6 +37,7 @@ > > > > #include <linux/clk.h> > > #include <linux/libata.h> > >+#include <linux/phy/phy.h> > > struct phy; > > should suffice. Unless it's actually likely to cause inclusion loop, I think it's better to include the header than adding explicit declarations. Thanks.
Hello,
On Sun, Sep 22, 2013 at 10:24:36PM +0400, Sergei Shtylyov wrote:
> Not sure why you asked -- I'm not using this driver, neither I'm
Well, you have better grip of what's going on in the embedded world
than me. I'm mostly curious whether adding dependency on PHY is okay.
Thanks.
Hi Tejun, On 09/23/2013 12:51 AM, Tejun Heo wrote: > Hello, > > On Sun, Sep 22, 2013 at 10:24:36PM +0400, Sergei Shtylyov wrote: >> Not sure why you asked -- I'm not using this driver, neither I'm > > Well, you have better grip of what's going on in the embedded world > than me. I'm mostly curious whether adding dependency on PHY is okay. > There is no hard dependency on presence of PHY. The driver will continue as usual if devm_phy_get() fails. I hope selecting GENERIC_PHY in Kconfig is not an issue. cheers, -roger
Hi, On 09/22/2013 09:22 PM, Sergei Shtylyov wrote: > Hello. > > On 09/19/2013 05:05 PM, Roger Quadros wrote: > >> From: Balaji T K <balajitk@ti.com> > >> Some platforms have a PHY hooked up to the >> SATA controller. The PHY needs to be initialized >> and powered up for SATA to work. We do that >> using the PHY framework. > >> [Roger Q] Cleaned up. > >> CC: Tejun Heo <tj@kernel.org> >> Signed-off-by: Balaji T K <balajitk@ti.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> > [...] > >> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h >> index 1145637..94484cb 100644 >> --- a/drivers/ata/ahci.h >> +++ b/drivers/ata/ahci.h >> @@ -37,6 +37,7 @@ >> >> #include <linux/clk.h> >> #include <linux/libata.h> >> +#include <linux/phy/phy.h> > > struct phy; > > should suffice. > >> @@ -322,6 +323,7 @@ struct ahci_host_priv { >> u32 em_buf_sz; /* EM buffer size in byte */ >> u32 em_msg_type; /* EM message type */ >> struct clk *clk; /* Only for platforms supporting clk */ >> + struct phy *phy; /* If platforms use phy */ >> void *plat_data; /* Other platform data */ >> }; >> >> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c >> index 2daaee0..f812ffa 100644 >> --- a/drivers/ata/ahci_platform.c >> +++ b/drivers/ata/ahci_platform.c >> @@ -23,6 +23,7 @@ >> #include <linux/platform_device.h> >> #include <linux/libata.h> >> #include <linux/ahci_platform.h> >> +#include <linux/phy/phy.h> > > Why include it from both ahci.h and here? > OK. will move it to just .c file. >> #include "ahci.h" >> >> static void ahci_host_stop(struct ata_host *host); >> @@ -141,16 +142,32 @@ static int ahci_probe(struct platform_device *pdev) >> } >> } >> >> + hpriv->phy = devm_phy_get(dev, "sata-phy"); >> + if (IS_ERR(hpriv->phy)) { >> + dev_err(dev, "can't get phy\n"); > > Don't think it's a good idea to complain about missing PHY when the driver doesn't even use it. OK. will change it to dev_dbg() instead. > >> + /* return only if -EPROBE_DEFER */ >> + if (PTR_ERR(hpriv->phy) == -EPROBE_DEFER) { >> + rc = -EPROBE_DEFER; >> + goto disable_unprepare_clk; >> + } >> + } >> + >> /* >> * Some platforms might need to prepare for mmio region access, >> * which could be done in the following init call. So, the mmio >> * region shouldn't be accessed before init (if provided) has >> * returned successfully. >> */ >> + >> + if (!(IS_ERR(hpriv->phy))) { > > () not needed around IS_ERR() invocation. OK. > >> + phy_init(hpriv->phy); >> + phy_power_on(hpriv->phy); >> + } >> + > > I think this is misplaced, i.e. it should precede the comment. > OK. >> if (pdata && pdata->init) { >> rc = pdata->init(dev, hpriv->mmio); >> if (rc) >> - goto disable_unprepare_clk; >> + goto disable_phy; >> } >> >> ahci_save_initial_config(dev, hpriv, > [...] >> @@ -328,6 +356,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume); >> static const struct of_device_id ahci_of_match[] = { >> { .compatible = "snps,spear-ahci", }, >> { .compatible = "snps,exynos5440-ahci", }, >> + { .compatible = "snps,dwc-ahci", }, > > Looks like the binding documentation is incomplete -- it doesn't list "snps,exynos5440-ahci"... OK, I'll update it. Thanks for review. cheers, -roger
Hello. On 23-09-2013 1:51, Tejun Heo wrote: >> Not sure why you asked -- I'm not using this driver, neither I'm > Well, you have better grip of what's going on in the embedded world > than me. I'm mostly curious whether adding dependency on PHY is okay. This driver already supports optional clock, the optional PHY support seems analogous. > Thanks. WBR, Sergei
Hello. On 23-09-2013 11:37, Roger Quadros wrote: >>> Not sure why you asked -- I'm not using this driver, neither I'm >> Well, you have better grip of what's going on in the embedded world >> than me. I'm mostly curious whether adding dependency on PHY is okay. > There is no hard dependency on presence of PHY. The driver will continue > as usual if devm_phy_get() fails. > I hope selecting GENERIC_PHY in Kconfig is not an issue. Selecting in the AHCI_PLATFORM section? It seems I have overlooked it. No, I don't think it's a good idea. The generic PHY functions seem to be stubbed when GENERIC_PHY=n. > cheers, > -roger WBR, Sergei
On 09/23/2013 03:59 PM, Sergei Shtylyov wrote: > Hello. > > On 23-09-2013 11:37, Roger Quadros wrote: > >>>> Not sure why you asked -- I'm not using this driver, neither I'm > >>> Well, you have better grip of what's going on in the embedded world >>> than me. I'm mostly curious whether adding dependency on PHY is okay. > >> There is no hard dependency on presence of PHY. The driver will continue >> as usual if devm_phy_get() fails. >> I hope selecting GENERIC_PHY in Kconfig is not an issue. > > Selecting in the AHCI_PLATFORM section? It seems I have overlooked it. No, I don't think it's a good idea. The generic PHY functions seem to be stubbed when GENERIC_PHY=n. > OK I will remove the select then. cheers, -roger
Hello. On 09/23/2013 01:48 AM, Tejun Heo wrote: >>> @@ -37,6 +37,7 @@ >>> #include <linux/clk.h> >>> #include <linux/libata.h> >>> +#include <linux/phy/phy.h> >> struct phy; >> should suffice. > Unless it's actually likely to cause inclusion loop, I think it's > better to include the header than adding explicit declarations. Apparently, tastes differ here. E.g. Greg KH would have also told Roger to use forward declaration in such case. :-) > Thanks. WBR, Sergei
On Mon, Sep 23, 2013 at 06:10:30PM +0400, Sergei Shtylyov wrote: > >Unless it's actually likely to cause inclusion loop, I think it's > >better to include the header than adding explicit declarations. > > Apparently, tastes differ here. E.g. Greg KH would have also told > Roger to use forward declaration in such case. :-) Yes, it's a matter of taste, but, of course mine is better than Greg's. :P
Hi, On Monday, September 23, 2013 04:53:52 PM Sergei Shtylyov wrote: > Hello. > > On 23-09-2013 1:51, Tejun Heo wrote: > > >> Not sure why you asked -- I'm not using this driver, neither I'm > > > Well, you have better grip of what's going on in the embedded world > > than me. I'm mostly curious whether adding dependency on PHY is okay. > > This driver already supports optional clock, the optional PHY support > seems analogous. Right, this reminds me that PHY support should probably also be added to ahci_suspend() and ahci_resume(). Also please note the generic PHY framework is not yet merged in Linus' tree so this patch should probably be merged only after the generic PHY framework is in. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Hi, On 09/23/2013 04:59 PM, Roger Quadros wrote: > On 09/23/2013 03:59 PM, Sergei Shtylyov wrote: >> Hello. >> >> On 23-09-2013 11:37, Roger Quadros wrote: >> >>>>> Not sure why you asked -- I'm not using this driver, neither I'm >> >>>> Well, you have better grip of what's going on in the embedded world >>>> than me. I'm mostly curious whether adding dependency on PHY is okay. >> >>> There is no hard dependency on presence of PHY. The driver will continue >>> as usual if devm_phy_get() fails. >>> I hope selecting GENERIC_PHY in Kconfig is not an issue. >> >> Selecting in the AHCI_PLATFORM section? It seems I have overlooked it. No, I don't think it's a good idea. The generic PHY functions seem to be stubbed when GENERIC_PHY=n. >> > OK I will remove the select then. If I remove the select then build fails like so if we set CONFIG_GENERIC_PHY to 'm' and CONFIG_SATA_AHCI_PLATFORM to 'y' drivers/built-in.o: In function `ahci_platform_enable_resources': (.text+0x162647): undefined reference to `phy_init' drivers/built-in.o: In function `ahci_platform_enable_resources': (.text+0x16267c): undefined reference to `phy_power_on' drivers/built-in.o: In function `ahci_platform_enable_resources': (.text+0x162694): undefined reference to `phy_exit' drivers/built-in.o: In function `ahci_platform_disable_resources': (.text+0x1626af): undefined reference to `phy_power_off' drivers/built-in.o: In function `ahci_platform_disable_resources': (.text+0x1626b7): undefined reference to `phy_exit' drivers/built-in.o: In function `ahci_platform_get_resources': (.text+0x162768): undefined reference to `devm_phy_get' make: *** [vmlinux] Error 1 This means we need to make CONFIG_SATA_AHCI_PLATFORM depend on CONFIG_GENERIC_PHY or select it. OR Generic PHY layer must be fixed so that the API's are always built in. What is the better option? I believe making the PHY API's always built in is the better option. cheers, -roger
On Friday 07 February 2014 12:33:38 Roger Quadros wrote: > > This means we need to make CONFIG_SATA_AHCI_PLATFORM depend on CONFIG_GENERIC_PHY or > select it. > > OR > > Generic PHY layer must be fixed so that the API's are always built in. > > What is the better option? I believe making the PHY API's always built in is the better option. > CONFIG_SATA_AHCI_PLATFORM should do "depends on CONFIG_GENERIC_PHY || !CONFIG_GENERIC_PHY" which is the Kconfig way of saying that if CONFIG_GENERIC_PHY is a module, CONFIG_SATA_AHCI_PLATFORM needs to be a module as well. Arnd
On 02/07/2014 12:39 PM, Arnd Bergmann wrote: > On Friday 07 February 2014 12:33:38 Roger Quadros wrote: >> >> This means we need to make CONFIG_SATA_AHCI_PLATFORM depend on CONFIG_GENERIC_PHY or >> select it. >> >> OR >> >> Generic PHY layer must be fixed so that the API's are always built in. >> >> What is the better option? I believe making the PHY API's always built in is the better option. >> > > CONFIG_SATA_AHCI_PLATFORM should do > > "depends on CONFIG_GENERIC_PHY || !CONFIG_GENERIC_PHY" > > which is the Kconfig way of saying that if CONFIG_GENERIC_PHY is a module, > CONFIG_SATA_AHCI_PLATFORM needs to be a module as well. > Ah, that's neat. Thanks :). cheers, -roger
diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt index 89de156..c6c549a 100644 --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt @@ -4,7 +4,7 @@ SATA nodes are defined to describe on-chip Serial ATA controllers. Each SATA controller should have its own node. Required properties: -- compatible : compatible list, contains "snps,spear-ahci" +- compatible : compatible list, contains "snps,spear-ahci" or "snps,dwc-ahci" - interrupts : <interrupt mapping for SATA IRQ> - reg : <registers mapping> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 4e73772..a53ef27 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -91,6 +91,7 @@ config SATA_AHCI config SATA_AHCI_PLATFORM tristate "Platform AHCI SATA support" + select GENERIC_PHY help This option enables support for Platform AHCI Serial ATA controllers. diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index 1145637..94484cb 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -37,6 +37,7 @@ #include <linux/clk.h> #include <linux/libata.h> +#include <linux/phy/phy.h> /* Enclosure Management Control */ #define EM_CTRL_MSG_TYPE 0x000f0000 @@ -322,6 +323,7 @@ struct ahci_host_priv { u32 em_buf_sz; /* EM buffer size in byte */ u32 em_msg_type; /* EM message type */ struct clk *clk; /* Only for platforms supporting clk */ + struct phy *phy; /* If platforms use phy */ void *plat_data; /* Other platform data */ }; diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c index 2daaee0..f812ffa 100644 --- a/drivers/ata/ahci_platform.c +++ b/drivers/ata/ahci_platform.c @@ -23,6 +23,7 @@ #include <linux/platform_device.h> #include <linux/libata.h> #include <linux/ahci_platform.h> +#include <linux/phy/phy.h> #include "ahci.h" static void ahci_host_stop(struct ata_host *host); @@ -141,16 +142,32 @@ static int ahci_probe(struct platform_device *pdev) } } + hpriv->phy = devm_phy_get(dev, "sata-phy"); + if (IS_ERR(hpriv->phy)) { + dev_err(dev, "can't get phy\n"); + /* return only if -EPROBE_DEFER */ + if (PTR_ERR(hpriv->phy) == -EPROBE_DEFER) { + rc = -EPROBE_DEFER; + goto disable_unprepare_clk; + } + } + /* * Some platforms might need to prepare for mmio region access, * which could be done in the following init call. So, the mmio * region shouldn't be accessed before init (if provided) has * returned successfully. */ + + if (!(IS_ERR(hpriv->phy))) { + phy_init(hpriv->phy); + phy_power_on(hpriv->phy); + } + if (pdata && pdata->init) { rc = pdata->init(dev, hpriv->mmio); if (rc) - goto disable_unprepare_clk; + goto disable_phy; } ahci_save_initial_config(dev, hpriv, @@ -220,6 +237,12 @@ static int ahci_probe(struct platform_device *pdev) pdata_exit: if (pdata && pdata->exit) pdata->exit(dev); +disable_phy: + if (!IS_ERR(hpriv->phy)) { + phy_power_off(hpriv->phy); + phy_exit(hpriv->phy); + } + disable_unprepare_clk: if (!IS_ERR(hpriv->clk)) clk_disable_unprepare(hpriv->clk); @@ -238,6 +261,11 @@ static void ahci_host_stop(struct ata_host *host) if (pdata && pdata->exit) pdata->exit(dev); + if (!IS_ERR(hpriv->phy)) { + phy_power_off(hpriv->phy); + phy_exit(hpriv->phy); + } + if (!IS_ERR(hpriv->clk)) { clk_disable_unprepare(hpriv->clk); clk_put(hpriv->clk); @@ -328,6 +356,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume); static const struct of_device_id ahci_of_match[] = { { .compatible = "snps,spear-ahci", }, { .compatible = "snps,exynos5440-ahci", }, + { .compatible = "snps,dwc-ahci", }, {}, }; MODULE_DEVICE_TABLE(of, ahci_of_match);