2019-06-24 14:06:45

by Parshuram Raju Thombare

[permalink] [raw]
Subject: [PATCH v5 0/5] net: macb: cover letter

Hello !

This is 5th version of patch set containing following patches
for Cadence ethernet controller driver.

1. 0001-net-macb-add-phylink-support.patch
Replace phylib API's with phylink API's.
2. 0002-net-macb-add-support-for-sgmii-MAC-PHY-interface.patch
This patch add support for SGMII mode.
3. 0004-net-macb-add-support-for-c45-PHY.patch
This patch is to support C45 PHY.
4. 0005-net-macb-add-support-for-high-speed-interface
This patch add support for 10G USXGMII PCS in fixed mode.
5. 0006-net-macb-parameter-added-to-cadence-ethernet-controller-DT-binding
New parameter added to Cadence ethernet controller DT binding
for USXGMII interface.

Changes in v2:
1. Dropped patch configuring TI PHY DP83867 from
Cadence PCI wrapper driver.
2. Removed code registering emulated PHY for fixed mode.
3. Code reformatting as per Andrew's and Florian's suggestions.

Changes in v3:
Based on Russell's suggestions
1. Configure MAC in mac_config only for non in-band modes
2. Handle dynamic phy_mode changes in mac_config
3. Move MAC configurations to mac_config
4. Removed seemingly redundant check for phylink handle
5. Removed code from mac_an_restart and mac_link_state
now just return -EOPNOTSUPP

Changes in v4:
1. Removed PHY_INTERFACE_MODE_2500BASEX, PHY_INTERFACE_MODE_1000BASEX and
2.5G PHY_INTERFACE_MODE_SGMII phy modes from supported modes

Changes in v5:
1. Code refactoring

Parshuram Thombare (5):
net: macb: add phylink support
net: macb: add support for sgmii MAC-PHY interface
net: macb: add support for c45 PHY
net: macb: add support for high speed interface
net: macb: parameter added to cadence ethernet controller DT binding

.../devicetree/bindings/net/macb.txt | 3 +
drivers/net/ethernet/cadence/Kconfig | 2 +-
drivers/net/ethernet/cadence/macb.h | 113 +++-
drivers/net/ethernet/cadence/macb_main.c | 593 +++++++++++++-----
4 files changed, 528 insertions(+), 183 deletions(-)

--
2.17.1


2019-06-24 14:07:58

by Parshuram Raju Thombare

[permalink] [raw]
Subject: [PATCH v5 3/5] net: macb: add support for c45 PHY

This patch modify MDIO read/write functions to support
communication with C45 PHY.

Signed-off-by: Parshuram Thombare <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/ethernet/cadence/macb.h | 15 ++++--
drivers/net/ethernet/cadence/macb_main.c | 61 +++++++++++++++++++-----
2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 6d268283c318..330da702b946 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -642,10 +642,17 @@
#define GEM_CLK_DIV96 5

/* Constants for MAN register */
-#define MACB_MAN_SOF 1
-#define MACB_MAN_WRITE 1
-#define MACB_MAN_READ 2
-#define MACB_MAN_CODE 2
+#define MACB_MAN_C22_SOF 1
+#define MACB_MAN_C22_WRITE 1
+#define MACB_MAN_C22_READ 2
+#define MACB_MAN_C22_CODE 2
+
+#define MACB_MAN_C45_SOF 0
+#define MACB_MAN_C45_ADDR 0
+#define MACB_MAN_C45_WRITE 1
+#define MACB_MAN_C45_POST_READ_INCR 2
+#define MACB_MAN_C45_READ 3
+#define MACB_MAN_C45_CODE 2

/* Capability mask bits */
#define MACB_CAPS_ISR_CLEAR_ON_WRITE BIT(0)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 572691d948e9..1323f9b4d3b8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -341,11 +341,30 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
if (status < 0)
goto mdio_read_exit;

- macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
- | MACB_BF(RW, MACB_MAN_READ)
- | MACB_BF(PHYA, mii_id)
- | MACB_BF(REGA, regnum)
- | MACB_BF(CODE, MACB_MAN_CODE)));
+ if (regnum & MII_ADDR_C45) {
+ macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+ | MACB_BF(RW, MACB_MAN_C45_ADDR)
+ | MACB_BF(PHYA, mii_id)
+ | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+ | MACB_BF(DATA, regnum & 0xFFFF)
+ | MACB_BF(CODE, MACB_MAN_C45_CODE)));
+
+ status = macb_mdio_wait_for_idle(bp);
+ if (status < 0)
+ goto mdio_read_exit;
+
+ macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+ | MACB_BF(RW, MACB_MAN_C45_READ)
+ | MACB_BF(PHYA, mii_id)
+ | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+ | MACB_BF(CODE, MACB_MAN_C45_CODE)));
+ } else {
+ macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF)
+ | MACB_BF(RW, MACB_MAN_C22_READ)
+ | MACB_BF(PHYA, mii_id)
+ | MACB_BF(REGA, regnum)
+ | MACB_BF(CODE, MACB_MAN_C22_CODE)));
+ }

status = macb_mdio_wait_for_idle(bp);
if (status < 0)
@@ -374,12 +393,32 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
if (status < 0)
goto mdio_write_exit;

- macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
- | MACB_BF(RW, MACB_MAN_WRITE)
- | MACB_BF(PHYA, mii_id)
- | MACB_BF(REGA, regnum)
- | MACB_BF(CODE, MACB_MAN_CODE)
- | MACB_BF(DATA, value)));
+ if (regnum & MII_ADDR_C45) {
+ macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+ | MACB_BF(RW, MACB_MAN_C45_ADDR)
+ | MACB_BF(PHYA, mii_id)
+ | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+ | MACB_BF(DATA, regnum & 0xFFFF)
+ | MACB_BF(CODE, MACB_MAN_C45_CODE)));
+
+ status = macb_mdio_wait_for_idle(bp);
+ if (status < 0)
+ goto mdio_write_exit;
+
+ macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+ | MACB_BF(RW, MACB_MAN_C45_WRITE)
+ | MACB_BF(PHYA, mii_id)
+ | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+ | MACB_BF(CODE, MACB_MAN_C45_CODE)
+ | MACB_BF(DATA, value)));
+ } else {
+ macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF)
+ | MACB_BF(RW, MACB_MAN_C22_WRITE)
+ | MACB_BF(PHYA, mii_id)
+ | MACB_BF(REGA, regnum)
+ | MACB_BF(CODE, MACB_MAN_C22_CODE)
+ | MACB_BF(DATA, value)));
+ }

status = macb_mdio_wait_for_idle(bp);
if (status < 0)
--
2.17.1

2019-06-24 14:08:13

by Parshuram Raju Thombare

[permalink] [raw]
Subject: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface

This patch add support for SGMII interface) and
2.5Gbps MAC in Cadence ethernet controller driver.

Signed-off-by: Parshuram Thombare <[email protected]>
---
drivers/net/ethernet/cadence/macb.h | 54 +++++++++++++-----
drivers/net/ethernet/cadence/macb_main.c | 72 ++++++++++++++++++++++--
2 files changed, 109 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 8629d345af31..6d268283c318 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -77,6 +77,7 @@
#define MACB_RBQPH 0x04D4

/* GEM register offsets. */
+#define GEM_NCR 0x0000 /* Network Control */
#define GEM_NCFGR 0x0004 /* Network Config */
#define GEM_USRIO 0x000c /* User IO */
#define GEM_DMACFG 0x0010 /* DMA Configuration */
@@ -156,6 +157,7 @@
#define GEM_PEFTN 0x01f4 /* PTP Peer Event Frame Tx Ns */
#define GEM_PEFRSL 0x01f8 /* PTP Peer Event Frame Rx Sec Low */
#define GEM_PEFRN 0x01fc /* PTP Peer Event Frame Rx Ns */
+#define GEM_PCS_CTRL 0x0200 /* PCS Control */
#define GEM_DCFG1 0x0280 /* Design Config 1 */
#define GEM_DCFG2 0x0284 /* Design Config 2 */
#define GEM_DCFG3 0x0288 /* Design Config 3 */
@@ -271,6 +273,10 @@
#define MACB_IRXFCS_OFFSET 19
#define MACB_IRXFCS_SIZE 1

