diff mbox series

[v2,03/12] PCI: aardvark: Add PHY support

Message ID 20181212102142.16053-4-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series Bring suspend to RAM support to PCIe Aardvark driver | expand

Commit Message

Miquel Raynal Dec. 12, 2018, 10:21 a.m. UTC
The IP needs its PHY to be properly configured to work. While the PHY
is usually already configured by the bootloader, we will need this
feature when adding S2RAM support. Take care of registering and
configuring the PHY from the driver itself.

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 62 +++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Comments

Marek Behún Dec. 14, 2018, 12:47 a.m. UTC | #1
Hi Miquel,
are there already patches for the A37xx comphy driver?

On Wed, 12 Dec 2018 11:21:33 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> The IP needs its PHY to be properly configured to work. While the PHY
> is usually already configured by the bootloader, we will need this
> feature when adding S2RAM support. Take care of registering and
> configuring the PHY from the driver itself.
> 
> Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/pci/controller/pci-aardvark.c | 62
> +++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c
> b/drivers/pci/controller/pci-aardvark.c index
> 1d31d74ddab7..da695572a2ed 100644 ---
> a/drivers/pci/controller/pci-aardvark.c +++
> b/drivers/pci/controller/pci-aardvark.c @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
>  #include <linux/of_address.h>
>  #include <linux/of_gpio.h>
>  #include <linux/of_pci.h>
> @@ -204,6 +205,7 @@ struct advk_pcie {
>  	int root_bus_nr;
>  	struct pci_bridge_emul bridge;
>  	struct gpio_desc *reset_gpio;
> +	struct phy *phy;
>  };
>  
>  static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64
> reg) @@ -1025,6 +1027,62 @@ static int
> advk_pcie_setup_reset_gpio(struct advk_pcie *pcie) return 0;
>  }
>  
> +static void advk_pcie_disable_phy(struct advk_pcie *pcie)
> +{
> +	phy_power_off(pcie->phy);
> +	phy_exit(pcie->phy);
> +}
> +
> +static int advk_pcie_enable_phy(struct advk_pcie *pcie)
> +{
> +	int ret;
> +
> +	if (!pcie->phy)
> +		return 0;
> +
> +	ret = phy_init(pcie->phy);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
> +	if (ret) {
> +		phy_exit(pcie->phy);
> +		return ret;
> +	}
> +
> +	ret = phy_power_on(pcie->phy);
> +	if (ret) {
> +		phy_exit(pcie->phy);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int advk_pcie_setup_phy(struct advk_pcie *pcie)
> +{
> +	struct device *dev = &pcie->pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret = 0;
> +
> +	pcie->phy = devm_of_phy_get(dev, node, NULL);
> +	if (IS_ERR(pcie->phy) && (PTR_ERR(pcie->phy) ==
> -EPROBE_DEFER))
> +		return PTR_ERR(pcie->phy);
> +
> +	/* Old bindings miss the PHY handle */
> +	if (IS_ERR(pcie->phy)) {
> +		dev_warn(dev, "PHY unavailable (%ld)\n",
> PTR_ERR(pcie->phy));
> +		pcie->phy = NULL;
> +		return 0;
> +	}
> +
> +	ret = advk_pcie_enable_phy(pcie);
> +	if (ret)
> +		dev_err(dev, "Failed to initialize PHY (%d)\n", ret);
> +
> +	return ret;
> +}
> +
>  static int advk_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1060,6 +1118,10 @@ static int advk_pcie_probe(struct
> platform_device *pdev) return ret;
>  	}
>  
> +	ret = advk_pcie_setup_phy(pcie);
> +	if (ret)
> +		return ret;
> +
>  	ret = advk_pcie_setup_reset_gpio(pcie);
>  	if (ret)
>  		return ret;
Marek Behún Dec. 14, 2018, 12:57 a.m. UTC | #2
On Fri, 14 Dec 2018 01:47:01 +0100
Marek Behun <marek.behun@nic.cz> wrote:

> Hi Miquel,
> are there already patches for the A37xx comphy driver?

Never mind, I found them. I shall test this on Turris Mox.
Is the comphy driver supposed to be able to change eth mode from
1000BASE-X to 2500BASE-X and back without reboot?

This would be cool for our SFP cage module of Turris Mox, just to add
support for comphy into mvneta.

Marek
Miquel Raynal Dec. 17, 2018, 4:07 p.m. UTC | #3
Hi Marek,

Marek Behun <marek.behun@nic.cz> wrote on Fri, 14 Dec 2018 01:57:12
+0100:

> On Fri, 14 Dec 2018 01:47:01 +0100
> Marek Behun <marek.behun@nic.cz> wrote:
> 
> > Hi Miquel,
> > are there already patches for the A37xx comphy driver?  

Please try the latest patches on top of phy -next (with PHY interface
mode), available there [1]. You can pick only the COMPHY patches, but
keep them over the phy -next branch.

Please note that for the SMC calls to succeed you must use a recent
ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
Marvell's misl-atf repository but it is not available anymore. If you
already have a clone, then you are good to go, otherwise it might be
great if someone from Marvell could share a public Github link?

> Never mind, I found them. I shall test this on Turris Mox.
> Is the comphy driver supposed to be able to change eth mode from
> 1000BASE-X to 2500BASE-X and back without reboot?

In theory yes, unfortunately I am working on an EspressoBin which uses
the SATA, PCIe and SATA configurations of the COMPHY, but not the
Ethernet ones (SGMII/1000BASE-X and HS_SGMII/2500BASE-X). So the
support is ready to be tested!

> 
> This would be cool for our SFP cage module of Turris Mox, just to add
> support for comphy into mvneta.
> 
> Marek

[1] https://github.com/miquelraynal/linux.git branch marvell/phy-next/pm


Thanks,
Miquèl
Baruch Siach Dec. 17, 2018, 6:27 p.m. UTC | #4
Hi Miquel,

Miquel Raynal writes:
> Marek Behun <marek.behun@nic.cz> wrote on Fri, 14 Dec 2018 01:57:12
> +0100:
>
>> On Fri, 14 Dec 2018 01:47:01 +0100
>> Marek Behun <marek.behun@nic.cz> wrote:
>> 
>> > are there already patches for the A37xx comphy driver?  
>
> Please try the latest patches on top of phy -next (with PHY interface
> mode), available there [1]. You can pick only the COMPHY patches, but
> keep them over the phy -next branch.
>
> Please note that for the SMC calls to succeed you must use a recent
> ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
> Marvell's misl-atf repository but it is not available anymore. If you
> already have a clone, then you are good to go, otherwise it might be
> great if someone from Marvell could share a public Github link?

The latest ATF from Marvell is at the 'atf-v1.5-armada-18.09' branch at

  https://github.com/MarvellEmbeddedProcessors/atf-marvell

baruch
Marek Behún Dec. 17, 2018, 9:34 p.m. UTC | #5
Miquel,

I tried the patches and they are working, with the exception of Compex
WLE900X card, but we know that this card is problematic.
I am interesting if there is a known way to turn of the comphy on
A3720, or at least change the SGMII mode from 1Gbps to 2.5Gbps and
back. Marvell documentation does not provide this information and the
code in ATF providing the SMC calls does not do this either.
Do you know how this can be achieved?
Thanks,

Marek

On Mon, 17 Dec 2018 17:07:24 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Marek,
> 
> Marek Behun <marek.behun@nic.cz> wrote on Fri, 14 Dec 2018 01:57:12
> +0100:
> 
> > On Fri, 14 Dec 2018 01:47:01 +0100
> > Marek Behun <marek.behun@nic.cz> wrote:
> >   
> > > Hi Miquel,
> > > are there already patches for the A37xx comphy driver?    
> 
> Please try the latest patches on top of phy -next (with PHY interface
> mode), available there [1]. You can pick only the COMPHY patches, but
> keep them over the phy -next branch.
> 
> Please note that for the SMC calls to succeed you must use a recent
> ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
> Marvell's misl-atf repository but it is not available anymore. If you
> already have a clone, then you are good to go, otherwise it might be
> great if someone from Marvell could share a public Github link?
> 
> > Never mind, I found them. I shall test this on Turris Mox.
> > Is the comphy driver supposed to be able to change eth mode from
> > 1000BASE-X to 2500BASE-X and back without reboot?  
> 
> In theory yes, unfortunately I am working on an EspressoBin which uses
> the SATA, PCIe and SATA configurations of the COMPHY, but not the
> Ethernet ones (SGMII/1000BASE-X and HS_SGMII/2500BASE-X). So the
> support is ready to be tested!
> 
> > 
> > This would be cool for our SFP cage module of Turris Mox, just to
> > add support for comphy into mvneta.
> > 
> > Marek  
> 
> [1] https://github.com/miquelraynal/linux.git branch
> marvell/phy-next/pm
> 
> 
> Thanks,
> Miquèl
Miquel Raynal Dec. 18, 2018, 8:12 a.m. UTC | #6
Hi Baruch,

Baruch Siach <baruch@tkos.co.il> wrote on Mon, 17 Dec 2018 20:27:42
+0200:

> Hi Miquel,
> 
> Miquel Raynal writes:
> > Marek Behun <marek.behun@nic.cz> wrote on Fri, 14 Dec 2018 01:57:12
> > +0100:
> >  
> >> On Fri, 14 Dec 2018 01:47:01 +0100
> >> Marek Behun <marek.behun@nic.cz> wrote:
> >>   
> >> > are there already patches for the A37xx comphy driver?    
> >
> > Please try the latest patches on top of phy -next (with PHY interface
> > mode), available there [1]. You can pick only the COMPHY patches, but
> > keep them over the phy -next branch.
> >
> > Please note that for the SMC calls to succeed you must use a recent
> > ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
> > Marvell's misl-atf repository but it is not available anymore. If you
> > already have a clone, then you are good to go, otherwise it might be
> > great if someone from Marvell could share a public Github link?  
> 
> The latest ATF from Marvell is at the 'atf-v1.5-armada-18.09' branch at
> 
>   https://github.com/MarvellEmbeddedProcessors/atf-marvell

Indeed, but I don't see A3700 COMPHY support there (only CP110's), I
think it will be merged into 18.12 release (will happen this month).


