| Message ID | 3382be108772ce56fe3e9bb99c9c53b7e9cd6bad.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:49:14PM +0800, Yanteng Si wrote: > Current GNET does not support half duplex mode. > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- > include/linux/stmmac.h | 1 + > 3 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index 264c4c198d5a..1753a3c46b77 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, > struct stmmac_resources *res, > struct device_node *np) > { > - if (pdev->revision == 0x00 || pdev->revision == 0x01) > + switch (pdev->revision) { > + case 0x00: > + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | > + STMMAC_FLAG_DISABLE_HALF_DUPLEX; > + break; > + case 0x01: > plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > + break; > + default: > + break; > + } Move this change into 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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 5617b40abbe4..3aa862269eb0 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) > static void stmmac_set_half_duplex(struct stmmac_priv *priv) > { > /* Half-Duplex can only work with single tx queue */ > - if (priv->plat->tx_queues_to_use > 1) > + if (priv->plat->tx_queues_to_use > 1 || > + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) > priv->phylink_config.mac_capabilities &= > ~(MAC_10HD | MAC_100HD | MAC_1000HD); > else > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index 2810361e4048..197f6f914104 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -222,6 +222,7 @@ struct dwmac4_addrs { > #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) > +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) > Place the patch with this change before [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support as a pre-requisite/preparation patch. Don't forget a thorough description of what is wrong with the GNET Half-Duplex mode. -Serge(y) > struct plat_stmmacenet_data { > int bus_id; > -- > 2.31.4 >
On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote: > On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote: > > Current GNET does not support half duplex mode. > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > > --- > > drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++- > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- > > include/linux/stmmac.h | 1 + > > 3 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > index 264c4c198d5a..1753a3c46b77 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, > > struct stmmac_resources *res, > > struct device_node *np) > > { > > > - if (pdev->revision == 0x00 || pdev->revision == 0x01) > > + switch (pdev->revision) { > > + case 0x00: > > + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | > > + STMMAC_FLAG_DISABLE_HALF_DUPLEX; > > + break; > > + case 0x01: > > plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > > + break; > > + default: > > + break; > > + } > > Move this change into 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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 5617b40abbe4..3aa862269eb0 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) > > static void stmmac_set_half_duplex(struct stmmac_priv *priv) > > { > > > /* Half-Duplex can only work with single tx queue */ > > - if (priv->plat->tx_queues_to_use > 1) > > + if (priv->plat->tx_queues_to_use > 1 || > > + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) > > priv->phylink_config.mac_capabilities &= > > ~(MAC_10HD | MAC_100HD | MAC_1000HD); > > else > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > > index 2810361e4048..197f6f914104 100644 > > --- a/include/linux/stmmac.h > > +++ b/include/linux/stmmac.h > > @@ -222,6 +222,7 @@ struct dwmac4_addrs { > > #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) > > +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) > > > > Place the patch with this change before > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > as a pre-requisite/preparation patch. Don't forget a thorough > description of what is wrong with the GNET Half-Duplex mode. BTW what about re-defining the stmmac_ops.phylink_get_caps() callback instead of adding fixup flags in this patch and in the next one? -Serge() > > -Serge(y) > > > struct plat_stmmacenet_data { > > int bus_id; > > -- > > 2.31.4 > >
在 2024/2/6 06:06, Serge Semin 写道: > On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote: >> On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote: >>> Current GNET does not support half duplex mode. >>> >>> Signed-off-by: Yanteng Si <siyanteng@loongson.cn> >>> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> >>> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> >>> --- >>> drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++- >>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- >>> include/linux/stmmac.h | 1 + >>> 3 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>> index 264c4c198d5a..1753a3c46b77 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>> @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, >>> struct stmmac_resources *res, >>> struct device_node *np) >>> { >>> - if (pdev->revision == 0x00 || pdev->revision == 0x01) >>> + switch (pdev->revision) { >>> + case 0x00: >>> + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | >>> + STMMAC_FLAG_DISABLE_HALF_DUPLEX; >>> + break; >>> + case 0x01: >>> plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; >>> + break; >>> + default: >>> + break; >>> + } >> Move this change into 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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> index 5617b40abbe4..3aa862269eb0 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) >>> static void stmmac_set_half_duplex(struct stmmac_priv *priv) >>> { >>> /* Half-Duplex can only work with single tx queue */ >>> - if (priv->plat->tx_queues_to_use > 1) >>> + if (priv->plat->tx_queues_to_use > 1 || >>> + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) >>> priv->phylink_config.mac_capabilities &= >>> ~(MAC_10HD | MAC_100HD | MAC_1000HD); >>> else >>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >>> index 2810361e4048..197f6f914104 100644 >>> --- a/include/linux/stmmac.h >>> +++ b/include/linux/stmmac.h >>> @@ -222,6 +222,7 @@ struct dwmac4_addrs { >>> #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) >>> +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) >>> >> Place the patch with this change before >> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >> as a pre-requisite/preparation patch. Don't forget a thorough >> description of what is wrong with the GNET Half-Duplex mode. > BTW what about re-defining the stmmac_ops.phylink_get_caps() callback > instead of adding fixup flags in this patch and in the next one? ok. diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index ac1b48ff7199..b57e1325ce62 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -238,6 +234,13 @@ static int loongson_gnet_get_hw_feature(void __iomem *ioaddr, return 0; } +static void loongson_phylink_get_caps(struct stmmac_priv *priv) +{ + priv->phylink_config.mac_capabilities = (MAC_10FD | + MAC_100FD | MAC_1000FD) & ~(MAC_10HD | MAC_100HD | MAC_1000HD); + +} + struct stmmac_pci_info { int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat); int (*config)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, @@ -403,10 +405,15 @@ static void loongson_gnet_fix_speed(void *priv, unsigned int speed, unsigned int static struct mac_device_info *loongson_setup(void *apriv) { + struct stmmac_ops *loongson_dwmac_ops; struct stmmac_priv *priv = apriv; struct mac_device_info *mac; struct stmmac_dma_ops *dma; + loongson_dwmac_ops = devm_kzalloc(priv->device, sizeof(*loongson_dwmac_ops), GFP_KERNEL); + if (!loongson_dwmac_ops) + return NULL; + mac = devm_kzalloc(priv->device, sizeof(*mac), GFP_KERNEL); if (!mac) return NULL; @@ -417,6 +424,10 @@ static struct mac_device_info *loongson_setup(void *apriv) priv->synopsys_id = 0x37; /*Overwrite custom IP*/ + *loongson_dwmac_ops = dwmac1000_ops; + loongson_dwmac_ops->phylink_get_caps = loongson_phylink_get_caps; + mac->mac = loongson_dwmac_ops; + *dma = dwmac1000_dma_ops; dma->init_chan = loongson_gnet_dma_init_channel; dma->dma_interrupt = loongson_gnet_dma_interrupt; @@ -375,8 +375,6 @@ static int loongson_gmac_config(struct pci_dev *pdev, struct stmmac_resources *res, struct device_node *np) { - plat->flags |= STMMAC_FLAG_DISABLE_FLOW_CONTROL; - return 0; } @@ -489,17 +487,7 @@ static int loongson_gnet_config(struct pci_dev *pdev, struct stmmac_resources *res, struct device_node *np) { - switch (pdev->revision) { - case 0x00: - plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | - STMMAC_FLAG_DISABLE_HALF_DUPLEX; - break; - case 0x01: - plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; - break; - default: - break; - } + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 8705e04913d1..7c656f970575 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1201,8 +1201,7 @@ static int stmmac_init_phy(struct net_device *dev) static void stmmac_set_half_duplex(struct stmmac_priv *priv) { /* Half-Duplex can only work with single tx queue */ - if (priv->plat->tx_queues_to_use > 1 || - (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) + if (priv->plat->tx_queues_to_use > 1) priv->phylink_config.mac_capabilities &= ~(MAC_10HD | MAC_100HD | MAC_1000HD); else @@ -1237,9 +1236,9 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) xpcs_get_interfaces(priv->hw->xpcs, priv->phylink_config.supported_interfaces); - priv->phylink_config.mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD; - if (!(priv->plat->flags & STMMAC_FLAG_DISABLE_FLOW_CONTROL)) - priv->phylink_config.mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE; + priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10FD | MAC_100FD | + MAC_1000FD; stmmac_set_half_duplex(priv); ----- loongson_gmac/gnet_config() I will try to remove these two functions according to your comment in another patch。 Thanks, Yanteng > > -Serge() > >> -Serge(y) >> >>> struct plat_stmmacenet_data { >>> int bus_id; >>> -- >>> 2.31.4 >>>
On Wed, Mar 13, 2024 at 05:24:52PM +0800, Yanteng Si wrote: > 在 2024/2/6 06:06, Serge Semin 写道: > > On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote: > > > On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote: > > > > Current GNET does not support half duplex mode. > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > > > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > > > > --- > > > > drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++- > > > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- > > > > include/linux/stmmac.h | 1 + > > > > 3 files changed, 13 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > index 264c4c198d5a..1753a3c46b77 100644 > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, > > > > struct stmmac_resources *res, > > > > struct device_node *np) > > > > { > > > > - if (pdev->revision == 0x00 || pdev->revision == 0x01) > > > > + switch (pdev->revision) { > > > > + case 0x00: > > > > + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | > > > > + STMMAC_FLAG_DISABLE_HALF_DUPLEX; > > > > + break; > > > > + case 0x01: > > > > plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > > > > + break; > > > > + default: > > > > + break; > > > > + } > > > Move this change into 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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > index 5617b40abbe4..3aa862269eb0 100644 > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) > > > > static void stmmac_set_half_duplex(struct stmmac_priv *priv) > > > > { > > > > /* Half-Duplex can only work with single tx queue */ > > > > - if (priv->plat->tx_queues_to_use > 1) > > > > + if (priv->plat->tx_queues_to_use > 1 || > > > > + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) > > > > priv->phylink_config.mac_capabilities &= > > > > ~(MAC_10HD | MAC_100HD | MAC_1000HD); > > > > else > > > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > > > > index 2810361e4048..197f6f914104 100644 > > > > --- a/include/linux/stmmac.h > > > > +++ b/include/linux/stmmac.h > > > > @@ -222,6 +222,7 @@ struct dwmac4_addrs { > > > > #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) > > > > +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) > > > Place the patch with this change before > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > > as a pre-requisite/preparation patch. Don't forget a thorough > > > description of what is wrong with the GNET Half-Duplex mode. > > BTW what about re-defining the stmmac_ops.phylink_get_caps() callback > > instead of adding fixup flags in this patch and in the next one? > > ok. > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index ac1b48ff7199..b57e1325ce62 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -238,6 +234,13 @@ static int loongson_gnet_get_hw_feature(void __iomem > *ioaddr, > return 0; > } > > +static void loongson_phylink_get_caps(struct stmmac_priv *priv) > +{ > + priv->phylink_config.mac_capabilities = (MAC_10FD | > + MAC_100FD | MAC_1000FD) & ~(MAC_10HD | MAC_100HD | MAC_1000HD); Why is this so complicated? It would be silly if the _full duplex_ definitions also defined the _half duplex_ bits. This should be just: priv->phylink_config.mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD; if that is all you support. Do you not support any pause modes (they would need to be included as well here.) As to this approach, I don't think it's a good model to override the stmmac MAC operations. Instead, I would suggest that a better approach would be for the platform to provide its capabilities to the stmmac core code (maybe a new member in stmmac_priv) which, when set, is used to reduce the capabilities provided to phylink via priv->phylink_config.mac_capabilities. Why? The driver has several components that are involved in the overall capabilities, and the capabilities of the system is the logical subset of all these capabilities. One component should not be setting capabilities that a different component doesn't support.
在 2024/3/13 18:21, Russell King (Oracle) 写道: > On Wed, Mar 13, 2024 at 05:24:52PM +0800, Yanteng Si wrote: >> 在 2024/2/6 06:06, Serge Semin 写道: >>> On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote: >>>> On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote: >>>>> Current GNET does not support half duplex mode. >>>>> >>>>> Signed-off-by: Yanteng Si <siyanteng@loongson.cn> >>>>> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> >>>>> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> >>>>> --- >>>>> drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++- >>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- >>>>> include/linux/stmmac.h | 1 + >>>>> 3 files changed, 13 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> index 264c4c198d5a..1753a3c46b77 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, >>>>> struct stmmac_resources *res, >>>>> struct device_node *np) >>>>> { >>>>> - if (pdev->revision == 0x00 || pdev->revision == 0x01) >>>>> + switch (pdev->revision) { >>>>> + case 0x00: >>>>> + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | >>>>> + STMMAC_FLAG_DISABLE_HALF_DUPLEX; >>>>> + break; >>>>> + case 0x01: >>>>> plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; >>>>> + break; >>>>> + default: >>>>> + break; >>>>> + } >>>> Move this change into 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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> index 5617b40abbe4..3aa862269eb0 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) >>>>> static void stmmac_set_half_duplex(struct stmmac_priv *priv) >>>>> { >>>>> /* Half-Duplex can only work with single tx queue */ >>>>> - if (priv->plat->tx_queues_to_use > 1) >>>>> + if (priv->plat->tx_queues_to_use > 1 || >>>>> + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) >>>>> priv->phylink_config.mac_capabilities &= >>>>> ~(MAC_10HD | MAC_100HD | MAC_1000HD); >>>>> else >>>>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >>>>> index 2810361e4048..197f6f914104 100644 >>>>> --- a/include/linux/stmmac.h >>>>> +++ b/include/linux/stmmac.h >>>>> @@ -222,6 +222,7 @@ struct dwmac4_addrs { >>>>> #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) >>>>> +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) >>>> Place the patch with this change before >>>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support >>>> as a pre-requisite/preparation patch. Don't forget a thorough >>>> description of what is wrong with the GNET Half-Duplex mode. >>> BTW what about re-defining the stmmac_ops.phylink_get_caps() callback >>> instead of adding fixup flags in this patch and in the next one? >> ok. >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> index ac1b48ff7199..b57e1325ce62 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> @@ -238,6 +234,13 @@ static int loongson_gnet_get_hw_feature(void __iomem >> *ioaddr, >> return 0; >> } Hi Russell, >> +static void loongson_phylink_get_caps(struct stmmac_priv *priv) >> +{ >> + priv->phylink_config.mac_capabilities = (MAC_10FD | >> + MAC_100FD | MAC_1000FD) & ~(MAC_10HD | MAC_100HD | MAC_1000HD); > Why is this so complicated? It would be silly if the _full duplex_ > definitions also defined the _half duplex_ bits. This should be just: > > priv->phylink_config.mac_capabilities = MAC_10FD | MAC_100FD | > MAC_1000FD; Yes, you are right. Our gnet device (7a2000) does not support half-duplex, while the gnet device (2k2000) does. I plan to use PCI IDand IP CORE as the condition to separate full-duplex and half-duplex. > > if that is all you support. Do you not support any pause modes (they > would need to be included as well here.) I have tested it and our gnet device supports MAC_ASYM_PAUSE and MAC_SYM_PAUSE, but gmac does not. I will fix this in the patch v9. This is also easy to do because all GMAC devices have the same PCI ID. > > As to this approach, I don't think it's a good model to override the > stmmac MAC operations. Instead, I would suggest that a better approach > would be for the platform to provide its capabilities to the stmmac > core code (maybe a new member in stmmac_priv) which, when set, is used > to reduce the capabilities provided to phylink via > priv->phylink_config.mac_capabilities. > > Why? The driver has several components that are involved in the > overall capabilities, and the capabilities of the system is the > logical subset of all these capabilities. One component should not > be setting capabilities that a different component doesn't support. Hi Serge, It seems to be going back again, what do you think? Thanks, Yanteng >
On Thu, Mar 14, 2024 at 09:08:41PM +0800, Yanteng Si wrote: > > 在 2024/3/13 18:21, Russell King (Oracle) 写道: > > On Wed, Mar 13, 2024 at 05:24:52PM +0800, Yanteng Si wrote: > > > 在 2024/2/6 06:06, Serge Semin 写道: > > > > On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote: > > > > > On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote: > > > > > > Current GNET does not support half duplex mode. > > > > > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > > > > > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > > > > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > > > > > > --- > > > > > > drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++- > > > > > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- > > > > > > include/linux/stmmac.h | 1 + > > > > > > 3 files changed, 13 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > > > index 264c4c198d5a..1753a3c46b77 100644 > > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > > > @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, > > > > > > struct stmmac_resources *res, > > > > > > struct device_node *np) > > > > > > { > > > > > > - if (pdev->revision == 0x00 || pdev->revision == 0x01) > > > > > > + switch (pdev->revision) { > > > > > > + case 0x00: > > > > > > + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | > > > > > > + STMMAC_FLAG_DISABLE_HALF_DUPLEX; > > > > > > + break; > > > > > > + case 0x01: > > > > > > plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > > > > > > + break; > > > > > > + default: > > > > > > + break; > > > > > > + } > > > > > Move this change into 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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > index 5617b40abbe4..3aa862269eb0 100644 > > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) > > > > > > static void stmmac_set_half_duplex(struct stmmac_priv *priv) > > > > > > { > > > > > > /* Half-Duplex can only work with single tx queue */ > > > > > > - if (priv->plat->tx_queues_to_use > 1) > > > > > > + if (priv->plat->tx_queues_to_use > 1 || > > > > > > + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) > > > > > > priv->phylink_config.mac_capabilities &= > > > > > > ~(MAC_10HD | MAC_100HD | MAC_1000HD); > > > > > > else > > > > > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > > > > > > index 2810361e4048..197f6f914104 100644 > > > > > > --- a/include/linux/stmmac.h > > > > > > +++ b/include/linux/stmmac.h > > > > > > @@ -222,6 +222,7 @@ struct dwmac4_addrs { > > > > > > #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) > > > > > > +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) > > > > > Place the patch with this change before > > > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support > > > > > as a pre-requisite/preparation patch. Don't forget a thorough > > > > > description of what is wrong with the GNET Half-Duplex mode. > > > > BTW what about re-defining the stmmac_ops.phylink_get_caps() callback > > > > instead of adding fixup flags in this patch and in the next one? > > > ok. > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > index ac1b48ff7199..b57e1325ce62 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > @@ -238,6 +234,13 @@ static int loongson_gnet_get_hw_feature(void __iomem > > > *ioaddr, > > > return 0; > > > } > > Hi Russell, > > > > +static void loongson_phylink_get_caps(struct stmmac_priv *priv) > > > +{ > > > + priv->phylink_config.mac_capabilities = (MAC_10FD | > > > + MAC_100FD | MAC_1000FD) & ~(MAC_10HD | MAC_100HD | MAC_1000HD); > > Why is this so complicated? It would be silly if the _full duplex_ > > definitions also defined the _half duplex_ bits. This should be just: > > > > priv->phylink_config.mac_capabilities = MAC_10FD | MAC_100FD | > > MAC_1000FD; > > Yes, you are right. Our gnet device (7a2000) does not support half-duplex, > while the gnet device (2k2000) does. > > I plan to use PCI IDand IP CORE as the condition to separate full-duplex and > half-duplex. > > > > > if that is all you support. Do you not support any pause modes (they > > would need to be included as well here.) > > I have tested it and our gnet device supports MAC_ASYM_PAUSE and > MAC_SYM_PAUSE, but gmac does not. I will fix this in the patch v9. > > This is also easy to do because all GMAC devices have the same PCI ID. > > > > > As to this approach, I don't think it's a good model to override the > > stmmac MAC operations. Instead, I would suggest that a better approach > > would be for the platform to provide its capabilities to the stmmac > > core code (maybe a new member in stmmac_priv) which, when set, is used > > to reduce the capabilities provided to phylink via > > priv->phylink_config.mac_capabilities. > > > > Why? The driver has several components that are involved in the > > overall capabilities, and the capabilities of the system is the > > logical subset of all these capabilities. One component should not > > be setting capabilities that a different component doesn't support. In general you are right for sure - it's better to avoid one part setting capabilities and another part unsetting them at least from the readability and maintainability point of view. But in this case we've already got implemented a ready-to-use internal interface stmmac_ops::phylink_get_caps() which can be used to extend/reduce the capabilities field based on the particular MAC abilities. Moreover it's called right from the component setting the capabilities. Are you saying that the callback is supposed to be utilized for extending the capabilities only? If you insist on not overriding the stmmac_ops::phylink_get_caps() anyway then please explain what is the principal difference between the next two code snippets: /* Get the MAC specific capabilities */ stmmac_mac_phylink_get_caps(priv); and priv->phylink_config.mac_capabilities &= ~priv->plat->mac_caps_mask; in the MAC-capabilities update implementation? Do you think the later approach would be more descriptive? If so then would the callback-based approach almost equally descriptive if the callback name was, suppose, stmmac_mac_phylink_set_caps() or similar? In anyway I am sure the approach suggested in the initial patch of this thread isn't good since it motivates the developers to implement more-and-more DW MAC-specific platform capabilities flags fixing another flags, which makes the generic code even more complicated than it already is with endless if-else-plat-flags statements. -Serge(y) > > Hi Serge, > > It seems to be going back again, what do you think? > > Thanks, > > Yanteng > > > > >
Serge, Again, please learn to trim your replies. On Wed, Mar 20, 2024 at 01:10:04PM +0300, Serge Semin wrote: > > > As to this approach, I don't think it's a good model to override the > > > stmmac MAC operations. Instead, I would suggest that a better approach > > > would be for the platform to provide its capabilities to the stmmac > > > core code (maybe a new member in stmmac_priv) which, when set, is used > > > to reduce the capabilities provided to phylink via > > > priv->phylink_config.mac_capabilities. > > > > > > Why? The driver has several components that are involved in the > > > overall capabilities, and the capabilities of the system is the > > > logical subset of all these capabilities. One component should not > > > be setting capabilities that a different component doesn't support. > > In general you are right for sure - it's better to avoid one part > setting capabilities and another part unsetting them at least from the > readability and maintainability point of view. But in this case we've > already got implemented a ready-to-use internal interface > stmmac_ops::phylink_get_caps() which can be used to extend/reduce the > capabilities field based on the particular MAC abilities. Moreover > it's called right from the component setting the capabilities. Are you > saying that the callback is supposed to be utilized for extending the > capabilities only? What concerns me is that the proposed code _overwrites_ the capabilities from the MAC layer, so from a maintanability point of view it's a nightmare, because you will now have the situation where MACs provide their capabilities, and then platform code may overwrite it - which means it's like a spiders web trying to work out what capabilities are provided. The reality is surely that the MAC dictates what it can do, but there may be further restrictions by other components in the platform, so the capabilities provided to phylink should be: mac_capabilities & platform_capabilities And what I'm proposing is that _that_ should be done in a way that makes it _easy_ for the platform code to get right. Overriding stmmac_ops::phylink_get_caps() doesn't do that - as can be seen in the proposed patch. Help your users write correct code by adopting a structure that makes it easy for them to do the right thing. > If you insist on not overriding the stmmac_ops::phylink_get_caps() > anyway then please explain what is the principal difference > between the next two code snippets: > /* Get the MAC specific capabilities */ > stmmac_mac_phylink_get_caps(priv); > and > priv->phylink_config.mac_capabilities &= ~priv->plat->mac_caps_mask; I was thinking: stmmac_mac_phylink_get_caps(priv); if (priv->plat->mac_capabilities) priv->phylink_config.mac_capabilities &= priv->plat->mac_capabilities; In other words, if a platform sets plat->mac_capabilities, then it is providing the capabilities that it supports, and those need to reduce the capabilities provided by the MAC. This will _also_ allow stmmac_set_half_duplex() to do the right thing. Consider something in the platform side that doesn't allow half-duplex, but allows tx_queues_to_use == 1. That'll set the half-duplex modes when stmmac_set_half_duplex() is called, overriding what the platform supports. Now that I look at the stmmac implementation, there's even more that is wrong. Consider plat->max_speed = 100, like arch/arc/boot/dts/axs10x_mb.dtsi sets. If stmmac_set_half_duplex() is called as it can be from stmmac_reinit_queues(), it'll enable 1000 half-duplex, despite the plat->max_speed = 100. > in the MAC-capabilities update implementation? Do you think the later > approach would be more descriptive? If so then would the > callback-based approach almost equally descriptive if the callback > name was, suppose, stmmac_mac_phylink_set_caps() or similar? From what I can see of the existing stmmac MAC phylink_get_caps implementations, there seem to be two - xgmac_phylink_get_caps() and dwmac4_phylink_get_caps(). Both of these merely set additional modes in priv->phylink_config.mac_capabilities. Is there a reason to have this as an instruction stream, rather than providing data to the core stmmac code from the MAC about its capabilities? Is there a reason why it would be necessary for the code in a MAC backend to make a decision about what capabilities to enable based on some condition? > In anyway I am sure the approach suggested in the initial patch of > this thread isn't good since it motivates the developers to implement > more-and-more DW MAC-specific platform capabilities flags fixing > another flags, which makes the generic code even more complicated > than it already is with endless if-else-plat-flags statements. Yes, I do agree with that.
On Wed, Mar 20, 2024 at 10:37:00AM +0000, Russell King (Oracle) wrote: > I was thinking: > > stmmac_mac_phylink_get_caps(priv); > > if (priv->plat->mac_capabilities) > priv->phylink_config.mac_capabilities &= > priv->plat->mac_capabilities; > > In other words, if a platform sets plat->mac_capabilities, then it is > providing the capabilities that it supports, and those need to reduce > the capabilities provided by the MAC. To further expand on this given the additional discussion, here's a patch that amagamates the ideas so far - however, it doesn't implement everything. I think an additional step would be to provide a function that does all the mac capability calculations, something like: static void stmmac_calculate_mac_capabilities(struct stmmac_priv *priv) { struct phylink_config cfg; /* do everything with cfg.mac_capabilities that is currently in * stmmac_phy_setup() */ /* this must be done with rtnl held if the device is open */ priv->phylink_config.mac_capabilities = cfg.mac_capabilities; } and that needs to be called from both stmmac_reinit_queues() and stmmac_phy_setup(). diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index a6fefe675ef1..a1e144b99213 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -605,6 +605,7 @@ struct mac_device_info { unsigned int pmt; unsigned int ps; unsigned int xlgmac; + u32 phylink_mac_capabilities; unsigned int num_vlan; u32 vlan_filter[32]; bool vlan_fail_q_en; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index 6b6d0de09619..2e4da6ac5173 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c @@ -68,11 +68,6 @@ static void dwmac4_core_init(struct mac_device_info *hw, init_waitqueue_head(&priv->tstamp_busy_wait); } -static void dwmac4_phylink_get_caps(struct stmmac_priv *priv) -{ - priv->phylink_config.mac_capabilities |= MAC_2500FD; -} - static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u8 mode, u32 queue) { @@ -1165,7 +1160,6 @@ static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw) const struct stmmac_ops dwmac4_ops = { .core_init = dwmac4_core_init, - .phylink_get_caps = dwmac4_phylink_get_caps, .set_mac = stmmac_set_mac, .rx_ipc = dwmac4_rx_ipc_enable, .rx_queue_enable = dwmac4_rx_queue_enable, @@ -1210,7 +1204,6 @@ const struct stmmac_ops dwmac4_ops = { const struct stmmac_ops dwmac410_ops = { .core_init = dwmac4_core_init, - .phylink_get_caps = dwmac4_phylink_get_caps, .set_mac = stmmac_dwmac4_set_mac, .rx_ipc = dwmac4_rx_ipc_enable, .rx_queue_enable = dwmac4_rx_queue_enable, @@ -1259,7 +1252,6 @@ const struct stmmac_ops dwmac410_ops = { const struct stmmac_ops dwmac510_ops = { .core_init = dwmac4_core_init, - .phylink_get_caps = dwmac4_phylink_get_caps, .set_mac = stmmac_dwmac4_set_mac, .rx_ipc = dwmac4_rx_ipc_enable, .rx_queue_enable = dwmac4_rx_queue_enable, @@ -1372,5 +1364,7 @@ int dwmac4_setup(struct stmmac_priv *priv) mac->mii.clk_csr_mask = GENMASK(11, 8); mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr); + mac->phylink_mac_capabilities |= MAC_2500FD; + return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index 1af2f89a0504..f3daa284012b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -47,14 +47,6 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); } -static void xgmac_phylink_get_caps(struct stmmac_priv *priv) -{ - priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | - MAC_10000FD | MAC_25000FD | - MAC_40000FD | MAC_50000FD | - MAC_100000FD; -} - static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) { u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); @@ -1516,7 +1508,6 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg * const struct stmmac_ops dwxgmac210_ops = { .core_init = dwxgmac2_core_init, - .phylink_get_caps = xgmac_phylink_get_caps, .set_mac = dwxgmac2_set_mac, .rx_ipc = dwxgmac2_rx_ipc, .rx_queue_enable = dwxgmac2_rx_queue_enable, @@ -1577,7 +1568,6 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, const struct stmmac_ops dwxlgmac2_ops = { .core_init = dwxgmac2_core_init, - .phylink_get_caps = xgmac_phylink_get_caps, .set_mac = dwxgmac2_set_mac, .rx_ipc = dwxgmac2_rx_ipc, .rx_queue_enable = dwxlgmac2_rx_queue_enable, @@ -1656,6 +1646,11 @@ int dwxgmac2_setup(struct stmmac_priv *priv) mac->mii.clk_csr_shift = 19; mac->mii.clk_csr_mask = GENMASK(21, 19); + mac->phylink_mac_capabilities = MAC_2500FD | MAC_5000FD | + MAC_10000FD | MAC_25000FD | + MAC_40000FD | MAC_50000FD | + MAC_100000FD; + return 0; } @@ -1693,5 +1688,10 @@ int dwxlgmac2_setup(struct stmmac_priv *priv) mac->mii.clk_csr_shift = 19; mac->mii.clk_csr_mask = GENMASK(21, 19); + mac->phylink_mac_capabilities = MAC_2500FD | MAC_5000FD | + MAC_10000FD | MAC_25000FD | + MAC_40000FD | MAC_50000FD | + MAC_100000FD; + return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index 7be04b54738b..7370cd963569 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -308,8 +308,6 @@ struct stmmac_est; struct stmmac_ops { /* MAC core initialization */ void (*core_init)(struct mac_device_info *hw, struct net_device *dev); - /* Get phylink capabilities */ - void (*phylink_get_caps)(struct stmmac_priv *priv); /* Enable the MAC RX/TX */ void (*set_mac)(void __iomem *ioaddr, bool enable); /* Enable and verify that the IPC module is supported */ @@ -430,8 +428,6 @@ struct stmmac_ops { #define stmmac_core_init(__priv, __args...) \ stmmac_do_void_callback(__priv, mac, core_init, __args) -#define stmmac_mac_phylink_get_caps(__priv) \ - stmmac_do_void_callback(__priv, mac, phylink_get_caps, __priv) #define stmmac_mac_set(__priv, __args...) \ stmmac_do_void_callback(__priv, mac, set_mac, __args) #define stmmac_rx_ipc(__priv, __args...) \ diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 24cd80490d19..5e8cffc49f1f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1240,15 +1240,25 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) MAC_10FD | MAC_100FD | MAC_1000FD; - stmmac_set_half_duplex(priv); + /* Merge in the MAC capabilities */ + priv->phylink_config.mac_capabilities |= + priv->hw->phylink_mac_capabilities; - /* Get the MAC specific capabilities */ - stmmac_mac_phylink_get_caps(priv); + stmmac_set_half_duplex(priv); max_speed = priv->plat->max_speed; if (max_speed) phylink_limit_mac_speed(&priv->phylink_config, max_speed); + /* If the platform supplies MAC capabilities, calculate the union + * of the MAC and platform capabilities to give the whole-system + * capabilities. This intetionally can not add additional capabilities + * so if this is populated, it must list everything that is supported. + */ + if (priv->plat->phylink_mac_capabilities) + priv->phylink_config.mac_capabilities &= + priv->plat->phylink_mac_capabilities; + fwnode = priv->plat->port_node; if (!fwnode) fwnode = dev_fwnode(priv->device); diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index dfa1828cd756..fe3f64df17ac 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -272,6 +272,7 @@ struct plat_stmmacenet_data { u8 tx_sched_algorithm; struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES]; struct stmmac_txq_cfg tx_queues_cfg[MTL_MAX_TX_QUEUES]; + u32 phylink_mac_capabilities; void (*fix_mac_speed)(void *priv, unsigned int speed, unsigned int mode); int (*fix_soc_reset)(void *priv, void __iomem *ioaddr); int (*serdes_powerup)(struct net_device *ndev, void *priv);
On Wed, Mar 20, 2024 at 12:50:28PM +0000, Russell King (Oracle) wrote: > On Wed, Mar 20, 2024 at 10:37:00AM +0000, Russell King (Oracle) wrote: > > I was thinking: > > > > stmmac_mac_phylink_get_caps(priv); > > > > if (priv->plat->mac_capabilities) > > priv->phylink_config.mac_capabilities &= > > priv->plat->mac_capabilities; > > > > In other words, if a platform sets plat->mac_capabilities, then it is > > providing the capabilities that it supports, and those need to reduce > > the capabilities provided by the MAC. > > To further expand on this given the additional discussion, here's a > patch that amagamates the ideas so far - however, it doesn't > implement everything. > > I think an additional step would be to provide a function that does all > the mac capability calculations, something like: Here's the complete patch: diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index a6fefe675ef1..a1e144b99213 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -605,6 +605,7 @@ struct mac_device_info { unsigned int pmt; unsigned int ps; unsigned int xlgmac; + u32 phylink_mac_capabilities; unsigned int num_vlan; u32 vlan_filter[32]; bool vlan_fail_q_en; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index 6b6d0de09619..2e4da6ac5173 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c @@ -68,11 +68,6 @@ static void dwmac4_core_init(struct mac_device_info *hw, init_waitqueue_head(&priv->tstamp_busy_wait); } -static void dwmac4_phylink_get_caps(struct stmmac_priv *priv) -{ - priv->phylink_config.mac_capabilities |= MAC_2500FD; -} - static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u8 mode, u32 queue) { @@ -1165,7 +1160,6 @@ static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw) const struct stmmac_ops dwmac4_ops = { .core_init = dwmac4_core_init, - .phylink_get_caps = dwmac4_phylink_get_caps, .set_mac = stmmac_set_mac, .rx_ipc = dwmac4_rx_ipc_enable, .rx_queue_enable = dwmac4_rx_queue_enable, @@ -1210,7 +1204,6 @@ const struct stmmac_ops dwmac4_ops = { const struct stmmac_ops dwmac410_ops = { .core_init = dwmac4_core_init, - .phylink_get_caps = dwmac4_phylink_get_caps, .set_mac = stmmac_dwmac4_set_mac, .rx_ipc = dwmac4_rx_ipc_enable, .rx_queue_enable = dwmac4_rx_queue_enable, @@ -1259,7 +1252,6 @@ const struct stmmac_ops dwmac410_ops = { const struct stmmac_ops dwmac510_ops = { .core_init = dwmac4_core_init, - .phylink_get_caps = dwmac4_phylink_get_caps, .set_mac = stmmac_dwmac4_set_mac, .rx_ipc = dwmac4_rx_ipc_enable, .rx_queue_enable = dwmac4_rx_queue_enable, @@ -1372,5 +1364,7 @@ int dwmac4_setup(struct stmmac_priv *priv) mac->mii.clk_csr_mask = GENMASK(11, 8); mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr); + mac->phylink_mac_capabilities |= MAC_2500FD; + return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index 1af2f89a0504..f3daa284012b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -47,14 +47,6 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); } -static void xgmac_phylink_get_caps(struct stmmac_priv *priv) -{ - priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | - MAC_10000FD | MAC_25000FD | - MAC_40000FD | MAC_50000FD | - MAC_100000FD; -} - static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) { u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); @@ -1516,7 +1508,6 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg * const struct stmmac_ops dwxgmac210_ops = { .core_init = dwxgmac2_core_init, - .phylink_get_caps = xgmac_phylink_get_caps, .set_mac = dwxgmac2_set_mac, .rx_ipc = dwxgmac2_rx_ipc, .rx_queue_enable = dwxgmac2_rx_queue_enable, @@ -1577,7 +1568,6 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, const struct stmmac_ops dwxlgmac2_ops = { .core_init = dwxgmac2_core_init, - .phylink_get_caps = xgmac_phylink_get_caps, .set_mac = dwxgmac2_set_mac, .rx_ipc = dwxgmac2_rx_ipc, .rx_queue_enable = dwxlgmac2_rx_queue_enable, @@ -1656,6 +1646,11 @@ int dwxgmac2_setup(struct stmmac_priv *priv) mac->mii.clk_csr_shift = 19; mac->mii.clk_csr_mask = GENMASK(21, 19); + mac->phylink_mac_capabilities = MAC_2500FD | MAC_5000FD | + MAC_10000FD | MAC_25000FD | + MAC_40000FD | MAC_50000FD | + MAC_100000FD; + return 0; } @@ -1693,5 +1688,10 @@ int dwxlgmac2_setup(struct stmmac_priv *priv) mac->mii.clk_csr_shift = 19; mac->mii.clk_csr_mask = GENMASK(21, 19); + mac->phylink_mac_capabilities = MAC_2500FD | MAC_5000FD | + MAC_10000FD | MAC_25000FD | + MAC_40000FD | MAC_50000FD | + MAC_100000FD; + return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index 7be04b54738b..7370cd963569 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -308,8 +308,6 @@ struct stmmac_est; struct stmmac_ops { /* MAC core initialization */ void (*core_init)(struct mac_device_info *hw, struct net_device *dev); - /* Get phylink capabilities */ - void (*phylink_get_caps)(struct stmmac_priv *priv); /* Enable the MAC RX/TX */ void (*set_mac)(void __iomem *ioaddr, bool enable); /* Enable and verify that the IPC module is supported */ @@ -430,8 +428,6 @@ struct stmmac_ops { #define stmmac_core_init(__priv, __args...) \ stmmac_do_void_callback(__priv, mac, core_init, __args) -#define stmmac_mac_phylink_get_caps(__priv) \ - stmmac_do_void_callback(__priv, mac, phylink_get_caps, __priv) #define stmmac_mac_set(__priv, __args...) \ stmmac_do_void_callback(__priv, mac, set_mac, __args) #define stmmac_rx_ipc(__priv, __args...) \ diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 24cd80490d19..278532290f0c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1198,15 +1198,33 @@ static int stmmac_init_phy(struct net_device *dev) return ret; } -static void stmmac_set_half_duplex(struct stmmac_priv *priv) +static void stmmac_set_mac_capabilties(struct stmmac_priv *priv) { + struct phylink_config cfg; + + cfg.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10FD | MAC_100FD | MAC_1000FD | + priv->hw->phylink_mac_capabilities; + /* Half-Duplex can only work with single tx queue */ if (priv->plat->tx_queues_to_use > 1) - priv->phylink_config.mac_capabilities &= - ~(MAC_10HD | MAC_100HD | MAC_1000HD); + cfg.mac_capabilities &= ~(MAC_10HD | MAC_100HD | MAC_1000HD); else - priv->phylink_config.mac_capabilities |= - (MAC_10HD | MAC_100HD | MAC_1000HD); + cfg.mac_capabilities |= MAC_10HD | MAC_100HD | MAC_1000HD; + + max_speed = priv->plat->max_speed; + if (max_speed) + phylink_limit_mac_speed(&cfg, max_speed); + + /* If the platform supplies MAC capabilities, calculate the union + * of the MAC and platform capabilities to give the whole-system + * capabilities. This intetionally can not add additional capabilities + * so if this is populated, it must list everything that is supported. + */ + if (priv->plat->phylink_mac_capabilities) + cfg.mac_capabilities &= priv->plat->phylink_mac_capabilities; + + priv->phylink_config.mac_capabilities = cfg.mac_capabilities; } static int stmmac_phy_setup(struct stmmac_priv *priv) @@ -1236,18 +1254,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) xpcs_get_interfaces(priv->hw->xpcs, priv->phylink_config.supported_interfaces); - priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | - MAC_10FD | MAC_100FD | - MAC_1000FD; - - stmmac_set_half_duplex(priv); - - /* Get the MAC specific capabilities */ - stmmac_mac_phylink_get_caps(priv); - - max_speed = priv->plat->max_speed; - if (max_speed) - phylink_limit_mac_speed(&priv->phylink_config, max_speed); + stmmac_set_mac_capabilties(priv); fwnode = priv->plat->port_node; if (!fwnode) @@ -7355,7 +7362,7 @@ int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt) priv->rss.table[i] = ethtool_rxfh_indir_default(i, rx_cnt); - stmmac_set_half_duplex(priv); + stmmac_set_mac_capabilties(priv); stmmac_napi_add(dev); if (netif_running(dev)) diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index dfa1828cd756..fe3f64df17ac 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -272,6 +272,7 @@ struct plat_stmmacenet_data { u8 tx_sched_algorithm; struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES]; struct stmmac_txq_cfg tx_queues_cfg[MTL_MAX_TX_QUEUES]; + u32 phylink_mac_capabilities; void (*fix_mac_speed)(void *priv, unsigned int speed, unsigned int mode); int (*fix_soc_reset)(void *priv, void __iomem *ioaddr); int (*serdes_powerup)(struct net_device *ndev, void *priv);
On Wed, Mar 20, 2024 at 10:37:00AM +0000, Russell King (Oracle) wrote: > Serge, > > Again, please learn to trim your replies. Ok, got it. > > On Wed, Mar 20, 2024 at 01:10:04PM +0300, Serge Semin wrote: > ... > > > > In general you are right for sure - it's better to avoid one part > > setting capabilities and another part unsetting them at least from the > > readability and maintainability point of view. But in this case we've > > already got implemented a ready-to-use internal interface > > stmmac_ops::phylink_get_caps() which can be used to extend/reduce the > > capabilities field based on the particular MAC abilities. Moreover > > it's called right from the component setting the capabilities. Are you > > saying that the callback is supposed to be utilized for extending the > > capabilities only? > > What concerns me is that the proposed code _overwrites_ the > capabilities from the MAC layer, so from a maintanability point of > view it's a nightmare, because you will now have the situation where > MACs provide their capabilities, and then platform code may overwrite > it - which means it's like a spiders web trying to work out what > capabilities are provided. > > The reality is surely that the MAC dictates what it can do, but there > may be further restrictions by other components in the platform, so > the capabilities provided to phylink should be: > > mac_capabilities & platform_capabilities > > And what I'm proposing is that _that_ should be done in a way that > makes it _easy_ for the platform code to get right. Overriding > stmmac_ops::phylink_get_caps() doesn't do that - as can be seen in > the proposed patch. > > Help your users write correct code by adopting a structure that makes > it easy for them to do the right thing. Got it. Thanks for the detailed explanation. So you prefer not to provide too much freedom in the interface to refrain the users from unwillingly doing something wrong. Sounds very reasonable. > > > If you insist on not overriding the stmmac_ops::phylink_get_caps() > > anyway then please explain what is the principal difference > > between the next two code snippets: > > /* Get the MAC specific capabilities */ > > stmmac_mac_phylink_get_caps(priv); > > and > > priv->phylink_config.mac_capabilities &= ~priv->plat->mac_caps_mask; > > > I was thinking: > > stmmac_mac_phylink_get_caps(priv); > > if (priv->plat->mac_capabilities) > priv->phylink_config.mac_capabilities &= > priv->plat->mac_capabilities; > > In other words, if a platform sets plat->mac_capabilities, then it is > providing the capabilities that it supports, and those need to reduce > the capabilities provided by the MAC. > > This will _also_ allow stmmac_set_half_duplex() to do the right thing. > Consider something in the platform side that doesn't allow half-duplex, > but allows tx_queues_to_use == 1. That'll set the half-duplex modes > when stmmac_set_half_duplex() is called, overriding what the platform > supports. > > Now that I look at the stmmac implementation, there's even more that > is wrong. Consider plat->max_speed = 100, like > arch/arc/boot/dts/axs10x_mb.dtsi sets. If stmmac_set_half_duplex() > is called as it can be from stmmac_reinit_queues(), it'll enable > 1000 half-duplex, despite the plat->max_speed = 100. Right. Plus to that I found three more issues in the MAC-capabilities detection code: 1. There is DW MAC100 (see dwmac100_core.c, dwmac100_dma.c), which doesn't seem like supporting 1G speed, but stmmac_phy_setup() and stmmac_set_half_duplex() enable the MAC_1000* capabilities anyway. 2. DW XGMAC doesn't support 10/100Mbps speed and half-duplex, but the stmmac_phy_setup() and stmmac_set_half_duplex() methods set them up anyway. 3. It seems that the Tx-queues-based constraint of the half-duplex-ness is only specific for DW QoS Eth (DW GMAC v4/v5). The databook explicitly says about that: "In multi queue/channel configurations, enable only Q0/CH0 on Tx and Rx for half-duplex operation. If you want to enable single queue/channel in full-duplex operation, any queue/channel can be enabled." There is no such note in the DW GMAC v3.x databook. At least first two problems look as bugs. The last note will be interesting for Yanteng who has the multi-channel DW GMAC _v3_ device with the half-duplex mode working for all channels. Therefore the statement if (priv->plat->tx_queues_to_use > 1) priv->phylink_config.mac_capabilities &= ~(MAC_10HD | MAC_100HD | MAC_1000HD); else priv->phylink_config.mac_capabilities |= (MAC_10HD | MAC_100HD | MAC_1000HD); won't be acceptable for his device. > > > in the MAC-capabilities update implementation? Do you think the later > > approach would be more descriptive? If so then would the > > callback-based approach almost equally descriptive if the callback > > name was, suppose, stmmac_mac_phylink_set_caps() or similar? > > From what I can see of the existing stmmac MAC phylink_get_caps > implementations, there seem to be two - xgmac_phylink_get_caps() > and dwmac4_phylink_get_caps(). Both of these merely set additional > modes in priv->phylink_config.mac_capabilities. Is there a reason > to have this as an instruction stream, rather than providing data > to the core stmmac code from the MAC about its capabilities? Is > there a reason why it would be necessary for the code in a MAC backend > to make a decision about what capabilities to enable based on some > condition? Seeing the Tx-queues-based constraint is DW QoS Eth-specific there is such reason. It might be better to move the selective Half-duplex mode disabling to the MAC-specific callback. But based on what you already demonstrated in the following up email messages there is a better option to implement the MAC capabilities detection procedure. Let's see what MAC-capabilities can be currently specified based on the DW MAC IP-core versions: DW MAC100: MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 | MAC_100 DW GMAC: MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000 DW QoS Eth: MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD but if the amount of the active Tx queues is > 1, then: MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10FD | MAC_100FD | MAC_1000FD | MAC_2500FD DW XGMAC: MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_1000FD | MAC_2500FD | MAC_5000FD | MAC_10000FD | MAC_25000FD | MAC_40000FD | MAC_50000FD | MAC_100000FD As you can see there are only two common capabilities: MAC_ASYM_PAUSE | MAC_SYM_PAUSE. Seeing the flow-control is implemented as a callback for each MAC IP-core (see dwmac100_flow_ctrl(), dwmac1000_flow_ctrl(), sun8i_dwmac_flow_ctrl(), etc) we can freely move all the PHYLINK MAC capabilities initializations to the MAC-specific setup methods (similarly to what you already did in your patch but specifying the full MAC capabilities list). After that the only IP-core which requires the capabilities update will be DW QoS Eth. So the Tx-queue-based capabilities update can be moved there and the rest of the xgmac_phylink_get_caps() callback can be dropped. We can go further. Instead of calling the stmmac_set_half_duplex()/stmmac_set_mac_capabilties() methods on the device init and queues reinit stages, we can move their bodies into the phylink:mac_get_caps() callback. Here is what the described changes will look like: diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index 721c1f8e892f..2339af32ac77 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -525,6 +525,7 @@ extern const struct stmmac_hwtimestamp stmmac_ptp; extern const struct stmmac_mode_ops dwmac4_ring_mode_ops; struct mac_link { + u32 caps; u32 speed_mask; u32 speed10; u32 speed100; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c index 137741b94122..6bc18d4c4da9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -1097,6 +1097,8 @@ static struct mac_device_info *sun8i_dwmac_setup(void *ppriv) priv->dev->priv_flags |= IFF_UNICAST_FLT; + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000; /* The loopback bit seems to be re-set when link change * Simply mask it each time * Speed 10/100/1000 are set in BIT(2)/BIT(3) diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c index 3927609abc44..8555299443f4 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c @@ -539,6 +539,8 @@ int dwmac1000_setup(struct stmmac_priv *priv) if (mac->multicast_filter_bins) mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000; mac->link.duplex = GMAC_CONTROL_DM; mac->link.speed10 = GMAC_CONTROL_PS; mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c index a6e8d7bd9588..7667d103cd0e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c @@ -175,6 +175,8 @@ int dwmac100_setup(struct stmmac_priv *priv) dev_info(priv->device, "\tDWMAC100\n"); mac->pcsr = priv->ioaddr; + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100; mac->link.duplex = MAC_CONTROL_F; mac->link.speed10 = 0; mac->link.speed100 = 0; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index 6b6d0de09619..c51dc946e8ef 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c @@ -70,7 +70,11 @@ static void dwmac4_core_init(struct mac_device_info *hw, static void dwmac4_phylink_get_caps(struct stmmac_priv *priv) { - priv->phylink_config.mac_capabilities |= MAC_2500FD; + /* Half-Duplex can only work with single tx queue */ + if (priv->plat->tx_queues_to_use > 1) + priv->hw->link.caps &= ~(MAC_10HD | MAC_100HD | MAC_1000HD); + else + priv->hw->link.caps |= (MAC_10HD | MAC_100HD | MAC_1000HD); } static void dwmac4_rx_queue_enable(struct mac_device_info *hw, @@ -1356,6 +1360,8 @@ int dwmac4_setup(struct stmmac_priv *priv) if (mac->multicast_filter_bins) mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD; mac->link.duplex = GMAC_CONFIG_DM; mac->link.speed10 = GMAC_CONFIG_PS; mac->link.speed100 = GMAC_CONFIG_FES | GMAC_CONFIG_PS; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index eb48211d9b0e..79ea8329ead5 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -47,14 +47,6 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); } -static void xgmac_phylink_get_caps(struct stmmac_priv *priv) -{ - priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | - MAC_10000FD | MAC_25000FD | - MAC_40000FD | MAC_50000FD | - MAC_100000FD; -} - static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) { u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); @@ -1618,6 +1610,11 @@ int dwxlgmac2_setup(struct stmmac_priv *priv) if (mac->multicast_filter_bins) mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_1000FD | MAC_2500FD | MAC_5000FD | + MAC_10000FD | MAC_25000FD | + MAC_40000FD | MAC_50000FD | + MAC_100000FD; mac->link.duplex = 0; mac->link.speed1000 = XLGMAC_CONFIG_SS_1000; mac->link.speed2500 = XLGMAC_CONFIG_SS_2500; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 25519952f754..24ff5d1eb963 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -936,6 +936,22 @@ static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex) priv->pause, tx_cnt); } +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, + phy_interface_t interface) +{ + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); + + /* Get the MAC-specific capabilities */ + stmmac_mac_phylink_get_caps(priv); + + config->mac_capabilities = priv->hw->link.caps; + + if (priv->plat->max_speed) + phylink_limit_mac_speed(config, priv->plat->max_speed); + + return config->mac_capabilities; +} + static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config, phy_interface_t interface) { @@ -1105,6 +1121,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, } static const struct phylink_mac_ops stmmac_phylink_mac_ops = { + .mac_get_caps = stmmac_mac_get_caps, .mac_select_pcs = stmmac_mac_select_pcs, .mac_config = stmmac_mac_config, .mac_link_down = stmmac_mac_link_down, @@ -1198,24 +1215,12 @@ static int stmmac_init_phy(struct net_device *dev) return ret; } -static void stmmac_set_half_duplex(struct stmmac_priv *priv) -{ - /* Half-Duplex can only work with single tx queue */ - if (priv->plat->tx_queues_to_use > 1) - priv->phylink_config.mac_capabilities &= - ~(MAC_10HD | MAC_100HD | MAC_1000HD); - else - priv->phylink_config.mac_capabilities |= - (MAC_10HD | MAC_100HD | MAC_1000HD); -} - static int stmmac_phy_setup(struct stmmac_priv *priv) { struct stmmac_mdio_bus_data *mdio_bus_data; int mode = priv->plat->phy_interface; struct fwnode_handle *fwnode; struct phylink *phylink; - int max_speed; priv->phylink_config.dev = &priv->dev->dev; priv->phylink_config.type = PHYLINK_NETDEV; @@ -1236,19 +1241,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) xpcs_get_interfaces(priv->hw->xpcs, priv->phylink_config.supported_interfaces); - priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | - MAC_10FD | MAC_100FD | - MAC_1000FD; - - stmmac_set_half_duplex(priv); - - /* Get the MAC specific capabilities */ - stmmac_mac_phylink_get_caps(priv); - - max_speed = priv->plat->max_speed; - if (max_speed) - phylink_limit_mac_speed(&priv->phylink_config, max_speed); - fwnode = priv->plat->port_node; if (!fwnode) fwnode = dev_fwnode(priv->device); --- Note I intentionally omitted the part with the platform-specific MAC capabilities filtering because AFAICS it won't be necessary for Yanteng. He'll have the plat_stmmacenet_data::setup() callback defined anyway, in which he'll be able to initialize the MAC capabilities as required by his platform. -Serge(y) > > > In anyway I am sure the approach suggested in the initial patch of > > this thread isn't good since it motivates the developers to implement > > more-and-more DW MAC-specific platform capabilities flags fixing > > another flags, which makes the generic code even more complicated > > than it already is with endless if-else-plat-flags statements. > > Yes, I do agree with that. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Mar 22, 2024 at 09:07:19PM +0300, Serge Semin wrote: > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 25519952f754..24ff5d1eb963 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -936,6 +936,22 @@ static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex) > priv->pause, tx_cnt); > } > > +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, > + phy_interface_t interface) > +{ > + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > + > + /* Get the MAC-specific capabilities */ > + stmmac_mac_phylink_get_caps(priv); > + > + config->mac_capabilities = priv->hw->link.caps; > + > + if (priv->plat->max_speed) > + phylink_limit_mac_speed(config, priv->plat->max_speed); > + > + return config->mac_capabilities; Yes, I think your approach is better - and it still allows for the platform's capabilities to be masked in towards the end of this function. Thanks.
On Fri, Mar 22, 2024 at 07:56:44PM +0000, Russell King (Oracle) wrote: > On Fri, Mar 22, 2024 at 09:07:19PM +0300, Serge Semin wrote: > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 25519952f754..24ff5d1eb963 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -936,6 +936,22 @@ static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex) > > priv->pause, tx_cnt); > > } > > > > +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, > > + phy_interface_t interface) > > +{ > > + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > > + > > + /* Get the MAC-specific capabilities */ > > + stmmac_mac_phylink_get_caps(priv); > > + > > + config->mac_capabilities = priv->hw->link.caps; > > + > > + if (priv->plat->max_speed) > > + phylink_limit_mac_speed(config, priv->plat->max_speed); > > + > > + return config->mac_capabilities; > > Yes, I think your approach is better - and it still allows for the > platform's capabilities to be masked in towards the end of this > function. Sorry for the long-term response. Thanks for your comment. Seeing Yanteng is struggling much with this series review I'll convert the suggested change into a patchset (taking into account that the change implies some fixes) and submit it for review later on this week. After finishing the review stage, the series could be either merged in right away or Yanteng will be able to pick it up and add it into his patchset. -Serge(y) > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
在 2024/4/3 20:37, Serge Semin 写道: > On Fri, Mar 22, 2024 at 07:56:44PM +0000, Russell King (Oracle) wrote: >> On Fri, Mar 22, 2024 at 09:07:19PM +0300, Serge Semin wrote: >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> index 25519952f754..24ff5d1eb963 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> @@ -936,6 +936,22 @@ static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex) >>> priv->pause, tx_cnt); >>> } >>> >>> +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, >>> + phy_interface_t interface) >>> +{ >>> + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); >>> + >>> + /* Get the MAC-specific capabilities */ >>> + stmmac_mac_phylink_get_caps(priv); >>> + >>> + config->mac_capabilities = priv->hw->link.caps; >>> + >>> + if (priv->plat->max_speed) >>> + phylink_limit_mac_speed(config, priv->plat->max_speed); >>> + >>> + return config->mac_capabilities; >> Yes, I think your approach is better - and it still allows for the >> platform's capabilities to be masked in towards the end of this >> function. > Sorry for the long-term response. Thanks for your comment. Seeing > Yanteng is struggling much with this series review I'll convert the > suggested change into a patchset (taking into account that the change > implies some fixes) and submit it for review later on this week. After > finishing the review stage, the series could be either merged in right > away or Yanteng will be able to pick it up and add it into his patchset. > Couldn't be better! I have already started to re-split the patches, and then a series of tests are needed. There are some network card parameters that I need to confirm with the hardware engineer. Of course, I will pay attention to the email list before sending it out. If I don't see your patch set, I will pick it up. Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 264c4c198d5a..1753a3c46b77 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev, struct stmmac_resources *res, struct device_node *np) { - if (pdev->revision == 0x00 || pdev->revision == 0x01) + switch (pdev->revision) { + case 0x00: + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 | + STMMAC_FLAG_DISABLE_HALF_DUPLEX; + break; + case 0x01: plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; + break; + default: + break; + } return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 5617b40abbe4..3aa862269eb0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev) static void stmmac_set_half_duplex(struct stmmac_priv *priv) { /* Half-Duplex can only work with single tx queue */ - if (priv->plat->tx_queues_to_use > 1) + if (priv->plat->tx_queues_to_use > 1 || + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags)) priv->phylink_config.mac_capabilities &= ~(MAC_10HD | MAC_100HD | MAC_1000HD); else diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index 2810361e4048..197f6f914104 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -222,6 +222,7 @@ struct dwmac4_addrs { #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) +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14) struct plat_stmmacenet_data { int bus_id;