+/* GEM specific NCR bitfields. */
+#define GEM_TWO_PT_FIVE_GIG_OFFSET 29
+#define GEM_TWO_PT_FIVE_GIG_SIZE 1
+
/* GEM specific NCFGR bitfields. */
#define GEM_GBE_OFFSET 10 /* Gigabit mode enable */
#define GEM_GBE_SIZE 1
@@ -323,6 +329,9 @@
#define MACB_MDIO_SIZE 1
#define MACB_IDLE_OFFSET 2 /* The PHY management logic is idle */
#define MACB_IDLE_SIZE 1
+#define MACB_DUPLEX_OFFSET 3
+#define MACB_DUPLEX_SIZE 1
+

/* Bitfields in TSR */
#define MACB_UBR_OFFSET 0 /* Used bit read */
@@ -456,11 +465,17 @@
#define MACB_REV_OFFSET 0
#define MACB_REV_SIZE 16

+/* Bitfields in PCS_CONTROL. */
+#define GEM_PCS_CTRL_RST_OFFSET 15
+#define GEM_PCS_CTRL_RST_SIZE 1
+
/* Bitfields in DCFG1. */
#define GEM_IRQCOR_OFFSET 23
#define GEM_IRQCOR_SIZE 1
#define GEM_DBWDEF_OFFSET 25
#define GEM_DBWDEF_SIZE 3
+#define GEM_NO_PCS_OFFSET 0
+#define GEM_NO_PCS_SIZE 1

/* Bitfields in DCFG2. */
#define GEM_RX_PKT_BUFF_OFFSET 20
@@ -633,19 +648,32 @@
#define MACB_MAN_CODE 2

/* Capability mask bits */
-#define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001
-#define MACB_CAPS_USRIO_HAS_CLKEN 0x00000002
-#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII 0x00000004
-#define MACB_CAPS_NO_GIGABIT_HALF 0x00000008
-#define MACB_CAPS_USRIO_DISABLED 0x00000010
-#define MACB_CAPS_JUMBO 0x00000020
-#define MACB_CAPS_GEM_HAS_PTP 0x00000040
-#define MACB_CAPS_BD_RD_PREFETCH 0x00000080
-#define MACB_CAPS_NEEDS_RSTONUBR 0x00000100
-#define MACB_CAPS_FIFO_MODE 0x10000000
-#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000
-#define MACB_CAPS_SG_DISABLED 0x40000000
-#define MACB_CAPS_MACB_IS_GEM 0x80000000
+#define MACB_CAPS_ISR_CLEAR_ON_WRITE BIT(0)
+#define MACB_CAPS_USRIO_HAS_CLKEN BIT(1)
+#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII BIT(2)
+#define MACB_CAPS_NO_GIGABIT_HALF BIT(3)
+#define MACB_CAPS_USRIO_DISABLED BIT(4)
+#define MACB_CAPS_JUMBO BIT(5)
+#define MACB_CAPS_GEM_HAS_PTP BIT(6)
+#define MACB_CAPS_BD_RD_PREFETCH BIT(7)
+#define MACB_CAPS_NEEDS_RSTONUBR BIT(8)
+#define MACB_CAPS_FIFO_MODE BIT(28)
+#define MACB_CAPS_GIGABIT_MODE_AVAILABLE BIT(29)
+#define MACB_CAPS_SG_DISABLED BIT(30)
+#define MACB_CAPS_MACB_IS_GEM BIT(31)
+#define MACB_CAPS_PCS BIT(24)
+#define MACB_CAPS_MACB_IS_GEM_GXL BIT(25)
+
+#define MACB_GEM7010_IDNUM 0x009
+#define MACB_GEM7014_IDNU 0x107
+#define MACB_GEM7014A_IDNUM 0x207
+#define MACB_GEM7016_IDNUM 0x10a
+#define MACB_GEM7017_IDNUM 0x00a
+#define MACB_GEM7017A_IDNUM 0x20a
+#define MACB_GEM7020_IDNUM 0x003
+#define MACB_GEM7021_IDNUM 0x00c
+#define MACB_GEM7021A_IDNUM 0x20c
+#define MACB_GEM7022_IDNUM 0x00b

/* LSO settings */
#define MACB_LSO_UFO_ENABLE 0x01
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ac5b233cd2e5..572691d948e9 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -445,6 +445,8 @@ static void gem_phylink_validate(struct phylink_config *pl_config,
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

switch (state->interface) {
+ case PHY_INTERFACE_MODE_NA:
+ case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
@@ -480,10 +482,31 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
{
struct net_device *netdev = to_net_dev(pl_config->dev);
struct macb *bp = netdev_priv(netdev);
+ bool change_interface = bp->phy_interface != state->interface;
unsigned long flags;

spin_lock_irqsave(&bp->lock, flags);

+ if (change_interface) {
+ bp->phy_interface = state->interface;
+ gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
+ gem_readl(bp, NCR));
+
+ if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+ gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
+ GEM_BIT(PCSSEL) |
+ gem_readl(bp, NCFGR));
+ } else {
+ /* Disable SGMII mode and PCS */
+ gem_writel(bp, NCFGR, ~(GEM_BIT(SGMIIEN) |
+ GEM_BIT(PCSSEL)) &
+ gem_readl(bp, NCFGR));
+ /* Reset PCS */
+ gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
+ GEM_BIT(PCS_CTRL_RST));
+ }
+ }
+
if (!phylink_autoneg_inband(mode) &&
(bp->speed != state->speed ||
bp->duplex != state->duplex)) {
@@ -493,6 +516,7 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
if (macb_is_gem(bp))
reg &= ~GEM_BIT(GBE);
+
if (state->duplex)
reg |= MACB_BIT(FD);

@@ -587,8 +611,8 @@ static int macb_mii_probe(struct net_device *dev)
}

bp->link = 0;
- bp->speed = 0;
- bp->duplex = -1;
+ bp->speed = SPEED_UNKNOWN;
+ bp->duplex = DUPLEX_UNKNOWN;

return ret;
}
@@ -3337,6 +3361,22 @@ static void macb_configure_caps(struct macb *bp,
dcfg = gem_readl(bp, DCFG1);
if (GEM_BFEXT(IRQCOR, dcfg) == 0)
bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
+ if (GEM_BFEXT(NO_PCS, dcfg) == 0)
+ bp->caps |= MACB_CAPS_PCS;
+ switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
+ case MACB_GEM7016_IDNUM:
+ case MACB_GEM7017_IDNUM:
+ case MACB_GEM7017A_IDNUM:
+ case MACB_GEM7020_IDNUM:
+ case MACB_GEM7021_IDNUM:
+ case MACB_GEM7021A_IDNUM:
+ case MACB_GEM7022_IDNUM:
+ bp->caps |= MACB_CAPS_USRIO_DISABLED;
+ bp->caps |= MACB_CAPS_MACB_IS_GEM_GXL;
+ break;
+ default:
+ break;
+ }
dcfg = gem_readl(bp, DCFG2);
if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
bp->caps |= MACB_CAPS_FIFO_MODE;
@@ -4319,11 +4359,35 @@ static int macb_probe(struct platform_device *pdev)
}

phy_mode = of_get_phy_mode(np);
- if (phy_mode < 0)
+ if (phy_mode < 0) {
/* not found in DT, MII by default */
bp->phy_interface = PHY_INTERFACE_MODE_MII;
- else
+ } else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) {
+ u32 interface_supported = 1;
+
+ if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
+ if (!(bp->caps & MACB_CAPS_PCS))
+ interface_supported = 0;
+ } else if (phy_mode == PHY_INTERFACE_MODE_GMII ||
+ phy_mode == PHY_INTERFACE_MODE_RGMII) {
+ if (!macb_is_gem(bp))
+ interface_supported = 0;
+ } else if (phy_mode != PHY_INTERFACE_MODE_RMII &&
+ phy_mode != PHY_INTERFACE_MODE_MII) {
+ /* Add new mode before this */
+ interface_supported = 0;
+ }
+
+ if (!interface_supported) {
+ netdev_err(dev, "Phy mode %s not supported",
+ phy_modes(phy_mode));
+ goto err_out_free_netdev;
+ }
+
+ bp->phy_interface = phy_mode;
+ } else {
bp->phy_interface = phy_mode;
+ }

/* IP specific init */
err = init(pdev);
--
2.17.1

2019-06-24 14:08:56

by Parshuram Raju Thombare

[permalink] [raw]
Subject: [PATCH v5 4/5] net: macb: add support for high speed interface

This patch add support for high speed USXGMII PCS and 10G
speed in Cadence ethernet controller driver.

