diff mbox series

[RFC,10/10] net: stmmac: dwmac-loongson: Add GNET support

Message ID 98b53d15bb983c309f79acf9619b88ea4fbb8f14.1689215889.git.chenfeiyang@loongson.cn (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series net: phy/stmmac: Add Loongson platform support | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Feiyang Chen July 13, 2023, 2:49 a.m. UTC
Add GNET support. Use the fix_mac_speed() callback to workaround
issues with the Loongson PHY.

Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  1 +
 .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 34 +++++++++++++++++++
 2 files changed, 35 insertions(+)

Comments

Andrew Lunn July 13, 2023, 4:07 a.m. UTC | #1
On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> Add GNET support. Use the fix_mac_speed() callback to workaround
> issues with the Loongson PHY.

What are the issues?

> +static void loongson_gnet_fix_speed(void *priv, unsigned int speed)
> +{
> +	struct net_device *ndev = (struct net_device *)(*(unsigned long *)priv);
> +	struct stmmac_priv *ptr = netdev_priv(ndev);
> +
> +	if (speed == SPEED_1000)
> +		if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */)
> +			phy_restart_aneg(ndev->phydev);

This needs a comment at least, but if you explain what is actually
FUBAR in this PHY, we might be able to tell you a better way to work
around its problems.

> +static int loongson_gnet_data(struct pci_dev *pdev,
> +			      struct plat_stmmacenet_data *plat)
> +{
> +	common_default_data(pdev, plat);
> +
> +	plat->mdio_bus_data->phy_mask = 0xfffffffb;
> +
> +	plat->phy_addr = 2;
> +	plat->phy_interface = PHY_INTERFACE_MODE_GMII;

You would normally get this from DT. Is the PHY integrated? Is it
really using GMII, or would PHY_INTERFACE_MODE_INTERNAL be better?

       Andrew
Russell King (Oracle) July 13, 2023, 8:06 a.m. UTC | #2
On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> Add GNET support. Use the fix_mac_speed() callback to workaround
> issues with the Loongson PHY.

It would be good to document what those issue(s) are, and if they are a
PHY issue, why they need to be resolved in a MAC driver rather than
using the PHY driver's link_change_notify().

Thanks.
Feiyang Chen July 14, 2023, 2:24 a.m. UTC | #3
On Thu, Jul 13, 2023 at 12:07 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> > Add GNET support. Use the fix_mac_speed() callback to workaround
> > issues with the Loongson PHY.
>
> What are the issues?
>

Hi, Andrew,

There is an issue with the synchronization between the network card
and the PHY. In the case of gigabit operation, if the PHY's speed
changes, the network card's speed remains unaffected. Hence, it is
necessary to initiate a re-negotiation process with the PHY to align
the link speeds properly.

> > +static void loongson_gnet_fix_speed(void *priv, unsigned int speed)
> > +{
> > +     struct net_device *ndev = (struct net_device *)(*(unsigned long *)priv);
> > +     struct stmmac_priv *ptr = netdev_priv(ndev);
> > +
> > +     if (speed == SPEED_1000)
> > +             if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */)
> > +                     phy_restart_aneg(ndev->phydev);
>
> This needs a comment at least, but if you explain what is actually
> FUBAR in this PHY, we might be able to tell you a better way to work
> around its problems.
>
> > +static int loongson_gnet_data(struct pci_dev *pdev,
> > +                           struct plat_stmmacenet_data *plat)
> > +{
> > +     common_default_data(pdev, plat);
> > +
> > +     plat->mdio_bus_data->phy_mask = 0xfffffffb;
> > +
> > +     plat->phy_addr = 2;
> > +     plat->phy_interface = PHY_INTERFACE_MODE_GMII;
>
> You would normally get this from DT. Is the PHY integrated? Is it
> really using GMII, or would PHY_INTERFACE_MODE_INTERNAL be better?
>

Yes, the PHY is integrated. I'll try to use PHY_INTERFACE_MODE_INTERNAL
later.

Thanks,
Feiyang

>        Andrew
Feiyang Chen July 14, 2023, 2:27 a.m. UTC | #4
On Thu, Jul 13, 2023 at 4:06 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> > Add GNET support. Use the fix_mac_speed() callback to workaround
> > issues with the Loongson PHY.
>
> It would be good to document what those issue(s) are, and if they are a
> PHY issue, why they need to be resolved in a MAC driver rather than
> using the PHY driver's link_change_notify().
>

Hi, Russell,

There is an issue with the synchronization between the network card
and the PHY. I believe that I am unable to access certain bits of the
network card's registers within the PHY driver.

Thanks,
Feiyang

> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Andrew Lunn July 14, 2023, 4:22 a.m. UTC | #5
> Hi, Russell,
> 
> There is an issue with the synchronization between the network card
> and the PHY.

Details. We need the details in order to tell you the best workaround
for your broken hardware.

I believe that I am unable to access certain bits of the
> network card's registers within the PHY driver.

The PHY driver and the MAC driver should never access each others
registers.

	Andrew
Andrew Lunn July 14, 2023, 4:26 a.m. UTC | #6
On Fri, Jul 14, 2023 at 10:24:37AM +0800, Feiyang Chen wrote:
> On Thu, Jul 13, 2023 at 12:07 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> > > Add GNET support. Use the fix_mac_speed() callback to workaround
> > > issues with the Loongson PHY.
> >
> > What are the issues?
> >
> 
> Hi, Andrew,
> 
> There is an issue with the synchronization between the network card
> and the PHY. In the case of gigabit operation, if the PHY's speed
> changes, the network card's speed remains unaffected. Hence, it is
> necessary to initiate a re-negotiation process with the PHY to align
> the link speeds properly.

When the line side speed changes, the PHY will first report link down
via the adjust_link callback in the MAC driver. Once the PHY has
determined the new speed, the adjust_link callback will be called
again with the new speed and pause parameters.

So what is actually going wrong?

   Andrew
Russell King (Oracle) July 14, 2023, 8:45 a.m. UTC | #7
On Fri, Jul 14, 2023 at 10:24:37AM +0800, Feiyang Chen wrote:
> On Thu, Jul 13, 2023 at 12:07 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> > > Add GNET support. Use the fix_mac_speed() callback to workaround
> > > issues with the Loongson PHY.
> >
> > What are the issues?
> >
> 
> Hi, Andrew,
> 
> There is an issue with the synchronization between the network card
> and the PHY. In the case of gigabit operation, if the PHY's speed
> changes, the network card's speed remains unaffected. Hence, it is
> necessary to initiate a re-negotiation process with the PHY to align
> the link speeds properly.

Please could you explain a bit more what happens when "the PHY's speed
changes". Are you suggesting that:

You have a connection where the media side has negotiated 1G speed.
The gigabit partner is disconnected, so the link goes down, and is then
replaced by a partner only capable of 100M. The link comes back up at
100M, but the network card continues trying to operate at 1G?

Thanks.
Russell King (Oracle) July 14, 2023, 8:52 a.m. UTC | #8
On Fri, Jul 14, 2023 at 06:26:14AM +0200, Andrew Lunn wrote:
> On Fri, Jul 14, 2023 at 10:24:37AM +0800, Feiyang Chen wrote:
> > On Thu, Jul 13, 2023 at 12:07 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> > > > Add GNET support. Use the fix_mac_speed() callback to workaround
> > > > issues with the Loongson PHY.
> > >
> > > What are the issues?
> > >
> > 
> > Hi, Andrew,
> > 
> > There is an issue with the synchronization between the network card
> > and the PHY. In the case of gigabit operation, if the PHY's speed
> > changes, the network card's speed remains unaffected. Hence, it is
> > necessary to initiate a re-negotiation process with the PHY to align
> > the link speeds properly.
> 
> When the line side speed changes, the PHY will first report link down
> via the adjust_link callback in the MAC driver. Once the PHY has
> determined the new speed, the adjust_link callback will be called
> again with the new speed and pause parameters.

Or in the case of stmmac, phylink is involved, so .mac_link_down will
get called and then .mac_link_up with the new parameters.
Feiyang Chen July 17, 2023, 7:54 a.m. UTC | #9
On Fri, Jul 14, 2023 at 4:45 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Jul 14, 2023 at 10:24:37AM +0800, Feiyang Chen wrote:
> > On Thu, Jul 13, 2023 at 12:07 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 10:49:38AM +0800, Feiyang Chen wrote:
> > > > Add GNET support. Use the fix_mac_speed() callback to workaround
> > > > issues with the Loongson PHY.
> > >
> > > What are the issues?
> > >
> >
> > Hi, Andrew,
> >
> > There is an issue with the synchronization between the network card
> > and the PHY. In the case of gigabit operation, if the PHY's speed
> > changes, the network card's speed remains unaffected. Hence, it is
> > necessary to initiate a re-negotiation process with the PHY to align
> > the link speeds properly.
>
> Please could you explain a bit more what happens when "the PHY's speed
> changes". Are you suggesting that:
>
> You have a connection where the media side has negotiated 1G speed.
> The gigabit partner is disconnected, so the link goes down, and is then
> replaced by a partner only capable of 100M. The link comes back up at
> 100M, but the network card continues trying to operate at 1G?
>

Hi, Russell, Andrew,

This bug shows up in the following way: when the speed is set to 1000M,
PHY is set up correctly and its status is normal. However, the controller
and PHY don't work well together, causing the controller to fail in
establishing a gigabit connection, which leads to a network disruption.
So, we need to use this bit to check if the controller's status is correct
and reset PHY if necessary.

if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */)

The troublesome issue is that we have to check this bit in stmmac.
As a result, we are forced to place phy_restart_aneg() there.

Thanks,
Feiyang

> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 5f5a997f21f3..8c25d5616941 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -279,6 +279,7 @@  config DWMAC_LOONGSON
 	default MACH_LOONGSON64
 	depends on STMMAC_ETH && PCI
 	depends on COMMON_CLK
+	select LOONGSON_PHY
 	help
 	  This selects the LOONGSON PCI bus support for the stmmac driver,
 	  Support for ethernet controller on Loongson-2K1000 SoC and LS7A1000 bridge.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index 3732f90a79c7..c840d9d1d72a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -69,6 +69,38 @@  static struct stmmac_pci_info loongson_gmac_pci_info = {
 	.setup = loongson_gmac_data,
 };
 
+static void loongson_gnet_fix_speed(void *priv, unsigned int speed)
+{
+	struct net_device *ndev = (struct net_device *)(*(unsigned long *)priv);
+	struct stmmac_priv *ptr = netdev_priv(ndev);
+
+	if (speed == SPEED_1000)
+		if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */)
+			phy_restart_aneg(ndev->phydev);
+}
+
+static int loongson_gnet_data(struct pci_dev *pdev,
+			      struct plat_stmmacenet_data *plat)
+{
+	common_default_data(pdev, plat);
+
+	plat->mdio_bus_data->phy_mask = 0xfffffffb;
+
+	plat->phy_addr = 2;
+	plat->phy_interface = PHY_INTERFACE_MODE_GMII;
+
+	plat->fix_mac_speed = loongson_gnet_fix_speed;
+
+	/* Get netdev pointer address */
+	plat->bsp_priv = &(pdev->dev.driver_data);
+
+	return 0;
+}
+
+static struct stmmac_pci_info loongson_gnet_pci_info = {
+	.setup = loongson_gnet_data,
+};
+
 static int loongson_dwmac_probe(struct pci_dev *pdev,
 				const struct pci_device_id *id)
 {
@@ -296,9 +328,11 @@  static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend,
 			 loongson_dwmac_resume);
 
 #define PCI_DEVICE_ID_LOONGSON_GMAC	0x7a03
+#define PCI_DEVICE_ID_LOONGSON_GNET	0x7a13
 
 static const struct pci_device_id loongson_dwmac_id_table[] = {
 	{ PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) },
+	{ PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) },
 	{}
 };
 MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);