diff mbox series

[net-next] net: stmmac: Fix ethool link settings ops for integrated PCS

Message ID 20231218135032.27209-1-quic_snehshah@quicinc.com
State New
Headers
Series [net-next] net: stmmac: Fix ethool link settings ops for integrated PCS |

Commit Message

Sneh Shah Dec. 18, 2023, 1:50 p.m. UTC 
  Currently get/set_link_ksettings ethtool ops are dependent on PCS.
When PCS is integrated in MAC, it will not have separate link config.
Bypass cofiguring and checking PCS link config for integrated PCS.

Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Andrew Halaney Dec. 18, 2023, 4:14 p.m. UTC | #1 
Hi,

I think the subject should be [PATCH net] since this is a fix.

On Mon, Dec 18, 2023 at 07:20:32PM +0530, Sneh Shah wrote:
> Currently get/set_link_ksettings ethtool ops are dependent on PCS.
> When PCS is integrated in MAC, it will not have separate link config.
> Bypass cofiguring and checking PCS link config for integrated PCS.

s/cofiguring/configuring/

Please add:

    Fixes: ("aa571b6275fb net: stmmac: add new switch to struct plat_stmmacenet_data")

This fixes using the ethtool ops for me so also please feel free to add:

    Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8775p-ride

Thanks for the patch!

> 
> Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index f628411ae4ae..e3ba4cd47b8d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -311,8 +311,9 @@ static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> -	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
> -	    priv->hw->pcs & STMMAC_PCS_SGMII) {
> +	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
> +	    (priv->hw->pcs & STMMAC_PCS_RGMII ||
> +	     priv->hw->pcs & STMMAC_PCS_SGMII)) {
>  		struct rgmii_adv adv;
>  		u32 supported, advertising, lp_advertising;
>  
> @@ -397,8 +398,9 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev,
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> -	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
> -	    priv->hw->pcs & STMMAC_PCS_SGMII) {
> +	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
> +	    (priv->hw->pcs & STMMAC_PCS_RGMII ||
> +	     priv->hw->pcs & STMMAC_PCS_SGMII)) {
>  		/* Only support ANE */
>  		if (cmd->base.autoneg != AUTONEG_ENABLE)
>  			return -EINVAL;
> -- 
> 2.17.1
>
Suman Ghosh Dec. 20, 2023, 2:46 p.m. UTC | #2 
> 	struct stmmac_priv *priv = netdev_priv(dev);
>
>-	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
>-	    priv->hw->pcs & STMMAC_PCS_SGMII) {
>+	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
[Suman] A personal preference here, I think the code will be more readable if we handle the !if condition @ the beginning. Something like,
if (priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS ||
	!(priv->hw->pcs & STMMAC_PCS_RGMII) || !(priv->hw->pcs & STMMAC_PCS_RGMII))
	return phylink_ethtool_ksettings_get(priv->phylink, cmd);

and keep the rest of the code without any check. But it is up-to you.

>+	    (priv->hw->pcs & STMMAC_PCS_RGMII ||
>+	     priv->hw->pcs & STMMAC_PCS_SGMII)) {
> 		struct rgmii_adv adv;
> 		u32 supported, advertising, lp_advertising;
>
>@@ -397,8 +398,9 @@ stmmac_ethtool_set_link_ksettings(struct net_device
>*dev,  {
> 	struct stmmac_priv *priv = netdev_priv(dev);
>
>-	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
>-	    priv->hw->pcs & STMMAC_PCS_SGMII) {
>+	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
>+	    (priv->hw->pcs & STMMAC_PCS_RGMII ||
>+	     priv->hw->pcs & STMMAC_PCS_SGMII)) {
> 		/* Only support ANE */
> 		if (cmd->base.autoneg != AUTONEG_ENABLE)
> 			return -EINVAL;
>--
>2.17.1
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index f628411ae4ae..e3ba4cd47b8d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -311,8 +311,9 @@  static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
-	    priv->hw->pcs & STMMAC_PCS_SGMII) {
+	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
+	    (priv->hw->pcs & STMMAC_PCS_RGMII ||
+	     priv->hw->pcs & STMMAC_PCS_SGMII)) {
 		struct rgmii_adv adv;
 		u32 supported, advertising, lp_advertising;
 
@@ -397,8 +398,9 @@  stmmac_ethtool_set_link_ksettings(struct net_device *dev,
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
-	    priv->hw->pcs & STMMAC_PCS_SGMII) {
+	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
+	    (priv->hw->pcs & STMMAC_PCS_RGMII ||
+	     priv->hw->pcs & STMMAC_PCS_SGMII)) {
 		/* Only support ANE */
 		if (cmd->base.autoneg != AUTONEG_ENABLE)
 			return -EINVAL;