Signed-off-by: Parshuram Thombare <[email protected]>
---
drivers/net/ethernet/cadence/macb.h | 41 +++++
drivers/net/ethernet/cadence/macb_main.c | 189 ++++++++++++++++++++---
2 files changed, 207 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 330da702b946..809acff19be9 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -82,6 +82,7 @@
#define GEM_USRIO 0x000c /* User IO */
#define GEM_DMACFG 0x0010 /* DMA Configuration */
#define GEM_JML 0x0048 /* Jumbo Max Length */
+#define GEM_HS_MAC_CONFIG 0x0050 /* GEM high speed config */
#define GEM_HRB 0x0080 /* Hash Bottom */
#define GEM_HRT 0x0084 /* Hash Top */
#define GEM_SA1B 0x0088 /* Specific1 Bottom */
@@ -167,6 +168,9 @@
#define GEM_DCFG7 0x0298 /* Design Config 7 */
#define GEM_DCFG8 0x029C /* Design Config 8 */
#define GEM_DCFG10 0x02A4 /* Design Config 10 */
+#define GEM_DCFG12 0x02AC /* Design Config 12 */
+#define GEM_USX_CONTROL 0x0A80 /* USXGMII control register */
+#define GEM_USX_STATUS 0x0A88 /* USXGMII status register */

#define GEM_TXBDCTRL 0x04cc /* TX Buffer Descriptor control register */
#define GEM_RXBDCTRL 0x04d0 /* RX Buffer Descriptor control register */
@@ -274,6 +278,8 @@
#define MACB_IRXFCS_SIZE 1

/* GEM specific NCR bitfields. */
+#define GEM_ENABLE_HS_MAC_OFFSET 31
+#define GEM_ENABLE_HS_MAC_SIZE 1
#define GEM_TWO_PT_FIVE_GIG_OFFSET 29
#define GEM_TWO_PT_FIVE_GIG_SIZE 1

@@ -465,6 +471,10 @@
#define MACB_REV_OFFSET 0
#define MACB_REV_SIZE 16

+/* Bitfield in HS_MAC_CONFIG */
+#define GEM_HS_MAC_SPEED_OFFSET 0
+#define GEM_HS_MAC_SPEED_SIZE 3
+
/* Bitfields in PCS_CONTROL. */
#define GEM_PCS_CTRL_RST_OFFSET 15
#define GEM_PCS_CTRL_RST_SIZE 1
@@ -510,6 +520,34 @@
#define GEM_RXBD_RDBUFF_OFFSET 8
#define GEM_RXBD_RDBUFF_SIZE 4

+/* Bitfields in DCFG12. */
+#define GEM_HIGH_SPEED_OFFSET 26
+#define GEM_HIGH_SPEED_SIZE 1
+
+/* Bitfields in USX_CONTROL. */
+#define GEM_USX_CTRL_SPEED_OFFSET 14
+#define GEM_USX_CTRL_SPEED_SIZE 3
+#define GEM_SERDES_RATE_OFFSET 12
+#define GEM_SERDES_RATE_SIZE 2
+#define GEM_RX_SCR_BYPASS_OFFSET 9
+#define GEM_RX_SCR_BYPASS_SIZE 1
+#define GEM_TX_SCR_BYPASS_OFFSET 8
+#define GEM_TX_SCR_BYPASS_SIZE 1
+#define GEM_RX_SYNC_RESET_OFFSET 2
+#define GEM_RX_SYNC_RESET_SIZE 1
+#define GEM_TX_EN_OFFSET 1
+#define GEM_TX_EN_SIZE 1
+#define GEM_SIGNAL_OK_OFFSET 0
+#define GEM_SIGNAL_OK_SIZE 1
+
+/* Bitfields in USX_STATUS. */
+#define GEM_USX_TX_FAULT_OFFSET 28
+#define GEM_USX_TX_FAULT_SIZE 1
+#define GEM_USX_RX_FAULT_OFFSET 27
+#define GEM_USX_RX_FAULT_SIZE 1
+#define GEM_USX_BLOCK_LOCK_OFFSET 0
+#define GEM_USX_BLOCK_LOCK_SIZE 1
+
/* Bitfields in TISUBN */
#define GEM_SUBNSINCR_OFFSET 0
#define GEM_SUBNSINCR_SIZE 16
@@ -670,6 +708,7 @@
#define MACB_CAPS_MACB_IS_GEM BIT(31)
#define MACB_CAPS_PCS BIT(24)
#define MACB_CAPS_MACB_IS_GEM_GXL BIT(25)
+#define MACB_CAPS_HIGH_SPEED BIT(26)

#define MACB_GEM7010_IDNUM 0x009
#define MACB_GEM7014_IDNU 0x107
@@ -749,6 +788,7 @@
})

#define MACB_READ_NSR(bp) macb_readl(bp, NSR)
+#define GEM_READ_USX_STATUS(bp) gem_readl(bp, USX_STATUS)

/* struct macb_dma_desc - Hardware DMA descriptor
* @addr: DMA address of data buffer
@@ -1262,6 +1302,7 @@ struct macb {
struct macb_pm_data pm_data;
struct phylink *pl;
struct phylink_config pl_config;
+ u32 serdes_rate;
};

#ifdef CONFIG_MACB_USE_HWSTAMP
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 1323f9b4d3b8..5afc03299bee 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -84,6 +84,20 @@ static struct sifive_fu540_macb_mgmt *mgmt;
#define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
#define MACB_WOL_ENABLED (0x1 << 1)

+enum {
+ HS_MAC_SPEED_100M,
+ HS_MAC_SPEED_1000M,
+ HS_MAC_SPEED_2500M,
+ HS_MAC_SPEED_5000M,
+ HS_MAC_SPEED_10000M,
+ HS_MAC_SPEED_25000M,
+};
+
+enum {
+ MACB_SERDES_RATE_5Gbps = 5,
+ MACB_SERDES_RATE_10Gbps = 10,
+};
+
/* Graceful stop timeouts in us. We should allow up to
* 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
*/
@@ -93,6 +107,8 @@ static struct sifive_fu540_macb_mgmt *mgmt;

#define MACB_MDIO_TIMEOUT 1000000 /* in usecs */

+#define MACB_USX_BLOCK_LOCK_TIMEOUT 1000000 /* in usecs */
+
/* DMA buffer descriptor might be different size
* depends on hardware configuration:
*
@@ -439,23 +455,37 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
*/
static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
{
+ struct macb *bp = netdev_priv(dev);
long ferr, rate, rate_rounded;

if (!clk)
return;

- switch (speed) {
- case SPEED_10:
- rate = 2500000;
- break;
- case SPEED_100:
- rate = 25000000;
- break;
- case SPEED_1000:
- rate = 125000000;
- break;
- default:
- return;
+ if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
+ switch (bp->serdes_rate) {
+ case MACB_SERDES_RATE_5Gbps:
+ rate = 78125000;
+ break;
+ case MACB_SERDES_RATE_10Gbps:
+ rate = 156250000;
+ break;
+ default:
+ return;
+ }
+ } else {
+ switch (speed) {
+ case SPEED_10:
+ rate = 2500000;
+ break;
+ case SPEED_100:
+ rate = 25000000;
+ break;
+ case SPEED_1000:
+ rate = 125000000;
+ break;
+ default:
+ return;
+ }
}

rate_rounded = clk_round_rate(clk, rate);
@@ -485,6 +515,21 @@ static void gem_phylink_validate(struct phylink_config *pl_config,

switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
+ case PHY_INTERFACE_MODE_USXGMII:
+ case PHY_INTERFACE_MODE_10GKR:
+ if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+ phylink_set(mask, 10000baseCR_Full);
+ phylink_set(mask, 10000baseER_Full);
+ phylink_set(mask, 10000baseKR_Full);
+ phylink_set(mask, 10000baseLR_Full);
+ phylink_set(mask, 10000baseLRM_Full);
+ phylink_set(mask, 10000baseSR_Full);
+ phylink_set(mask, 10000baseT_Full);
+ phylink_set(mask, 5000baseT_Full);
+ phylink_set(mask, 2500baseX_Full);
+ phylink_set(mask, 1000baseX_Full);
+ }
+ /* fallthrough */
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
@@ -516,6 +561,91 @@ static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
return -EOPNOTSUPP;
}

