2023-02-07 14:20:34

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 00/11] 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.

Note that this series depends on commit 697c3892d825
("regmap: apply reg_base and reg_downshift for single register ops") to
not break mt7530 pcs register access.

Changes since v1:
* apply reverse xmas tree everywhere
* improve commit descriptions
* add dt bindings
* various small changes addressing all comments received for v1

Daniel Golle (11):
net: ethernet: mtk_eth_soc: add support for MT7981 SoC
dt-bindings: net: mediatek,net: add mt7981-eth binding
dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property
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

.../arm/mediatek/mediatek,sgmiisys.txt | 4 +
.../devicetree/bindings/net/mediatek,net.yaml | 42 +++
MAINTAINERS | 7 +
drivers/net/dsa/Kconfig | 1 +
drivers/net/dsa/mt7530.c | 277 ++++-----------
drivers/net/dsa/mt7530.h | 47 +--
drivers/net/ethernet/mediatek/Kconfig | 2 +
drivers/net/ethernet/mediatek/mtk_eth_path.c | 14 +-
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 65 +++-
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 100 ++----
drivers/net/ethernet/mediatek/mtk_ppe.c | 114 ++++++-
drivers/net/ethernet/mediatek/mtk_ppe.h | 25 +-
.../net/ethernet/mediatek/mtk_ppe_debugfs.c | 9 +-
.../net/ethernet/mediatek/mtk_ppe_offload.c | 8 +
drivers/net/ethernet/mediatek/mtk_ppe_regs.h | 14 +
drivers/net/ethernet/mediatek/mtk_sgmii.c | 192 ++---------
drivers/net/pcs/Kconfig | 8 +
drivers/net/pcs/Makefile | 1 +
drivers/net/pcs/pcs-mtk-lynxi.c | 315 ++++++++++++++++++
include/linux/pcs/pcs-mtk-lynxi.h | 13 +
20 files changed, 756 insertions(+), 502 deletions(-)
create mode 100644 drivers/net/pcs/pcs-mtk-lynxi.c
create mode 100644 include/linux/pcs/pcs-mtk-lynxi.h


base-commit: 129af770823407ee115a56c69a04b440fd2fbe61
--
2.39.1


2023-02-07 14:20:43

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 01/11] net: ethernet: mtk_eth_soc: add support for MT7981 SoC

The MediaTek MT7981 SoC comes with two 1G/2.5G SGMII ports, just like
MT7986.

In addition MT7981 is equipped with a built-in 1000Base-T PHY which can
be used with GMAC1.

As many MT7981 boards make use of inverting SGMII signal polarity, add
new device-tree attribute 'mediatek,pn_swap' to support them.

Signed-off-by: Daniel Golle <[email protected]>
---
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(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_path.c b/drivers/net/ethernet/mediatek/mtk_eth_path.c
index 72648535a14d..317e447f4991 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_path.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_path.c
@@ -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);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 4570a39a430b..fc5174f83ef1 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4858,6 +4858,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,
@@ -4900,6 +4920,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},
{},
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index afc9d52e79bf..7230dcb29315 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -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
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index bb00de1003ac..0071352c93ba 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -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);
--
2.39.1


2023-02-07 14:21:13

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 02/11] dt-bindings: net: mediatek,net: add mt7981-eth binding

Introduce DT bindings for the MT7981 SoC to mediatek,net.yaml.

Signed-off-by: Daniel Golle <[email protected]>
---
.../devicetree/bindings/net/mediatek,net.yaml | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
index 7ef696204c5a..d17e2eb46118 100644
--- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
+++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
@@ -21,6 +21,7 @@ properties:
- mediatek,mt7623-eth
- mediatek,mt7622-eth
- mediatek,mt7629-eth
+ - mediatek,mt7981-eth
- mediatek,mt7986-eth
- ralink,rt5350-eth

@@ -206,6 +207,47 @@ allOf:

mediatek,wed: false

+ - if:
+ properties:
+ compatible:
+ contains:
+ const: mediatek,mt7981-eth
+ then:
+ properties:
+ interrupts:
+ minItems: 4
+
+ clocks:
+ minItems: 15
+ maxItems: 15
+
+ clock-names:
+ items:
+ - const: fe
+ - const: gp2
+ - const: gp1
+ - const: wocpu0
+ - const: sgmii_ck
+ - const: sgmii_tx250m
+ - const: sgmii_rx250m
+ - const: sgmii_cdr_ref
+ - const: sgmii_cdr_fb
+ - const: sgmii2_tx250m
+ - const: sgmii2_rx250m
+ - const: sgmii2_cdr_ref
+ - const: sgmii2_cdr_fb
+ - const: netsys0
+ - const: netsys1
+
+ mediatek,sgmiisys:
+ minItems: 2
+ maxItems: 2
+
+ mediatek,wed-pcie:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description:
+ Phandle to the mediatek wed-pcie controller.
+
- if:
properties:
compatible:
--
2.39.1


2023-02-07 14:21:45

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

Add documentation for the newly introduced 'mediatek,pn_swap' property
to mediatek,sgmiisys.txt.

Signed-off-by: Daniel Golle <[email protected]>
---
.../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
index d2c24c277514..b38dd0fde21d 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
@@ -14,6 +14,10 @@ Required Properties:
- "mediatek,mt7986-sgmiisys_1", "syscon"
- #clock-cells: Must be 1

+Optional Properties:
+
+- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
+
The SGMIISYS controller uses the common clk binding from
Documentation/devicetree/bindings/clock/clock-bindings.txt
The available clocks are defined in dt-bindings/clock/mt*-clk.h.
--
2.39.1


2023-02-07 14:22:37

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 04/11] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

Set MDIO bus clock frequency and allow setting a custom maximum
frquency from device tree.

Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 21 +++++++++++++++++++++
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 7 +++++++
2 files changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index fc5174f83ef1..4ee582e39c12 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -789,8 +789,10 @@ static const struct phylink_mac_ops mtk_phylink_ops = {

static int mtk_mdio_init(struct mtk_eth *eth)
{
+ unsigned int max_clk = 2500000, divider;
struct device_node *mii_np;
int ret;
+ u32 val;

mii_np = of_get_child_by_name(eth->dev->of_node, "mdio-bus");
if (!mii_np) {
@@ -818,6 +820,25 @@ 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)) {
+ if (val > MDC_MAX_FREQ || val < MDC_MAX_FREQ / MDC_MAX_DIVIDER) {
+ dev_err(eth->dev, "MDIO clock frequency out of range");
+ ret = -EINVAL;
+ goto err_put_node;
+ }
+ max_clk = val;
+ }
+ divider = min_t(unsigned int, DIV_ROUND_UP(MDC_MAX_FREQ, max_clk), 63);
+
+ /* Configure MDC Divider */
+ val = mtk_r32(eth, MTK_PPSC);
+ val &= ~PPSC_MDC_CFG;
+ val |= FIELD_PREP(PPSC_MDC_CFG, divider) | PPSC_MDC_TURBO;
+ mtk_w32(eth, val, MTK_PPSC);
+
+ dev_dbg(eth->dev, "MDC is running on %d Hz\n", MDC_MAX_FREQ / divider);
+
ret = of_mdiobus_register(eth->mii_bus, mii_np);

err_put_node:
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 7230dcb29315..7014c02ba2d4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -363,6 +363,13 @@
#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)
+#define MDC_MAX_FREQ 25000000
+#define MDC_MAX_DIVIDER 63
+
/* PHY Indirect Access Control registers */
#define MTK_PHY_IAC 0x10004
#define PHY_IAC_ACCESS BIT(31)
--
2.39.1


2023-02-07 14:23:00

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 05/11] net: ethernet: mtk_eth_soc: reset PCS state

Reset PCS state when changing interface mode.

Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 ++++
drivers/net/ethernet/mediatek/mtk_sgmii.c | 4 ++++
2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 7014c02ba2d4..142def8629c8 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -548,6 +548,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))
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 0071352c93ba..d8184448cd5a 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -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,
--
2.39.1


2023-02-07 14:23:18

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 06/11] net: ethernet: mtk_eth_soc: only write values if needed

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(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index d8184448cd5a..9c58006d1c33 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -38,20 +38,16 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
const unsigned long *advertising,
bool permit_pause_to_mac)
{
+ bool mode_changed = false, changed, use_an;
struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
unsigned int rgc3, sgm_mode, bmcr;
int advertise, link_timer;
- bool 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)
--
2.39.1


2023-02-07 14:24:09

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 07/11] net: ethernet: mtk_eth_soc: fix RX data corruption issue

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]
Unfortunately the meaning of bit 12 is not documented in datasheets or
vendor code.

[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/d8a2975939a12686c4a95c40db21efdc3f821f63
Signed-off-by: Daniel Golle <[email protected]>
---
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(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 4ee582e39c12..aabbf0337589 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -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! */
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 142def8629c8..084d07c96c04 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -404,6 +404,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)
--
2.39.1


2023-02-07 14:24:34

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 08/11] net: ethernet: mtk_eth_soc: ppe: add support for flow accounting

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].

[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/bc6a6a375c800dc2b80e1a325a2c732d1737df92
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 | 114 +++++++++++++++++-
drivers/net/ethernet/mediatek/mtk_ppe.h | 25 +++-
.../net/ethernet/mediatek/mtk_ppe_debugfs.c | 9 +-
.../net/ethernet/mediatek/mtk_ppe_offload.c | 8 ++
drivers/net/ethernet/mediatek/mtk_ppe_regs.h | 14 +++
7 files changed, 170 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index aabbf0337589..97df77c7999e 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4709,8 +4709,8 @@ static int mtk_probe(struct platform_device *pdev)
for (i = 0; i < num_ppe; i++) {
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->ppe[i] = mtk_ppe_init(eth, eth->base + ppe_addr, i);
+
if (!eth->ppe[i]) {
err = -ENOMEM;
goto err_deinit_ppe;
@@ -4832,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),
@@ -4869,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),
@@ -4889,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),
@@ -4909,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),
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 084d07c96c04..c2e0fd773cc2 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -1087,6 +1087,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;
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
index 6883eb34cd8b..c099e8736716 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
@@ -74,6 +74,48 @@ 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;
+}
+
+static int mtk_mib_entry_read(struct mtk_ppe *ppe, u16 index, u64 *bytes, u64 *packets)
+{
+ u32 byte_cnt_low, byte_cnt_high, pkt_cnt_low, pkt_cnt_high;
+ u32 val, cnt_r0, cnt_r1, cnt_r2;
+ int ret;
+
+ 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);
+
+ ret = mtk_ppe_mib_wait_busy(ppe);
+ if (ret)
+ return ret;
+
+ 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);
@@ -458,6 +500,13 @@ __mtk_foe_entry_clear(struct mtk_ppe *ppe, struct mtk_flow_entry *entry)
hwe->ib1 &= ~MTK_FOE_IB1_STATE;
hwe->ib1 |= FIELD_PREP(MTK_FOE_IB1_STATE, MTK_FOE_STATE_INVALID);
dma_wmb();
+ if (ppe->accounting) {
+ struct mtk_foe_accounting *acct;
+
+ acct = ppe->acct_table + entry->hash * sizeof(*acct);
+ acct->packets = 0;
+ acct->bytes = 0;
+ }
}
entry->hash = 0xffff;