Thanks,
Miquèl
Miquel Raynal Dec. 18, 2018, 8:18 a.m. UTC | #7
Hi Marek,

Marek Behun <marek.behun@nic.cz> wrote on Mon, 17 Dec 2018 22:34:30
+0100:

> Miquel,
> 
> I tried the patches and they are working, with the exception of Compex
> WLE900X card, but we know that this card is problematic.

How did you test them? I am surprised that COMPHY calls worked for you
out of the box. I am not sure you have A3700 COMPHY support in your ATF
but it probably work because U-Boot is doing the initial configuration.

> I am interesting if there is a known way to turn of the comphy on
> A3720, or at least change the SGMII mode from 1Gbps to 2.5Gbps and
> back. Marvell documentation does not provide this information and the
> code in ATF providing the SMC calls does not do this either.
> Do you know how this can be achieved?

The driver already handles these calls (see [1]), making use of
them is just a matter of hacking into drivers (in your case, I
suppose it is mvneta) and add phy_set_mode()/phy_power_on() calls.

[1] https://github.com/miquelraynal/linux/blob/marvell/phy-next/pm/drivers/phy/marvell/phy-mvebu-a3700-comphy.c#L189


Thanks,
Miquèl
Miquel Raynal Dec. 18, 2018, 8:23 a.m. UTC | #8
Hello,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Tue, 18 Dec 2018
09:18:17 +0100:

