[PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Rafał Miłecki]

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(-)

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;
-- 
2.34.1

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Leon Romanovsky]

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>

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Florian Fainelli]

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>
-- 
Florian

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Ricardo Cañuelo]

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

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Rafał Miłecki]

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.

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Linux regression tracking #update (Thorsten Leemhuis)]

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.

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Linux regression tracking (Thorsten Leemhuis)]

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

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Ricardo Cañuelo]

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

Re: [PATCH net] bgmac: fix *initial* chip reset to support BCM5358

[Linux regression tracking #update (Thorsten Leemhuis)]

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.