+static int macb_wait_for_usx_block_lock(struct macb *bp)
+{
+ u32 val;
+
+ return readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
+ val & GEM_BIT(USX_BLOCK_LOCK),
+ 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
+}
+
+static inline int gem_mac_usx_configure(struct macb *bp, int spd)
+{
+ u32 speed, config;
+
+ gem_writel(bp, NCFGR, GEM_BIT(PCSSEL) |
+ (~GEM_BIT(SGMIIEN) & gem_readl(bp, NCFGR)));
+ gem_writel(bp, NCR, gem_readl(bp, NCR) |
+ GEM_BIT(ENABLE_HS_MAC));
+ gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) |
+ MACB_BIT(FD));
+ config = gem_readl(bp, USX_CONTROL);
+ config = GEM_BFINS(SERDES_RATE, bp->serdes_rate, config);
+ config &= ~GEM_BIT(TX_SCR_BYPASS);
+ config &= ~GEM_BIT(RX_SCR_BYPASS);
+ gem_writel(bp, USX_CONTROL, config |
+ GEM_BIT(TX_EN));
+ config = gem_readl(bp, USX_CONTROL);
+ gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK));
+ if (macb_wait_for_usx_block_lock(bp) < 0) {
+ netdev_warn(bp->dev, "USXGMII block lock failed");
+ return -ETIMEDOUT;
+ }
+
+ switch (spd) {
+ case SPEED_10000:
+ if (bp->serdes_rate >= MACB_SERDES_RATE_10Gbps) {
+ speed = HS_MAC_SPEED_10000M;
+ } else {
+ netdev_warn(bp->dev, "10G speed isn't supported by HW");
+ netdev_warn(bp->dev, "Setting speed to 1G");
+ speed = HS_MAC_SPEED_1000M;
+ }
+ break;
+ case SPEED_5000:
+ speed = HS_MAC_SPEED_5000M;
+ break;
+ case SPEED_2500:
+ speed = HS_MAC_SPEED_2500M;
+ break;
+ case SPEED_1000:
+ speed = HS_MAC_SPEED_1000M;
+ break;
+ default:
+ case SPEED_100:
+ speed = HS_MAC_SPEED_100M;
+ break;
+ }
+
+ gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, speed,
+ gem_readl(bp, HS_MAC_CONFIG)));
+ gem_writel(bp, USX_CONTROL, GEM_BFINS(USX_CTRL_SPEED, speed,
+ gem_readl(bp, USX_CONTROL)));
+ return 0;
+}
+
+static inline void gem_mac_configure(struct macb *bp, int speed)
+{
+ if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
+ gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
+ GEM_BIT(PCSSEL) |
+ gem_readl(bp, NCFGR));
+
+ switch (speed) {
+ case SPEED_1000:
+ gem_writel(bp, NCFGR, GEM_BIT(GBE) |
+ gem_readl(bp, NCFGR));
+ break;
+ case SPEED_100:
+ macb_writel(bp, NCFGR, MACB_BIT(SPD) |
+ macb_readl(bp, NCFGR));
+ break;
+ default:
+ break;
+ }
+}
+
static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
const struct phylink_link_state *state)
{
@@ -558,18 +688,17 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,

if (state->duplex)
reg |= MACB_BIT(FD);
+ macb_or_gem_writel(bp, NCFGR, reg);

- switch (state->speed) {
- case SPEED_1000:
- reg |= GEM_BIT(GBE);
- break;
- case SPEED_100:
- reg |= MACB_BIT(SPD);
- break;
- default:
- break;
+ if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
+ if (gem_mac_usx_configure(bp, state->speed) < 0) {
+ spin_unlock_irqrestore(&bp->lock, flags);
+ phylink_mac_change(bp->pl, false);
+ return;
+ }
+ } else {
+ gem_mac_configure(bp, state->speed);
}
- macb_or_gem_writel(bp, NCFGR, reg);

bp->speed = state->speed;
bp->duplex = state->duplex;
@@ -3416,6 +3545,9 @@ static void macb_configure_caps(struct macb *bp,
default:
break;
}
+ dcfg = gem_readl(bp, DCFG12);
+ if (GEM_BFEXT(HIGH_SPEED, dcfg) == 1)
+ bp->caps |= MACB_CAPS_HIGH_SPEED;
dcfg = gem_readl(bp, DCFG2);
if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
bp->caps |= MACB_CAPS_FIFO_MODE;
@@ -4404,7 +4536,18 @@ static int macb_probe(struct platform_device *pdev)
} else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) {
u32 interface_supported = 1;

- if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
+ if (phy_mode == PHY_INTERFACE_MODE_USXGMII) {
+ if (!(bp->caps & MACB_CAPS_HIGH_SPEED &&
+ bp->caps & MACB_CAPS_PCS))
+ interface_supported = 0;
+
+ if (of_property_read_u32(np, "serdes-rate-gbps",
+ &bp->serdes_rate)) {
+ netdev_err(dev,
+ "GEM serdes_rate not specified");
+ interface_supported = 0;
+ }
+ } else if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
if (!(bp->caps & MACB_CAPS_PCS))
interface_supported = 0;
} else if (phy_mode == PHY_INTERFACE_MODE_GMII ||
--
2.17.1

2019-06-24 14:09:15

by Parshuram Raju Thombare

[permalink] [raw]
Subject: [PATCH v5 5/5] net: macb: parameter added to cadence ethernet controller DT binding

New parameters added to Cadence ethernet controller DT binding
for USXGMII interface.

Signed-off-by: Parshuram Thombare <[email protected]>
---
Documentation/devicetree/bindings/net/macb.txt | 3 +++
1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 63c73fafe26d..dabdf9d3b574 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -28,6 +28,9 @@ Required properties:
Optional elements: 'rx_clk' applies to cdns,zynqmp-gem
Optional elements: 'tsu_clk'
- clocks: Phandles to input clocks.
+- serdes-rate-gbps External serdes rate.Mandatory for USXGMII mode.
+ 5 - 5G
+ 10 - 10G

The MAC address will be determined using the optional properties
defined in ethernet.txt.
--
2.17.1

2019-06-24 14:16:13

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface

On Mon, Jun 24, 2019 at 01:11:14PM +0100, Parshuram Thombare wrote:
> This patch add support for SGMII interface) and
> 2.5Gbps MAC in Cadence ethernet controller driver.
>
> Signed-off-by: Parshuram Thombare <[email protected]>
> ---
> drivers/net/ethernet/cadence/macb.h | 54 +++++++++++++-----
> drivers/net/ethernet/cadence/macb_main.c | 72 ++++++++++++++++++++++--
> 2 files changed, 109 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 8629d345af31..6d268283c318 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -77,6 +77,7 @@
> #define MACB_RBQPH 0x04D4
>
> /* GEM register offsets. */
> +#define GEM_NCR 0x0000 /* Network Control */
> #define GEM_NCFGR 0x0004 /* Network Config */
> #define GEM_USRIO 0x000c /* User IO */
> #define GEM_DMACFG 0x0010 /* DMA Configuration */
> @@ -156,6 +157,7 @@
> #define GEM_PEFTN 0x01f4 /* PTP Peer Event Frame Tx Ns */
> #define GEM_PEFRSL 0x01f8 /* PTP Peer Event Frame Rx Sec Low */
> #define GEM_PEFRN 0x01fc /* PTP Peer Event Frame Rx Ns */
> +#define GEM_PCS_CTRL 0x0200 /* PCS Control */
> #define GEM_DCFG1 0x0280 /* Design Config 1 */
> #define GEM_DCFG2 0x0284 /* Design Config 2 */
> #define GEM_DCFG3 0x0288 /* Design Config 3 */
> @@ -271,6 +273,10 @@
> #define MACB_IRXFCS_OFFSET 19
> #define MACB_IRXFCS_SIZE 1
>
> +/* GEM specific NCR bitfields. */
> +#define GEM_TWO_PT_FIVE_GIG_OFFSET 29
> +#define GEM_TWO_PT_FIVE_GIG_SIZE 1
> +
> /* GEM specific NCFGR bitfields. */
> #define GEM_GBE_OFFSET 10 /* Gigabit mode enable */
> #define GEM_GBE_SIZE 1
> @@ -323,6 +329,9 @@
> #define MACB_MDIO_SIZE 1
> #define MACB_IDLE_OFFSET 2 /* The PHY management logic is idle */
> #define MACB_IDLE_SIZE 1
> +#define MACB_DUPLEX_OFFSET 3
> +#define MACB_DUPLEX_SIZE 1
> +
>
> /* Bitfields in TSR */
> #define MACB_UBR_OFFSET 0 /* Used bit read */
> @@ -456,11 +465,17 @@
> #define MACB_REV_OFFSET 0
> #define MACB_REV_SIZE 16
>
> +/* Bitfields in PCS_CONTROL. */
> +#define GEM_PCS_CTRL_RST_OFFSET 15
> +#define GEM_PCS_CTRL_RST_SIZE 1
> +
> /* Bitfields in DCFG1. */
> #define GEM_IRQCOR_OFFSET 23
> #define GEM_IRQCOR_SIZE 1
> #define GEM_DBWDEF_OFFSET 25
> #define GEM_DBWDEF_SIZE 3
> +#define GEM_NO_PCS_OFFSET 0
> +#define GEM_NO_PCS_SIZE 1
>
> /* Bitfields in DCFG2. */
> #define GEM_RX_PKT_BUFF_OFFSET 20
> @@ -633,19 +648,32 @@
> #define MACB_MAN_CODE 2
>
> /* Capability mask bits */
> -#define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001
> -#define MACB_CAPS_USRIO_HAS_CLKEN 0x00000002
> -#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII 0x00000004
> -#define MACB_CAPS_NO_GIGABIT_HALF 0x00000008
> -#define MACB_CAPS_USRIO_DISABLED 0x00000010
> -#define MACB_CAPS_JUMBO 0x00000020
> -#define MACB_CAPS_GEM_HAS_PTP 0x00000040
> -#define MACB_CAPS_BD_RD_PREFETCH 0x00000080
> -#define MACB_CAPS_NEEDS_RSTONUBR 0x00000100
> -#define MACB_CAPS_FIFO_MODE 0x10000000
> -#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000
> -#define MACB_CAPS_SG_DISABLED 0x40000000
> -#define MACB_CAPS_MACB_IS_GEM 0x80000000
> +#define MACB_CAPS_ISR_CLEAR_ON_WRITE BIT(0)
> +#define MACB_CAPS_USRIO_HAS_CLKEN BIT(1)
> +#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII BIT(2)
> +#define MACB_CAPS_NO_GIGABIT_HALF BIT(3)
> +#define MACB_CAPS_USRIO_DISABLED BIT(4)
> +#define MACB_CAPS_JUMBO BIT(5)
> +#define MACB_CAPS_GEM_HAS_PTP BIT(6)
> +#define MACB_CAPS_BD_RD_PREFETCH BIT(7)
> +#define MACB_CAPS_NEEDS_RSTONUBR BIT(8)
> +#define MACB_CAPS_FIFO_MODE BIT(28)
> +#define MACB_CAPS_GIGABIT_MODE_AVAILABLE BIT(29)
> +#define MACB_CAPS_SG_DISABLED BIT(30)
> +#define MACB_CAPS_MACB_IS_GEM BIT(31)
> +#define MACB_CAPS_PCS BIT(24)
> +#define MACB_CAPS_MACB_IS_GEM_GXL BIT(25)
> +
> +#define MACB_GEM7010_IDNUM 0x009
> +#define MACB_GEM7014_IDNU 0x107
> +#define MACB_GEM7014A_IDNUM 0x207
> +#define MACB_GEM7016_IDNUM 0x10a
> +#define MACB_GEM7017_IDNUM 0x00a
> +#define MACB_GEM7017A_IDNUM 0x20a
> +#define MACB_GEM7020_IDNUM 0x003
> +#define MACB_GEM7021_IDNUM 0x00c
> +#define MACB_GEM7021A_IDNUM 0x20c
> +#define MACB_GEM7022_IDNUM 0x00b
>
> /* LSO settings */
> #define MACB_LSO_UFO_ENABLE 0x01
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ac5b233cd2e5..572691d948e9 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -445,6 +445,8 @@ static void gem_phylink_validate(struct phylink_config *pl_config,
> __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>
> switch (state->interface) {
> + case PHY_INTERFACE_MODE_NA:
> + case PHY_INTERFACE_MODE_SGMII:
> case PHY_INTERFACE_MODE_GMII:
> case PHY_INTERFACE_MODE_RGMII:
> if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> @@ -480,10 +482,31 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
> {
> struct net_device *netdev = to_net_dev(pl_config->dev);
> struct macb *bp = netdev_priv(netdev);
> + bool change_interface = bp->phy_interface != state->interface;
> unsigned long flags;
>
> spin_lock_irqsave(&bp->lock, flags);
>
> + if (change_interface) {
> + bp->phy_interface = state->interface;
> + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
> + gem_readl(bp, NCR));

This could do with a comment, such as the one I gave in my example.

> +
> + if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
> + GEM_BIT(PCSSEL) |
> + gem_readl(bp, NCFGR));
> + } else {
> + /* Disable SGMII mode and PCS */
> + gem_writel(bp, NCFGR, ~(GEM_BIT(SGMIIEN) |
> + GEM_BIT(PCSSEL)) &
> + gem_readl(bp, NCFGR));
> + /* Reset PCS */
> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
> + GEM_BIT(PCS_CTRL_RST));
> + }
> + }

