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@xxxxxxxxxxx>
> >> ---
> >> .../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.
> >> };
> >>
> >> 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?
> >>
> >> 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
> >>
> >
>
