netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* STMMAC Ethernet Driver support
@ 2023-12-08  6:03 Richard Tresidder
  2023-12-08 18:12 ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Tresidder @ 2023-12-08  6:03 UTC (permalink / raw)
  To: netdev

Hi
    I'm having a problem when checksum offloading in transmit is enabled.
The Mac is in an Altera Cyclone V SOC.
compatible = "altr,socfpga-stmmac", "snps,dwmac-3.70a", "snps,dwmac";

Previous working revision was a 6.3.3 based kernel.
Current testing version is a 6.6.3 LongTerm branch.
None of our patches touch anything in network, just a few custom drivers 
for HDL built in the FPGA.

The 6.3.3 iteration of the STMMAC worked fine out of the box.
And appears to be using Checksum Offload in TX according to the dma_cap 
dump from debugfs.

For the 6.6.3 based driver I was seeing bad CRC's on any tcp data 
transmitted from the MAC, icmp was fine.

I manually disabled the tx_coe cap by setting:
dma_cap->tx_coe = 0;
in dwmac1000_dma.c at about line 258 in lieu of (hw_cap & 
DMA_HW_FEAT_TXCOESEL) >> 16;

This got the interface working.

I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
drivers\net\ethernet\stmicro\stmmac
But nothing is jumping out at me.

I could use a pointer as to where to look to start tracing this.

Cheers
    Richard Tresidder

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-08  6:03 STMMAC Ethernet Driver support Richard Tresidder
@ 2023-12-08 18:12 ` Jakub Kicinski
  2023-12-09  7:06   ` Richard Tresidder
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2023-12-08 18:12 UTC (permalink / raw)
  To: Richard Tresidder; +Cc: netdev

On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
> I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
> drivers\net\ethernet\stmicro\stmmac
> But nothing is jumping out at me.
> 
> I could use a pointer as to where to look to start tracing this.

Bisection is good way to zero in on the bad change if you don't
have much hard to rebase code in your tree.

Otherwise you can dump the relevant registers and the descriptors
(descriptors_status file in debugfs) and see if driver is doing
anything differently on the newer kernel?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-08 18:12 ` Jakub Kicinski
@ 2023-12-09  7:06   ` Richard Tresidder
  2023-12-11  9:13     ` Richard Tresidder
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Tresidder @ 2023-12-09  7:06 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev

On 9/12/2023 2:12 am, Jakub Kicinski wrote:
> On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
>> I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
>> drivers\net\ethernet\stmicro\stmmac
>> But nothing is jumping out at me.
>>
>> I could use a pointer as to where to look to start tracing this.
> Bisection is good way to zero in on the bad change if you don't
> have much hard to rebase code in your tree.
>
> Otherwise you can dump the relevant registers and the descriptors
> (descriptors_status file in debugfs) and see if driver is doing
> anything differently on the newer kernel?
>
Thanks Jakub
   Yep I think I'll have to start bisecting things on Monday.
Luckily to work through this I shouldn't have to merge very much.
Have a great weekend

Cheers
   Richard Tresidder

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-09  7:06   ` Richard Tresidder
@ 2023-12-11  9:13     ` Richard Tresidder
  2023-12-11  9:23       ` Richard Tresidder
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Tresidder @ 2023-12-11  9:13 UTC (permalink / raw)
  To: Jakub Kicinski, vinschen; +Cc: netdev


Richard Tresidder


On 9/12/2023 3:06 pm, Richard Tresidder wrote:
> On 9/12/2023 2:12 am, Jakub Kicinski wrote:
>> On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
>>> I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
>>> drivers\net\ethernet\stmicro\stmmac
>>> But nothing is jumping out at me.
>>>
>>> I could use a pointer as to where to look to start tracing this.
>> Bisection is good way to zero in on the bad change if you don't
>> have much hard to rebase code in your tree.
>>
>> Otherwise you can dump the relevant registers and the descriptors
>> (descriptors_status file in debugfs) and see if driver is doing
>> anything differently on the newer kernel?
>>
> Thanks Jakub
>   Yep I think I'll have to start bisecting things on Monday.
> Luckily to work through this I shouldn't have to merge very much.
> Have a great weekend
>
> Cheers
>   Richard Tresidder
>
Hi Jakub
    Ok the bad commit is the following:
************************************
6b2c6e4a938fece9b539c8085f21d17c5e6eb9de is the first bad commit
commit 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de
Author: Corinna Vinschen <vinschen@redhat.com>
Date:   Mon Apr 17 21:28:45 2023 +0200

     net: stmmac: propagate feature flags to vlan

     stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
     like offloading don't correspond with the general features and it's not
     possible to manipulate features via ethtool -K to affect VLANs.

     Propagate feature flags to vlan features.  Drop TSO feature because
     it does not work on VLANs yet.

     Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
     Link: 
https://lore.kernel.org/r/20230417192845.590034-1-vinschen@redhat.com
     Signed-off-by: Jakub Kicinski <kuba@kernel.org>

  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
  1 file changed, 4 insertions(+)
****************************************
 From back in the work for 6.4-rc1

Theres a single line addition  approx line 7506 
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

ndev->vlan_features |= ndev->features;

Commenting this out and things work again.
We're not using VLANs on this interface, or in this system at all.
Also verified I had reenabled tx crc offload during this test.

Cheers
   Richard Tresidder

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-11  9:13     ` Richard Tresidder
@ 2023-12-11  9:23       ` Richard Tresidder
  2023-12-11 10:03         ` Corinna Vinschen
  2023-12-11 16:16         ` Andrew Lunn
  0 siblings, 2 replies; 14+ messages in thread
From: Richard Tresidder @ 2023-12-11  9:23 UTC (permalink / raw)
  To: Jakub Kicinski, vinschen; +Cc: netdev