Good.

> +
> if (!phylink_autoneg_inband(mode) &&
> (bp->speed != state->speed ||
> bp->duplex != state->duplex)) {
> @@ -493,6 +516,7 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
> reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> if (macb_is_gem(bp))
> reg &= ~GEM_BIT(GBE);
> +

Useless change.

> if (state->duplex)
> reg |= MACB_BIT(FD);
>
> @@ -587,8 +611,8 @@ static int macb_mii_probe(struct net_device *dev)
> }
>
> bp->link = 0;
> - bp->speed = 0;
> - bp->duplex = -1;
> + bp->speed = SPEED_UNKNOWN;
> + bp->duplex = DUPLEX_UNKNOWN;
>
> return ret;
> }
> @@ -3337,6 +3361,22 @@ static void macb_configure_caps(struct macb *bp,
> dcfg = gem_readl(bp, DCFG1);
> if (GEM_BFEXT(IRQCOR, dcfg) == 0)
> bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
> + if (GEM_BFEXT(NO_PCS, dcfg) == 0)
> + bp->caps |= MACB_CAPS_PCS;
> + switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
> + case MACB_GEM7016_IDNUM:
> + case MACB_GEM7017_IDNUM:
> + case MACB_GEM7017A_IDNUM:
> + case MACB_GEM7020_IDNUM:
> + case MACB_GEM7021_IDNUM:
> + case MACB_GEM7021A_IDNUM:
> + case MACB_GEM7022_IDNUM:
> + bp->caps |= MACB_CAPS_USRIO_DISABLED;
> + bp->caps |= MACB_CAPS_MACB_IS_GEM_GXL;
> + break;
> + default:
> + break;
> + }
> dcfg = gem_readl(bp, DCFG2);
> if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
> bp->caps |= MACB_CAPS_FIFO_MODE;
> @@ -4319,11 +4359,35 @@ static int macb_probe(struct platform_device *pdev)
> }
>
> phy_mode = of_get_phy_mode(np);
> - if (phy_mode < 0)
> + if (phy_mode < 0) {
> /* not found in DT, MII by default */
> bp->phy_interface = PHY_INTERFACE_MODE_MII;
> - else
> + } else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) {
> + u32 interface_supported = 1;
> +
> + if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
> + if (!(bp->caps & MACB_CAPS_PCS))
> + interface_supported = 0;

So if bp->caps does not have MACB_CAPS_PCS set, then SGMII mode is not
supported?

In which case, gem_phylink_validate() must clear the support mask when
SGMII mode is requested to indicate that the interface mode is not
supported.

The same goes for _all_ other PHY link modes that the hardware does not
actually support, such as PHY_INTERFACE_MODE_10GKR...

> + } else if (phy_mode == PHY_INTERFACE_MODE_GMII ||
> + phy_mode == PHY_INTERFACE_MODE_RGMII) {
> + if (!macb_is_gem(bp))
> + interface_supported = 0;
> + } else if (phy_mode != PHY_INTERFACE_MODE_RMII &&
> + phy_mode != PHY_INTERFACE_MODE_MII) {
> + /* Add new mode before this */
> + interface_supported = 0;
> + }

... and the same goes for these too.

> +
> + if (!interface_supported) {
> + netdev_err(dev, "Phy mode %s not supported",
> + phy_modes(phy_mode));
> + goto err_out_free_netdev;
> + }
> +
> + bp->phy_interface = phy_mode;
> + } else {
> bp->phy_interface = phy_mode;
> + }
>
> /* IP specific init */
> err = init(pdev);
> --
> 2.17.1
>
>

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-24 14:16:56

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v5 4/5] net: macb: add support for high speed interface