@@ -565,6 +614,9 @@ __mtk_foe_entry_commit(struct mtk_ppe *ppe, struct mtk_foe_entry *entry,
wmb();
hwe->ib1 = entry->ib1;

+ if (ppe->accounting)
+ *mtk_foe_entry_ib2(eth, hwe) |= MTK_FOE_IB2_MIB_CNT;
+
dma_wmb();

mtk_ppe_cache_clear(ppe);
@@ -756,11 +808,39 @@ int mtk_ppe_prepare_reset(struct mtk_ppe *ppe)
return mtk_ppe_wait_busy(ppe);
}

-struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
- int version, int index)
+struct mtk_foe_accounting *mtk_foe_entry_get_mib(struct mtk_ppe *ppe, u32 index,
+ struct mtk_foe_accounting *diff)
+{
+ struct mtk_foe_accounting *acct;
+ int size = sizeof(struct mtk_foe_accounting);
+ u64 bytes, packets;
+
+ if (!ppe->accounting)
+ return NULL;
+
+ if (mtk_mib_entry_read(ppe, index, &bytes, &packets))
+ return NULL;
+
+ acct = ppe->acct_table + index * size;
+
+ acct->bytes += bytes;
+ acct->packets += packets;
+
+ if (diff) {
+ diff->bytes = bytes;
+ diff->packets = packets;
+ }
+
+ return acct;
+}
+
+struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base, int index)
{
+ bool accounting = eth->soc->has_accounting;
const struct mtk_soc_data *soc = eth->soc;
+ struct mtk_foe_accounting *acct;
struct device *dev = eth->dev;
+ struct mtk_mib_entry *mib;
struct mtk_ppe *ppe;
u32 foe_flow_size;
void *foe;
@@ -777,7 +857,8 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
ppe->base = base;
ppe->eth = eth;
ppe->dev = dev;
- ppe->version = version;
+ ppe->version = eth->soc->offload_version;
+ ppe->accounting = accounting;

foe = dmam_alloc_coherent(ppe->dev,
MTK_PPE_ENTRIES * soc->foe_entry_size,
@@ -793,6 +874,23 @@ 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;
+
+ 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)
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.h b/drivers/net/ethernet/mediatek/mtk_ppe.h
index 5e8bc48252b1..e1aab2e8e262 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.h
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.h
@@ -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;

@@ -303,8 +322,8 @@ struct mtk_ppe {
void *acct_table;
};

-struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
- int version, int index);
+struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base, int index);
+
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 +378,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
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c b/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
index 391b071bcff3..53cf87e9acbb 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
@@ -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=%llu bytes=%llu\n",
h_source, h_dest, ntohs(l2->etype),
- l2->vlan1, l2->vlan2, entry->ib1, ib2);
+ l2->vlan1, l2->vlan2, entry->ib1, ib2,
+ acct ? acct->packets : 0, acct ? acct->bytes : 0);
}

return 0;
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 81afd5ee3fbf..f02ccffb1e79 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -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(&eth->flow_table, &f->cookie,
@@ -507,6 +508,13 @@ 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;
}

diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_regs.h b/drivers/net/ethernet/mediatek/mtk_ppe_regs.h
index 0fdb983b0a88..a2e61b3eb006 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_regs.h
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_regs.h
@@ -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)
--
2.39.1


2023-02-07 14:25:02

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 09/11] net: pcs: add driver for MediaTek SGMII PCS

The 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.

Signed-off-by: Daniel Golle <[email protected]>
---
MAINTAINERS | 7 +
drivers/net/pcs/Kconfig | 8 +
drivers/net/pcs/Makefile | 1 +
drivers/net/pcs/pcs-mtk-lynxi.c | 315 ++++++++++++++++++++++++++++++
include/linux/pcs/pcs-mtk-lynxi.h | 13 ++
5 files changed, 344 insertions(+)
create mode 100644 drivers/net/pcs/pcs-mtk-lynxi.c
create mode 100644 include/linux/pcs/pcs-mtk-lynxi.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f76ca2808474..41f4de91a434 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12998,6 +12998,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-lynxi.c
+F: include/linux/pcs/pcs-mtk-lynxi.h
+
MEDIATEK I2C CONTROLLER DRIVER
M: Qii Wang <[email protected]>
L: [email protected]
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 6e7e6c346a3e..9b792b61c768 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -18,6 +18,14 @@ 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_LYNXI
+ tristate
+ select PHYLINK
+ select REGMAP
+ 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)
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4c780d8f2e98..9b9afd6b1c22 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -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_LYNXI) += pcs-mtk-lynxi.o
obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
obj-$(CONFIG_PCS_ALTERA_TSE) += pcs-altera-tse.o
diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
new file mode 100644
index 000000000000..0100def53d45
--- /dev/null
+++ b/drivers/net/pcs/pcs-mtk-lynxi.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018-2019 MediaTek Inc.
+/* A library for MediaTek SGMII circuit
+ *
+ * Author: Sean Wang <[email protected]>
+ * Author: Daniel Golle <[email protected]>
+ *
+ */
+#include <linux/mdio.h>
+#include <linux/phylink.h>
+#include <linux/pcs/pcs-mtk-lynxi.h>
+#include <linux/of.h>
+#include <linux/phylink.h>
+#include <linux/regmap.h>
+
+/* SGMII subsystem config registers */
+/* BMCR (low 16) BMSR (high 16) */
+#define SGMSYS_PCS_CONTROL_1 0x0
+#define SGMII_BMCR GENMASK(15, 0)
+#define SGMII_BMSR GENMASK(31, 16)
+#define SGMII_AN_RESTART BIT(9)
+#define SGMII_ISOLATE BIT(10)
+#define SGMII_AN_ENABLE BIT(12)
+#define SGMII_LINK_STATYS BIT(18)
+#define SGMII_AN_ABILITY BIT(19)
+#define SGMII_AN_COMPLETE BIT(21)
+#define SGMII_PCS_FAULT BIT(23)
+#define SGMII_AN_EXPANSION_CLR BIT(30)
+
+#define SGMSYS_PCS_DEVICE_ID 0x4
+#define SGMII_LYNXI_DEV_ID 0x4d544950
+
+#define SGMSYS_PCS_ADVERTISE 0x8
+#define SGMII_ADVERTISE GENMASK(15, 0)
+#define SGMII_LPA GENMASK(31, 16)
+
+#define SGMSYS_PCS_SCRATCH 0x14
+#define SGMII_DEV_VERSION GENMASK(31, 16)
+
+/* Register to programmable link timer, the unit in 2 * 8ns */
+#define SGMSYS_PCS_LINK_TIMER 0x18
+#define SGMII_LINK_TIMER_MASK GENMASK(19, 0)
+#define SGMII_LINK_TIMER_DEFAULT (0x186a0 & SGMII_LINK_TIMER_MASK)
+
+/* Register to control remote fault */
+#define SGMSYS_SGMII_MODE 0x20
+#define SGMII_IF_MODE_SGMII BIT(0)
+#define SGMII_SPEED_DUPLEX_AN BIT(1)
+#define SGMII_SPEED_MASK GENMASK(3, 2)
+#define SGMII_SPEED_10 FIELD_PREP(SGMII_SPEED_MASK, 0)
+#define SGMII_SPEED_100 FIELD_PREP(SGMII_SPEED_MASK, 1)
+#define SGMII_SPEED_1000 FIELD_PREP(SGMII_SPEED_MASK, 2)
+#define SGMII_DUPLEX_HALF BIT(4)
+#define SGMII_REMOTE_FAULT_DIS BIT(8)
+#define SGMII_CODE_SYNC_SET_VAL BIT(9)
+#define SGMII_CODE_SYNC_SET_EN BIT(10)
+#define SGMII_SEND_AN_ERROR_EN BIT(11)
+
+/* 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))
+#define RG_PHY_SPEED_1_25G 0x0
+#define RG_PHY_SPEED_3_125G BIT(2)
+
+/* Register to power up QPHY */
+#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))
+
+/* struct mtk_pcs_lynxi - This structure holds each sgmii regmap andassociated
+ * data
+ * @regmap: The register map pointing at the range used to setup
+ * SGMII modes
+ * @dev: Pointer to device owning the PCS
+ * @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_lynxi {
+ struct regmap *regmap;
+ struct device *dev;
+ u32 ana_rgc3;
+ phy_interface_t interface;
+ struct phylink_pcs pcs;
+ u32 flags;
+};
+
+static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs)
+{
+ return container_of(pcs, struct mtk_pcs_lynxi, pcs);
+}
+
+static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs,
+ struct phylink_link_state *state)
+{
+ struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+ unsigned int bm, adv;
+
+ /* Read the BMSR and LPA */
+ regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
+ regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
+
+ phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
+ FIELD_GET(SGMII_LPA, adv));
+}
+
+static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit_pause_to_mac)
+{
+ struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+ unsigned int rgc3, sgm_mode, bmcr;
+ int advertise, link_timer;
+ bool mode_changed = false, changed, use_an;
+
+ advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
+ advertising);
+ if (advertise < 0)
+ return advertise;
+
+ /* 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).
+ */
+ if (interface == PHY_INTERFACE_MODE_SGMII) {
+ sgm_mode = SGMII_IF_MODE_SGMII;
+ if (phylink_autoneg_inband(mode)) {
+ sgm_mode |= SGMII_REMOTE_FAULT_DIS |
+ SGMII_SPEED_DUPLEX_AN;
+ use_an = true;
+ } else {
+ use_an = false;
+ }
+ } else if (phylink_autoneg_inband(mode)) {
+ /* 1000base-X or 2500base-X autoneg */
+ sgm_mode = SGMII_REMOTE_FAULT_DIS;
+ use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+ advertising);
+ } else {
+ /* 1000base-X or 2500base-X without autoneg */
+ sgm_mode = 0;
+ use_an = false;
+ }
+
+ if (use_an)
+ bmcr = SGMII_AN_ENABLE;
+ else
+ bmcr = 0;
+
+ if (mpcs->interface != interface) {
+ /* PHYA power down */
+ 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,
+ SGMII_PN_SWAP_TX_RX);
+
+ if (interface == PHY_INTERFACE_MODE_2500BASEX)
+ rgc3 = RG_PHY_SPEED_3_125G;
+ else
+ rgc3 = 0;
+
+ /* Configure the underlying interface speed */
+ regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
+ RG_PHY_SPEED_MASK, 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);
+
+ /* Update the sgmsys mode register */
+ regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
+ SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
+ SGMII_IF_MODE_SGMII, sgm_mode);
+
+ /* Update the BMCR */
+ regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
+ SGMII_AN_ENABLE, bmcr);
+
+ /* Release PHYA power down state
+ * Only removing bit SGMII_PHYA_PWD isn't enough.
+ * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
+ * prevents SGMII from working. The SGMII still shows link but no traffic
+ * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
+ * taken from a good working state of the SGMII interface.
+ * Unknown how much the QPHY needs but it is racy without a sleep.
+ * Tested on mt7622 & mt7986.
+ */
+ usleep_range(50, 100);
+ regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
+
+ return changed || mode_changed;
+}
+
+static void mtk_pcs_lynxi_restart_an(struct phylink_pcs *pcs)
+{
+ struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+
+ regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
+ SGMII_AN_RESTART, SGMII_AN_RESTART);
+}
+
+static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface, int speed, int duplex)
+{
+ struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+ unsigned int sgm_mode;
+
+ if (!phylink_autoneg_inband(mode)) {
+ /* Force the speed and duplex setting */
+ if (speed == SPEED_10)
+ sgm_mode = SGMII_SPEED_10;
+ else if (speed == SPEED_100)
+ sgm_mode = SGMII_SPEED_100;
+ else
+ sgm_mode = SGMII_SPEED_1000;
+
+ if (duplex != DUPLEX_FULL)
+ sgm_mode |= SGMII_DUPLEX_HALF;
+
+ regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
+ SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
+ sgm_mode);
+ }
+}
+
+static const struct phylink_pcs_ops mtk_pcs_lynxi_ops = {
+ .pcs_get_state = mtk_pcs_lynxi_get_state,
+ .pcs_config = mtk_pcs_lynxi_config,
+ .pcs_an_restart = mtk_pcs_lynxi_restart_an,
+ .pcs_link_up = mtk_pcs_lynxi_link_up,
+};
+
+struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev, struct regmap *regmap,
+ u32 ana_rgc3, u32 flags)
+{
+ struct mtk_pcs_lynxi *mpcs;
+ u32 id, ver;
+ int ret;
+
+ ret = regmap_read(regmap, SGMSYS_PCS_DEVICE_ID, &id);
+ if (ret < 0)
+ return NULL;
+
+ if (id != SGMII_LYNXI_DEV_ID) {
+ dev_err(dev, "unknown PCS device id %08x\n", id);
+ return NULL;
+ }
+
+ ret = regmap_read(regmap, SGMSYS_PCS_SCRATCH, &ver);
+ if (ret < 0)
+ return NULL;
+
+ ver = FIELD_GET(SGMII_DEV_VERSION, ver);
+ if (ver != 0x1) {
+ dev_err(dev, "unknown PCS device version %04x\n", ver);
+ return NULL;
+ }
+
+ dev_dbg(dev, "MediaTek LynxI SGMII PCS (id 0x%08x, ver 0x%04x)\n", id,
+ ver);
+
+ mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
+ if (!mpcs)
+ return NULL;
+
+ mpcs->dev = dev;
+ mpcs->ana_rgc3 = ana_rgc3;
+ mpcs->regmap = regmap;
+ mpcs->flags = flags;
+ mpcs->pcs.ops = &mtk_pcs_lynxi_ops;
+ mpcs->pcs.poll = true;
+ mpcs->interface = PHY_INTERFACE_MODE_NA;
+
+ return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_create);
+
+void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs)
+{
+ if (!pcs)
+ return;
+
+ kfree(pcs_to_mtk_pcs_lynxi(pcs));
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_destroy);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs/pcs-mtk-lynxi.h b/include/linux/pcs/pcs-mtk-lynxi.h
new file mode 100644
index 000000000000..be3b4ab32f4a
--- /dev/null
+++ b/include/linux/pcs/pcs-mtk-lynxi.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PCS_MTK_LYNXI_H
+#define __LINUX_PCS_MTK_LYNXI_H
+
+#include <linux/phylink.h>
+#include <linux/regmap.h>
+
+#define MTK_SGMII_FLAG_PN_SWAP BIT(0)
+struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
+ struct regmap *regmap,
+ u32 ana_rgc3, u32 flags);
+void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs);
+#endif
--
2.39.1