> Hi Marek,
> 
> Marek Behun <marek.behun@nic.cz> wrote on Mon, 17 Dec 2018 22:34:30
> +0100:
> 
> > Miquel,
> > 
> > I tried the patches and they are working, with the exception of Compex
> > WLE900X card, but we know that this card is problematic.  
> 
> How did you test them? I am surprised that COMPHY calls worked for you
> out of the box. I am not sure you have A3700 COMPHY support in your ATF
> but it probably work because U-Boot is doing the initial configuration.

Let me correct myself: actually the feature is already mainline! See
[2]. It will be added to Marvell's BSP by the end of the year. So if
you are running a recent mainline ATF you are good to go!

> 
> > I am interesting if there is a known way to turn of the comphy on
> > A3720, or at least change the SGMII mode from 1Gbps to 2.5Gbps and
> > back. Marvell documentation does not provide this information and the
> > code in ATF providing the SMC calls does not do this either.
> > Do you know how this can be achieved?  
> 
> The driver already handles these calls (see [1]), making use of
> them is just a matter of hacking into drivers (in your case, I
> suppose it is mvneta) and add phy_set_mode()/phy_power_on() calls.
> 
> [1] https://github.com/miquelraynal/linux/blob/marvell/phy-next/pm/drivers/phy/marvell/phy-mvebu-a3700-comphy.c#L189
> 
> 
> Thanks,
> Miquèl

