* [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G
@ 2019-12-13 9:40 Milind Parab
2019-12-13 9:41 ` [PATCH v2 1/3] net: macb: fix for fixed-link mode Milind Parab
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Milind Parab @ 2019-12-13 9:40 UTC (permalink / raw)
To: nicolas.nerre, andrew, antoine.tenart, f.fainelli, rmk+kernel
Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum,
brad.mouring, pthombar, Milind Parab
This patch series applies to Cadence Ethernet MAC Controller.
The first patch in this series is related to the patch that converts the
driver to phylink interface in net-next "net: macb: convert to phylink".
Next patch is for adding support for C45 interface and the final patch
adds 10G MAC support.
The patch sequences are detailed as below
1. 0001-net-macb-fix-for-fixed-link-mode
This patch fix the issue with fixed-link mode in macb phylink interface.
In fixed-link we don't need to parse phandle because it's better handled
by phylink_of_phy_connect()
2. 0002-net-macb-add-support-for-C45-MDIO-read-write
This patch is to support C45 interface to PHY. This upgrade is downward compatible.
All versions of the MAC (old and new) using the GPL driver support both Clause 22 and
Clause 45 operation. Whether the access is in Clause 22 or Clause 45 format depends
on the data pattern written to the PHY management register.
3. 0003-net-macb-add-support-for-high-speed-interface
This patch add support for 10G fixed mode.
Changes since v1:
1. phy_node reference count handling in patch 0001-net-macb-fix-for-fixed-link-mode
2. Using fixed value for HS_MAC_SPEED_x
Thanks,
Milind Parab
Milind Parab (3):
net: macb: fix for fixed-link mode
net: macb: add support for C45 MDIO read/write
net: macb: add support for high speed interface
drivers/net/ethernet/cadence/macb.h | 65 ++++++-
drivers/net/ethernet/cadence/macb_main.c | 224 +++++++++++++++++++----
2 files changed, 247 insertions(+), 42 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/3] net: macb: fix for fixed-link mode
2019-12-13 9:40 [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab
@ 2019-12-13 9:41 ` Milind Parab
2019-12-15 4:33 ` Jakub Kicinski
2019-12-13 9:41 ` [PATCH v2 2/3] net: macb: add support for C45 MDIO read/write Milind Parab
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Milind Parab @ 2019-12-13 9:41 UTC (permalink / raw)
To: nicolas.nerre, andrew, antoine.tenart, f.fainelli, rmk+kernel
Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum,
brad.mouring, pthombar, Milind Parab
This patch fix the issue with fixed link. With fixed-link
device opening fails due to macb_phylink_connect not
handling fixed-link mode, in which case no MAC-PHY connection
is needed and phylink_connect return success (0), however
in current driver attempt is made to search and connect to
PHY even for fixed-link.
Signed-off-by: Milind Parab <mparab@cadence.com>
---
drivers/net/ethernet/cadence/macb_main.c | 25 +++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 9c767ee252ac..8c1812f39927 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -611,21 +611,25 @@ static const struct phylink_mac_ops macb_phylink_ops = {
.mac_link_up = macb_mac_link_up,
};
+static bool macb_phy_handle_exists(struct device_node *dn)
+{
+ dn = of_parse_phandle(dn, "phy-handle", 0);
+ of_node_put(dn);
+ return dn != NULL;
+}
+
+
static int macb_phylink_connect(struct macb *bp)
{
struct net_device *dev = bp->dev;
struct phy_device *phydev;
+ struct device_node *dn = bp->pdev->dev.of_node;
int ret;
- if (bp->pdev->dev.of_node &&
- of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) {
- ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node,
- 0);
- if (ret) {
- netdev_err(dev, "Could not attach PHY (%d)\n", ret);
- return ret;
- }
- } else {
+ if (dn)
+ ret = phylink_of_phy_connect(bp->phylink, dn, 0);
+
+ if (!dn || (ret && !macb_phy_handle_exists(dn))) {
phydev = phy_find_first(bp->mii_bus);
if (!phydev) {
netdev_err(dev, "no PHY found\n");
@@ -638,6 +642,9 @@ static int macb_phylink_connect(struct macb *bp)
netdev_err(dev, "Could not attach to PHY (%d)\n", ret);
return ret;
}
+ } else if (ret) {
+ netdev_err(dev, "Could not attach PHY (%d)\n", ret);
+ return ret;
}
phylink_start(bp->phylink);
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] net: macb: add support for C45 MDIO read/write
2019-12-13 9:40 [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab
2019-12-13 9:41 ` [PATCH v2 1/3] net: macb: fix for fixed-link mode Milind Parab
@ 2019-12-13 9:41 ` Milind Parab
2019-12-15 14:56 ` Andrew Lunn
2019-12-13 9:42 ` [PATCH v2 3/3] net: macb: add support for high speed interface Milind Parab
2019-12-15 9:49 ` [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Parshuram Raju Thombare
3 siblings, 1 reply; 15+ messages in thread
From: Milind Parab @ 2019-12-13 9:41 UTC (permalink / raw)
To: nicolas.nerre, andrew, antoine.tenart, f.fainelli, rmk+kernel
Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum,
brad.mouring, pthombar, Milind Parab
This patch modify MDIO read/write functions to support
communication with C45 PHY.
Signed-off-by: Milind Parab <mparab@cadence.com>
---
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 19fe4f4867c7..dbf7070fcdba 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -630,10 +630,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 0x00000001
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8c1812f39927..ced32d2a85e1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -337,11 +337,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)
@@ -370,12 +389,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-13 9:40 [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab
2019-12-13 9:41 ` [PATCH v2 1/3] net: macb: fix for fixed-link mode Milind Parab
2019-12-13 9:41 ` [PATCH v2 2/3] net: macb: add support for C45 MDIO read/write Milind Parab
@ 2019-12-13 9:42 ` Milind Parab
2019-12-15 15:12 ` Russell King - ARM Linux admin
2019-12-15 9:49 ` [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Parshuram Raju Thombare
3 siblings, 1 reply; 15+ messages in thread
From: Milind Parab @ 2019-12-13 9:42 UTC (permalink / raw)
To: nicolas.nerre, andrew, antoine.tenart, f.fainelli, rmk+kernel
Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum,
brad.mouring, pthombar, Milind Parab
This patch add support for high speed USXGMII PCS and 10G
speed in Cadence ethernet controller driver.
Signed-off-by: Milind Parab <mparab@cadence.com>
---
drivers/net/ethernet/cadence/macb.h | 50 ++++++++
drivers/net/ethernet/cadence/macb_main.c | 138 ++++++++++++++++++++---
2 files changed, 170 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index dbf7070fcdba..b731807d1c49 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -76,10 +76,12 @@
#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 */
#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 */
@@ -164,6 +166,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 */
@@ -270,11 +275,19 @@
#define MACB_IRXFCS_OFFSET 19
#define MACB_IRXFCS_SIZE 1
+/* GEM specific NCR bitfields. */
+#define GEM_ENABLE_HS_MAC_OFFSET 31
+#define GEM_ENABLE_HS_MAC_SIZE 1
+
/* GEM specific NCFGR bitfields. */
+#define GEM_FD_OFFSET 1 /* Full duplex */
+#define GEM_FD_SIZE 1
#define GEM_GBE_OFFSET 10 /* Gigabit mode enable */
#define GEM_GBE_SIZE 1
#define GEM_PCSSEL_OFFSET 11
#define GEM_PCSSEL_SIZE 1
+#define GEM_PAE_OFFSET 13 /* Pause enable */
+#define GEM_PAE_SIZE 1
#define GEM_CLK_OFFSET 18 /* MDC clock division */
#define GEM_CLK_SIZE 3
#define GEM_DBW_OFFSET 21 /* Data bus width */
@@ -455,11 +468,17 @@
#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 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
@@ -494,6 +513,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_SUBNSINCRL_OFFSET 24
@@ -656,6 +703,8 @@
#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000
#define MACB_CAPS_SG_DISABLED 0x40000000
#define MACB_CAPS_MACB_IS_GEM 0x80000000
+#define MACB_CAPS_PCS 0x01000000
+#define MACB_CAPS_HIGH_SPEED 0x02000000
/* LSO settings */
#define MACB_LSO_UFO_ENABLE 0x01
@@ -724,6 +773,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
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ced32d2a85e1..5963a60d54b9 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -81,6 +81,14 @@ struct sifive_fu540_macb_mgmt {
#define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
#define MACB_WOL_ENABLED (0x1 << 1)
+#define HS_MAC_SPEED_100M 0
+#define HS_MAC_SPEED_1000M 1
+#define HS_MAC_SPEED_2500M 2
+#define HS_MAC_SPEED_5000M 3
+#define HS_MAC_SPEED_10000M 4
+
+#define MACB_SERDES_RATE_10G 1
+
/* Graceful stop timeouts in us. We should allow up to
* 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
*/
@@ -90,6 +98,8 @@ struct sifive_fu540_macb_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:
*
@@ -506,6 +516,7 @@ static void macb_validate(struct phylink_config *config,
state->interface != PHY_INTERFACE_MODE_RMII &&
state->interface != PHY_INTERFACE_MODE_GMII &&
state->interface != PHY_INTERFACE_MODE_SGMII &&
+ state->interface != PHY_INTERFACE_MODE_USXGMII &&
!phy_interface_mode_is_rgmii(state->interface)) {
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
@@ -518,6 +529,13 @@ static void macb_validate(struct phylink_config *config,
return;
}
+ if (state->interface == PHY_INTERFACE_MODE_USXGMII &&
+ !(bp->caps & MACB_CAPS_HIGH_SPEED &&
+ bp->caps & MACB_CAPS_PCS)) {
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ return;
+ }
+
phylink_set_port_modes(mask);
phylink_set(mask, Autoneg);
phylink_set(mask, Asym_Pause);
@@ -527,6 +545,22 @@ static void macb_validate(struct phylink_config *config,
phylink_set(mask, 100baseT_Half);
phylink_set(mask, 100baseT_Full);
+ if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
+ (state->interface == PHY_INTERFACE_MODE_NA ||
+ state->interface == PHY_INTERFACE_MODE_USXGMII)) {
+ 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);
+ phylink_set(mask, 1000baseT_Full);
+ }
+
if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
(state->interface == PHY_INTERFACE_MODE_NA ||
state->interface == PHY_INTERFACE_MODE_GMII ||
@@ -544,6 +578,60 @@ static void macb_validate(struct phylink_config *config,
__ETHTOOL_LINK_MODE_MASK_NBITS);
}
+static int gem_mac_usx_configure(struct macb *bp,
+ const struct phylink_link_state *state)
+{
+ u32 speed, config, val;
+ int ret;
+
+ val = gem_readl(bp, NCFGR);
+ val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
+ if (state->pause & MLO_PAUSE_TX)
+ val |= GEM_BIT(PAE);
+ gem_writel(bp, NCFGR, val);
+ gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC));
+ gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) | GEM_BIT(FD));
+ config = gem_readl(bp, USX_CONTROL);
+ config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, 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));
+ ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
+ val & GEM_BIT(USX_BLOCK_LOCK),
+ 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
+ if (ret < 0) {
+ netdev_warn(bp->dev, "USXGMII block lock failed");
+ return -ETIMEDOUT;
+ }
+
+ switch (state->speed) {
+ case SPEED_10000:
+ speed = HS_MAC_SPEED_10000M;
+ 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 void macb_mac_pcs_get_state(struct phylink_config *config,
struct phylink_link_state *state)
{
@@ -565,30 +653,39 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
spin_lock_irqsave(&bp->lock, flags);
- old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
+ if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
+ if (gem_mac_usx_configure(bp, state) < 0) {
+ spin_unlock_irqrestore(&bp->lock, flags);
+ phylink_mac_change(bp->phylink, false);
+ return;
+ }
+ } else {
+ old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
- /* Clear all the bits we might set later */
- ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | MACB_BIT(FD) | MACB_BIT(PAE) |
- GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
+ /* Clear all the bits we might set later */
+ ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) |
+ MACB_BIT(FD) | MACB_BIT(PAE) |
+ GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
- if (state->speed == SPEED_1000)
- ctrl |= GEM_BIT(GBE);
- else if (state->speed == SPEED_100)
- ctrl |= MACB_BIT(SPD);
+ if (state->speed == SPEED_1000)
+ ctrl |= GEM_BIT(GBE);
+ else if (state->speed == SPEED_100)
+ ctrl |= MACB_BIT(SPD);
- if (state->duplex)
- ctrl |= MACB_BIT(FD);
+ if (state->duplex)
+ ctrl |= MACB_BIT(FD);
- /* We do not support MLO_PAUSE_RX yet */
- if (state->pause & MLO_PAUSE_TX)
- ctrl |= MACB_BIT(PAE);
+ /* We do not support MLO_PAUSE_RX yet */
+ if (state->pause & MLO_PAUSE_TX)
+ ctrl |= MACB_BIT(PAE);
- if (state->interface == PHY_INTERFACE_MODE_SGMII)
- ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
+ if (state->interface == PHY_INTERFACE_MODE_SGMII)
+ ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
- /* Apply the new configuration, if any */
- if (old_ctrl ^ ctrl)
- macb_or_gem_writel(bp, NCFGR, ctrl);
+ /* Apply the new configuration, if any */
+ if (old_ctrl ^ ctrl)
+ macb_or_gem_writel(bp, NCFGR, ctrl);
+ }
bp->speed = state->speed;
@@ -3407,6 +3504,11 @@ 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;
+ 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;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] net: macb: fix for fixed-link mode
2019-12-13 9:41 ` [PATCH v2 1/3] net: macb: fix for fixed-link mode Milind Parab
@ 2019-12-15 4:33 ` Jakub Kicinski
0 siblings, 0 replies; 15+ messages in thread
From: Jakub Kicinski @ 2019-12-15 4:33 UTC (permalink / raw)
To: Milind Parab
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, rmk+kernel,
davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum,
brad.mouring, pthombar
On Fri, 13 Dec 2019 09:41:01 +0000, Milind Parab wrote:
> This patch fix the issue with fixed link. With fixed-link
> device opening fails due to macb_phylink_connect not
> handling fixed-link mode, in which case no MAC-PHY connection
> is needed and phylink_connect return success (0), however
> in current driver attempt is made to search and connect to
> PHY even for fixed-link.
>
> Signed-off-by: Milind Parab <mparab@cadence.com>
We'll wait to give a chance for Russell, Andrew and others to review,
but this patch looks like a fix and the other ones look like features.
You should post the fix separately so it's included in Linus'es tree
ASAP (mark the patch with [PATCH net]), and the rest of the patches can
wait for the next merge window (mark [PATCH net-next]). Fixes should
also have an appropriate Fixes tag pointing at the first commit where
the bug was present.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G
2019-12-13 9:40 [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab
` (2 preceding siblings ...)
2019-12-13 9:42 ` [PATCH v2 3/3] net: macb: add support for high speed interface Milind Parab
@ 2019-12-15 9:49 ` Parshuram Raju Thombare
3 siblings, 0 replies; 15+ messages in thread
From: Parshuram Raju Thombare @ 2019-12-15 9:49 UTC (permalink / raw)
To: Milind Parab, nicolas.ferre, andrew, antoine.tenart, f.fainelli,
rmk+kernel
Cc: davem, netdev, hkallweit1, linux-kernel,
Dhananjay Vilasrao Kangude, a.fatoum, brad.mouring, Milind Parab
Corrected typo in email id of Nicolas Ferre.
>-----Original Message-----
>From: Milind Parab <mparab@cadence.com>
>Sent: Friday, December 13, 2019 3:10 PM
>To: nicolas.nerre@microchip.com; andrew@lunn.ch;
>antoine.tenart@bootlin.com; f.fainelli@gmail.com; rmk+kernel@armlinux.org.uk
>Cc: davem@davemloft.net; netdev@vger.kernel.org; hkallweit1@gmail.com;
>linux-kernel@vger.kernel.org; Dhananjay Vilasrao Kangude
><dkangude@cadence.com>; a.fatoum@pengutronix.de; brad.mouring@ni.com;
>Parshuram Raju Thombare <pthombar@cadence.com>; Milind Parab
><mparab@cadence.com>
>Subject: [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and
>10G
>
>This patch series applies to Cadence Ethernet MAC Controller.
>The first patch in this series is related to the patch that converts the
>driver to phylink interface in net-next "net: macb: convert to phylink".
>Next patch is for adding support for C45 interface and the final patch
>adds 10G MAC support.
>
>The patch sequences are detailed as below
>
>1. 0001-net-macb-fix-for-fixed-link-mode
>This patch fix the issue with fixed-link mode in macb phylink interface.
>In fixed-link we don't need to parse phandle because it's better handled
>by phylink_of_phy_connect()
>
>2. 0002-net-macb-add-support-for-C45-MDIO-read-write
>This patch is to support C45 interface to PHY. This upgrade is downward
>compatible.
>All versions of the MAC (old and new) using the GPL driver support both Clause 22
>and
>Clause 45 operation. Whether the access is in Clause 22 or Clause 45 format
>depends
>on the data pattern written to the PHY management register.
>
>3. 0003-net-macb-add-support-for-high-speed-interface
>This patch add support for 10G fixed mode.
>
>Changes since v1:
>1. phy_node reference count handling in patch 0001-net-macb-fix-for-fixed-link-
>mode
>2. Using fixed value for HS_MAC_SPEED_x
>
>Thanks,
>Milind Parab
>
>
>Milind Parab (3):
> net: macb: fix for fixed-link mode
> net: macb: add support for C45 MDIO read/write
> net: macb: add support for high speed interface
>
> drivers/net/ethernet/cadence/macb.h | 65 ++++++-
> drivers/net/ethernet/cadence/macb_main.c | 224 +++++++++++++++++++----
> 2 files changed, 247 insertions(+), 42 deletions(-)
>
>--
>2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] net: macb: add support for C45 MDIO read/write
2019-12-13 9:41 ` [PATCH v2 2/3] net: macb: add support for C45 MDIO read/write Milind Parab
@ 2019-12-15 14:56 ` Andrew Lunn
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2019-12-15 14:56 UTC (permalink / raw)
To: Milind Parab
Cc: nicolas.nerre, antoine.tenart, f.fainelli, rmk+kernel, davem,
netdev, hkallweit1, linux-kernel, dkangude, a.fatoum,
brad.mouring, pthombar
On Fri, Dec 13, 2019 at 09:41:51AM +0000, Milind Parab wrote:
> This patch modify MDIO read/write functions to support
> communication with C45 PHY.
>
> Signed-off-by: Milind Parab <mparab@cadence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-13 9:42 ` [PATCH v2 3/3] net: macb: add support for high speed interface Milind Parab
@ 2019-12-15 15:12 ` Russell King - ARM Linux admin
2019-12-15 15:20 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-15 15:12 UTC (permalink / raw)
To: Milind Parab
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev,
hkallweit1, linux-kernel, dkangude, a.fatoum, brad.mouring,
pthombar
On Fri, Dec 13, 2019 at 09:42:57AM +0000, Milind Parab wrote:
> This patch add support for high speed USXGMII PCS and 10G
> speed in Cadence ethernet controller driver.
>
> Signed-off-by: Milind Parab <mparab@cadence.com>
> ---
> drivers/net/ethernet/cadence/macb.h | 50 ++++++++
> drivers/net/ethernet/cadence/macb_main.c | 138 ++++++++++++++++++++---
> 2 files changed, 170 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index dbf7070fcdba..b731807d1c49 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -76,10 +76,12 @@
> #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 */
> #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 */
> @@ -164,6 +166,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 */
> @@ -270,11 +275,19 @@
> #define MACB_IRXFCS_OFFSET 19
> #define MACB_IRXFCS_SIZE 1
>
> +/* GEM specific NCR bitfields. */
> +#define GEM_ENABLE_HS_MAC_OFFSET 31
> +#define GEM_ENABLE_HS_MAC_SIZE 1
> +
> /* GEM specific NCFGR bitfields. */
> +#define GEM_FD_OFFSET 1 /* Full duplex */
> +#define GEM_FD_SIZE 1
> #define GEM_GBE_OFFSET 10 /* Gigabit mode enable */
> #define GEM_GBE_SIZE 1
> #define GEM_PCSSEL_OFFSET 11
> #define GEM_PCSSEL_SIZE 1
> +#define GEM_PAE_OFFSET 13 /* Pause enable */
> +#define GEM_PAE_SIZE 1
> #define GEM_CLK_OFFSET 18 /* MDC clock division */
> #define GEM_CLK_SIZE 3
> #define GEM_DBW_OFFSET 21 /* Data bus width */
> @@ -455,11 +468,17 @@
> #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 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
> @@ -494,6 +513,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_SUBNSINCRL_OFFSET 24
> @@ -656,6 +703,8 @@
> #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000
> #define MACB_CAPS_SG_DISABLED 0x40000000
> #define MACB_CAPS_MACB_IS_GEM 0x80000000
> +#define MACB_CAPS_PCS 0x01000000
> +#define MACB_CAPS_HIGH_SPEED 0x02000000
>
> /* LSO settings */
> #define MACB_LSO_UFO_ENABLE 0x01
> @@ -724,6 +773,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
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ced32d2a85e1..5963a60d54b9 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -81,6 +81,14 @@ struct sifive_fu540_macb_mgmt {
> #define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
> #define MACB_WOL_ENABLED (0x1 << 1)
>
> +#define HS_MAC_SPEED_100M 0
> +#define HS_MAC_SPEED_1000M 1
> +#define HS_MAC_SPEED_2500M 2
> +#define HS_MAC_SPEED_5000M 3
> +#define HS_MAC_SPEED_10000M 4
> +
> +#define MACB_SERDES_RATE_10G 1
> +
> /* Graceful stop timeouts in us. We should allow up to
> * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
> */
> @@ -90,6 +98,8 @@ struct sifive_fu540_macb_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:
> *
> @@ -506,6 +516,7 @@ static void macb_validate(struct phylink_config *config,
> state->interface != PHY_INTERFACE_MODE_RMII &&
> state->interface != PHY_INTERFACE_MODE_GMII &&
> state->interface != PHY_INTERFACE_MODE_SGMII &&
> + state->interface != PHY_INTERFACE_MODE_USXGMII &&
> !phy_interface_mode_is_rgmii(state->interface)) {
> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> return;
> @@ -518,6 +529,13 @@ static void macb_validate(struct phylink_config *config,
> return;
> }
>
> + if (state->interface == PHY_INTERFACE_MODE_USXGMII &&
> + !(bp->caps & MACB_CAPS_HIGH_SPEED &&
> + bp->caps & MACB_CAPS_PCS)) {
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + return;
> + }
> +
> phylink_set_port_modes(mask);
> phylink_set(mask, Autoneg);
> phylink_set(mask, Asym_Pause);
> @@ -527,6 +545,22 @@ static void macb_validate(struct phylink_config *config,
> phylink_set(mask, 100baseT_Half);
> phylink_set(mask, 100baseT_Full);
>
> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> + (state->interface == PHY_INTERFACE_MODE_NA ||
> + state->interface == PHY_INTERFACE_MODE_USXGMII)) {
> + 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);
> + phylink_set(mask, 1000baseT_Full);
> + }
> +
> if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> (state->interface == PHY_INTERFACE_MODE_NA ||
> state->interface == PHY_INTERFACE_MODE_GMII ||
> @@ -544,6 +578,60 @@ static void macb_validate(struct phylink_config *config,
> __ETHTOOL_LINK_MODE_MASK_NBITS);
> }
>
> +static int gem_mac_usx_configure(struct macb *bp,
> + const struct phylink_link_state *state)
> +{
> + u32 speed, config, val;
> + int ret;
> +
> + val = gem_readl(bp, NCFGR);
> + val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
> + if (state->pause & MLO_PAUSE_TX)
> + val |= GEM_BIT(PAE);
> + gem_writel(bp, NCFGR, val);
> + gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC));
> + gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) | GEM_BIT(FD));
> + config = gem_readl(bp, USX_CONTROL);
> + config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, 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));
> + ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
> + val & GEM_BIT(USX_BLOCK_LOCK),
> + 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
> + if (ret < 0) {
> + netdev_warn(bp->dev, "USXGMII block lock failed");
> + return -ETIMEDOUT;
> + }
I mentioned this last time around, so I'm a little surprised it's
still here. As I already stated, there is no requirement that the
USXGMII locks before any of these functions return.
You're also waiting for up to a second in a work queue, which is not
nice behaviour.
> +
> + switch (state->speed) {
> + case SPEED_10000:
> + speed = HS_MAC_SPEED_10000M;
> + 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;
macb_validate() goes down to 10Mbps, but you don't handle that here.
If it isn't supported, then macb_validate() shouldn't allow it for this
mode.
> + }
> +
> + 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 void macb_mac_pcs_get_state(struct phylink_config *config,
> struct phylink_link_state *state)
> {
> @@ -565,30 +653,39 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
>
> spin_lock_irqsave(&bp->lock, flags);
>
> - old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
> + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
Why bp->phy_interface and not state->interface?
If you don't support selecting between USXGMII and other modes at
runtime, should macb_validate() be allowing ethtool link modes for
it when it's different from the configured setting?
> + if (gem_mac_usx_configure(bp, state) < 0) {
> + spin_unlock_irqrestore(&bp->lock, flags);
> + phylink_mac_change(bp->phylink, false);
I guess this is the reason you're waiting for the USXGMII block
to lock - do you not have any way to raise an interrupt when
something changes with the USXGMII (or for that matter SGMII)
blocks? Without that, you're fixed to a single speed.
> + return;
> + }
> + } else {
> + old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
>
> - /* Clear all the bits we might set later */
> - ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | MACB_BIT(FD) | MACB_BIT(PAE) |
> - GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
> + /* Clear all the bits we might set later */
> + ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) |
> + MACB_BIT(FD) | MACB_BIT(PAE) |
> + GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
>
> - if (state->speed == SPEED_1000)
> - ctrl |= GEM_BIT(GBE);
> - else if (state->speed == SPEED_100)
> - ctrl |= MACB_BIT(SPD);
> + if (state->speed == SPEED_1000)
> + ctrl |= GEM_BIT(GBE);
> + else if (state->speed == SPEED_100)
> + ctrl |= MACB_BIT(SPD);
>
> - if (state->duplex)
> - ctrl |= MACB_BIT(FD);
> + if (state->duplex)
> + ctrl |= MACB_BIT(FD);
>
> - /* We do not support MLO_PAUSE_RX yet */
> - if (state->pause & MLO_PAUSE_TX)
> - ctrl |= MACB_BIT(PAE);
> + /* We do not support MLO_PAUSE_RX yet */
> + if (state->pause & MLO_PAUSE_TX)
> + ctrl |= MACB_BIT(PAE);
>
> - if (state->interface == PHY_INTERFACE_MODE_SGMII)
> - ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
> + if (state->interface == PHY_INTERFACE_MODE_SGMII)
> + ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>
> - /* Apply the new configuration, if any */
> - if (old_ctrl ^ ctrl)
> - macb_or_gem_writel(bp, NCFGR, ctrl);
> + /* Apply the new configuration, if any */
> + if (old_ctrl ^ ctrl)
> + macb_or_gem_writel(bp, NCFGR, ctrl);
> + }
>
> bp->speed = state->speed;
>
> @@ -3407,6 +3504,11 @@ 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;
> + 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;
> --
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-15 15:12 ` Russell King - ARM Linux admin
@ 2019-12-15 15:20 ` Russell King - ARM Linux admin
2019-12-16 12:49 ` Milind Parab
0 siblings, 1 reply; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-15 15:20 UTC (permalink / raw)
To: Milind Parab
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev,
hkallweit1, linux-kernel, dkangude, a.fatoum, brad.mouring,
pthombar
On Sun, Dec 15, 2019 at 03:12:49PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Dec 13, 2019 at 09:42:57AM +0000, Milind Parab wrote:
> > This patch add support for high speed USXGMII PCS and 10G
> > speed in Cadence ethernet controller driver.
> >
> > Signed-off-by: Milind Parab <mparab@cadence.com>
> > ---
> > drivers/net/ethernet/cadence/macb.h | 50 ++++++++
> > drivers/net/ethernet/cadence/macb_main.c | 138 ++++++++++++++++++++---
> > 2 files changed, 170 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > index dbf7070fcdba..b731807d1c49 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
> > @@ -76,10 +76,12 @@
> > #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 */
> > #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 */
> > @@ -164,6 +166,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 */
> > @@ -270,11 +275,19 @@
> > #define MACB_IRXFCS_OFFSET 19
> > #define MACB_IRXFCS_SIZE 1
> >
> > +/* GEM specific NCR bitfields. */
> > +#define GEM_ENABLE_HS_MAC_OFFSET 31
> > +#define GEM_ENABLE_HS_MAC_SIZE 1
> > +
> > /* GEM specific NCFGR bitfields. */
> > +#define GEM_FD_OFFSET 1 /* Full duplex */
> > +#define GEM_FD_SIZE 1
> > #define GEM_GBE_OFFSET 10 /* Gigabit mode enable */
> > #define GEM_GBE_SIZE 1
> > #define GEM_PCSSEL_OFFSET 11
> > #define GEM_PCSSEL_SIZE 1
> > +#define GEM_PAE_OFFSET 13 /* Pause enable */
> > +#define GEM_PAE_SIZE 1
> > #define GEM_CLK_OFFSET 18 /* MDC clock division */
> > #define GEM_CLK_SIZE 3
> > #define GEM_DBW_OFFSET 21 /* Data bus width */
> > @@ -455,11 +468,17 @@
> > #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 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
> > @@ -494,6 +513,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_SUBNSINCRL_OFFSET 24
> > @@ -656,6 +703,8 @@
> > #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000
> > #define MACB_CAPS_SG_DISABLED 0x40000000
> > #define MACB_CAPS_MACB_IS_GEM 0x80000000
> > +#define MACB_CAPS_PCS 0x01000000
> > +#define MACB_CAPS_HIGH_SPEED 0x02000000
> >
> > /* LSO settings */
> > #define MACB_LSO_UFO_ENABLE 0x01
> > @@ -724,6 +773,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
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index ced32d2a85e1..5963a60d54b9 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -81,6 +81,14 @@ struct sifive_fu540_macb_mgmt {
> > #define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
> > #define MACB_WOL_ENABLED (0x1 << 1)
> >
> > +#define HS_MAC_SPEED_100M 0
> > +#define HS_MAC_SPEED_1000M 1
> > +#define HS_MAC_SPEED_2500M 2
> > +#define HS_MAC_SPEED_5000M 3
> > +#define HS_MAC_SPEED_10000M 4
> > +
> > +#define MACB_SERDES_RATE_10G 1
> > +
> > /* Graceful stop timeouts in us. We should allow up to
> > * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
> > */
> > @@ -90,6 +98,8 @@ struct sifive_fu540_macb_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:
> > *
> > @@ -506,6 +516,7 @@ static void macb_validate(struct phylink_config *config,
> > state->interface != PHY_INTERFACE_MODE_RMII &&
> > state->interface != PHY_INTERFACE_MODE_GMII &&
> > state->interface != PHY_INTERFACE_MODE_SGMII &&
> > + state->interface != PHY_INTERFACE_MODE_USXGMII &&
> > !phy_interface_mode_is_rgmii(state->interface)) {
> > bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > return;
> > @@ -518,6 +529,13 @@ static void macb_validate(struct phylink_config *config,
> > return;
> > }
> >
> > + if (state->interface == PHY_INTERFACE_MODE_USXGMII &&
> > + !(bp->caps & MACB_CAPS_HIGH_SPEED &&
> > + bp->caps & MACB_CAPS_PCS)) {
> > + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > + return;
> > + }
> > +
> > phylink_set_port_modes(mask);
> > phylink_set(mask, Autoneg);
> > phylink_set(mask, Asym_Pause);
> > @@ -527,6 +545,22 @@ static void macb_validate(struct phylink_config *config,
> > phylink_set(mask, 100baseT_Half);
> > phylink_set(mask, 100baseT_Full);
> >
> > + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> > + (state->interface == PHY_INTERFACE_MODE_NA ||
> > + state->interface == PHY_INTERFACE_MODE_USXGMII)) {
> > + 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);
> > + phylink_set(mask, 1000baseT_Full);
> > + }
> > +
> > if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> > (state->interface == PHY_INTERFACE_MODE_NA ||
> > state->interface == PHY_INTERFACE_MODE_GMII ||
> > @@ -544,6 +578,60 @@ static void macb_validate(struct phylink_config *config,
> > __ETHTOOL_LINK_MODE_MASK_NBITS);
> > }
> >
> > +static int gem_mac_usx_configure(struct macb *bp,
> > + const struct phylink_link_state *state)
> > +{
> > + u32 speed, config, val;
> > + int ret;
> > +
> > + val = gem_readl(bp, NCFGR);
> > + val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
> > + if (state->pause & MLO_PAUSE_TX)
> > + val |= GEM_BIT(PAE);
> > + gem_writel(bp, NCFGR, val);
> > + gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC));
> > + gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) | GEM_BIT(FD));
> > + config = gem_readl(bp, USX_CONTROL);
> > + config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, 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));
> > + ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
> > + val & GEM_BIT(USX_BLOCK_LOCK),
> > + 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
> > + if (ret < 0) {
> > + netdev_warn(bp->dev, "USXGMII block lock failed");
> > + return -ETIMEDOUT;
> > + }
>
> I mentioned this last time around, so I'm a little surprised it's
> still here. As I already stated, there is no requirement that the
> USXGMII locks before any of these functions return.
>
> You're also waiting for up to a second in a work queue, which is not
> nice behaviour.
>
> > +
> > + switch (state->speed) {
> > + case SPEED_10000:
> > + speed = HS_MAC_SPEED_10000M;
> > + 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;
>
> macb_validate() goes down to 10Mbps, but you don't handle that here.
> If it isn't supported, then macb_validate() shouldn't allow it for this
> mode.
>
> > + }
> > +
> > + 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 void macb_mac_pcs_get_state(struct phylink_config *config,
> > struct phylink_link_state *state)
> > {
> > @@ -565,30 +653,39 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
> >
> > spin_lock_irqsave(&bp->lock, flags);
> >
> > - old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
> > + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
>
> Why bp->phy_interface and not state->interface?
>
> If you don't support selecting between USXGMII and other modes at
> runtime, should macb_validate() be allowing ethtool link modes for
> it when it's different from the configured setting?
>
> > + if (gem_mac_usx_configure(bp, state) < 0) {
> > + spin_unlock_irqrestore(&bp->lock, flags);
> > + phylink_mac_change(bp->phylink, false);
>
> I guess this is the reason you're waiting for the USXGMII block
> to lock - do you not have any way to raise an interrupt when
> something changes with the USXGMII (or for that matter SGMII)
> blocks? Without that, you're fixed to a single speed.
BTW, if you don't have an macb_mac_pcs_get_state() implementation,
and from what you described last time around, I don't see how SGMII
nor this new addition of USXGMII can work for you. Both these
protocols use in-band control words, which should be read and
interpreted in macb_mac_pcs_get_state().
What I think you're trying to do is to use your PCS PHY as a normal
PHY, or maybe you're ignoring the PCS PHY completely and relying on
an external PHY (and hence always using MLO_AN_PHY or MLO_AN_FIXED
mode.)
--
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-15 15:20 ` Russell King - ARM Linux admin
@ 2019-12-16 12:49 ` Milind Parab
2019-12-16 13:09 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 15+ messages in thread
From: Milind Parab @ 2019-12-16 12:49 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev,
hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum,
brad.mouring, Parshuram Raju Thombare
>> > + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
>>
>> Why bp->phy_interface and not state->interface?
okay, this needs to change to state->interface
>>
>> If you don't support selecting between USXGMII and other modes at
>> runtime, should macb_validate() be allowing ethtool link modes for
>> it when it's different from the configured setting?
We have separate SGMII and USXGMII PCS, which are enabled and programmed
by MAC driver. Also, there are separate low speed (up to 1G) and high
speed MAC which can be programmed though MAC driver.
As long as, PHY (PMA, external to Cadence MAC controller) can handle
this change, GEM can work with interface changes at a runtime.
>>
>> > + if (gem_mac_usx_configure(bp, state) < 0) {
>> > + spin_unlock_irqrestore(&bp->lock, flags);
>> > + phylink_mac_change(bp->phylink, false);
>>
>> I guess this is the reason you're waiting for the USXGMII block
>> to lock - do you not have any way to raise an interrupt when
>> something changes with the USXGMII (or for that matter SGMII)
>> blocks? Without that, you're fixed to a single speed.
Yes, we need to wait (poll) until USXGMII block lock is set.
Interrupt for USXGMII block lock set event is not supported.
>
>BTW, if you don't have an macb_mac_pcs_get_state() implementation,
>and from what you described last time around, I don't see how SGMII
>nor this new addition of USXGMII can work for you. Both these
>protocols use in-band control words, which should be read and
>interpreted in macb_mac_pcs_get_state().
>
>What I think you're trying to do is to use your PCS PHY as a normal
>PHY, or maybe you're ignoring the PCS PHY completely and relying on
>an external PHY (and hence always using MLO_AN_PHY or MLO_AN_FIXED
>mode.)
We are limiting our functionality to 10G fixed link using PCS and SFP+
Though the Cadence MAC is a full functional ethernet MAC controller,
we are not sure what PHY or PCS be used in the end system.
Hence we are using PCS PHY as a normal PHY and not dependent on
macb_mac_pcs_get_state(). Also it should be noted that we are
not doing any change in SGMII. Status available in PCS is
just a "status transferred" from PHY. So in case of SGMII, whether
we read from PCS or from PHY, it is the same information.
Below are listed all the possible use cases of Cadence GEM 10G controller
Basic MII MAC/PHY interconnect using MDIO for link status xfer.
+-------------+ +--------+
| | | |
| GEM MAC/DMA | <------ GMII/RGMII/RMII/MII -----> | PHY |
| | | |
+-------------+ +--------+
^ ^
|_____________________ MDIO ______________________|
No PHY. No status xfer required. GEM PCS responsible for auto-negotiation
across link. Driver must interrogate PCS registers within GEM.
+-------------+ +--------+
| | | | | |
| GEM MAC/DMA | <---> | SerDes | <- 1000BASE-X -> | SFP |
| PCS | | (PMA) | | |
+-------------+ +--------+
SGMII MAC/PHY interconnect using MDIO for link status xfer.
+-------------+ +--------+
| | | | | |
| GEM MAC/DMA | <---> | SerDes | <--- SGMII ---> | PHY |
| SGMII PCS | | (PMA) | | |
+-------------+ +--------+
^ ^
|_____________________ MDIO ______________________|
SGMII MAC/PHY interconnect using inline status xfer. Multi-rate.
Driver must interrogate PCS registers within GEM.
+-------------+ +--------+
| | | | | |
| GEM MAC/DMA | <---> | SerDes | <--- SGMII ---> | PHY |
| SGMII PCS | | (PMA) | | |
+-------------+ +--------+
Up to 2.5G. MAC/PHY interconnect. Rate determined by 2.5GBASE-T PHY capability.
+--------------+ +-----------+
| | | | | |
| GEM MAC/DMA | <---> | SerDes | <-2500BASE-X-> |2.5GBASE-T |
|2.5GBASE-X PCS| | (PMA) | | PHY |
+--------------+ +-----------+
No ability for host to interrogate Optical.
+--------------+ +-----------+
| | | | | SFP+ |
| GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
| USX PCS| | | (PMA) | | Module |
+--------------+ +-----------+
Additional 3rd party I2C IP required (not part of GEM) for module
interrogation (MDIO to I2C handled by SW
+--------------+ +-----------+
| | | | | SFP+ |
| GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
| USX PCS| | | (PMA) | | Module |
+--------------+ +-----------+
^
+--------+ |
| I2C | |
| Master | <-------------------------------------|
+--------+
Rate determined by 10GBASE-T PHY capability through auto-negotiation.
I2C IP required
+--------------+ +-----------+
| | | | | SFP+ to |
| GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | 10GBASE-T |
| USX PCS| | | (PMA) | | |
+--------------+ +-----------+
^
+--------+ |
| I2C | |
| Master | <-------------------------------------|
+--------+
USXGMII PHY. Uses MDIO or equivalent for status xfer
+-------------+ +--------+
| | | | | |
| GEM MAC/DMA | <---> | SerDes | <--- USXGMII ---> | PHY |
| USX PCS | | (PMA) | | |
+-------------+ +--------+
^ ^
|_____________________ MDIO ______________________|
>
>
>
>--
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-16 12:49 ` Milind Parab
@ 2019-12-16 13:09 ` Russell King - ARM Linux admin
2019-12-16 14:30 ` Russell King - ARM Linux admin
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-16 13:09 UTC (permalink / raw)
To: Milind Parab
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev,
hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum,
brad.mouring, Parshuram Raju Thombare
On Mon, Dec 16, 2019 at 12:49:59PM +0000, Milind Parab wrote:
> >> > + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> >>
> >> Why bp->phy_interface and not state->interface?
>
> okay, this needs to change to state->interface
>
> >>
> >> If you don't support selecting between USXGMII and other modes at
> >> runtime, should macb_validate() be allowing ethtool link modes for
> >> it when it's different from the configured setting?
>
> We have separate SGMII and USXGMII PCS, which are enabled and programmed
> by MAC driver. Also, there are separate low speed (up to 1G) and high
> speed MAC which can be programmed though MAC driver.
> As long as, PHY (PMA, external to Cadence MAC controller) can handle
> this change, GEM can work with interface changes at a runtime.
>
> >>
> >> > + if (gem_mac_usx_configure(bp, state) < 0) {
> >> > + spin_unlock_irqrestore(&bp->lock, flags);
> >> > + phylink_mac_change(bp->phylink, false);
> >>
> >> I guess this is the reason you're waiting for the USXGMII block
> >> to lock - do you not have any way to raise an interrupt when
> >> something changes with the USXGMII (or for that matter SGMII)
> >> blocks? Without that, you're fixed to a single speed.
>
> Yes, we need to wait (poll) until USXGMII block lock is set.
> Interrupt for USXGMII block lock set event is not supported.
You should poll for that status. We already have some polling support
in phylink (in the case of a fixed link using the callback, or a GPIO
that has no interrupt support) so it probably makes sense to extend
that functionality for MACs that do not provide status interrupts.
> >BTW, if you don't have an macb_mac_pcs_get_state() implementation,
> >and from what you described last time around, I don't see how SGMII
> >nor this new addition of USXGMII can work for you. Both these
> >protocols use in-band control words, which should be read and
> >interpreted in macb_mac_pcs_get_state().
> >
> >What I think you're trying to do is to use your PCS PHY as a normal
> >PHY, or maybe you're ignoring the PCS PHY completely and relying on
> >an external PHY (and hence always using MLO_AN_PHY or MLO_AN_FIXED
> >mode.)
>
> We are limiting our functionality to 10G fixed link using PCS and SFP+
> Though the Cadence MAC is a full functional ethernet MAC controller,
> we are not sure what PHY or PCS be used in the end system.
> Hence we are using PCS PHY as a normal PHY and not dependent on
> macb_mac_pcs_get_state().
If you use the PCS PHY as a normal PHY, then this knocks out the
idea below of using phylib to access the external PHY - it is not
possible to stack two phylib controlled PHYs sanely on one network
device.
> Also it should be noted that we are
> not doing any change in SGMII. Status available in PCS is
> just a "status transferred" from PHY. So in case of SGMII, whether
> we read from PCS or from PHY, it is the same information.
So how do you plan to deal with a PHY that you can't read the
status from? This is where the SGMII in-band is required. Such
SFP modules do exist.
> Below are listed all the possible use cases of Cadence GEM 10G controller
>
> Basic MII MAC/PHY interconnect using MDIO for link status xfer.
> +-------------+ +--------+
> | | | |
> | GEM MAC/DMA | <------ GMII/RGMII/RMII/MII -----> | PHY |
> | | | |
> +-------------+ +--------+
> ^ ^
> |_____________________ MDIO ______________________|
>
> No PHY. No status xfer required. GEM PCS responsible for auto-negotiation
> across link. Driver must interrogate PCS registers within GEM.
> +-------------+ +--------+
> | | | | | |
> | GEM MAC/DMA | <---> | SerDes | <- 1000BASE-X -> | SFP |
> | PCS | | (PMA) | | |
> +-------------+ +--------+
This setup requires macb_mac_pcs_get_state() to be implemented to
interrogate the PCS at the MAC. Note that some SFPs require SGMII,
and others can also operate at 2500baseX.
> SGMII MAC/PHY interconnect using MDIO for link status xfer.
> +-------------+ +--------+
> | | | | | |
> | GEM MAC/DMA | <---> | SerDes | <--- SGMII ---> | PHY |
> | SGMII PCS | | (PMA) | | |
> +-------------+ +--------+
> ^ ^
> |_____________________ MDIO ______________________|
>
> SGMII MAC/PHY interconnect using inline status xfer. Multi-rate.
> Driver must interrogate PCS registers within GEM.
> +-------------+ +--------+
> | | | | | |
> | GEM MAC/DMA | <---> | SerDes | <--- SGMII ---> | PHY |
> | SGMII PCS | | (PMA) | | |
> +-------------+ +--------+
This setup requires macb_mac_pcs_get_state() to be implemented to
interogate the SGMII PCS at the MAC.
> Up to 2.5G. MAC/PHY interconnect. Rate determined by 2.5GBASE-T PHY capability.
> +--------------+ +-----------+
> | | | | | |
> | GEM MAC/DMA | <---> | SerDes | <-2500BASE-X-> |2.5GBASE-T |
> |2.5GBASE-X PCS| | (PMA) | | PHY |
> +--------------+ +-----------+
This is fixed at 2.5G speeds.
> No ability for host to interrogate Optical.
> +--------------+ +-----------+
> | | | | | SFP+ |
> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
> | USX PCS| | | (PMA) | | Module |
> +--------------+ +-----------+
>
> Additional 3rd party I2C IP required (not part of GEM) for module
> interrogation (MDIO to I2C handled by SW
> +--------------+ +-----------+
> | | | | | SFP+ |
> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
> | USX PCS| | | (PMA) | | Module |
> +--------------+ +-----------+
> ^
> +--------+ |
> | I2C | |
> | Master | <-------------------------------------|
> +--------+
The kernel supports this through the sfp and phylink support. SFI is
more commonly known as 10GBASE-R. Note that this is *not* USXGMII.
Link status needs to come from the MAC side, so macb_mac_pcs_get_state()
is required.
> Rate determined by 10GBASE-T PHY capability through auto-negotiation.
> I2C IP required
> +--------------+ +-----------+
> | | | | | SFP+ to |
> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | 10GBASE-T |
> | USX PCS| | | (PMA) | | |
> +--------------+ +-----------+
> ^
> +--------+ |
> | I2C | |
> | Master | <-------------------------------------|
> +--------+
The 10G copper module I have uses 10GBASE-R, 5000BASE-X, 2500BASE-X,
and SGMII (without in-band status), dynamically switching between
these depending on the results of the copper side negotiation.
> USXGMII PHY. Uses MDIO or equivalent for status xfer
> +-------------+ +--------+
> | | | | | |
> | GEM MAC/DMA | <---> | SerDes | <--- USXGMII ---> | PHY |
> | USX PCS | | (PMA) | | |
> +-------------+ +--------+
> ^ ^
> |_____________________ MDIO ______________________|
Overall, please implement phylink properly for your MAC, rather than
the current half-hearted approach that *will* break in various
circumstances.
I think part of the problem here is that you have a different view how
phylink should be used to cover all these cases from my view. I'm not
prepared to guarantee that the phylink code will work with your view
into the future. I would much prefer there to be consistency in the
way phylink is used between implementations so we don't end up with
major maintanence problems into the future, so having consistency is
important.
--
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-16 13:09 ` Russell King - ARM Linux admin
@ 2019-12-16 14:30 ` Russell King - ARM Linux admin
2019-12-20 9:05 ` Milind Parab
2019-12-21 11:08 ` Milind Parab
2 siblings, 0 replies; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-16 14:30 UTC (permalink / raw)
To: Milind Parab
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev,
hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum,
brad.mouring, Parshuram Raju Thombare
On Mon, Dec 16, 2019 at 01:09:08PM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Dec 16, 2019 at 12:49:59PM +0000, Milind Parab wrote:
> > >> > + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> > >>
> > >> Why bp->phy_interface and not state->interface?
> >
> > okay, this needs to change to state->interface
> >
> > >>
> > >> If you don't support selecting between USXGMII and other modes at
> > >> runtime, should macb_validate() be allowing ethtool link modes for
> > >> it when it's different from the configured setting?
> >
> > We have separate SGMII and USXGMII PCS, which are enabled and programmed
> > by MAC driver. Also, there are separate low speed (up to 1G) and high
> > speed MAC which can be programmed though MAC driver.
> > As long as, PHY (PMA, external to Cadence MAC controller) can handle
> > this change, GEM can work with interface changes at a runtime.
> >
> > >>
> > >> > + if (gem_mac_usx_configure(bp, state) < 0) {
> > >> > + spin_unlock_irqrestore(&bp->lock, flags);
> > >> > + phylink_mac_change(bp->phylink, false);
> > >>
> > >> I guess this is the reason you're waiting for the USXGMII block
> > >> to lock - do you not have any way to raise an interrupt when
> > >> something changes with the USXGMII (or for that matter SGMII)
> > >> blocks? Without that, you're fixed to a single speed.
> >
> > Yes, we need to wait (poll) until USXGMII block lock is set.
> > Interrupt for USXGMII block lock set event is not supported.
>
> You should poll for that status. We already have some polling support
> in phylink (in the case of a fixed link using the callback, or a GPIO
> that has no interrupt support) so it probably makes sense to extend
> that functionality for MACs that do not provide status interrupts.
And here's that extension (untested):
8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phylink: add support for polling MAC PCS
Some MAC PCS blocks are unable to provide interrupts when their status
changes. As we already have support in phylink for polling status, use
this to provide a hook for MACs to enable polling mode.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Documentation/networking/sfp-phylink.rst | 8 +++--
drivers/net/phy/phylink.c | 44 ++++++++++++++++++------
include/linux/phylink.h | 1 +
3 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/Documentation/networking/sfp-phylink.rst b/Documentation/networking/sfp-phylink.rst
index a5e00a159d21..5695204be09a 100644
--- a/Documentation/networking/sfp-phylink.rst
+++ b/Documentation/networking/sfp-phylink.rst
@@ -115,7 +115,7 @@ this documentation.
* - Original function
- Replacement function
* - phy_start(phydev)
- - phylink_start(priv->phylink)
+ - phylink_start(priv->phylink) or phylink_start_poll(priv->phylink)
* - phy_stop(phydev)
- phylink_stop(priv->phylink)
* - phy_mii_ioctl(phydev, ifr, cmd)
@@ -251,7 +251,9 @@ this documentation.
phylink_mac_change(priv->phylink, link_is_up);
where ``link_is_up`` is true if the link is currently up or false
- otherwise.
+ otherwise. If a MAC is uanble to provide these interrupts, then
+ :c:func:`phylink_start_poll` should be used in step 5.
+
11. Verify that the driver does not call::
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fc45657bb9c9..616d208b348e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -964,15 +964,7 @@ static irqreturn_t phylink_link_handler(int irq, void *data)
return IRQ_HANDLED;
}
-/**
- * phylink_start() - start a phylink instance
- * @pl: a pointer to a &struct phylink returned from phylink_create()
- *
- * Start the phylink instance specified by @pl, configuring the MAC for the
- * desired link mode(s) and negotiation style. This should be called from the
- * network device driver's &struct net_device_ops ndo_open() method.
- */
-void phylink_start(struct phylink *pl)
+static void __phylink_start(struct phylink *pl, bool poll)
{
ASSERT_RTNL();
@@ -1014,15 +1006,47 @@ void phylink_start(struct phylink *pl)
if (irq <= 0)
mod_timer(&pl->link_poll, jiffies + HZ);
}
- if (pl->cfg_link_an_mode == MLO_AN_FIXED && pl->get_fixed_state)
+ if ((pl->cfg_link_an_mode == MLO_AN_FIXED && pl->get_fixed_state) ||
+ poll)
mod_timer(&pl->link_poll, jiffies + HZ);
if (pl->phydev)
phy_start(pl->phydev);
if (pl->sfp_bus)
sfp_upstream_start(pl->sfp_bus);
}
+
+/**
+ * phylink_start() - start a phylink instance
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Start the phylink instance specified by @pl, configuring the MAC for the
+ * desired link mode(s) and negotiation style. This should be called from the
+ * network device driver's &struct net_device_ops ndo_open() method.
+ */
+void phylink_start(struct phylink *pl)
+{
+ __phylink_start(pl, false);
+}
EXPORT_SYMBOL_GPL(phylink_start);
+/**
+ * phylink_start_poll() - start a phylink instance with polling
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Start the phylink instance specified by @pl, as per phylink_start(), but
+ * also enable polling mode. This should be used if the MAC has no support
+ * for MAC PCS status interrupts.
+ *
+ * The MAC PCS must ensure that if it detects the link going down, that must
+ * be reported via mac_pcs_get_state() method to ensure proper update of the
+ * MAC.
+ */
+void phylink_start_poll(struct phylink *pl)
+{
+ __phylink_start(pl, true);
+}
+EXPORT_SYMBOL_GPL(phylink_start_poll);
+
/**
* phylink_stop() - stop a phylink instance
* @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index bedff1a217fe..529fb5ce440d 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -250,6 +250,7 @@ int phylink_fixed_state_cb(struct phylink *,
void phylink_mac_change(struct phylink *, bool up);
void phylink_start(struct phylink *);
+void phylink_start_poll(struct phylink *pl);
void phylink_stop(struct phylink *);
void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
--
2.20.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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-16 13:09 ` Russell King - ARM Linux admin
2019-12-16 14:30 ` Russell King - ARM Linux admin
@ 2019-12-20 9:05 ` Milind Parab
2019-12-21 11:08 ` Milind Parab
2 siblings, 0 replies; 15+ messages in thread
From: Milind Parab @ 2019-12-20 9:05 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev,
hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum,
brad.mouring, Parshuram Raju Thombare
>>
>> Additional 3rd party I2C IP required (not part of GEM) for module
>> interrogation (MDIO to I2C handled by SW
>> +--------------+ +-----------+
>> | | | | | SFP+ |
>> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
>> | USX PCS| | | (PMA) | | Module |
>> +--------------+ +-----------+
>> ^
>> +--------+ |
>> | I2C | |
>> | Master | <-------------------------------------|
>> +--------+
>The kernel supports this through the sfp and phylink support. SFI is
>more commonly known as 10GBASE-R. Note that this is *not* USXGMII.
>Link status needs to come from the MAC side, so macb_mac_pcs_get_state()
>is required.
>
>> Rate determined by 10GBASE-T PHY capability through auto-negotiation.
>> I2C IP required
>> +--------------+ +-----------+
>> | | | | | SFP+ to |
>> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | 10GBASE-T |
>> | USX PCS| | | (PMA) | | |
>> +--------------+ +-----------+
>> ^
>> +--------+ |
>> | I2C | |
>> | Master | <-------------------------------------|
>> +--------+
>
>The 10G copper module I have uses 10GBASE-R, 5000BASE-X, 2500BASE-X,
>and SGMII (without in-band status), dynamically switching between
>these depending on the results of the copper side negotiation.
>
>> USXGMII PHY. Uses MDIO or equivalent for status xfer
>> +-------------+ +--------+
>> | | | | | |
>> | GEM MAC/DMA | <---> | SerDes | <--- USXGMII ---> | PHY |
>> | USX PCS | | (PMA) | | |
>> +-------------+ +--------+
>> ^ ^
>> |_____________________ MDIO ______________________|
>
>Overall, please implement phylink properly for your MAC, rather than
>the current half-hearted approach that *will* break in various
>circumstances.
>
We would need more time to get back on the restructured implementation. While we
work on that, is it okay to accept patch 1/3 and patch 2/3?
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-16 13:09 ` Russell King - ARM Linux admin
2019-12-16 14:30 ` Russell King - ARM Linux admin
2019-12-20 9:05 ` Milind Parab
@ 2019-12-21 11:08 ` Milind Parab
2020-01-08 10:32 ` Nicolas.Ferre
2 siblings, 1 reply; 15+ messages in thread
From: Milind Parab @ 2019-12-21 11:08 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: andrew, antoine.tenart, f.fainelli, davem, netdev, hkallweit1,
linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum, brad.mouring,
Parshuram Raju Thombare, Nicolas.Ferre
>>
>> Additional 3rd party I2C IP required (not part of GEM) for module
>> interrogation (MDIO to I2C handled by SW
>> +--------------+ +-----------+
>> | | | | | SFP+ |
>> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
>> | USX PCS| | | (PMA) | | Module |
>> +--------------+ +-----------+
>> ^
>> +--------+ |
>> | I2C | |
>> | Master | <-------------------------------------|
>> +--------+
>The kernel supports this through the sfp and phylink support. SFI is
>more commonly known as 10GBASE-R. Note that this is *not* USXGMII.
>Link status needs to come from the MAC side, so macb_mac_pcs_get_state()
>is required.
>
>> Rate determined by 10GBASE-T PHY capability through auto-negotiation.
>> I2C IP required
>> +--------------+ +-----------+
>> | | | | | SFP+ to |
>> | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | 10GBASE-T |
>> | USX PCS| | | (PMA) | | |
>> +--------------+ +-----------+
>> ^
>> +--------+ |
>> | I2C | |
>> | Master | <-------------------------------------|
>> +--------+
>
>The 10G copper module I have uses 10GBASE-R, 5000BASE-X, 2500BASE-X,
>and SGMII (without in-band status), dynamically switching between
>these depending on the results of the copper side negotiation.
>
>> USXGMII PHY. Uses MDIO or equivalent for status xfer
>> +-------------+ +--------+
>> | | | | | |
>> | GEM MAC/DMA | <---> | SerDes | <--- USXGMII ---> | PHY |
>> | USX PCS | | (PMA) | | |
>> +-------------+ +--------+
>> ^ ^
>> |_____________________ MDIO ______________________|
>
>Overall, please implement phylink properly for your MAC, rather than
>the current half-hearted approach that *will* break in various
>circumstances.
>
We would need more time to get back on the restructured implementation.
While we work on that, is it okay to accept patch 1/3 and patch 2/3?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] net: macb: add support for high speed interface
2019-12-21 11:08 ` Milind Parab
@ 2020-01-08 10:32 ` Nicolas.Ferre
0 siblings, 0 replies; 15+ messages in thread
From: Nicolas.Ferre @ 2020-01-08 10:32 UTC (permalink / raw)
To: linux, mparab
Cc: antoine.tenart, dkangude, hkallweit1, andrew, davem,
linux-kernel, Claudiu.Beznea, netdev, f.fainelli, a.fatoum,
brad.mouring, pthombar
Le samedi 21 décembre 2019 à 11:08 +0000, Milind Parab a écrit :
> > > Additional 3rd party I2C IP required (not part of GEM) for module
> > > interrogation (MDIO to I2C handled by SW
> > > +--------------+ +-----------+
> > > | | | | | SFP+ |
> > > | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | Optical |
> > > | USX PCS| | | (PMA) | | Module |
> > > +--------------+ +-----------+
> > > ^
> > > +--------+ |
> > > | I2C | |
> > > | Master | <-------------------------------------|
> > > +--------+
> > The kernel supports this through the sfp and phylink support. SFI is
> > more commonly known as 10GBASE-R. Note that this is *not* USXGMII.
> > Link status needs to come from the MAC side, so macb_mac_pcs_get_state()
> > is required.
> >
> > > Rate determined by 10GBASE-T PHY capability through auto-negotiation.
> > > I2C IP required
> > > +--------------+ +-----------+
> > > | | | | | SFP+ to |
> > > | GEM MAC/DMA | <---> | SerDes | <---- SFI-----> | 10GBASE-T |
> > > | USX PCS| | | (PMA) | | |
> > > +--------------+ +-----------+
> > > ^
> > > +--------+ |
> > > | I2C | |
> > > | Master | <-------------------------------------|
> > > +--------+
> >
> > The 10G copper module I have uses 10GBASE-R, 5000BASE-X, 2500BASE-X,
> > and SGMII (without in-band status), dynamically switching between
> > these depending on the results of the copper side negotiation.
> >
> > > USXGMII PHY. Uses MDIO or equivalent for status xfer
> > > +-------------+ +--------+
> > > | | | | | |
> > > | GEM MAC/DMA | <---> | SerDes | <--- USXGMII ---> | PHY |
> > > | USX PCS | | (PMA) | | |
> > > +-------------+ +--------+
> > > ^ ^
> > > |_____________________ MDIO ______________________|
> >
> > Overall, please implement phylink properly for your MAC, rather than
> > the current half-hearted approach that *will* break in various
> > circumstances.
> >
>
> We would need more time to get back on the restructured implementation.
> While we work on that, is it okay to accept patch 1/3 and patch 2/3?
Milind,
I'm not against queuing patches 1 & 2 of this series while the 3rd one is
still in review.
However, I would prefer that you follow-up Jakub Kicinski's advice when he
answered to your v2 serries.
So please, re-send the patches 1 as a "fix" type of patch, making sure that it
still applies after latest Antoine's changes. Then, re-send the 2/3 patch of
the series as a v3 collecting reviews (as I didn't received v2).
When the first 2 are queued by David, we can resume the discussion about your
3rd patch and what I can tell you about this topic is that it's really by
following Russell comments and advice that we will make progress: I won't
accept anything without his acknowledgment, of course.
Best regards,
Nicolas
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-01-08 10:32 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-13 9:40 [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab
2019-12-13 9:41 ` [PATCH v2 1/3] net: macb: fix for fixed-link mode Milind Parab
2019-12-15 4:33 ` Jakub Kicinski
2019-12-13 9:41 ` [PATCH v2 2/3] net: macb: add support for C45 MDIO read/write Milind Parab
2019-12-15 14:56 ` Andrew Lunn
2019-12-13 9:42 ` [PATCH v2 3/3] net: macb: add support for high speed interface Milind Parab
2019-12-15 15:12 ` Russell King - ARM Linux admin
2019-12-15 15:20 ` Russell King - ARM Linux admin
2019-12-16 12:49 ` Milind Parab
2019-12-16 13:09 ` Russell King - ARM Linux admin
2019-12-16 14:30 ` Russell King - ARM Linux admin
2019-12-20 9:05 ` Milind Parab
2019-12-21 11:08 ` Milind Parab
2020-01-08 10:32 ` Nicolas.Ferre
2019-12-15 9:49 ` [PATCH v2 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Parshuram Raju Thombare
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).