2023-02-07 14:25:50

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 10/11] net: ethernet: mtk_eth_soc: switch to external PCS driver

Now that we got a PCS driver, use it and remove the now redundant
PCS code and it's header macros from the Ethernet driver.

Signed-off-by: Daniel Golle <[email protected]>
---
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 | 202 +++-----------------
4 files changed, 38 insertions(+), 259 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig
index 97374fb3ee79..da0db417ab69 100644
--- a/drivers/net/ethernet/mediatek/Kconfig
+++ b/drivers/net/ethernet/mediatek/Kconfig
@@ -19,6 +19,8 @@ config NET_MEDIATEK_SOC
select DIMLIB
select PAGE_POOL
select PAGE_POOL_STATS
+ select PCS_MTK_LYNXI
+ select REGMAP_MMIO
help
This driver supports the gigabit ethernet MACs in the
MediaTek SoC family.
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 97df77c7999e..54e85f54d7fc 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4077,6 +4077,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(&eth->pending_work);
@@ -4580,6 +4581,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);

@@ -4592,14 +4594,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);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index c2e0fd773cc2..a72748d80bba 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -510,65 +510,6 @@
#define ETHSYS_DMA_AG_MAP_QDMA BIT(1)
#define ETHSYS_DMA_AG_MAP_PPE BIT(2)

-/* SGMII subsystem config registers */
-/* BMCR (low 16) BMSR (high 16) */
-#define SGMSYS_PCS_CONTROL_1 0x0
-#define SGMII_BMCR GENMASK(15, 0)
-#define SGMII_BMSR GENMASK(31, 16)
-#define SGMII_AN_RESTART BIT(9)
-#define SGMII_ISOLATE BIT(10)
-#define SGMII_AN_ENABLE BIT(12)
-#define SGMII_LINK_STATYS BIT(18)
-#define SGMII_AN_ABILITY BIT(19)
-#define SGMII_AN_COMPLETE BIT(21)
-#define SGMII_PCS_FAULT BIT(23)
-#define SGMII_AN_EXPANSION_CLR BIT(30)
-
-#define SGMSYS_PCS_ADVERTISE 0x8
-#define SGMII_ADVERTISE GENMASK(15, 0)
-#define SGMII_LPA GENMASK(31, 16)
-
-/* Register to programmable link timer, the unit in 2 * 8ns */
-#define SGMSYS_PCS_LINK_TIMER 0x18
-#define SGMII_LINK_TIMER_MASK GENMASK(19, 0)
-#define SGMII_LINK_TIMER_DEFAULT (0x186a0 & SGMII_LINK_TIMER_MASK)
-
-/* Register to control remote fault */
-#define SGMSYS_SGMII_MODE 0x20
-#define SGMII_IF_MODE_SGMII BIT(0)
-#define SGMII_SPEED_DUPLEX_AN BIT(1)
-#define SGMII_SPEED_MASK GENMASK(3, 2)
-#define SGMII_SPEED_10 FIELD_PREP(SGMII_SPEED_MASK, 0)
-#define SGMII_SPEED_100 FIELD_PREP(SGMII_SPEED_MASK, 1)
-#define SGMII_SPEED_1000 FIELD_PREP(SGMII_SPEED_MASK, 2)
-#define SGMII_DUPLEX_HALF BIT(4)
-#define SGMII_IF_MODE_BIT5 BIT(5)
-#define SGMII_REMOTE_FAULT_DIS BIT(8)
-#define SGMII_CODE_SYNC_SET_VAL BIT(9)
-#define SGMII_CODE_SYNC_SET_EN BIT(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))
-#define RG_PHY_SPEED_1_25G 0x0
-#define RG_PHY_SPEED_3_125G BIT(2)
-
-/* Register to power up QPHY */
-#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)
@@ -1103,29 +1044,13 @@ struct mtk_soc_data {
/* currently no SoC has more than 2 macs */
#define MTK_MAX_DEVS 2

-/* struct mtk_pcs - This structure holds each sgmii regmap and associated
- * data
- * @regmap: The register map pointing at the range used to setup
- * SGMII modes
- * @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
* characteristics
* @pcs Array of individual PCS structures
*/
struct mtk_sgmii {
- struct mtk_pcs pcs[MTK_MAX_DEVS];
+ struct phylink_pcs *pcs[MTK_MAX_DEVS];
+ struct device *dev;
};

/* struct mtk_eth - This is the main datasructure for holding the state
@@ -1353,6 +1278,7 @@ u32 mtk_r32(struct mtk_eth *eth, unsigned reg);
struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id);
int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np,
u32 ana_rgc3);
+void mtk_sgmii_destroy(struct mtk_sgmii *ss);

int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id);
int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 9c58006d1c33..d4b428e23cfc 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -10,199 +10,35 @@
#include <linux/mfd/syscon.h>
#include <linux/of.h>
#include <linux/phylink.h>
+#include <linux/pcs/pcs-mtk-lynxi.h>
#include <linux/regmap.h>

#include "mtk_eth_soc.h"

-static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
-{
- return container_of(pcs, struct mtk_pcs, pcs);
-}
-
-static void mtk_pcs_get_state(struct phylink_pcs *pcs,
- struct phylink_link_state *state)
-{
- struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
- unsigned int bm, adv;
-
- /* Read the BMSR and LPA */
- regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
- regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
-
- phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
- FIELD_GET(SGMII_LPA, adv));
-}
-
-static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
- phy_interface_t interface,
- const unsigned long *advertising,
- bool permit_pause_to_mac)
-{
- bool mode_changed = false, changed, use_an;
- struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
- unsigned int rgc3, sgm_mode, bmcr;
- int advertise, link_timer;
-
- advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
- advertising);
- if (advertise < 0)
- return advertise;
-
- /* 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).
- */
- if (interface == PHY_INTERFACE_MODE_SGMII) {
- sgm_mode = SGMII_IF_MODE_SGMII;
- if (phylink_autoneg_inband(mode)) {
- sgm_mode |= SGMII_REMOTE_FAULT_DIS |
- SGMII_SPEED_DUPLEX_AN;
- use_an = true;
- } else {
- use_an = false;
- }
- } else if (phylink_autoneg_inband(mode)) {
- /* 1000base-X or 2500base-X autoneg */
- sgm_mode = SGMII_REMOTE_FAULT_DIS;
- use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
- advertising);
- } else {
- /* 1000base-X or 2500base-X without autoneg */
- sgm_mode = 0;
- use_an = false;
- }
-
- if (use_an) {
- bmcr = SGMII_AN_ENABLE;
- } else {
- bmcr = 0;
- }
-
- if (mpcs->interface != interface) {
- /* PHYA power down */
- 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,
- SGMII_PN_SWAP_TX_RX);
-
- if (interface == PHY_INTERFACE_MODE_2500BASEX)
- rgc3 = RG_PHY_SPEED_3_125G;
- else
- rgc3 = 0;
-
- /* Configure the underlying interface speed */
- 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);
-
- /* Update the sgmsys mode register */
- regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
- SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
- SGMII_IF_MODE_SGMII, sgm_mode);
-
- /* Update the BMCR */
- regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
- SGMII_AN_ENABLE, bmcr);
-
- /* Release PHYA power down state
- * Only removing bit SGMII_PHYA_PWD isn't enough.
- * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
- * prevents SGMII from working. The SGMII still shows link but no traffic
- * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
- * taken from a good working state of the SGMII interface.
- * Unknown how much the QPHY needs but it is racy without a sleep.
- * Tested on mt7622 & mt7986.
- */
- usleep_range(50, 100);
- regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
-
- return changed || mode_changed;
-}
-
-static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
-{
- struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-
- regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
- SGMII_AN_RESTART, SGMII_AN_RESTART);
-}
-
-static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
- phy_interface_t interface, int speed, int duplex)
-{
- struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
- unsigned int sgm_mode;
-
- if (!phylink_autoneg_inband(mode)) {
- /* Force the speed and duplex setting */
- if (speed == SPEED_10)
- sgm_mode = SGMII_SPEED_10;
- else if (speed == SPEED_100)
- sgm_mode = SGMII_SPEED_100;
- else
- sgm_mode = SGMII_SPEED_1000;
-
- if (duplex != DUPLEX_FULL)
- sgm_mode |= SGMII_DUPLEX_HALF;
-
- regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
- SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
- sgm_mode);
- }
-}
-
-static const struct phylink_pcs_ops mtk_pcs_ops = {
- .pcs_get_state = mtk_pcs_get_state,
- .pcs_config = mtk_pcs_config,
- .pcs_an_restart = mtk_pcs_restart_an,
- .pcs_link_up = mtk_pcs_link_up,
-};
-
int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
{
struct device_node *np;
+ struct regmap *regmap;
+ u32 flags;
int i;

- for (i = 0; i < MTK_MAX_DEVS; i++) {
+ for (i = 0; id < 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;
+ regmap = syscon_node_to_regmap(np);
+ flags = 0;
if (of_property_read_bool(np, "mediatek,pn_swap"))
- ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
+ 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;
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ ss->pcs[i] = mtk_pcs_lynxi_create(ss->dev, regmap, ana_rgc3,
+ flags);
}

return 0;
@@ -210,8 +46,16 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)

struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id)
{
- if (!ss->pcs[id].regmap)
- return NULL;
+ return ss->pcs[id];
+}
+
+void mtk_sgmii_destroy(struct mtk_sgmii *ss)
+{
+ int i;
+
+ if (!ss)
+ return;

- return &ss->pcs[id].pcs;
+ for (i = 0; i < MTK_MAX_DEVS; i++)
+ mtk_pcs_lynxi_destroy(ss->pcs[i]);
}
--
2.39.1