[2] https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c


Thanks,
Miquèl
Marek Behún Dec. 18, 2018, 1:09 p.m. UTC | #9
> [2]
> https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c

Yes, I used mainline atf (it did not work out of the box with 18.09
atf-marvell of course). But there is no _power_off function for SGMII,
nor a digital_reset function like in cp110 implementation.

Marek
Miquel Raynal Dec. 18, 2018, 1:41 p.m. UTC | #10
Hi Marek,

Marek Behun <marek.behun@nic.cz> wrote on Tue, 18 Dec 2018 14:09:20
+0100:

> > [2]
> > https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c  
> 
> Yes, I used mainline atf (it did not work out of the box with 18.09
> atf-marvell of course). But there is no _power_off function for SGMII,
> nor a digital_reset function like in cp110 implementation.

Indeed, but why would you need one? Just use the helpers from the core
and if there is no implementation, nothing should happen and the helper
should exit without error. Just call phy_set_mode()/phy_power_on() an
you should be good.


Thanks,
Miquèl
Marek Behún Dec. 19, 2018, 3:28 p.m. UTC | #11
Hi,

One thing for which I would like to be able to disable comphy is that
each consumes about 100mW of power. On Turris Mox we configure the
comphys to SGMII1, PCIe and USB3 modes. If there is no USB device
plugged, the USB3 phy can be disabled, and save 100mW of power. If the
PCIe extension module is not present, the PCIe can too be disabled, and
if there is no switch nor SFP module present, so can SGMII1.

The other reason is this: if the SGMII phy is set to 1G mode, and then
powered on second time in 2.5G mode, will it work? I would like to
patch mvneta driver to power on/off the comphy, if the device node is
present in device tree. But then the system can request such a change
(SGMII to 2500BASE-X or back).

Marek

On Tue, 18 Dec 2018 14:41:30 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Marek,
> 
> Marek Behun <marek.behun@nic.cz> wrote on Tue, 18 Dec 2018 14:09:20
> +0100:
> 
> > > [2]
> > > https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c    
> > 
> > Yes, I used mainline atf (it did not work out of the box with 18.09
> > atf-marvell of course). But there is no _power_off function for
> > SGMII, nor a digital_reset function like in cp110 implementation.  
> 
> Indeed, but why would you need one? Just use the helpers from the core
> and if there is no implementation, nothing should happen and the
> helper should exit without error. Just call
> phy_set_mode()/phy_power_on() an you should be good.
> 
> 
> Thanks,
> Miquèl
Miquel Raynal Dec. 19, 2018, 4:48 p.m. UTC | #12
Hi Marek,

Marek Behún <marek.behun@nic.cz> wrote on Wed, 19 Dec 2018 16:28:35
+0100:

