RE: [PATCH net-next v3 39/47] net: fman: memac: Add serdes support

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@xxxxxxxx>
> Sent: Thursday, July 21, 2022 18:38
> To: Camelia Alexandra Groza <camelia.groza@xxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Madalin Bucur
> <madalin.bucur@xxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Cc: Paolo Abeni <pabeni@xxxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Russell
> King <linux@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v3 39/47] net: fman: memac: Add serdes
> support
> 
> 
> 
> On 7/21/22 9:30 AM, Camelia Alexandra Groza wrote:
> >> -----Original Message-----
> >> From: Sean Anderson <sean.anderson@xxxxxxxx>
> >> Sent: Saturday, July 16, 2022 1:00
> >> To: David S . Miller <davem@xxxxxxxxxxxxx>; Jakub Kicinski
> >> <kuba@xxxxxxxxxx>; Madalin Bucur <madalin.bucur@xxxxxxx>;
> >> netdev@xxxxxxxxxxxxxxx
> >> Cc: Paolo Abeni <pabeni@xxxxxxxxxx>; Eric Dumazet
> >> <edumazet@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Russell
> >> King <linux@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Sean
> Anderson
> >> <sean.anderson@xxxxxxxx>
> >> Subject: [PATCH net-next v3 39/47] net: fman: memac: Add serdes
> support
> >>
> >> This adds support for using a serdes which has to be configured. This is
> >> primarly in preparation for the next commit, which will then change the
> >> serdes mode dynamically.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  .../net/ethernet/freescale/fman/fman_memac.c  | 48
> >> ++++++++++++++++++-
> >>  1 file changed, 46 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c
> >> b/drivers/net/ethernet/freescale/fman/fman_memac.c
> >> index 02b3a0a2d5d1..a62fe860b1d0 100644
> >> --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> >> +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> >> @@ -13,6 +13,7 @@
> >>  #include <linux/io.h>
> >>  #include <linux/phy.h>
> >>  #include <linux/phy_fixed.h>
> >> +#include <linux/phy/phy.h>
> >>  #include <linux/of_mdio.h>
> >>
> >>  /* PCS registers */
> >> @@ -324,6 +325,7 @@ struct fman_mac {
> >>  	void *fm;
> >>  	struct fman_rev_info fm_rev_info;
> >>  	bool basex_if;
> >> +	struct phy *serdes;
> >>  	struct phy_device *pcsphy;
> >>  	bool allmulti_enabled;
> >>  };
> >> @@ -1203,17 +1205,55 @@ int memac_initialization(struct mac_device
> >> *mac_dev,
> >>  		}
> >>  	}
> >>
> >> +	memac->serdes = devm_of_phy_get(mac_dev->dev, mac_node,
> >> "serdes");
> >
> > devm_of_phy_get returns -ENOSYS on PPC builds because
> CONFIG_GENERIC_PHY isn't
> > enabled by default. Please add a dependency.
> >
> >> +	if (PTR_ERR(memac->serdes) == -ENODEV) {
> 
> I think it is better to add -ENOSYS to the condition here. That way,
> the phy subsystem stays optional.
> 
> --Sean

Sure, sounds good.

> >> +		memac->serdes = NULL;
> >> +	} else if (IS_ERR(memac->serdes)) {
> >> +		err = PTR_ERR(memac->serdes);
> >> +		dev_err_probe(mac_dev->dev, err, "could not get
> >> serdes\n");
> >> +		goto _return_fm_mac_free;
> >> +	} else {
> >> +		err = phy_init(memac->serdes);
> >> +		if (err) {
> >> +			dev_err_probe(mac_dev->dev, err,
> >> +				      "could not initialize serdes\n");
> >> +			goto _return_fm_mac_free;
> >> +		}
> >> +
> >> +		err = phy_power_on(memac->serdes);
> >> +		if (err) {
> >> +			dev_err_probe(mac_dev->dev, err,
> >> +				      "could not power on serdes\n");
> >> +			goto _return_phy_exit;
> >> +		}
> >> +
> >> +		if (memac->phy_if == PHY_INTERFACE_MODE_SGMII ||
> >> +		    memac->phy_if == PHY_INTERFACE_MODE_1000BASEX ||
> >> +		    memac->phy_if == PHY_INTERFACE_MODE_2500BASEX ||
> >> +		    memac->phy_if == PHY_INTERFACE_MODE_QSGMII ||
> >> +		    memac->phy_if == PHY_INTERFACE_MODE_XGMII) {
> >> +			err = phy_set_mode_ext(memac->serdes,
> >> PHY_MODE_ETHERNET,
> >> +					       memac->phy_if);
> >> +			if (err) {
> >> +				dev_err_probe(mac_dev->dev, err,
> >> +					      "could not set serdes mode
> >> to %s\n",
> >> +					      phy_modes(memac->phy_if));
> >> +				goto _return_phy_power_off;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >>  	if (!mac_dev->phy_node && of_phy_is_fixed_link(mac_node)) {
> >>  		struct phy_device *phy;
> >>
> >>  		err = of_phy_register_fixed_link(mac_node);
> >>  		if (err)
> >> -			goto _return_fm_mac_free;
> >> +			goto _return_phy_power_off;
> >>
> >>  		fixed_link = kzalloc(sizeof(*fixed_link), GFP_KERNEL);
> >>  		if (!fixed_link) {
> >>  			err = -ENOMEM;
> >> -			goto _return_fm_mac_free;
> >> +			goto _return_phy_power_off;
> >>  		}
> >>
> >>  		mac_dev->phy_node = of_node_get(mac_node);
> >> @@ -1242,6 +1282,10 @@ int memac_initialization(struct mac_device
> >> *mac_dev,
> >>
> >>  	goto _return;
> >>
> >> +_return_phy_power_off:
> >> +	phy_power_off(memac->serdes);
> >> +_return_phy_exit:
> >> +	phy_exit(memac->serdes);
> >>  _return_fixed_link_free:
> >>  	kfree(fixed_link);
> >
> > _return_fixed_link_free should execute before _return_phy_power_off
> and _return_phy_exit
> >
> >>  _return_fm_mac_free:
> >> --
> >> 2.35.1.1320.gc452695387.dirty
> >