2023-02-07 14:26:14

by Daniel Golle

[permalink] [raw]
Subject: [PATCH v2 11/11] net: dsa: mt7530: use external PCS driver

Implement 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.

Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/dsa/Kconfig | 1 +
drivers/net/dsa/mt7530.c | 277 ++++++++++-----------------------------
drivers/net/dsa/mt7530.h | 47 +------
3 files changed, 71 insertions(+), 254 deletions(-)

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index f6f3b43dfb06..6b45fa8b6907 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -38,6 +38,7 @@ config NET_DSA_MT7530
tristate "MediaTek MT7530 and MT7531 Ethernet switch support"
select NET_DSA_TAG_MTK
select MEDIATEK_GE_PHY
+ select PCS_MTK_LYNXI
help
This enables support for the MediaTek MT7530 and MT7531 Ethernet
switch chips. Multi-chip module MT7530 in MT7621AT, MT7621DAT,
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 616b21c90d05..af4a7168107a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -14,6 +14,7 @@
#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <linux/of_platform.h>
+#include <linux/pcs/pcs-mtk-lynxi.h>
#include <linux/phylink.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
@@ -2555,128 +2556,11 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
return 0;
}

-static void mt7531_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
- phy_interface_t interface, int speed, int duplex)
-{
- struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
- int port = pcs_to_mt753x_pcs(pcs)->port;
- unsigned int val;
-
- /* For adjusting speed and duplex of SGMII force mode. */
- if (interface != PHY_INTERFACE_MODE_SGMII ||
- phylink_autoneg_inband(mode))
- return;
-
- /* SGMII force mode setting */
- val = mt7530_read(priv, MT7531_SGMII_MODE(port));
- val &= ~MT7531_SGMII_IF_MODE_MASK;
-
- switch (speed) {
- case SPEED_10:
- val |= MT7531_SGMII_FORCE_SPEED_10;
- break;
- case SPEED_100:
- val |= MT7531_SGMII_FORCE_SPEED_100;
- break;
- case SPEED_1000:
- val |= MT7531_SGMII_FORCE_SPEED_1000;
- break;
- }
-
- /* MT7531 SGMII 1G force mode can only work in full duplex mode,
- * no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
- *
- * The speed check is unnecessary as the MAC capabilities apply
- * this restriction. --rmk
- */
- if ((speed == SPEED_10 || speed == SPEED_100) &&
- duplex != DUPLEX_FULL)
- val |= MT7531_SGMII_FORCE_HALF_DUPLEX;
-
- mt7530_write(priv, MT7531_SGMII_MODE(port), val);
-}
-
static bool mt753x_is_mac_port(u32 port)
{
return (port == 5 || port == 6);
}

-static int mt7531_sgmii_setup_mode_force(struct mt7530_priv *priv, u32 port,
- phy_interface_t interface)
-{
- u32 val;
-
- if (!mt753x_is_mac_port(port))
- return -EINVAL;
-
- mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
- MT7531_SGMII_PHYA_PWD);
-
- val = mt7530_read(priv, MT7531_PHYA_CTRL_SIGNAL3(port));
- val &= ~MT7531_RG_TPHY_SPEED_MASK;
- /* Setup 2.5 times faster clock for 2.5Gbps data speeds with 10B/8B
- * encoding.
- */
- val |= (interface == PHY_INTERFACE_MODE_2500BASEX) ?
- MT7531_RG_TPHY_SPEED_3_125G : MT7531_RG_TPHY_SPEED_1_25G;
- mt7530_write(priv, MT7531_PHYA_CTRL_SIGNAL3(port), val);
-
- mt7530_clear(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
-
- /* MT7531 SGMII 1G and 2.5G force mode can only work in full duplex
- * mode, no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
- */
- mt7530_rmw(priv, MT7531_SGMII_MODE(port),
- MT7531_SGMII_IF_MODE_MASK | MT7531_SGMII_REMOTE_FAULT_DIS,
- MT7531_SGMII_FORCE_SPEED_1000);
-
- mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
-
- return 0;
-}
-
-static int mt7531_sgmii_setup_mode_an(struct mt7530_priv *priv, int port,
- phy_interface_t interface)
-{
- if (!mt753x_is_mac_port(port))
- return -EINVAL;
-
- mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
- MT7531_SGMII_PHYA_PWD);
-
- mt7530_rmw(priv, MT7531_PHYA_CTRL_SIGNAL3(port),
- MT7531_RG_TPHY_SPEED_MASK, MT7531_RG_TPHY_SPEED_1_25G);
-
- mt7530_set(priv, MT7531_SGMII_MODE(port),
- MT7531_SGMII_REMOTE_FAULT_DIS |
- MT7531_SGMII_SPEED_DUPLEX_AN);
-
- mt7530_rmw(priv, MT7531_PCS_SPEED_ABILITY(port),
- MT7531_SGMII_TX_CONFIG_MASK, 1);
-
- mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
-
- mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_RESTART);
-
- mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
-
- return 0;
-}
-
-static void mt7531_pcs_an_restart(struct phylink_pcs *pcs)
-{
- struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
- int port = pcs_to_mt753x_pcs(pcs)->port;
- u32 val;
-
- /* Only restart AN when AN is enabled */
- val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
- if (val & MT7531_SGMII_AN_ENABLE) {
- val |= MT7531_SGMII_AN_RESTART;
- mt7530_write(priv, MT7531_PCS_CONTROL_1(port), val);
- }
-}
-
static int
mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
@@ -2699,11 +2583,11 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phydev = dp->slave->phydev;
return mt7531_rgmii_setup(priv, port, interface, phydev);
case PHY_INTERFACE_MODE_SGMII:
- return mt7531_sgmii_setup_mode_an(priv, port, interface);
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_1000BASEX:
case PHY_INTERFACE_MODE_2500BASEX:
- return mt7531_sgmii_setup_mode_force(priv, port, interface);
+ /* handled in SGMII PCS driver */
+ return 0;
default:
return -EINVAL;
}
@@ -2728,11 +2612,11 @@ 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;
-
+ return priv->ports[port].sgmii_pcs;
default:
return NULL;
}
@@ -2970,86 +2854,6 @@ static void mt7530_pcs_get_state(struct phylink_pcs *pcs,
state->pause |= MLO_PAUSE_TX;
}

-static int
-mt7531_sgmii_pcs_get_state_an(struct mt7530_priv *priv, int port,
- struct phylink_link_state *state)
-{
- u32 status, val;
- u16 config_reg;
-
- status = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
- state->link = !!(status & MT7531_SGMII_LINK_STATUS);
- state->an_complete = !!(status & MT7531_SGMII_AN_COMPLETE);
- if (state->interface == PHY_INTERFACE_MODE_SGMII &&
- (status & MT7531_SGMII_AN_ENABLE)) {
- val = mt7530_read(priv, MT7531_PCS_SPEED_ABILITY(port));
- config_reg = val >> 16;
-
- switch (config_reg & LPA_SGMII_SPD_MASK) {
- case LPA_SGMII_1000:
- state->speed = SPEED_1000;
- break;
- case LPA_SGMII_100:
- state->speed = SPEED_100;
- break;
- case LPA_SGMII_10:
- state->speed = SPEED_10;
- break;
- default:
- dev_err(priv->dev, "invalid sgmii PHY speed\n");
- state->link = false;
- return -EINVAL;
- }
-
- if (config_reg & LPA_SGMII_FULL_DUPLEX)
- state->duplex = DUPLEX_FULL;
- else
- state->duplex = DUPLEX_HALF;
- }
-
- return 0;
-}
-
-static void
-mt7531_sgmii_pcs_get_state_inband(struct mt7530_priv *priv, int port,
- struct phylink_link_state *state)
-{
- unsigned int val;
-
- val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
- state->link = !!(val & MT7531_SGMII_LINK_STATUS);
- if (!state->link)
- return;
-
- state->an_complete = state->link;
-
- if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
- state->speed = SPEED_2500;
- else
- state->speed = SPEED_1000;
-
- state->duplex = DUPLEX_FULL;
- state->pause = MLO_PAUSE_NONE;
-}
-
-static void mt7531_pcs_get_state(struct phylink_pcs *pcs,
- struct phylink_link_state *state)
-{
- struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
- int port = pcs_to_mt753x_pcs(pcs)->port;
-
- if (state->interface == PHY_INTERFACE_MODE_SGMII) {
- mt7531_sgmii_pcs_get_state_an(priv, port, state);
- return;
- } else if ((state->interface == PHY_INTERFACE_MODE_1000BASEX) ||
- (state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
- mt7531_sgmii_pcs_get_state_inband(priv, port, state);
- return;
- }
-
- state->link = false;
-}
-
static int mt753x_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
phy_interface_t interface,
const unsigned long *advertising,
@@ -3069,18 +2873,57 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
.pcs_an_restart = mt7530_pcs_an_restart,
};

-static const struct phylink_pcs_ops mt7531_pcs_ops = {
- .pcs_validate = mt753x_pcs_validate,
- .pcs_get_state = mt7531_pcs_get_state,
- .pcs_config = mt753x_pcs_config,
- .pcs_an_restart = mt7531_pcs_an_restart,
- .pcs_link_up = mt7531_pcs_link_up,
+static int mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct mt7530_priv *priv = context;
+
+ *val = mt7530_read(priv, reg);
+ return 0;
+};
+
+static int mt7530_regmap_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct mt7530_priv *priv = context;
+
+ mt7530_write(priv, reg, val);
+ return 0;
+};
+
+static int mt7530_regmap_update_bits(void *context, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ struct mt7530_priv *priv = context;
+
+ mt7530_rmw(priv, reg, mask, val);
+ return 0;
+};
+
+static const struct regmap_bus mt7531_regmap_bus = {
+ .reg_write = mt7530_regmap_write,
+ .reg_read = mt7530_regmap_read,
+ .reg_update_bits = mt7530_regmap_update_bits,
+};
+
+#define MT7531_PCS_REGMAP_CONFIG(_name, _reg_base) \
+ { \
+ .name = _name, \
+ .reg_bits = 16, \
+ .val_bits = 32, \
+ .reg_stride = 4, \
+ .reg_base = _reg_base, \
+ .max_register = 0x17c, \
+ }
+
+static const struct regmap_config mt7531_pcs_config[] = {
+ MT7531_PCS_REGMAP_CONFIG("port5", MT7531_SGMII_REG_BASE(5)),
+ MT7531_PCS_REGMAP_CONFIG("port6", MT7531_SGMII_REG_BASE(6)),
};

