[net-next,v3,11/11] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration

Message ID 00989856964175eafbe1435a70862c2ac66cffc0.1536912834.git-series.quentin.schulz@bootlin.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • mscc: ocelot: add support for SerDes muxing configuration
Related show

Commit Message

Quentin Schulz Sept. 14, 2018, 8:16 a.m.
Previously, the SerDes muxing was hardcoded to a given mode in the MAC
controller driver. Now, the SerDes muxing is configured within the
Device Tree and is enforced in the MAC controller driver so we can have
a lot of different SerDes configurations.

Make use of the SerDes PHYs in the MAC controller to set up the SerDes
according to the SerDes<->switch port mapping and the communication mode
with the Ethernet PHY.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/ethernet/mscc/Kconfig        |  2 +-
 drivers/net/ethernet/mscc/ocelot.c       | 16 +++++++-
 drivers/net/ethernet/mscc/ocelot.h       |  5 +++-
 drivers/net/ethernet/mscc/ocelot_board.c | 50 ++++++++++++++++++++-----
 4 files changed, 62 insertions(+), 11 deletions(-)

Comments

Florian Fainelli Sept. 15, 2018, 9:25 p.m. | #1
On 09/14/18 01:16, Quentin Schulz wrote:
> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> controller driver. Now, the SerDes muxing is configured within the
> Device Tree and is enforced in the MAC controller driver so we can have
> a lot of different SerDes configurations.
> 
> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> according to the SerDes<->switch port mapping and the communication mode
> with the Ethernet PHY.

This looks good, just a few comments below:

[snip]

> +		err = of_get_phy_mode(portnp);
> +		if (err < 0)
> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> +		else
> +			ocelot->ports[port]->phy_mode = err;
> +
> +		switch (ocelot->ports[port]->phy_mode) {
> +		case PHY_INTERFACE_MODE_NA:
> +			continue;

Would not you want to issue a message indicating that the Device Tree
must be updated here? AFAICT with your patch series, this should no
longer be a condition that you will hit unless you kept the old DTB
around, right?

> +		case PHY_INTERFACE_MODE_SGMII:
> +			phy_mode = PHY_MODE_SGMII;
> +			break;
> +		case PHY_INTERFACE_MODE_QSGMII:
> +			phy_mode = PHY_MODE_QSGMII;
> +			break;
> +		default:
> +			dev_err(ocelot->dev,
> +				"invalid phy mode for port%d, (Q)SGMII only\n",
> +				port);
> +			return -EINVAL;
> +		}
> +
> +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> +		if (IS_ERR(serdes)) {
> +			err = PTR_ERR(serdes);
> +			if (err == -EPROBE_DEFER) {

This can be simplified into:

			if (err == -EPROBE_DEFER)
				dev_dbg();
			else
				dev_err();
			goto err_probe_ports;

> +				dev_dbg(ocelot->dev, "deferring probe\n");
> +				goto err_probe_ports;
> +			}
> +
> +			dev_err(ocelot->dev, "missing SerDes phys for port%d\n",
> +				port);
>  			goto err_probe_ports;
>  		}
> +
> +		ocelot->ports[port]->serdes = serdes;
>  	}
>  
>  	register_netdevice_notifier(&ocelot_netdevice_nb);
>
Quentin Schulz Oct. 1, 2018, 9:42 a.m. | #2
Hi Florian,

