| Message ID | 20230227091156.19509-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f99e6d7c4ed3be2531bd576425a5bd07fb133bd7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] bgmac: fix *initial* chip reset to support BCM5358 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Single patches do not need cover letters |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 86 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 1 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Feb 27, 2023 at 10:11:56AM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > While bringing hardware up we should perform a full reset including the > switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what > specification says and what reference driver does. > > This seems to be critical for the BCM5358. Without this hardware doesn't > get initialized properly and doesn't seem to transmit or receive any > packets. > > Originally bgmac was calling bgmac_chip_reset() before setting > "has_robosw" property which resulted in expected behaviour. That has > changed as a side effect of adding platform device support which > regressed BCM5358 support. > > Fixes: f6a95a24957a ("net: ethernet: bgmac: Add platform device support") > Cc: Jon Mason <jdmason@kudzu.us> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/net/ethernet/broadcom/bgmac.c | 8 ++++++-- > drivers/net/ethernet/broadcom/bgmac.h | 2 ++ > 2 files changed, 8 insertions(+), 2 deletions(-) > Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On 2/27/2023 1:11 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > While bringing hardware up we should perform a full reset including the > switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what > specification says and what reference driver does. > > This seems to be critical for the BCM5358. Without this hardware doesn't > get initialized properly and doesn't seem to transmit or receive any > packets. > > Originally bgmac was calling bgmac_chip_reset() before setting > "has_robosw" property which resulted in expected behaviour. That has > changed as a side effect of adding platform device support which > regressed BCM5358 support. > > Fixes: f6a95a24957a ("net: ethernet: bgmac: Add platform device support") > Cc: Jon Mason <jdmason@kudzu.us> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Mon, 27 Feb 2023 10:11:56 +0100 you wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > While bringing hardware up we should perform a full reset including the > switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what > specification says and what reference driver does. > > This seems to be critical for the BCM5358. Without this hardware doesn't > get initialized properly and doesn't seem to transmit or receive any > packets. > > [...] Here is the summary with links: - [net] bgmac: fix *initial* chip reset to support BCM5358 https://git.kernel.org/netdev/net/c/f99e6d7c4ed3 You are awesome, thank you!
Hi Rafał, On mar 07-02-2023 23:53:27, Rafał Miłecki wrote: > While bringing hardware up we should perform a full reset including the > switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what > specification says and what reference driver does. > > This seems to be critical for the BCM5358. Without this hardware doesn't > get initialized properly and doesn't seem to transmit or receive any > packets. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> KernelCI found this patch causes a regression in the bootrr.deferred-probe-empty test [1] on sun8i-h3-libretech-all-h3-cc [2], see the bisection report for more details [3] Does it make sense to you? Cheers, Ricardo [1] https://github.com/kernelci/bootrr/blob/3ae9fd5dffc667fa96012892ea08532bc6877276/helpers/bootrr-generic-tests#L3 [2] https://linux.kernelci.org/test/case/id/642a0f5c78c0feaf5d62f79c/ [3] https://groups.io/g/kernelci-results/message/40156 #regzbot introduced: f6a95a24957a
Hi, On 4.04.2023 15:46, Ricardo Cañuelo wrote: > On mar 07-02-2023 23:53:27, Rafał Miłecki wrote: >> While bringing hardware up we should perform a full reset including the >> switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what >> specification says and what reference driver does. >> >> This seems to be critical for the BCM5358. Without this hardware doesn't >> get initialized properly and doesn't seem to transmit or receive any >> packets. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > KernelCI found this patch causes a regression in the > bootrr.deferred-probe-empty test [1] on sun8i-h3-libretech-all-h3-cc > [2], see the bisection report for more details [3] > > Does it make sense to you? It doesn't seem to make any sense. I guess that on your platform /sys/kernel/debug/devices_deferred is not empty anymore? Does your platform use Broadcom Ethernet controller at all? It seems like some false positive at first sight.
On 04.04.23 15:46, Ricardo Cañuelo wrote: > > On mar 07-02-2023 23:53:27, Rafał Miłecki wrote: >> While bringing hardware up we should perform a full reset including the >> switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what >> specification says and what reference driver does. >> >> This seems to be critical for the BCM5358. Without this hardware doesn't >> get initialized properly and doesn't seem to transmit or receive any >> packets. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > KernelCI found this patch causes a regression in the > bootrr.deferred-probe-empty test [1] on sun8i-h3-libretech-all-h3-cc > [2], see the bisection report for more details [3] > > Does it make sense to you? > > Cheers, > Ricardo > > [1] https://github.com/kernelci/bootrr/blob/3ae9fd5dffc667fa96012892ea08532bc6877276/helpers/bootrr-generic-tests#L3 > [2] https://linux.kernelci.org/test/case/id/642a0f5c78c0feaf5d62f79c/ > [3] https://groups.io/g/kernelci-results/message/40156 > > #regzbot introduced: f6a95a24957a Thx for telling regzbot about this. It seems something went wrong here, the patch this thread about is f99e6d7c4ed3 (which contains a Fixes: tag for f6a95a24957a); copy and paste mistake maybe, whatever. Fixing this and while at it giving this a better title: #regzbot introduced: f99e6d7c4ed3 #regzbot title: net: bgmac: bootrr.deferred-probe-empty test fails in KernelCi on sun8i-h3-libretech-all-h3-cc #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
On 4/4/2023 6:52 AM, Rafał Miłecki wrote: > Hi, > > On 4.04.2023 15:46, Ricardo Cañuelo wrote: >> On mar 07-02-2023 23:53:27, Rafał Miłecki wrote: >>> While bringing hardware up we should perform a full reset including the >>> switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what >>> specification says and what reference driver does. >>> >>> This seems to be critical for the BCM5358. Without this hardware doesn't >>> get initialized properly and doesn't seem to transmit or receive any >>> packets. >>> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> >> KernelCI found this patch causes a regression in the >> bootrr.deferred-probe-empty test [1] on sun8i-h3-libretech-all-h3-cc >> [2], see the bisection report for more details [3] >> >> Does it make sense to you? > > It doesn't seem to make any sense. I guess that on your platform > /sys/kernel/debug/devices_deferred > is not empty anymore? > > Does your platform use Broadcom Ethernet controller at all? I do not believe it does, however according to the log, the driver is enabled: <6>[ 1.819466] bgmac_bcma: Broadcom 47xx GBit MAC driver loaded but it should not be probing any device since you don't have any internal BCMA bus to match gigabit devices with. Later in the log we see: 1c22c00.codec sun4i-codec: Failed to register our card and most likely as you already wrote the deferred device list might not be empty.
On 05.04.23 14:42, Florian Fainelli wrote: > On 4/4/2023 6:52 AM, Rafał Miłecki wrote: >> On 4.04.2023 15:46, Ricardo Cañuelo wrote: >>> On mar 07-02-2023 23:53:27, Rafał Miłecki wrote: >>>> While bringing hardware up we should perform a full reset including the >>>> switch bit (BGMAC_BCMA_IOCTL_SW_RESET aka SICF_SWRST). It's what >>>> specification says and what reference driver does. >>>> >>>> This seems to be critical for the BCM5358. Without this hardware >>>> doesn't >>>> get initialized properly and doesn't seem to transmit or receive any >>>> packets. >>>> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>> >>> KernelCI found this patch causes a regression in the >>> bootrr.deferred-probe-empty test [1] on sun8i-h3-libretech-all-h3-cc >>> [2], see the bisection report for more details [3] >>> >>> Does it make sense to you? >> >> It doesn't seem to make any sense. I guess that on your platform >> /sys/kernel/debug/devices_deferred >> is not empty anymore? >> >> Does your platform use Broadcom Ethernet controller at all? > > I do not believe it does, however according to the log, the driver is > enabled: > > <6>[ 1.819466] bgmac_bcma: Broadcom 47xx GBit MAC driver loaded > > but it should not be probing any device since you don't have any > internal BCMA bus to match gigabit devices with. Later in the log we see: > > 1c22c00.codec sun4i-codec: Failed to register our card > > and most likely as you already wrote the deferred device list might not > be empty. What happened to this? It seems there wasn't any progress since above mail week. But well, seems to be a odd issue anyway (is that one of those issues that CI systems find, but don't cause practical issues in the field?). Hence: can somebody with more knowledge about this please tell if it this is something I can likely drop from the list of tacked regressions? Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke
Hi Thorsten, On 14/4/23 16:04, Linux regression tracking (Thorsten Leemhuis) wrote: > What happened to this? It seems there wasn't any progress since above > mail week. But well, seems to be a odd issue anyway (is that one of > those issues that CI systems find, but don't cause practical issues in > the field?). Hence: can somebody with more knowledge about this please > tell if it this is something I can likely drop from the list of tacked > regressions? From Rafał's answer, I think we can consider this a false positive and move on. Cheers, Ricardo
On 14.04.23 16:08, Ricardo Cañuelo wrote: > Hi Thorsten, > > On 14/4/23 16:04, Linux regression tracking (Thorsten Leemhuis) wrote: >> What happened to this? It seems there wasn't any progress since above >> mail week. But well, seems to be a odd issue anyway (is that one of >> those issues that CI systems find, but don't cause practical issues in >> the field?). Hence: can somebody with more knowledge about this please >> tell if it this is something I can likely drop from the list of tacked >> regressions? > > From Rafał's answer, I think we can consider this a false positive and > move on. great, thx for confirming! #regzbot inconclusive: CI false positive #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index 3038386a5afd..1761df8fb7f9 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -890,13 +890,13 @@ static void bgmac_chip_reset_idm_config(struct bgmac *bgmac) if (iost & BGMAC_BCMA_IOST_ATTACHED) { flags = BGMAC_BCMA_IOCTL_SW_CLKEN; - if (!bgmac->has_robosw) + if (bgmac->in_init || !bgmac->has_robosw) flags |= BGMAC_BCMA_IOCTL_SW_RESET; } bgmac_clk_enable(bgmac, flags); } - if (iost & BGMAC_BCMA_IOST_ATTACHED && !bgmac->has_robosw) + if (iost & BGMAC_BCMA_IOST_ATTACHED && (bgmac->in_init || !bgmac->has_robosw)) bgmac_idm_write(bgmac, BCMA_IOCTL, bgmac_idm_read(bgmac, BCMA_IOCTL) & ~BGMAC_BCMA_IOCTL_SW_RESET); @@ -1490,6 +1490,8 @@ int bgmac_enet_probe(struct bgmac *bgmac) struct net_device *net_dev = bgmac->net_dev; int err; + bgmac->in_init = true; + bgmac_chip_intrs_off(bgmac); net_dev->irq = bgmac->irq; @@ -1542,6 +1544,8 @@ int bgmac_enet_probe(struct bgmac *bgmac) /* Omit FCS from max MTU size */ net_dev->max_mtu = BGMAC_RX_MAX_FRAME_SIZE - ETH_FCS_LEN; + bgmac->in_init = false; + err = register_netdev(bgmac->net_dev); if (err) { dev_err(bgmac->dev, "Cannot register net device\n"); diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h index e05ac92c0650..d73ef262991d 100644 --- a/drivers/net/ethernet/broadcom/bgmac.h +++ b/drivers/net/ethernet/broadcom/bgmac.h @@ -472,6 +472,8 @@ struct bgmac { int irq; u32 int_mask; + bool in_init; + /* Current MAC state */ int mac_speed; int mac_duplex;