static int
mt753x_setup(struct dsa_switch *ds)
{
struct mt7530_priv *priv = ds->priv;
+ struct regmap *regmap;
int i, ret;

/* Initialise the PCS devices */
@@ -3088,8 +2931,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);
@@ -3104,6 +2945,16 @@ mt753x_setup(struct dsa_switch *ds)
if (ret && priv->irq)
mt7530_free_irq_common(priv);

+ if (priv->id == ID_MT7531)
+ for (i = 0; i < 2; i++) {
+ regmap = devm_regmap_init(ds->dev,
+ &mt7531_regmap_bus, priv,
+ &mt7531_pcs_config[i]);
+ priv->ports[5 + i].sgmii_pcs =
+ mtk_pcs_lynxi_create(ds->dev, regmap,
+ MT7531_PHYA_CTRL_SIGNAL3, 0);
+ }
+
return ret;
}

@@ -3199,7 +3050,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 +3160,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 +3179,10 @@ mt7530_remove(struct mdio_device *mdiodev)
mt7530_free_irq(priv);

dsa_unregister_switch(priv->ds);
+
+ for (i = 0; i < 2; ++i)
+ mtk_pcs_lynxi_destroy(priv->ports[5 + i].sgmii_pcs);
+
mutex_destroy(&priv->reg_mutex);
}

diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 6b2fc6290ea8..c5d29f3fc1d8 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -364,47 +364,8 @@ enum mt7530_vlan_port_acc_frm {
CCR_TX_OCT_CNT_BAD)

/* MT7531 SGMII register group */
-#define MT7531_SGMII_REG_BASE 0x5000
-#define MT7531_SGMII_REG(p, r) (MT7531_SGMII_REG_BASE + \
- ((p) - 5) * 0x1000 + (r))
-
-/* Register forSGMII PCS_CONTROL_1 */
-#define MT7531_PCS_CONTROL_1(p) MT7531_SGMII_REG(p, 0x00)
-#define MT7531_SGMII_LINK_STATUS BIT(18)
-#define MT7531_SGMII_AN_ENABLE BIT(12)
-#define MT7531_SGMII_AN_RESTART BIT(9)
-#define MT7531_SGMII_AN_COMPLETE BIT(21)
-
-/* Register for SGMII PCS_SPPED_ABILITY */
-#define MT7531_PCS_SPEED_ABILITY(p) MT7531_SGMII_REG(p, 0x08)
-#define MT7531_SGMII_TX_CONFIG_MASK GENMASK(15, 0)
-#define MT7531_SGMII_TX_CONFIG BIT(0)
-
-/* Register for SGMII_MODE */
-#define MT7531_SGMII_MODE(p) MT7531_SGMII_REG(p, 0x20)
-#define MT7531_SGMII_REMOTE_FAULT_DIS BIT(8)
-#define MT7531_SGMII_IF_MODE_MASK GENMASK(5, 1)
-#define MT7531_SGMII_FORCE_DUPLEX BIT(4)
-#define MT7531_SGMII_FORCE_SPEED_MASK GENMASK(3, 2)
-#define MT7531_SGMII_FORCE_SPEED_1000 BIT(3)
-#define MT7531_SGMII_FORCE_SPEED_100 BIT(2)
-#define MT7531_SGMII_FORCE_SPEED_10 0
-#define MT7531_SGMII_SPEED_DUPLEX_AN BIT(1)
-
-enum mt7531_sgmii_force_duplex {
- MT7531_SGMII_FORCE_FULL_DUPLEX = 0,
- MT7531_SGMII_FORCE_HALF_DUPLEX = 0x10,
-};
-
-/* Fields of QPHY_PWR_STATE_CTRL */
-#define MT7531_QPHY_PWR_STATE_CTRL(p) MT7531_SGMII_REG(p, 0xe8)
-#define MT7531_SGMII_PHYA_PWD BIT(4)
-
-/* Values of SGMII SPEED */
-#define MT7531_PHYA_CTRL_SIGNAL3(p) MT7531_SGMII_REG(p, 0x128)
-#define MT7531_RG_TPHY_SPEED_MASK (BIT(2) | BIT(3))
-#define MT7531_RG_TPHY_SPEED_1_25G 0x0
-#define MT7531_RG_TPHY_SPEED_3_125G BIT(2)
+#define MT7531_SGMII_REG_BASE(p) (0x5000 + ((p) - 5) * 0x1000)
+#define MT7531_PHYA_CTRL_SIGNAL3 0x128

/* Register for system reset */
#define MT7530_SYS_CTRL 0x7000
@@ -703,13 +664,13 @@ struct mt7530_fdb {
* @pm: The matrix used to show all connections with the port.
* @pvid: The VLAN specified is to be considered a PVID at ingress. Any
* untagged frames will be assigned to the related VLAN.
- * @vlan_filtering: The flags indicating whether the port that can recognize
- * VLAN-tagged frames.
+ * @sgmii_pcs: Pointer to PCS instance for SerDes ports
*/
struct mt7530_port {
bool enable;
u32 pm;
u16 pvid;
+ struct phylink_pcs *sgmii_pcs;
};

/* Port 5 interface select definitions */
--
2.39.1


2023-02-07 14:35:26

by Daniel Golle

[permalink] [raw]
Subject: Re: [PATCH v2 10/11] net: ethernet: mtk_eth_soc: switch to external PCS driver

On Tue, Feb 07, 2023 at 02:23:48PM +0000, Daniel Golle wrote:
> Now that we got a PCS driver, use it and remove the now redundant
> PCS code and it's header macros from the Ethernet driver.
>
> Signed-off-by: Daniel Golle <[email protected]>
> ---
> 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 | 202 +++-----------------
> 4 files changed, 38 insertions(+), 259 deletions(-)
>
> diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig
> index 97374fb3ee79..da0db417ab69 100644
> --- a/drivers/net/ethernet/mediatek/Kconfig
> +++ b/drivers/net/ethernet/mediatek/Kconfig
> @@ -19,6 +19,8 @@ config NET_MEDIATEK_SOC
> select DIMLIB
> select PAGE_POOL
> select PAGE_POOL_STATS
> + select PCS_MTK_LYNXI
> + select REGMAP_MMIO
> help
> This driver supports the gigabit ethernet MACs in the
> MediaTek SoC family.
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 97df77c7999e..54e85f54d7fc 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -4077,6 +4077,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(&eth->pending_work);
> @@ -4580,6 +4581,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);
>
> @@ -4592,14 +4594,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);
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index c2e0fd773cc2..a72748d80bba 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -510,65 +510,6 @@
> #define ETHSYS_DMA_AG_MAP_QDMA BIT(1)
> #define ETHSYS_DMA_AG_MAP_PPE BIT(2)
>
> -/* SGMII subsystem config registers */
> -/* BMCR (low 16) BMSR (high 16) */
> -#define SGMSYS_PCS_CONTROL_1 0x0
> -#define SGMII_BMCR GENMASK(15, 0)
> -#define SGMII_BMSR GENMASK(31, 16)
> -#define SGMII_AN_RESTART BIT(9)
> -#define SGMII_ISOLATE BIT(10)
> -#define SGMII_AN_ENABLE BIT(12)
> -#define SGMII_LINK_STATYS BIT(18)
> -#define SGMII_AN_ABILITY BIT(19)
> -#define SGMII_AN_COMPLETE BIT(21)
> -#define SGMII_PCS_FAULT BIT(23)
> -#define SGMII_AN_EXPANSION_CLR BIT(30)
> -
> -#define SGMSYS_PCS_ADVERTISE 0x8
> -#define SGMII_ADVERTISE GENMASK(15, 0)
> -#define SGMII_LPA GENMASK(31, 16)
> -
> -/* Register to programmable link timer, the unit in 2 * 8ns */
> -#define SGMSYS_PCS_LINK_TIMER 0x18
> -#define SGMII_LINK_TIMER_MASK GENMASK(19, 0)
> -#define SGMII_LINK_TIMER_DEFAULT (0x186a0 & SGMII_LINK_TIMER_MASK)
> -
> -/* Register to control remote fault */
> -#define SGMSYS_SGMII_MODE 0x20
> -#define SGMII_IF_MODE_SGMII BIT(0)
> -#define SGMII_SPEED_DUPLEX_AN BIT(1)
> -#define SGMII_SPEED_MASK GENMASK(3, 2)
> -#define SGMII_SPEED_10 FIELD_PREP(SGMII_SPEED_MASK, 0)
> -#define SGMII_SPEED_100 FIELD_PREP(SGMII_SPEED_MASK, 1)
> -#define SGMII_SPEED_1000 FIELD_PREP(SGMII_SPEED_MASK, 2)
> -#define SGMII_DUPLEX_HALF BIT(4)
> -#define SGMII_IF_MODE_BIT5 BIT(5)
> -#define SGMII_REMOTE_FAULT_DIS BIT(8)
> -#define SGMII_CODE_SYNC_SET_VAL BIT(9)
> -#define SGMII_CODE_SYNC_SET_EN BIT(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))
> -#define RG_PHY_SPEED_1_25G 0x0
> -#define RG_PHY_SPEED_3_125G BIT(2)
> -
> -/* Register to power up QPHY */
> -#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)
> @@ -1103,29 +1044,13 @@ struct mtk_soc_data {
> /* currently no SoC has more than 2 macs */
> #define MTK_MAX_DEVS 2
>
> -/* struct mtk_pcs - This structure holds each sgmii regmap and associated
> - * data
> - * @regmap: The register map pointing at the range used to setup
> - * SGMII modes
> - * @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
> * characteristics
> * @pcs Array of individual PCS structures
> */
> struct mtk_sgmii {
> - struct mtk_pcs pcs[MTK_MAX_DEVS];
> + struct phylink_pcs *pcs[MTK_MAX_DEVS];
> + struct device *dev;
> };
>
> /* struct mtk_eth - This is the main datasructure for holding the state
> @@ -1353,6 +1278,7 @@ u32 mtk_r32(struct mtk_eth *eth, unsigned reg);
> struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id);
> int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np,
> u32 ana_rgc3);
> +void mtk_sgmii_destroy(struct mtk_sgmii *ss);
>
> int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id);
> int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index 9c58006d1c33..d4b428e23cfc 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -10,199 +10,35 @@
> #include <linux/mfd/syscon.h>
> #include <linux/of.h>
> #include <linux/phylink.h>
> +#include <linux/pcs/pcs-mtk-lynxi.h>
> #include <linux/regmap.h>
>
> #include "mtk_eth_soc.h"
>
> -static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
> -{
> - return container_of(pcs, struct mtk_pcs, pcs);
> -}
> -
> -static void mtk_pcs_get_state(struct phylink_pcs *pcs,
> - struct phylink_link_state *state)
> -{
> - struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> - unsigned int bm, adv;
> -
> - /* Read the BMSR and LPA */
> - regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
> - regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
> -
> - phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
> - FIELD_GET(SGMII_LPA, adv));
> -}
> -
> -static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> - phy_interface_t interface,
> - const unsigned long *advertising,
> - bool permit_pause_to_mac)
> -{
> - bool mode_changed = false, changed, use_an;
> - struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> - unsigned int rgc3, sgm_mode, bmcr;
> - int advertise, link_timer;
> -
> - advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
> - advertising);
> - if (advertise < 0)
> - return advertise;
> -
> - /* 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).
> - */
> - if (interface == PHY_INTERFACE_MODE_SGMII) {
> - sgm_mode = SGMII_IF_MODE_SGMII;
> - if (phylink_autoneg_inband(mode)) {
> - sgm_mode |= SGMII_REMOTE_FAULT_DIS |
> - SGMII_SPEED_DUPLEX_AN;
> - use_an = true;
> - } else {
> - use_an = false;
> - }
> - } else if (phylink_autoneg_inband(mode)) {
> - /* 1000base-X or 2500base-X autoneg */
> - sgm_mode = SGMII_REMOTE_FAULT_DIS;
> - use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> - advertising);
> - } else {
> - /* 1000base-X or 2500base-X without autoneg */
> - sgm_mode = 0;
> - use_an = false;
> - }
> -
> - if (use_an) {
> - bmcr = SGMII_AN_ENABLE;
> - } else {
> - bmcr = 0;
> - }
> -
> - if (mpcs->interface != interface) {
> - /* PHYA power down */
> - 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,
> - SGMII_PN_SWAP_TX_RX);
> -
> - if (interface == PHY_INTERFACE_MODE_2500BASEX)
> - rgc3 = RG_PHY_SPEED_3_125G;
> - else
> - rgc3 = 0;
> -
> - /* Configure the underlying interface speed */
> - 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);
> -
> - /* Update the sgmsys mode register */
> - regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
> - SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
> - SGMII_IF_MODE_SGMII, sgm_mode);
> -
> - /* Update the BMCR */
> - regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
> - SGMII_AN_ENABLE, bmcr);
> -
> - /* Release PHYA power down state
> - * Only removing bit SGMII_PHYA_PWD isn't enough.
> - * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
> - * prevents SGMII from working. The SGMII still shows link but no traffic
> - * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
> - * taken from a good working state of the SGMII interface.
> - * Unknown how much the QPHY needs but it is racy without a sleep.
> - * Tested on mt7622 & mt7986.
> - */
> - usleep_range(50, 100);
> - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
> -
> - return changed || mode_changed;
> -}
> -
> -static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
> -{
> - struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> -
> - regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
> - SGMII_AN_RESTART, SGMII_AN_RESTART);
> -}
> -
> -static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
> - phy_interface_t interface, int speed, int duplex)
> -{
> - struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> - unsigned int sgm_mode;
> -
> - if (!phylink_autoneg_inband(mode)) {
> - /* Force the speed and duplex setting */
> - if (speed == SPEED_10)
> - sgm_mode = SGMII_SPEED_10;
> - else if (speed == SPEED_100)
> - sgm_mode = SGMII_SPEED_100;
> - else
> - sgm_mode = SGMII_SPEED_1000;
> -
> - if (duplex != DUPLEX_FULL)
> - sgm_mode |= SGMII_DUPLEX_HALF;
> -
> - regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
> - SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
> - sgm_mode);
> - }
> -}
> -
> -static const struct phylink_pcs_ops mtk_pcs_ops = {
> - .pcs_get_state = mtk_pcs_get_state,
> - .pcs_config = mtk_pcs_config,
> - .pcs_an_restart = mtk_pcs_restart_an,
> - .pcs_link_up = mtk_pcs_link_up,
> -};
> -
> int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
> {
> struct device_node *np;
> + struct regmap *regmap;
> + u32 flags;
> int i;
>
> - for (i = 0; i < MTK_MAX_DEVS; i++) {
> + for (i = 0; id < MTK_MAX_DEVS; i++) {

This change was unintentional and will break the build. I've fixed it
for my test builds, but forgot to commit it before sending out the series.
Please discard for now.

> 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;
> + regmap = syscon_node_to_regmap(np);
> + flags = 0;
> if (of_property_read_bool(np, "mediatek,pn_swap"))
> - ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
> + 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;
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ss->pcs[i] = mtk_pcs_lynxi_create(ss->dev, regmap, ana_rgc3,
> + flags);
> }
>
> return 0;
> @@ -210,8 +46,16 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
>
> struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id)
> {
> - if (!ss->pcs[id].regmap)
> - return NULL;
> + return ss->pcs[id];
> +}
> +
> +void mtk_sgmii_destroy(struct mtk_sgmii *ss)
> +{
> + int i;
> +
> + if (!ss)
> + return;
>
> - return &ss->pcs[id].pcs;
> + for (i = 0; i < MTK_MAX_DEVS; i++)
> + mtk_pcs_lynxi_destroy(ss->pcs[i]);
> }
> --
> 2.39.1
>
>

2023-02-07 17:08:20

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v2 04/11] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