> Richard Tresidder
>
>
> On 9/12/2023 3:06 pm, Richard Tresidder wrote:
>> On 9/12/2023 2:12 am, Jakub Kicinski wrote:
>>> On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
>>>> I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
>>>> drivers\net\ethernet\stmicro\stmmac
>>>> But nothing is jumping out at me.
>>>>
>>>> I could use a pointer as to where to look to start tracing this.
>>> Bisection is good way to zero in on the bad change if you don't
>>> have much hard to rebase code in your tree.
>>>
>>> Otherwise you can dump the relevant registers and the descriptors
>>> (descriptors_status file in debugfs) and see if driver is doing
>>> anything differently on the newer kernel?
>>>
>> Thanks Jakub
>>   Yep I think I'll have to start bisecting things on Monday.
>> Luckily to work through this I shouldn't have to merge very much.
>> Have a great weekend
>>
>> Cheers
>>   Richard Tresidder
>>
> Hi Jakub
>    Ok the bad commit is the following:
> ************************************
> 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de is the first bad commit
> commit 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de
> Author: Corinna Vinschen <vinschen@redhat.com>
> Date:   Mon Apr 17 21:28:45 2023 +0200
>
>     net: stmmac: propagate feature flags to vlan
>
>     stmmac_dev_probe doesn't propagate feature flags to VLANs.  So 
> features
>     like offloading don't correspond with the general features and 
> it's not
>     possible to manipulate features via ethtool -K to affect VLANs.
>
>     Propagate feature flags to vlan features.  Drop TSO feature because
>     it does not work on VLANs yet.
>
>     Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
>     Link: 
> https://lore.kernel.org/r/20230417192845.590034-1-vinschen@redhat.com
>     Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> ****************************************
> From back in the work for 6.4-rc1
>
> Theres a single line addition  approx line 7506 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>
> ndev->vlan_features |= ndev->features;
>
> Commenting this out and things work again.
> ***   BAD COMMENT!  We're not using VLANs on this interface, or in 
> this system at all. ***   BAD COMMENT!
> Also verified I had reenabled tx crc offload during this test.
>
> Cheers
>   Richard Tresidder
>
Actually my apologies we are using VLAN. I'd forgot how a switch was 
linked in..
We use the SOC's internal  STMMAC interface to connect to a Marvel 
switch IC and expose each port individually using vlan, I'd forgot that 
part.
It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085' 
compatible driver  in drivers\net\dsa\mv88e6xxx

Cheers
    Richard Tresidder


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-11  9:23       ` Richard Tresidder
@ 2023-12-11 10:03         ` Corinna Vinschen
  2023-12-12  4:13           ` Richard Tresidder
  2023-12-11 16:16         ` Andrew Lunn
  1 sibling, 1 reply; 14+ messages in thread
From: Corinna Vinschen @ 2023-12-11 10:03 UTC (permalink / raw)
  To: Richard Tresidder; +Cc: Jakub Kicinski, netdev

On Dec 11 17:23, Richard Tresidder wrote:
> 
> > Richard Tresidder
> > 
> > 
> > On 9/12/2023 3:06 pm, Richard Tresidder wrote:
> > > On 9/12/2023 2:12 am, Jakub Kicinski wrote:
> > > > On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
> > > > > I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
> > > > > drivers\net\ethernet\stmicro\stmmac
> > > > > But nothing is jumping out at me.
> > > > > 
> > > > > I could use a pointer as to where to look to start tracing this.
> > > > Bisection is good way to zero in on the bad change if you don't
> > > > have much hard to rebase code in your tree.
> > > > 
> > > > Otherwise you can dump the relevant registers and the descriptors
> > > > (descriptors_status file in debugfs) and see if driver is doing
> > > > anything differently on the newer kernel?
> > > > 
> > > Thanks Jakub
> > >   Yep I think I'll have to start bisecting things on Monday.
> > > Luckily to work through this I shouldn't have to merge very much.
> > > Have a great weekend
> > > 
> > > Cheers
> > >   Richard Tresidder
> > > 
> > Hi Jakub
> >    Ok the bad commit is the following:
> > ************************************
> > 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de is the first bad commit
> > commit 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de
> > Author: Corinna Vinschen <vinschen@redhat.com>
> > Date:   Mon Apr 17 21:28:45 2023 +0200
> > 
> >     net: stmmac: propagate feature flags to vlan
> > 
> >     stmmac_dev_probe doesn't propagate feature flags to VLANs.  So
> > features
> >     like offloading don't correspond with the general features and it's
> > not
> >     possible to manipulate features via ethtool -K to affect VLANs.
> > 
> >     Propagate feature flags to vlan features.  Drop TSO feature because
> >     it does not work on VLANs yet.
> > 
> >     Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> >     Link:
> > https://lore.kernel.org/r/20230417192845.590034-1-vinschen@redhat.com
> >     Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > 
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > ****************************************
> > From back in the work for 6.4-rc1
> > 
> > Theres a single line addition  approx line 7506
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > 
> > ndev->vlan_features |= ndev->features;

This is missing the 2nd line

    ndev->vlan_features &= ~NETIF_F_TSO;

I assume that another flag or two have to be dropped from being
propagated as well.  That may depend on the platform, just like the
feature flags depend on STMMAC_VLAN_TAG_USED, for instance.

I'm sorry, but I'm not working on stmmac anymore.  Can you perhaps test
removing flags from vlan_features and see what actual flag is breaking
your scenario?

Other than that, maybe reverting the patch is the better option and the
vlan_feature flags should be set explicitely in the various
platform-specific code.


Corinna


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-11  9:23       ` Richard Tresidder
  2023-12-11 10:03         ` Corinna Vinschen
@ 2023-12-11 16:16         ` Andrew Lunn
  2023-12-12  3:57           ` Richard Tresidder
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2023-12-11 16:16 UTC (permalink / raw)
  To: Richard Tresidder; +Cc: Jakub Kicinski, vinschen, netdev

> We use the SOC's internal  STMMAC interface to connect to a Marvel switch IC
> and expose each port individually using vlan, I'd forgot that part.
> It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085'
> compatible driver  in drivers\net\dsa\mv88e6xxx

Its odd you need VLANs. Each port should already be exposed to the
host as netdev interfaces. That is what DSA does.

     Andrew

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-11 16:16         ` Andrew Lunn
@ 2023-12-12  3:57           ` Richard Tresidder
  2023-12-12  9:27             ` Serge Semin
  2023-12-13  9:17             ` Andrew Lunn
  0 siblings, 2 replies; 14+ messages in thread
