| Message ID | 20240416121032.52108-3-eichest@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mxl-gpy: add option to match SGMII speed to the TPI speed | expand |
On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > Add a new device tree property to disable SGMII autonegotiation and > instead use the option to match the SGMII speed to what was negotiated > on the twisted pair interface (tpi). Could you explain this is more detail. SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate the symbols 100 times when running at 10Mbs, and 10 times when running at 100Mbps. Andrew
Hi Andrew, Thanks a lot for the feedback. On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > Add a new device tree property to disable SGMII autonegotiation and > > instead use the option to match the SGMII speed to what was negotiated > > on the twisted pair interface (tpi). > > Could you explain this is more detail. > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > the symbols 100 times when running at 10Mbs, and 10 times when running > at 100Mbps. Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an Octeon TX2 SoC, this means that we have to enable "in-band-status" on the controller. This will work for all three speed settings. However, if we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will disable SGMII autonegotiation in gpy_update_interface. This is not supported by this Ethernet controller because in-band-status is still enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, the SGMII link will not go into a working state. What this patch does is, if the maxlinear,sgmii-match-tpi-speed property is set, it will always use the link speed negotiated on the twisted pair interface and adjust the SGMII data rate accordingly. For the Octeon controller, this means that we don't set the in-band-status mode because we don't use the SGMII autnegotiation and all 4 speeds (10, 100, 1000, 2500 Mbps) are working. Here the description from the datasheet (this patch forces the SGMII speed to the TPI speed): If bit 12 is set to a logic one, ANMODE field determines the Auto- Negotiation protocol. If bit 12 is cleared to a logic zero, speed is set to maximum in full duplex mode. Once the TPI link is up, the SGMII speed is automatically forced to match the TPI speed. This bit has no effect when SGMII_FIXED2G5 is ‘1’. Best regards, Stefan
On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > Hi Andrew, > > Thanks a lot for the feedback. > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > Add a new device tree property to disable SGMII autonegotiation and > > > instead use the option to match the SGMII speed to what was negotiated > > > on the twisted pair interface (tpi). > > > > Could you explain this is more detail. > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > the symbols 100 times when running at 10Mbs, and 10 times when running > > at 100Mbps. > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > the controller. This will work for all three speed settings. However, if > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > disable SGMII autonegotiation in gpy_update_interface. This is not > supported by this Ethernet controller because in-band-status is still > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > the SGMII link will not go into a working state. I have been working on a phylink/phylib patch set to address this. As I've been busy with health-based appointments during last week and this week, I haven't been able to spend enough time to get that to a point that I'm happy to publish it yet.
On Tue, Apr 16, 2024 at 04:48:34PM +0100, Russell King (Oracle) wrote: > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > > Hi Andrew, > > > > Thanks a lot for the feedback. > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > > Add a new device tree property to disable SGMII autonegotiation and > > > > instead use the option to match the SGMII speed to what was negotiated > > > > on the twisted pair interface (tpi). > > > > > > Could you explain this is more detail. > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > > the symbols 100 times when running at 10Mbs, and 10 times when running > > > at 100Mbps. > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > > the controller. This will work for all three speed settings. However, if > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > > disable SGMII autonegotiation in gpy_update_interface. This is not > > supported by this Ethernet controller because in-band-status is still > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > > the SGMII link will not go into a working state. > > I have been working on a phylink/phylib patch set to address this. As > I've been busy with health-based appointments during last week and this > week, I haven't been able to spend enough time to get that to a point > that I'm happy to publish it yet. You can find the experimental patches at: http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=0c2fb62db211312ad2f5695997694908b54e9a17 and the three parents to that patch. It's buried in: http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > Hi Andrew, > > Thanks a lot for the feedback. > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > Add a new device tree property to disable SGMII autonegotiation and > > > instead use the option to match the SGMII speed to what was negotiated > > > on the twisted pair interface (tpi). > > > > Could you explain this is more detail. > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > the symbols 100 times when running at 10Mbs, and 10 times when running > > at 100Mbps. > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > the controller. This will work for all three speed settings. However, if > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > disable SGMII autonegotiation in gpy_update_interface. This is not > supported by this Ethernet controller because in-band-status is still > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > the SGMII link will not go into a working state. This is where i expect Russel to point out that SGMII does not support 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And 2500BaseX does not perform speed negotiation, since it only supports 2500. So you also need the MAC to swap to 2500BaseX. I don't think any DT property is required here. This is fundamental to SGMII only be 10/100/1G, and when you go above that, you swap to something else. Lets see what Russell patches do. Andrew --- pw-bot: cr
On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote: > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > > Hi Andrew, > > > > Thanks a lot for the feedback. > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > > Add a new device tree property to disable SGMII autonegotiation and > > > > instead use the option to match the SGMII speed to what was negotiated > > > > on the twisted pair interface (tpi). > > > > > > Could you explain this is more detail. > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > > the symbols 100 times when running at 10Mbs, and 10 times when running > > > at 100Mbps. > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > > the controller. This will work for all three speed settings. However, if > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > > disable SGMII autonegotiation in gpy_update_interface. This is not > > supported by this Ethernet controller because in-band-status is still > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > > the SGMII link will not go into a working state. > > This is where i expect Russel to point out that SGMII does not support > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And > 2500BaseX does not perform speed negotiation, since it only supports > 2500. So you also need the MAC to swap to 2500BaseX. Yes, absolutely true that SGMII does not support 2.5G... and when operating faster, than 2500base-X is normally used. How, 2500base-X was slow to be standardised, and consequently different manufacturers came up with different ideas. The common theme is that it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether in-band negotiation is supported or not. This has been a pain point for a while now. As I mentioned in my previous two messages, I have an experimental patch series that helps to address this. The issue is that implementations mix manufacturers, so we need to know the capabilities of the PHY and the capabilities of the PCS, and then hope that we can find some common ground between their requirements. There is then the issue that if you're not using phylink, then... guess what... you either need to convert to use phylink or implement the logic in your own MAC driver to detect what the PHY is doing and what its capabilities are - but I think from what you've said, you are using phylink.
Hi Russell and Andrew, On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote: > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote: > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > > > Hi Andrew, > > > > > > Thanks a lot for the feedback. > > > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > > > Add a new device tree property to disable SGMII autonegotiation and > > > > > instead use the option to match the SGMII speed to what was negotiated > > > > > on the twisted pair interface (tpi). > > > > > > > > Could you explain this is more detail. > > > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > > > the symbols 100 times when running at 10Mbs, and 10 times when running > > > > at 100Mbps. > > > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > > > the controller. This will work for all three speed settings. However, if > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > > > disable SGMII autonegotiation in gpy_update_interface. This is not > > > supported by this Ethernet controller because in-band-status is still > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > > > the SGMII link will not go into a working state. > > > > This is where i expect Russel to point out that SGMII does not support > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And > > 2500BaseX does not perform speed negotiation, since it only supports > > 2500. So you also need the MAC to swap to 2500BaseX. > > Yes, absolutely true that SGMII does not support 2.5G... and when > operating faster, than 2500base-X is normally used. > > How, 2500base-X was slow to be standardised, and consequently different > manufacturers came up with different ideas. The common theme is that > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether > in-band negotiation is supported or not. This has been a pain point for > a while now. > > As I mentioned in my previous two messages, I have an experimental > patch series that helps to address this. > > The issue is that implementations mix manufacturers, so we need to > know the capabilities of the PHY and the capabilities of the PCS, and > then hope that we can find some common ground between their > requirements. > > There is then the issue that if you're not using phylink, then... > guess what... you either need to convert to use phylink or implement > the logic in your own MAC driver to detect what the PHY is doing > and what its capabilities are - but I think from what you've said, > you are using phylink. Thanks for the patch series and the explanation. In our use case we have the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode. If I understand the patches correctly, the PHY should just return in its query_inband function that it does not support inband when 2500BaseX is configured as it is done for the Marvell phy driver. I will try to test this on my end and give feedback, unfortunately it will become next week as I won't have access to my test setup until then. @Russell: I hope it is nothing serious with your health and wish you a fast recovery! Best regards, Stefan
On Tue, Apr 16, 2024 at 07:23:03PM +0200, Stefan Eichenberger wrote: > Hi Russell and Andrew, > > On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote: > > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote: > > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > > > > Hi Andrew, > > > > > > > > Thanks a lot for the feedback. > > > > > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > > > > Add a new device tree property to disable SGMII autonegotiation and > > > > > > instead use the option to match the SGMII speed to what was negotiated > > > > > > on the twisted pair interface (tpi). > > > > > > > > > > Could you explain this is more detail. > > > > > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > > > > the symbols 100 times when running at 10Mbs, and 10 times when running > > > > > at 100Mbps. > > > > > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > > > > the controller. This will work for all three speed settings. However, if > > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > > > > disable SGMII autonegotiation in gpy_update_interface. This is not > > > > supported by this Ethernet controller because in-band-status is still > > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > > > > the SGMII link will not go into a working state. > > > > > > This is where i expect Russel to point out that SGMII does not support > > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And > > > 2500BaseX does not perform speed negotiation, since it only supports > > > 2500. So you also need the MAC to swap to 2500BaseX. > > > > Yes, absolutely true that SGMII does not support 2.5G... and when > > operating faster, than 2500base-X is normally used. > > > > How, 2500base-X was slow to be standardised, and consequently different > > manufacturers came up with different ideas. The common theme is that > > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether > > in-band negotiation is supported or not. This has been a pain point for > > a while now. > > > > As I mentioned in my previous two messages, I have an experimental > > patch series that helps to address this. > > > > The issue is that implementations mix manufacturers, so we need to > > know the capabilities of the PHY and the capabilities of the PCS, and > > then hope that we can find some common ground between their > > requirements. > > > > There is then the issue that if you're not using phylink, then... > > guess what... you either need to convert to use phylink or implement > > the logic in your own MAC driver to detect what the PHY is doing > > and what its capabilities are - but I think from what you've said, > > you are using phylink. > > Thanks for the patch series and the explanation. In our use case we have > the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode. Ah, mvpp2. This is one of those cases where I think you have a disagreement between manufacturers over 2500base-X. Marvell's documentation clearly states that when operating in 1000base-X mode, AN _must_ be enabled. Since programming 2500base-X is programming the mvpp2 hardware for 1000base-X and then configuring the COMPHY to clock faster, AN must also be enabled when operating at 2500base-X. It seems you've coupled it with the mxl-gpy PHY which doesn't apparently support AN when in 2500base-X. Welcome to the mess of 2500base-X, and sadly we finally have the situation that I've feared for years: one end of a 2500base-X link that's documented as requiring AN, and the other end not providing it. Sigh. If only the IEEE 802.3 committee had been more pro-active and standardised 2500base-X _before_ manufacturers went off and did their own different things.
On Tue, Apr 16, 2024 at 07:12:49PM +0100, Russell King (Oracle) wrote: > On Tue, Apr 16, 2024 at 07:23:03PM +0200, Stefan Eichenberger wrote: > > Hi Russell and Andrew, > > > > On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote: > > > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote: > > > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > > > > > Hi Andrew, > > > > > > > > > > Thanks a lot for the feedback. > > > > > > > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > > > > > Add a new device tree property to disable SGMII autonegotiation and > > > > > > > instead use the option to match the SGMII speed to what was negotiated > > > > > > > on the twisted pair interface (tpi). > > > > > > > > > > > > Could you explain this is more detail. > > > > > > > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > > > > > the symbols 100 times when running at 10Mbs, and 10 times when running > > > > > > at 100Mbps. > > > > > > > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > > > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > > > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > > > > > the controller. This will work for all three speed settings. However, if > > > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > > > > > disable SGMII autonegotiation in gpy_update_interface. This is not > > > > > supported by this Ethernet controller because in-band-status is still > > > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > > > > > the SGMII link will not go into a working state. > > > > > > > > This is where i expect Russel to point out that SGMII does not support > > > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And > > > > 2500BaseX does not perform speed negotiation, since it only supports > > > > 2500. So you also need the MAC to swap to 2500BaseX. > > > > > > Yes, absolutely true that SGMII does not support 2.5G... and when > > > operating faster, than 2500base-X is normally used. > > > > > > How, 2500base-X was slow to be standardised, and consequently different > > > manufacturers came up with different ideas. The common theme is that > > > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether > > > in-band negotiation is supported or not. This has been a pain point for > > > a while now. > > > > > > As I mentioned in my previous two messages, I have an experimental > > > patch series that helps to address this. > > > > > > The issue is that implementations mix manufacturers, so we need to > > > know the capabilities of the PHY and the capabilities of the PCS, and > > > then hope that we can find some common ground between their > > > requirements. > > > > > > There is then the issue that if you're not using phylink, then... > > > guess what... you either need to convert to use phylink or implement > > > the logic in your own MAC driver to detect what the PHY is doing > > > and what its capabilities are - but I think from what you've said, > > > you are using phylink. > > > > Thanks for the patch series and the explanation. In our use case we have > > the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode. > > Ah, mvpp2. This is one of those cases where I think you have a > disagreement between manufacturers over 2500base-X. > > Marvell's documentation clearly states that when operating in 1000base-X > mode, AN _must_ be enabled. Since programming 2500base-X is programming > the mvpp2 hardware for 1000base-X and then configuring the COMPHY to > clock faster, AN must also be enabled when operating at 2500base-X. > > It seems you've coupled it with the mxl-gpy PHY which doesn't apparently > support AN when in 2500base-X. > > Welcome to the mess of 2500base-X, and sadly we finally have the > situation that I've feared for years: one end of a 2500base-X link > that's documented as requiring AN, and the other end not providing it. > > Sigh. If only the IEEE 802.3 committee had been more pro-active and > standardised 2500base-X _before_ manufacturers went off and did their > own different things. I also checked the datasheet and you are right about the 1000base-X mode and in-band AN. What worked for us so far was to use SGMII mode even for 2.5Gbps and disable in-band AN (which is possible for SGMII). I think this works because as you wrote, the genphy just multiplies the clock by 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we might even be able to use in-band autonegoation for 10,100 and 1000Mbps and then just disable it for 2.5Gbps. I need to test it, but I have hope that this should work. Regards, Stefan
On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > On Tue, Apr 16, 2024 at 07:12:49PM +0100, Russell King (Oracle) wrote: > > On Tue, Apr 16, 2024 at 07:23:03PM +0200, Stefan Eichenberger wrote: > > > Hi Russell and Andrew, > > > > > > On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote: > > > > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote: > > > > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote: > > > > > > Hi Andrew, > > > > > > > > > > > > Thanks a lot for the feedback. > > > > > > > > > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote: > > > > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > > > > > > > > Add a new device tree property to disable SGMII autonegotiation and > > > > > > > > instead use the option to match the SGMII speed to what was negotiated > > > > > > > > on the twisted pair interface (tpi). > > > > > > > > > > > > > > Could you explain this is more detail. > > > > > > > > > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate > > > > > > > the symbols 100 times when running at 10Mbs, and 10 times when running > > > > > > > at 100Mbps. > > > > > > > > > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps, > > > > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an > > > > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on > > > > > > the controller. This will work for all three speed settings. However, if > > > > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will > > > > > > disable SGMII autonegotiation in gpy_update_interface. This is not > > > > > > supported by this Ethernet controller because in-band-status is still > > > > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps, > > > > > > the SGMII link will not go into a working state. > > > > > > > > > > This is where i expect Russel to point out that SGMII does not support > > > > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And > > > > > 2500BaseX does not perform speed negotiation, since it only supports > > > > > 2500. So you also need the MAC to swap to 2500BaseX. > > > > > > > > Yes, absolutely true that SGMII does not support 2.5G... and when > > > > operating faster, than 2500base-X is normally used. > > > > > > > > How, 2500base-X was slow to be standardised, and consequently different > > > > manufacturers came up with different ideas. The common theme is that > > > > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether > > > > in-band negotiation is supported or not. This has been a pain point for > > > > a while now. > > > > > > > > As I mentioned in my previous two messages, I have an experimental > > > > patch series that helps to address this. > > > > > > > > The issue is that implementations mix manufacturers, so we need to > > > > know the capabilities of the PHY and the capabilities of the PCS, and > > > > then hope that we can find some common ground between their > > > > requirements. > > > > > > > > There is then the issue that if you're not using phylink, then... > > > > guess what... you either need to convert to use phylink or implement > > > > the logic in your own MAC driver to detect what the PHY is doing > > > > and what its capabilities are - but I think from what you've said, > > > > you are using phylink. > > > > > > Thanks for the patch series and the explanation. In our use case we have > > > the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode. > > > > Ah, mvpp2. This is one of those cases where I think you have a > > disagreement between manufacturers over 2500base-X. > > > > Marvell's documentation clearly states that when operating in 1000base-X > > mode, AN _must_ be enabled. Since programming 2500base-X is programming > > the mvpp2 hardware for 1000base-X and then configuring the COMPHY to > > clock faster, AN must also be enabled when operating at 2500base-X. > > > > It seems you've coupled it with the mxl-gpy PHY which doesn't apparently > > support AN when in 2500base-X. > > > > Welcome to the mess of 2500base-X, and sadly we finally have the > > situation that I've feared for years: one end of a 2500base-X link > > that's documented as requiring AN, and the other end not providing it. > > > > Sigh. If only the IEEE 802.3 committee had been more pro-active and > > standardised 2500base-X _before_ manufacturers went off and did their > > own different things. > > I also checked the datasheet and you are right about the 1000base-X mode > and in-band AN. What worked for us so far was to use SGMII mode even for > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > this works because as you wrote, the genphy just multiplies the clock by > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > and then just disable it for 2.5Gbps. I need to test it, but I have hope > that this should work. There is another way we could address this. If the querying support had a means to identify that the endpoint supports bypass mode, we could then have phylink identify that, and arrange to program the mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would mean it wouldn't require the inband 16-bit word to be present. I haven't fully thought it through yet - for example, I haven't considered how we should indicate to the PCS that AN bypass mode should be enabled or disabled via the pcs_config() method.
On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote: > Add a new device tree property to disable SGMII autonegotiation and > instead use the option to match the SGMII speed to what was negotiated > on the twisted pair interface (tpi). This allows us to disable SGMII > autonegotiation on Ethernet controllers that are not compatible with > this mode. > > Signed-off-by: Stefan Eichenberger <eichest@gmail.com> > --- > drivers/net/phy/mxl-gpy.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c > index b2d36a3a96f1..4147b4c29eaf 100644 > --- a/drivers/net/phy/mxl-gpy.c > +++ b/drivers/net/phy/mxl-gpy.c > @@ -114,6 +114,7 @@ struct gpy_priv { > * is enabled. > */ > u64 lb_dis_to; > + bool sgmii_match_tpi_speed; > }; > > static const struct { > @@ -262,8 +263,17 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr) > > static int gpy_config_init(struct phy_device *phydev) > { > + struct gpy_priv *priv = phydev->priv; > int ret; > > + /* Disalbe SGMII Autoneg if we want to match SGMII to TPI speed */ nit: Disable
On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > I also checked the datasheet and you are right about the 1000base-X mode > > and in-band AN. What worked for us so far was to use SGMII mode even for > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > this works because as you wrote, the genphy just multiplies the clock by > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > that this should work. > > There is another way we could address this. If the querying support > had a means to identify that the endpoint supports bypass mode, we > could then have phylink identify that, and arrange to program the > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > mean it wouldn't require the inband 16-bit word to be present. > > I haven't fully thought it through yet - for example, I haven't > considered how we should indicate to the PCS that AN bypass mode > should be enabled or disabled via the pcs_config() method. Okay, I've been trying to put more effort into this, but it's been slow progress (sorry). My thoughts from a design point of view were that we could just switch to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and everything at the PCS layer should be able to cope, but this is not the case, especially with mvneta/mvpp2. The problem is that mvneta/mvpp2 (and probably more) expect that 1) MLO_AN_INBAND means that the PCS will be using inband, and that means the link up/down state won't be forced. This basically implies that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the PCS. 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and that means that the link needs to be forced (since there's no way for the hardware to know whether the link should be up or down.) It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be used for the PCS. So, attempting to put a resolution of the PHY and PCS abilities into phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* mode alone just doesn't work. Yet... we need to do that in there when considering whether inband can be enabled or not for non-PHY links. Basically, it needs a re-think how to solve this...
On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote: > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > > I also checked the datasheet and you are right about the 1000base-X mode > > > and in-band AN. What worked for us so far was to use SGMII mode even for > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > > this works because as you wrote, the genphy just multiplies the clock by > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > > that this should work. > > > > There is another way we could address this. If the querying support > > had a means to identify that the endpoint supports bypass mode, we > > could then have phylink identify that, and arrange to program the > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > > mean it wouldn't require the inband 16-bit word to be present. > > > > I haven't fully thought it through yet - for example, I haven't > > considered how we should indicate to the PCS that AN bypass mode > > should be enabled or disabled via the pcs_config() method. > > Okay, I've been trying to put more effort into this, but it's been slow > progress (sorry). > > My thoughts from a design point of view were that we could just switch > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and > everything at the PCS layer should be able to cope, but this is not the > case, especially with mvneta/mvpp2. > > The problem is that mvneta/mvpp2 (and probably more) expect that > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that > means the link up/down state won't be forced. This basically implies > that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the > PCS. > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and > that means that the link needs to be forced (since there's no way > for the hardware to know whether the link should be up or down.) > It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be > used for the PCS. > > So, attempting to put a resolution of the PHY and PCS abilities into > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* > mode alone just doesn't work. Yet... we need to do that in there when > considering whether inband can be enabled or not for non-PHY links. > > Basically, it needs a re-think how to solve this... Today I was playing around with my combination of mxl-gpy and mvpp2 and I got it working again with your patches applied. However, I hacked the phylink driver to only rely on what the phy and pcs support. I know this is not a proper solution, but it allowed me to verify the other changes. My idea was if the phy and pcs support inband then use it, otherwise use outband and ignore the rest. Here is how my minimal phylink_pcs_neg_mode test function looks like: static unsigned int phylink_pcs_neg_mode(struct phylink *pl, struct phylink_pcs *pcs, unsigned int mode, phy_interface_t interface, const unsigned long *advertising) { unsigned int phy_link_mode = 0; unsigned int pcs_link_mode; pcs_link_mode = phylink_pcs_query_inband(pcs, interface); if (pl->phydev) phy_link_mode = phy_query_inband(pl->phydev, interface); /* If the PCS or PHY can not provide inband, then use * outband. */ if (!(pcs_link_mode & LINK_INBAND_VALID) || !(phy_link_mode & LINK_INBAND_VALID)) return PHYLINK_PCS_NEG_OUTBAND; return PHYLINK_PCS_NEG_INBAND_ENABLED; }
On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote: > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote: > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > > > I also checked the datasheet and you are right about the 1000base-X mode > > > > and in-band AN. What worked for us so far was to use SGMII mode even for > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > > > this works because as you wrote, the genphy just multiplies the clock by > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > > > that this should work. > > > > > > There is another way we could address this. If the querying support > > > had a means to identify that the endpoint supports bypass mode, we > > > could then have phylink identify that, and arrange to program the > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > > > mean it wouldn't require the inband 16-bit word to be present. > > > > > > I haven't fully thought it through yet - for example, I haven't > > > considered how we should indicate to the PCS that AN bypass mode > > > should be enabled or disabled via the pcs_config() method. > > > > Okay, I've been trying to put more effort into this, but it's been slow > > progress (sorry). > > > > My thoughts from a design point of view were that we could just switch > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and > > everything at the PCS layer should be able to cope, but this is not the > > case, especially with mvneta/mvpp2. > > > > The problem is that mvneta/mvpp2 (and probably more) expect that > > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that > > means the link up/down state won't be forced. This basically implies > > that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the > > PCS. > > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and > > that means that the link needs to be forced (since there's no way > > for the hardware to know whether the link should be up or down.) > > It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be > > used for the PCS. > > > > So, attempting to put a resolution of the PHY and PCS abilities into > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* > > mode alone just doesn't work. Yet... we need to do that in there when > > considering whether inband can be enabled or not for non-PHY links. > > > > Basically, it needs a re-think how to solve this... > > Today I was playing around with my combination of mxl-gpy and mvpp2 and > I got it working again with your patches applied. However, I hacked the > phylink driver to only rely on what the phy and pcs support. I know this > is not a proper solution, but it allowed me to verify the other changes. > My idea was if the phy and pcs support inband then use it, otherwise use > outband and ignore the rest. > > Here is how my minimal phylink_pcs_neg_mode test function looks like: > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl, > struct phylink_pcs *pcs, > unsigned int mode, > phy_interface_t interface, > const unsigned long *advertising) > { > unsigned int phy_link_mode = 0; > unsigned int pcs_link_mode; > > pcs_link_mode = phylink_pcs_query_inband(pcs, interface); > if (pl->phydev) > phy_link_mode = phy_query_inband(pl->phydev, interface); > > /* If the PCS or PHY can not provide inband, then use > * outband. > */ > if (!(pcs_link_mode & LINK_INBAND_VALID) || > !(phy_link_mode & LINK_INBAND_VALID)) > return PHYLINK_PCS_NEG_OUTBAND; > > return PHYLINK_PCS_NEG_INBAND_ENABLED; > } Note that I've changed the flags that get reported to be disable (bit 0)/ enable (bit 1) rather than valid/possible/required because the former makes the resolution easier. The problem is that merely returning outband doesn't cause mvneta/mvpp2 to force the link up. So for example, here's a SFP module which doesn't support any inband for 2500base-X nor SGMII: mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp= 4,23,27] mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin g 4,23 mvneta f1034000.ethernet eno2: interface 4 (sgmii) rate match none supports 2-3 ,5-6,13 mvneta f1034000.ethernet eno2: interface 23 (2500base-x) rate match none suppor ts 6,13,47 mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq= POLL) mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000 08000,0000206c advertising 00,00000000,00008000,0000206c mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1 mvneta f1034000.ethernet eno2: major config 2500base-x mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a dv=00,00000000,00008000,0000206c pause=04 mvneta f1034000.ethernet eno2: phylink_sfp_module_start() mvneta f1034000.ethernet eno2: phylink_sfp_link_up() mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off mvneta f1034000.ethernet eno2: major config sgmii mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00 mvneta f1034000.ethernet eno2: pcs link down mvneta f1034000.ethernet eno2: pcs link down mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off This looks like the link is up, but it isn't - note "pcs link down". If we look at the value of the GMAC AN status register: Value at address 0xf1036c10: 0x0000600a which indicates that the link is down, so no packets will pass.
On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote: > On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote: > > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote: > > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > > > > I also checked the datasheet and you are right about the 1000base-X mode > > > > > and in-band AN. What worked for us so far was to use SGMII mode even for > > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > > > > this works because as you wrote, the genphy just multiplies the clock by > > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > > > > that this should work. > > > > > > > > There is another way we could address this. If the querying support > > > > had a means to identify that the endpoint supports bypass mode, we > > > > could then have phylink identify that, and arrange to program the > > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > > > > mean it wouldn't require the inband 16-bit word to be present. > > > > > > > > I haven't fully thought it through yet - for example, I haven't > > > > considered how we should indicate to the PCS that AN bypass mode > > > > should be enabled or disabled via the pcs_config() method. > > > > > > Okay, I've been trying to put more effort into this, but it's been slow > > > progress (sorry). > > > > > > My thoughts from a design point of view were that we could just switch > > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and > > > everything at the PCS layer should be able to cope, but this is not the > > > case, especially with mvneta/mvpp2. > > > > > > The problem is that mvneta/mvpp2 (and probably more) expect that > > > > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that > > > means the link up/down state won't be forced. This basically implies > > > that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the > > > PCS. > > > > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and > > > that means that the link needs to be forced (since there's no way > > > for the hardware to know whether the link should be up or down.) > > > It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be > > > used for the PCS. > > > > > > So, attempting to put a resolution of the PHY and PCS abilities into > > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* > > > mode alone just doesn't work. Yet... we need to do that in there when > > > considering whether inband can be enabled or not for non-PHY links. > > > > > > Basically, it needs a re-think how to solve this... > > > > Today I was playing around with my combination of mxl-gpy and mvpp2 and > > I got it working again with your patches applied. However, I hacked the > > phylink driver to only rely on what the phy and pcs support. I know this > > is not a proper solution, but it allowed me to verify the other changes. > > My idea was if the phy and pcs support inband then use it, otherwise use > > outband and ignore the rest. > > > > Here is how my minimal phylink_pcs_neg_mode test function looks like: > > > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl, > > struct phylink_pcs *pcs, > > unsigned int mode, > > phy_interface_t interface, > > const unsigned long *advertising) > > { > > unsigned int phy_link_mode = 0; > > unsigned int pcs_link_mode; > > > > pcs_link_mode = phylink_pcs_query_inband(pcs, interface); > > if (pl->phydev) > > phy_link_mode = phy_query_inband(pl->phydev, interface); > > > > /* If the PCS or PHY can not provide inband, then use > > * outband. > > */ > > if (!(pcs_link_mode & LINK_INBAND_VALID) || > > !(phy_link_mode & LINK_INBAND_VALID)) > > return PHYLINK_PCS_NEG_OUTBAND; > > > > return PHYLINK_PCS_NEG_INBAND_ENABLED; > > } > > Note that I've changed the flags that get reported to be disable (bit 0)/ > enable (bit 1) rather than valid/possible/required because the former > makes the resolution easier. > > The problem is that merely returning outband doesn't cause mvneta/mvpp2 > to force the link up. So for example, here's a SFP module which doesn't > support any inband for 2500base-X nor SGMII: > > mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp= > 4,23,27] > mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface > mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin > g 4,23 > mvneta f1034000.ethernet eno2: interface 4 (sgmii) rate match none supports 2-3 > ,5-6,13 > mvneta f1034000.ethernet eno2: interface 23 (2500base-x) rate match none suppor > ts 6,13,47 > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq= > POLL) > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000 > 08000,0000206c advertising 00,00000000,00008000,0000206c > mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1 > mvneta f1034000.ethernet eno2: major config 2500base-x > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a > dv=00,00000000,00008000,0000206c pause=04 > mvneta f1034000.ethernet eno2: phylink_sfp_module_start() > mvneta f1034000.ethernet eno2: phylink_sfp_link_up() > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off > mvneta f1034000.ethernet eno2: major config sgmii > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00 > mvneta f1034000.ethernet eno2: pcs link down > mvneta f1034000.ethernet eno2: pcs link down > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off > > This looks like the link is up, but it isn't - note "pcs link down". > If we look at the value of the GMAC AN status register: > > Value at address 0xf1036c10: 0x0000600a > > which indicates that the link is down, so no packets will pass. What I changed in mvpp2 is to allow turing off inband in 2500base-x. The mvpp2 driver can handle this use case in pcs_config, it will turn off AN and force the link up. I think it should also work for mvneta, at least the code looks almost the same. I get the following for the Port Auto-Negotiation Configuration Register: For 1Gbit/s it switches to SGMII and enables inband AN: Memory mapped at address 0xffffa0112000. Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6 For 2.5Gbit/s it disables inband AN and forces the link to be up: Memory mapped at address 0xffffaff88000. Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042 Then the status register shows also link up for 2.5Gbit/s: Memory mapped at address 0xffffa5fe2000. Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B What might be confusing is that the port type, bit 1 in MAC Control Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off autonegotiation for SGMII: Memory mapped at address 0xffff8c26c000. Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD My example patch still uses the old macros: diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 97e38f61ac65..15fadfd61313 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs, * When <PortType> = 1 (1000BASE-X) this field must be set to 1. * Therefore, inband is "required". */ - if (phy_interface_mode_is_8023z(interface)) + if (interface == PHY_INTERFACE_MODE_1000BASEX) return LINK_INBAND_VALID | LINK_INBAND_REQUIRED; + if (interface == PHY_INTERFACE_MODE_2500BASEX) + return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE; + /* SGMII and RGMII can be configured to use inband signalling of the * AN result. Indicate these as "possible". */
On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote: > On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote: > > On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote: > > > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote: > > > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > > > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > > > > > I also checked the datasheet and you are right about the 1000base-X mode > > > > > > and in-band AN. What worked for us so far was to use SGMII mode even for > > > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > > > > > this works because as you wrote, the genphy just multiplies the clock by > > > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > > > > > that this should work. > > > > > > > > > > There is another way we could address this. If the querying support > > > > > had a means to identify that the endpoint supports bypass mode, we > > > > > could then have phylink identify that, and arrange to program the > > > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > > > > > mean it wouldn't require the inband 16-bit word to be present. > > > > > > > > > > I haven't fully thought it through yet - for example, I haven't > > > > > considered how we should indicate to the PCS that AN bypass mode > > > > > should be enabled or disabled via the pcs_config() method. > > > > > > > > Okay, I've been trying to put more effort into this, but it's been slow > > > > progress (sorry). > > > > > > > > My thoughts from a design point of view were that we could just switch > > > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and > > > > everything at the PCS layer should be able to cope, but this is not the > > > > case, especially with mvneta/mvpp2. > > > > > > > > The problem is that mvneta/mvpp2 (and probably more) expect that > > > > > > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that > > > > means the link up/down state won't be forced. This basically implies > > > > that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the > > > > PCS. > > > > > > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and > > > > that means that the link needs to be forced (since there's no way > > > > for the hardware to know whether the link should be up or down.) > > > > It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be > > > > used for the PCS. > > > > > > > > So, attempting to put a resolution of the PHY and PCS abilities into > > > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* > > > > mode alone just doesn't work. Yet... we need to do that in there when > > > > considering whether inband can be enabled or not for non-PHY links. > > > > > > > > Basically, it needs a re-think how to solve this... > > > > > > Today I was playing around with my combination of mxl-gpy and mvpp2 and > > > I got it working again with your patches applied. However, I hacked the > > > phylink driver to only rely on what the phy and pcs support. I know this > > > is not a proper solution, but it allowed me to verify the other changes. > > > My idea was if the phy and pcs support inband then use it, otherwise use > > > outband and ignore the rest. > > > > > > Here is how my minimal phylink_pcs_neg_mode test function looks like: > > > > > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl, > > > struct phylink_pcs *pcs, > > > unsigned int mode, > > > phy_interface_t interface, > > > const unsigned long *advertising) > > > { > > > unsigned int phy_link_mode = 0; > > > unsigned int pcs_link_mode; > > > > > > pcs_link_mode = phylink_pcs_query_inband(pcs, interface); > > > if (pl->phydev) > > > phy_link_mode = phy_query_inband(pl->phydev, interface); > > > > > > /* If the PCS or PHY can not provide inband, then use > > > * outband. > > > */ > > > if (!(pcs_link_mode & LINK_INBAND_VALID) || > > > !(phy_link_mode & LINK_INBAND_VALID)) > > > return PHYLINK_PCS_NEG_OUTBAND; > > > > > > return PHYLINK_PCS_NEG_INBAND_ENABLED; > > > } > > > > Note that I've changed the flags that get reported to be disable (bit 0)/ > > enable (bit 1) rather than valid/possible/required because the former > > makes the resolution easier. > > > > The problem is that merely returning outband doesn't cause mvneta/mvpp2 > > to force the link up. So for example, here's a SFP module which doesn't > > support any inband for 2500base-X nor SGMII: > > > > mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp= > > 4,23,27] > > mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface > > mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin > > g 4,23 > > mvneta f1034000.ethernet eno2: interface 4 (sgmii) rate match none supports 2-3 > > ,5-6,13 > > mvneta f1034000.ethernet eno2: interface 23 (2500base-x) rate match none suppor > > ts 6,13,47 > > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq= > > POLL) > > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000 > > 08000,0000206c advertising 00,00000000,00008000,0000206c > > mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1 > > mvneta f1034000.ethernet eno2: major config 2500base-x > > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 > > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a > > dv=00,00000000,00008000,0000206c pause=04 > > mvneta f1034000.ethernet eno2: phylink_sfp_module_start() > > mvneta f1034000.ethernet eno2: phylink_sfp_link_up() > > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off > > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off > > mvneta f1034000.ethernet eno2: major config sgmii > > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 > > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00 > > mvneta f1034000.ethernet eno2: pcs link down > > mvneta f1034000.ethernet eno2: pcs link down > > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active > > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us > > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off > > > > This looks like the link is up, but it isn't - note "pcs link down". > > If we look at the value of the GMAC AN status register: > > > > Value at address 0xf1036c10: 0x0000600a > > > > which indicates that the link is down, so no packets will pass. > > What I changed in mvpp2 is to allow turing off inband in 2500base-x. The > mvpp2 driver can handle this use case in pcs_config, it will turn off AN > and force the link up. pcs_config can't force the link up. > I think it should also work for mvneta, at least > the code looks almost the same. I get the following for the Port > Auto-Negotiation Configuration Register: > > For 1Gbit/s it switches to SGMII and enables inband AN: > Memory mapped at address 0xffffa0112000. > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6 So here the link is forced up which is wrong for inband, because then we have no way to detect the link going down. > For 2.5Gbit/s it disables inband AN and forces the link to be up: > Memory mapped at address 0xffffaff88000. > Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042 > > Then the status register shows also link up for 2.5Gbit/s: > Memory mapped at address 0xffffa5fe2000. > Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B > > What might be confusing is that the port type, bit 1 in MAC Control > Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off > autonegotiation for SGMII: > Memory mapped at address 0xffff8c26c000. > Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD Control register 0 bit 1 is the port type bit, which controls whether the port is in "1000base-X" mode or SGMII mode. This has no effect on the interpretation of the inband control word. Control register 2 bit 0 controls whether the port uses 802.3z format control words or SGMII format control words. > My example patch still uses the old macros: > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index 97e38f61ac65..15fadfd61313 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs, > * When <PortType> = 1 (1000BASE-X) this field must be set to 1. > * Therefore, inband is "required". > */ > - if (phy_interface_mode_is_8023z(interface)) > + if (interface == PHY_INTERFACE_MODE_1000BASEX) > return LINK_INBAND_VALID | LINK_INBAND_REQUIRED; > > + if (interface == PHY_INTERFACE_MODE_2500BASEX) > + return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE; This is not correct though. If we set PortType = 1, then this applies: Bit Field Type / InitVal Description 2 InBandAnEn RW In-band Auto-Negotiation enable. ... When <PortType> = 1 (1000BASE-X) this field must be set to 1. When <PortType> = 0 (SGMII) and this field is 1, in-band Flow Control not supported. So, if we have the port in 1000base-X mode (PortType = 1) then bit 2 of the Autoneg configuration register needs to be set to be compliant with the documentation. Therefore, since we set PortType = 1 for 2500BASE-X (and note that I _do_ run 2500BASE-X with inband AN enabled over fibre transceivers here, so this is needed), we also need to enable InBandAnEn. This is exactly where the difficulty comes - because what you're suggesting will break currently working setups such as what I have here. Switching the port to SGMII mode for 2500base-X doesn't sound like a sensible thing to do.
On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote: > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote: > > On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote: > > > On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote: > > > > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote: > > > > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > > > > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > > > > > > I also checked the datasheet and you are right about the 1000base-X mode > > > > > > > and in-band AN. What worked for us so far was to use SGMII mode even for > > > > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > > > > > > this works because as you wrote, the genphy just multiplies the clock by > > > > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > > > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > > > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > > > > > > that this should work. > > > > > > > > > > > > There is another way we could address this. If the querying support > > > > > > had a means to identify that the endpoint supports bypass mode, we > > > > > > could then have phylink identify that, and arrange to program the > > > > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > > > > > > mean it wouldn't require the inband 16-bit word to be present. > > > > > > > > > > > > I haven't fully thought it through yet - for example, I haven't > > > > > > considered how we should indicate to the PCS that AN bypass mode > > > > > > should be enabled or disabled via the pcs_config() method. > > > > > > > > > > Okay, I've been trying to put more effort into this, but it's been slow > > > > > progress (sorry). > > > > > > > > > > My thoughts from a design point of view were that we could just switch > > > > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and > > > > > everything at the PCS layer should be able to cope, but this is not the > > > > > case, especially with mvneta/mvpp2. > > > > > > > > > > The problem is that mvneta/mvpp2 (and probably more) expect that > > > > > > > > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that > > > > > means the link up/down state won't be forced. This basically implies > > > > > that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the > > > > > PCS. > > > > > > > > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and > > > > > that means that the link needs to be forced (since there's no way > > > > > for the hardware to know whether the link should be up or down.) > > > > > It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be > > > > > used for the PCS. > > > > > > > > > > So, attempting to put a resolution of the PHY and PCS abilities into > > > > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* > > > > > mode alone just doesn't work. Yet... we need to do that in there when > > > > > considering whether inband can be enabled or not for non-PHY links. > > > > > > > > > > Basically, it needs a re-think how to solve this... > > > > > > > > Today I was playing around with my combination of mxl-gpy and mvpp2 and > > > > I got it working again with your patches applied. However, I hacked the > > > > phylink driver to only rely on what the phy and pcs support. I know this > > > > is not a proper solution, but it allowed me to verify the other changes. > > > > My idea was if the phy and pcs support inband then use it, otherwise use > > > > outband and ignore the rest. > > > > > > > > Here is how my minimal phylink_pcs_neg_mode test function looks like: > > > > > > > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl, > > > > struct phylink_pcs *pcs, > > > > unsigned int mode, > > > > phy_interface_t interface, > > > > const unsigned long *advertising) > > > > { > > > > unsigned int phy_link_mode = 0; > > > > unsigned int pcs_link_mode; > > > > > > > > pcs_link_mode = phylink_pcs_query_inband(pcs, interface); > > > > if (pl->phydev) > > > > phy_link_mode = phy_query_inband(pl->phydev, interface); > > > > > > > > /* If the PCS or PHY can not provide inband, then use > > > > * outband. > > > > */ > > > > if (!(pcs_link_mode & LINK_INBAND_VALID) || > > > > !(phy_link_mode & LINK_INBAND_VALID)) > > > > return PHYLINK_PCS_NEG_OUTBAND; > > > > > > > > return PHYLINK_PCS_NEG_INBAND_ENABLED; > > > > } > > > > > > Note that I've changed the flags that get reported to be disable (bit 0)/ > > > enable (bit 1) rather than valid/possible/required because the former > > > makes the resolution easier. > > > > > > The problem is that merely returning outband doesn't cause mvneta/mvpp2 > > > to force the link up. So for example, here's a SFP module which doesn't > > > support any inband for 2500base-X nor SGMII: > > > > > > mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp= > > > 4,23,27] > > > mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface > > > mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin > > > g 4,23 > > > mvneta f1034000.ethernet eno2: interface 4 (sgmii) rate match none supports 2-3 > > > ,5-6,13 > > > mvneta f1034000.ethernet eno2: interface 23 (2500base-x) rate match none suppor > > > ts 6,13,47 > > > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq= > > > POLL) > > > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000 > > > 08000,0000206c advertising 00,00000000,00008000,0000206c > > > mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1 > > > mvneta f1034000.ethernet eno2: major config 2500base-x > > > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 > > > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a > > > dv=00,00000000,00008000,0000206c pause=04 > > > mvneta f1034000.ethernet eno2: phylink_sfp_module_start() > > > mvneta f1034000.ethernet eno2: phylink_sfp_link_up() > > > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off > > > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off > > > mvneta f1034000.ethernet eno2: major config sgmii > > > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 > > > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00 > > > mvneta f1034000.ethernet eno2: pcs link down > > > mvneta f1034000.ethernet eno2: pcs link down > > > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active > > > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us > > > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off > > > > > > This looks like the link is up, but it isn't - note "pcs link down". > > > If we look at the value of the GMAC AN status register: > > > > > > Value at address 0xf1036c10: 0x0000600a > > > > > > which indicates that the link is down, so no packets will pass. > > > > What I changed in mvpp2 is to allow turing off inband in 2500base-x. The > > mvpp2 driver can handle this use case in pcs_config, it will turn off AN > > and force the link up. > > pcs_config can't force the link up. > > > I think it should also work for mvneta, at least > > the code looks almost the same. I get the following for the Port > > Auto-Negotiation Configuration Register: > > > > For 1Gbit/s it switches to SGMII and enables inband AN: > > Memory mapped at address 0xffffa0112000. > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6 > > So here the link is forced up which is wrong for inband, because then > we have no way to detect the link going down. Yes I also saw this and didn't understand it. When I clear the force bit it will be set back to 1 again when AN is enabled. I thought this might be a bug of the controller. > > > For 2.5Gbit/s it disables inband AN and forces the link to be up: > > Memory mapped at address 0xffffaff88000. > > Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042 > > > > Then the status register shows also link up for 2.5Gbit/s: > > Memory mapped at address 0xffffa5fe2000. > > Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B > > > > What might be confusing is that the port type, bit 1 in MAC Control > > Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off > > autonegotiation for SGMII: > > Memory mapped at address 0xffff8c26c000. > > Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD > > Control register 0 bit 1 is the port type bit, which controls whether > the port is in "1000base-X" mode or SGMII mode. This has no effect on > the interpretation of the inband control word. > > Control register 2 bit 0 controls whether the port uses 802.3z > format control words or SGMII format control words. > > > My example patch still uses the old macros: > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > index 97e38f61ac65..15fadfd61313 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > @@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs, > > * When <PortType> = 1 (1000BASE-X) this field must be set to 1. > > * Therefore, inband is "required". > > */ > > - if (phy_interface_mode_is_8023z(interface)) > > + if (interface == PHY_INTERFACE_MODE_1000BASEX) > > return LINK_INBAND_VALID | LINK_INBAND_REQUIRED; > > > > + if (interface == PHY_INTERFACE_MODE_2500BASEX) > > + return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE; > > This is not correct though. If we set PortType = 1, then this applies: > > Bit Field Type / InitVal Description > > 2 InBandAnEn RW In-band Auto-Negotiation enable. > ... > When <PortType> = 1 (1000BASE-X) > this field must be set to 1. > When <PortType> = 0 (SGMII) and this > field is 1, in-band Flow Control not > supported. > > So, if we have the port in 1000base-X mode (PortType = 1) then bit 2 > of the Autoneg configuration register needs to be set to be compliant > with the documentation. Therefore, since we set PortType = 1 for > 2500BASE-X (and note that I _do_ run 2500BASE-X with inband AN enabled > over fibre transceivers here, so this is needed), we also need to > enable InBandAnEn. > > This is exactly where the difficulty comes - because what you're > suggesting will break currently working setups such as what I have > here. > > Switching the port to SGMII mode for 2500base-X doesn't sound like a > sensible thing to do. Maybe this is my misunderstanding. Is SGMII and 1000BASE-X not the same if there is no inband AN? I thought the only difference is how the inband AN works. Regards, Stefan
On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote: > On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote: > > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote: > > > I think it should also work for mvneta, at least > > > the code looks almost the same. I get the following for the Port > > > Auto-Negotiation Configuration Register: > > > > > > For 1Gbit/s it switches to SGMII and enables inband AN: > > > Memory mapped at address 0xffffa0112000. > > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6 > > > > So here the link is forced up which is wrong for inband, because then > > we have no way to detect the link going down. > > Yes I also saw this and didn't understand it. When I clear the force bit > it will be set back to 1 again when AN is enabled. I thought this might > be a bug of the controller. No it isn't, the hardware never changes the value in this register. The difference will be because of what I explained previously. Because the mvneta and mvpp2 hardware is "weird" in that these two registers provide both PCS and MAC functions, we have to deal with them in a way that is split between the phylink PCS and MAC operations. This code was written at a time when all we had was MLO_AN_* and none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_* constants which were the same as what was passed via the MAC operations. This made the implementation entirely consistent. However, that lead PCS implementers to go off and do their own things, so we ended up with a range of different PCS behaviours depending on the implementation (because the way people interpreted MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising mask was all different. So to bring some consistency, I changed the PCS interface to what we have now, in the belief that it would later allow us to solve the 2500base-X problem. However, I'd forgotten about the mvneta/mvpp2 details, but that was fine at the time of this change because everything was still consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband when using MLO_AN_INBAND. Now, when trying to solve the 2500base-X problem which needs us to use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is what is causing me problems (the link isn't forced up.) Conversely, I suspect you have the situation where you have MLO_AN_PHY or MLO_AN_FIXED being passed to the mac_config() and mac_link_up() operations, but the PCS is being configured for a different mode. I am wondering whether we should at the very least move the configuration of the control register 0 and 2 to the pcs_config() method so at least that's consistent with the PHYLINK_PCS_NEG_* value passed to the PCS and thus the values programmed into the autoneg config register. However, that still leaves a big hole in how we handle the link forcing - which is necessary if inband AN is disabled (in other words, if we need to read the link status from the PHY as is done in MLO_AN_PHY mode.)
On Thu, Apr 25, 2024 at 05:30:50PM +0100, Russell King (Oracle) wrote: > On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote: > > On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote: > > > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote: > > > > I think it should also work for mvneta, at least > > > > the code looks almost the same. I get the following for the Port > > > > Auto-Negotiation Configuration Register: > > > > > > > > For 1Gbit/s it switches to SGMII and enables inband AN: > > > > Memory mapped at address 0xffffa0112000. > > > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6 > > > > > > So here the link is forced up which is wrong for inband, because then > > > we have no way to detect the link going down. > > > > Yes I also saw this and didn't understand it. When I clear the force bit > > it will be set back to 1 again when AN is enabled. I thought this might > > be a bug of the controller. > > No it isn't, the hardware never changes the value in this register. > The difference will be because of what I explained previously. > > Because the mvneta and mvpp2 hardware is "weird" in that these two > registers provide both PCS and MAC functions, we have to deal with > them in a way that is split between the phylink PCS and MAC > operations. > > This code was written at a time when all we had was MLO_AN_* and > none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_* > constants which were the same as what was passed via the MAC > operations. This made the implementation entirely consistent. > > However, that lead PCS implementers to go off and do their own > things, so we ended up with a range of different PCS behaviours > depending on the implementation (because the way people interpreted > MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising > mask was all different. > > So to bring some consistency, I changed the PCS interface to what we > have now, in the belief that it would later allow us to solve the > 2500base-X problem. > > However, I'd forgotten about the mvneta/mvpp2 details, but that was > fine at the time of this change because everything was still > consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or > PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and > PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband > when using MLO_AN_INBAND. > > Now, when trying to solve the 2500base-X problem which needs us to > use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can > end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is > what is causing me problems (the link isn't forced up.) > > Conversely, I suspect you have the situation where you have MLO_AN_PHY > or MLO_AN_FIXED being passed to the mac_config() and mac_link_up() > operations, but the PCS is being configured for a different mode. > > I am wondering whether we should at the very least move the > configuration of the control register 0 and 2 to the pcs_config() > method so at least that's consistent with the PHYLINK_PCS_NEG_* > value passed to the PCS and thus the values programmed into the > autoneg config register. However, that still leaves a big hole in > how we handle the link forcing - which is necessary if inband AN > is disabled (in other words, if we need to read the link status > from the PHY as is done in MLO_AN_PHY mode.) > Now I got it, thanks a lot for the explanation. So the issue is that MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and therefore the link is not forced up because for that MLO_AN_PHY would be needed. I will also try to think about it. Thanks, Stefan
On Thu, Apr 25, 2024 at 07:33:59PM +0200, Stefan Eichenberger wrote: > Now I got it, thanks a lot for the explanation. So the issue is that > MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and > therefore the link is not forced up because for that MLO_AN_PHY would be > needed. I will also try to think about it. Now that I've moved the setting of PortType and InBandAutoNegMode into the pcs_config() method, I now have (on mvneta): Value at address 0xf1036c00: 0x00008bfd - PortType = 0 (SGMII, necessary to be able to set InBandAnEn=0 below) Value at address 0xf1036c08: 0x0000c018 - InBandAutoNegMode = 0 (1000Base-X mode) Value at address 0xf1036c0c: 0x00009240 - 1000M, FD, unforced link InBandAnEn = 0 Value at address 0xf1036c10: 0x0000600a - Sync, 1000M, FD, but no link The reason that the link isn't being forced is because mvneta_mac_link_up() is being called with mode = MLO_AN_INBAND which expects the link to be controlled as a result of autoneg, but we've configured autoneg to be off. I'm wondering whether we need pl->cur_link_an_mode to be the desired mode for selecting the result from phylink_pcs_neg_mode(), but also maintain a separate pl->act_link_an_mode which phylink_pcs_neg_mode() chooses, dependent on whether the PCS is using inband or outband mode - and pl->act_link_an_mode is what gets passed to the MAC layer. That would at least keep the MAC MLO_AN_* consistent with what the PCS layer is using - and also has the advantage that it makes it clear that pl->act_link_an_mode only gets updated in the "major config" path. A quick test of that... seems to work: mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=POLL) mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,00008000,0000206c advertising 00,00000000,00008000,0000206c mvneta f1034000.ethernet eno2: major config 2500base-x mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04 mvneta f1034000.ethernet eno2: phylink_sfp_module_start() mvneta f1034000.ethernet eno2: phylink_sfp_link_up() mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off mvneta f1034000.ethernet eno2: major config sgmii mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 mvneta f1034000.ethernet eno2: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00008000,0000206c pause=00 mvneta f1034000.ethernet eno2: pcs link down mvneta f1034000.ethernet eno2: pcs link down mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off Value at address 0xf1036c00: 0x00008bfd Value at address 0xf1036c08: 0x0000c018 Value at address 0xf1036c0c: 0x00009242 Value at address 0xf1036c10: 0x0000600b So we can see in the two phylink_mac_config calls that the mode has switched from "inband" to "phy" with this PHY (BCM84881) which doesn't support inband in any interface modes. However, there's still the issue with: link modes: pcs=02 phy=01 phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04 and this is because of the missing code in this part: /* PHY present, inband mode depends on the capabilities * of both. */ but there's also the issue that the PCS and PHY capabilities like that are incompatible. In this case, we're saved by the fact that if we do this act_link_an_mode thing: pl->act_link_an_mode = pl->cur_link_an_mode; if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_OUTBAND && pl->act_link_an_mode == MLO_AN_INBAND) pl->act_link_an_mode = MLO_AN_PHY; coupled with the new _behaviour_ of mvneta/mvpp2, we don't actually end up in the "1000base-X must have AN enabled" trap... but that is no basis to basing decisions at the phylink layer on. So, I'm wondering whether we need to be a little more creative here. Instead of simply passing a few bits, maybe something like: 31-24: bitfield of "partner" capabilities that are supported for inband enabled mode 23-16: bitfield of "partner" capabilities that are supported for inband disabled mode 15-8: bitfield of "partner" capabilities that are supported for outband mode 2: bypass mode supported 1: inband enabled mode supported 0: inband disabled mode supported Now, a question will come up... what is different between inband disabled and outband mode? Consider 1000base-X fibre. 1000base-X is the media interface, and we need to be able to configure autoneg there, enabling or disabling it. If we don't support disabling autoneg (as is the case with mvneta et.al. over fibre) then being able to use ethtool to disable autoneg can't be used. In both these modes, the 1000base-X is the media side. However, 1000base-X can be used to connect to a PHY, and the PHY could do rate matching, so the we need to use an outband way to access the media side (we need to talk to the PHY.) Hence why PCS have a distinction between OUTBAND and INBAND_DISABLED. Now, with 2500base-X we run into the problem that e.g. mvneta operating in 1000base-X mode upclocked to 2.5G can only support INBAND_ENABLED and not INBAND_DISABLED (we can't just turn off the InBandAnEn bit). The change between INBAND_ENABLED and INBAND_DISABLED can happen with the link up. However, it can support OUTBAND by disabling the PortType bit and then turning off InBandAnEn (which can only be done with the link *down* and that is only guaranteed during a "major config" not through the ethtool settings API - which is why pcs_config() can't do this for INBAND_DISABLED.) So, a little bit of progress but not at a usable solution yet. I'm afraid my current tree is in a hacky mess at the moment, I'll see about updating the published patches as soon as I can.
Hi Russell, Sorry for the late reply but I wanted to give you some update after testing with the latest version of your patches on net-queue. On Thu, Apr 25, 2024 at 09:15:54PM +0100, Russell King (Oracle) wrote: > On Thu, Apr 25, 2024 at 07:33:59PM +0200, Stefan Eichenberger wrote: > > Now I got it, thanks a lot for the explanation. So the issue is that > > MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and > > therefore the link is not forced up because for that MLO_AN_PHY would be > > needed. I will also try to think about it. > > Now that I've moved the setting of PortType and InBandAutoNegMode into > the pcs_config() method, I now have (on mvneta): > > Value at address 0xf1036c00: 0x00008bfd - PortType = 0 > (SGMII, necessary to be able > to set InBandAnEn=0 below) > > Value at address 0xf1036c08: 0x0000c018 - InBandAutoNegMode = 0 > (1000Base-X mode) > > Value at address 0xf1036c0c: 0x00009240 - 1000M, FD, unforced link > InBandAnEn = 0 > > Value at address 0xf1036c10: 0x0000600a - Sync, 1000M, FD, but no link > > The reason that the link isn't being forced is because > mvneta_mac_link_up() is being called with mode = MLO_AN_INBAND > which expects the link to be controlled as a result of autoneg, > but we've configured autoneg to be off. > > I'm wondering whether we need pl->cur_link_an_mode to be the desired > mode for selecting the result from phylink_pcs_neg_mode(), but also > maintain a separate pl->act_link_an_mode which phylink_pcs_neg_mode() > chooses, dependent on whether the PCS is using inband or outband > mode - and pl->act_link_an_mode is what gets passed to the MAC layer. > That would at least keep the MAC MLO_AN_* consistent with what the > PCS layer is using - and also has the advantage that it makes it > clear that pl->act_link_an_mode only gets updated in the "major > config" path. > > A quick test of that... seems to work: > > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=POLL) > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,00008000,0000206c advertising 00,00000000,00008000,0000206c > mvneta f1034000.ethernet eno2: major config 2500base-x > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04 > mvneta f1034000.ethernet eno2: phylink_sfp_module_start() > mvneta f1034000.ethernet eno2: phylink_sfp_link_up() > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off > mvneta f1034000.ethernet eno2: major config sgmii > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00008000,0000206c pause=00 > mvneta f1034000.ethernet eno2: pcs link down > mvneta f1034000.ethernet eno2: pcs link down > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off > > Value at address 0xf1036c00: 0x00008bfd > Value at address 0xf1036c08: 0x0000c018 > Value at address 0xf1036c0c: 0x00009242 > Value at address 0xf1036c10: 0x0000600b > > So we can see in the two phylink_mac_config calls that the mode has > switched from "inband" to "phy" with this PHY (BCM84881) which > doesn't support inband in any interface modes. > > However, there's still the issue with: > > link modes: pcs=02 phy=01 > phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04 > > and this is because of the missing code in this part: > > /* PHY present, inband mode depends on the capabilities > * of both. > */ > > but there's also the issue that the PCS and PHY capabilities like that > are incompatible. In this case, we're saved by the fact that if we do > this act_link_an_mode thing: > > pl->act_link_an_mode = pl->cur_link_an_mode; > if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_OUTBAND && > pl->act_link_an_mode == MLO_AN_INBAND) > pl->act_link_an_mode = MLO_AN_PHY; > > coupled with the new _behaviour_ of mvneta/mvpp2, we don't actually > end up in the "1000base-X must have AN enabled" trap... but that is > no basis to basing decisions at the phylink layer on. > > So, I'm wondering whether we need to be a little more creative here. > Instead of simply passing a few bits, maybe something like: > > 31-24: bitfield of "partner" capabilities that are supported > for inband enabled mode > 23-16: bitfield of "partner" capabilities that are supported > for inband disabled mode > 15-8: bitfield of "partner" capabilities that are supported > for outband mode > 2: bypass mode supported > 1: inband enabled mode supported > 0: inband disabled mode supported > > Now, a question will come up... what is different between inband > disabled and outband mode? > > Consider 1000base-X fibre. 1000base-X is the media interface, and we > need to be able to configure autoneg there, enabling or disabling it. > If we don't support disabling autoneg (as is the case with mvneta > et.al. over fibre) then being able to use ethtool to disable autoneg > can't be used. In both these modes, the 1000base-X is the media side. > > However, 1000base-X can be used to connect to a PHY, and the PHY > could do rate matching, so the we need to use an outband way to > access the media side (we need to talk to the PHY.) > > Hence why PCS have a distinction between OUTBAND and INBAND_DISABLED. > > Now, with 2500base-X we run into the problem that e.g. mvneta > operating in 1000base-X mode upclocked to 2.5G can only support > INBAND_ENABLED and not INBAND_DISABLED (we can't just turn off the > InBandAnEn bit). The change between INBAND_ENABLED and INBAND_DISABLED > can happen with the link up. > > However, it can support OUTBAND by disabling the PortType bit and then > turning off InBandAnEn (which can only be done with the link *down* > and that is only guaranteed during a "major config" not through the > ethtool settings API - which is why pcs_config() can't do this for > INBAND_DISABLED.) > > So, a little bit of progress but not at a usable solution yet. > > I'm afraid my current tree is in a hacky mess at the moment, I'll see > about updating the published patches as soon as I can. I think I see the problem you are describing. When the driver starts it will negotiate MLO_AN_PHY based on the capabilities of the PHY and of the PCS. However when I switch to 1GBit/s it should switch to MLO_AN_INBAND but this does not work. Here the output of phylink: [ 14.695170] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL) [ 14.706541] mvpp2 f2000000.ethernet eth1: phy: sgmii setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f [ 14.707770] mvpp2 f2000000.ethernet eth1: configuring for phy/sgmii link mode [ 14.716437] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii [ 14.723260] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband [ 14.731623] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii [ 14.731630] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=00 [ 14.733905] mvpp2 f2000000.ethernet eth1: phy link down sgmii/2.5Gbps/Full/none/rx/tx [ 18.825056] mvpp2 f2000000.ethernet eth1: phy link up 2500base-x/2.5Gbps/Full/none/rx/tx [ 18.825083] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x [ 18.831331] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x [ 18.832568] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=03 [ 18.832638] mvpp2 f2000000.ethernet eth1: pcs link up [ 18.832905] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive [ 18.832936] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control rx/tx [ 60.808001] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/Unknown/Full/none/rx/tx [ 60.808038] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive [ 60.808090] mvpp2 f2000000.ethernet eth1: pcs link down [ 60.809075] mvpp2 f2000000.ethernet eth1: Link is Down [ 62.857142] mvpp2 f2000000.ethernet eth1: phy link up sgmii/1Gbps/Full/none/rx/tx [ 62.857163] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii [ 62.863412] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband [ 62.871786] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii [ 62.872987] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=03 [ 62.873059] mvpp2 f2000000.ethernet eth1: pcs link up [ 62.873362] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active [ 62.873379] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us [ 62.873414] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx The problem is that the PCS continues to be in phy mode but the PHY driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s. What I'm wondering is if it wouldn't make sense to adapt the phy driver to support MLO_AN_PHY in SGMII/1000BASE-X mode. However, I don't know how I could do this because I can't get the information in the PHY driver (I can't use phylink_autoneg_inband to querry if inband should be enabled or disabled). I know we would still have the problem you described above. However, it would allow us to configure the phy to what phylink decided works best. Maybe this would also solve other issues where e.g. the PCS is not as flexible as it is for mvpp2 and mvneta? Currently the mxl-gpy phy driver can only support: 10/100/1000 MBit/s: SGMII with MLO_AN_INBAND 2500MBit/s 2500Base-X with MLO_AN_PHY However, the PHY would also support the following mode: 10/100/1000 MBit/s: SGMII with MLO_AN_PHY I just don't know how the PHY driver could know about what it should configure. Regards, Stefan
On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote: > Hi Russell, > > Sorry for the late reply but I wanted to give you some update after > testing with the latest version of your patches on net-queue. I've also been randomly distracted, and I've been meaning to ping you to test some of the updates. http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue The current set begins with: "net: sfp-bus: constify link_modes to sfp_select_interface()" which is now in net-next, then the patches between and including: "net: phylink: validate sfp_select_interface() returned interface" to "net: phylink: clean up phylink_resolve()" That should get enough together for the PCS "neg" mode to be consistent with what the MAC driver sees. The remaining bits that I still need to sort out is the contents of phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working out some way of handling the SGMII case where the PHY and PCS disagree (one only supporting inband the other not supporting inband.) I'm not sure when I'll be able to get to that - things are getting fairly chaotic again, meaning I have again less time to spend on mainline... and I'd like to take some vacation time very soon (I really need some time off!) > I think I see the problem you are describing. > > When the driver starts it will negotiate MLO_AN_PHY based on the > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s > it should switch to MLO_AN_INBAND but this does not work. Here the > output of phylink: I'm designing this to work the other way - inband being able to fall back to PHY (out of band) mode rather than PHY mode being able to fall forwards to inband mode. > The problem is that the PCS continues to be in phy mode but the PHY > driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s. > > What I'm wondering is if it wouldn't make sense to adapt the phy driver > to support MLO_AN_PHY in SGMII/1000BASE-X mode. PHYs have no idea about MLO_AN_xxx at all, neither should they. This is not part of phylib's API, and I don't think PHYs should ever know about MLO_AN_xxx stuff (which is something purely between phylink and the MAC driver.) The structure here is: MAC PCS PHY ^ ^ ^ ^-----. v v v | MAC driver <-> PCS driver <-------> PHY driver | ^ ^ ^ | | | | | MLO_AN_xxx PHYLINK_PCS_NEG_xxx | | ` ' | | \ / v | phylink <----------------> phylib <------' MLO_AN_xxx is far beyond the PHY, and more or less defines the overall "system" operating mode. PHYLINK_PCS_NEG_xxx defines the properties used for the PCS link to the next device towards the media. This is more of relevance to what the PHY should be using on its MAC-facing interface. > Currently the mxl-gpy phy driver can only support: > 10/100/1000 MBit/s: SGMII with MLO_AN_INBAND > 2500MBit/s 2500Base-X with MLO_AN_PHY > > However, the PHY would also support the following mode: > 10/100/1000 MBit/s: SGMII with MLO_AN_PHY The problem with this is some PHYs will not pass data _unless_ their SGMII control frame to the PCS has been acknowledged by the PCS - in other words, inband has to be used. However, that can be coped with, because such a PHY driver should be saying that it only supports LINK_INBAND_ENABLE in SGMII mode... and firmware must tell phylink that it wants to use inband mode (as that's exactly what firmware must do today in this situation.) > I just don't know how the PHY driver could know about what it should > configure. Currently, I haven't added an interface to cope with the case where a PHY says LINK_INBAND_ENABLE | LINK_INBAND_DISABLE to allow it to be configured in that case... that's something that will eventually be needed.
On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote: > On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote: > > Hi Russell, > > > > Sorry for the late reply but I wanted to give you some update after > > testing with the latest version of your patches on net-queue. > > I've also been randomly distracted, and I've been meaning to ping you > to test some of the updates. > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue > > The current set begins with: > > "net: sfp-bus: constify link_modes to sfp_select_interface()" which is > now in net-next, then the patches between and including: > > "net: phylink: validate sfp_select_interface() returned interface" to > "net: phylink: clean up phylink_resolve()" > > That should get enough together for the PCS "neg" mode to be consistent > with what the MAC driver sees. > > The remaining bits that I still need to sort out is the contents of > phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working > out some way of handling the SGMII case where the PHY and PCS disagree > (one only supporting inband the other not supporting inband.) > > I'm not sure when I'll be able to get to that - things are getting > fairly chaotic again, meaning I have again less time to spend on > mainline... and I'd like to take some vacation time very soon (I really > need some time off!) No problem, I'm also quite busy at the moment and I have the workaround to test the hardware, so it is nothing urgent for me. > > I think I see the problem you are describing. > > > > When the driver starts it will negotiate MLO_AN_PHY based on the > > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s > > it should switch to MLO_AN_INBAND but this does not work. Here the > > output of phylink: > > I'm designing this to work the other way - inband being able to fall > back to PHY (out of band) mode rather than PHY mode being able to fall > forwards to inband mode. I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think in my setup the problem is that it doesn't fall back to PHY mode but takes it as default mode. Here what happens when I have the mxl-gpy PHY connected to a 1000 GBit/s port: [ 9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02 [ 14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces [ 14.674853] mvpp2 f2000000.ethernet eth1: interface 2 (mii) rate match none supports 0-3,6,13-14 [ 14.674864] mvpp2 f2000000.ethernet eth1: interface 4 (sgmii) rate match none supports 0-3,5-6,13-14 [ 14.674871] mvpp2 f2000000.ethernet eth1: interface 9 (rgmii) rate match none supports 0-3,5-6,13-14 [ 14.674877] mvpp2 f2000000.ethernet eth1: interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14 [ 14.674883] mvpp2 f2000000.ethernet eth1: interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14 [ 14.674889] mvpp2 f2000000.ethernet eth1: interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14 [ 14.674895] mvpp2 f2000000.ethernet eth1: interface 22 (1000base-x) rate match none supports 5-6,13-14 [ 14.674900] mvpp2 f2000000.ethernet eth1: interface 23 (2500base-x) rate match none supports 6,13-14,47 [ 14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL) [ 14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f [ 14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode [ 14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x [ 14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x [ 14.700408] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=00 [ 14.702726] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/1Gbps/Full/none/off [ 17.768349] mvpp2 f2000000.ethernet eth1: phy link up sgmii/1Gbps/Full/none/rx/tx [ 17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii [ 17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband [ 17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii [ 17.784200] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=03 [ 17.784278] mvpp2 f2000000.ethernet eth1: pcs link up [ 17.784485] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active [ 17.784504] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us [ 17.784543] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx It agrees on PHY mode when starting, which then sets MLO_AN_PHY. However, when it sees a 1 GBit/s link it will just print the error message "firmware wants phy mode, but PHY requires inband". After "Link is Up" is shown the link will not work becuse of the missmatch between the PCS and the PHY. > > The problem is that the PCS continues to be in phy mode but the PHY > > driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s. > > > > What I'm wondering is if it wouldn't make sense to adapt the phy driver > > to support MLO_AN_PHY in SGMII/1000BASE-X mode. > > PHYs have no idea about MLO_AN_xxx at all, neither should they. This > is not part of phylib's API, and I don't think PHYs should ever know > about MLO_AN_xxx stuff (which is something purely between phylink and > the MAC driver.) The structure here is: > > MAC PCS PHY > ^ ^ ^ ^-----. > v v v | > MAC driver <-> PCS driver <-------> PHY driver | > ^ ^ ^ | > | | | | > MLO_AN_xxx PHYLINK_PCS_NEG_xxx | | > ` ' | | > \ / v | > phylink <----------------> phylib <------' > > MLO_AN_xxx is far beyond the PHY, and more or less defines the overall > "system" operating mode. PHYLINK_PCS_NEG_xxx defines the properties > used for the PCS link to the next device towards the media. This is > more of relevance to what the PHY should be using on its MAC-facing > interface. Okay this makes sense. I more thought about if the PHY e.g. would signalise it can do inband/outband and the PCS can only do outband. Basically exactly what you write in the last comment. > > Currently the mxl-gpy phy driver can only support: > > 10/100/1000 MBit/s: SGMII with MLO_AN_INBAND > > 2500MBit/s 2500Base-X with MLO_AN_PHY > > > > However, the PHY would also support the following mode: > > 10/100/1000 MBit/s: SGMII with MLO_AN_PHY > > The problem with this is some PHYs will not pass data _unless_ their > SGMII control frame to the PCS has been acknowledged by the PCS - in > other words, inband has to be used. However, that can be coped with, > because such a PHY driver should be saying that it only supports > LINK_INBAND_ENABLE in SGMII mode... and firmware must tell phylink > that it wants to use inband mode (as that's exactly what firmware > must do today in this situation.) > > > I just don't know how the PHY driver could know about what it should > > configure. > > Currently, I haven't added an interface to cope with the case where > a PHY says LINK_INBAND_ENABLE | LINK_INBAND_DISABLE to allow it to be > configured in that case... that's something that will eventually be > needed. Thanks a lot for the work and help so far. Regards, Stefan
On Fri, May 03, 2024 at 06:35:53PM +0200, Stefan Eichenberger wrote: > On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote: > > On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote: > > > Hi Russell, > > > > > > Sorry for the late reply but I wanted to give you some update after > > > testing with the latest version of your patches on net-queue. > > > > I've also been randomly distracted, and I've been meaning to ping you > > to test some of the updates. > > > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue > > > > The current set begins with: > > > > "net: sfp-bus: constify link_modes to sfp_select_interface()" which is > > now in net-next, then the patches between and including: > > > > "net: phylink: validate sfp_select_interface() returned interface" to > > "net: phylink: clean up phylink_resolve()" > > > > That should get enough together for the PCS "neg" mode to be consistent > > with what the MAC driver sees. > > > > The remaining bits that I still need to sort out is the contents of > > phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working > > out some way of handling the SGMII case where the PHY and PCS disagree > > (one only supporting inband the other not supporting inband.) > > > > I'm not sure when I'll be able to get to that - things are getting > > fairly chaotic again, meaning I have again less time to spend on > > mainline... and I'd like to take some vacation time very soon (I really > > need some time off!) > > No problem, I'm also quite busy at the moment and I have the workaround > to test the hardware, so it is nothing urgent for me. > > > > I think I see the problem you are describing. > > > > > > When the driver starts it will negotiate MLO_AN_PHY based on the > > > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s > > > it should switch to MLO_AN_INBAND but this does not work. Here the > > > output of phylink: > > > > I'm designing this to work the other way - inband being able to fall > > back to PHY (out of band) mode rather than PHY mode being able to fall > > forwards to inband mode. > > I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think > in my setup the problem is that it doesn't fall back to PHY mode but > takes it as default mode. Here what happens when I have the mxl-gpy PHY > connected to a 1000 GBit/s port: > [ 9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02 > [ 14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces > [ 14.674853] mvpp2 f2000000.ethernet eth1: interface 2 (mii) rate match none supports 0-3,6,13-14 > [ 14.674864] mvpp2 f2000000.ethernet eth1: interface 4 (sgmii) rate match none supports 0-3,5-6,13-14 > [ 14.674871] mvpp2 f2000000.ethernet eth1: interface 9 (rgmii) rate match none supports 0-3,5-6,13-14 > [ 14.674877] mvpp2 f2000000.ethernet eth1: interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14 > [ 14.674883] mvpp2 f2000000.ethernet eth1: interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14 > [ 14.674889] mvpp2 f2000000.ethernet eth1: interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14 > [ 14.674895] mvpp2 f2000000.ethernet eth1: interface 22 (1000base-x) rate match none supports 5-6,13-14 > [ 14.674900] mvpp2 f2000000.ethernet eth1: interface 23 (2500base-x) rate match none supports 6,13-14,47 > [ 14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL) > [ 14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f > [ 14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode > [ 14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x ^^^ You're still requesting (from firmware) for PHY mode, and phylink will _always_ use out-of-band if firmware requests that. > [ 14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x So it uses PHY mode for 2500base-X, which is correct. > [ 17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii Still requesting PHY mode with SGMII, which historically we've always used out-of-band mode for, so we preserve that behaviour. > [ 17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband So we complain about it with an error, because it is wrong... > [ 17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii and we still try to use it (correctly, because that's what phylink has always done in this case.) As I tried to explain, there is fall-back from MLO_AN_INBAND to MLO_AN_PHY, but there won't be fall-forward from MLO_AN_PHY to MLO_AN_INBAND.
On Fri, May 03, 2024 at 11:41:03PM +0100, Russell King (Oracle) wrote: > On Fri, May 03, 2024 at 06:35:53PM +0200, Stefan Eichenberger wrote: > > On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote: > > > On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote: > > > > Hi Russell, > > > > > > > > Sorry for the late reply but I wanted to give you some update after > > > > testing with the latest version of your patches on net-queue. > > > > > > I've also been randomly distracted, and I've been meaning to ping you > > > to test some of the updates. > > > > > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue > > > > > > The current set begins with: > > > > > > "net: sfp-bus: constify link_modes to sfp_select_interface()" which is > > > now in net-next, then the patches between and including: > > > > > > "net: phylink: validate sfp_select_interface() returned interface" to > > > "net: phylink: clean up phylink_resolve()" > > > > > > That should get enough together for the PCS "neg" mode to be consistent > > > with what the MAC driver sees. > > > > > > The remaining bits that I still need to sort out is the contents of > > > phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working > > > out some way of handling the SGMII case where the PHY and PCS disagree > > > (one only supporting inband the other not supporting inband.) > > > > > > I'm not sure when I'll be able to get to that - things are getting > > > fairly chaotic again, meaning I have again less time to spend on > > > mainline... and I'd like to take some vacation time very soon (I really > > > need some time off!) > > > > No problem, I'm also quite busy at the moment and I have the workaround > > to test the hardware, so it is nothing urgent for me. > > > > > > I think I see the problem you are describing. > > > > > > > > When the driver starts it will negotiate MLO_AN_PHY based on the > > > > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s > > > > it should switch to MLO_AN_INBAND but this does not work. Here the > > > > output of phylink: > > > > > > I'm designing this to work the other way - inband being able to fall > > > back to PHY (out of band) mode rather than PHY mode being able to fall > > > forwards to inband mode. > > > > I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think > > in my setup the problem is that it doesn't fall back to PHY mode but > > takes it as default mode. Here what happens when I have the mxl-gpy PHY > > connected to a 1000 GBit/s port: > > [ 9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02 > > [ 14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces > > [ 14.674853] mvpp2 f2000000.ethernet eth1: interface 2 (mii) rate match none supports 0-3,6,13-14 > > [ 14.674864] mvpp2 f2000000.ethernet eth1: interface 4 (sgmii) rate match none supports 0-3,5-6,13-14 > > [ 14.674871] mvpp2 f2000000.ethernet eth1: interface 9 (rgmii) rate match none supports 0-3,5-6,13-14 > > [ 14.674877] mvpp2 f2000000.ethernet eth1: interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14 > > [ 14.674883] mvpp2 f2000000.ethernet eth1: interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14 > > [ 14.674889] mvpp2 f2000000.ethernet eth1: interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14 > > [ 14.674895] mvpp2 f2000000.ethernet eth1: interface 22 (1000base-x) rate match none supports 5-6,13-14 > > [ 14.674900] mvpp2 f2000000.ethernet eth1: interface 23 (2500base-x) rate match none supports 6,13-14,47 > > [ 14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL) > > [ 14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f > > [ 14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode > > [ 14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x > > ^^^ > > You're still requesting (from firmware) for PHY mode, and phylink will > _always_ use out-of-band if firmware requests that. > > > [ 14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x > > So it uses PHY mode for 2500base-X, which is correct. > > > [ 17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii > > Still requesting PHY mode with SGMII, which historically we've always > used out-of-band mode for, so we preserve that behaviour. > > > [ 17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband > > So we complain about it with an error, because it is wrong... > > > [ 17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii > > and we still try to use it (correctly, because that's what phylink > has always done in this case.) > > As I tried to explain, there is fall-back from MLO_AN_INBAND to > MLO_AN_PHY, but there won't be fall-forward from MLO_AN_PHY to > MLO_AN_INBAND. I still don't get it, sorry. So the mxl-gpy driver currently has two modes: 2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband) 1000 MBit/s -> PHY_INTERFACE_MODE_SGMII -> LINK_INBAND_ENABLE If I use this configureation it will not work because there is no fallback from MLO_AN_PHY to MLO_AN_INBAND. Now if I understand everyting correctly, this happens for 1000 MBit/s and SGMII because the firmware decides to use PHY mode. I modified the PHY driver to use 1000BASEX instead: 2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband) 1000 MBit/s -> PHY_INTERFACE_MODE_1000BASEX -> LINK_INBAND_ENABLE However, the same thing happens: [ 14.635831] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL) [ 14.646742] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f [ 14.647986] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode [ 14.655784] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x [ 14.663313] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x [ 14.663323] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=00 [ 14.666098] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/2.5Gbps/Full/none/rx/tx [ 18.760959] mvpp2 f2000000.ethernet eth1: phy link up 2500base-x/2.5Gbps/Full/none/rx/tx [ 18.760977] mvpp2 f2000000.ethernet eth1: pcs link up [ 18.761211] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive [ 18.761231] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control rx/tx [ 70.983936] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/Unknown/Full/none/rx/tx [ 70.983965] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive [ 70.984017] mvpp2 f2000000.ethernet eth1: pcs link down [ 70.985000] mvpp2 f2000000.ethernet eth1: Link is Down [ 74.057088] mvpp2 f2000000.ethernet eth1: phy link up 1000base-x/1Gbps/Full/none/rx/tx [ 74.057109] mvpp2 f2000000.ethernet eth1: major config, requested phy/1000base-x [ 74.063342] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband [ 74.071706] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/1000base-x [ 74.072902] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/1000base-x/none adv=00,00000000,00000000,00000000 pause=03 [ 74.072976] mvpp2 f2000000.ethernet eth1: pcs link up [ 74.073225] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active [ 74.073245] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us [ 74.073279] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx If I then request inband by setting managed = "in-band-status" in the device tree the logic will fail because there is not phydev and therefore the link will never be configured for phy mode (it falls back to "Legacy, so determine inband depending on the advertising bit"): [ 9.299037] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02 [ 14.586343] mvpp2 f2000000.ethernet eth1: configuring for inband/2500base-x link mode [ 14.595669] mvpp2 f2000000.ethernet eth1: major config, requested inband/2500base-x [ 14.631876] mvpp2 f2000000.ethernet eth1: major config, active inband/inband/an-enabled/2500base-x [ 14.631887] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000e240 pause=04 [ 14.633208] mvpp2 f2000000.ethernet eth1: pcs link up [ 14.633241] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive [ 14.633262] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control off [ 60.713862] mvpp2 f2000000.ethernet eth1: pcs link down [ 60.713947] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive [ 60.714978] mvpp2 f2000000.ethernet eth1: Link is Down [ 60.720200] mvpp2 f2000000.ethernet eth1: pcs link up [ 60.720586] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive [ 60.720619] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control off [ 63.012413] mvpp2 f2000000.ethernet eth1: pcs link down [ 63.012444] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive [ 63.013480] mvpp2 f2000000.ethernet eth1: Link is Down Or is there a way to configure the firmware to use inband by default but sill having a phydev device? For me it is not entirely clear which scenario should work for the mxl-gpy in the end? Scenario A: 2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband) 1000 MBit/s -> PHY_INTERFACE_MODE_SGMII -> LINK_INBAND_ENABLE Then I would need a way to tell that the default mode is inband but that there is still a phydev device available. Maybe this is possible and I simply don't know how to do it? Scenario B: 2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> LINK_INBAND_DISABLE 1000 MBit/s -> PHY_INTERFACE_MODE_1000BASEX -> LINK_INBAND_ENABLE Then the phylink logic should change the firmwares mode depending on the speed. Would this also work with 100MBit/s and 10MBit/s? Scenario C (which I understand is not wanted): 2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband) 1000 MBit/s -> PHY_INTERFACE_MODE_SGMII -> (0 no inband) This would already work but would require me to change the phy driver and is not the desired behaviour anyways. Sorry for my confusion, regards, Stefan
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c index b2d36a3a96f1..4147b4c29eaf 100644 --- a/drivers/net/phy/mxl-gpy.c +++ b/drivers/net/phy/mxl-gpy.c @@ -114,6 +114,7 @@ struct gpy_priv { * is enabled. */ u64 lb_dis_to; + bool sgmii_match_tpi_speed; }; static const struct { @@ -262,8 +263,17 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr) static int gpy_config_init(struct phy_device *phydev) { + struct gpy_priv *priv = phydev->priv; int ret; + /* Disalbe SGMII Autoneg if we want to match SGMII to TPI speed */ + if (priv->sgmii_match_tpi_speed) { + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL, + VSPEC1_SGMII_CTRL_ANEN, 0); + if (ret < 0) + return ret; + } + /* Mask all interrupts */ ret = phy_write(phydev, PHY_IMASK, 0); if (ret) @@ -304,6 +314,9 @@ static int gpy_probe(struct phy_device *phydev) if (!device_property_present(dev, "maxlinear,use-broken-interrupts")) phydev->dev_flags |= PHY_F_NO_IRQ; + priv->sgmii_match_tpi_speed = + device_property_present(dev, "maxlinear,sgmii-match-tpi-speed"); + fw_version = phy_read(phydev, PHY_FWV); if (fw_version < 0) return fw_version; @@ -516,6 +529,7 @@ static int gpy_update_mdix(struct phy_device *phydev) static int gpy_update_interface(struct phy_device *phydev) { + struct gpy_priv *priv = phydev->priv; int ret; /* Interface mode is fixed for USXGMII and integrated PHY */ @@ -529,6 +543,8 @@ static int gpy_update_interface(struct phy_device *phydev) switch (phydev->speed) { case SPEED_2500: phydev->interface = PHY_INTERFACE_MODE_2500BASEX; + if (!gpy_sgmii_aneg_en(phydev)) + break; ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL, VSPEC1_SGMII_CTRL_ANEN, 0); if (ret < 0) { @@ -542,7 +558,7 @@ static int gpy_update_interface(struct phy_device *phydev) case SPEED_100: case SPEED_10: phydev->interface = PHY_INTERFACE_MODE_SGMII; - if (gpy_sgmii_aneg_en(phydev)) + if (gpy_sgmii_aneg_en(phydev) || priv->sgmii_match_tpi_speed) break; /* Enable and restart SGMII ANEG for 10/100/1000Mbps link speed * if ANEG is disabled (in 2500-BaseX mode).
Add a new device tree property to disable SGMII autonegotiation and instead use the option to match the SGMII speed to what was negotiated on the twisted pair interface (tpi). This allows us to disable SGMII autonegotiation on Ethernet controllers that are not compatible with this mode. Signed-off-by: Stefan Eichenberger <eichest@gmail.com> --- drivers/net/phy/mxl-gpy.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)