On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
> 
> 
> On 09/14/18 01:16, Quentin Schulz wrote:
> > Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> > controller driver. Now, the SerDes muxing is configured within the
> > Device Tree and is enforced in the MAC controller driver so we can have
> > a lot of different SerDes configurations.
> > 
> > Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> > according to the SerDes<->switch port mapping and the communication mode
> > with the Ethernet PHY.
> 
> This looks good, just a few comments below:
> 
> [snip]
> 
> > +		err = of_get_phy_mode(portnp);
> > +		if (err < 0)
> > +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> > +		else
> > +			ocelot->ports[port]->phy_mode = err;
> > +
> > +		switch (ocelot->ports[port]->phy_mode) {
> > +		case PHY_INTERFACE_MODE_NA:
> > +			continue;
> 
> Would not you want to issue a message indicating that the Device Tree
> must be updated here? AFAICT with your patch series, this should no
> longer be a condition that you will hit unless you kept the old DTB
> around, right?
> 

It'll occur for internal PHYs. On the PCB123[1], there are four of them,
so we need to be able to give no mode in the DT for those. For the
upcoming PCB120, there'll be 4 external PHYs that require a mode in the
DT and 4 internal PHYs that do not require any mode. I could put a debug
message that says this or that PHY is configured as an internal PHY but
I wouldn't put a message that is printed with the default log level.

So I think we should keep it, shouldn't we?

[1] https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/mscc/ocelot_pcb123.dts

> > +		case PHY_INTERFACE_MODE_SGMII:
> > +			phy_mode = PHY_MODE_SGMII;
> > +			break;
> > +		case PHY_INTERFACE_MODE_QSGMII:
> > +			phy_mode = PHY_MODE_QSGMII;
> > +			break;
> > +		default:
> > +			dev_err(ocelot->dev,
> > +				"invalid phy mode for port%d, (Q)SGMII only\n",
> > +				port);
> > +			return -EINVAL;
> > +		}
> > +
> > +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> > +		if (IS_ERR(serdes)) {
> > +			err = PTR_ERR(serdes);
> > +			if (err == -EPROBE_DEFER) {
> 
> This can be simplified into:
> 
> 			if (err == -EPROBE_DEFER)
> 				dev_dbg();
> 			else
> 				dev_err();
> 			goto err_probe_ports;
> 

Indeed, good catch.

Thanks,
Quentin
Florian Fainelli Oct. 1, 2018, 4:29 p.m. | #3
On 10/01/2018 02:42 AM, Quentin Schulz wrote:
> Hi Florian,
> 
> On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
>>
>>
>> On 09/14/18 01:16, Quentin Schulz wrote:
>>> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
>>> controller driver. Now, the SerDes muxing is configured within the
>>> Device Tree and is enforced in the MAC controller driver so we can have
>>> a lot of different SerDes configurations.
>>>
>>> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
>>> according to the SerDes<->switch port mapping and the communication mode
>>> with the Ethernet PHY.
>>
>> This looks good, just a few comments below:
>>
>> [snip]
>>
>>> +		err = of_get_phy_mode(portnp);
>>> +		if (err < 0)
>>> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
>>> +		else
>>> +			ocelot->ports[port]->phy_mode = err;
>>> +
>>> +		switch (ocelot->ports[port]->phy_mode) {
>>> +		case PHY_INTERFACE_MODE_NA:
>>> +			continue;
>>
>> Would not you want to issue a message indicating that the Device Tree
>> must be updated here? AFAICT with your patch series, this should no
>> longer be a condition that you will hit unless you kept the old DTB
>> around, right?
>>
> 
> It'll occur for internal PHYs. On the PCB123[1], there are four of them,
> so we need to be able to give no mode in the DT for those. For the
> upcoming PCB120, there'll be 4 external PHYs that require a mode in the
> DT and 4 internal PHYs that do not require any mode. I could put a debug
> message that says this or that PHY is configured as an internal PHY but
> I wouldn't put a message that is printed with the default log level.
> 
> So I think we should keep it, shouldn't we?

Internal PHYs either use a standard connection internally (e.g: GMII) or
they are using a proprietary connection interface, in which case
phy-mode = "internal" is what we defined to represent those.

> 
> [1] https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> 
>>> +		case PHY_INTERFACE_MODE_SGMII:
>>> +			phy_mode = PHY_MODE_SGMII;
>>> +			break;
>>> +		case PHY_INTERFACE_MODE_QSGMII:
>>> +			phy_mode = PHY_MODE_QSGMII;
>>> +			break;
>>> +		default:
>>> +			dev_err(ocelot->dev,
>>> +				"invalid phy mode for port%d, (Q)SGMII only\n",
>>> +				port);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
>>> +		if (IS_ERR(serdes)) {
>>> +			err = PTR_ERR(serdes);
>>> +			if (err == -EPROBE_DEFER) {
>>
>> This can be simplified into:
>>
>> 			if (err == -EPROBE_DEFER)
>> 				dev_dbg();
>> 			else
>> 				dev_err();
>> 			goto err_probe_ports;
>>
> 
> Indeed, good catch.
> 
> Thanks,
> Quentin
>
Quentin Schulz Oct. 4, 2018, 12:20 p.m. | #4
Hi Florian,

On Mon, Oct 01, 2018 at 09:29:07AM -0700, Florian Fainelli wrote:
> On 10/01/2018 02:42 AM, Quentin Schulz wrote:
> > Hi Florian,
> > 
> > On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
> >>
> >>
> >> On 09/14/18 01:16, Quentin Schulz wrote:
> >>> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> >>> controller driver. Now, the SerDes muxing is configured within the
> >>> Device Tree and is enforced in the MAC controller driver so we can have
> >>> a lot of different SerDes configurations.
> >>>
> >>> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> >>> according to the SerDes<->switch port mapping and the communication mode
> >>> with the Ethernet PHY.
> >>
> >> This looks good, just a few comments below:
> >>
> >> [snip]
> >>
> >>> +		err = of_get_phy_mode(portnp);
> >>> +		if (err < 0)
> >>> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> >>> +		else
> >>> +			ocelot->ports[port]->phy_mode = err;
> >>> +
> >>> +		switch (ocelot->ports[port]->phy_mode) {
> >>> +		case PHY_INTERFACE_MODE_NA:
> >>> +			continue;
> >>
> >> Would not you want to issue a message indicating that the Device Tree
> >> must be updated here? AFAICT with your patch series, this should no
> >> longer be a condition that you will hit unless you kept the old DTB
> >> around, right?
> >>
> > 
> > It'll occur for internal PHYs. On the PCB123[1], there are four of them,
> > so we need to be able to give no mode in the DT for those. For the
> > upcoming PCB120, there'll be 4 external PHYs that require a mode in the
> > DT and 4 internal PHYs that do not require any mode. I could put a debug
> > message that says this or that PHY is configured as an internal PHY but
> > I wouldn't put a message that is printed with the default log level.
> > 
> > So I think we should keep it, shouldn't we?
> 
> Internal PHYs either use a standard connection internally (e.g: GMII) or
> they are using a proprietary connection interface, in which case
> phy-mode = "internal" is what we defined to represent those.
> 

Just to let you know that I'll send a new version in a few minutes that
does not contain the requested change. I don't have the information yet,
I know it's MII compatible but nothing more.
I haven't forgotten (yet; so don't hesitate to tell me if I do) your
suggestion.

Two thoughts:
1) Doing what you suggested is rather straightforward once I have the
information so it does not impact the actual overall behaviour of the
driver (reviewable as is),

2) The current behaviour aligns with the behaviour induced by the code
snippet above, so we don't break anything or introduce any change in
behaviour. Once I have an answer, I could always send a small patch for
this if the driver gets merged before, which would also change the DT
(to add the phy-mode property).

Thanks,
Quentin

Patch

diff --git a/drivers/net/ethernet/mscc/Kconfig b/drivers/net/ethernet/mscc/Kconfig
index 36c8462..bcec058 100644
--- a/drivers/net/ethernet/mscc/Kconfig
+++ b/drivers/net/ethernet/mscc/Kconfig
@@ -23,6 +23,8 @@  config MSCC_OCELOT_SWITCH
 config MSCC_OCELOT_SWITCH_OCELOT
 	tristate "Ocelot switch driver on Ocelot"
 	depends on MSCC_OCELOT_SWITCH
+	depends on GENERIC_PHY
+	depends on OF_NET
 	help
 	  This driver supports the Ocelot network switch device as present on
 	  the Ocelot SoCs.
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 1a4f2bb..8f11fdb 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -472,6 +472,7 @@  static int ocelot_port_open(struct net_device *dev)
 {
 	struct ocelot_port *port = netdev_priv(dev);
 	struct ocelot *ocelot = port->ocelot;
+	enum phy_mode phy_mode;
 	int err;
 
 	/* Enable receiving frames on the port, and activate auto-learning of
@@ -482,8 +483,21 @@  static int ocelot_port_open(struct net_device *dev)
 			 ANA_PORT_PORT_CFG_PORTID_VAL(port->chip_port),
 			 ANA_PORT_PORT_CFG, port->chip_port);
 
+	if (port->serdes) {
+		if (port->phy_mode == PHY_INTERFACE_MODE_SGMII)
+			phy_mode = PHY_MODE_SGMII;
+		else
+			phy_mode = PHY_MODE_QSGMII;
+
+		err = phy_set_mode(port->serdes, phy_mode);
+		if (err) {
+			netdev_err(dev, "Could not set mode of SerDes\n");
+			return err;
+		}
+	}
+
 	err = phy_connect_direct(dev, port->phy, &ocelot_port_adjust_link,
-				 PHY_INTERFACE_MODE_NA);
+				 port->phy_mode);
 	if (err) {
 		netdev_err(dev, "Could not attach to PHY\n");
 		return err;
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 3720e51..62c7c8e 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -11,6 +11,8 @@ 
 #include <linux/bitops.h>
 #include <linux/etherdevice.h>
 #include <linux/if_vlan.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
@@ -453,6 +455,9 @@  struct ocelot_port {
 	u8 vlan_aware;
 
 	u64 *stats;
+
+	phy_interface_t phy_mode;
+	struct phy *serdes;
 };
 
 u32 __ocelot_read_ix(struct ocelot *ocelot, u32 reg, u32 offset);
diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
index b7d755b..a26a06e 100644
--- a/drivers/net/ethernet/mscc/ocelot_board.c
+++ b/drivers/net/ethernet/mscc/ocelot_board.c
@@ -6,6 +6,7 @@ 
  */
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/of_net.h>
 #include <linux/netdevice.h>
 #include <linux/of_mdio.h>
 #include <linux/of_platform.h>
@@ -247,18 +248,12 @@  static int mscc_ocelot_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&ocelot->multicast);
 	ocelot_init(ocelot);
 
-	ocelot_rmw(ocelot, HSIO_HW_CFG_DEV1G_4_MODE |
-		     HSIO_HW_CFG_DEV1G_6_MODE |
-		     HSIO_HW_CFG_DEV1G_9_MODE,
-		     HSIO_HW_CFG_DEV1G_4_MODE |
-		     HSIO_HW_CFG_DEV1G_6_MODE |
-		     HSIO_HW_CFG_DEV1G_9_MODE,
-		     HSIO_HW_CFG);
-
 	for_each_available_child_of_node(ports, portnp) {
 		struct device_node *phy_node;
 		struct phy_device *phy;
 		struct resource *res;
+		struct phy *serdes;
+		enum phy_mode phy_mode;
 		void __iomem *regs;
 		char res_name[8];
 		u32 port;
@@ -283,10 +278,45 @@  static int mscc_ocelot_probe(struct platform_device *pdev)
 			continue;
 
 		err = ocelot_probe_port(ocelot, port, regs, phy);
-		if (err) {
-			dev_err(&pdev->dev, "failed to probe ports\n");
+		if (err)
+			return err;
+
+		err = of_get_phy_mode(portnp);
+		if (err < 0)
+			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
+		else
+			ocelot->ports[port]->phy_mode = err;
+
+		switch (ocelot->ports[port]->phy_mode) {
+		case PHY_INTERFACE_MODE_NA:
+			continue;
+		case PHY_INTERFACE_MODE_SGMII:
+			phy_mode = PHY_MODE_SGMII;
+			break;
+		case PHY_INTERFACE_MODE_QSGMII:
+			phy_mode = PHY_MODE_QSGMII;
+			break;
+		default:
+			dev_err(ocelot->dev,
+				"invalid phy mode for port%d, (Q)SGMII only\n",
+				port);
+			return -EINVAL;
+		}
+
+		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
+		if (IS_ERR(serdes)) {
+			err = PTR_ERR(serdes);
+			if (err == -EPROBE_DEFER) {
+				dev_dbg(ocelot->dev, "deferring probe\n");
+				goto err_probe_ports;
+			}
+
+			dev_err(ocelot->dev, "missing SerDes phys for port%d\n",
+				port);
 			goto err_probe_ports;
 		}
+
+		ocelot->ports[port]->serdes = serdes;
 	}
 
 	register_netdevice_notifier(&ocelot_netdevice_nb);