| Message ID | 20230816152926.4093-3-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: add new features to xgmac | expand |
On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote: > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps > or not. The commit message fails to explain the 'Why?' question. GMII does normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also implies 10/100/1000? So why also set dma_cap->mbps_10_100? Maybe a better change would be to modify: seq_printf(seq, "\t1000 Mbps: %s\n", (priv->dma_cap.mbps_1000) ? "Y" : "N"); to actually say 10/100/1000 Mbps? It does not appear this is used for anything other than debugfs? Andrew
On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote: > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote: > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps > > or not. > > The commit message fails to explain the 'Why?' question. GMII does > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also > implies 10/100/1000? So why also set dma_cap->mbps_10_100? > > Maybe a better change would be to modify: > > seq_printf(seq, "\t1000 Mbps: %s\n", > (priv->dma_cap.mbps_1000) ? "Y" : "N"); > > to actually say 10/100/1000 Mbps? It does not appear this is used for > anything other than debugfs? Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only used to print things in the debugfs file, and do not have any effect on the driver. Moreover: drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_GMIISEL BIT(1) drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_GMIISEL 0x00000002 /* 1000 Mbps Support */ drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define XGMAC_HWFEAT_GMIISEL BIT(1) Seems to be all the same bit, and: drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_MIISEL BIT(0) drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_MIISEL 0x00000001 /* 10/100 Mbps Support */ So, if everyone defines the first few bits of the hw_cap identically, is there any point to decoding this separately in each driver? Couldn't the debugfs "show" function just parse the hw_cap directly? Wouldn't it make more sense to print MII / GMII rather than 10/100 and 1000 ? It does bring up one last question though: if the driver makes no use of these hw_cap bits, then is there any point in printing them in the debugfs file?
Hi Russel, Andrew On Sun, Aug 20, 2023 at 08:51:41PM +0100, Russell King (Oracle) wrote: > On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote: > > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote: > > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps > > > or not. > > > > The commit message fails to explain the 'Why?' question. GMII does > > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also > > implies 10/100/1000? So why also set dma_cap->mbps_10_100? Regarding DW XGMAC. I can't say for sure. Based on DW XGMAC v2.10 IP-core HW manual it has MAC_HW_Feature0.GMIISEL(1) flag indicating whether there is GMII interface besides of the XGMII interface. But in my databook MAC_HW_Feature0.BIT(0) is marked as reserved and MAC_Tx_Configuration.SS field doesn't have 10/100Mbps modes despite of what is defined in dwxgmac2.h by means of the XGMAC_CONFIG_SS_10_MII and XGMAC_CONFIG_SS_1000_GMII macros. But DW GMAC or DW Eth QoS can be synthesized with the 1000-only mode enabled. GMIISEL and MIISEL flags reflect the OP_MODE IP-core synthesize parameter state. It can have three different values: Mode of Operation Description: Configures the MAC to work in 10/100/1000 Mbps mode. Select 10/100/1000 Mbps for enabling both Fast Ethernet and Gigabit operations, 10/100 Mbps for Fast Ethernet-only operations, and 1000 Mbps for Gigabit-only operations. !!! Value Range: 10/100/1000 Mbps, 10/100 Mbps, or 1000 Mbps Default Value: 10/100/1000 Mbps with Gigabit License 10/100 with Fast Ethernet license HDL Parameter Name: OP_MODE > > > > Maybe a better change would be to modify: > > > > seq_printf(seq, "\t1000 Mbps: %s\n", > > (priv->dma_cap.mbps_1000) ? "Y" : "N"); > > > > to actually say 10/100/1000 Mbps? It does not appear this is used for > > anything other than debugfs? > > Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only > used to print things in the debugfs file, and do not have any effect > on the driver. They should have been utilized somehow in the stmmac_mac_link_up() and in the dwmac1000_setup(), dwmac4_setup(), etc methods in order to select the proper speed. But yeah, currently they are used to print the DebugFS node data only. > > Moreover: > > drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_GMIISEL BIT(1) > drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_GMIISEL 0x00000002 /* 1000 Mbps Support */ > drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define XGMAC_HWFEAT_GMIISEL BIT(1) > > Seems to be all the same bit, and: > > drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_FEAT_MIISEL BIT(0) > drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_MIISEL 0x00000001 /* 10/100 Mbps Support */ > > So, if everyone defines the first few bits of the hw_cap identically, > is there any point to decoding this separately in each driver? Couldn't > the debugfs "show" function just parse the hw_cap directly? The rest of the data in the HW-feature registers is almost completely different. DW GMAC (common.h) has a single HW-Feature register which has very little in common with the DW XGMAC (dwxgmac2.h) and DW Eth QoS (dwmac4.h) MAC_HW_Feature0 register. The later two IP-cores have the HW-feature registers looking very similar but still differing in some flags. So in order not to have a partly measured change I would suggest to preserve the separate HW-features macros space for each type of the devices for now. If somebody cares to have them indicating common and separate flags one could provide a comprehensive patch fixing the entire HW-feature macros definitions. Although I don't see this being that much necessary. > Wouldn't it > make more sense to print MII / GMII rather than 10/100 and 1000 ? > Based on the GMIISEL and MIISEL flags description they are speed-related, not the interface type: GMIISEL 1000 Mbps Support MIISEL 10 or 100 Mbps support > It does bring up one last question though: if the driver makes no use > of these hw_cap bits, then is there any point in printing them in the > debugfs file? This question can be applied to almost the half of the dma_feature structure fields.) One more patch extends it with even more mainly unused fields: https://lore.kernel.org/netdev/20230819105440.226892-1-0x1207@gmail.com/ -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! >
On Mon, Aug 21, 2023 at 04:25:42PM +0300, Serge Semin wrote: > Hi Russel, Andrew > > On Sun, Aug 20, 2023 at 08:51:41PM +0100, Russell King (Oracle) wrote: > > On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote: > > > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote: > > > > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps > > > > or not. > > > > > > > The commit message fails to explain the 'Why?' question. GMII does > > > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also > > > implies 10/100/1000? So why also set dma_cap->mbps_10_100? > > Regarding DW XGMAC. I can't say for sure. Based on DW XGMAC v2.10 > IP-core HW manual it has MAC_HW_Feature0.GMIISEL(1) flag indicating > whether there is GMII interface besides of the XGMII interface. But in > my databook MAC_HW_Feature0.BIT(0) is marked as reserved and > MAC_Tx_Configuration.SS field doesn't have 10/100Mbps modes despite of > what is defined in dwxgmac2.h by means of the XGMAC_CONFIG_SS_10_MII > and XGMAC_CONFIG_SS_1000_GMII macros. > > But DW GMAC or DW Eth QoS can be synthesized with the 1000-only > mode enabled. GMIISEL and MIISEL flags reflect the OP_MODE IP-core > synthesize parameter state. It can have three different values: > > Mode of Operation Description: Configures the MAC to work in > 10/100/1000 Mbps mode. Select 10/100/1000 > Mbps for enabling both Fast Ethernet and Gigabit > operations, 10/100 Mbps for Fast Ethernet-only > operations, and 1000 Mbps for Gigabit-only operations. > !!! Value Range: 10/100/1000 Mbps, 10/100 Mbps, or 1000 Mbps > Default Value: > 10/100/1000 Mbps with Gigabit License > 10/100 with Fast Ethernet license > HDL Parameter Name: OP_MODE > > > > > > > Maybe a better change would be to modify: > > > > > > seq_printf(seq, "\t1000 Mbps: %s\n", > > > (priv->dma_cap.mbps_1000) ? "Y" : "N"); > > > > > > to actually say 10/100/1000 Mbps? It does not appear this is used for > > > anything other than debugfs? > > > > > Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only > > used to print things in the debugfs file, and do not have any effect > > on the driver. > > They should have been utilized somehow in the stmmac_mac_link_up() and No, definitely not in mac_link_up(). If these flags indicate what speeds are available, then what would mac_link_up() do if, e.g. the core says "I don't support 1G" and phylink determines that the result of negotiation is 1G? This is clearly not the right place. The right place is when initialising the phylink MAC capabilities, which is currently done in stmmac_phy_setup() without *any* regard what so ever for what speeds are actually supported, with the exception of "oh, is that the maximum speed". > > It does bring up one last question though: if the driver makes no use > > of these hw_cap bits, then is there any point in printing them in the > > debugfs file? > > This question can be applied to almost the half of the dma_feature > structure fields.) One more patch extends it with even more mainly > unused fields: > https://lore.kernel.org/netdev/20230819105440.226892-1-0x1207@gmail.com/ If the hw_cap field is specific, then how about some hardware specific data giving something like an enum listing the capabilties, the enum used to index a string table of capabilities, and an array of bit numbers in hw_cap for those fields, or an array of masks? This data could be const, which means that stmmac_dma_cap_show() only needs the hw_cap value and the struct. That also means that stmmac_phy_setup() could also index the array of bit numbers to test for e.g. GMII/MII support to determine whether 10/100 and 1000 capabilities should be added for phylink. If we look at the "half_duplex" dma capability, things are similarly stupid. Pulling out of dwmac4: dma_cap->half_duplex = (hw_cap & GMAC_HW_FEAT_HDSEL) >> 2; This is not tested elsewhere from what I can find - neither the hwcap nor the half_duplex field except for reporting in debugfs. It isn't used to restrict the phylink capabilities for HD, since the only test is this: /* Half-Duplex can only work with single queue */ if (priv->plat->tx_queues_to_use > 1) priv->phylink_config.mac_capabilities &= ~(MAC_10HD | MAC_100HD | MAC_1000HD); So, the reporting of "half duplex" mode in debugfs has absolutely nothing to do with whether we try to use half duplex modes in the driver. This is rubbish. Utter rubbish.
On Mon, Aug 21, 2023 at 02:57:51PM +0100, Russell King (Oracle) wrote: > On Mon, Aug 21, 2023 at 04:25:42PM +0300, Serge Semin wrote: > > Hi Russel, Andrew > > > > On Sun, Aug 20, 2023 at 08:51:41PM +0100, Russell King (Oracle) wrote: > > > On Sun, Aug 20, 2023 at 09:15:06PM +0200, Andrew Lunn wrote: > > > > On Wed, Aug 16, 2023 at 11:29:19PM +0800, Jisheng Zhang wrote: > > > > > > > The XGMAC_HWFEAT_GMIISEL bit also indicates whether support 10/100Mbps > > > > > or not. > > > > > > > > > > The commit message fails to explain the 'Why?' question. GMII does > > > > normally imply 10/100/1000, so i would expect dma_cap->mbps_1000 also > > > > implies 10/100/1000? So why also set dma_cap->mbps_10_100? > > > > Regarding DW XGMAC. I can't say for sure. Based on DW XGMAC v2.10 > > IP-core HW manual it has MAC_HW_Feature0.GMIISEL(1) flag indicating > > whether there is GMII interface besides of the XGMII interface. But in > > my databook MAC_HW_Feature0.BIT(0) is marked as reserved and > > MAC_Tx_Configuration.SS field doesn't have 10/100Mbps modes despite of > > what is defined in dwxgmac2.h by means of the XGMAC_CONFIG_SS_10_MII > > and XGMAC_CONFIG_SS_1000_GMII macros. > > > > But DW GMAC or DW Eth QoS can be synthesized with the 1000-only > > mode enabled. GMIISEL and MIISEL flags reflect the OP_MODE IP-core > > synthesize parameter state. It can have three different values: > > > > Mode of Operation Description: Configures the MAC to work in > > 10/100/1000 Mbps mode. Select 10/100/1000 > > Mbps for enabling both Fast Ethernet and Gigabit > > operations, 10/100 Mbps for Fast Ethernet-only > > operations, and 1000 Mbps for Gigabit-only operations. > > !!! Value Range: 10/100/1000 Mbps, 10/100 Mbps, or 1000 Mbps > > Default Value: > > 10/100/1000 Mbps with Gigabit License > > 10/100 with Fast Ethernet license > > HDL Parameter Name: OP_MODE > > > > > > > > > > Maybe a better change would be to modify: > > > > > > > > seq_printf(seq, "\t1000 Mbps: %s\n", > > > > (priv->dma_cap.mbps_1000) ? "Y" : "N"); > > > > > > > > to actually say 10/100/1000 Mbps? It does not appear this is used for > > > > anything other than debugfs? > > > > > > > > Indeed, it also looks to me like mbps_1000 and mbps_10_100 are only > > > used to print things in the debugfs file, and do not have any effect > > > on the driver. > > > > They should have been utilized somehow in the stmmac_mac_link_up() and > > No, definitely not in mac_link_up(). If these flags indicate what speeds > are available, then what would mac_link_up() do if, e.g. the core says > "I don't support 1G" and phylink determines that the result of > negotiation is 1G? > > This is clearly not the right place. The right place is when > initialising the phylink MAC capabilities, which is currently done in > stmmac_phy_setup() without *any* regard what so ever for what speeds > are actually supported, with the exception of "oh, is that the maximum > speed". Ok. My suggestion was based on the current stmmac_mac_link_up() implementation which configures the MAC speed based on the speed coming from the phylink core and silently returns if the speed is unsupported.) > > > > It does bring up one last question though: if the driver makes no use > > > of these hw_cap bits, then is there any point in printing them in the > > > debugfs file? > > > > This question can be applied to almost the half of the dma_feature > > structure fields.) One more patch extends it with even more mainly > > unused fields: > > https://lore.kernel.org/netdev/20230819105440.226892-1-0x1207@gmail.com/ > > If the hw_cap field is specific, then how about some hardware specific > data giving something like an enum listing the capabilties, the enum > used to index a string table of capabilities, and an array of bit > numbers in hw_cap for those fields, or an array of masks? This data > could be const, which means that stmmac_dma_cap_show() only needs > the hw_cap value and the struct. That would have been a great solution. > > That also means that stmmac_phy_setup() could also index the > array of bit numbers to test for e.g. GMII/MII support to determine > whether 10/100 and 1000 capabilities should be added for phylink. > > If we look at the "half_duplex" dma capability, things are similarly > stupid. Pulling out of dwmac4: > > dma_cap->half_duplex = (hw_cap & GMAC_HW_FEAT_HDSEL) >> 2; > > This is not tested elsewhere from what I can find - neither the > hwcap nor the half_duplex field except for reporting in debugfs. > It isn't used to restrict the phylink capabilities for HD, since > the only test is this: > > /* Half-Duplex can only work with single queue */ > if (priv->plat->tx_queues_to_use > 1) > priv->phylink_config.mac_capabilities &= > ~(MAC_10HD | MAC_100HD | MAC_1000HD); > > So, the reporting of "half duplex" mode in debugfs has absolutely > nothing to do with whether we try to use half duplex modes in the > driver. > > This is rubbish. Utter rubbish. So is a lot of stuff in the STMMAC driver. Look closely at what is implemented there. One bright example is the plat_stmmacenet_data structure content. For instance, msi_mac_vec, msi_wol_vec, msi_lpi_vec, msi_sfty_ce_vec, msi_sfty_ue_vec, msi_rx_base_vec, msi_tx_base_vec aren't even utilized in the core driver, but in the Intel glue driver only. Some other plat_stmmacenet_data fields are utilized to either override the dma_features fields or being utilized even though there is a auto-detectable HW-features field. All of the HW-abstraction macros accept stmmac_priv pointer as a parameter meanwhile the abstracting functions don't. So the respective functions need to have all of parameters passed as arguments which makes some function prototypes too bulky and would require the prototype modification should some additional data is required in the function implementation. Moreover the HW-abstraction function prototypes aren't unified: some accept the regs base address, some mac_device_info pointer, etc. mac_device_info instance is always required but it's separately malloced all the time the stmmac_drv_probe() is called. It should have been just embedded into the stmmac_priv data. and so on and so forth. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c index 3aacf791efeb..1ef8fc132c2d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c @@ -410,6 +410,7 @@ static int dwxgmac2_get_hw_feature(void __iomem *ioaddr, dma_cap->vlhash = (hw_cap & XGMAC_HWFEAT_VLHASH) >> 4; dma_cap->half_duplex = (hw_cap & XGMAC_HWFEAT_HDSEL) >> 3; dma_cap->mbps_1000 = (hw_cap & XGMAC_HWFEAT_GMIISEL) >> 1; + dma_cap->mbps_10_100 = (hw_cap & XGMAC_HWFEAT_GMIISEL) >> 1; /* MAC HW feature 1 */ hw_cap = readl(ioaddr + XGMAC_HW_FEATURE1);