net: ethernet: mtk_eth_soc: various enhancements
Subsystems (7)
Daniel Golle
maintainercover net: ethernet: mtk_eth_soc: various enhancements
This series brings a variety of fixes and enhancements for mtk_eth_soc, adds support for the MT7981 SoC and facilitates sharing the SGMII PCS code between mtk_eth_soc and mt7530.
Daniel Golle (9):
net: ethernet: mtk_eth_soc: add support for MT7981 SoC
net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
net: ethernet: mtk_eth_soc: reset PCS state
net: ethernet: mtk_eth_soc: only write values if needed
net: ethernet: mtk_eth_soc: fix RX data corruption issue
net: ethernet: mtk_eth_soc: ppe: add support for flow accounting
net: pcs: add driver for MediaTek SGMII PCS
net: ethernet: mtk_eth_soc: switch to external PCS driver
net: dsa: mt7530: use external PCS driver
MAINTAINERS | 7 + drivers/net/dsa/Kconfig | 2 + drivers/net/dsa/mt7530.c | 278 ++++------------ drivers/net/dsa/mt7530.h | 43 +-- drivers/net/ethernet/mediatek/Kconfig | 2 + drivers/net/ethernet/mediatek/mtk_eth_path.c | 14 +- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 69 +++- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 98 ++---- drivers/net/ethernet/mediatek/mtk_ppe.c | 110 +++++- drivers/net/ethernet/mediatek/mtk_ppe.h | 24 +- .../net/ethernet/mediatek/mtk_ppe_debugfs.c | 9 +- .../net/ethernet/mediatek/mtk_ppe_offload.c | 7 + drivers/net/ethernet/mediatek/mtk_ppe_regs.h | 14 + drivers/net/ethernet/mediatek/mtk_sgmii.c | 190 ++--------- drivers/net/pcs/Kconfig | 6 + drivers/net/pcs/Makefile | 1 + drivers/net/pcs/pcs-mtk.c | 314 ++++++++++++++++++ include/linux/pcs/pcs-mtk.h | 13 + 18 files changed, 707 insertions(+), 494 deletions(-) create mode 100644 drivers/net/pcs/pcs-mtk.c create mode 100644 include/linux/pcs/pcs-mtk.h
base-commit: 4fafd96910add124586b549ad005dcd179de8a18
Signature · 1 line
2.39.1
Daniel Golle
maintainerPATCH 1/9 net: ethernet: mtk_eth_soc: add support for MT7981 SoC
The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.
In addition MT7981 comes with a built-in 1000Base-T PHY which can be used with GMAC1.
As many MT7981 boards make use of swapping SGMII phase and neutral, add new device-tree attribute 'mediatek,pn_swap' to support them.
--- drivers/net/ethernet/mediatek/mtk_eth_path.c | 14 +++++++-- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 21 +++++++++++++ drivers/net/ethernet/mediatek/mtk_eth_soc.h | 31 ++++++++++++++++++++ drivers/net/ethernet/mediatek/mtk_sgmii.c | 10 +++++++ 4 files changed, 73 insertions(+), 3 deletions(-)
drivers/net/ethernet/mediatek/mtk_eth_path.c +11 -3
@@ -96,12 +96,20 @@ static int set_mux_gmac2_gmac0_to_gephy(struct mtk_eth *eth, int path)static int set_mux_u3_gmac2_to_qphy (struct mtk_eth *eth,int path) { -unsigned int val =0 ; +unsigned int val =0 , mask =0 , reg =0 ;bool updated = true;switch (path) {case MTK_ETH_PATH_GMAC2_SGMII : - val =CO_QPHY_SEL ; +if (MTK_HAS_CAPS (eth->soc->caps,MTK_U3_COPHY_V2 )) { + reg =USB_PHY_SWITCH_REG ; + val =SGMII_QPHY_SEL ; + mask =QPHY_SEL_MASK ; + }else { + reg =INFRA_MISC2 ; + val =CO_QPHY_SEL ; + mask = val; + }break ;default : updated = false;
@@ -109,7 +117,7 @@ static int set_mux_u3_gmac2_to_qphy(struct mtk_eth *eth, int path) }if (updated) -regmap_update_bits (eth->infra,INFRA_MISC2 ,CO_QPHY_SEL , val); +regmap_update_bits (eth->infra, reg, mask, val);dev_dbg (eth->dev,"path %s in %s updated = %d\n" ,mtk_eth_path_name (path), __func__, updated);
drivers/net/ethernet/mediatek/mtk_eth_soc.c +21 -0
@@ -4852,6 +4852,26 @@ static const struct mtk_soc_data mt7629_data = { }, }; +static const struct mtk_soc_data mt7981_data = { + .reg_map = &mt7986_reg_map, + .ana_rgc3 =0x128 , + .caps =MT7981_CAPS , + .hw_features =MTK_HW_FEATURES , + .required_clks =MT7981_CLKS_BITMAP , + .required_pctl = false, + .offload_version =2 , + .hash_offset =4 , + .foe_entry_size =sizeof (struct mtk_foe_entry ), + .txrx = { + .txd_size =sizeof (struct mtk_tx_dma_v2 ), + .rxd_size =sizeof (struct mtk_rx_dma_v2 ), + .rx_irq_done_mask =MTK_RX_DONE_INT_V2 , + .rx_dma_l4_valid =RX_DMA_L4_VALID_V2 , + .dma_max_len =MTK_TX_DMA_BUF_LEN_V2 , + .dma_len_offset =8 , + }, +}; +static const struct mtk_soc_data mt7986_data = { .reg_map = &mt7986_reg_map, .ana_rgc3 =0x128 ,
@@ -4894,6 +4914,7 @@ const struct of_device_id of_mtk_match[] = { { .compatible = "mediatek,mt7622-eth", .data = &mt7622_data}, { .compatible = "mediatek,mt7623-eth", .data = &mt7623_data}, { .compatible = "mediatek,mt7629-eth", .data = &mt7629_data}, + { .compatible = "mediatek,mt7981-eth", .data = &mt7981_data}, { .compatible = "mediatek,mt7986-eth", .data = &mt7986_data}, { .compatible = "ralink,rt5350-eth", .data = &rt5350_data}, {},
drivers/net/ethernet/mediatek/mtk_eth_soc.h +31 -0
@@ -551,11 +551,22 @@#define SGMSYS_QPHY_PWR_STATE_CTRL 0xe8#define SGMII_PHYA_PWD BIT(4) +/* Register to QPHY wrapper control */ +#define SGMSYS_QPHY_WRAP_CTRL 0xec +#define SGMII_PN_SWAP_MASK GENMASK(1, 0) +#define SGMII_PN_SWAP_TX_RX (BIT(0) | BIT(1)) +#define MTK_SGMII_FLAG_PN_SWAP BIT(0) +/* Infrasys subsystem config registers */ #define INFRA_MISC2 0x70c#define CO_QPHY_SEL BIT(0)#define GEPHY_MAC_SEL BIT(1) +/* Top misc registers */ +#define USB_PHY_SWITCH_REG 0x218 +#define QPHY_SEL_MASK GENMASK(1, 0) +#define SGMII_QPHY_SEL 0x2 +/* MT7628/88 specific stuff */ #define MT7628_PDMA_OFFSET 0x0800#define MT7628_SDM_OFFSET 0x0c00
@@ -736,6 +747,17 @@ enum mtk_clks_map { BIT(MTK_CLK_SGMII2_CDR_FB) | \ BIT(MTK_CLK_SGMII_CK) | \ BIT(MTK_CLK_ETH2PLL) | BIT(MTK_CLK_SGMIITOP)) +#define MT7981_CLKS_BITMAP (BIT(MTK_CLK_FE) | BIT(MTK_CLK_GP2) | BIT(MTK_CLK_GP1) | \ + BIT(MTK_CLK_WOCPU0) | \ + BIT(MTK_CLK_SGMII_TX_250M) | \ + BIT(MTK_CLK_SGMII_RX_250M) | \ + BIT(MTK_CLK_SGMII_CDR_REF) | \ + BIT(MTK_CLK_SGMII_CDR_FB) | \ + BIT(MTK_CLK_SGMII2_TX_250M) | \ + BIT(MTK_CLK_SGMII2_RX_250M) | \ + BIT(MTK_CLK_SGMII2_CDR_REF) | \ + BIT(MTK_CLK_SGMII2_CDR_FB) | \ + BIT(MTK_CLK_SGMII_CK))#define MT7986_CLKS_BITMAP (BIT(MTK_CLK_FE) | BIT(MTK_CLK_GP2) | BIT(MTK_CLK_GP1) | \ BIT(MTK_CLK_WOCPU1) | BIT(MTK_CLK_WOCPU0) | \ BIT(MTK_CLK_SGMII_TX_250M) | \
@@ -849,6 +871,7 @@ enum mkt_eth_capabilities {MTK_NETSYS_V2_BIT ,MTK_SOC_MT7628_BIT ,MTK_RSTCTRL_PPE1_BIT , +MTK_U3_COPHY_V2_BIT ,/* MUX BITS*/ MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT,
@@ -883,6 +906,7 @@ enum mkt_eth_capabilities {#define MTK_NETSYS_V2 BIT(MTK_NETSYS_V2_BIT)#define MTK_SOC_MT7628 BIT(MTK_SOC_MT7628_BIT)#define MTK_RSTCTRL_PPE1 BIT(MTK_RSTCTRL_PPE1_BIT) +#define MTK_U3_COPHY_V2 BIT(MTK_U3_COPHY_V2_BIT)#define MTK_ETH_MUX_GDM1_TO_GMAC1_ESW \ BIT(MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT)
@@ -955,6 +979,11 @@ enum mkt_eth_capabilities { MTK_MUX_U3_GMAC2_TO_QPHY | \ MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA) +#define MT7981_CAPS (MTK_GMAC1_SGMII | MTK_GMAC2_SGMII | MTK_GMAC2_GEPHY | \ + MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA | \ + MTK_MUX_U3_GMAC2_TO_QPHY | MTK_U3_COPHY_V2 | \ + MTK_NETSYS_V2 | MTK_RSTCTRL_PPE1) +#define MT7986_CAPS (MTK_GMAC1_SGMII | MTK_GMAC2_SGMII | \ MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA | \ MTK_NETSYS_V2 | MTK_RSTCTRL_PPE1)
@@ -1068,12 +1097,14 @@ struct mtk_soc_data {* @ana_rgc3: The offset refers to register ANA_RGC3 related to regmap * @interface: Currently configured interface mode * @pcs: Phylink PCS structure +* @flags: Flags indicating hardware properties */ struct mtk_pcs {struct regmap *regmap;u32 ana_rgc3;phy_interface_t interface;struct phylink_pcs pcs; +u32 flags; };/* struct mtk_sgmii - This is the structure holding sgmii regmap and its
drivers/net/ethernet/mediatek/mtk_sgmii.c +10 -0
@@ -88,6 +88,11 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,regmap_update_bits (mpcs->regmap,SGMSYS_QPHY_PWR_STATE_CTRL ,SGMII_PHYA_PWD ,SGMII_PHYA_PWD ); +if (mpcs->flags &MTK_SGMII_FLAG_PN_SWAP ) +regmap_update_bits (mpcs->regmap,SGMSYS_QPHY_WRAP_CTRL , +SGMII_PN_SWAP_MASK , +SGMII_PN_SWAP_TX_RX ); +if (interface ==PHY_INTERFACE_MODE_2500BASEX ) rgc3 =RG_PHY_SPEED_3_125G ;else
@@ -182,6 +187,11 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3) ss->pcs[i].ana_rgc3 = ana_rgc3; ss->pcs[i].regmap =syscon_node_to_regmap (np); + + ss->pcs[i].flags =0 ; +if (of_property_read_bool (np,"mediatek,pn_swap" )) + ss->pcs[i].flags |=MTK_SGMII_FLAG_PN_SWAP ; +of_node_put (np);if (IS_ERR (ss->pcs[i].regmap))return PTR_ERR (ss->pcs[i].regmap);
Signature · 1 line
2.39.1
Andrew Lunn
maintainerRe: [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC
Quoted 7 lines · Daniel Golle
The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.
In addition MT7981 comes with a built-in 1000Base-T PHY which can be used with GMAC1.
As many MT7981 boards make use of swapping SGMII phase and neutral, add new device-tree attribute 'mediatek,pn_swap' to support them.
Is this new property documented in the device tree binding?
Andrew
Vladimir Oltean
maintainerRe: [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC
In reply to Daniel GolleQuoted 10 lines · Daniel Golle
The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.
In addition MT7981 comes with a built-in 1000Base-T PHY which can be used with GMAC1.
As many MT7981 boards make use of swapping SGMII phase and neutral, add new device-tree attribute 'mediatek,pn_swap' to support them.
Signed-off-by Daniel Golle <[email protected]>---
Phase and neutral? What is this, a power plug?
Since SGMII uses differential signaling, I wonder if this isn't about the polarity of the TX lane (which pin carries the Positive signal and which the Negative one).
I think there is room for a more general device tree property name than "mediatek,pn_swap". The Designware XPCS also supports this, see DW_VR_MII_DIG_CTRL2_TX_POL_INV and the comments in nxp_sja1105_sgmii_pma_config().
Vladimir Oltean
maintainerRe: [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC
In reply to Daniel GolleQuoted 1 line · Daniel Golle
The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.
comes /with/ two 1G/2.5G SGMII /ports/
Daniel Golle
maintainerPATCH 2/9 net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
In reply to Daniel GolleSet MDIO bus clock frequency and allow setting a custom maximum frequency from device tree.
--- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 +++++++++++++++++++++ drivers/net/ethernet/mediatek/mtk_eth_soc.h | 5 +++++ 2 files changed, 30 insertions(+)
drivers/net/ethernet/mediatek/mtk_eth_soc.c +25 -0
@@ -790,7 +790,9 @@ static const struct phylink_mac_ops mtk_phylink_ops = {static int mtk_mdio_init (struct mtk_eth *eth) {struct device_node *mii_np; +int clk =25000000 , max_clk =2500000 , divider =1 ;int ret; +u32 val; mii_np =of_get_child_by_name (eth->dev->of_node,"mdio-bus" );if (!mii_np) {
@@ -818,6 +820,29 @@ static int mtk_mdio_init(struct mtk_eth *eth) eth->mii_bus->parent = eth->dev;snprintf (eth->mii_bus->id,MII_BUS_ID_SIZE ,"%pOFn" , mii_np); + +if (!of_property_read_u32 (mii_np,"clock-frequency" , &val)) + max_clk = val; + +while (clk / divider > max_clk) { +if (divider >=63 ) +break ; + + divider++; + }; + + val =mtk_r32 (eth,MTK_PPSC ); + val |=PPSC_MDC_TURBO ; +mtk_w32 (eth, val,MTK_PPSC ); + +/* Configure MDC Divider */ + val =mtk_r32 (eth,MTK_PPSC ); + val &= ~PPSC_MDC_CFG ; + val |=FIELD_PREP (PPSC_MDC_CFG , divider); +mtk_w32 (eth, val,MTK_PPSC ); + +dev_dbg (eth->dev,"MDC is running on %d Hz\n" , clk / divider); + ret =of_mdiobus_register (eth->mii_bus, mii_np); err_put_node:
drivers/net/ethernet/mediatek/mtk_eth_soc.h +5 -0
@@ -363,6 +363,11 @@#define RX_DMA_VTAG_V2 BIT(0)#define RX_DMA_L4_VALID_V2 BIT(2) +/* PHY Polling and SMI Master Control registers */ +#define MTK_PPSC 0x10000 +#define PPSC_MDC_CFG GENMASK(29, 24) +#define PPSC_MDC_TURBO BIT(20) +/* PHY Indirect Access Control registers */ #define MTK_PHY_IAC 0x10004#define PHY_IAC_ACCESS BIT(31)
Signature · 1 line
2.39.1
Andrew Lunn
maintainerRe: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
drivers/net/ethernet/mediatek/mtk_eth_soc.c static const struct phylink_mac_ops mtk_phylink_ops = { · line 795
Quoted 6 lines
static int
mtk_mdio_init(struct mtk_eth *eth)
{drivers/net/ethernet/mediatek/mtk_eth_soc.c +2
-0struct device_node *mii_np; +int clk =25000000 , max_clk =2500000 , divider =1 ;int ret; +u32 val;
Reverse Christmas tree please.
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_mdio_init(struct mtk_eth *eth) · line 832
Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10
-0+ +if (!of_property_read_u32 (mii_np,"clock-frequency" , &val)) + max_clk = val; + +while (clk / divider > max_clk) { +if (divider >=63 ) +break ; + + divider++; + };
Please add some range checks here. Return -EINVAL if val > max_clock. Also, if divider = 63 indicating the requested clock is too slow.
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_mdio_init(struct mtk_eth *eth) · line 842
Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10
-0+ + val =mtk_r32 (eth,MTK_PPSC ); + val |=PPSC_MDC_TURBO ; +mtk_w32 (eth, val,MTK_PPSC ); + +/* Configure MDC Divider */ + val =mtk_r32 (eth,MTK_PPSC ); + val &= ~PPSC_MDC_CFG ; + val |=FIELD_PREP (PPSC_MDC_CFG , divider); +mtk_w32 (eth, val,MTK_PPSC );
Can these two writes to MTK_PPSC be combined into one?
val |= FIELD_PREP(PPSC_MDC_CFG, divider) | PPSC_MDC_TURBO;
Andrew
Vladimir Oltean
maintainerRe: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
In reply to Daniel GolleQuoted 14 lines · Daniel Golle
Set MDIO bus clock frequency and allow setting a custom maximum frequency from device tree.
Signed-off-by Daniel Golle <[email protected]>--- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 +++++++++++++++++++++ drivers/net/ethernet/mediatek/mtk_eth_soc.h | 5 +++++ 2 files changed, 30 insertions(+)drivers/net/ethernet/mediatek/mtk_eth_soc.c +0
-0
drivers/net/ethernet/mediatek/mtk_eth_soc.c static const struct phylink_mac_ops mtk_phylink_ops = { · line 793
Quoted 4 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +1
-0@@ -790,7 +790,9 @@ static const struct phylink_mac_ops mtk_phylink_ops = {static int mtk_mdio_init (struct mtk_eth *eth) {struct device_node *mii_np; +int clk =25000000 , max_clk =2500000 , divider =1 ;
Would be good if constant values (clk) weren't put in variables.
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_mdio_init(struct mtk_eth *eth) · line 825
Quoted 12 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +4
-0int ret; +u32 val; mii_np =of_get_child_by_name (eth->dev->of_node,"mdio-bus" );if (!mii_np) {@@ -818,6 +820,29 @@ static int mtk_mdio_init(struct mtk_eth *eth) eth->mii_bus->parent = eth->dev;snprintf (eth->mii_bus->id,MII_BUS_ID_SIZE ,"%pOFn" , mii_np); + +if (!of_property_read_u32 (mii_np,"clock-frequency" , &val)) + max_clk = val;
Checking for valid range? There should also probably be a dt-bindings patch for this.
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_mdio_init(struct mtk_eth *eth) · line 832
Quoted 7 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +7
-0+ +while (clk / divider > max_clk) { +if (divider >=63 ) +break ; + + divider++; + };
uhm, "divider = min(DIV_ROUND_UP(25000000, max_clk), 63);"? I don't think the compiler is smart enough to optimize away this loop.
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_mdio_init(struct mtk_eth *eth) · line 836
Quoted 4 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +4
-0+ + val =mtk_r32 (eth,MTK_PPSC ); + val |=PPSC_MDC_TURBO ; +mtk_w32 (eth, val,MTK_PPSC );
What does "TURBO" do and why do you set it unconditionally?
Quoted 31 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +9
-0+ +/* Configure MDC Divider */ + val =mtk_r32 (eth,MTK_PPSC ); + val &= ~PPSC_MDC_CFG ; + val |=FIELD_PREP (PPSC_MDC_CFG , divider); +mtk_w32 (eth, val,MTK_PPSC ); + +dev_dbg (eth->dev,"MDC is running on %d Hz\n" , clk / divider); + ret =of_mdiobus_register (eth->mii_bus, mii_np); err_put_node:drivers/net/ethernet/mediatek/mtk_eth_soc.h +5
-0@@ -363,6 +363,11 @@#define RX_DMA_VTAG_V2 BIT(0)#define RX_DMA_L4_VALID_V2 BIT(2) +/* PHY Polling and SMI Master Control registers */ +#define MTK_PPSC 0x10000 +#define PPSC_MDC_CFG GENMASK(29, 24) +#define PPSC_MDC_TURBO BIT(20) +/* PHY Indirect Access Control registers */ #define MTK_PHY_IAC 0x10004#define PHY_IAC_ACCESS BIT(31)Signature · 1 line
2.39.1
Andrew Lunn
maintainerRe: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
Quoted 2 lines
Checking for valid range? There should also probably be a dt-bindings patch for this.
This is a common mdio property in mdio.yaml. So there should not be any need for a driver specific dt-binding.
Andrew
Vladimir Oltean
maintainerRe: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
Quoted 5 lines · Andrew Lunn
Quoted 2 lines
Checking for valid range? There should also probably be a dt-bindings patch for this.
This is a common mdio property in
mdio.yaml. So there should not be any need for a driver specific dt-binding.
I meant it would probably be informative if the dt schema for this MDIO controller had a "default: $val", "minimum: $val" and "maximum: $val".
Daniel Golle
maintainerPATCH 3/9 net: ethernet: mtk_eth_soc: reset PCS state
In reply to Daniel GolleReset PCS state when changing interface mode.
--- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 ++++ drivers/net/ethernet/mediatek/mtk_sgmii.c | 4 ++++ 2 files changed, 8 insertions(+)
drivers/net/ethernet/mediatek/mtk_eth_soc.h +4 -0
@@ -546,6 +546,10 @@#define SGMII_SEND_AN_ERROR_EN BIT(11)#define SGMII_IF_MODE_MASK GENMASK(5, 1) +/* Register to reset SGMII design */ +#define SGMII_RESERVED_0 0x34 +#define SGMII_SW_RESET BIT(0) +/* Register to set SGMII speed, ANA RG_ Control Signals III*/ #define SGMSYS_ANA_RG_CS3 0x2028#define RG_PHY_SPEED_MASK (BIT(2) | BIT(3))
drivers/net/ethernet/mediatek/mtk_sgmii.c +4 -0
@@ -88,6 +88,10 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,regmap_update_bits (mpcs->regmap,SGMSYS_QPHY_PWR_STATE_CTRL ,SGMII_PHYA_PWD ,SGMII_PHYA_PWD ); +/* Reset SGMII PCS state */ +regmap_update_bits (mpcs->regmap,SGMII_RESERVED_0 , +SGMII_SW_RESET ,SGMII_SW_RESET ); +if (mpcs->flags &MTK_SGMII_FLAG_PN_SWAP ) regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL, SGMII_PN_SWAP_MASK,
Signature · 1 line
2.39.1
Daniel Golle
maintainerPATCH 4/9 net: ethernet: mtk_eth_soc: only write values if needed
In reply to Daniel GolleOnly restart auto-negotiation and write link timer if actually necessary.
--- drivers/net/ethernet/mediatek/mtk_sgmii.c | 24 +++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/net/ethernet/mediatek/mtk_sgmii.c +12 -12
@@ -41,17 +41,13 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,struct mtk_pcs *mpcs =pcs_to_mtk_pcs (pcs);unsigned int rgc3, sgm_mode, bmcr;int advertise, link_timer; -bool changed, use_an; +bool mode_changed = false, changed, use_an; advertise =phylink_mii_c22_pcs_encode_advertisement (interface, advertising);if (advertise <0 )return advertise; - link_timer =phylink_get_link_timer_ns (interface); -if (link_timer <0 ) -return link_timer; -/* Clearing IF_MODE_BIT0 switches the PCS to BASE-X mode, and * we assume that fixes it's speed at bitrate = line rate (in * other words, 1000Mbps or 2500Mbps).
@@ -77,8 +73,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, }if (use_an) { -/* FIXME: Do we need to set AN_RESTART here? */ - bmcr =SGMII_AN_RESTART |SGMII_AN_ENABLE ; + bmcr =SGMII_AN_ENABLE ; }else { bmcr =0 ; }
@@ -106,16 +101,21 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,regmap_update_bits (mpcs->regmap, mpcs->ana_rgc3,RG_PHY_SPEED_3_125G , rgc3); +/* Setup the link timer and QPHY power up inside SGMIISYS */ + link_timer =phylink_get_link_timer_ns (interface); +if (link_timer <0 ) +return link_timer; + +regmap_write (mpcs->regmap,SGMSYS_PCS_LINK_TIMER , link_timer /2 /8 ); + mpcs->interface = interface; + mode_changed = true; }/* Update the advertisement, noting whether it has changed */ regmap_update_bits_check (mpcs->regmap,SGMSYS_PCS_ADVERTISE ,SGMII_ADVERTISE , advertise, &changed); -/* Setup the link timer and QPHY power up inside SGMIISYS */ -regmap_write (mpcs->regmap,SGMSYS_PCS_LINK_TIMER , link_timer /2 /8 ); -/* Update the sgmsys mode register */ regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE, SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
@@ -123,7 +123,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,/* Update the BMCR */ regmap_update_bits (mpcs->regmap,SGMSYS_PCS_CONTROL_1 , -SGMII_AN_RESTART |SGMII_AN_ENABLE , bmcr); +SGMII_AN_ENABLE , bmcr);/* Release PHYA power down state * Only removing bit SGMII_PHYA_PWD isn't enough.
@@ -137,7 +137,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,usleep_range (50 ,100 );regmap_write (mpcs->regmap,SGMSYS_QPHY_PWR_STATE_CTRL ,0 ); -return changed; +return changed || mode_changed; }static void mtk_pcs_restart_an (struct phylink_pcs *pcs)
Signature · 1 line
2.39.1
Andrew Lunn
maintainerRe: [PATCH 4/9] net: ethernet: mtk_eth_soc: only write values if needed
Quoted 13 lines · Daniel Golle
Only restart auto-negotiation and write link timer if actually necessary.
Signed-off-by Daniel Golle <[email protected]>--- drivers/net/ethernet/mediatek/mtk_sgmii.c | 24 +++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)drivers/net/ethernet/mediatek/mtk_sgmii.c +0
-0
drivers/net/ethernet/mediatek/mtk_sgmii.c static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, · line 44
Quoted 5 lines
drivers/net/ethernet/mediatek/mtk_sgmii.c +1
-1@@ -41,17 +41,13 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,struct mtk_pcs *mpcs =pcs_to_mtk_pcs (pcs);unsigned int rgc3, sgm_mode, bmcr;int advertise, link_timer; -bool changed, use_an; +bool mode_changed = false, changed, use_an;
Reverse Christmas tree .... Please check all the patches.
Andrew
Daniel Golle
maintainerPATCH 5/9 net: ethernet: mtk_eth_soc: fix RX data corruption issue
In reply to Daniel GolleAlso set bit 12 when setting up MAC MCR, as MediaTek SDK did the same change stating:
"If without this patch, kernel might receive invalid packets that are corrupted by GMAC."[1]
--- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mediatek/mtk_eth_soc.c +1 -1
@@ -615,7 +615,7 @@ static int mtk_mac_finish(struct phylink_config *config, unsigned int mode,/* Setup gmac */ mcr_cur =mtk_r32 (mac->hw,MTK_MAC_MCR (mac->id)); mcr_new = mcr_cur; - mcr_new |=MAC_MCR_IPG_CFG |MAC_MCR_FORCE_MODE | + mcr_new |=MAC_MCR_IPG_CFG |MAC_MCR_BIT_12 |MAC_MCR_FORCE_MODE |MAC_MCR_BACKOFF_EN |MAC_MCR_BACKPR_EN |MAC_MCR_FORCE_LINK ;/* Only update control register when needed! */
drivers/net/ethernet/mediatek/mtk_eth_soc.h +1 -0
@@ -402,6 +402,7 @@#define MAC_MCR_FORCE_MODE BIT(15)#define MAC_MCR_TX_EN BIT(14)#define MAC_MCR_RX_EN BIT(13) +#define MAC_MCR_BIT_12 BIT(12)#define MAC_MCR_BACKOFF_EN BIT(9)#define MAC_MCR_BACKPR_EN BIT(8)#define MAC_MCR_FORCE_RX_FC BIT(5)
Signature · 1 line
2.39.1
Andrew Lunn
maintainerRe: [PATCH 5/9] net: ethernet: mtk_eth_soc: fix RX data corruption issue
Quoted 4 lines · Daniel Golle
Also set bit 12 when setting up MAC MCR, as MediaTek SDK did the same change stating:
"If without this patch, kernel might receive invalid packets that are corrupted by GMAC."[1]
Do you have any idea what this bit means? It would be nice to have a more meaningful description.
Andrew
Daniel Golle
maintainerPATCH 6/9 net: ethernet: mtk_eth_soc: ppe: add support for flow accounting
In reply to Daniel GolleThe PPE units found in MT7622 and newer support packet and byte accounting of hw-offloaded flows. Add support for reading those counters as found in MediaTek's SDK[1], make them accessible via debugfs and add them to the flow offload stats.
--- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 8 +- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 1 + drivers/net/ethernet/mediatek/mtk_ppe.c | 110 +++++++++++++++++- drivers/net/ethernet/mediatek/mtk_ppe.h | 24 +++- .../net/ethernet/mediatek/mtk_ppe_debugfs.c | 9 +- .../net/ethernet/mediatek/mtk_ppe_offload.c | 7 ++ drivers/net/ethernet/mediatek/mtk_ppe_regs.h | 14 +++ 7 files changed, 168 insertions(+), 5 deletions(-)
drivers/net/ethernet/mediatek/mtk_eth_soc.c +7 -1
@@ -4708,7 +4708,9 @@ static int mtk_probe(struct platform_device *pdev)u32 ppe_addr = eth->soc->reg_map->ppe_base + i *0x400 ; eth->ppe[i] =mtk_ppe_init (eth, eth->base + ppe_addr, - eth->soc->offload_version, i); + eth->soc->offload_version, i, + eth->soc->has_accounting); +if (!eth->ppe[i]) { err = -ENOMEM ; goto err_deinit_ppe;
@@ -4830,6 +4832,7 @@ static const struct mtk_soc_data mt7622_data = { .required_pctl = false, .offload_version = 2, .hash_offset = 2, + .has_accounting = true, .foe_entry_size = sizeof(struct mtk_foe_entry) - 16, .txrx = { .txd_size = sizeof(struct mtk_tx_dma),
@@ -4867,6 +4870,7 @@ static const struct mtk_soc_data mt7629_data = { .hw_features = MTK_HW_FEATURES, .required_clks = MT7629_CLKS_BITMAP, .required_pctl = false, + .has_accounting = true, .txrx = { .txd_size = sizeof(struct mtk_tx_dma), .rxd_size = sizeof(struct mtk_rx_dma),
@@ -4887,6 +4891,7 @@ static const struct mtk_soc_data mt7981_data = { .offload_version = 2, .hash_offset = 4, .foe_entry_size = sizeof(struct mtk_foe_entry), + .has_accounting = true, .txrx = { .txd_size = sizeof(struct mtk_tx_dma_v2), .rxd_size = sizeof(struct mtk_rx_dma_v2),
@@ -4907,6 +4912,7 @@ static const struct mtk_soc_data mt7986_data = { .offload_version = 2, .hash_offset = 4, .foe_entry_size = sizeof(struct mtk_foe_entry), + .has_accounting = true, .txrx = { .txd_size = sizeof(struct mtk_tx_dma_v2), .rxd_size = sizeof(struct mtk_rx_dma_v2),
drivers/net/ethernet/mediatek/mtk_eth_soc.h +1 -0
@@ -1085,6 +1085,7 @@ struct mtk_soc_data {u8 hash_offset;u16 foe_entry_size;netdev_features_t hw_features; +bool has_accounting;struct {u32 txd_size;u32 rxd_size;
drivers/net/ethernet/mediatek/mtk_ppe.c +109 -1
159 lines not on this page · raw message
drivers/net/ethernet/mediatek/mtk_ppe.h +23 -1
@@ -57,6 +57,7 @@ enum {#define MTK_FOE_IB2_MULTICAST BIT(8)#define MTK_FOE_IB2_WDMA_QID2 GENMASK(13, 12) +#define MTK_FOE_IB2_MIB_CNT BIT(15)#define MTK_FOE_IB2_WDMA_DEVIDX BIT(16)#define MTK_FOE_IB2_WDMA_WINFO BIT(17)
@@ -285,16 +286,34 @@ struct mtk_flow_entry {unsigned long cookie; }; +struct mtk_mib_entry { +u32 byt_cnt_l; +u16 byt_cnt_h; +u32 pkt_cnt_l; +u8 pkt_cnt_h; +u8 _rsv0; +u32 _rsv1; +} __packed; + +struct mtk_foe_accounting { +u64 bytes; +u64 packets; +}; +struct mtk_ppe {struct mtk_eth *eth;struct device *dev;void __iomem *base;int version;char dirname[5 ]; +bool accounting;void *foe_table;dma_addr_t foe_phys; +struct mtk_mib_entry *mib_table; +dma_addr_t mib_phys; +u16 foe_check_time[MTK_PPE_ENTRIES ];struct hlist_head *foe_flow;
@@ -304,7 +323,8 @@ struct mtk_ppe { };struct mtk_ppe *mtk_ppe_init (struct mtk_eth *eth,void __iomem *base, -int version,int index); +int version,int index,bool accounting); +void mtk_ppe_deinit (struct mtk_eth *eth);void mtk_ppe_start (struct mtk_ppe *ppe);int mtk_ppe_stop (struct mtk_ppe *ppe);
@@ -359,5 +379,7 @@ int mtk_foe_entry_commit(struct mtk_ppe *ppe, struct mtk_flow_entry *entry);void mtk_foe_entry_clear (struct mtk_ppe *ppe,struct mtk_flow_entry *entry);int mtk_foe_entry_idle_time (struct mtk_ppe *ppe,struct mtk_flow_entry *entry);int mtk_ppe_debugfs_init (struct mtk_ppe *ppe,int index); +struct mtk_foe_accounting *mtk_foe_entry_get_mib (struct mtk_ppe *ppe,u32 index, +struct mtk_foe_accounting *diff);#endif
drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c +7 -2
@@ -82,6 +82,7 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)struct mtk_foe_entry *entry =mtk_foe_get_entry (ppe, i);struct mtk_foe_mac_info *l2;struct mtk_flow_addr_info ai = {}; +struct mtk_foe_accounting *acct;unsigned char h_source[ETH_ALEN ];unsigned char h_dest[ETH_ALEN ];int type, state;
@@ -95,6 +96,8 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)if (bind && state !=MTK_FOE_STATE_BIND )continue ; + acct =mtk_foe_entry_get_mib (ppe, i,NULL ); + type =FIELD_GET (MTK_FOE_IB1_PACKET_TYPE , entry->ib1); seq_printf(m, "%05x %s %7s", i, mtk_foe_entry_state_str(state),
@@ -153,9 +156,11 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind) *((__be16 *)&h_dest[4 ]) =htons (l2->dest_mac_lo);seq_printf (m," eth=%pM->%pM etype=%04x" -" vlan=%d,%d ib1=%08x ib2=%08x\n" , +" vlan=%d,%d ib1=%08x ib2=%08x" +" packets=%lld bytes=%lld\n" , h_source, h_dest,ntohs (l2->etype), - l2->vlan1, l2->vlan2, entry->ib1, ib2); + l2->vlan1, l2->vlan2, entry->ib1, ib2, + acct->packets, acct->bytes); }return 0 ;
drivers/net/ethernet/mediatek/mtk_ppe_offload.c +7 -0
@@ -497,6 +497,7 @@ static int mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f) { struct mtk_flow_entry *entry; +struct mtk_foe_accounting diff;u32 idle; entry = rhashtable_lookup(ð->flow_table, &f->cookie,
@@ -507,6 +508,12 @@ mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f) idle =mtk_foe_entry_idle_time (eth->ppe[entry->ppe_index], entry); f->stats.lastused = jiffies - idle *HZ ; +if (entry->hash !=0xFFFF ) { +mtk_foe_entry_get_mib (eth->ppe[entry->ppe_index], entry->hash, &diff); + f->stats.pkts += diff.packets; + f->stats.bytes += diff.bytes; + } +return 0 ; }
drivers/net/ethernet/mediatek/mtk_ppe_regs.h +14 -0
@@ -149,6 +149,20 @@ enum {#define MTK_PPE_MIB_TB_BASE 0x338 +#define MTK_PPE_MIB_SER_CR 0x33C +#define MTK_PPE_MIB_SER_CR_ST BIT(16) +#define MTK_PPE_MIB_SER_CR_ADDR GENMASK(13, 0) + +#define MTK_PPE_MIB_SER_R0 0x340 +#define MTK_PPE_MIB_SER_R0_BYTE_CNT_LOW GENMASK(31, 0) + +#define MTK_PPE_MIB_SER_R1 0x344 +#define MTK_PPE_MIB_SER_R1_PKT_CNT_LOW GENMASK(31, 16) +#define MTK_PPE_MIB_SER_R1_BYTE_CNT_HIGH GENMASK(15, 0) + +#define MTK_PPE_MIB_SER_R2 0x348 +#define MTK_PPE_MIB_SER_R2_PKT_CNT_HIGH GENMASK(23, 0) +#define MTK_PPE_MIB_CACHE_CTL 0x350#define MTK_PPE_MIB_CACHE_CTL_EN BIT(0)#define MTK_PPE_MIB_CACHE_CTL_FLUSH BIT(2)
Signature · 1 line
2.39.1
Vladimir Oltean
maintainerRe: [PATCH 6/9] net: ethernet: mtk_eth_soc: ppe: add support for flow accounting
Quoted 22 lines · Daniel Golle
The PPE units found in MT7622 and newer support packet and byte accounting of hw-offloaded flows. Add support for reading those counters as found in MediaTek's SDK[1], make them accessible via debugfs and add them to the flow offload stats.
Signed-off-by Daniel Golle <[email protected]>--- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 8 +- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 1 + drivers/net/ethernet/mediatek/mtk_ppe.c | 110 +++++++++++++++++- drivers/net/ethernet/mediatek/mtk_ppe.h | 24 +++- .../net/ethernet/mediatek/mtk_ppe_debugfs.c | 9 +- .../net/ethernet/mediatek/mtk_ppe_offload.c | 7 ++ drivers/net/ethernet/mediatek/mtk_ppe_regs.h | 14 +++ 7 files changed, 168 insertions(+), 5 deletions(-)drivers/net/ethernet/mediatek/mtk_eth_soc.c +0
-0
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_probe(struct platform_device *pdev) · line 4712
Quoted 6 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +2
-1@@ -4708,7 +4708,9 @@ static int mtk_probe(struct platform_device *pdev)u32 ppe_addr = eth->soc->reg_map->ppe_base + i *0x400 ; eth->ppe[i] =mtk_ppe_init (eth, eth->base + ppe_addr, - eth->soc->offload_version, i); + eth->soc->offload_version, i, + eth->soc->has_accounting);
All arguments mtk_ppe_init() needs (this includes eth->soc->offload_version) are already available to it. See, first line in mtk_ppe_init() is:
const struct mtk_soc_data *soc = eth->soc;
Quoted 27 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +1
-0+if (!eth->ppe[i]) { err = -ENOMEM ; goto err_deinit_ppe;drivers/net/ethernet/mediatek/mtk_ppe.c +15
-0@@ -74,6 +74,46 @@ static int mtk_ppe_wait_busy(struct mtk_ppe *ppe)return ret; } +static int mtk_ppe_mib_wait_busy (struct mtk_ppe *ppe) +{ +int ret; +u32 val; + + ret =readl_poll_timeout (ppe->base +MTK_PPE_MIB_SER_CR , val, + !(val &MTK_PPE_MIB_SER_CR_ST ), +20 ,MTK_PPE_WAIT_TIMEOUT_US ); + +if (ret) +dev_err (ppe->dev,"MIB table busy" ); + +return ret; +} +
drivers/net/ethernet/mediatek/mtk_ppe.c static int mtk_ppe_wait_busy(struct mtk_ppe *ppe) · line 101
Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_ppe.c +10
-0+static int mtk_mib_entry_read (struct mtk_ppe *ppe,u16 index,u64 *bytes,u64 *packets) +{ +u32 val, cnt_r0, cnt_r1, cnt_r2; +u32 byte_cnt_low, byte_cnt_high, pkt_cnt_low, pkt_cnt_high; + + val =FIELD_PREP (MTK_PPE_MIB_SER_CR_ADDR , index) |MTK_PPE_MIB_SER_CR_ST ; +ppe_w32 (ppe,MTK_PPE_MIB_SER_CR , val); + +if (mtk_ppe_mib_wait_busy (ppe)) +return -ETIMEDOUT ;
err = mtk_ppe_mib_wait_busy();
if (err)
return err;
Quoted 41 lines
drivers/net/ethernet/mediatek/mtk_ppe.c +15
-0+ + cnt_r0 =readl (ppe->base +MTK_PPE_MIB_SER_R0 ); + cnt_r1 =readl (ppe->base +MTK_PPE_MIB_SER_R1 ); + cnt_r2 =readl (ppe->base +MTK_PPE_MIB_SER_R2 ); + + byte_cnt_low =FIELD_GET (MTK_PPE_MIB_SER_R0_BYTE_CNT_LOW , cnt_r0); + byte_cnt_high =FIELD_GET (MTK_PPE_MIB_SER_R1_BYTE_CNT_HIGH , cnt_r1); + pkt_cnt_low =FIELD_GET (MTK_PPE_MIB_SER_R1_PKT_CNT_LOW , cnt_r1); + pkt_cnt_high =FIELD_GET (MTK_PPE_MIB_SER_R2_PKT_CNT_HIGH , cnt_r2); + *bytes = ((u64 )byte_cnt_high <<32 ) | byte_cnt_low; + *packets = (pkt_cnt_high <<16 ) | pkt_cnt_low; + +return 0 ; +} +static void mtk_ppe_cache_clear (struct mtk_ppe *ppe) {ppe_set (ppe,MTK_PPE_CACHE_CTL ,MTK_PPE_CACHE_CTL_CLEAR );struct
mtk_ppe*mtk_ppe_init(structmtk_eth*eth, void__iomem*base,drivers/net/ethernet/mediatek/mtk_ppe.c +4
-1- int version, int index) + int version, int index, bool accounting) { const struct mtk_soc_data *soc = eth->soc;struct device *dev = eth->dev;struct mtk_ppe *ppe;u32 foe_flow_size;void *foe; +struct mtk_mib_entry *mib; +struct mtk_foe_accounting *acct; ppe =devm_kzalloc (dev,sizeof (*ppe),GFP_KERNEL );if (!ppe)@@ -778,6 +856,7 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base, ppe->eth = eth; ppe->dev = dev; ppe->version = version; + ppe->accounting = accounting; foe = dmam_alloc_coherent(ppe->dev, MTK_PPE_ENTRIES * soc->foe_entry_size,
drivers/net/ethernet/mediatek/mtk_ppe.c struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base, · line 881
Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_ppe.c +7
-0@@ -793,6 +872,25 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,if (!ppe->foe_flow) goto err_free_l2_flows; +if (accounting) { + mib =dmam_alloc_coherent (ppe->dev,MTK_PPE_ENTRIES *sizeof (*mib), + &ppe->mib_phys,GFP_KERNEL ); +if (!mib) +return NULL ; + +memset (mib,0 ,MTK_PPE_ENTRIES *sizeof (*mib));
I remember Jakub pointing out in another email that consistent DMA memory is already zero-initialized, and it appears in scripts/coccinelle/api/alloc/zalloc-simple.cocci.
Quoted 45 lines
drivers/net/ethernet/mediatek/mtk_ppe.c +22
-0+ + ppe->mib_table = mib; + + acct =devm_kzalloc (dev,MTK_PPE_ENTRIES *sizeof (*acct), +GFP_KERNEL ); + +if (!acct) +return NULL ; + + ppe->acct_table = acct; + } +mtk_ppe_debugfs_init (ppe, index);return ppe;@@ -922,6 +1020,16 @@ void mtk_ppe_start(struct mtk_ppe *ppe)ppe_w32 (ppe,MTK_PPE_DEFAULT_CPU_PORT1 ,0xcb777 );ppe_w32 (ppe,MTK_PPE_SBW_CTRL ,0x7f ); } + +if (ppe->accounting && ppe->mib_phys) { +ppe_w32 (ppe,MTK_PPE_MIB_TB_BASE , ppe->mib_phys); +ppe_m32 (ppe,MTK_PPE_MIB_CFG ,MTK_PPE_MIB_CFG_EN , +MTK_PPE_MIB_CFG_EN ); +ppe_m32 (ppe,MTK_PPE_MIB_CFG ,MTK_PPE_MIB_CFG_RD_CLR , +MTK_PPE_MIB_CFG_RD_CLR ); +ppe_m32 (ppe,MTK_PPE_MIB_CACHE_CTL ,MTK_PPE_MIB_CACHE_CTL_EN , +MTK_PPE_MIB_CFG_RD_CLR ); + } }int mtk_ppe_stop (struct mtk_ppe *ppe)drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c +1
-0@@ -82,6 +82,7 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)struct mtk_foe_entry *entry =mtk_foe_get_entry (ppe, i);struct mtk_foe_mac_info *l2;struct mtk_flow_addr_info ai = {}; +struct mtk_foe_accounting *acct;unsigned char h_source[ETH_ALEN ];unsigned char h_dest[ETH_ALEN ];int type, state;
drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind) · line 99
Quoted 4 lines
drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c +1
-0@@ -95,6 +96,8 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)if (bind && state !=MTK_FOE_STATE_BIND )continue ; + acct =mtk_foe_entry_get_mib (ppe, i,NULL );
might return NULL
drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind) · line 160
Quoted 11 lines
drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c +3
-1+ type =FIELD_GET (MTK_FOE_IB1_PACKET_TYPE , entry->ib1); seq_printf(m, "%05x %s %7s", i, mtk_foe_entry_state_str(state),@@ -153,9 +156,11 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind) *((__be16 *)&h_dest[4 ]) =htons (l2->dest_mac_lo); seq_printf(m, " eth=%pM->%pM etype=%04x" - " vlan=%d,%d ib1=%08x ib2=%08x\n", + " vlan=%d,%d ib1=%08x ib2=%08x" + " packets=%lld bytes=%lld\n",
%llu
Quoted 20 lines
drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c +2
-1h_source, h_dest,ntohs (l2->etype), - l2->vlan1, l2->vlan2, entry->ib1, ib2); + l2->vlan1, l2->vlan2, entry->ib1, ib2, + acct->packets, acct->bytes); }return 0 ;drivers/net/ethernet/mediatek/mtk_ppe_offload.c +1
-0@@ -497,6 +497,7 @@ static int mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f) { struct mtk_flow_entry *entry; +struct mtk_foe_accounting diff;u32 idle; entry = rhashtable_lookup(ð->flow_table, &f->cookie,
drivers/net/ethernet/mediatek/mtk_ppe_offload.c mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f) · line 512
Quoted 5 lines
drivers/net/ethernet/mediatek/mtk_ppe_offload.c +2
-0@@ -507,6 +508,12 @@ mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f) idle =mtk_foe_entry_idle_time (eth->ppe[entry->ppe_index], entry); f->stats.lastused = jiffies - idle *HZ ; +if (entry->hash !=0xFFFF ) { +mtk_foe_entry_get_mib (eth->ppe[entry->ppe_index], entry->hash, &diff);
If this returns NULL, you don't want to add diff.packets and diff.bytes to f->stats, because no one bothers to initialize "diff" to all-zeroes. So it contains junk from kernel stack memory. You might want to avoid this.
Quoted 7 lines
drivers/net/ethernet/mediatek/mtk_ppe_offload.c +4
-0+ f->stats.pkts += diff.packets; + f->stats.bytes += diff.bytes; + } +return 0 ; }
Daniel Golle
maintainerPATCH 7/9 net: pcs: add driver for MediaTek SGMII PCS
In reply to Daniel GolleThe SGMII core found in several MediaTek SoCs is identical to what can also be found in MediaTek's MT7531 Ethernet switch IC.
As this has not always been clear, both drivers developed different implementations to deal with the PCS.
Add a dedicated driver, mostly by copying the code now found in the Ethernet driver. The now redundant code will be removed by a follow-up commit.
--- MAINTAINERS | 7 + drivers/net/pcs/Kconfig | 6 + drivers/net/pcs/Makefile | 1 + drivers/net/pcs/pcs-mtk.c | 314 ++++++++++++++++++++++++++++++++++++ include/linux/pcs/pcs-mtk.h | 13 ++ 5 files changed, 341 insertions(+) create mode 100644 drivers/net/pcs/pcs-mtk.c create mode 100644 include/linux/pcs/pcs-mtk.h
MAINTAINERS +7 -0
@@ -13000,6 +13000,13 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/mediatek/ +MEDIATEK ETHERNET PCS DRIVER +M: Daniel Golle <[email protected]> +L: [email protected] +S: Maintained +F: drivers/net/pcs/pcs-mtk.c +F: include/linux/pcs/pcs-mtk.h + MEDIATEK I2C CONTROLLER DRIVER M: Qii Wang <[email protected]> L: [email protected]
drivers/net/pcs/Kconfig +6 -0
@@ -18,6 +18,12 @@ config PCS_LYNX This module provides helpers to phylink for managing the Lynx PCS which is part of the Layerscape and QorIQ Ethernet SERDES. +config PCS_MTK + tristate + help + This module provides helpers to phylink for managing the LynxI PCS + which is part of MediaTek's SoC and Ethernet switch ICs. + config PCS_RZN1_MIIC tristate "Renesas RZ/N1 MII converter" depends on OF && (ARCH_RZN1 || COMPILE_TEST)
drivers/net/pcs/Makefile +1 -0
@@ -5,5 +5,6 @@ pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o +obj-$(CONFIG_PCS_MTK) += pcs-mtk.o obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o obj-$(CONFIG_PCS_ALTERA_TSE) += pcs-altera-tse.o
drivers/net/pcs/pcs-mtk.c new file +314 -0
314 lines not on this page · raw message
include/linux/pcs/pcs-mtk.h new file +13 -0
@@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __LINUX_PCS_MTK_H +#define __LINUX_PCS_MTK_H + +#include <linux/phylink.h> +#include <linux/regmap.h> + +#define MTK_SGMII_FLAG_PN_SWAP BIT(0) +struct phylink_pcs *mtk_pcs_create (struct device *dev,struct regmap *regmap, +u32 ana_rgc3,u32 flags); +void mtk_pcs_destroy (struct phylink_pcs *pcs); + +#endif
Signature · 1 line
2.39.1
Andrew Lunn
maintainerRe: [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS
drivers/net/pcs/Kconfig config PCS_LYNX · line 25
Quoted 12 lines
index
6e7e6c346a3e..cf65646656e9100644drivers/net/pcs/Kconfig +5
-0@@ -18,6 +18,12 @@ config PCS_LYNX This module provides helpers to phylink for managing the Lynx PCS which is part of the Layerscape and QorIQ Ethernet SERDES. +config PCS_MTK + tristate + help + This module provides helpers to phylink for managing the LynxI PCS + which is part of MediaTek's SoC and Ethernet switch ICs.
You should probably have a more specific name, for when MTK produces a new PCS which is completely different.
Also, how similar is this LynxI PCS to the Lynx PCS?
Andrew
Vladimir Oltean
maintainerRe: [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS
Quoted 16 lines · Andrew Lunn
drivers/net/pcs/Kconfig config PCS_LYNX · line 25
Quoted 12 lines
index
6e7e6c346a3e..cf65646656e9100644drivers/net/pcs/Kconfig +5
-0@@ -18,6 +18,12 @@ config PCS_LYNX This module provides helpers to phylink for managing the Lynx PCS which is part of the Layerscape and QorIQ Ethernet SERDES. +config PCS_MTK + tristate + help + This module provides helpers to phylink for managing the LynxI PCS + which is part of MediaTek's SoC and Ethernet switch ICs.You should probably have a more specific name, for when MTK produces a new PCS which is completely different.
Quoted 1 line
Also, how similar is this LynxI PCS to the Lynx PCS?
Probably not very similar. Here's the Mediatek 32-bit memory map, translated by me to a 16-bit MDIO memory map:
/* SGMII subsystem config registers */
/* BMCR (low 16) BMSR (high 16) */
#define SGMSYS_PCS_CONTROL_1 0x0 // BMCR at MDIO addr 0x0, BMSR at 0x1, aka standard
#define SGMSYS_PCS_DEVICE_ID 0x4 // PHYSID1 at 0x2, PHYSID2 at 0x3, aka standard
#define SGMSYS_PCS_ADVERTISE 0x8 // MII_ADV at 0x4, MII_LPA at 0x5
#define SGMSYS_PCS_SCRATCH 0x14 // MDIO address 0xa
/* Register to programmable link timer, the unit in 2 * 8ns */
#define SGMSYS_PCS_LINK_TIMER 0x18 // MDIO address 0xc
/* Register to control remote fault */
#define SGMSYS_SGMII_MODE 0x20 // MDIO address 0x10
/* Register to reset SGMII design */
#define SGMII_RESERVED_0 0x34 // MDIO address 0x1a
/* Register to set SGMII speed, ANA RG_ Control Signals III */
#define SGMSYS_ANA_RG_CS3 0x2028 // not sure how to access this through C22, OTOH not used?
/* Register to power up QPHY */
#define SGMSYS_QPHY_PWR_STATE_CTRL 0xe8 // again, not sure how to access through C22
Compared to these definitions for Lynx, the rest being standard regs:
#define LINK_TIMER_LO 0x12
#define LINK_TIMER_HI 0x13
#define IF_MODE 0x14
So the standard bits appear to be common, the vendor extensions different. When I say common, I only take into consideration the memory map, not the differences in handling. For example, what Lynx handles as a single call to phylink_mii_c22_pcs_get_state(), the Mediatek PCS handles as a call to mtk_pcs_get_state().
Andrew Lunn
maintainerRe: [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS
Quoted 17 lines · Vladimir Oltean
Quoted 16 lines · Andrew Lunn
drivers/net/pcs/Kconfig config PCS_LYNX · line 25
Quoted 12 lines
index
6e7e6c346a3e..cf65646656e9100644drivers/net/pcs/Kconfig +5
-0@@ -18,6 +18,12 @@ config PCS_LYNX This module provides helpers to phylink for managing the Lynx PCS which is part of the Layerscape and QorIQ Ethernet SERDES. +config PCS_MTK + tristate + help + This module provides helpers to phylink for managing the LynxI PCS + which is part of MediaTek's SoC and Ethernet switch ICs.You should probably have a more specific name, for when MTK produces a new PCS which is completely different.
Quoted 4 lines
Quoted 1 line
Also, how similar is this LynxI PCS to the Lynx PCS?
Probably not very similar. Here's the Mediatek 32-bit memory map, translated by me to a 16-bit MDIO memory map:
Thanks. Given the similarities in the name, i had to ask...
Andrew
Daniel Golle
maintainerPATCH 8/9 net: ethernet: mtk_eth_soc: switch to external PCS driver
In reply to Daniel GolleNow that we got a PCS driver, use it and remove the now redundant PCS code and its header macros from the Ethernet driver.
--- drivers/net/ethernet/mediatek/Kconfig | 2 + drivers/net/ethernet/mediatek/mtk_eth_soc.c | 13 +- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 80 +------- drivers/net/ethernet/mediatek/mtk_sgmii.c | 200 +++----------------- 4 files changed, 37 insertions(+), 258 deletions(-)
drivers/net/ethernet/mediatek/Kconfig +2 -0
@@ -19,6 +19,8 @@ config NET_MEDIATEK_SOC select DIMLIB select PAGE_POOL select PAGE_POOL_STATS + select PCS_MTK + select REGMAP_MMIO help This driver supports the gigabit ethernet MACs in the MediaTek SoC family.
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10 -3
@@ -4081,6 +4081,7 @@ static int mtk_unreg_dev(struct mtk_eth *eth)static int mtk_cleanup (struct mtk_eth *eth) { +mtk_sgmii_destroy (eth->sgmii);mtk_unreg_dev (eth);mtk_free_dev (eth);cancel_work_sync (ð->pending_work);
@@ -4578,6 +4579,7 @@ static int mtk_probe(struct platform_device *pdev)if (!eth->sgmii)return -ENOMEM ; + eth->sgmii->dev = eth->dev; err =mtk_sgmii_init (eth->sgmii, pdev->dev.of_node, eth->soc->ana_rgc3);
@@ -4590,14 +4592,17 @@ static int mtk_probe(struct platform_device *pdev) "mediatek,pctl");if (IS_ERR (eth->pctl)) {dev_err (&pdev->dev,"no pctl regmap found\n" ); -return PTR_ERR (eth->pctl); + err =PTR_ERR (eth->pctl); + goto err_destroy_sgmii; } }if (MTK_HAS_CAPS (eth->soc->caps,MTK_NETSYS_V2 )) { res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); -if (!res) -return -EINVAL ; +if (!res) { + err = -EINVAL ; + goto err_destroy_sgmii; + } }if (eth->soc->offload_version) {
@@ -4749,6 +4754,8 @@ static int mtk_probe(struct platform_device *pdev)return 0 ; +err_destroy_sgmii: +mtk_sgmii_destroy (eth->sgmii); err_deinit_ppe:mtk_ppe_deinit (eth);mtk_mdio_cleanup (eth);
drivers/net/ethernet/mediatek/mtk_eth_soc.h +3 -77
103 lines not on this page · raw message
drivers/net/ethernet/mediatek/mtk_sgmii.c +22 -178
229 lines not on this page · raw message
Signature · 1 line
2.39.1
Re: [PATCH 8/9] net: ethernet: mtk_eth_soc: switch to external PCS driver
drivers/net/ethernet/mediatek/mtk_sgmii.c line 32
Quoted 9 lines · Daniel Golle
drivers/net/ethernet/mediatek/mtk_sgmii.c +2
-5- ss->pcs[i].ana_rgc3 = ana_rgc3; - ss->pcs[i].regmap =syscon_node_to_regmap (np); - - ss->pcs[i].flags =0 ; + flags =0 ;if (of_property_read_bool (np,"pn_swap" )) - ss->pcs[i].flags |=MTK_SGMII_FLAG_PN_SWAP ; + flags |=MTK_SGMII_FLAG_PN_SWAP ;
patch 1 added "mediatek,pn_swap" so this doesn't apply. We've all done last minute cleanups - never a good idea :-)
Bjørn
Vladimir Oltean
maintainerRe: [PATCH 8/9] net: ethernet: mtk_eth_soc: switch to external PCS driver
In reply to Daniel GolleQuoted 19 lines · Daniel Golle
int
mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3){drivers/net/ethernet/mediatek/mtk_sgmii.c +3
-5struct device_node *np;int i; +u32 flags; +struct regmap *regmap;for (i =0 ; i <MTK_MAX_DEVS ; i++) { np =of_parse_phandle (r,"mediatek,sgmiisys" , i);if (!np)break ; - ss->pcs[i].ana_rgc3 = ana_rgc3; - ss->pcs[i].regmap =syscon_node_to_regmap (np); - - ss->pcs[i].flags =0 ; + flags =0 ;if (of_property_read_bool (np,"pn_swap" )) - ss->pcs[i].flags |=MTK_SGMII_FLAG_PN_SWAP ;
drivers/net/ethernet/mediatek/mtk_sgmii.c line 36
Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_sgmii.c +2
-5+ flags |=MTK_SGMII_FLAG_PN_SWAP ;of_node_put (np); -if (IS_ERR (ss->pcs[i].regmap)) -return PTR_ERR (ss->pcs[i].regmap); - ss->pcs[i].pcs.ops = &mtk_pcs_ops; - ss->pcs[i].pcs.poll = true; - ss->pcs[i].interface =PHY_INTERFACE_MODE_NA ; + regmap =syscon_node_to_regmap (np);
Not supposed to use "np" after of_node_put().
Quoted 5 lines
drivers/net/ethernet/mediatek/mtk_sgmii.c +4
-0+if (IS_ERR (regmap)) +return PTR_ERR (regmap); + + ss->pcs[i] =mtk_pcs_create (ss->dev, regmap, ana_rgc3, flags); }
Daniel Golle
maintainerPATCH 9/9 net: dsa: mt7530: use external PCS driver
In reply to Daniel GolleImplement regmap access wrappers, for now only to be used by the pcs-mtk driver.
Make use of external PCS driver and drop the reduntant implementation in mt7530.c.
As a nice side effect the SGMII registers can now also more easily be inspected for debugging via /sys/kernel/debug/regmap.
--- drivers/net/dsa/Kconfig | 2 + drivers/net/dsa/mt7530.c | 278 ++++++++++----------------------------- drivers/net/dsa/mt7530.h | 43 +----- 3 files changed, 72 insertions(+), 251 deletions(-)
drivers/net/dsa/Kconfig +2 -0
@@ -38,6 +38,8 @@ config NET_DSA_MT7530 tristate "MediaTek MT7530 and MT7531 Ethernet switch support" select NET_DSA_TAG_MTK select MEDIATEK_GE_PHY + select PCS_MTK + select REGMAP help This enables support for the MediaTek MT7530 and MT7531 Ethernet switch chips. Multi-chip module MT7530 in MT7621AT, MT7621DAT,
drivers/net/dsa/mt7530.c +68 -210
362 lines not on this page · raw message
drivers/net/dsa/mt7530.h +2 -41
55 lines not on this page · raw message
Signature · 1 line
2.39.1
Vladimir Oltean
maintainerRe: [PATCH 9/9] net: dsa: mt7530: use external PCS driver
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2620
Quoted 11 lines · Daniel Golle
drivers/net/dsa/mt7530.c +3
-1@@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,switch (interface) {case PHY_INTERFACE_MODE_TRGMII : +return &priv->pcs[port].pcs;case PHY_INTERFACE_MODE_SGMII :case PHY_INTERFACE_MODE_1000BASEX :case PHY_INTERFACE_MODE_2500BASEX : -return &priv->pcs[port].pcs; +if (!mt753x_is_mac_port (port)) +return ERR_PTR (-EINVAL );
What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2622
Quoted 2 lines
drivers/net/dsa/mt7530.c +1
-0+return priv->sgmii_pcs[port -5 ];
How about putting the pcs pointer in struct mt7530_port?
Quoted 13 lines
drivers/net/dsa/mt7530.c +0
-2default :return NULL ; }@@ -3088,8 +2934,6 @@ mt753x_setup(struct dsa_switch *ds) priv->pcs[i].pcs.ops = priv->info->pcs_ops; priv->pcs[i].priv = priv; priv->pcs[i].port = i; -if (mt753x_is_mac_port (i)) - priv->pcs[i].pcs.poll =1 ; } ret = priv->info->sw_setup (ds);
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2949
Quoted 2 lines
drivers/net/dsa/mt7530.c +0
-0@@ -3104,6 +2948,15 @@ mt753x_setup(struct dsa_switch *ds)if (ret && priv->irq)mt7530_free_irq_common (priv);
You need to patch the previous code to "return ret".
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2951
Quoted 2 lines
drivers/net/dsa/mt7530.c +1
-0+if (priv->id ==ID_MT7531 )
if the code block below is multi-line (which it is), put braces here too
or can return early if priv->id != ID_MT7531, and this reduces the indentation by one level.
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2952
Quoted 1 line
drivers/net/dsa/mt7530.c +1
-0+for (i =0 ; i <2 ; ++i) {
could also iterate over all ports and ignore those which have !mt753x_is_mac_port(port)
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2955
Quoted 3 lines
drivers/net/dsa/mt7530.c +3
-0+ regmap =devm_regmap_init (ds->dev, + &mt7531_regmap_bus, priv, + &mt7531_pcs_config[i]);
can fail
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2957
Quoted 2 lines
drivers/net/dsa/mt7530.c +2
-0+ priv->sgmii_pcs[i] =mtk_pcs_create (ds->dev, regmap, +0x128 ,0 );
can fail
Don't forget to do error teardown which isn't leaky
0x128 comes from the old definition of MT7531_PHYA_CTRL_SIGNAL3(port), so please keep a macro of some sorts to denote the offset of ana_rgc3 for MT7531, rather than just this obscure magic number.
Quoted 24 lines
drivers/net/dsa/mt7530.c +4
-2+ } +return ret; }@@ -3199,7 +3052,7 @@ static const struct mt753x_info mt753x_table[] = { }, [ID_MT7531] = { .id = ID_MT7531, - .pcs_ops = &mt7531_pcs_ops, + .pcs_ops = &mt7530_pcs_ops, .sw_setup = mt7531_setup, .phy_read_c22 = mt7531_ind_c22_phy_read, .phy_write_c22 = mt7531_ind_c22_phy_write,@@ -3309,7 +3162,7 @@ static void mt7530_remove(struct mdio_device *mdiodev) {struct mt7530_priv *priv =dev_get_drvdata (&mdiodev->dev); -int ret =0 ; +int ret =0 , i;if (!priv)return ;
drivers/net/dsa/mt7530.c mt7530_remove(struct mdio_device *mdiodev) · line 3185
Quoted 5 lines
drivers/net/dsa/mt7530.c +2
-0@@ -3328,6 +3181,11 @@ mt7530_remove(struct mdio_device *mdiodev)mt7530_free_irq (priv);dsa_unregister_switch (priv->ds); + +for (i =0 ; i <2 ; ++i)
There is no ++i in this driver and there are 11 i++, so please be consistent with what exists.
Quoted 6 lines
drivers/net/dsa/mt7530.c +3
-0+if (priv->sgmii_pcs[i]) +mtk_pcs_destroy (priv->sgmii_pcs[i]); +mutex_destroy (&priv->reg_mutex); }
Daniel Golle
maintainerRe: [PATCH 9/9] net: dsa: mt7530: use external PCS driver
Hi Vladimir,
thank you for the review!
Quoted 14 lines · Vladimir Oltean
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2620
Quoted 11 lines · Daniel Golle
drivers/net/dsa/mt7530.c +3
-1@@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,switch (interface) {case PHY_INTERFACE_MODE_TRGMII : +return &priv->pcs[port].pcs;case PHY_INTERFACE_MODE_SGMII :case PHY_INTERFACE_MODE_1000BASEX :case PHY_INTERFACE_MODE_2500BASEX : -return &priv->pcs[port].pcs; +if (!mt753x_is_mac_port (port)) +return ERR_PTR (-EINVAL );What is the reason for returning
ERR_PTR(-EINVAL)tomac_select_pcs()?
The SerDes PCS units are only available for port 5 and 6. The code should make sure that the corresponding interface modes are only used on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also do the trick, I guess. However, as dsa_port_phylink_mac_select_pcs may also return ERR_PTR(-EOPNOTSUPP), returning ERR_PTR(-EINVAL) felt like the right thing to do in that case.
Are you suggesting to use BUG_ON() instead or rather return ERR_PTR(-EOPNOTSUPP)?
Quoted 5 lines
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2622
Quoted 3 lines
drivers/net/dsa/mt7530.c +1
-0+return priv->sgmii_pcs[port -5 ];How about putting the pcs pointer in struct
mt7530_port?
There are only two SerDes units available, only for port 5 and port 6. Hence I wouldn't want to allocate additional unused sizeof(void*) for ports 0 to 4.
Quoted 75 lines
Quoted 14 lines
drivers/net/dsa/mt7530.c +0
-2default :return NULL ; }@@ -3088,8 +2934,6 @@ mt753x_setup(struct dsa_switch *ds) priv->pcs[i].pcs.ops = priv->info->pcs_ops; priv->pcs[i].priv = priv; priv->pcs[i].port = i; -if (mt753x_is_mac_port (i)) - priv->pcs[i].pcs.poll =1 ; } ret = priv->info->sw_setup (ds);drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2949
Quoted 2 lines
drivers/net/dsa/mt7530.c +0
-0@@ -3104,6 +2948,15 @@ mt753x_setup(struct dsa_switch *ds)if (ret && priv->irq)mt7530_free_irq_common (priv);You need to patch the previous code to "return ret".
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2951
Quoted 2 lines
drivers/net/dsa/mt7530.c +1
-0+if (priv->id ==ID_MT7531 )if the code block below is multi-line (which it is), put braces here too
or can return early if
priv->id!=ID_MT7531, and this reduces the indentation by one level.drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2952
Quoted 1 line
drivers/net/dsa/mt7530.c +1
-0+for (i =0 ; i <2 ; ++i) {could also iterate over all ports and ignore those which have !mt753x_is_mac_port(port)
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2955
Quoted 3 lines
drivers/net/dsa/mt7530.c +3
-0+ regmap =devm_regmap_init (ds->dev, + &mt7531_regmap_bus, priv, + &mt7531_pcs_config[i]);can fail
drivers/net/dsa/mt7530.c mt753x_setup(struct dsa_switch *ds) · line 2957
Quoted 2 lines
drivers/net/dsa/mt7530.c +2
-0+ priv->sgmii_pcs[i] =mtk_pcs_create (ds->dev, regmap, +0x128 ,0 );can fail
Don't forget to do error teardown which isn't leaky
0x128comes from the old definition ofMT7531_PHYA_CTRL_SIGNAL3(port), so please keep a macro of some sorts to denote the offset ofana_rgc3for MT7531, rather than just this obscure magic number.Quoted 26 lines
drivers/net/dsa/mt7530.c +4
-2+ } +return ret; }@@ -3199,7 +3052,7 @@ static const struct mt753x_info mt753x_table[] = { }, [ID_MT7531] = { .id = ID_MT7531, - .pcs_ops = &mt7531_pcs_ops, + .pcs_ops = &mt7530_pcs_ops, .sw_setup = mt7531_setup, .phy_read_c22 = mt7531_ind_c22_phy_read, .phy_write_c22 = mt7531_ind_c22_phy_write,@@ -3309,7 +3162,7 @@ static void mt7530_remove(struct mdio_device *mdiodev) {struct mt7530_priv *priv =dev_get_drvdata (&mdiodev->dev); -int ret =0 ; +int ret =0 , i;if (!priv)return ;@@ -3328,6 +3181,11 @@ mt7530_remove(struct mdio_device *mdiodev)mt7530_free_irq (priv);
Quoted 6 lines
drivers/net/dsa/mt7530.c mt7530_remove(struct mdio_device *mdiodev) · line 3185
Quoted 3 lines
drivers/net/dsa/mt7530.c +2
-0dsa_unregister_switch (priv->ds); + +for (i =0 ; i <2 ; ++i)There is no ++i in this driver and there are 11 i++, so please be consistent with what exists.
As most likely in all cases a pre-increment is sufficient and less resource consuming than a post-increment operation we should consider switching them all to ++i instead of i++.
I will anyway use i++ in v2 for now.
Quoted 7 lines
Quoted 7 lines
drivers/net/dsa/mt7530.c +3
-0+if (priv->sgmii_pcs[i]) +mtk_pcs_destroy (priv->sgmii_pcs[i]); +mutex_destroy (&priv->reg_mutex); }
Andrew Lunn
maintainerRe: [PATCH 9/9] net: dsa: mt7530: use external PCS driver
Quoted 8 lines
The SerDes PCS units are only available for port 5 and 6. The code should make sure that the corresponding interface modes are only used on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also do the trick, I guess. However, as
dsa_port_phylink_mac_select_pcsmay also returnERR_PTR(-EOPNOTSUPP), returningERR_PTR(-EINVAL)felt like the right thing to do in that case.
Are you suggesting to useBUG_ON()instead or rather returnERR_PTR(-EOPNOTSUPP)?
BUG_ON() is considered to mean something which you cannot recover from has happened, and going further will only cause more file system corruption, memory corruption, etc. WARN_ON() is better, since it gives the user a chance to perform a controlled shutdown, so potentially not loosing files etc.
But even a WARN_ON() seems a bit extreme. If -EINVAL is sufficient to cause either the whole device, or the port to fail to probe, that should be enough to get the DT developers attention. Then either a dev_err() or a dev_dbg() to help narrow down which check failed.
Andrew
Russell King (Oracle)
reviewerRe: [PATCH 9/9] net: dsa: mt7530: use external PCS driver
In reply to Daniel GolleHi Daniel,
I haven't reviewed the patches yet, and probably won't for a while. (I'm recovering from Covid).
Quoted 20 lines · Daniel Golle
Hi Vladimir,
thank you for the review!
Quoted 14 lines · Vladimir Oltean
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2620
Quoted 11 lines · Daniel Golle
drivers/net/dsa/mt7530.c +3
-1@@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,switch (interface) {case PHY_INTERFACE_MODE_TRGMII : +return &priv->pcs[port].pcs;case PHY_INTERFACE_MODE_SGMII :case PHY_INTERFACE_MODE_1000BASEX :case PHY_INTERFACE_MODE_2500BASEX : -return &priv->pcs[port].pcs; +if (!mt753x_is_mac_port (port)) +return ERR_PTR (-EINVAL );What is the reason for returning
ERR_PTR(-EINVAL)tomac_select_pcs()?
Quoted 8 lines
The SerDes PCS units are only available for port 5 and 6. The code should make sure that the corresponding interface modes are only used on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also do the trick, I guess. However, as
dsa_port_phylink_mac_select_pcsmay also returnERR_PTR(-EOPNOTSUPP), returningERR_PTR(-EINVAL)felt like the right thing to do in that case.
Are you suggesting to useBUG_ON()instead or rather returnERR_PTR(-EOPNOTSUPP)?
Returning ERR_PTR(-EOPNOTSUPP) from mac_select_pcs() must not be done conditionally - it means that "this instance does not support the mac_select_pcs() interface" and phylink will never call it again.
It was implemented to provide DSA a way to tell phylink that the DSA driver doesn't implement this interface - phylink originally checked whether mac_ops->mac_select_pcs was NULL, but with DSA's layering, such a check can only be made by a runtime call.
Returning NULL means "there is no PCS for this interface mode", and returning any other error code essentially signifies that the interface mode is not supported (even e.g. GMII or INTERNAL)... which really *should* be handled by setting supported_interfaces correctly in the first place.
I would much prefer drivers to just return NULL if they have no PCS, even for ports where it's not possible, or not implement the interface over returning some kind of error code.
The only time I would expect mac_select_pcs() to return an error is where it needed to do something in order to evaluate whether to return a PCS or not, and it encountered an error while trying to evaluate that or some truely bizarre situation. E.g. a failed memory allocation, or "we know that a PCS is required for this but we're missing it". Something of that ilk.
Anyway, more rest needed.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Vladimir Oltean
maintainerRe: [PATCH 9/9] net: dsa: mt7530: use external PCS driver
In reply to Daniel GolleQuoted 20 lines · Daniel Golle
Hi Vladimir,
thank you for the review!
Quoted 14 lines · Vladimir Oltean
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2620
Quoted 11 lines · Daniel Golle
drivers/net/dsa/mt7530.c +3
-1@@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,switch (interface) {case PHY_INTERFACE_MODE_TRGMII : +return &priv->pcs[port].pcs;case PHY_INTERFACE_MODE_SGMII :case PHY_INTERFACE_MODE_1000BASEX :case PHY_INTERFACE_MODE_2500BASEX : -return &priv->pcs[port].pcs; +if (!mt753x_is_mac_port (port)) +return ERR_PTR (-EINVAL );What is the reason for returning
ERR_PTR(-EINVAL)tomac_select_pcs()?
Quoted 8 lines
The SerDes PCS units are only available for port 5 and 6. The code should make sure that the corresponding interface modes are only used on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also do the trick, I guess. However, as
dsa_port_phylink_mac_select_pcsmay also returnERR_PTR(-EOPNOTSUPP), returningERR_PTR(-EINVAL)felt like the right thing to do in that case.
Are you suggesting to useBUG_ON()instead or rather returnERR_PTR(-EOPNOTSUPP)?
I was not suggesting anything, I was just asking why the inconsistency exists between the handling of SERDES protocols on ports != 5 or 6 (return ERR_PTR(-EINVAL)), and the handling of unknown PHY modes (return NULL). If you don't want to convey any special message, then returning NULL to phylink should be sufficient to say there is no PCS. The driver already ensures that phylink validation fails with wrong PHY mode on wrong port due to the way in which it populates supported_interfaces in mt7531_mac_port_get_caps().
Also, no BUG_ON() for the reasons Andrew pointed out.
Quoted 7 lines
Quoted 3 lines
drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2622
Quoted 1 line
drivers/net/dsa/mt7530.c +1
-0+return priv->sgmii_pcs[port -5 ];How about putting the pcs pointer in struct
mt7530_port?There are only two SerDes units available, only for port 5 and port 6. Hence I wouldn't want to allocate additional unused
sizeof(void*)for ports 0 to 4.
I asked the question fully knowing that there will be no more than 2 ports with a non-NULL PCS pointer. There's a time and place for (premature) optimizations, but I don't think that the control path of a switch is one particular place that comes at the top. Between priv->ports[port].sgmii_pcs and priv->sgmii_pcs[port - 5], my personal sensibilities for simple and maintainable code would always choose the former. However I'm not going to cling onto this. Whichever way you prefer.
Quoted 9 lines
Quoted 4 lines
drivers/net/dsa/mt7530.c mt7530_remove(struct mdio_device *mdiodev) · line 3185
Quoted 1 line
drivers/net/dsa/mt7530.c +1
-0+for (i =0 ; i <2 ; ++i)There is no ++i in this driver and there are 11 i++, so please be consistent with what exists.
As most likely in all cases a pre-increment is sufficient and less resource consuming than a post-increment operation we should consider switching them all to ++i instead of i++.
I will anyway use i++ in v2 for now.
And what would you suggest is the difference in the compiled code between "for (i = 0; i < n; i++)" and "for (i = 0; i < n; ++i)"?
Quoted 3 lines · Daniel Golle
This series brings a variety of fixes and enhancements for
mtk_eth_soc, adds support for the MT7981 SoC and facilitates sharing the SGMII PCS code betweenmtk_eth_socand mt7530.
Thanks! I've now successfully tested this on an MT7986 board with 2.5G phys (GPY211C) connected to one of the mtk_eth_soc macs and on port 5 of the the MT7531. The series fixes a number of issues for me, including a mysterious packet drop at 1G only which I believe is related to the undocumented MAC_MCR_BIT_12 in patch 5.
With Vladimir's (WiP?) "PCS and PHY in-band sync" patch set and your related implementations in mxl-gpy, mt7530 and mtk_eth_soc to top of this series, both 2.5G ports are now usable at any speed.
Note that the mt7530.c part of the series depends on
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/commit/?id=697c3892d825fb78f42ec8e53bed065dd728db3e
which is not yet in net-next. It appears to be destined for v6.2 so this is not a problem. Just mentioning it in case someone else is struggling with the testing of this series.
Feel free to add
to the complete series.
Bjørn