On Mon, Jun 24, 2019 at 01:12:35PM +0100, Parshuram Thombare wrote:
> This patch add support for high speed USXGMII PCS and 10G
> speed in Cadence ethernet controller driver.
>
> Signed-off-by: Parshuram Thombare <[email protected]>
> ---
> drivers/net/ethernet/cadence/macb.h | 41 +++++
> drivers/net/ethernet/cadence/macb_main.c | 189 ++++++++++++++++++++---
> 2 files changed, 207 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 330da702b946..809acff19be9 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -82,6 +82,7 @@
> #define GEM_USRIO 0x000c /* User IO */
> #define GEM_DMACFG 0x0010 /* DMA Configuration */
> #define GEM_JML 0x0048 /* Jumbo Max Length */
> +#define GEM_HS_MAC_CONFIG 0x0050 /* GEM high speed config */
> #define GEM_HRB 0x0080 /* Hash Bottom */
> #define GEM_HRT 0x0084 /* Hash Top */
> #define GEM_SA1B 0x0088 /* Specific1 Bottom */
> @@ -167,6 +168,9 @@
> #define GEM_DCFG7 0x0298 /* Design Config 7 */
> #define GEM_DCFG8 0x029C /* Design Config 8 */
> #define GEM_DCFG10 0x02A4 /* Design Config 10 */
> +#define GEM_DCFG12 0x02AC /* Design Config 12 */
> +#define GEM_USX_CONTROL 0x0A80 /* USXGMII control register */
> +#define GEM_USX_STATUS 0x0A88 /* USXGMII status register */
>
> #define GEM_TXBDCTRL 0x04cc /* TX Buffer Descriptor control register */
> #define GEM_RXBDCTRL 0x04d0 /* RX Buffer Descriptor control register */
> @@ -274,6 +278,8 @@
> #define MACB_IRXFCS_SIZE 1
>
> /* GEM specific NCR bitfields. */
> +#define GEM_ENABLE_HS_MAC_OFFSET 31
> +#define GEM_ENABLE_HS_MAC_SIZE 1
> #define GEM_TWO_PT_FIVE_GIG_OFFSET 29
> #define GEM_TWO_PT_FIVE_GIG_SIZE 1
>
> @@ -465,6 +471,10 @@
> #define MACB_REV_OFFSET 0
> #define MACB_REV_SIZE 16
>
> +/* Bitfield in HS_MAC_CONFIG */
> +#define GEM_HS_MAC_SPEED_OFFSET 0
> +#define GEM_HS_MAC_SPEED_SIZE 3
> +
> /* Bitfields in PCS_CONTROL. */
> #define GEM_PCS_CTRL_RST_OFFSET 15
> #define GEM_PCS_CTRL_RST_SIZE 1
> @@ -510,6 +520,34 @@
> #define GEM_RXBD_RDBUFF_OFFSET 8
> #define GEM_RXBD_RDBUFF_SIZE 4
>
> +/* Bitfields in DCFG12. */
> +#define GEM_HIGH_SPEED_OFFSET 26
> +#define GEM_HIGH_SPEED_SIZE 1
> +
> +/* Bitfields in USX_CONTROL. */
> +#define GEM_USX_CTRL_SPEED_OFFSET 14
> +#define GEM_USX_CTRL_SPEED_SIZE 3
> +#define GEM_SERDES_RATE_OFFSET 12
> +#define GEM_SERDES_RATE_SIZE 2
> +#define GEM_RX_SCR_BYPASS_OFFSET 9
> +#define GEM_RX_SCR_BYPASS_SIZE 1
> +#define GEM_TX_SCR_BYPASS_OFFSET 8
> +#define GEM_TX_SCR_BYPASS_SIZE 1
> +#define GEM_RX_SYNC_RESET_OFFSET 2
> +#define GEM_RX_SYNC_RESET_SIZE 1
> +#define GEM_TX_EN_OFFSET 1
> +#define GEM_TX_EN_SIZE 1
> +#define GEM_SIGNAL_OK_OFFSET 0
> +#define GEM_SIGNAL_OK_SIZE 1
> +
> +/* Bitfields in USX_STATUS. */
> +#define GEM_USX_TX_FAULT_OFFSET 28
> +#define GEM_USX_TX_FAULT_SIZE 1
> +#define GEM_USX_RX_FAULT_OFFSET 27
> +#define GEM_USX_RX_FAULT_SIZE 1
> +#define GEM_USX_BLOCK_LOCK_OFFSET 0
> +#define GEM_USX_BLOCK_LOCK_SIZE 1
> +
> /* Bitfields in TISUBN */
> #define GEM_SUBNSINCR_OFFSET 0
> #define GEM_SUBNSINCR_SIZE 16
> @@ -670,6 +708,7 @@
> #define MACB_CAPS_MACB_IS_GEM BIT(31)
> #define MACB_CAPS_PCS BIT(24)
> #define MACB_CAPS_MACB_IS_GEM_GXL BIT(25)
> +#define MACB_CAPS_HIGH_SPEED BIT(26)
>
> #define MACB_GEM7010_IDNUM 0x009
> #define MACB_GEM7014_IDNU 0x107
> @@ -749,6 +788,7 @@
> })
>
> #define MACB_READ_NSR(bp) macb_readl(bp, NSR)
> +#define GEM_READ_USX_STATUS(bp) gem_readl(bp, USX_STATUS)
>
> /* struct macb_dma_desc - Hardware DMA descriptor
> * @addr: DMA address of data buffer
> @@ -1262,6 +1302,7 @@ struct macb {
> struct macb_pm_data pm_data;
> struct phylink *pl;
> struct phylink_config pl_config;
> + u32 serdes_rate;
> };
>
> #ifdef CONFIG_MACB_USE_HWSTAMP
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 1323f9b4d3b8..5afc03299bee 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -84,6 +84,20 @@ static struct sifive_fu540_macb_mgmt *mgmt;
> #define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
> #define MACB_WOL_ENABLED (0x1 << 1)
>
> +enum {
> + HS_MAC_SPEED_100M,
> + HS_MAC_SPEED_1000M,
> + HS_MAC_SPEED_2500M,
> + HS_MAC_SPEED_5000M,
> + HS_MAC_SPEED_10000M,
> + HS_MAC_SPEED_25000M,
> +};
> +
> +enum {
> + MACB_SERDES_RATE_5Gbps = 5,
> + MACB_SERDES_RATE_10Gbps = 10,
> +};
> +
> /* Graceful stop timeouts in us. We should allow up to
> * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
> */
> @@ -93,6 +107,8 @@ static struct sifive_fu540_macb_mgmt *mgmt;
>
> #define MACB_MDIO_TIMEOUT 1000000 /* in usecs */
>
> +#define MACB_USX_BLOCK_LOCK_TIMEOUT 1000000 /* in usecs */
> +
> /* DMA buffer descriptor might be different size
> * depends on hardware configuration:
> *
> @@ -439,23 +455,37 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> */
> static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
> {
> + struct macb *bp = netdev_priv(dev);
> long ferr, rate, rate_rounded;
>
> if (!clk)
> return;
>
> - switch (speed) {
> - case SPEED_10:
> - rate = 2500000;
> - break;
> - case SPEED_100:
> - rate = 25000000;
> - break;
> - case SPEED_1000:
> - rate = 125000000;
> - break;
> - default:
> - return;
> + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> + switch (bp->serdes_rate) {
> + case MACB_SERDES_RATE_5Gbps:
> + rate = 78125000;
> + break;
> + case MACB_SERDES_RATE_10Gbps:
> + rate = 156250000;
> + break;
> + default:
> + return;
> + }
> + } else {
> + switch (speed) {
> + case SPEED_10:
> + rate = 2500000;
> + break;
> + case SPEED_100:
> + rate = 25000000;
> + break;
> + case SPEED_1000:
> + rate = 125000000;
> + break;
> + default:
> + return;
> + }
> }
>
> rate_rounded = clk_round_rate(clk, rate);
> @@ -485,6 +515,21 @@ static void gem_phylink_validate(struct phylink_config *pl_config,
>
> switch (state->interface) {
> case PHY_INTERFACE_MODE_NA:
> + case PHY_INTERFACE_MODE_USXGMII:
> + case PHY_INTERFACE_MODE_10GKR:
> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> + phylink_set(mask, 10000baseCR_Full);
> + phylink_set(mask, 10000baseER_Full);
> + phylink_set(mask, 10000baseKR_Full);
> + phylink_set(mask, 10000baseLR_Full);
> + phylink_set(mask, 10000baseLRM_Full);
> + phylink_set(mask, 10000baseSR_Full);
> + phylink_set(mask, 10000baseT_Full);
> + phylink_set(mask, 5000baseT_Full);
> + phylink_set(mask, 2500baseX_Full);
> + phylink_set(mask, 1000baseX_Full);
> + }

If MACB_CAPS_GIGABIT_MODE_AVAILABLE is not set, are these interface
modes supported by the hardware? If the PHY interface mode is not
supported, then the returned support mask must be cleared.

> + /* fallthrough */
> case PHY_INTERFACE_MODE_SGMII:
> case PHY_INTERFACE_MODE_GMII:
> case PHY_INTERFACE_MODE_RGMII:
> @@ -516,6 +561,91 @@ static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
> return -EOPNOTSUPP;
> }
>
> +static int macb_wait_for_usx_block_lock(struct macb *bp)
> +{
> + u32 val;
> +
> + return readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
> + val & GEM_BIT(USX_BLOCK_LOCK),
> + 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
> +}
> +
> +static inline int gem_mac_usx_configure(struct macb *bp, int spd)
> +{
> + u32 speed, config;
> +
> + gem_writel(bp, NCFGR, GEM_BIT(PCSSEL) |
> + (~GEM_BIT(SGMIIEN) & gem_readl(bp, NCFGR)));
> + gem_writel(bp, NCR, gem_readl(bp, NCR) |
> + GEM_BIT(ENABLE_HS_MAC));
> + gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) |
> + MACB_BIT(FD));
> + config = gem_readl(bp, USX_CONTROL);
> + config = GEM_BFINS(SERDES_RATE, bp->serdes_rate, config);
> + config &= ~GEM_BIT(TX_SCR_BYPASS);
> + config &= ~GEM_BIT(RX_SCR_BYPASS);
> + gem_writel(bp, USX_CONTROL, config |
> + GEM_BIT(TX_EN));
> + config = gem_readl(bp, USX_CONTROL);
> + gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK));
> + if (macb_wait_for_usx_block_lock(bp) < 0) {
> + netdev_warn(bp->dev, "USXGMII block lock failed");
> + return -ETIMEDOUT;
> + }
> +
> + switch (spd) {
> + case SPEED_10000:
> + if (bp->serdes_rate >= MACB_SERDES_RATE_10Gbps) {
> + speed = HS_MAC_SPEED_10000M;
> + } else {
> + netdev_warn(bp->dev, "10G speed isn't supported by HW");
> + netdev_warn(bp->dev, "Setting speed to 1G");
> + speed = HS_MAC_SPEED_1000M;
> + }
> + break;
> + case SPEED_5000:
> + speed = HS_MAC_SPEED_5000M;
> + break;
> + case SPEED_2500:
> + speed = HS_MAC_SPEED_2500M;
> + break;
> + case SPEED_1000:
> + speed = HS_MAC_SPEED_1000M;
> + break;
> + default:
> + case SPEED_100:
> + speed = HS_MAC_SPEED_100M;
> + break;
> + }
> +
> + gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, speed,
> + gem_readl(bp, HS_MAC_CONFIG)));
> + gem_writel(bp, USX_CONTROL, GEM_BFINS(USX_CTRL_SPEED, speed,
> + gem_readl(bp, USX_CONTROL)));
> + return 0;
> +}
> +
> +static inline void gem_mac_configure(struct macb *bp, int speed)
> +{
> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
> + GEM_BIT(PCSSEL) |
> + gem_readl(bp, NCFGR));

