| Message ID | 20181212102142.16053-4-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Bring suspend to RAM support to PCIe Aardvark driver | expand |
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;
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
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
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
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
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
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
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
> [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
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
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
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 --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;
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(+)