2023-12-18 13:51:12

by Sneh Shah

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

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 <[email protected]>
---
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

2023-12-18 16:40:06

by Andrew Halaney

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

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 <[email protected]> # sa8775p-ride

Thanks for the patch!

>
> Signed-off-by: Sneh Shah <[email protected]>
> ---
> 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
>

2023-12-20 14:47:13

by Suman Ghosh

Subject: RE: [EXT] [PATCH net-next] net: stmmac: Fix ethool link settings ops for integrated PCS

> 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
>