On Tue, Feb 07, 2023 at 02:20:40PM +0000, Daniel Golle wrote:
> Set MDIO bus clock frequency and allow setting a custom maximum
> frquency from device tree.
>
> Signed-off-by: Daniel Golle <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2023-02-07 17:36:47

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 02/11] dt-bindings: net: mediatek,net: add mt7981-eth binding

On 07/02/2023 15:19, Daniel Golle wrote:
> Introduce DT bindings for the MT7981 SoC to mediatek,net.yaml.
>
> Signed-off-by: Daniel Golle <[email protected]>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Since this skipped also lists, there are no automated checks running on
it, thus no point to review it.

Resend with proper address list, please.

> ---
> .../devicetree/bindings/net/mediatek,net.yaml | 42 +++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
> index 7ef696204c5a..d17e2eb46118 100644
> --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
> +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
> @@ -21,6 +21,7 @@ properties:
> - mediatek,mt7623-eth
> - mediatek,mt7622-eth
> - mediatek,mt7629-eth
> + - mediatek,mt7981-eth
> - mediatek,mt7986-eth
> - ralink,rt5350-eth
>
> @@ -206,6 +207,47 @@ allOf:
>
> mediatek,wed: false
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: mediatek,mt7981-eth
> + then:
> + properties:
> + interrupts:
> + minItems: 4
> +
> + clocks:
> + minItems: 15
> + maxItems: 15
> +
> + clock-names:
> + items:
> + - const: fe
> + - const: gp2
> + - const: gp1
> + - const: wocpu0
> + - const: sgmii_ck
> + - const: sgmii_tx250m
> + - const: sgmii_rx250m
> + - const: sgmii_cdr_ref
> + - const: sgmii_cdr_fb
> + - const: sgmii2_tx250m
> + - const: sgmii2_rx250m
> + - const: sgmii2_cdr_ref
> + - const: sgmii2_cdr_fb
> + - const: netsys0
> + - const: netsys1
> +
> + mediatek,sgmiisys:
> + minItems: 2
> + maxItems: 2
> +
> + mediatek,wed-pcie:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + Phandle to the mediatek wed-pcie controller.

Do not define properties in each variant.

Best regards,
Krzysztof


2023-02-07 17:38:37

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On 07/02/2023 15:19, Daniel Golle wrote:
> Add documentation for the newly introduced 'mediatek,pn_swap' property
> to mediatek,sgmiisys.txt.
>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

> Signed-off-by: Daniel Golle <[email protected]>
> ---
> .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> index d2c24c277514..b38dd0fde21d 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> @@ -14,6 +14,10 @@ Required Properties:
> - "mediatek,mt7986-sgmiisys_1", "syscon"
> - #clock-cells: Must be 1
>
> +Optional Properties:
> +
> +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.

No:
1. No new properties for TXT bindings,
2. Underscore is not allowed.
3. Does not look like property of this node. This is a clock controller
or system controller, not SGMII/phy etc.

Best regards,
Krzysztof


2023-02-07 18:01:21

by Daniel Golle

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

Hi Krzysztof,

On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 15:19, Daniel Golle wrote:
> > Add documentation for the newly introduced 'mediatek,pn_swap' property
> > to mediatek,sgmiisys.txt.
> >
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> > Signed-off-by: Daniel Golle <[email protected]>
> > ---
> > .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > index d2c24c277514..b38dd0fde21d 100644
> > --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > @@ -14,6 +14,10 @@ Required Properties:
> > - "mediatek,mt7986-sgmiisys_1", "syscon"
> > - #clock-cells: Must be 1
> >
> > +Optional Properties:
> > +
> > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
>
> No:
> 1. No new properties for TXT bindings,

So I'll have to convert the bindings to YAML, right?

> 2. Underscore is not allowed.

Ack, will change the name of the property.

> 3. Does not look like property of this node. This is a clock controller
> or system controller, not SGMII/phy etc.

The register range referred to by this node *does* represent also an
SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
are referenced in the node of the Ethernet core, and then used by
drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
(This is the current situation already, and not related to the patchset
now adding only a new property to support hardware which needs that)

So: Should I introduce a new binding for the same compatible strings
related to the SGMII PHY features? Or is it fine in this case to add
this property to the existing binding?

Thank you in advance for your advise!


Daniel

2023-02-08 09:33:50

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On 07/02/2023 19:00, Daniel Golle wrote:
> Hi Krzysztof,
>
> On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote:
>> On 07/02/2023 15:19, Daniel Golle wrote:
>>> Add documentation for the newly introduced 'mediatek,pn_swap' property
>>> to mediatek,sgmiisys.txt.
>>>
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
>>> Signed-off-by: Daniel Golle <[email protected]>
>>> ---
>>> .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>>> index d2c24c277514..b38dd0fde21d 100644
>>> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>>> @@ -14,6 +14,10 @@ Required Properties:
>>> - "mediatek,mt7986-sgmiisys_1", "syscon"
>>> - #clock-cells: Must be 1
>>>
>>> +Optional Properties:
>>> +
>>> +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
>>
>> No:
>> 1. No new properties for TXT bindings,
>
> So I'll have to convert the bindings to YAML, right?

Yes, please.

>
>> 2. Underscore is not allowed.
>
> Ack, will change the name of the property.
>
>> 3. Does not look like property of this node. This is a clock controller
>> or system controller, not SGMII/phy etc.
>
> The register range referred to by this node *does* represent also an
> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
> are referenced in the node of the Ethernet core, and then used by
> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
> (This is the current situation already, and not related to the patchset
> now adding only a new property to support hardware which needs that)

Just because a register is located in syscon block, does not mean that
SGMII configuration is a property of this device.

>
> So: Should I introduce a new binding for the same compatible strings
> related to the SGMII PHY features? Or is it fine in this case to add
> this property to the existing binding?

The user of syscon should configure it. I don't think you need new
binding. You just have to update the user of this syscon.

Best regards,
Krzysztof


2023-02-08 13:52:08

by Daniel Golle

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 19:00, Daniel Golle wrote:
> > ...
> >> 3. Does not look like property of this node. This is a clock controller
> >> or system controller, not SGMII/phy etc.
> >
> > The register range referred to by this node *does* represent also an
> > SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
> > are referenced in the node of the Ethernet core, and then used by
> > drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
> > (This is the current situation already, and not related to the patchset
> > now adding only a new property to support hardware which needs that)
>
> Just because a register is located in syscon block, does not mean that
> SGMII configuration is a property of this device.

It's not just one register, the whole SGMII PCS is located in those
mediatek,sgmiisys syscon nodes.

>
> >
> > So: Should I introduce a new binding for the same compatible strings
> > related to the SGMII PHY features? Or is it fine in this case to add
> > this property to the existing binding?
>
> The user of syscon should configure it. I don't think you need new
> binding. You just have to update the user of this syscon.

Excuse my confusion, but it's still not entirely clear to me.
So in this case I should add the description of the added propterty of
the individual SGMII units (there can be more than one) to
Documentation/devicetree/bindings/net/mediatek,net.yaml
eventhough the properties are in the sgmiisys syscon nodes?

If so I will have to figure out how to describe properties of other
nodes in the binding of the node referencing them. Are there any
good examples for that?

Or should the property itself be moved into yet another array of
booleans which should be added in the node describing the ethernet
controller and referencing these sgmiisys syscons using phandles?