Is this still necessary?

> +
> + switch (speed) {
> + case SPEED_1000:
> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> + gem_readl(bp, NCFGR));
> + break;
> + case SPEED_100:
> + macb_writel(bp, NCFGR, MACB_BIT(SPD) |
> + macb_readl(bp, NCFGR));

What happens to the NCFGR register if we call mac_config() first for
a 1G speed, then 100M and finally 10M - what value does the NCFGR
register end up with?

I suspect it ends up with both the GBE and SPD bits set, and that is
probably not what you want.

> + break;
> + default:
> + break;
> + }
> +}
> +
> static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
> const struct phylink_link_state *state)
> {
> @@ -558,18 +688,17 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
>
> if (state->duplex)
> reg |= MACB_BIT(FD);
> + macb_or_gem_writel(bp, NCFGR, reg);
>
> - switch (state->speed) {
> - case SPEED_1000:
> - reg |= GEM_BIT(GBE);
> - break;
> - case SPEED_100:
> - reg |= MACB_BIT(SPD);
> - break;
> - default:
> - break;
> + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> + if (gem_mac_usx_configure(bp, state->speed) < 0) {
> + spin_unlock_irqrestore(&bp->lock, flags);
> + phylink_mac_change(bp->pl, false);
> + return;
> + }
> + } else {
> + gem_mac_configure(bp, state->speed);
> }
> - macb_or_gem_writel(bp, NCFGR, reg);
>
> bp->speed = state->speed;
> bp->duplex = state->duplex;
> @@ -3416,6 +3545,9 @@ static void macb_configure_caps(struct macb *bp,
> default:
> break;
> }
> + dcfg = gem_readl(bp, DCFG12);
> + if (GEM_BFEXT(HIGH_SPEED, dcfg) == 1)
> + bp->caps |= MACB_CAPS_HIGH_SPEED;
> dcfg = gem_readl(bp, DCFG2);
> if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
> bp->caps |= MACB_CAPS_FIFO_MODE;
> @@ -4404,7 +4536,18 @@ static int macb_probe(struct platform_device *pdev)
> } else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) {
> u32 interface_supported = 1;
>
> - if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
> + if (phy_mode == PHY_INTERFACE_MODE_USXGMII) {
> + if (!(bp->caps & MACB_CAPS_HIGH_SPEED &&
> + bp->caps & MACB_CAPS_PCS))
> + interface_supported = 0;
> +
> + if (of_property_read_u32(np, "serdes-rate-gbps",
> + &bp->serdes_rate)) {
> + netdev_err(dev,
> + "GEM serdes_rate not specified");
> + interface_supported = 0;
> + }
> + } else if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
> if (!(bp->caps & MACB_CAPS_PCS))
> interface_supported = 0;
> } else if (phy_mode == PHY_INTERFACE_MODE_GMII ||
> --
> 2.17.1
>
>

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 08:52:05

by Parshuram Raju Thombare

[permalink] [raw]
Subject: RE: [PATCH v5 4/5] net: macb: add support for high speed interface

>> switch (state->interface) {
>> case PHY_INTERFACE_MODE_NA:
>> + case PHY_INTERFACE_MODE_USXGMII:
>> + case PHY_INTERFACE_MODE_10GKR:
>> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>> + phylink_set(mask, 10000baseCR_Full);
>> + phylink_set(mask, 10000baseER_Full);
>> + phylink_set(mask, 10000baseKR_Full);
>> + phylink_set(mask, 10000baseLR_Full);
>> + phylink_set(mask, 10000baseLRM_Full);
>> + phylink_set(mask, 10000baseSR_Full);
>> + phylink_set(mask, 10000baseT_Full);
>> + phylink_set(mask, 5000baseT_Full);
>> + phylink_set(mask, 2500baseX_Full);
>> + phylink_set(mask, 1000baseX_Full);
>> + }
>If MACB_CAPS_GIGABIT_MODE_AVAILABLE is not set, are these interface
>modes supported by the hardware? If the PHY interface mode is not
>supported, then the returned support mask must be cleared.[]
There are some configs which uses this macro to limit data rate to 100M
even if hardware support higher rates.
Empty link mode mask is initialized at the beginning and supported link
modes are added to it and at the end of this function this mask is AND'ed
with supported mask.

>> +static inline void gem_mac_configure(struct macb *bp, int speed)
>> +{
>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
>> + GEM_BIT(PCSSEL) |
>> + gem_readl(bp, NCFGR));
>Is this still necessary?
Sorry, missed this. I will remove in next patch set.

Regards,
Parshuram Thombare

2019-06-25 08:56:10

by Parshuram Raju Thombare

[permalink] [raw]
Subject: RE: [PATCH v5 4/5] net: macb: add support for high speed interface

>> +static inline void gem_mac_configure(struct macb *bp, int speed)
>> + switch (speed) {
>> + case SPEED_1000:
>> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
>> + gem_readl(bp, NCFGR));
>> + break;
>> + case SPEED_100:
>> + macb_writel(bp, NCFGR, MACB_BIT(SPD) |
>> + macb_readl(bp, NCFGR));
>What happens to the NCFGR register if we call mac_config() first for
>a 1G speed, then 100M and finally 10M - what value does the NCFGR
>register end up with?
>
>I suspect it ends up with both the GBE and SPD bits set, and that is
>probably not what you want.

No, In gem_mac_config GBE and SPD bits are always cleared
before setting appropriate bits as per requested speed, duplex mode.


Regards,
Parshuram Thombare

2019-06-25 09:04:25

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v5 4/5] net: macb: add support for high speed interface

