| Message ID | e3c83d1e62cd67d5f3b50b30f46c232a307504ab.1706601050.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
On Tue, Jan 30, 2024 at 04:48:20PM +0800, Yanteng Si wrote: > Current GNET on LS7A only supports ANE when speed is > set to 1000M. If so you need to merge it into the patch [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > --- > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 19 +++++++++++++++++++ > .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 ++++++ > include/linux/stmmac.h | 1 + > 3 files changed, 26 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index 60d0a122d7c9..264c4c198d5a 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -344,6 +344,21 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { > .config = loongson_gmac_config, > }; > > +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, unsigned int mode) > +{ > + struct loongson_data *ld = (struct loongson_data *)priv; > + struct net_device *ndev = dev_get_drvdata(ld->dev); > + struct stmmac_priv *ptr = netdev_priv(ndev); > + > + /* The controller and PHY don't work well together. So there _is_ a PHY. What is the interface between MAC and PHY then? > + * We need to use the PS bit to check if the controller's status > + * is correct and reset PHY if necessary. > + */ > + if (speed == SPEED_1000) > + if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) > + phy_restart_aneg(ndev->phydev); 1. Please add curly braces for the outer if-statement. 2. MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. 3. How is the AN-restart helps? PHY-reset is done in stmmac_init_phy()->phylink_connect_phy()->... a bit earlier than this is called in the framework of the stmmac_mac_link_up() callback. Wouldn't that restart AN too? > +} > + > static struct mac_device_info *loongson_setup(void *apriv) > { > struct stmmac_priv *priv = apriv; > @@ -401,6 +416,7 @@ static int loongson_gnet_data(struct pci_dev *pdev, > plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > > plat->bsp_priv = &pdev->dev; > + plat->fix_mac_speed = loongson_gnet_fix_speed; > > plat->dma_cfg->pbl = 32; > plat->dma_cfg->pblx8 = true; > @@ -416,6 +432,9 @@ static int loongson_gnet_config(struct pci_dev *pdev, > struct stmmac_resources *res, > struct device_node *np) > { > + if (pdev->revision == 0x00 || pdev->revision == 0x01) > + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > + This should be in the patch [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > return 0; > } > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > index 42d27b97dd1d..31068fbc23c9 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, > return 0; > } > > + if (FIELD_GET(STMMAC_FLAG_DISABLE_FORCE_1000, priv->plat->flags)) { FIELD_GET()? > + if (cmd->base.speed == SPEED_1000 && > + cmd->base.autoneg != AUTONEG_ENABLE) > + return -EOPNOTSUPP; > + } > + > return phylink_ethtool_ksettings_set(priv->phylink, cmd); > } > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index dee5ad6e48c5..2810361e4048 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -221,6 +221,7 @@ struct dwmac4_addrs { > #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) > #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) > #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) > +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) Detach the change introducing the STMMAC_FLAG_DISABLE_FORCE_1000 flag into a separate patch a place it before [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support as a pre-requisite/preparation patch. Don't forget a _detailed_ description of why it's necessary, what is wrong with GNET so 1G speed doesn't work without AN. -Serge(y) > > struct plat_stmmacenet_data { > int bus_id; > -- > 2.31.4 >
在 2024/3/14 17:43, Yanteng Si 写道: > 在 2024/2/6 05:55, Serge Semin 写道: > > On Tue, Jan 30, 2024 at 04:48:20PM +0800, Yanteng Si wrote: > >> Current GNET on LS7A only supports ANE when speed is > >> set to 1000M. > > If so you need to merge it into the patch > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >> Current GNET on LS7A only supports ANE when speed is >> set to 1000M. >If so you need to merge it into the patch >[PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support OK. > > > >> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > >> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > >> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > >> --- > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 19 +++++++++++++++++++ > >> .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 ++++++ > >> include/linux/stmmac.h | 1 + > >> 3 files changed, 26 insertions(+) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> index 60d0a122d7c9..264c4c198d5a 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> @@ -344,6 +344,21 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { > >> .config = loongson_gmac_config, > >> }; > >> > >> +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, unsigned int mode) > >> +{ > >> + struct loongson_data *ld = (struct loongson_data *)priv; > >> + struct net_device *ndev = dev_get_drvdata(ld->dev); > >> + struct stmmac_priv *ptr = netdev_priv(ndev); > >> + > >> + /* The controller and PHY don't work well together. > > So there _is_ a PHY. What is the interface between MAC and PHY then? > > > > GMAC only has a MAC chip inside the chip and needs an external PHY chip; GNET > > has the PHY chip inside the chip. > >> + * We need to use the PS bit to check if the controller's status > >> + * is correct and reset PHY if necessary. > >> + */ > >> + if (speed == SPEED_1000) > >> + if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) > >> + phy_restart_aneg(ndev->phydev); > > 1. Please add curly braces for the outer if-statement. > OK, > > 2. MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. > > OK. > > if(speed==SPEED_1000){ > /*MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro.*/ > if(readl(ptr->ioaddr+MAC_CTRL_REG) &(1<<15)) > phy_restart_aneg(ndev->phydev); > } > > > 3. How is the AN-restart helps? PHY-reset is done in > > stmmac_init_phy()->phylink_connect_phy()->... a bit earlier than > > this is called in the framework of the stmmac_mac_link_up() callback. > > Wouldn't that restart AN too? > > Due to a bug in the chip's internal PHY, the network is still not working after > the first self-negotiation, and it needs to be self-negotiated again. > > > > >> +} > >> + > >> static struct mac_device_info *loongson_setup(void *apriv) > >> { > >> struct stmmac_priv *priv = apriv; > >> @@ -401,6 +416,7 @@ static int loongson_gnet_data(struct pci_dev *pdev, > >> plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > >> > >> plat->bsp_priv = &pdev->dev; > >> + plat->fix_mac_speed = loongson_gnet_fix_speed; > >> > >> plat->dma_cfg->pbl = 32; > >> plat->dma_cfg->pblx8 = true; > >> @@ -416,6 +432,9 @@ static int loongson_gnet_config(struct pci_dev *pdev, > >> struct stmmac_resources *res, > >> struct device_node *np) > >> { > >> + if (pdev->revision == 0x00 || pdev->revision == 0x01) > >> + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > >> + > > This should be in the patch > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > OK. > > > >> return 0; > >> } > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > >> index 42d27b97dd1d..31068fbc23c9 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > >> @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, > >> return 0; > >> } > >> > >> + if (FIELD_GET(STMMAC_FLAG_DISABLE_FORCE_1000, priv->plat->flags)) { > > FIELD_GET()? > > OK, > > if (STMMAC_FLAG_DISABLE_FORCE_1000 & priv->plat->flags) { > > > > >> + if (cmd->base.speed == SPEED_1000 && > >> + cmd->base.autoneg != AUTONEG_ENABLE) > >> + return -EOPNOTSUPP; > >> + } > >> + > >> return phylink_ethtool_ksettings_set(priv->phylink, cmd); > >> } > >> > >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > >> index dee5ad6e48c5..2810361e4048 100644 > >> --- a/include/linux/stmmac.h > >> +++ b/include/linux/stmmac.h > >> @@ -221,6 +221,7 @@ struct dwmac4_addrs { > >> #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) > >> #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) > >> #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) > >> +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) > > Detach the change introducing the STMMAC_FLAG_DISABLE_FORCE_1000 flag > > into a separate patch a place it before > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > as a pre-requisite/preparation patch. > > Don't forget a _detailed_ description of why it's necessary, what is > > wrong with GNET so 1G speed doesn't work without AN. > > OK. > > > Thanks, > > Yanteng > > > > > -Serge(y) > > > >> > >> struct plat_stmmacenet_data { > >> int bus_id; > >> -- > >> 2.31.4 > >>
On Thu, Mar 14, 2024 at 09:18:15PM +0800, Yanteng Si wrote: > > 在 2024/3/14 17:43, Yanteng Si 写道: > > 在 2024/2/6 05:55, Serge Semin 写道: > > > On Tue, Jan 30, 2024 at 04:48:20PM +0800, Yanteng Si wrote: > > >> Current GNET on LS7A only supports ANE when speed is > > >> set to 1000M. > > > If so you need to merge it into the patch > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > > > Current GNET on LS7A only supports ANE when speed is > > > set to 1000M. > > > If so you need to merge it into the patch > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > OK. > > > > > > >> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > > >> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > > >> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > > >> --- > > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 19 +++++++++++++++++++ > > >> .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 ++++++ > > >> include/linux/stmmac.h | 1 + > > >> 3 files changed, 26 insertions(+) > > >> > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> index 60d0a122d7c9..264c4c198d5a 100644 > > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> @@ -344,6 +344,21 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { > > >> .config = loongson_gmac_config, > > >> }; > > >> >> +static void loongson_gnet_fix_speed(void *priv, unsigned int > > speed, unsigned int mode) > > >> +{ > > >> + struct loongson_data *ld = (struct loongson_data *)priv; > > >> + struct net_device *ndev = dev_get_drvdata(ld->dev); > > >> + struct stmmac_priv *ptr = netdev_priv(ndev); > > >> + > > >> + /* The controller and PHY don't work well together. > > > So there _is_ a PHY. What is the interface between MAC and PHY then? > > > > > > GMAC only has a MAC chip inside the chip and needs an external PHY > > chip; GNET > has the PHY chip inside the chip. We are talking about GNETs in this method since it has the loongson_gnet_ prefix. You are referring to GMAC. I am getting confused about all of these. Based on the patch 06/11 of this series you call "Loongson GNET" of all the devices placed on the PCI devices with PCI ID 0x7a13. PCIe device with ID 0x7a03 is called "Loongson GMAC". Right? Anyway no matter whether the PHY is placed externally or inside the chip. AFAIU as long as you know the interface type between MAC and PHY it would be better to have it specified. > > >> + * We need to use the PS bit to check if the controller's status > > >> + * is correct and reset PHY if necessary. > > >> + */ > > >> + if (speed == SPEED_1000) > > >> + if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) > > >> + phy_restart_aneg(ndev->phydev); > > > 1. Please add curly braces for the outer if-statement. > > OK, > > > 2. MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. > > > > OK. > > > > if(speed==SPEED_1000){ > > /*MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro.*/ > > if(readl(ptr->ioaddr+MAC_CTRL_REG) &(1<<15)) > > phy_restart_aneg(ndev->phydev); > > } > > > > > 3. How is the AN-restart helps? PHY-reset is done in > > > stmmac_init_phy()->phylink_connect_phy()->... a bit earlier than > > > this is called in the framework of the stmmac_mac_link_up() callback. > > > Wouldn't that restart AN too? > > > > Due to a bug in the chip's internal PHY, the network is still not working after > > the first self-negotiation, and it needs to be self-negotiated again. Then please describe the bug in more details then. Getting back to the code you implemented here. In the in-situ comment you say: "We need to use the PS bit to check if the controller's status is correct and reset PHY if necessary." By calling phy_restart_aneg() you don't reset the PHY. Moreover if "PS" flag is set, then the MAC has been pre-configured to work in the 10/100Mbps mode. Since 1000Mbps speed is requested, the MAC_CTRL_REG.PS flag will be cleared later in the stmmac_mac_link_up() method and then phylink_start() shall cause the link speed re-auto-negotiation. Why do you need the auto-negotiation started for the default MAC config which will be changed just in a moment later? All of that seems weird. Most importantly I have doubts the networking subsystem maintainers will permit you calling the phy_restart_aneg() method from the MAC driver code. > > > > > > > >> +} > > >> + > > >> static struct mac_device_info *loongson_setup(void *apriv) > > >> { > > >> struct stmmac_priv *priv = apriv; > > >> @@ -401,6 +416,7 @@ static int loongson_gnet_data(struct pci_dev *pdev, > > >> plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > > >> >> plat->bsp_priv = &pdev->dev; > > >> + plat->fix_mac_speed = loongson_gnet_fix_speed; > > >> >> plat->dma_cfg->pbl = 32; > > >> plat->dma_cfg->pblx8 = true; > > >> @@ -416,6 +432,9 @@ static int loongson_gnet_config(struct pci_dev *pdev, > > >> struct stmmac_resources *res, > > >> struct device_node *np) > > >> { > > >> + if (pdev->revision == 0x00 || pdev->revision == 0x01) > > >> + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > > >> + > > > This should be in the patch > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > OK. > > > > > >> return 0; > > >> } > > >> >> diff --git > > a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > >> index 42d27b97dd1d..31068fbc23c9 100644 > > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > >> @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, > > >> return 0; > > >> } > > >> >> + if (FIELD_GET(STMMAC_FLAG_DISABLE_FORCE_1000, > > priv->plat->flags)) { > > > FIELD_GET()? > > > > OK, > > > > if (STMMAC_FLAG_DISABLE_FORCE_1000 & priv->plat->flags) { it's better to change the order of the operands: if (priv->plat->flags & STMMAC_FLAG_DISABLE_FORCE_1000) { -Serge(y) > > > > > > > >> + if (cmd->base.speed == SPEED_1000 && > > >> + cmd->base.autoneg != AUTONEG_ENABLE) > > >> + return -EOPNOTSUPP; > > >> + } > > >> + > > >> return phylink_ethtool_ksettings_set(priv->phylink, cmd); > > >> } > > >> >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > > >> index dee5ad6e48c5..2810361e4048 100644 > > >> --- a/include/linux/stmmac.h > > >> +++ b/include/linux/stmmac.h > > >> @@ -221,6 +221,7 @@ struct dwmac4_addrs { > > >> #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) > > >> #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) > > >> #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) > > >> +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) > > > Detach the change introducing the STMMAC_FLAG_DISABLE_FORCE_1000 flag > > > into a separate patch a place it before > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > > as a pre-requisite/preparation patch. > > > Don't forget a _detailed_ description of why it's necessary, what is > > > wrong with GNET so 1G speed doesn't work without AN. > > > > OK. > > > > > > Thanks, > > > > Yanteng > > > > > > > > -Serge(y) > > > > > >> >> struct plat_stmmacenet_data { > > >> int bus_id; > > >> -- >> 2.31.4 > > >> >
在 2024/3/20 01:02, Serge Semin 写道: > On Thu, Mar 14, 2024 at 09:18:15PM +0800, Yanteng Si wrote: >> 在 2024/3/14 17:43, Yanteng Si 写道: >>> 在 2024/2/6 05:55, Serge Semin 写道: >>>> On Tue, Jan 30, 2024 at 04:48:20PM +0800, Yanteng Si wrote: >>>>> Current GNET on LS7A only supports ANE when speed is >>>>> set to 1000M. >>>> If so you need to merge it into the patch >>>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >>>> Current GNET on LS7A only supports ANE when speed is >>>> set to 1000M. >>> If so you need to merge it into the patch >>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >> OK. >> >>>>> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> >>>>> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> >>>>> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> >>>>> --- >>>>> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 19 +++++++++++++++++++ >>>>> .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 ++++++ >>>>> include/linux/stmmac.h | 1 + >>>>> 3 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> index 60d0a122d7c9..264c4c198d5a 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> @@ -344,6 +344,21 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { >>>>> .config = loongson_gmac_config, >>>>> }; >>>>> >> +static void loongson_gnet_fix_speed(void *priv, unsigned int >>> speed, unsigned int mode) >>>>> +{ >>>>> + struct loongson_data *ld = (struct loongson_data *)priv; >>>>> + struct net_device *ndev = dev_get_drvdata(ld->dev); >>>>> + struct stmmac_priv *ptr = netdev_priv(ndev); >>>>> + >>>>> + /* The controller and PHY don't work well together. >>>> So there _is_ a PHY. What is the interface between MAC and PHY then? >>>> >>>> GMAC only has a MAC chip inside the chip and needs an external PHY >>> chip; GNET > has the PHY chip inside the chip. > We are talking about GNETs in this method since it has the > loongson_gnet_ prefix. You are referring to GMAC. I am getting > confused about all of these. Based on the patch 06/11 of this series > you call "Loongson GNET" of all the devices placed on the PCI devices > with PCI ID 0x7a13. PCIe device with ID 0x7a03 is called "Loongson > GMAC". Right? ls2k is SOC, ls7a ls bridge: device type pci_id gmac_version ls7a1000 gmac 7a03 0x37 ls2k1000 gmac 7a03 0x37 ls7a2000 gnet 7a13 0x37 ls2k2000 gnet 7a13 0x10 > > Anyway no matter whether the PHY is placed externally or inside the > chip. AFAIU as long as you know the interface type between MAC and PHY > it would be better to have it specified. > >>>>> + * We need to use the PS bit to check if the controller's status >>>>> + * is correct and reset PHY if necessary. >>>>> + */ >>>>> + if (speed == SPEED_1000) >>>>> + if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) >>>>> + phy_restart_aneg(ndev->phydev); >>>> 1. Please add curly braces for the outer if-statement. >>> OK, >>>> 2. MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. >>> OK. >>> >>> if(speed==SPEED_1000){ >>> /*MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro.*/ >>> if(readl(ptr->ioaddr+MAC_CTRL_REG) &(1<<15)) >>> phy_restart_aneg(ndev->phydev); >>> } >>> >>>> 3. How is the AN-restart helps? PHY-reset is done in >>>> stmmac_init_phy()->phylink_connect_phy()->... a bit earlier than >>>> this is called in the framework of the stmmac_mac_link_up() callback. >>>> Wouldn't that restart AN too? >>> Due to a bug in the chip's internal PHY, the network is still not working after >>> the first self-negotiation, and it needs to be self-negotiated again. > Then please describe the bug in more details then. > > Getting back to the code you implemented here. In the in-situ comment > you say: "We need to use the PS bit to check if the controller's > status is correct and reset PHY if necessary." By calling > phy_restart_aneg() you don't reset the PHY. > > Moreover if "PS" flag is set, then the MAC has been pre-configured to > work in the 10/100Mbps mode. Since 1000Mbps speed is requested, the > MAC_CTRL_REG.PS flag will be cleared later in the > stmmac_mac_link_up() method and then phylink_start() shall cause the > link speed re-auto-negotiation. Why do you need the auto-negotiation > started for the default MAC config which will be changed just in a > moment later? All of that seems weird. > > Most importantly I have doubts the networking subsystem maintainers > will permit you calling the phy_restart_aneg() method from the MAC > driver code. I will reply to you tomorrow. > >>>>> +} >>>>> + >>>>> static struct mac_device_info *loongson_setup(void *apriv) >>>>> { >>>>> struct stmmac_priv *priv = apriv; >>>>> @@ -401,6 +416,7 @@ static int loongson_gnet_data(struct pci_dev *pdev, >>>>> plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; >>>>> >> plat->bsp_priv = &pdev->dev; >>>>> + plat->fix_mac_speed = loongson_gnet_fix_speed; >>>>> >> plat->dma_cfg->pbl = 32; >>>>> plat->dma_cfg->pblx8 = true; >>>>> @@ -416,6 +432,9 @@ static int loongson_gnet_config(struct pci_dev *pdev, >>>>> struct stmmac_resources *res, >>>>> struct device_node *np) >>>>> { >>>>> + if (pdev->revision == 0x00 || pdev->revision == 0x01) >>>>> + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; >>>>> + >>>> This should be in the patch >>>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >>> OK. >>>>> return 0; >>>>> } >>>>> >> diff --git >>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>>>> index 42d27b97dd1d..31068fbc23c9 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>>>> @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, >>>>> return 0; >>>>> } >>>>> >> + if (FIELD_GET(STMMAC_FLAG_DISABLE_FORCE_1000, >>> priv->plat->flags)) { >>>> FIELD_GET()? >>> OK, >>> >>> if (STMMAC_FLAG_DISABLE_FORCE_1000 & priv->plat->flags) { > it's better to change the order of the operands: > if (priv->plat->flags & STMMAC_FLAG_DISABLE_FORCE_1000) { OK, Thanks, Yanteng > > -Serge(y) > >>>>> + if (cmd->base.speed == SPEED_1000 && >>>>> + cmd->base.autoneg != AUTONEG_ENABLE) >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + >>>>> return phylink_ethtool_ksettings_set(priv->phylink, cmd); >>>>> } >>>>> >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >>>>> index dee5ad6e48c5..2810361e4048 100644 >>>>> --- a/include/linux/stmmac.h >>>>> +++ b/include/linux/stmmac.h >>>>> @@ -221,6 +221,7 @@ struct dwmac4_addrs { >>>>> #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) >>>>> #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) >>>>> #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) >>>>> +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) >>>> Detach the change introducing the STMMAC_FLAG_DISABLE_FORCE_1000 flag >>>> into a separate patch a place it before >>>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >>>> as a pre-requisite/preparation patch. >>>> Don't forget a _detailed_ description of why it's necessary, what is >>>> wrong with GNET so 1G speed doesn't work without AN. >>> OK. >>> >>> >>> Thanks, >>> >>> Yanteng >>> >>>> -Serge(y) >>>> >>>>> >> struct plat_stmmacenet_data { >>>>> int bus_id; >>>>> -- >> 2.31.4 >>>>>
在 2024/3/20 01:02, Serge Semin 写道: >>> Due to a bug in the chip's internal PHY, the network is still not working after >>> the first self-negotiation, and it needs to be self-negotiated again. > Then please describe the bug in more details then. > > Getting back to the code you implemented here. In the in-situ comment > you say: "We need to use the PS bit to check if the controller's > status is correct and reset PHY if necessary." By calling > phy_restart_aneg() you don't reset the PHY. > > Moreover if "PS" flag is set, then the MAC has been pre-configured to > work in the 10/100Mbps mode. Since 1000Mbps speed is requested, the > MAC_CTRL_REG.PS flag will be cleared later in the > stmmac_mac_link_up() method and then phylink_start() shall cause the > link speed re-auto-negotiation. Why do you need the auto-negotiation > started for the default MAC config which will be changed just in a > moment later? All of that seems weird. When switching speeds (from 100M to 1000M), the phy cannot output clocks, resulting in the unavailability of the network card. At this time, a reset of the phy is required. BTW, This bug has been fixed in gnet of 2k2000 (0x10, 7a13). > > Most importantly I have doubts the networking subsystem maintainers > will permit you calling the phy_restart_aneg() method from the MAC > driver code. We are happy to accept the opinions of the community. Thanks, Yanteng >
On Thu, Mar 21, 2024 at 05:29:55PM +0800, Yanteng Si wrote: > > 在 2024/3/20 01:02, Serge Semin 写道: > > > > Due to a bug in the chip's internal PHY, the network is still not working after > > > > the first self-negotiation, and it needs to be self-negotiated again. > > Then please describe the bug in more details then. > > > > Getting back to the code you implemented here. In the in-situ comment > > you say: "We need to use the PS bit to check if the controller's > > status is correct and reset PHY if necessary." By calling > > phy_restart_aneg() you don't reset the PHY. > > > > Moreover if "PS" flag is set, then the MAC has been pre-configured to > > work in the 10/100Mbps mode. Since 1000Mbps speed is requested, the > > MAC_CTRL_REG.PS flag will be cleared later in the > > stmmac_mac_link_up() method and then phylink_start() shall cause the > > link speed re-auto-negotiation. Why do you need the auto-negotiation > > started for the default MAC config which will be changed just in a > > moment later? All of that seems weird. > > When switching speeds (from 100M to 1000M), the phy cannot output clocks, > > resulting in the unavailability of the network card. At this time, a reset > of the > > phy is required. reset, or restart of autoneg? > BTW, This bug has been fixed in gnet of 2k2000 (0x10, 7a13). > > > > > Most importantly I have doubts the networking subsystem maintainers > > will permit you calling the phy_restart_aneg() method from the MAC > > driver code. That is O.K. It should have a comment explaining that it is working around a hardware bug. And you need to take care of locking. But a MAC driver can call this, e.g. if it implements ethtool nway_reset, it needs to do exactly this. See phy_ethtool_nway_reset(). Andrew
On Thu, Mar 21, 2024 at 04:02:54PM +0100, Andrew Lunn wrote: > On Thu, Mar 21, 2024 at 05:29:55PM +0800, Yanteng Si wrote: > > > > 在 2024/3/20 01:02, Serge Semin 写道: > > > > > Due to a bug in the chip's internal PHY, the network is still not working after > > > > > the first self-negotiation, and it needs to be self-negotiated again. > > > Then please describe the bug in more details then. > > > > > > Getting back to the code you implemented here. In the in-situ comment > > > you say: "We need to use the PS bit to check if the controller's > > > status is correct and reset PHY if necessary." By calling > > > phy_restart_aneg() you don't reset the PHY. > > > > > > Moreover if "PS" flag is set, then the MAC has been pre-configured to > > > work in the 10/100Mbps mode. Since 1000Mbps speed is requested, the > > > MAC_CTRL_REG.PS flag will be cleared later in the > > > stmmac_mac_link_up() method and then phylink_start() shall cause the > > > link speed re-auto-negotiation. Why do you need the auto-negotiation > > > started for the default MAC config which will be changed just in a > > > moment later? All of that seems weird. > > > > When switching speeds (from 100M to 1000M), the phy cannot output clocks, > > > > resulting in the unavailability of the network card. At this time, a reset > > of the > > > > phy is required. > > reset, or restart of autoneg? > > > BTW, This bug has been fixed in gnet of 2k2000 (0x10, 7a13). > > > > > > > > Most importantly I have doubts the networking subsystem maintainers > > > will permit you calling the phy_restart_aneg() method from the MAC > > > driver code. > > That is O.K. It should have a comment explaining that it is working > around a hardware bug. And you need to take care of locking. But a MAC > driver can call this, e.g. if it implements ethtool nway_reset, it > needs to do exactly this. See phy_ethtool_nway_reset(). However, because stmmac uses phylink, we should be adding phylink interfaces that forward to phylib to avoid the layering violation.
> However, because stmmac uses phylink, we should be adding phylink > interfaces that forward to phylib to avoid the layering violation. Yes. Maybe just call phylink_ethtool_nway_reset()? Just depends if the additional phylink_pcs_an_restart(pl) will mess things up for this device. Andrew
在 2024/3/21 23:02, Andrew Lunn 写道: >> When switching speeds (from 100M to 1000M), the phy cannot output clocks, >> >> resulting in the unavailability of the network card. At this time, a reset >> of the >> >> phy is required. > reset, or restart of autoneg? reset. Thanks, Yanteng
On Tue, Mar 26, 2024 at 08:02:55PM +0800, Yanteng Si wrote: > > 在 2024/3/21 23:02, Andrew Lunn 写道: > > > When switching speeds (from 100M to 1000M), the phy cannot output clocks, > > > > > > resulting in the unavailability of the network card. At this time, a reset > > > of the > > > > > > phy is required. > > reset, or restart of autoneg? > > reset. If you need a reset, why are you asking it to restart auto-neg? Andrew
Hi Andrew, Serge and Russell, 在 2024/3/21 23:38, Andrew Lunn 写道: >> However, because stmmac uses phylink, we should be adding phylink >> interfaces that forward to phylib to avoid the layering violation. > Yes. > > Maybe just call phylink_ethtool_nway_reset()? Just depends if the Nice! It works. In fact, after repeated testing, my previous code did not fix all issues, such as network unavailability when switching from 1000M to 100M(physical). But your method is perfect! Thanks, Yanteng > additional phylink_pcs_an_restart(pl) will mess things up for this > device. > > Andrew
在 2024/3/26 20:21, Andrew Lunn 写道: > On Tue, Mar 26, 2024 at 08:02:55PM +0800, Yanteng Si wrote: >> 在 2024/3/21 23:02, Andrew Lunn 写道: >>>> When switching speeds (from 100M to 1000M), the phy cannot output clocks, >>>> >>>> resulting in the unavailability of the network card. At this time, a reset >>>> of the >>>> >>>> phy is required. >>> reset, or restart of autoneg? >> reset. > If you need a reset, why are you asking it to restart auto-neg? Autoneg was discussed in patch v1, but we may have misunderstood the description from our hardware engineers at the time. The root cause is that there is an error in the connection between the MAC and PHY. After repeated tests, we have found that auto-negcannot solve all problems and can only be reset. Thanks, Yanteng
On Wed, Mar 27, 2024 at 10:41:57AM +0800, Yanteng Si wrote: > > 在 2024/3/26 20:21, Andrew Lunn 写道: > > On Tue, Mar 26, 2024 at 08:02:55PM +0800, Yanteng Si wrote: > > > 在 2024/3/21 23:02, Andrew Lunn 写道: > > > > > When switching speeds (from 100M to 1000M), the phy cannot output clocks, > > > > > > > > > > resulting in the unavailability of the network card. At this time, a reset > > > > > of the > > > > > > > > > > phy is required. > > > > reset, or restart of autoneg? > > > reset. > > If you need a reset, why are you asking it to restart auto-neg? > Autoneg was discussed in patch v1, but we may have misunderstood the > description from our hardware engineers at the time. The root cause is that > there is an error in the connection between the MAC and PHY. After repeated > tests, we have found that > > auto-negcannot solve all problems and can only be reset. Thanks, Yanteng So calling phylink_ethtool_nway_reset() does not fix your problem, and you need some other fix. Andrew --- pw-bot: cr
在 2024/3/27 20:47, Andrew Lunn 写道: > On Wed, Mar 27, 2024 at 10:41:57AM +0800, Yanteng Si wrote: >> 在 2024/3/26 20:21, Andrew Lunn 写道: >>> On Tue, Mar 26, 2024 at 08:02:55PM +0800, Yanteng Si wrote: >>>> 在 2024/3/21 23:02, Andrew Lunn 写道: >>>>>> When switching speeds (from 100M to 1000M), the phy cannot output clocks, >>>>>> >>>>>> resulting in the unavailability of the network card. At this time, a reset >>>>>> of the >>>>>> >>>>>> phy is required. >>>>> reset, or restart of autoneg? >>>> reset. >>> If you need a reset, why are you asking it to restart auto-neg? >> Autoneg was discussed in patch v1, but we may have misunderstood the >> description from our hardware engineers at the time. The root cause is that >> there is an error in the connection between the MAC and PHY. After repeated >> tests, we have found that >> >> auto-negcannot solve all problems and can only be reset. Thanks, Yanteng > > So calling phylink_ethtool_nway_reset() does not fix your problem, and > you need some other fix. > Sorry, it's my fault. I was stupid. Let me explain the whole story: First, here is the original code we got: if (speed == SPEED_1000) { if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) { /* reset phy */ phy_set_bits(ndev->phydev, 0 /*MII_BMCR*/, 0x200 /*BMCR_ANRESTART*/); Although the comment in the code is **reset phy**, the actual operation is to modify the autoneg bit. This method fixes all problems. V1: if (phydev->speed == SPEED_1000) phydev->autoneg = AUTONEG_ENABLE After discussing with you, see: <https://lore.kernel.org/loongarch/be1874e517f4f4cc50906f18689a0add3594c2e0.1689215889.git.chenfeiyang@loongson.cn/> v2 -> v8: if (speed == SPEED_1000) if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) phy_restart_aneg(ndev->phydev); Subsequent versions have been using this method. In recent days, I accidentally conducted an erroneous test, possibly due to the network cable not being firmly connected, leading me to mistakenly believe that this did not fix all problems. After conducting sufficient testing today, I found that I was mistaken and that calling phy_restart_aneg() directly fixed all problems. At your suggestion, calling phylink_et Athletic_nway_reset() also works well, but it will report an assertion failed at drivers/net/phy/phylink.c in the dmesg. So let's continue to call phy_restart_aneg()? Finally, why did I reply you with reset? It's because I first checked the original code, and the comment in the original code was /* reset phy */. I thought it was reset, but today I found out that it was actually autoneg. I apologize for my carelessness. Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 60d0a122d7c9..264c4c198d5a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -344,6 +344,21 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { .config = loongson_gmac_config, }; +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, unsigned int mode) +{ + struct loongson_data *ld = (struct loongson_data *)priv; + struct net_device *ndev = dev_get_drvdata(ld->dev); + struct stmmac_priv *ptr = netdev_priv(ndev); + + /* The controller and PHY don't work well together. + * We need to use the PS bit to check if the controller's status + * is correct and reset PHY if necessary. + */ + if (speed == SPEED_1000) + if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */) + phy_restart_aneg(ndev->phydev); +} + static struct mac_device_info *loongson_setup(void *apriv) { struct stmmac_priv *priv = apriv; @@ -401,6 +416,7 @@ static int loongson_gnet_data(struct pci_dev *pdev, plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; plat->bsp_priv = &pdev->dev; + plat->fix_mac_speed = loongson_gnet_fix_speed; plat->dma_cfg->pbl = 32; plat->dma_cfg->pblx8 = true; @@ -416,6 +432,9 @@ static int loongson_gnet_config(struct pci_dev *pdev, struct stmmac_resources *res, struct device_node *np) { + if (pdev->revision == 0x00 || pdev->revision == 0x01) + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; + return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 42d27b97dd1d..31068fbc23c9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, return 0; } + if (FIELD_GET(STMMAC_FLAG_DISABLE_FORCE_1000, priv->plat->flags)) { + if (cmd->base.speed == SPEED_1000 && + cmd->base.autoneg != AUTONEG_ENABLE) + return -EOPNOTSUPP; + } + return phylink_ethtool_ksettings_set(priv->phylink, cmd); } diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index dee5ad6e48c5..2810361e4048 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -221,6 +221,7 @@ struct dwmac4_addrs { #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) struct plat_stmmacenet_data { int bus_id;