2023-02-08 20:09:02

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On 08/02/2023 14:51, Daniel Golle wrote:
> On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote:
>> On 07/02/2023 19:00, Daniel Golle wrote:
>>> ...
>>>> 3. Does not look like property of this node. This is a clock controller
>>>> or system controller, not SGMII/phy etc.
>>>
>>> The register range referred to by this node *does* represent also an
>>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
>>> are referenced in the node of the Ethernet core, and then used by
>>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
>>> (This is the current situation already, and not related to the patchset
>>> now adding only a new property to support hardware which needs that)
>>
>> Just because a register is located in syscon block, does not mean that
>> SGMII configuration is a property of this device.
>
> It's not just one register, the whole SGMII PCS is located in those
> mediatek,sgmiisys syscon nodes.

Then maybe this should be a PCS PHY device instead of adding properties
unrelated to clock/system controller? I don't know, currently this
binding says it is a provider of clocks...

>
>>
>>>
>>> So: Should I introduce a new binding for the same compatible strings
>>> related to the SGMII PHY features? Or is it fine in this case to add
>>> this property to the existing binding?
>>
>> The user of syscon should configure it. I don't think you need new
>> binding. You just have to update the user of this syscon.
>
> Excuse my confusion, but it's still not entirely clear to me.
> So in this case I should add the description of the added propterty of
> the individual SGMII units (there can be more than one) to
> Documentation/devicetree/bindings/net/mediatek,net.yaml
> eventhough the properties are in the sgmiisys syscon nodes?

I guess the property should be in the node representing the SGMII. You
add it now to the clock (or system) controller, so it does not look
right. It's not a property of a clock controller.

Now which node should have this property depends on your devices - which
I have no clue about, I read what is in the bindings.

>
> If so I will have to figure out how to describe properties of other
> nodes in the binding of the node referencing them. Are there any
> good examples for that?

phys and pcs'es?

>
> Or should the property itself be moved into yet another array of
> booleans which should be added in the node describing the ethernet
> controller and referencing these sgmiisys syscons using phandles?

Best regards,
Krzysztof


2023-02-08 22:31:31

by Daniel Golle

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

Hi Krzysztof,

thank you for taking the time to review and explain.

On Wed, Feb 08, 2023 at 09:08:40PM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2023 14:51, Daniel Golle wrote:
> > On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote:
> >> On 07/02/2023 19:00, Daniel Golle wrote:
> >>> ...
> >>>> 3. Does not look like property of this node. This is a clock controller
> >>>> or system controller, not SGMII/phy etc.
> >>>
> >>> The register range referred to by this node *does* represent also an
> >>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
> >>> are referenced in the node of the Ethernet core, and then used by
> >>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
> >>> (This is the current situation already, and not related to the patchset
> >>> now adding only a new property to support hardware which needs that)
> >>
> >> Just because a register is located in syscon block, does not mean that
> >> SGMII configuration is a property of this device.
> >
> > It's not just one register, the whole SGMII PCS is located in those
> > mediatek,sgmiisys syscon nodes.
>
> Then maybe this should be a PCS PHY device instead of adding properties
> unrelated to clock/system controller? I don't know, currently this
> binding says it is a provider of clocks...

As in reality it is really a clock provider and also SGMII PCS at the
same time, maybe we should just update the description of the binding
to match that:

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
index d2c24c2775141..db6f75df200ba 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
@@ -2,6 +2,7 @@ MediaTek SGMIISYS controller
============================

The MediaTek SGMIISYS controller provides various clocks to the system.
+It also represents the SGMII PCS used by the Ethernet core.

Required Properties:


See

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mediatek/mtk_sgmii.c#n179

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a.dtsi#n409


>
> >
> >>
> >>>
> >>> So: Should I introduce a new binding for the same compatible strings
> >>> related to the SGMII PHY features? Or is it fine in this case to add
> >>> this property to the existing binding?
> >>
> >> The user of syscon should configure it. I don't think you need new
> >> binding. You just have to update the user of this syscon.
> >
> > Excuse my confusion, but it's still not entirely clear to me.
> > So in this case I should add the description of the added propterty of
> > the individual SGMII units (there can be more than one) to
> > Documentation/devicetree/bindings/net/mediatek,net.yaml
> > eventhough the properties are in the sgmiisys syscon nodes?
>
> I guess the property should be in the node representing the SGMII. You
> add it now to the clock (or system) controller, so it does not look
> right. It's not a property of a clock controller.

Well maybe this node needs to be split then into one node representing
only the clock controller and another node representing the SGMII PCS?
I'm not sure if this is even possible, some registers in this range
represent clocks, other registers are accessed using regmap API in
mtk_sgmii.c.

And (see the rest of this series) the exact same SGMII PCS can also be
found in MT7531 switch IC which has it's own (a bit odd) way to access
32-bit registers over MDIO, also in this case it is simply not easily
possible to represent the SGMII PCS in device tree.

>
> Now which node should have this property depends on your devices - which
> I have no clue about, I read what is in the bindings.

There isn't any other node exclusively representing the SGMII PCS.
I guess the only other option would be to move the property to the
Ethernet controller node, which imho complicates things as it is
really a property of an individual SGMII PHY (of which there can be
more than one).

>
> >
> > If so I will have to figure out how to describe properties of other
> > nodes in the binding of the node referencing them. Are there any
> > good examples for that?
>
> phys and pcs'es?

Hm, none of the current PCS (or PHY) drivers are represented by a
syscon node... (and maybe that's the mistake in first place?)

>
> >
> > Or should the property itself be moved into yet another array of
> > booleans which should be added in the node describing the ethernet
> > controller and referencing these sgmiisys syscons using phandles?
>
> Best regards,
> Krzysztof
>

2023-02-09 11:52:45

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On 08/02/2023 23:30, Daniel Golle wrote:
> Hi Krzysztof,
>
> thank you for taking the time to review and explain.
>
> On Wed, Feb 08, 2023 at 09:08:40PM +0100, Krzysztof Kozlowski wrote:
>> On 08/02/2023 14:51, Daniel Golle wrote:
>>> On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote:
>>>> On 07/02/2023 19:00, Daniel Golle wrote:
>>>>> ...
>>>>>> 3. Does not look like property of this node. This is a clock controller
>>>>>> or system controller, not SGMII/phy etc.
>>>>>
>>>>> The register range referred to by this node *does* represent also an
>>>>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
>>>>> are referenced in the node of the Ethernet core, and then used by
>>>>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
>>>>> (This is the current situation already, and not related to the patchset
>>>>> now adding only a new property to support hardware which needs that)
>>>>
>>>> Just because a register is located in syscon block, does not mean that
>>>> SGMII configuration is a property of this device.
>>>
>>> It's not just one register, the whole SGMII PCS is located in those
>>> mediatek,sgmiisys syscon nodes.
>>
>> Then maybe this should be a PCS PHY device instead of adding properties
>> unrelated to clock/system controller? I don't know, currently this
>> binding says it is a provider of clocks...
>
> As in reality it is really a clock provider and also SGMII PCS at the
> same time, maybe we should just update the description of the binding
> to match that:
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> index d2c24c2775141..db6f75df200ba 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> @@ -2,6 +2,7 @@ MediaTek SGMIISYS controller
> ============================
>
> The MediaTek SGMIISYS controller provides various clocks to the system.
> +It also represents the SGMII PCS used by the Ethernet core.
>
> Required Properties:
>
>
> See
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mediatek/mtk_sgmii.c#n179
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a.dtsi#n409

But it is not used as phy or PCS. It is used as syscon. If it was a phy
or PCS, then the property probably belonged here, but bindings and Linux
implementation were created in different way, so it is not a phy or PCS,
just a syscon.

>>>>> So: Should I introduce a new binding for the same compatible strings
>>>>> related to the SGMII PHY features? Or is it fine in this case to add
>>>>> this property to the existing binding?
>>>>
>>>> The user of syscon should configure it. I don't think you need new
>>>> binding. You just have to update the user of this syscon.
>>>
>>> Excuse my confusion, but it's still not entirely clear to me.
>>> So in this case I should add the description of the added propterty of
>>> the individual SGMII units (there can be more than one) to
>>> Documentation/devicetree/bindings/net/mediatek,net.yaml
>>> eventhough the properties are in the sgmiisys syscon nodes?
>>
>> I guess the property should be in the node representing the SGMII. You
>> add it now to the clock (or system) controller, so it does not look
>> right. It's not a property of a clock controller.
>
> Well maybe this node needs to be split then into one node representing
> only the clock controller and another node representing the SGMII PCS?
> I'm not sure if this is even possible, some registers in this range
> represent clocks, other registers are accessed using regmap API in
> mtk_sgmii.c.

This can be the same node, but it must be used like PCS. syscon phandle
for getting regmap is something entirely else.

>
> And (see the rest of this series) the exact same SGMII PCS can also be
> found in MT7531 switch IC which has it's own (a bit odd) way to access
> 32-bit registers over MDIO, also in this case it is simply not easily
> possible to represent the SGMII PCS in device tree.
>
>>
>> Now which node should have this property depends on your devices - which
>> I have no clue about, I read what is in the bindings.
>
> There isn't any other node exclusively representing the SGMII PCS.
> I guess the only other option would be to move the property to the
> Ethernet controller node, which imho complicates things as it is
> really a property of an individual SGMII PHY (of which there can be
> more than one).
>
>>
>>>
>>> If so I will have to figure out how to describe properties of other
>>> nodes in the binding of the node referencing them. Are there any
>>> good examples for that?
>>
>> phys and pcs'es?
>
> Hm, none of the current PCS (or PHY) drivers are represented by a
> syscon node... (and maybe that's the mistake in first place?)

Yes.


Best regards,
Krzysztof


2023-02-10 10:31:10

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 15:19, Daniel Golle wrote:
> > +Optional Properties:
> > +
> > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
>
> No:
> 3. Does not look like property of this node. This is a clock controller
> or system controller, not SGMII/phy etc.

Wrong. Totally wrong.

SGMII data lines consist of a "positive" and a "negative" signal.
Clearly, this PHY supports swapping the sense of those.

This CLEARLY has nothing to do with a clock controller. It's also not
system-controller related either.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 10:34:25

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2023 23:30, Daniel Golle wrote:
> > Hm, none of the current PCS (or PHY) drivers are represented by a
> > syscon node... (and maybe that's the mistake in first place?)
>
> Yes.

Nos, it isn't.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 10:36:45

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 05/11] net: ethernet: mtk_eth_soc: reset PCS state

On Tue, Feb 07, 2023 at 02:21:02PM +0000, Daniel Golle wrote:
> Reset PCS state when changing interface mode.
>
> Signed-off-by: Daniel Golle <[email protected]>

Reviewed-by: Russell King (Oracle) <[email protected]>

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 10:38:57

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 06/11] net: ethernet: mtk_eth_soc: only write values if needed

On Tue, Feb 07, 2023 at 02:21:25PM +0000, Daniel Golle wrote:
> @@ -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;

I know I asked for this to be placed inside this if(), but I'd prefer it
if it were before any register state was touched, so if it returns an
error then no register state is affected by the pcs_config() call.

IIRC, we don't touch any state before the if(), so it should be a
matter of placing it as one of the first things in the if() block.

Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 10:50:46

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 09/11] net: pcs: add driver for MediaTek SGMII PCS

On Tue, Feb 07, 2023 at 02:23:08PM +0000, Daniel Golle wrote:
> +config PCS_MTK_LYNXI
> + tristate
> + select PHYLINK

Does this need to select PHYLINK? If the user of this doesn't already
select phylink, then phylink_create() won't be called and thus trying
to use PCS_MTK_LYNXI becomes impossible. I know PCS_XPCS does, none of
the others do though.