On Tue, Jun 25, 2019 at 08:49:33AM +0000, Parshuram Raju Thombare wrote:
> >> switch (state->interface) {
> >> case PHY_INTERFACE_MODE_NA:
> >> + case PHY_INTERFACE_MODE_USXGMII:
> >> + case PHY_INTERFACE_MODE_10GKR:
> >> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> >> + phylink_set(mask, 10000baseCR_Full);
> >> + phylink_set(mask, 10000baseER_Full);
> >> + phylink_set(mask, 10000baseKR_Full);
> >> + phylink_set(mask, 10000baseLR_Full);
> >> + phylink_set(mask, 10000baseLRM_Full);
> >> + phylink_set(mask, 10000baseSR_Full);
> >> + phylink_set(mask, 10000baseT_Full);
> >> + phylink_set(mask, 5000baseT_Full);
> >> + phylink_set(mask, 2500baseX_Full);
> >> + phylink_set(mask, 1000baseX_Full);
> >> + }
> >If MACB_CAPS_GIGABIT_MODE_AVAILABLE is not set, are these interface
> >modes supported by the hardware? If the PHY interface mode is not
> >supported, then the returned support mask must be cleared.[]
> There are some configs which uses this macro to limit data rate to 100M
> even if hardware support higher rates.

I'm sorry, this response does not address my statement, maybe I wasn't
clear enough. I am asking about the *PHY* interface modes, in
other words (e.g.) PHY_INTERFACE_MODE_USXGMII.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 11:58:43

by Parshuram Raju Thombare

[permalink] [raw]
Subject: RE: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface

>> + if (change_interface) {
>> + bp->phy_interface = state->interface;
>> + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
>> + gem_readl(bp, NCR));
>This could do with a comment, such as the one I gave in my example.
Sure. I will add a comment here.

>> @@ -493,6 +516,7 @@ static void gem_mac_config(struct phylink_config
>*pl_config, unsigned int mode,
>> reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>> if (macb_is_gem(bp))
>> reg &= ~GEM_BIT(GBE);
>> +
>Useless change.
Ok, I will remove this empty line.


>> + if (phy_mode == PHY_INTERFACE_MODE_SGMII) {
>> + if (!(bp->caps & MACB_CAPS_PCS))
>> + interface_supported = 0;
>
>So if bp->caps does not have MACB_CAPS_PCS set, then SGMII mode is not
>supported?
Yes

>In which case, gem_phylink_validate() must clear the support mask when
>SGMII mode is requested to indicate that the interface mode is not
>supported.
>The same goes for _all_ other PHY link modes that the hardware does not
>actually support, such as PHY_INTERFACE_MODE_10GKR...
If interface is not supported by hardware probe returns with error, so we don't
net interface is not registered at all.
I think what is missing is setting appropriate err value (-ENOTSUPP ?), right now it is returning
default err.



Regards,
Parshuram Thombare

2019-06-25 12:00:15

by Parshuram Raju Thombare

[permalink] [raw]
Subject: RE: [PATCH v5 4/5] net: macb: add support for high speed interface

>> >> switch (state->interface) {
>> >> case PHY_INTERFACE_MODE_NA:
>> >> + case PHY_INTERFACE_MODE_USXGMII:
>> >> + case PHY_INTERFACE_MODE_10GKR:
>> >> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>> >> + phylink_set(mask, 10000baseCR_Full);
>> >> + phylink_set(mask, 10000baseER_Full);
>> >> + phylink_set(mask, 10000baseKR_Full);
>> >> + phylink_set(mask, 10000baseLR_Full);
>> >> + phylink_set(mask, 10000baseLRM_Full);
>> >> + phylink_set(mask, 10000baseSR_Full);
>> >> + phylink_set(mask, 10000baseT_Full);
>> >> + phylink_set(mask, 5000baseT_Full);
>> >> + phylink_set(mask, 2500baseX_Full);
>> >> + phylink_set(mask, 1000baseX_Full);
>> >> + }
>> >If MACB_CAPS_GIGABIT_MODE_AVAILABLE is not set, are these interface
>> >modes supported by the hardware? If the PHY interface mode is not
>> >supported, then the returned support mask must be cleared.[]
>> There are some configs which uses this macro to limit data rate to 100M
>> even if hardware support higher rates.
>I'm sorry, this response does not address my statement, maybe I wasn't
>clear enough. I am asking about the *PHY* interface modes, in
>other words (e.g.) PHY_INTERFACE_MODE_USXGMII.

If interface is not supported by hardware probe returns with error, so net
device is not registered at all.

Regards,
Parshuram Thombare

2019-06-25 12:00:51

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface

On Tue, Jun 25, 2019 at 09:26:29AM +0000, Parshuram Raju Thombare wrote:
> >In which case, gem_phylink_validate() must clear the support mask when
> >SGMII mode is requested to indicate that the interface mode is not
> >supported.
> >The same goes for _all_ other PHY link modes that the hardware does not
> >actually support, such as PHY_INTERFACE_MODE_10GKR...
> If interface is not supported by hardware probe returns with error, so we don't
> net interface is not registered at all.

That does not negate my comment above.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 12:01:41

by Parshuram Raju Thombare

[permalink] [raw]
Subject: RE: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface


>> >In which case, gem_phylink_validate() must clear the support mask when
>> >SGMII mode is requested to indicate that the interface mode is not
>> >supported.
>> >The same goes for _all_ other PHY link modes that the hardware does not
>> >actually support, such as PHY_INTERFACE_MODE_10GKR...
>> If interface is not supported by hardware probe returns with error, so we don't
>> net interface is not registered at all.
>That does not negate my comment above.
Sorry if I misunderstood your question, but hardware supports interfaces and based
on that link modes are supported. So if interface is not supported by hardware,
net device is not registered and there will be no phylink_validate call.
If hardware support particular interface, link modes supported by interface
are added to (not cleared from) supported mask, provided configs is not trying to limit
data rate using GIGABIT_ENABLE* macro.

Regards,
Parshuram Thombare

2019-06-25 12:28:59

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface

On Tue, Jun 25, 2019 at 09:38:37AM +0000, Parshuram Raju Thombare wrote:
>
> >> >In which case, gem_phylink_validate() must clear the support mask when
> >> >SGMII mode is requested to indicate that the interface mode is not
> >> >supported.
> >> >The same goes for _all_ other PHY link modes that the hardware does not
> >> >actually support, such as PHY_INTERFACE_MODE_10GKR...
> >> If interface is not supported by hardware probe returns with error, so we don't
> >> net interface is not registered at all.
> >That does not negate my comment above.
> Sorry if I misunderstood your question, but hardware supports interfaces and based
> on that link modes are supported. So if interface is not supported by hardware,
> net device is not registered and there will be no phylink_validate call.
> If hardware support particular interface, link modes supported by interface
> are added to (not cleared from) supported mask, provided configs is not trying to limit
> data rate using GIGABIT_ENABLE* macro.

Why do you want to use phylink?

If you are only interested in supporting 10G PHYs, you don't need
phylink for that.

If you are interested in supporting SFPs as well, then using phylink
makes sense, but you need to implement your phylink conversion properly,
and that means supporting dynamic switching of the PHY interface mode,
and allowing phylink to determine whether a PHY interface mode is
supported or not.

However, with what you've indicated through our discussion, your MAC
does not support BASE-X modes, nor does it support 10GBASE-R, both of
which are required for direct connection of SFP or SFP+ modules.

The only phy link mode that you support which SFPs can make use of is
SGMII, and that will only be useful for copper SFPs configured for
SGMII mode. It basically means you can not use any fiber SFPs.

The only other way SFPs can be supported is via an intermediary PHY to
convert the MAC interface to BASE-X, SGMII or 10GBASE-R, and we don't
yet have support for that in mainline.

So, given that you seem unwilling to take on board my comments, and
your hardware does not appear support SFPs, I'm wondering what the
point of this conversion actually is.

As a result of our reviews, I've been improving the documentation for
phylink, so there has been some positives coming out of this - which
will hopefully help others to avoid the same mistakes.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

2019-06-25 13:03:05

by Parshuram Raju Thombare

[permalink] [raw]
Subject: RE: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface

>If you are interested in supporting SFPs as well, then using phylink
>makes sense, but you need to implement your phylink conversion properly,
>and that means supporting dynamic switching of the PHY interface mode,
>and allowing phylink to determine whether a PHY interface mode is
>supported or not.
Yes, we want to support SFP+.
10G is tested in fixed mode on SFP+.
Based on your suggestions, dynamic switching of PHY interface is handled.
And check in probe is for hardware support for user selected interface,
which can be moved to validate callback but then supported mask will be empty.

>However, with what you've indicated through our discussion, your MAC
>does not support BASE-X modes, nor does it support 10GBASE-R, both of
>which are required for direct connection of SFP or SFP+ modules.
1000Base-X and 10GBASE-R supported by separate PCS.

Regards,
Parshuram Thombare