| Message ID | 20220930194923.954551-1-mmyangfl@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mv643xx_eth: support MII/GMII/RGMII modes | expand |
On Sat, Oct 01, 2022 at 03:49:23AM +0800, David Yang wrote: > On device reset all ports are automatically set to RGMII mode. MII > mode must be explicitly enabled. > > If SoC has two Ethernet controllers, by setting both of them into MII > mode, the first controller enters GMII mode, while the second > controller is effectively disabled. This requires configuring (and > maybe enabling) the second controller in the device tree, even though > it cannot be used. > > Signed-off-by: David Yang <mmyangfl@gmail.com> > --- > drivers/net/ethernet/marvell/mv643xx_eth.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c > index b6be0552a..e2216ce5e 100644 > --- a/drivers/net/ethernet/marvell/mv643xx_eth.c > +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c > @@ -108,6 +108,7 @@ static char mv643xx_eth_driver_version[] = "1.4"; > #define TXQ_COMMAND 0x0048 > #define TXQ_FIX_PRIO_CONF 0x004c > #define PORT_SERIAL_CONTROL1 0x004c > +#define RGMII_EN 0x00000008 > #define CLK125_BYPASS_EN 0x00000010 > #define TX_BW_RATE 0x0050 > #define TX_BW_MTU 0x0058 > @@ -1245,6 +1246,21 @@ static void mv643xx_eth_adjust_link(struct net_device *dev) > > out_write: > wrlp(mp, PORT_SERIAL_CONTROL, pscr); > + > + /* If two Ethernet controllers present in the SoC, MII modes follow the > + * following matrix: > + * > + * Port0 Mode Port1 Mode Port0 RGMII_EN Port1 RGMII_EN > + * RGMII RGMII 1 1 > + * RGMII MII/MMII 1 0 > + * MII/MMII RGMII 0 1 > + * GMII N/A 0 0 > + * > + * To enable GMII on Port 0, Port 1 must also disable RGMII_EN too. > + */ > + if (!phy_interface_is_rgmii(dev->phydev)) > + wrlp(mp, PORT_SERIAL_CONTROL1, > + rdlp(mp, PORT_SERIAL_CONTROL1) & ~RGMII_EN); I could be reading this wrong, but doesn't this break the third line: > + * MII/MMII RGMII 0 1 Port 1 probes first, phy_interface is rgmii, so nothing happens, port1 RGMII_EN is left true. Port 0 then probes, MII/MMII is not RGMII, so port1 RGMII_EN is cleared, breaking port1. I think you need to be more specific with the comparison. Andrew
Andrew Lunn <andrew@lunn.ch> 于2022年10月1日周六 04:28写道： > > On Sat, Oct 01, 2022 at 03:49:23AM +0800, David Yang wrote: > > On device reset all ports are automatically set to RGMII mode. MII > > mode must be explicitly enabled. > > > > If SoC has two Ethernet controllers, by setting both of them into MII > > mode, the first controller enters GMII mode, while the second > > controller is effectively disabled. This requires configuring (and > > maybe enabling) the second controller in the device tree, even though > > it cannot be used. > > > > Signed-off-by: David Yang <mmyangfl@gmail.com> > > --- > > drivers/net/ethernet/marvell/mv643xx_eth.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c > > index b6be0552a..e2216ce5e 100644 > > --- a/drivers/net/ethernet/marvell/mv643xx_eth.c > > +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c > > @@ -108,6 +108,7 @@ static char mv643xx_eth_driver_version[] = "1.4"; > > #define TXQ_COMMAND 0x0048 > > #define TXQ_FIX_PRIO_CONF 0x004c > > #define PORT_SERIAL_CONTROL1 0x004c > > +#define RGMII_EN 0x00000008 > > #define CLK125_BYPASS_EN 0x00000010 > > #define TX_BW_RATE 0x0050 > > #define TX_BW_MTU 0x0058 > > @@ -1245,6 +1246,21 @@ static void mv643xx_eth_adjust_link(struct net_device *dev) > > > > out_write: > > wrlp(mp, PORT_SERIAL_CONTROL, pscr); > > + > > + /* If two Ethernet controllers present in the SoC, MII modes follow the > > + * following matrix: > > + * > > + * Port0 Mode Port1 Mode Port0 RGMII_EN Port1 RGMII_EN > > + * RGMII RGMII 1 1 > > + * RGMII MII/MMII 1 0 > > + * MII/MMII RGMII 0 1 > > + * GMII N/A 0 0 > > + * > > + * To enable GMII on Port 0, Port 1 must also disable RGMII_EN too. > > + */ > > + if (!phy_interface_is_rgmii(dev->phydev)) > > + wrlp(mp, PORT_SERIAL_CONTROL1, > > + rdlp(mp, PORT_SERIAL_CONTROL1) & ~RGMII_EN); > > I could be reading this wrong, but doesn't this break the third line: > > > + * MII/MMII RGMII 0 1 > > Port 1 probes first, phy_interface is rgmii, so nothing happens, port1 > RGMII_EN is left true. > > Port 0 then probes, MII/MMII is not RGMII, so port1 RGMII_EN is > cleared, breaking port1. > > I think you need to be more specific with the comparison. > > Andrew Oh, I see. So you mean "phy-mode" property should belong to controller, not port? I thought one controller can have at most one port.
On Sat, Oct 01, 2022 at 04:47:42AM +0800, Yangfl wrote: > Andrew Lunn <andrew@lunn.ch> 于2022年10月1日周六 04:28写道： > > > > On Sat, Oct 01, 2022 at 03:49:23AM +0800, David Yang wrote: > > > On device reset all ports are automatically set to RGMII mode. MII > > > mode must be explicitly enabled. > > > > > > If SoC has two Ethernet controllers, by setting both of them into MII > > > mode, the first controller enters GMII mode, while the second > > > controller is effectively disabled. This requires configuring (and > > > maybe enabling) the second controller in the device tree, even though > > > it cannot be used. > > > > > > Signed-off-by: David Yang <mmyangfl@gmail.com> > > > --- > > > drivers/net/ethernet/marvell/mv643xx_eth.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c > > > index b6be0552a..e2216ce5e 100644 > > > --- a/drivers/net/ethernet/marvell/mv643xx_eth.c > > > +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c > > > @@ -108,6 +108,7 @@ static char mv643xx_eth_driver_version[] = "1.4"; > > > #define TXQ_COMMAND 0x0048 > > > #define TXQ_FIX_PRIO_CONF 0x004c > > > #define PORT_SERIAL_CONTROL1 0x004c > > > +#define RGMII_EN 0x00000008 > > > #define CLK125_BYPASS_EN 0x00000010 > > > #define TX_BW_RATE 0x0050 > > > #define TX_BW_MTU 0x0058 > > > @@ -1245,6 +1246,21 @@ static void mv643xx_eth_adjust_link(struct net_device *dev) > > > > > > out_write: > > > wrlp(mp, PORT_SERIAL_CONTROL, pscr); > > > + > > > + /* If two Ethernet controllers present in the SoC, MII modes follow the > > > + * following matrix: > > > + * > > > + * Port0 Mode Port1 Mode Port0 RGMII_EN Port1 RGMII_EN > > > + * RGMII RGMII 1 1 > > > + * RGMII MII/MMII 1 0 > > > + * MII/MMII RGMII 0 1 > > > + * GMII N/A 0 0 > > > + * > > > + * To enable GMII on Port 0, Port 1 must also disable RGMII_EN too. > > > + */ > > > + if (!phy_interface_is_rgmii(dev->phydev)) > > > + wrlp(mp, PORT_SERIAL_CONTROL1, > > > + rdlp(mp, PORT_SERIAL_CONTROL1) & ~RGMII_EN); > > > > I could be reading this wrong, but doesn't this break the third line: > > > > > + * MII/MMII RGMII 0 1 > > > > Port 1 probes first, phy_interface is rgmii, so nothing happens, port1 > > RGMII_EN is left true. > > > > Port 0 then probes, MII/MMII is not RGMII, so port1 RGMII_EN is > > cleared, breaking port1. > > > > I think you need to be more specific with the comparison. > > > > Andrew > > Oh, I see. So you mean "phy-mode" property should belong to > controller, not port? I thought one controller can have at most one > port. If you look at mv643xx_eth_shared_of_probe(), it appears a controller can have multiple ports. And: if (dev_num == 3) { dev_err(&pdev->dev, "too many ports registered\n"); return -EINVAL; } I don't know the details? Andrew
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c index b6be0552a..e2216ce5e 100644 --- a/drivers/net/ethernet/marvell/mv643xx_eth.c +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c @@ -108,6 +108,7 @@ static char mv643xx_eth_driver_version[] = "1.4"; #define TXQ_COMMAND 0x0048 #define TXQ_FIX_PRIO_CONF 0x004c #define PORT_SERIAL_CONTROL1 0x004c +#define RGMII_EN 0x00000008 #define CLK125_BYPASS_EN 0x00000010 #define TX_BW_RATE 0x0050 #define TX_BW_MTU 0x0058 @@ -1245,6 +1246,21 @@ static void mv643xx_eth_adjust_link(struct net_device *dev) out_write: wrlp(mp, PORT_SERIAL_CONTROL, pscr); + + /* If two Ethernet controllers present in the SoC, MII modes follow the + * following matrix: + * + * Port0 Mode Port1 Mode Port0 RGMII_EN Port1 RGMII_EN + * RGMII RGMII 1 1 + * RGMII MII/MMII 1 0 + * MII/MMII RGMII 0 1 + * GMII N/A 0 0 + * + * To enable GMII on Port 0, Port 1 must also disable RGMII_EN too. + */ + if (!phy_interface_is_rgmii(dev->phydev)) + wrlp(mp, PORT_SERIAL_CONTROL1, + rdlp(mp, PORT_SERIAL_CONTROL1) & ~RGMII_EN); } /* statistics ***************************************************************/
On device reset all ports are automatically set to RGMII mode. MII mode must be explicitly enabled. If SoC has two Ethernet controllers, by setting both of them into MII mode, the first controller enters GMII mode, while the second controller is effectively disabled. This requires configuring (and maybe enabling) the second controller in the device tree, even though it cannot be used. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/ethernet/marvell/mv643xx_eth.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)