> + select REGMAP
> + 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)
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4c780d8f2e98..9b9afd6b1c22 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -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_LYNXI) += pcs-mtk-lynxi.o
> obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
> obj-$(CONFIG_PCS_ALTERA_TSE) += pcs-altera-tse.o
> diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
> new file mode 100644
> index 000000000000..0100def53d45
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-mtk-lynxi.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018-2019 MediaTek Inc.
> +/* A library for MediaTek SGMII circuit
> + *
> + * Author: Sean Wang <[email protected]>
> + * Author: Daniel Golle <[email protected]>
> + *
> + */
> +#include <linux/mdio.h>
> +#include <linux/phylink.h>
> +#include <linux/pcs/pcs-mtk-lynxi.h>
> +#include <linux/of.h>
> +#include <linux/phylink.h>
> +#include <linux/regmap.h>
> +
> +/* SGMII subsystem config registers */
> +/* BMCR (low 16) BMSR (high 16) */
> +#define SGMSYS_PCS_CONTROL_1 0x0
> +#define SGMII_BMCR GENMASK(15, 0)
> +#define SGMII_BMSR GENMASK(31, 16)
> +#define SGMII_AN_RESTART BIT(9)
> +#define SGMII_ISOLATE BIT(10)
> +#define SGMII_AN_ENABLE BIT(12)

Not really a review comment but a question: would it be sensible to
define these as:

#define SGMII_AN_RESTART BMCR_ANRESTART

etc, since they follow the IEEE802.3 clause 22 register layout?

> +#define SGMII_LINK_STATYS BIT(18)
> +#define SGMII_AN_ABILITY BIT(19)
> +#define SGMII_AN_COMPLETE BIT(21)

These also correspond to BMSR bits (<<16).

> +#define SGMII_PCS_FAULT BIT(23)
> +#define SGMII_AN_EXPANSION_CLR BIT(30)

This define doesn't seem to be used.

> +
> +#define SGMSYS_PCS_DEVICE_ID 0x4
> +#define SGMII_LYNXI_DEV_ID 0x4d544950
> +
> +#define SGMSYS_PCS_ADVERTISE 0x8
> +#define SGMII_ADVERTISE GENMASK(15, 0)
> +#define SGMII_LPA GENMASK(31, 16)
> +
> +#define SGMSYS_PCS_SCRATCH 0x14
> +#define SGMII_DEV_VERSION GENMASK(31, 16)
> +
> +/* Register to programmable link timer, the unit in 2 * 8ns */
> +#define SGMSYS_PCS_LINK_TIMER 0x18
> +#define SGMII_LINK_TIMER_MASK GENMASK(19, 0)
> +#define SGMII_LINK_TIMER_DEFAULT (0x186a0 & SGMII_LINK_TIMER_MASK)

We no longer make use of SGMII_LINK_TIMER_DEFAULT, so this can be
removed.

> +
> +/* Register to control remote fault */
> +#define SGMSYS_SGMII_MODE 0x20
> +#define SGMII_IF_MODE_SGMII BIT(0)
> +#define SGMII_SPEED_DUPLEX_AN BIT(1)
> +#define SGMII_SPEED_MASK GENMASK(3, 2)
> +#define SGMII_SPEED_10 FIELD_PREP(SGMII_SPEED_MASK, 0)
> +#define SGMII_SPEED_100 FIELD_PREP(SGMII_SPEED_MASK, 1)
> +#define SGMII_SPEED_1000 FIELD_PREP(SGMII_SPEED_MASK, 2)
> +#define SGMII_DUPLEX_HALF BIT(4)
> +#define SGMII_REMOTE_FAULT_DIS BIT(8)

> +#define SGMII_CODE_SYNC_SET_VAL BIT(9)
> +#define SGMII_CODE_SYNC_SET_EN BIT(10)
> +#define SGMII_SEND_AN_ERROR_EN BIT(11)

These three don't appear to be used.

> +
> +/* 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

SGMSYS_ANA_RG_CS3 isn't used here, although its register bits below
are.

> +#define RG_PHY_SPEED_MASK (BIT(2) | BIT(3))
> +#define RG_PHY_SPEED_1_25G 0x0
> +#define RG_PHY_SPEED_3_125G BIT(2)
> +

...

> +struct mtk_pcs_lynxi {
> + struct regmap *regmap;
> + struct device *dev;

I can only find one place that this is written to in this patch, it
seems its otherwise never used. Do we need it?

Other than that, I don't see anything else to comment on that hasn't
been mentioned in previous patches. Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 10:57:08

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] net: dsa: mt7530: use external PCS driver

On Tue, Feb 07, 2023 at 02:24:17PM +0000, Daniel Golle wrote:
> @@ -2728,11 +2612,11 @@ 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;
> -
> + return priv->ports[port].sgmii_pcs;

My only concern here is that we also use the PCS when in TRGMII mode in
this driver, but the mtk pcs code from mtk_eth_soc doesn't handle
TRGMII (and getting the link timer will fail for this mode, causing
the pcs_config() method to fail.)

Thus, this driver will stop working in TRGMII mode.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 12:23:27

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On Fri, Feb 10, 2023 at 10:34:17AM +0000, Russell King (Oracle) wrote:
> On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote:
> > On 08/02/2023 23:30, Daniel Golle wrote:
> > > Hm, none of the current PCS (or PHY) drivers are represented by a
> > > syscon node... (and maybe that's the mistake in first place?)
> >
> > Yes.
>
> Nos, it isn't.

To expand on this - I have no idea why you consider it a mistake that
apparently all PCS aren't represented by a syscon node.

PCS is a sub-block in an ethernet system, just the same as a MAC is a
sub-block. PCS can appear in several locations of an ethernet system,
but are generally found either side of a serial ethernet link such
as 1000base-X, SGMII, USXGMII, 10Gbase-R etc.

So, one can find PCS within an ethernet PHY - and there may be one
facing the MAC connection, and there will be another facing the media.
We generally do not need to separate these PCS from the PHY itself
because we view the PHY as one whole device.

The optional PCS on the MAC side of the link is something that we
need to know about, because this has to be configured to talk to the
PHY, or to configure and obtain negotiation results from in the case of
fibre links.

PCS on the MAC side are not a system level device, they are very much a
specific piece of ethernet hardware in the same way that the MAC is,
and we don't represent the MAC as a syscon node. There is no reason
to do so with PCS.

These PCS on the MAC side tend to be accessed via direct MMIO accesses,
or over a MDIO bus.

There's other blocks in the IEEE 802.3 ethernet layering, such as the
PMA/PMD module (which for the MAC side we tend to model with the
drivers/phy layer) - but again, these also appear in ethernet PHYs
in order to produce the electrical signals for e.g. twisted pair
ethernet.

So, to effectively state that you consider that PCS should always be
represented as a syscon node is rather naieve, and really as a DT
reviewer you should not be making such decisions, but soliciting
opinions from those who know this subject area in detail _whether_
they are some kind of system controller before making such a
decision.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-10 12:35:19

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

On 10/02/2023 13:23, Russell King (Oracle) wrote:
> On Fri, Feb 10, 2023 at 10:34:17AM +0000, Russell King (Oracle) wrote:
>> On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote:
>>> On 08/02/2023 23:30, Daniel Golle wrote:
>>>> Hm, none of the current PCS (or PHY) drivers are represented by a
>>>> syscon node... (and maybe that's the mistake in first place?)
>>>
>>> Yes.
>>
>> Nos, it isn't.
>
> To expand on this - I have no idea why you consider it a mistake that
> apparently all PCS aren't represented by a syscon node.
>
> PCS is a sub-block in an ethernet system, just the same as a MAC is a
> sub-block. PCS can appear in several locations of an ethernet system,
> but are generally found either side of a serial ethernet link such
> as 1000base-X, SGMII, USXGMII, 10Gbase-R etc.
>
> So, one can find PCS within an ethernet PHY - and there may be one
> facing the MAC connection, and there will be another facing the media.
> We generally do not need to separate these PCS from the PHY itself
> because we view the PHY as one whole device.
>
> The optional PCS on the MAC side of the link is something that we
> need to know about, because this has to be configured to talk to the
> PHY, or to configure and obtain negotiation results from in the case of
> fibre links.
>
> PCS on the MAC side are not a system level device, they are very much a
> specific piece of ethernet hardware in the same way that the MAC is,
> and we don't represent the MAC as a syscon node. There is no reason
> to do so with PCS.
>
> These PCS on the MAC side tend to be accessed via direct MMIO accesses,
> or over a MDIO bus.
>
> There's other blocks in the IEEE 802.3 ethernet layering, such as the
> PMA/PMD module (which for the MAC side we tend to model with the
> drivers/phy layer) - but again, these also appear in ethernet PHYs
> in order to produce the electrical signals for e.g. twisted pair
> ethernet.
>
> So, to effectively state that you consider that PCS should always be
> represented as a syscon node is rather naieve, and really as a DT
> reviewer you should not be making such decisions, but soliciting
> opinions from those who know this subject area in detail _whether_
> they are some kind of system controller before making such a
> decision.

Daniel switched to private emails, so unfortunately our talk is not
visible here, nevertheless thanks for the feedback. Much appreciated!

Best regards,
Krzysztof


2023-02-10 12:53:32

by Daniel Golle

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] net: dsa: mt7530: use external PCS driver

On Fri, Feb 10, 2023 at 10:56:57AM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 07, 2023 at 02:24:17PM +0000, Daniel Golle wrote:
> > @@ -2728,11 +2612,11 @@ 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;
> > -
> > + return priv->ports[port].sgmii_pcs;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> My only concern here is that we also use the PCS when in TRGMII mode in
> this driver, but the mtk pcs code from mtk_eth_soc doesn't handle
> TRGMII (and getting the link timer will fail for this mode, causing
> the pcs_config() method to fail.)
>
> Thus, this driver will stop working in TRGMII mode.

But in TRGMII mode we return &priv->pcs[port].pcs which is not the PCS
code from mtk_eth_soc now moved to the new pcs-mtk-lynxi driver, but
rather the old existing codepath from mt7530.c.

The new PCS driver only replaces mt7531_sgmii_pcs_* functions, which
were all nearly identical with what is now in pcs-mtk-lynxi.c.

Or do I miss something here?

2023-02-10 22:17:31

by Daniel Golle

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] net: dsa: mt7530: use external PCS driver

On Fri, Feb 10, 2023 at 10:56:57AM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 07, 2023 at 02:24:17PM +0000, Daniel Golle wrote:
> > @@ -2728,11 +2612,11 @@ 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;
> > -
> > + return priv->ports[port].sgmii_pcs;
>
> My only concern here is that we also use the PCS when in TRGMII mode in
> this driver, but the mtk pcs code from mtk_eth_soc doesn't handle
> TRGMII (and getting the link timer will fail for this mode, causing
> the pcs_config() method to fail.)
>
> Thus, this driver will stop working in TRGMII mode.

I've just built and tested this on MT7623 BPi-R2 board with MT7530
connnected to SoC via TRGMII and observed that it all works fine.

However, on this platform we could safe some bytes by still allowing
mt7530.c to be built without pcs-mtk-lynxi which isn't used on MT7623
by either the ethernet or the switch driver.

To me this is not a priority, and doing that would involve adding some
ifdef'ery which isn't really nice. However, if anyone has a strong
opinion in favor of allowing mt7530.c (or even mtk_eth_soc) to be built
without pcs-mtk-lynxi, let me know and I will make the necessary
changes.