From: Richard Tresidder @ 2023-12-12  3:57 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Jakub Kicinski, vinschen, netdev


<font face="monospace">Richard Tresidder</font>


On 12/12/2023 12:16 am, Andrew Lunn wrote:
>> We use the SOC's internal  STMMAC interface to connect to a Marvel switch IC
>> and expose each port individually using vlan, I'd forgot that part.
>> It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085'
>> compatible driver  in drivers\net\dsa\mv88e6xxx
> Its odd you need VLANs. Each port should already be exposed to the
> host as netdev interfaces. That is what DSA does.
>
>       Andrew
Hi Andrew
    I'll read further on that one as this is the first time I've had to 
dig into this side of the system.
It had always "just worked".
The ports show up in an 'ip l' list in the same style as a vlan with an 
@ symbol, naming isn't quite vlan style though.
That in concert with the fact this 'vlan_feature' line broke things has 
possibly distorted my view of how they're propagated.
It's a rather trimmed down busybox image, so I'm missing some tools I'd 
usually use to examine stuff.

This is the config in the dts
**************************************
//------------------------------------------------------------------------------
// connected to dsa network switch
&gmac1 {
   clock-names = "stmmaceth", "clk_ptp_ref";
   clocks = <&emac1_clk &hps_eosc1>;
   f2h_ptp_ref_clk;
   fixed-link {
     speed = <1000>;
     full-duplex;
   };
};

//------------------------------------------------------------------------------
&mdio1 {
   #address-cells = <1>;
   #size-cells = <0>;

   switch0: switch0@0 {
     compatible = "marvell,mv88e6085";
     #address-cells = <1>;
     reg = <0>;
     //reset-gpios = <&pio_a0 2 GPIO_ACTIVE_LOW>;

     dsa,member = <0 0>;

     ports {
       #address-cells = <1>;
       #size-cells = <0>;

       port@2 {
         reg = <2>;
         label = "lan1";
         phy-handle = <&switch1phy2>;
       };

       port@3 {
         reg = <3>;
         label = "lan2";
         phy-handle = <&switch1phy3>;
       };

       port@4 {
         reg = <4>;
         label = "lan3";
         phy-handle = <&switch1phy4>;
       };

       port@5 {
         reg = <5>;
         label = "wifi";
         fixed-link {
           speed = <100>;
           full-duplex;
         };
       };

       port@6 {
         reg = <6>;
         label = "cpu";
         ethernet = <&gmac1>;
         fixed-link {
           speed = <1000>;
           full-duplex;
         };
       };

     };

     mdio {
       #address-cells = <1>;
       #size-cells = <0>;
       switch1phy2: switch1phy2@2 {
         reg = <2>;
         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx 
Energy Detect, no FLPs sents
       };
       switch1phy3: switch1phy3@3 {
         reg = <3>;
         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx 
Energy Detect, no FLPs sents
       };
       switch1phy4: switch1phy4@4 {
         reg = <4>;
         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx 
Energy Detect, no FLPs sents
       };
     };

     };
};
*************************************************

This is how they appear using 'ip l'
The @ symbol got me as I've usually associated this with vlan's in my 
day to day networking.
But the config files don't configure them as vlans.
*************************************************
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue qlen 1000
     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1508 qdisc mq qlen 1000
     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
3: sit0@NONE: <NOARP> mtu 1480 qdisc noop qlen 1000
     link/sit 0.0.0.0 brd 0.0.0.0
4: lan1@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue 
qlen 1000
     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
5: lan2@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue 
qlen 1000
     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
6: lan3@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue 
qlen 1000
     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
7: wifi@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue 
qlen 1000
     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff

*************************************************

And these are the systemd config files
No vlan mentioned in there..
*************************************************
/etc/systemd/network/eth0.network
[Match]
Name=eth0

[Network]

/etc/systemd/network/lan.network
[Match]
Name=lan*

[Network]
DHCP=yes
BindCarrier=eth0
LinkLocalAddressing=yes

[DHCP]
UseDomains=yes
ClientIdentifier=mac

*************************************************

