From: Jakub Kicinski <kuba@kernel.org>
To: Sneh Shah <quic_snehshah@quicinc.com>
Cc: Vinod Koul <vkoul@kernel.org>,
Bhupesh Sharma <bhupesh.sharma@linaro.org>,
Alexandre Torgue <alexandre.torgue@foss.st.com>,
Jose Abreu <joabreu@synopsys.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>,
Maxime Coquelin <mcoquelin.stm32@gmail.com>,
netdev@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, kernel@quicinc.com,
Andrew Halaney <ahalaney@redhat.com>
Subject: Re: [PATCH net-next v4] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII
Date: Mon, 12 Feb 2024 17:30:15 -0800 [thread overview]
Message-ID: <20240212173015.0341f0ee@kernel.org> (raw)
In-Reply-To: <20240208111714.11456-1-quic_snehshah@quicinc.com>
On Thu, 8 Feb 2024 16:47:14 +0530 Sneh Shah wrote:
> Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000
> mode for 1G/100M/10M speed.
> Added changes to configure serdes phy and mac based on link speed.
> Changing serdes phy speed involves multiple register writes for
> serdes block. To avoid redundant write operations only update serdes
> phy when new speed is different.
Sounds like 2 separate changes in one patch, please split the
optimization of not writing the registers multiple times and
the 2.5G support.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 31631e3f89d0..6bbdbb7bef44 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -106,6 +106,7 @@ struct qcom_ethqos {
> struct clk *link_clk;
> struct phy *serdes_phy;
> unsigned int speed;
> + int serdes_speed;
Why signed if speed itself is unsigned?
> /* Enable and restart the Auto-Negotiation */
> if (ane)
> value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
> + else
> + value &= ~GMAC_AN_CTRL_ANE;
That looks unrelated. Either a separate patch or please explain in the
commit msg why.
--
pw-bot: cr
WARNING: multiple messages have this Message-ID (diff)
From: Jakub Kicinski <kuba@kernel.org>
To: Sneh Shah <quic_snehshah@quicinc.com>
Cc: Vinod Koul <vkoul@kernel.org>,
Bhupesh Sharma <bhupesh.sharma@linaro.org>,
Alexandre Torgue <alexandre.torgue@foss.st.com>,
Jose Abreu <joabreu@synopsys.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>,
Maxime Coquelin <mcoquelin.stm32@gmail.com>,
netdev@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, kernel@quicinc.com,
Andrew Halaney <ahalaney@redhat.com>
Subject: Re: [PATCH net-next v4] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII
Date: Mon, 12 Feb 2024 17:30:15 -0800 [thread overview]
Message-ID: <20240212173015.0341f0ee@kernel.org> (raw)
In-Reply-To: <20240208111714.11456-1-quic_snehshah@quicinc.com>
On Thu, 8 Feb 2024 16:47:14 +0530 Sneh Shah wrote:
> Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000
> mode for 1G/100M/10M speed.
> Added changes to configure serdes phy and mac based on link speed.
> Changing serdes phy speed involves multiple register writes for
> serdes block. To avoid redundant write operations only update serdes
> phy when new speed is different.
Sounds like 2 separate changes in one patch, please split the
optimization of not writing the registers multiple times and
the 2.5G support.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 31631e3f89d0..6bbdbb7bef44 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -106,6 +106,7 @@ struct qcom_ethqos {
> struct clk *link_clk;
> struct phy *serdes_phy;
> unsigned int speed;
> + int serdes_speed;
Why signed if speed itself is unsigned?
> /* Enable and restart the Auto-Negotiation */
> if (ane)
> value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
> + else
> + value &= ~GMAC_AN_CTRL_ANE;
That looks unrelated. Either a separate patch or please explain in the
commit msg why.
--
pw-bot: cr
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-02-13 1:30 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-08 11:17 [PATCH net-next v4] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII Sneh Shah
2024-02-08 11:17 ` Sneh Shah
2024-02-13 1:30 ` Jakub Kicinski [this message]
2024-02-13 1:30 ` Jakub Kicinski
2024-02-15 4:47 ` Sneh Shah
2024-02-15 4:47 ` Sneh Shah
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240212173015.0341f0ee@kernel.org \
--to=kuba@kernel.org \
--cc=ahalaney@redhat.com \
--cc=alexandre.torgue@foss.st.com \
--cc=bhupesh.sharma@linaro.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=joabreu@synopsys.com \
--cc=kernel@quicinc.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=quic_snehshah@quicinc.com \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.