[PATCH net-next 0/2] doc: sfp-phylink: update the porting guide

Hi everyone,

I'm currently in the process of porting fs_enet to phylink, and I'm
using this occasion to update the porting guide along the way. Let me
know if these changes are clear enough, and if there are any mistakes in
there.

Thanks,

Maxime

Maxime Chevallier (2):
  doc: sfp-phylink: drop the mention to phylink_config->pcs_poll
  doc: sfp-phylink: mention the mac_capabilities and
    supported_interfaces

 Documentation/networking/sfp-phylink.rst | 48 ++++++++++++++++++------
 1 file changed, 37 insertions(+), 11 deletions(-)

-- 
2.43.2

commit 64b4a0f8b51b ("net: phylink: remove phylink_config's pcs_poll")
dropped the phylink_config->pcs_poll field, which is no longer required
to be set by MAC drivers. Remove that mention in the phylink porting
guide.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/sfp-phylink.rst | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/networking/sfp-phylink.rst b/Documentation/networking/sfp-phylink.rst
index 8054d33f449f..bc3365bbf096 100644
--- a/Documentation/networking/sfp-phylink.rst
+++ b/Documentation/networking/sfp-phylink.rst

@@ -264,8 +264,7 @@ this documentation.
 	phylink_mac_change(priv->phylink, link_is_up);
 
     where ``link_is_up`` is true if the link is currently up or false
-    otherwise. If a MAC is unable to provide these interrupts, then
-    it should set ``priv->phylink_config.pcs_poll = true;`` in step 9.
+    otherwise.
 
 11. Verify that the driver does not call::
 

-- 
2.43.2

When porting a driver from bare phylib to phylink, one of the mandatory
steps is to fill-in the phylink_config.mac_capabilities and the
supported_interfaces. Add a dedicated step in the porting guide, with
some examples.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/sfp-phylink.rst | 45 +++++++++++++++++++-----
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/Documentation/networking/sfp-phylink.rst b/Documentation/networking/sfp-phylink.rst
index bc3365bbf096..30b1f2cf997f 100644
--- a/Documentation/networking/sfp-phylink.rst
+++ b/Documentation/networking/sfp-phylink.rst

@@ -231,16 +231,43 @@ this documentation.
    For further information on these methods, please see the inline
    documentation in :c:type:`struct phylink_mac_ops <phylink_mac_ops>`.
 
-9. Remove calls to of_parse_phandle() for the PHY,
-   of_phy_register_fixed_link() for fixed links etc. from the probe
-   function, and replace with:
+9. Fill-in the :c:type:`struct phylink_config <phylink_config>` fields with
+   a reference to the :c:type:`struct device <device>` associated to your
+   :c:type:`struct net_device <net_device>`:
 
    .. code-block:: c
 
-	struct phylink *phylink;
 	priv->phylink_config.dev = &dev.dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
 
+   Fill-in the various speeds, pause and duplex modes your MAC can handle:
+
+   .. code-block:: c
+
+        priv->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD;
+
+   Fill-in all the :c:type:`phy_interface_t <phy_interface_t>` (i.e. all MAC to
+   PHY link modes) that your MAC can output. The following example shows a
+   configuration for a MAC that can handle all RGMII modes, SGMII and 1000BaseX.
+   You must adjust these according to what your MAC is capable of, and not just
+   the interface you wish to use:
+
+   .. code-block:: c
+
+       phy_interface_set_rgmii(priv->phylink_config.supported_interfaces);
+        __set_bit(PHY_INTERFACE_MODE_SGMII,
+                  priv->phylink_config.supported_interfaces);
+        __set_bit(PHY_INTERFACE_MODE_1000BASEX,
+                  priv->phylink_config.supported_interfaces);
+
+10. Remove calls to of_parse_phandle() for the PHY,
+    of_phy_register_fixed_link() for fixed links etc. from the probe
+    function, and replace with:
+
+    .. code-block:: c
+
+	struct phylink *phylink;
+
 	phylink = phylink_create(&priv->phylink_config, node, phy_mode, &phylink_ops);
 	if (IS_ERR(phylink)) {
 		err = PTR_ERR(phylink);

@@ -249,14 +276,14 @@ this documentation.
 
 	priv->phylink = phylink;
 
-   and arrange to destroy the phylink in the probe failure path as
-   appropriate and the removal path too by calling:
+    and arrange to destroy the phylink in the probe failure path as
+    appropriate and the removal path too by calling:
 
-   .. code-block:: c
+    .. code-block:: c
 
 	phylink_destroy(priv->phylink);
 
-10. Arrange for MAC link state interrupts to be forwarded into
+11. Arrange for MAC link state interrupts to be forwarded into
     phylink, via:
 
     .. code-block:: c

@@ -266,7 +293,7 @@ this documentation.
     where ``link_is_up`` is true if the link is currently up or false
     otherwise.
 
-11. Verify that the driver does not call::
+12. Verify that the driver does not call::
 
 	netif_carrier_on()
 	netif_carrier_off()

-- 
2.43.2

Hello everyone,

Russell, did you get a chance to take a look at this series (maybe it
went under the radar, if not, sorry about the ping) ? I'd like to make
sure the examples are meaningful and correct, and you're clearly the
most qualified to comment on this :)