Cheers
    Richard

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-11 10:03         ` Corinna Vinschen
@ 2023-12-12  4:13           ` Richard Tresidder
  2023-12-12  6:49             ` Richard Tresidder
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Tresidder @ 2023-12-12  4:13 UTC (permalink / raw)
  To: vinschen, Jakub Kicinski, netdev

On 11/12/2023 6:03 pm, Corinna Vinschen wrote:

> On Dec 11 17:23, Richard Tresidder wrote:
>
>>> Richard Tresidder
>>>
>>>
>>> On 9/12/2023 3:06 pm, Richard Tresidder wrote:
>>>> On 9/12/2023 2:12 am, Jakub Kicinski wrote:
>>>>> On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
>>>>>> I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
>>>>>> drivers\net\ethernet\stmicro\stmmac
>>>>>> But nothing is jumping out at me.
>>>>>>
>>>>>> I could use a pointer as to where to look to start tracing this.
>>>>> Bisection is good way to zero in on the bad change if you don't
>>>>> have much hard to rebase code in your tree.
>>>>>
>>>>> Otherwise you can dump the relevant registers and the descriptors
>>>>> (descriptors_status file in debugfs) and see if driver is doing
>>>>> anything differently on the newer kernel?
>>>>>
>>>> Thanks Jakub
>>>>    Yep I think I'll have to start bisecting things on Monday.
>>>> Luckily to work through this I shouldn't have to merge very much.
>>>> Have a great weekend
>>>>
>>>> Cheers
>>>>    Richard Tresidder
>>>>
>>> Hi Jakub
>>>     Ok the bad commit is the following:
>>> ************************************
>>> 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de is the first bad commit
>>> commit 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de
>>> Author: Corinna Vinschen <vinschen@redhat.com>
>>> Date:   Mon Apr 17 21:28:45 2023 +0200
>>>
>>>      net: stmmac: propagate feature flags to vlan
>>>
>>>      stmmac_dev_probe doesn't propagate feature flags to VLANs.  So
>>> features
>>>      like offloading don't correspond with the general features and it's
>>> not
>>>      possible to manipulate features via ethtool -K to affect VLANs.
>>>
>>>      Propagate feature flags to vlan features.  Drop TSO feature because
>>>      it does not work on VLANs yet.
>>>
>>>      Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
>>>      Link:
>>> https://lore.kernel.org/r/20230417192845.590034-1-vinschen@redhat.com
>>>      Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>>
>>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>> ****************************************
>>>  From back in the work for 6.4-rc1
>>>
>>> Theres a single line addition  approx line 7506
>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>
>>> ndev->vlan_features |= ndev->features;
> This is missing the 2nd line
>
>      ndev->vlan_features &= ~NETIF_F_TSO;
>
> I assume that another flag or two have to be dropped from being
> propagated as well.  That may depend on the platform, just like the
> feature flags depend on STMMAC_VLAN_TAG_USED, for instance.
>
> I'm sorry, but I'm not working on stmmac anymore.  Can you perhaps test
> removing flags from vlan_features and see what actual flag is breaking
> your scenario?
>
> Other than that, maybe reverting the patch is the better option and the
> vlan_feature flags should be set explicitely in the various
> platform-specific code.
>
>
> Corinna
>
Hi Corinna    Sorry you are correct, there is that additional line in 
the patch. I'd had an earlier version of it open on lore.kernel.org I 
was reading while I was performing the bisect. I'm rather new down in 
the network driver infrastructure, so I'm learning as I go. But I'll 
have a crack at understanding the flags and try to seeing whats causing 
the problem specifically. No problems if you're off on other things, 
just included you in the look as it was that patch that seemed to 
uncover a problem with how things are chained.

Cheers     Richard


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-12  4:13           ` Richard Tresidder
@ 2023-12-12  6:49             ` Richard Tresidder
  0 siblings, 0 replies; 14+ messages in thread
From: Richard Tresidder @ 2023-12-12  6:49 UTC (permalink / raw)
  To: Jakub Kicinski, Andrew Lunn, netdev

On 12/12/2023 12:13 pm, Richard Tresidder wrote:

> On 11/12/2023 6:03 pm, Corinna Vinschen wrote:
>> On Dec 11 17:23, Richard Tresidder wrote:
>>>> Richard Tresidder
>>>> On 9/12/2023 3:06 pm, Richard Tresidder wrote:
>>>>> On 9/12/2023 2:12 am, Jakub Kicinski wrote:
>>>>>> On Fri, 8 Dec 2023 14:03:25 +0800 Richard Tresidder wrote:
>>>>>>> I've looked through the diffset 6.3.3 >>> 6.6.3 on the driver
>>>>>>> drivers\net\ethernet\stmicro\stmmac
>>>>>>> But nothing is jumping out at me.
>>>>>>> I could use a pointer as to where to look to start tracing this.
>>>>>> Bisection is good way to zero in on the bad change if you don't
>>>>>> have much hard to rebase code in your tree.
>>>>>> Otherwise you can dump the relevant registers and the descriptors
>>>>>> (descriptors_status file in debugfs) and see if driver is doing
>>>>>> anything differently on the newer kernel?
>>>>> Thanks Jakub
>>>>>     Yep I think I'll have to start bisecting things on Monday.
>>>>> Luckily to work through this I shouldn't have to merge very much.
>>>>> Have a great weekend
>>>>> Cheers
>>>>>     Richard Tresidder
>>>> Hi Jakub
>>>>      Ok the bad commit is the following:
>>>> ************************************
>>>> 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de is the first bad commit
>>>> commit 6b2c6e4a938fece9b539c8085f21d17c5e6eb9de
>>>> Author: Corinna Vinschen <vinschen@redhat.com>
>>>> Date:   Mon Apr 17 21:28:45 2023 +0200
>>>>       net: stmmac: propagate feature flags to vlan
>>>>       stmmac_dev_probe doesn't propagate feature flags to VLANs.  So
>>>> features
>>>>       like offloading don't correspond with the general features and it's
>>>> not
>>>>       possible to manipulate features via ethtool -K to affect VLANs.
>>>>       Propagate feature flags to vlan features.  Drop TSO feature because
>>>>       it does not work on VLANs yet.
>>>>       Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
>>>>       Link:
>>>> https://lore.kernel.org/r/20230417192845.590034-1-vinschen@redhat.com
>>>>       Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>>>    drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>> ****************************************
>>>>   From back in the work for 6.4-rc1
>>>> Theres a single line addition  approx line 7506
>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>> ndev->vlan_features |= ndev->features;
>> This is missing the 2nd line
>>       ndev->vlan_features &= ~NETIF_F_TSO;
>> I assume that another flag or two have to be dropped from being
>> propagated as well.  That may depend on the platform, just like the
>> feature flags depend on STMMAC_VLAN_TAG_USED, for instance.
>> I'm sorry, but I'm not working on stmmac anymore.  Can you perhaps test
>> removing flags from vlan_features and see what actual flag is breaking
>> your scenario?
>> Other than that, maybe reverting the patch is the better option and the
>> vlan_feature flags should be set explicitely in the various
>> platform-specific code.
>> Corinna
> Hi Corinna    Sorry you are correct, there is that additional line in
> the patch. I'd had an earlier version of it open on lore.kernel.org I
> was reading while I was performing the bisect. I'm rather new down in
> the network driver infrastructure, so I'm learning as I go. But I'll
> have a crack at understanding the flags and try to seeing whats causing
> the problem specifically. No problems if you're off on other things,
> just included you in the look as it was that patch that seemed to
> uncover a problem with how things are chained.
> Cheers     Richard
These are the flags that are being applied to the vlan_features set.
ndev->features  0x0000410000000133
ndev->vlan_features is empty prior to this.

So we have
NETIF_F_SG_BIT
NETIF_F_IP_CSUM_BIT

NETIF_F_IPV6_CSUM_BIT
NETIF_F_HIGHDMA_BIT

NETIF_F_HW_VLAN_CTAG_RX_BIT      Interestingly not 
NETIF_F_HW_VLAN_CTAG_TX_BIT   also

NETIF_F_RXCSUM_BIT
NETIF_F_HW_VLAN_STAG_RX_BIT

These flags are set with the following macros:
NETIF_F_SG
NETIF_F_IP_CSUM
NETIF_F_IPV6_CSUM
NETIF_F_HIGHDMA
NETIF_F_HW_VLAN_CTAG_RX
NETIF_F_RXCSUM
NETIF_F_HW_VLAN_STAG_RX

Down in the slave ports on the Marvel switch we have:

In function   int dsa_slave_create(struct dsa_port *port)
about line 2640 of slave.c
     slave_dev->vlan_features = master->vlan_features;

and a few lines later, this function   void 
dsa_slave_setup_tagger(struct net_device *slave)    is called
about line 2553 of slave.c
     slave->features = master->vlan_features | NETIF_F_HW_TC;

Possibly a bit superfluous in regards to the fact that 
master->vlan_features is already applied in dsa_slave_create?
luckily it doesn't look like anything touches that before going to the 
setu pfunction.
Anyway.
Added a debug print prior to that to get the features values prior to copy.
(Without modified vlan_features in stmmac driver)
mv88e6085 stmmac-0:00 wifi (uninitialized): slave->features 
0x0000000000000000  master->vlan_features   0x0000000000000020
So thats just:
NETIF_F_HIGHDMA_BIT

And with the modified vlan features in the stmmac driver we get:
mv88e6085 stmmac-0:00 wifi (uninitialized): slave->features 
0x0000000000000000  master->vlan_features   0x0000410000000133

So the stmmac vlan_feature flags are ending up in the marvells port 
feature flags?
Kind of makes sense? put I'm not sure how that gets applied down the 
chain at each stage.
Thats about where I'm at sofar as I'm not sure where this "feature" flag 
gets used.

Possibly
nt skb_csum_hwoffload_help(struct sk_buff *skb,
                 const netdev_features_t features)

But thats getting well into the mud for me..

Cheers
     Richard


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-12  3:57           ` Richard Tresidder
@ 2023-12-12  9:27             ` Serge Semin
  2023-12-13  4:55               ` Richard Tresidder
  2023-12-13  9:17             ` Andrew Lunn
  1 sibling, 1 reply; 14+ messages in thread
From: Serge Semin @ 2023-12-12  9:27 UTC (permalink / raw)
  To: Richard Tresidder; +Cc: Andrew Lunn, Jakub Kicinski, vinschen, netdev

On Tue, Dec 12, 2023 at 11:57:22AM +0800, Richard Tresidder wrote:
> 
> <font face="monospace">Richard Tresidder</font>
> 
> 
> On 12/12/2023 12:16 am, Andrew Lunn wrote:
> > > We use the SOC's internal  STMMAC interface to connect to a Marvel switch IC
> > > and expose each port individually using vlan, I'd forgot that part.
> > > It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085'
> > > compatible driver  in drivers\net\dsa\mv88e6xxx
> > Its odd you need VLANs. Each port should already be exposed to the
> > host as netdev interfaces. That is what DSA does.
> > 
> >       Andrew
> Hi Andrew
>    I'll read further on that one as this is the first time I've had to dig
> into this side of the system.
> It had always "just worked".
> The ports show up in an 'ip l' list in the same style as a vlan with an @
> symbol, naming isn't quite vlan style though.
> That in concert with the fact this 'vlan_feature' line broke things has
> possibly distorted my view of how they're propagated.
> It's a rather trimmed down busybox image, so I'm missing some tools I'd
> usually use to examine stuff.
> 
> This is the config in the dts
> **************************************
> //------------------------------------------------------------------------------
> // connected to dsa network switch
> &gmac1 {

>   clock-names = "stmmaceth", "clk_ptp_ref";

Just a side note irrelevant to the topic. You might be interested to
know STMMAC driver expects to have the PTP reference clock source
passed with the "ptp_ref" name, not "clk_ptp_ref".

-Serge(y)

>   clocks = <&emac1_clk &hps_eosc1>;
>   f2h_ptp_ref_clk;
>   fixed-link {
>     speed = <1000>;
>     full-duplex;
>   };
> };
> 
> //------------------------------------------------------------------------------
> &mdio1 {
>   #address-cells = <1>;
>   #size-cells = <0>;
> 
>   switch0: switch0@0 {
>     compatible = "marvell,mv88e6085";
>     #address-cells = <1>;
>     reg = <0>;
>     //reset-gpios = <&pio_a0 2 GPIO_ACTIVE_LOW>;
> 
>     dsa,member = <0 0>;
> 
>     ports {
>       #address-cells = <1>;
>       #size-cells = <0>;
> 
>       port@2 {
>         reg = <2>;
>         label = "lan1";
>         phy-handle = <&switch1phy2>;
>       };
> 
>       port@3 {
>         reg = <3>;
>         label = "lan2";
>         phy-handle = <&switch1phy3>;
>       };
> 
>       port@4 {
>         reg = <4>;
>         label = "lan3";
>         phy-handle = <&switch1phy4>;
>       };
> 
>       port@5 {
>         reg = <5>;
>         label = "wifi";
>         fixed-link {
>           speed = <100>;
>           full-duplex;
>         };
>       };
> 
>       port@6 {
>         reg = <6>;
>         label = "cpu";
>         ethernet = <&gmac1>;
>         fixed-link {
>           speed = <1000>;
>           full-duplex;
>         };
>       };
> 
>     };
> 
>     mdio {
>       #address-cells = <1>;
>       #size-cells = <0>;
>       switch1phy2: switch1phy2@2 {
>         reg = <2>;
>         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
> Detect, no FLPs sents
>       };
>       switch1phy3: switch1phy3@3 {
>         reg = <3>;
>         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
> Detect, no FLPs sents
>       };
>       switch1phy4: switch1phy4@4 {
>         reg = <4>;
>         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
> Detect, no FLPs sents
>       };
>     };
> 
>     };
> };
> *************************************************
> 
> This is how they appear using 'ip l'
> The @ symbol got me as I've usually associated this with vlan's in my day to
> day networking.
> But the config files don't configure them as vlans.
> *************************************************
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1508 qdisc mq qlen 1000
>     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
> 3: sit0@NONE: <NOARP> mtu 1480 qdisc noop qlen 1000
>     link/sit 0.0.0.0 brd 0.0.0.0
> 4: lan1@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue
> qlen 1000
>     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
> 5: lan2@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue qlen
> 1000
>     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
> 6: lan3@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue
> qlen 1000
>     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
> 7: wifi@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue qlen
> 1000
>     link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
> 
> *************************************************
> 
> And these are the systemd config files
> No vlan mentioned in there..
> *************************************************
> /etc/systemd/network/eth0.network
> [Match]
> Name=eth0
> 
> [Network]
> 
> /etc/systemd/network/lan.network
> [Match]
> Name=lan*
> 
> [Network]
> DHCP=yes
> BindCarrier=eth0
> LinkLocalAddressing=yes
> 
> [DHCP]
> UseDomains=yes
> ClientIdentifier=mac
> 
> *************************************************
> 
> Cheers
>    Richard
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-12  9:27             ` Serge Semin
@ 2023-12-13  4:55               ` Richard Tresidder
  0 siblings, 0 replies; 14+ messages in thread
From: Richard Tresidder @ 2023-12-13  4:55 UTC (permalink / raw)
  To: Serge Semin; +Cc: Andrew Lunn, Jakub Kicinski, vinschen, netdev

On 12/12/2023 5:27 pm, Serge Semin wrote:
> On Tue, Dec 12, 2023 at 11:57:22AM +0800, Richard Tresidder wrote:
>> <font face="monospace">Richard Tresidder</font>
>>
>>
>> On 12/12/2023 12:16 am, Andrew Lunn wrote:
>>>> We use the SOC's internal  STMMAC interface to connect to a Marvel switch IC
>>>> and expose each port individually using vlan, I'd forgot that part.
>>>> It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085'
>>>> compatible driver  in drivers\net\dsa\mv88e6xxx
>>> Its odd you need VLANs. Each port should already be exposed to the
>>> host as netdev interfaces. That is what DSA does.
>>>
>>>        Andrew
>> Hi Andrew
>>     I'll read further on that one as this is the first time I've had to dig
>> into this side of the system.
>> It had always "just worked".
>> The ports show up in an 'ip l' list in the same style as a vlan with an @
>> symbol, naming isn't quite vlan style though.
>> That in concert with the fact this 'vlan_feature' line broke things has
>> possibly distorted my view of how they're propagated.
>> It's a rather trimmed down busybox image, so I'm missing some tools I'd
>> usually use to examine stuff.
>>
>> This is the config in the dts
>> **************************************
>> //------------------------------------------------------------------------------
>> // connected to dsa network switch
>> &gmac1 {
>>    clock-names = "stmmaceth", "clk_ptp_ref";
> Just a side note irrelevant to the topic. You might be interested to
> know STMMAC driver expects to have the PTP reference clock source
> passed with the "ptp_ref" name, not "clk_ptp_ref".
>
> -Serge(y)
Cheers for that Surge
The dts for this device hasn't been updated in a while!
It was based on a 4.7 series kernel
I vaguely remember coming across this change on clock naming a while 
back when dealing with a pll device driver but looks like we missed this 
one!
Good catch

  Cheers Richard
>>    clocks = <&emac1_clk &hps_eosc1>;
>>    f2h_ptp_ref_clk;
>>    fixed-link {
>>      speed = <1000>;
>>      full-duplex;
>>    };
>> };
>>
>> //------------------------------------------------------------------------------
>> &mdio1 {
>>    #address-cells = <1>;
>>    #size-cells = <0>;
>>
>>    switch0: switch0@0 {
>>      compatible = "marvell,mv88e6085";
>>      #address-cells = <1>;
>>      reg = <0>;
>>      //reset-gpios = <&pio_a0 2 GPIO_ACTIVE_LOW>;
>>
>>      dsa,member = <0 0>;
>>
>>      ports {
>>        #address-cells = <1>;
>>        #size-cells = <0>;
>>
>>        port@2 {
>>          reg = <2>;
>>          label = "lan1";
>>          phy-handle = <&switch1phy2>;
>>        };
>>
>>        port@3 {
>>          reg = <3>;
>>          label = "lan2";
>>          phy-handle = <&switch1phy3>;
>>        };
>>
>>        port@4 {
>>          reg = <4>;
>>          label = "lan3";
>>          phy-handle = <&switch1phy4>;
>>        };
>>
>>        port@5 {
>>          reg = <5>;
>>          label = "wifi";
>>          fixed-link {
>>            speed = <100>;
>>            full-duplex;
>>          };
>>        };
>>
>>        port@6 {
>>          reg = <6>;
>>          label = "cpu";
>>          ethernet = <&gmac1>;
>>          fixed-link {
>>            speed = <1000>;
>>            full-duplex;
>>          };
>>        };
>>
>>      };
>>
>>      mdio {
>>        #address-cells = <1>;
>>        #size-cells = <0>;
>>        switch1phy2: switch1phy2@2 {
>>          reg = <2>;
>>          marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
>> Detect, no FLPs sents
>>        };
>>        switch1phy3: switch1phy3@3 {
>>          reg = <3>;
>>          marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
>> Detect, no FLPs sents
>>        };
>>        switch1phy4: switch1phy4@4 {
>>          reg = <4>;
>>          marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
>> Detect, no FLPs sents
>>        };
>>      };
>>
>>      };
>> };
>> *************************************************
>>
>> This is how they appear using 'ip l'
>> The @ symbol got me as I've usually associated this with vlan's in my day to
>> day networking.
>> But the config files don't configure them as vlans.
>> *************************************************
>> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue qlen 1000
>>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1508 qdisc mq qlen 1000
>>      link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
>> 3: sit0@NONE: <NOARP> mtu 1480 qdisc noop qlen 1000
>>      link/sit 0.0.0.0 brd 0.0.0.0
>> 4: lan1@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue
>> qlen 1000
>>      link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
>> 5: lan2@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue qlen
>> 1000
>>      link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
>> 6: lan3@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue
>> qlen 1000
>>      link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
>> 7: wifi@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue qlen
>> 1000
>>      link/ether 04:91:62:cf:4f:06 brd ff:ff:ff:ff:ff:ff
>>
>> *************************************************
>>
>> And these are the systemd config files
>> No vlan mentioned in there..
>> *************************************************
>> /etc/systemd/network/eth0.network
>> [Match]
>> Name=eth0
>>
>> [Network]
>>
>> /etc/systemd/network/lan.network
>> [Match]
>> Name=lan*
>>
>> [Network]
>> DHCP=yes
>> BindCarrier=eth0
>> LinkLocalAddressing=yes
>>
>> [DHCP]
>> UseDomains=yes
>> ClientIdentifier=mac
>>
>> *************************************************
>>
>> Cheers
>>     Richard
>>
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-12  3:57           ` Richard Tresidder
  2023-12-12  9:27             ` Serge Semin
@ 2023-12-13  9:17             ` Andrew Lunn
  2023-12-13 10:15               ` Richard Tresidder
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2023-12-13  9:17 UTC (permalink / raw)
  To: Richard Tresidder; +Cc: Jakub Kicinski, vinschen, netdev

On Tue, Dec 12, 2023 at 11:57:22AM +0800, Richard Tresidder wrote:
> 
> <font face="monospace">Richard Tresidder</font>
> 
> 
> On 12/12/2023 12:16 am, Andrew Lunn wrote:
> > > We use the SOC's internal  STMMAC interface to connect to a Marvel switch IC
> > > and expose each port individually using vlan, I'd forgot that part.
> > > It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085'
> > > compatible driver  in drivers\net\dsa\mv88e6xxx
> > Its odd you need VLANs. Each port should already be exposed to the
> > host as netdev interfaces. That is what DSA does.
> > 
> >       Andrew
> Hi Andrew
>    I'll read further on that one as this is the first time I've had to dig
> into this side of the system.
> It had always "just worked".
> The ports show up in an 'ip l' list in the same style as a vlan with an @
> symbol, naming isn't quite vlan style though.
> That in concert with the fact this 'vlan_feature' line broke things has
> possibly distorted my view of how they're propagated.
> It's a rather trimmed down busybox image, so I'm missing some tools I'd
> usually use to examine stuff.
> 
> This is the config in the dts
> **************************************
> //------------------------------------------------------------------------------
> // connected to dsa network switch
> &gmac1 {
>   clock-names = "stmmaceth", "clk_ptp_ref";
>   clocks = <&emac1_clk &hps_eosc1>;
>   f2h_ptp_ref_clk;
>   fixed-link {
>     speed = <1000>;
>     full-duplex;
>   };
> };
> 
> //------------------------------------------------------------------------------
> &mdio1 {
>   #address-cells = <1>;
>   #size-cells = <0>;
> 
>   switch0: switch0@0 {
>     compatible = "marvell,mv88e6085";
>     #address-cells = <1>;
>     reg = <0>;
>     //reset-gpios = <&pio_a0 2 GPIO_ACTIVE_LOW>;
> 
>     dsa,member = <0 0>;
> 
>     ports {
>       #address-cells = <1>;
>       #size-cells = <0>;
> 
>       port@2 {
>         reg = <2>;
>         label = "lan1";
>         phy-handle = <&switch1phy2>;
>       };
> 
>       port@3 {
>         reg = <3>;
>         label = "lan2";
>         phy-handle = <&switch1phy3>;
>       };
> 
>       port@4 {
>         reg = <4>;
>         label = "lan3";
>         phy-handle = <&switch1phy4>;
>       };
> 
>       port@5 {
>         reg = <5>;
>         label = "wifi";
>         fixed-link {
>           speed = <100>;
>           full-duplex;
>         };
>       };
> 
>       port@6 {
>         reg = <6>;
>         label = "cpu";
>         ethernet = <&gmac1>;
>         fixed-link {
>           speed = <1000>;
>           full-duplex;
>         };
>       };
> 
>     };
> 
>     mdio {
>       #address-cells = <1>;
>       #size-cells = <0>;
>       switch1phy2: switch1phy2@2 {
>         reg = <2>;
>         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
> Detect, no FLPs sents
>       };
>       switch1phy3: switch1phy3@3 {
>         reg = <3>;
>         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
> Detect, no FLPs sents
>       };
>       switch1phy4: switch1phy4@4 {
>         reg = <4>;
>         marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
> Detect, no FLPs sents
>       };
>     };
> 
>     };
> };

That all looks normal, expect the marvell,reg-init. That is a pretty
ugly hack, from years and years ago, which should not be used any
more. It would be better to add a DT property for what you want, or a
PHY tunable.


> This is how they appear using 'ip l'
> The @ symbol got me as I've usually associated this with vlan's in my day to
> day networking.

The @ is just trying to show there is a relationship between to
interfaces. Its a VLAN on top of a base interface, or its a DSA user
port on top of a conduit interface.

So there is nothing odd at all here. What i have seen is user space
hacks to run Marvell SDK to program the switch to map a VLAN to a
port. There is no point doing that when you have a perfectly good
kernel driver.

     Andrew

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: STMMAC Ethernet Driver support
  2023-12-13  9:17             ` Andrew Lunn
