| Message ID | cover.1557300602.git.joabreu@synopsys.com |
|---|---|
| Headers | show |
| Series | net: stmmac: Selftests | expand |
From: Jose Abreu <Jose.Abreu@synopsys.com> Date: Wed, 8 May 2019 09:51:00 +0200 > [ Submitting with net-next closed for proper review and testing. ] Please use "RFC" in the subject line when doing this in the future. Thank you.
On Wed, May 08, 2019 at 09:51:00AM +0200, Jose Abreu wrote: > [ Submitting with net-next closed for proper review and testing. ] > > This introduces selftests support in stmmac driver. We add 4 basic sanity > checks and MAC loopback support for all cores within the driver. This way > more tests can easily be added in the future and can be run in virtually > any MAC/GMAC/QoS/XGMAC platform. > > Having this we can find regressions and missing features in the driver > while at the same time we can check if the IP is correctly working. > > We have been using this for some time now and I do have more tests to > submit in the feature. My experience is that although writing the tests > adds more development time, the gain results are obvious. > > I let this feature optional within the driver under a Kconfig option. > > For this series the output result will be something like this > (e.g. for dwmac1000): > ---- > # ethtool -t eth0 > The test result is PASS > The test extra info: > 1. MAC Loopback 0 > 2. PHY Loopback -95 > 3. MMC Counters 0 > 4. EEE -95 > 5. Hash Filter MC 0 > 6. Perfect Filter UC 0 > 7. Flow Control 0 Hi Jose The man page says: -t --test Executes adapter selftest on the specified network device. Possible test modes are: offline Perform full set of tests, possibly interrupting normal operation during the tests, online Perform limited set of tests, not interrupting normal operation, external_lb Perform full set of tests, as for offline, and additionally an external-loopback test. The normal operation is interrupted by the tests you carry out here. But i don't see any code looking for ETH_TEST_FL_OFFLINE > (Error code -95 means EOPNOTSUPP in current HW). How deep do you have to go before you know about EOPNOTSUPP? It would be better to not return the string and result at all. Or patch ethtool to call strerror(3). Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: Wed, May 08, 2019 at 20:50:11 > The normal operation is interrupted by the tests you carry out > here. But i don't see any code looking for ETH_TEST_FL_OFFLINE Ok will fix to only run in offline mode then. > > > (Error code -95 means EOPNOTSUPP in current HW). > > How deep do you have to go before you know about EOPNOTSUPP? It would > be better to not return the string and result at all. Or patch ethtool > to call strerror(3). When I looked at other drivers I saw that they return positive value (1) or zero so calling strerror in ethtool may not be ideal. I think its useful to let the user know if a given test is not supported in HW so maybe I can return 1 instead of EOPNOTSUPP ? Thanks, Jose Miguel Abreu
On Wed, May 08, 2019 at 09:51:00AM +0200, Jose Abreu wrote: > [ Submitting with net-next closed for proper review and testing. ] > > This introduces selftests support in stmmac driver. We add 4 basic sanity > checks and MAC loopback support for all cores within the driver. This way > more tests can easily be added in the future and can be run in virtually > any MAC/GMAC/QoS/XGMAC platform. > > Having this we can find regressions and missing features in the driver > while at the same time we can check if the IP is correctly working. > > We have been using this for some time now and I do have more tests to > submit in the feature. My experience is that although writing the tests > adds more development time, the gain results are obvious. > > I let this feature optional within the driver under a Kconfig option. > > For this series the output result will be something like this > (e.g. for dwmac1000): > ---- > # ethtool -t eth0 > The test result is PASS > The test extra info: > 1. MAC Loopback 0 > 2. PHY Loopback -95 > 3. MMC Counters 0 > 4. EEE -95 > 5. Hash Filter MC 0 > 6. Perfect Filter UC 0 > 7. Flow Control 0 > ---- > > (Error code -95 means EOPNOTSUPP in current HW). > Hello I have started to patch dwmac_sun8i for using your patchset and get the following: The test result is FAIL The test extra info: 1. MAC Loopback 0 2. PHY Loopback -95 3. MMC Counters -95 4. EEE -95 5. Hash Filter MC 0 6. Perfect Filter UC 1 7. Flow Control -95 What means 1 for "Perfect Filter UC" ? I have added my patch below Regards --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -976,6 +976,18 @@ static void sun8i_dwmac_exit(struct platform_device *pdev, void *priv) regulator_disable(gmac->regulator); } +static void sun8i_dwmac_set_mac_loopback(void __iomem *ioaddr, bool enable) +{ + u32 value = readl(ioaddr + EMAC_BASIC_CTL0); + + if (enable) + value |= EMAC_LOOPBACK; + else + value &= ~EMAC_LOOPBACK; + + writel(value, ioaddr + EMAC_BASIC_CTL0); +} + static const struct stmmac_ops sun8i_dwmac_ops = { .core_init = sun8i_dwmac_core_init, .set_mac = sun8i_dwmac_set_mac, @@ -985,6 +997,7 @@ static const struct stmmac_ops sun8i_dwmac_ops = { .flow_ctrl = sun8i_dwmac_flow_ctrl, .set_umac_addr = sun8i_dwmac_set_umac_addr, .get_umac_addr = sun8i_dwmac_get_umac_addr, + .set_mac_loopback = sun8i_dwmac_set_mac_loopback, }; static struct mac_device_info *sun8i_dwmac_setup(void *ppriv)
From: Corentin Labbe <clabbe.montjoie@gmail.com> Date: Thu, May 09, 2019 at 10:04:16 > What means 1 for "Perfect Filter UC" ? Thank you for the testing :) 1 means that either the expected packet was not received or that the filter did not work. For GMAC there is the need to set the HPF bit in order for the test to pass. Do you have such bit in your HW ? It should be in EMAC_RX_FRM_FLT register. > I have added my patch below Do you want me to add your patch to the series ? If you send me with git-send-email I can apply it with your SoB. Thanks, Jose Miguel Abreu
From: Jose Abreu <joabreu@synopsys.com> Date: Thu, May 09, 2019 at 09:17:03 > From: Andrew Lunn <andrew@lunn.ch> > Date: Wed, May 08, 2019 at 20:50:11 > > > The normal operation is interrupted by the tests you carry out > > here. But i don't see any code looking for ETH_TEST_FL_OFFLINE > > Ok will fix to only run in offline mode then. > > > > > > (Error code -95 means EOPNOTSUPP in current HW). > > > > How deep do you have to go before you know about EOPNOTSUPP? It would > > be better to not return the string and result at all. Or patch ethtool > > to call strerror(3). > > When I looked at other drivers I saw that they return positive value (1) > or zero so calling strerror in ethtool may not be ideal. > > I think its useful to let the user know if a given test is not supported > in HW so maybe I can return 1 instead of EOPNOTSUPP ? After thinking about this in more detail I now realize that returning 1 is not ideal because when a test fails it will also return 1. So if I do it this way and more than one test fails then user won't know if the test really failed or if it wasn't supported in the first time. Any advice ? Thanks, Jose Miguel Abreu
On Thu, May 09, 2019 at 10:11:53AM +0000, Jose Abreu wrote: > From: Corentin Labbe <clabbe.montjoie@gmail.com> > Date: Thu, May 09, 2019 at 10:04:16 > > > What means 1 for "Perfect Filter UC" ? > > Thank you for the testing :) > > 1 means that either the expected packet was not received or that the > filter did not work. > > For GMAC there is the need to set the HPF bit in order for the test to > pass. Do you have such bit in your HW ? It should be in EMAC_RX_FRM_FLT > register. The problem was missing IFF_UNICAST_FLT, so netcore put the device in promiscous when adding an unicast address. Next step will be to enable Flow Control, but I will start a new thread for that. > > > I have added my patch below > > Do you want me to add your patch to the series ? If you send me with > git-send-email I can apply it with your SoB. > I will do. I will send the patch for IFF_UNICAST_FLT appart since it fixes a bug. Regards