Best regards,

Maxime

On Tue, 20 Feb 2024 17:04:02 +0100
Maxime Chevallier [off-list ref] wrote:

Hi everyone,

I'm currently in the process of porting fs_enet to phylink, and I'm
using this occasion to update the porting guide along the way. Let me
know if these changes are clear enough, and if there are any mistakes in
there.

Thanks,

Maxime

Maxime Chevallier (2):
  doc: sfp-phylink: drop the mention to phylink_config->pcs_poll
  doc: sfp-phylink: mention the mac_capabilities and
    supported_interfaces

 Documentation/networking/sfp-phylink.rst | 48 ++++++++++++++++++------
 1 file changed, 37 insertions(+), 11 deletions(-)

On Tue, Feb 20, 2024 at 05:04:03PM +0100, Maxime Chevallier wrote:

commit 64b4a0f8b51b ("net: phylink: remove phylink_config's pcs_poll")
dropped the phylink_config->pcs_poll field, which is no longer required
to be set by MAC drivers. Remove that mention in the phylink porting
guide.

The porting guide needs to be updated with the PCS, and the details for
that moved over rather than being deleted. While it's true that this
member is gone from phylink_config, it was only removed after the
introduction of the similarly named member in phylink_pcs.

In other words, point 10 should probably read:

10. Arrange for PCS link state interrupts to be forwarded into
    phylink, via:

    .. code-block:: c

        phylink_pcs_change(pcs, link_is_up);

    where ``link_is_up`` is true if the link is currently up or false
    otherwise. If a PCS is unable to provide these interrupts, then
    it should set ``pcs->pcs_poll = true;`` when creating the PCS.

However, for that to make sense, we then need the guide to provide
details about creating the PCS, and also the mac_select_pcs() method.
Thus my comment about a much bigger update being required.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Tue, Feb 20, 2024 at 05:04:04PM +0100, Maxime Chevallier wrote:

+   Fill-in all the :c:type:`phy_interface_t <phy_interface_t>` (i.e. all MAC to
+   PHY link modes) that your MAC can output. The following example shows a

Technically, this should be "MAC and all PCS associated with this MAC".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Hi Russell,

On Mon, 26 Feb 2024 11:41:07 +0000
"Russell King (Oracle)" [off-list ref] wrote:

On Tue, Feb 20, 2024 at 05:04:03PM +0100, Maxime Chevallier wrote:

quoted

commit 64b4a0f8b51b ("net: phylink: remove phylink_config's pcs_poll")
dropped the phylink_config->pcs_poll field, which is no longer required
to be set by MAC drivers. Remove that mention in the phylink porting
guide.  

The porting guide needs to be updated with the PCS, and the details for
that moved over rather than being deleted. While it's true that this
member is gone from phylink_config, it was only removed after the
introduction of the similarly named member in phylink_pcs.

In other words, point 10 should probably read:

10. Arrange for PCS link state interrupts to be forwarded into
    phylink, via:

    .. code-block:: c

        phylink_pcs_change(pcs, link_is_up);

    where ``link_is_up`` is true if the link is currently up or false
    otherwise. If a PCS is unable to provide these interrupts, then
    it should set ``pcs->pcs_poll = true;`` when creating the PCS.

However, for that to make sense, we then need the guide to provide
details about creating the PCS, and also the mac_select_pcs() method.
Thus my comment about a much bigger update being required.

Makes sense. If that's OK I'll send a V2 with a PCS handling addition
to it, and see how that looks.

Thanks for the review,

Maxime

On Mon, 26 Feb 2024 11:45:03 +0000
"Russell King (Oracle)" [off-list ref] wrote:

On Tue, Feb 20, 2024 at 05:04:04PM +0100, Maxime Chevallier wrote:

quoted

+   Fill-in all the :c:type:`phy_interface_t <phy_interface_t>` (i.e. all MAC to
+   PHY link modes) that your MAC can output. The following example shows a  

Technically, this should be "MAC and all PCS associated with this MAC".

Given that PCS should be covered in the doc, that's true :) I'll add
that in V2.

Thanks,

Maxime