@ 2023-12-13 10:15               ` Richard Tresidder
  0 siblings, 0 replies; 14+ messages in thread
From: Richard Tresidder @ 2023-12-13 10:15 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Jakub Kicinski, vinschen, netdev

On 13/12/2023 5:17 pm, Andrew Lunn wrote:

> On Tue, Dec 12, 2023 at 11:57:22AM +0800, Richard Tresidder wrote:
>> <font face="monospace">Richard Tresidder</font>
>>
>>
>> On 12/12/2023 12:16 am, Andrew Lunn wrote:
>>>> We use the SOC's internal  STMMAC interface to connect to a Marvel switch IC
>>>> and expose each port individually using vlan, I'd forgot that part.
>>>> It's an  88E6352-xx-TFJ2I000  device utilising the 'marvell,mv88e6085'
>>>> compatible driver  in drivers\net\dsa\mv88e6xxx
>>> Its odd you need VLANs. Each port should already be exposed to the
>>> host as netdev interfaces. That is what DSA does.
>>>
>>>        Andrew
>> Hi Andrew
>>     I'll read further on that one as this is the first time I've had to dig
>> into this side of the system.
>> It had always "just worked".
>> The ports show up in an 'ip l' list in the same style as a vlan with an @
>> symbol, naming isn't quite vlan style though.
>> That in concert with the fact this 'vlan_feature' line broke things has
>> possibly distorted my view of how they're propagated.
>> It's a rather trimmed down busybox image, so I'm missing some tools I'd
>> usually use to examine stuff.
>>
>> This is the config in the dts
>> **************************************
>> //------------------------------------------------------------------------------
>> // connected to dsa network switch
>> &gmac1 {
>>    clock-names = "stmmaceth", "clk_ptp_ref";
>>    clocks = <&emac1_clk &hps_eosc1>;
>>    f2h_ptp_ref_clk;
>>    fixed-link {
>>      speed = <1000>;
>>      full-duplex;
>>    };
>> };
>>
>> //------------------------------------------------------------------------------
>> &mdio1 {
>>    #address-cells = <1>;
>>    #size-cells = <0>;
>>
>>    switch0: switch0@0 {
>>      compatible = "marvell,mv88e6085";
>>      #address-cells = <1>;
>>      reg = <0>;
>>      //reset-gpios = <&pio_a0 2 GPIO_ACTIVE_LOW>;
>>
>>      dsa,member = <0 0>;
>>
>>      ports {
>>        #address-cells = <1>;
>>        #size-cells = <0>;
>>
>>        port@2 {
>>          reg = <2>;
>>          label = "lan1";
>>          phy-handle = <&switch1phy2>;
>>        };
>>
>>        port@3 {
>>          reg = <3>;
>>          label = "lan2";
>>          phy-handle = <&switch1phy3>;
>>        };
>>
>>        port@4 {
>>          reg = <4>;
>>          label = "lan3";
>>          phy-handle = <&switch1phy4>;
>>        };
>>
>>        port@5 {
>>          reg = <5>;
>>          label = "wifi";
>>          fixed-link {
>>            speed = <100>;
>>            full-duplex;
>>          };
>>        };
>>
>>        port@6 {
>>          reg = <6>;
>>          label = "cpu";
>>          ethernet = <&gmac1>;
>>          fixed-link {
>>            speed = <1000>;
>>            full-duplex;
>>          };
>>        };
>>
>>      };
>>
>>      mdio {
>>        #address-cells = <1>;
>>        #size-cells = <0>;
>>        switch1phy2: switch1phy2@2 {
>>          reg = <2>;
>>          marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
>> Detect, no FLPs sents
>>        };
>>        switch1phy3: switch1phy3@3 {
>>          reg = <3>;
>>          marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
>> Detect, no FLPs sents
>>        };
>>        switch1phy4: switch1phy4@4 {
>>          reg = <4>;
>>          marvell,reg-init = <0 0x10 0 0x0200>; // Sense only on Rx Energy
>> Detect, no FLPs sents
>>        };
>>      };
>>
>>      };
>> };
> That all looks normal, expect the marvell,reg-init. That is a pretty
> ugly hack, from years and years ago, which should not be used any
> more. It would be better to add a DT property for what you want, or a
> PHY tunable.
>
Yep this dts chunk is what was done back from a 4.7 kernel level
We've been running a 5.1.7 kernel for a while now while we were 
concentrating on getting the rest of the system running and the software 
stack done.
I'm just starting pulling the kernel up to near the latest now so I'll 
have to look at how to get those flags set under whats available in the 
devicetree now.
>> This is how they appear using 'ip l'
>> The @ symbol got me as I've usually associated this with vlan's in my day to
>> day networking.
> The @ is just trying to show there is a relationship between to
> interfaces. Its a VLAN on top of a base interface, or its a DSA user
> port on top of a conduit interface.
>
> So there is nothing odd at all here. What i have seen is user space
> hacks to run Marvell SDK to program the switch to map a VLAN to a
> port. There is no point doing that when you have a perfectly good
> kernel driver.
>
>       Andrew
Yep this is the first time I've looked at a dsa style link.
So from my initial glimpse without digging it looked like a VLAN which I 
think I'd seen done with some other switch IC's in the past.
Definitely not using any marvel userspace trickery here.

Any idea on what should be done about that 'vlan_feature' permeating 
through and breaking stuff in this instance?
I'm really not sure how this 'feature' set is meant to propagate or be 
applied at each stage in a chained set of devices.
When the feature is on there does appear to be a value in the 
transmitted CRC but it's bad.
Not sure if thats just uninitialized data or the CRC is being applied at 
multiple stages and breaking stuff.
Pretty sure the stmmac ip needs zeros in the packets CRC field for it to 
be applied correctly.
maybe something similar is occurring in the marvel device.  But I'm not 
sure if thats where the problem is.

I can definitely dig further, but it'd be nice to be given a starting point?
Theres a LOT of stuff in the network stack..

I can just disable this patch in our systems, but seems like a missed 
opportunity to track down the problem.

Cheers
   Richard


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-12-13 10:16 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-08  6:03 STMMAC Ethernet Driver support Richard Tresidder
2023-12-08 18:12 ` Jakub Kicinski
2023-12-09  7:06   ` Richard Tresidder
2023-12-11  9:13     ` Richard Tresidder
2023-12-11  9:23       ` Richard Tresidder
2023-12-11 10:03         ` Corinna Vinschen
2023-12-12  4:13           ` Richard Tresidder
2023-12-12  6:49             ` Richard Tresidder
2023-12-11 16:16         ` Andrew Lunn
2023-12-12  3:57           ` Richard Tresidder
2023-12-12  9:27             ` Serge Semin
2023-12-13  4:55               ` Richard Tresidder
2023-12-13  9:17             ` Andrew Lunn
2023-12-13 10:15               ` Richard Tresidder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).