| Message ID | E1uPkbO-004EyA-EU@rmk-PC.armlinux.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: stmmac: qcom-ethqos: add ethqos_pcs_set_inband() | expand |
On Thu, Jun 12, 2025 at 05:16:30PM +0100, Russell King (Oracle) wrote: > Add ethqos_pcs_set_inband() to improve readability, and to allow future > changes when phylink PCS support is properly merged. > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> # sa8775p-ride-r3 > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks Russell, The nit below notwithstanding this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org> > --- > .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > index e30bdf72331a..2e398574c7a7 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > @@ -622,6 +622,11 @@ static void ethqos_set_serdes_speed(struct qcom_ethqos *ethqos, int speed) > } > } > > +static void ethqos_pcs_set_inband(struct stmmac_priv *priv, bool enable) > +{ > + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, enable, 0, 0); FWIIW, I would have gone for the following, as all the type of three of the trailing parameters is bool. stmmac_pcs_ctrl_ane(priv, priv->ioaddr, enable, false, false); > +} > + > /* On interface toggle MAC registers gets reset. > * Configure MAC block for SGMII on ethernet phy link up > */ ...
On Sat, Jun 14, 2025 at 07:22:04PM +0100, Russell King (Oracle) wrote: > On Sat, Jun 14, 2025 at 04:35:12PM +0100, Simon Horman wrote: > > On Thu, Jun 12, 2025 at 05:16:30PM +0100, Russell King (Oracle) wrote: > > > Add ethqos_pcs_set_inband() to improve readability, and to allow future > > > changes when phylink PCS support is properly merged. > > > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> # sa8775p-ride-r3 > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > Thanks Russell, > > > > The nit below notwithstanding this looks good to me. > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > > > --- > > > .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 13 +++++++++---- > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > > > index e30bdf72331a..2e398574c7a7 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > > > @@ -622,6 +622,11 @@ static void ethqos_set_serdes_speed(struct qcom_ethqos *ethqos, int speed) > > > } > > > } > > > > > > +static void ethqos_pcs_set_inband(struct stmmac_priv *priv, bool enable) > > > +{ > > > + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, enable, 0, 0); > > > > FWIIW, I would have gone for the following, as all the type of > > three of the trailing parameters is bool. > > > > stmmac_pcs_ctrl_ane(priv, priv->ioaddr, enable, false, false); > > So the original code: > > stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 0, 0, 0); > stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); > stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); > stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); > > While one could convert the last two arguments to true/false, I'd prefer > leaving them as is, as less change means less chance to introduce a bug. Sure. No objections on my side.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c index e30bdf72331a..2e398574c7a7 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c @@ -622,6 +622,11 @@ static void ethqos_set_serdes_speed(struct qcom_ethqos *ethqos, int speed) } } +static void ethqos_pcs_set_inband(struct stmmac_priv *priv, bool enable) +{ + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, enable, 0, 0); +} + /* On interface toggle MAC registers gets reset. * Configure MAC block for SGMII on ethernet phy link up */ @@ -640,7 +645,7 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos, int speed) RGMII_CONFIG2_RGMII_CLK_SEL_CFG, RGMII_IO_MACRO_CONFIG2); ethqos_set_serdes_speed(ethqos, SPEED_2500); - stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 0, 0, 0); + ethqos_pcs_set_inband(priv, false); break; case SPEED_1000: val &= ~ETHQOS_MAC_CTRL_PORT_SEL; @@ -648,12 +653,12 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos, int speed) RGMII_CONFIG2_RGMII_CLK_SEL_CFG, RGMII_IO_MACRO_CONFIG2); ethqos_set_serdes_speed(ethqos, SPEED_1000); - stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); + ethqos_pcs_set_inband(priv, true); break; case SPEED_100: val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE; ethqos_set_serdes_speed(ethqos, SPEED_1000); - stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); + ethqos_pcs_set_inband(priv, true); break; case SPEED_10: val |= ETHQOS_MAC_CTRL_PORT_SEL; @@ -663,7 +668,7 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos, int speed) SGMII_10M_RX_CLK_DVDR), RGMII_IO_MACRO_CONFIG); ethqos_set_serdes_speed(ethqos, SPEED_1000); - stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); + ethqos_pcs_set_inband(priv, true); break; }