| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8f94489d-5f0e-4166-a14e-4959098a5c80@quicinc.com>
Date: Thu, 21 Dec 2023 14:23:57 +0530
From: Sneh Shah <quic_snehshah@...cinc.com>
To: Andrew Halaney <ahalaney@...hat.com>
CC: Vinod Koul <vkoul@...nel.org>, Bhupesh Sharma <bhupesh.sharma@...aro.org>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu
<joabreu@...opsys.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni
<pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>, <netdev@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<kernel@...cinc.com>
Subject: Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for
2.5G SGMII
On 12/20/2023 9:29 PM, Andrew Halaney wrote:
> On Wed, Dec 20, 2023 at 01:02:45PM +0530, Sneh Shah wrote:
>>
>>
>> On 12/18/2023 9:50 PM, Andrew Halaney wrote:
>>> On Mon, Dec 18, 2023 at 12:41:18PM +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.
>>>>
>>>> Signed-off-by: Sneh Shah <quic_snehshah@...cinc.com>
>>>> ---
>>>> .../stmicro/stmmac/dwmac-qcom-ethqos.c | 31 +++++++++++++++++--
>>>> 1 file changed, 29 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>> index d3bf42d0fceb..b3a28dc19161 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>> @@ -21,6 +21,7 @@
>>>> #define RGMII_IO_MACRO_CONFIG2 0x1C
>>>> #define RGMII_IO_MACRO_DEBUG1 0x20
>>>> #define EMAC_SYSTEM_LOW_POWER_DEBUG 0x28
>>>> +#define ETHQOS_MAC_AN_CTRL 0xE0
>>>>
>>>> /* RGMII_IO_MACRO_CONFIG fields */
>>>> #define RGMII_CONFIG_FUNC_CLK_EN BIT(30)
>>>> @@ -78,6 +79,10 @@
>>>> #define ETHQOS_MAC_CTRL_SPEED_MODE BIT(14)
>>>> #define ETHQOS_MAC_CTRL_PORT_SEL BIT(15)
>>>>
>>>> +/*ETHQOS_MAC_AN_CTRL bits */
>>>> +#define ETHQOS_MAC_AN_CTRL_RAN BIT(9)
>>>> +#define ETHQOS_MAC_AN_CTRL_ANE BIT(12)
>>>> +
>>>
>>> nit: space please add a space before ETHQOS_MAC_AN_CTRL
>>>
>> will take care of this in next patch
>>
>>>> struct ethqos_emac_por {
>>>> unsigned int offset;
>>>> unsigned int value;
>>>> @@ -109,6 +114,7 @@ struct qcom_ethqos {
>>>> unsigned int num_por;
>>>> bool rgmii_config_loopback_en;
>>>> bool has_emac_ge_3;
>>>> + unsigned int serdes_speed;
>
> Another nit as I look closer: I think this should be grouped by phy_mode
> etc just for readability.
Didn't get this. can you please elaborate more?
>
>>>> };
>>>>
>>>> static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
>>>> @@ -600,27 +606,47 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos)
>>>>
>>>> static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
>>>> {
>>>> - int val;
>>>> -
>>>> + int val, mac_an_value;
>>>> val = readl(ethqos->mac_base + MAC_CTRL_REG);
>>>> + mac_an_value = readl(ethqos->mac_base + ETHQOS_MAC_AN_CTRL);
>>>>
>>>> switch (ethqos->speed) {
>>>> + case SPEED_2500:
>>>> + val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
>>>> + rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>>>> + RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>>>> + RGMII_IO_MACRO_CONFIG2);
>>>> + if (ethqos->serdes_speed != SPEED_2500)
>>>> + phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>>>> + mac_an_value &= ~ETHQOS_MAC_AN_CTRL_ANE;
>>>> + break;
>>>> case SPEED_1000:
>>>> val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
>>>> rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>>>> RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>>>> RGMII_IO_MACRO_CONFIG2);
>>>> + if (ethqos->serdes_speed != SPEED_1000)
>>>> + phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>>>> + mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
>>>> break;
>>>> case SPEED_100:
>>>> val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE;
>>>> + if (ethqos->serdes_speed != SPEED_1000)
>>>> + phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>>>> + mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
>>>> break;
>>>> case SPEED_10:
>>>> val |= ETHQOS_MAC_CTRL_PORT_SEL;
>>>> val &= ~ETHQOS_MAC_CTRL_SPEED_MODE;
>>>> + if (ethqos->serdes_speed != SPEED_1000)
>>>> + phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>>>> + mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
>>>> break;
>>>> }
>>>>
>>>> writel(val, ethqos->mac_base + MAC_CTRL_REG);
>>>> + writel(mac_an_value, ethqos->mac_base + ETHQOS_MAC_AN_CTRL);
>>>> + ethqos->serdes_speed = ethqos->speed;
>>>
>>> I see these bits are generic and there's some functions in stmmac_pcs.h
>>> that muck with these...
>>>
>>> Could you help me understand if this really should be Qualcomm specific,
>>> or if this is something that should be considered for the more core bits
>>> of the driver? I feel in either case we should take advantage of the
>>> common definitions in that file if possible.
>>>
>> we do have function dwmac_ctrl_ane in core driver which updates same registers. However, it does not have the option to reset ANE bit, it can only set bits. For SPEED_2500 we need to reset ANE bit. Hence I am adding it here. Not sure if we can extend dwmac_ctrl_ane function to reset bits as well.
>
> I'd evaluate if you can update that function to clear the ANE bit when
> the ane boolean is false. From the usage I see I feel that makes sense,
> but correct me if you think I'm wrong.
> At the very least let's use the defines from there, and possibly add a
> new function if clearing is not acceptable in dwmac_ctrl_ane().
>
> Stepping back, I was asking in general is the need to muck with ANE here
> is a Qualcomm specific problem, or is that a generic thing that should be
> handled in the core (and the phy_set_speed() bit stay here)? i.e. would
> any dwmac5 based IP need to do something like this for SPEED_2500?
I think disabling ANE for SPEED_2500 is generic not specific to qualcomm. Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500. Autoneg clause 37 stadard doesn't support 2500 speed. So we need to disable autoneg for speed 2500
>
>>>>
>>>> return val;
>>>> }
>>>> @@ -789,6 +815,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>> "Failed to get serdes phy\n");
>>>>
>>>> ethqos->speed = SPEED_1000;
>>>> + ethqos->serdes_speed = SPEED_1000;
>>>> ethqos_update_link_clk(ethqos, SPEED_1000);
>>>> ethqos_set_func_clk_en(ethqos);
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>
>>
>
Powered by blists - more mailing lists