Re: [net-next PATCH v2 11/14] net: axienet: Convert to use PCS subsystem

[Sean Anderson <sean.anderson@linux.dev>]

On 4/8/25 08:19, Gupta, Suraj wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Tuesday, April 8, 2025 4:51 AM
>> To: netdev@vger.kernel.org; Andrew Lunn <andrew+netdev@lunn.ch>; David S .
>> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
>> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Russell King
>> <linux@armlinux.org.uk>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>; upstream@airoha.com; Kory
>> Maincent <kory.maincent@bootlin.com>; Christian Marangi
>> <ansuelsmth@gmail.com>; linux-kernel@vger.kernel.org; Simek, Michal
>> <michal.simek@amd.com>; Pandey, Radhey Shyam
>> <radhey.shyam.pandey@amd.com>; Robert Hancock
>> <robert.hancock@calian.com>; linux-arm-kernel@lists.infradead.org; Sean
>> Anderson <sean.anderson@linux.dev>
>> Subject: [net-next PATCH v2 11/14] net: axienet: Convert to use PCS subsystem
>>
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> Convert the AXI Ethernet driver to use the PCS subsystem, including the new Xilinx
>> PCA/PMA driver. Unfortunately, we must use a helper to work with bare MDIO
>> nodes without a compatible.
>>
> 
> AXI ethernet changes looks fine to me, except one minor nit mentioned below. Using DT changesets for backward compatibility is impressive :)
> I'll try reviewing pcs/pma patch also and test it with our setups.
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> 
> Reviewed-by: Suraj Gupta <suraj.gupta2@amd.com>
>> ---
>>
>> (no changes since v1)
>>
>>  drivers/net/ethernet/xilinx/Kconfig           |   1 +
>>  drivers/net/ethernet/xilinx/xilinx_axienet.h  |   4 +-
>>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 104 ++++--------------
>>  drivers/net/pcs/Kconfig                       |   1 -
>>  4 files changed, 22 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
>> index 7502214cc7d5..2eab64cf1646 100644
>> --- a/drivers/net/ethernet/xilinx/Kconfig
>> +++ b/drivers/net/ethernet/xilinx/Kconfig
>> @@ -27,6 +27,7 @@ config XILINX_AXI_EMAC
>>         tristate "Xilinx 10/100/1000 AXI Ethernet support"
>>         depends on HAS_IOMEM
>>         depends on XILINX_DMA
>> +       select OF_DYNAMIC if PCS_XILINX
>>         select PHYLINK
>>         select DIMLIB
>>         help
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> index 5ff742103beb..f46e862245eb 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> @@ -473,7 +473,6 @@ struct skbuf_dma_descriptor {
>>   * @dev:       Pointer to device structure
>>   * @phylink:   Pointer to phylink instance
>>   * @phylink_config: phylink configuration settings
>> - * @pcs_phy:   Reference to PCS/PMA PHY if used
>>   * @pcs:       phylink pcs structure for PCS PHY
>>   * @switch_x_sgmii: Whether switchable 1000BaseX/SGMII mode is enabled in
>> the core
>>   * @axi_clk:   AXI4-Lite bus clock
>> @@ -553,8 +552,7 @@ struct axienet_local {
>>         struct phylink *phylink;
>>         struct phylink_config phylink_config;
>>
>> -       struct mdio_device *pcs_phy;
>> -       struct phylink_pcs pcs;
>> +       struct phylink_pcs *pcs;
>>
>>         bool switch_x_sgmii;
>>
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index 054abf283ab3..07487c4b2141 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -35,6 +35,8 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/skbuff.h>
>>  #include <linux/math64.h>
>> +#include <linux/pcs.h>
>> +#include <linux/pcs-xilinx.h>
>>  #include <linux/phy.h>
>>  #include <linux/mii.h>
>>  #include <linux/ethtool.h>
>> @@ -2519,63 +2521,6 @@ static const struct ethtool_ops axienet_ethtool_ops = {
>>         .get_rmon_stats = axienet_ethtool_get_rmon_stats,  };
>>
>> -static struct axienet_local *pcs_to_axienet_local(struct phylink_pcs *pcs) -{
>> -       return container_of(pcs, struct axienet_local, pcs);
>> -}
>> -
>> -static void axienet_pcs_get_state(struct phylink_pcs *pcs,
>> -                                 unsigned int neg_mode,
>> -                                 struct phylink_link_state *state)
>> -{
>> -       struct mdio_device *pcs_phy = pcs_to_axienet_local(pcs)->pcs_phy;
>> -
>> -       phylink_mii_c22_pcs_get_state(pcs_phy, neg_mode, state);
>> -}
>> -
>> -static void axienet_pcs_an_restart(struct phylink_pcs *pcs) -{
>> -       struct mdio_device *pcs_phy = pcs_to_axienet_local(pcs)->pcs_phy;
>> -
>> -       phylink_mii_c22_pcs_an_restart(pcs_phy);
>> -}
>> -
>> -static int axienet_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>> -                             phy_interface_t interface,
>> -                             const unsigned long *advertising,
>> -                             bool permit_pause_to_mac)
>> -{
>> -       struct mdio_device *pcs_phy = pcs_to_axienet_local(pcs)->pcs_phy;
>> -       struct net_device *ndev = pcs_to_axienet_local(pcs)->ndev;
>> -       struct axienet_local *lp = netdev_priv(ndev);
>> -       int ret;
>> -
>> -       if (lp->switch_x_sgmii) {
>> -               ret = mdiodev_write(pcs_phy, XLNX_MII_STD_SELECT_REG,
>> -                                   interface == PHY_INTERFACE_MODE_SGMII ?
>> -                                       XLNX_MII_STD_SELECT_SGMII : 0);
>> -               if (ret < 0) {
>> -                       netdev_warn(ndev,
>> -                                   "Failed to switch PHY interface: %d\n",
>> -                                   ret);
>> -                       return ret;
>> -               }
>> -       }
>> -
>> -       ret = phylink_mii_c22_pcs_config(pcs_phy, interface, advertising,
>> -                                        neg_mode);
>> -       if (ret < 0)
>> -               netdev_warn(ndev, "Failed to configure PCS: %d\n", ret);
>> -
>> -       return ret;
>> -}
>> -
>> -static const struct phylink_pcs_ops axienet_pcs_ops = {
>> -       .pcs_get_state = axienet_pcs_get_state,
>> -       .pcs_config = axienet_pcs_config,
>> -       .pcs_an_restart = axienet_pcs_an_restart,
>> -};
>> -
>>  static struct phylink_pcs *axienet_mac_select_pcs(struct phylink_config *config,
>>                                                   phy_interface_t interface)  { @@ -2583,8 +2528,8
>> @@ static struct phylink_pcs *axienet_mac_select_pcs(struct phylink_config
>> *config,
>>         struct axienet_local *lp = netdev_priv(ndev);
>>
>>         if (interface == PHY_INTERFACE_MODE_1000BASEX ||
>> -           interface ==  PHY_INTERFACE_MODE_SGMII)
>> -               return &lp->pcs;
>> +           interface == PHY_INTERFACE_MODE_SGMII)
> 
> nit: unchanged check.

Just fixing up the spacing while I was "in the area"

--Sean