> Hi,
> 
> One thing for which I would like to be able to disable comphy is that
> each consumes about 100mW of power. On Turris Mox we configure the
> comphys to SGMII1, PCIe and USB3 modes. If there is no USB device
> plugged, the USB3 phy can be disabled, and save 100mW of power. If the
> PCIe extension module is not present, the PCIe can too be disabled, and
> if there is no switch nor SFP module present, so can SGMII1.

Indeed not all PHY types implement ->power_off() (see in ATF code).
Better ask Marvell directly for that.

> 
> The other reason is this: if the SGMII phy is set to 1G mode, and then
> powered on second time in 2.5G mode, will it work? I would like to

This should work, yes.

> patch mvneta driver to power on/off the comphy, if the device node is
> present in device tree. But then the system can request such a change
> (SGMII to 2500BASE-X or back).
> 
> Marek
> 
> On Tue, 18 Dec 2018 14:41:30 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Marek,
> > 
> > Marek Behun <marek.behun@nic.cz> wrote on Tue, 18 Dec 2018 14:09:20
> > +0100:
> >   
> > > > [2]
> > > > https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c      
> > > 
> > > Yes, I used mainline atf (it did not work out of the box with 18.09
> > > atf-marvell of course). But there is no _power_off function for
> > > SGMII, nor a digital_reset function like in cp110 implementation.    
> > 
> > Indeed, but why would you need one? Just use the helpers from the core
> > and if there is no implementation, nothing should happen and the
> > helper should exit without error. Just call
> > phy_set_mode()/phy_power_on() an you should be good.
> > 
> > 
> > Thanks,
> > Miquèl  
> 

For the record, I found out what was wrong in my code, toggling the
reset lines do not produce random effects anymore.


Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 1d31d74ddab7..da695572a2ed 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -17,6 +17,7 @@ 
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/phy/phy.h>
 #include <linux/of_address.h>
 #include <linux/of_gpio.h>
 #include <linux/of_pci.h>
@@ -204,6 +205,7 @@  struct advk_pcie {
 	int root_bus_nr;
 	struct pci_bridge_emul bridge;
 	struct gpio_desc *reset_gpio;
+	struct phy *phy;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -1025,6 +1027,62 @@  static int advk_pcie_setup_reset_gpio(struct advk_pcie *pcie)
 	return 0;
 }
 
+static void advk_pcie_disable_phy(struct advk_pcie *pcie)
+{
+	phy_power_off(pcie->phy);
+	phy_exit(pcie->phy);
+}
+
+static int advk_pcie_enable_phy(struct advk_pcie *pcie)
+{
+	int ret;
+
+	if (!pcie->phy)
+		return 0;
+
+	ret = phy_init(pcie->phy);
+	if (ret)
+		return ret;
+
+	ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
+	if (ret) {
+		phy_exit(pcie->phy);
+		return ret;
+	}
+
+	ret = phy_power_on(pcie->phy);
+	if (ret) {
+		phy_exit(pcie->phy);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int advk_pcie_setup_phy(struct advk_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+	struct device_node *node = dev->of_node;
+	int ret = 0;
+
+	pcie->phy = devm_of_phy_get(dev, node, NULL);
+	if (IS_ERR(pcie->phy) && (PTR_ERR(pcie->phy) == -EPROBE_DEFER))
+		return PTR_ERR(pcie->phy);
+
+	/* Old bindings miss the PHY handle */
+	if (IS_ERR(pcie->phy)) {
+		dev_warn(dev, "PHY unavailable (%ld)\n", PTR_ERR(pcie->phy));
+		pcie->phy = NULL;
+		return 0;
+	}
+
+	ret = advk_pcie_enable_phy(pcie);
+	if (ret)
+		dev_err(dev, "Failed to initialize PHY (%d)\n", ret);
+
+	return ret;
+}
+
 static int advk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1060,6 +1118,10 @@  static int advk_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = advk_pcie_setup_phy(pcie);
+	if (ret)
+		return ret;
+
 	ret = advk_pcie_setup_reset_gpio(pcie);
 	if (ret)
 		return ret;