* [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
@ 2016-06-07 3:01 thloh
2016-06-07 8:30 ` Giuseppe CAVALLARO
2016-06-08 19:52 ` Rob Herring
0 siblings, 2 replies; 7+ messages in thread
From: thloh @ 2016-06-07 3:01 UTC (permalink / raw)
To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
peppe.cavallaro, devicetree, linux-kernel, netdev
Cc: Tien Hock Loh
From: Tien Hock Loh <thloh@altera.com>
This adds support for TSE PCS that uses SGMII adapter when the phy-mode of
the dwmac is set to sgmii
Signed-off-by: Tien Hock Loh <thloh@altera.com>
---
v2:
- Refactored the TSE PCS out from the dwmac-socfpga.c file
- Added binding documentation for TSE PCS sgmii adapter
v3:
- Added missing license header for new source files
- Updated tse_pcs.h include headers
- Standardize if statements
---
.../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
5 files changed, 419 insertions(+), 24 deletions(-)
create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
diff --git a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
index 3a9d679..2bc39f1 100644
--- a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
@@ -15,6 +15,8 @@ Required properties:
Optional properties:
altr,emac-splitter: Should be the phandle to the emac splitter soft IP node if
DWMAC controller is connected emac splitter.
+phy-mode: The phy mode the ethernet operates in
+altr,sgmii_to_sgmii_converter: phandle to the TSE SGMII converter
Example:
@@ -28,4 +30,6 @@ gmac0: ethernet@ff700000 {
mac-address = [00 00 00 00 00 00];/* Filled in by U-Boot */
clocks = <&emac_0_clk>;
clock-names = "stmmaceth";
+ phy-mode = "sgmii";
+ altr,gmii_to_sgmii_converter = <&sgmii_1_gmii_to_sgmii_converter_0>;
};
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 73c2715..29c1dee 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \
obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o \
- dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
+ dwmac-sti.o dwmac-socfpga.o dwmac-rk.o tse_pcs.o
obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
stmmac-pci-objs:= stmmac_pci.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 3f9588e..88fba4e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -27,6 +27,8 @@
#include "stmmac.h"
#include "stmmac_platform.h"
+#include "tse_pcs.h"
+
#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
@@ -47,48 +49,60 @@ struct socfpga_dwmac {
struct regmap *sys_mgr_base_addr;
struct reset_control *stmmac_rst;
void __iomem *splitter_base;
+ struct tse_pcs pcs;
};
static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
{
struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
void __iomem *splitter_base = dwmac->splitter_base;
+ void __iomem *tse_pcs_base = dwmac->pcs.tse_pcs_base;
+ void __iomem *sgmii_adapter_base = dwmac->pcs.sgmii_adapter_base;
+ struct device *dev = dwmac->dev;
+ struct net_device *ndev = dev_get_drvdata(dev);
+ struct phy_device *phy_dev = ndev->phydev;
u32 val;
- if (!splitter_base)
- return;
-
- val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
- val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
-
- switch (speed) {
- case 1000:
- val |= EMAC_SPLITTER_CTRL_SPEED_1000;
- break;
- case 100:
- val |= EMAC_SPLITTER_CTRL_SPEED_100;
- break;
- case 10:
- val |= EMAC_SPLITTER_CTRL_SPEED_10;
- break;
- default:
- return;
+ if (splitter_base) {
+ val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
+ val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
+
+ switch (speed) {
+ case 1000:
+ val |= EMAC_SPLITTER_CTRL_SPEED_1000;
+ break;
+ case 100:
+ val |= EMAC_SPLITTER_CTRL_SPEED_100;
+ break;
+ case 10:
+ val |= EMAC_SPLITTER_CTRL_SPEED_10;
+ break;
+ default:
+ return;
+ }
+ writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
}
- writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
+ if ((tse_pcs_base) && (sgmii_adapter_base))
+ tse_pcs_fix_mac_speed(&dwmac->pcs, phy_dev, speed);
}
-static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *dev)
+static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac,
+ struct device *dev)
{
struct device_node *np = dev->of_node;
struct regmap *sys_mgr_base_addr;
u32 reg_offset, reg_shift;
- int ret;
- struct device_node *np_splitter;
+ int ret, index;
+ struct device_node *np_splitter = NULL;
+ struct device_node *np_sgmii_adapter = NULL;
+
struct resource res_splitter;
+ struct resource res_tse_pcs;
+ struct resource res_sgmii_adapter;
dwmac->stmmac_rst = devm_reset_control_get(dev,
- STMMAC_RESOURCE_NAME);
+ STMMAC_RESOURCE_NAME);
if (IS_ERR(dwmac->stmmac_rst)) {
dev_info(dev, "Could not get reset control!\n");
if (PTR_ERR(dwmac->stmmac_rst) == -EPROBE_DEFER)
@@ -99,6 +113,7 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
dwmac->interface = of_get_phy_mode(np);
sys_mgr_base_addr = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
+
if (IS_ERR(sys_mgr_base_addr)) {
dev_info(dev, "No sysmgr-syscon node found\n");
return PTR_ERR(sys_mgr_base_addr);
@@ -130,6 +145,74 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
}
}
+ np_sgmii_adapter = of_parse_phandle(np,
+ "altr,gmii_to_sgmii_converter", 0);
+ if (np_sgmii_adapter) {
+ index = of_property_match_string(np_sgmii_adapter, "reg-names",
+ "hps_emac_interface_splitter_avalon_slave");
+
+ if (index >= 0) {
+ if (of_address_to_resource(np_sgmii_adapter, index,
+ &res_splitter)) {
+ dev_err(dev, "%s: ERROR: missing emac splitter address\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ dwmac->splitter_base = devm_ioremap_resource(
+ dev, &res_splitter);
+
+ if (IS_ERR(dwmac->splitter_base)) {
+ dev_err(dev, "%s: ERROR: failed mapping emac splitter\n",
+ __func__);
+ return PTR_ERR(dwmac->splitter_base);
+ }
+ }
+
+ index = of_property_match_string(np_sgmii_adapter, "reg-names",
+ "gmii_to_sgmii_adapter_avalon_slave");
+
+ if (index >= 0) {
+ if (of_address_to_resource(np_sgmii_adapter, index,
+ &res_sgmii_adapter)) {
+ dev_err(dev,
+ "%s: ERROR: failed mapping adapter\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ dwmac->pcs.sgmii_adapter_base =
+ devm_ioremap_resource(dev, &res_sgmii_adapter);
+
+ if (IS_ERR(dwmac->pcs.sgmii_adapter_base)) {
+ dev_err(dev, "%s: failed to mapping adapter\n",
+ __func__);
+ return PTR_ERR(dwmac->pcs.sgmii_adapter_base);
+ }
+ }
+
+ index = of_property_match_string(np_sgmii_adapter, "reg-names",
+ "eth_tse_control_port");
+
+ if (index >= 0) {
+ if (of_address_to_resource(np_sgmii_adapter, index,
+ &res_tse_pcs)) {
+ dev_err(dev,
+ "%s: ERROR: failed mapping tse control port\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ dwmac->pcs.tse_pcs_base = devm_ioremap_resource(
+ dev, &res_tse_pcs);
+
+ if (IS_ERR(dwmac->pcs.tse_pcs_base)) {
+ dev_err(dev, "%s: ERROR: failed mapping tse control port\n",
+ __func__);
+ return PTR_ERR(dwmac->pcs.sgmii_adapter_base);
+ }
+ }
+ }
dwmac->reg_offset = reg_offset;
dwmac->reg_shift = reg_shift;
dwmac->sys_mgr_base_addr = sys_mgr_base_addr;
@@ -153,6 +236,7 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
break;
case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_GMII:
+ case PHY_INTERFACE_MODE_SGMII:
val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
break;
default:
@@ -172,6 +256,10 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
ctrl |= val << reg_shift;
regmap_write(sys_mgr_base_addr, reg_offset, ctrl);
+
+ if (phymode == PHY_INTERFACE_MODE_SGMII)
+ tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs);
+
return 0;
}
@@ -191,6 +279,12 @@ static void *socfpga_dwmac_probe(struct platform_device *pdev)
return ERR_PTR(ret);
}
+ ret = socfpga_dwmac_setup(dwmac);
+ if (ret) {
+ dev_err(dev, "couldn't setup SoC glue (%d)\n", ret);
+ return ERR_PTR(ret);
+ }
+
return dwmac;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
new file mode 100644
index 0000000..1989bd1
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
@@ -0,0 +1,261 @@
+/* Copyright Altera Corporation (C) 2016. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Tien Hock Loh <thloh@altera.com>
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/stmmac.h>
+
+#include "stmmac.h"
+#include "stmmac_platform.h"
+#include "tse_pcs.h"
+
+#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
+#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
+#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
+#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH 2
+#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK 0x00000003
+
+#define EMAC_SPLITTER_CTRL_REG 0x0
+#define EMAC_SPLITTER_CTRL_SPEED_MASK 0x3
+#define EMAC_SPLITTER_CTRL_SPEED_10 0x2
+#define EMAC_SPLITTER_CTRL_SPEED_100 0x3
+#define EMAC_SPLITTER_CTRL_SPEED_1000 0x0
+
+#define TSE_PCS_CONTROL_AN_EN_MASK 0x1000
+#define TSE_PCS_CONTROL_REG 0x00
+#define TSE_PCS_CONTROL_RESTART_AN_MASK 0x0200
+#define TSE_PCS_IF_MODE_REG 0x28
+#define TSE_PCS_LINK_TIMER_0_REG 0x24
+#define TSE_PCS_LINK_TIMER_1_REG 0x26
+#define TSE_PCS_SIZE 0x40
+#define TSE_PCS_STATUS_AN_COMPLETED_MASK 0x0020
+#define TSE_PCS_STATUS_LINK_MASK 0x0004
+#define TSE_PCS_STATUS_REG 0x02
+#define TSE_PCS_SGMII_SPEED_1000 0x8
+#define TSE_PCS_SGMII_SPEED_100 0x4
+#define TSE_PCS_SGMII_SPEED_10 0x0
+#define TSE_PCS_SW_RST_MASK 0x8000
+#define TSE_PCS_PARTNER_ABILITY_REG 0x0A
+#define TSE_PCS_PARTNER_DUPLEX_FULL 0x1000
+#define TSE_PCS_PARTNER_DUPLEX_HALF 0x0000
+#define TSE_PCS_PARTNER_DUPLEX_MASK 0x1000
+#define TSE_PCS_PARTNER_SPEED_MASK 0x0c00
+#define TSE_PCS_PARTNER_SPEED_1000 0x0800
+#define TSE_PCS_PARTNER_SPEED_100 0x0400
+#define TSE_PCS_PARTNER_SPEED_10 0x0000
+#define TSE_PCS_PARTNER_SPEED_1000 0x0800
+#define TSE_PCS_PARTNER_SPEED_100 0x0400
+#define TSE_PCS_PARTNER_SPEED_10 0x0000
+#define TSE_PCS_SGMII_SPEED_MASK 0x000C
+#define TSE_PCS_SGMII_LINK_TIMER_0 0x0D40
+#define TSE_PCS_SGMII_LINK_TIMER_1 0x0003
+#define TSE_PCS_SW_RESET_TIMEOUT 100
+#define TSE_PCS_USE_SGMII_AN_MASK 0x0002
+
+#define SGMII_ADAPTER_CTRL_REG 0x00
+#define SGMII_ADAPTER_DISABLE 0x0001
+#define SGMII_ADAPTER_ENABLE 0x0000
+#define LINK_TIMER 20
+#define AUTONEGO_TIMER 20
+
+static void config_tx_buffer(u16 data, void __iomem *base)
+{
+ writew(data, base + SGMII_ADAPTER_CTRL_REG);
+}
+
+static void tse_pcs_reset(void __iomem *base, struct tse_pcs *pcs)
+{
+ int counter = 0;
+ u16 val;
+
+ val = readw(base + TSE_PCS_CONTROL_REG);
+ val |= TSE_PCS_SW_RST_MASK;
+ writew(val, base + TSE_PCS_CONTROL_REG);
+
+ while (counter < TSE_PCS_SW_RESET_TIMEOUT) {
+ val = readw(base + TSE_PCS_CONTROL_REG);
+ val &= TSE_PCS_SW_RST_MASK;
+ if (val == 0)
+ break;
+ counter++;
+ udelay(1);
+ }
+ if (counter >= TSE_PCS_SW_RESET_TIMEOUT)
+ dev_err(pcs->dev, "PCS could not get out of sw reset\n");
+}
+
+void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs)
+{
+ writew(0x0001, base + TSE_PCS_IF_MODE_REG);
+
+ writew(TSE_PCS_SGMII_LINK_TIMER_0, base + TSE_PCS_LINK_TIMER_0_REG);
+ writew(TSE_PCS_SGMII_LINK_TIMER_1, base + TSE_PCS_LINK_TIMER_1_REG);
+
+ tse_pcs_reset(base, pcs);
+ config_tx_buffer(SGMII_ADAPTER_ENABLE,
+ pcs->sgmii_adapter_base);
+}
+
+static void pcs_link_timer_callback(unsigned long data)
+{
+ u16 val = 0;
+
+ struct tse_pcs *pcs = (struct tse_pcs *)data;
+ void __iomem *tse_pcs_base = pcs->tse_pcs_base;
+ void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
+
+ val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
+ val &= TSE_PCS_STATUS_LINK_MASK;
+
+ if (val != 0) {
+ dev_dbg(pcs->dev, "Adapter: Link is established\n");
+ config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base);
+ } else {
+ mod_timer(&pcs->link_timer, jiffies +
+ msecs_to_jiffies(LINK_TIMER));
+ }
+}
+
+static void auto_nego_timer_callback(unsigned long data)
+{
+ u16 val = 0;
+ u16 speed = 0;
+ u16 duplex = 0;
+
+ struct tse_pcs *pcs = (struct tse_pcs *)data;
+ void __iomem *tse_pcs_base = pcs->tse_pcs_base;
+ void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
+
+ val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
+ val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
+
+ if (val != 0) {
+ dev_dbg(pcs->dev, "Adapter: Auto Negotiation is completed\n");
+ val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG);
+ speed = val & TSE_PCS_PARTNER_SPEED_MASK;
+ duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK;
+
+ if (speed == TSE_PCS_PARTNER_SPEED_10 &&
+ duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
+ dev_dbg(pcs->dev,
+ "Adapter: Link Partner is Up - 10/Full\n");
+ else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
+ duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
+ dev_dbg(pcs->dev,
+ "Adapter: Link Partner is Up - 100/Full\n");
+ else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
+ duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
+ dev_dbg(pcs->dev,
+ "Adapter: Link Partner is Up - 1000/Full\n");
+ else if (speed == TSE_PCS_PARTNER_SPEED_10 &&
+ duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
+ dev_err(pcs->dev,
+ "Adapter does not support Half Duplex\n");
+ else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
+ duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
+ dev_err(pcs->dev,
+ "Adapter does not support Half Duplex\n");
+ else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
+ duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
+ dev_err(pcs->dev,
+ "Adapter does not support Half Duplex\n");
+ else
+ dev_err(pcs->dev,
+ "Adapter: Invalid Partner Speed and Duplex\n");
+
+ config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base);
+ } else {
+ val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
+ val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
+
+ tse_pcs_reset(tse_pcs_base, pcs);
+ mod_timer(&pcs->an_timer, jiffies +
+ msecs_to_jiffies(AUTONEGO_TIMER));
+ }
+}
+
+void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
+ unsigned int speed)
+{
+ void __iomem *tse_pcs_base = pcs->tse_pcs_base;
+ void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
+ u32 val;
+
+ config_tx_buffer(SGMII_ADAPTER_DISABLE, sgmii_adapter_base);
+
+ if (phy_dev->autoneg == AUTONEG_ENABLE) {
+ val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
+ val |= TSE_PCS_CONTROL_AN_EN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
+
+ val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
+ val |= TSE_PCS_USE_SGMII_AN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
+
+ val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
+ val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
+
+ tse_pcs_reset(tse_pcs_base, pcs);
+
+ setup_timer(&pcs->an_timer,
+ auto_nego_timer_callback,
+ (unsigned long)pcs);
+ mod_timer(&pcs->an_timer, jiffies +
+ msecs_to_jiffies(AUTONEGO_TIMER));
+ } else if (phy_dev->autoneg == AUTONEG_DISABLE) {
+ val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
+ val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
+
+ val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
+ val &= ~TSE_PCS_USE_SGMII_AN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
+
+ val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
+ val &= ~TSE_PCS_SGMII_SPEED_MASK;
+
+ switch (speed) {
+ case 1000:
+ val |= TSE_PCS_SGMII_SPEED_1000;
+ break;
+ case 100:
+ val |= TSE_PCS_SGMII_SPEED_100;
+ break;
+ case 10:
+ val |= TSE_PCS_SGMII_SPEED_10;
+ break;
+ default:
+ return;
+ }
+ writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
+
+ tse_pcs_reset(tse_pcs_base, pcs);
+
+ setup_timer(&pcs->link_timer,
+ pcs_link_timer_callback,
+ (unsigned long)pcs);
+ mod_timer(&pcs->link_timer, jiffies +
+ msecs_to_jiffies(LINK_TIMER));
+ }
+}
diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
new file mode 100644
index 0000000..e3d4511
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
@@ -0,0 +1,36 @@
+/* Copyright Altera Corporation (C) 2016. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Tien Hock Loh <thloh@altera.com>
+ */
+
+#ifndef __TSE_PCS_H__
+#define __TSE_PCS_H__
+
+#include <linux/phy.h>
+#include <linux/timer.h>
+
+struct tse_pcs {
+ struct device *dev;
+ void __iomem *tse_pcs_base;
+ void __iomem *sgmii_adapter_base;
+ struct timer_list an_timer;
+ struct timer_list link_timer;
+};
+
+void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs);
+void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
+ unsigned int speed);
+
+#endif /* __TSE_PCS_H__ */
--
1.7.11.GIT
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
2016-06-07 3:01 [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga thloh
@ 2016-06-07 8:30 ` Giuseppe CAVALLARO
2016-06-09 5:48 ` Tien Hock Loh
2016-06-08 19:52 ` Rob Herring
1 sibling, 1 reply; 7+ messages in thread
From: Giuseppe CAVALLARO @ 2016-06-07 8:30 UTC (permalink / raw)
To: thloh, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
devicetree, linux-kernel, netdev
Hello
On 6/7/2016 5:01 AM, thloh@altera.com wrote:
> From: Tien Hock Loh <thloh@altera.com>
>
> This adds support for TSE PCS that uses SGMII adapter when the phy-mode of
> the dwmac is set to sgmii
>
> Signed-off-by: Tien Hock Loh <thloh@altera.com>
>
> ---
> v2:
> - Refactored the TSE PCS out from the dwmac-socfpga.c file
> - Added binding documentation for TSE PCS sgmii adapter
> v3:
> - Added missing license header for new source files
> - Updated tse_pcs.h include headers
> - Standardize if statements
> ---
> .../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
> .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
> drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
> drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
> 5 files changed, 419 insertions(+), 24 deletions(-)
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
I just wonder if it could make sense to rename the
tse_pcs.[hc] files or creating a sub-directory for altera devel.
It seems that tse_pcs.[hc] are common files but this support
is for Altera.
Anyway, I let you decide and I also ask you to update the stmmac.txt
file.
> diff --git a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> index 3a9d679..2bc39f1 100644
> --- a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> @@ -15,6 +15,8 @@ Required properties:
> Optional properties:
> altr,emac-splitter: Should be the phandle to the emac splitter soft IP node if
> DWMAC controller is connected emac splitter.
> +phy-mode: The phy mode the ethernet operates in
> +altr,sgmii_to_sgmii_converter: phandle to the TSE SGMII converter
>
> Example:
>
> @@ -28,4 +30,6 @@ gmac0: ethernet@ff700000 {
> mac-address = [00 00 00 00 00 00];/* Filled in by U-Boot */
> clocks = <&emac_0_clk>;
> clock-names = "stmmaceth";
> + phy-mode = "sgmii";
> + altr,gmii_to_sgmii_converter = <&sgmii_1_gmii_to_sgmii_converter_0>;
> };
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index 73c2715..29c1dee 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \
>
> obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
> stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o \
> - dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
> + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o tse_pcs.o
>
> obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
> stmmac-pci-objs:= stmmac_pci.o
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 3f9588e..88fba4e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -27,6 +27,8 @@
> #include "stmmac.h"
> #include "stmmac_platform.h"
>
> +#include "tse_pcs.h"
> +
> #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
> #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
> #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
> @@ -47,48 +49,60 @@ struct socfpga_dwmac {
> struct regmap *sys_mgr_base_addr;
> struct reset_control *stmmac_rst;
> void __iomem *splitter_base;
> + struct tse_pcs pcs;
> };
>
> static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
> {
> struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
> void __iomem *splitter_base = dwmac->splitter_base;
> + void __iomem *tse_pcs_base = dwmac->pcs.tse_pcs_base;
> + void __iomem *sgmii_adapter_base = dwmac->pcs.sgmii_adapter_base;
> + struct device *dev = dwmac->dev;
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct phy_device *phy_dev = ndev->phydev;
> u32 val;
>
> - if (!splitter_base)
> - return;
> -
> - val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
> - val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
> -
> - switch (speed) {
> - case 1000:
> - val |= EMAC_SPLITTER_CTRL_SPEED_1000;
> - break;
> - case 100:
> - val |= EMAC_SPLITTER_CTRL_SPEED_100;
> - break;
> - case 10:
> - val |= EMAC_SPLITTER_CTRL_SPEED_10;
> - break;
> - default:
> - return;
> + if (splitter_base) {
> + val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
> + val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
> +
> + switch (speed) {
> + case 1000:
> + val |= EMAC_SPLITTER_CTRL_SPEED_1000;
> + break;
> + case 100:
> + val |= EMAC_SPLITTER_CTRL_SPEED_100;
> + break;
> + case 10:
> + val |= EMAC_SPLITTER_CTRL_SPEED_10;
> + break;
> + default:
> + return;
> + }
> + writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
> }
>
> - writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
> + if ((tse_pcs_base) && (sgmii_adapter_base))
> + tse_pcs_fix_mac_speed(&dwmac->pcs, phy_dev, speed);
> }
>
> -static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *dev)
> +static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac,
> + struct device *dev)
> {
> struct device_node *np = dev->of_node;
> struct regmap *sys_mgr_base_addr;
> u32 reg_offset, reg_shift;
> - int ret;
> - struct device_node *np_splitter;
> + int ret, index;
> + struct device_node *np_splitter = NULL;
> + struct device_node *np_sgmii_adapter = NULL;
> +
> struct resource res_splitter;
> + struct resource res_tse_pcs;
> + struct resource res_sgmii_adapter;
>
> dwmac->stmmac_rst = devm_reset_control_get(dev,
> - STMMAC_RESOURCE_NAME);
> + STMMAC_RESOURCE_NAME);
> if (IS_ERR(dwmac->stmmac_rst)) {
> dev_info(dev, "Could not get reset control!\n");
> if (PTR_ERR(dwmac->stmmac_rst) == -EPROBE_DEFER)
> @@ -99,6 +113,7 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
> dwmac->interface = of_get_phy_mode(np);
>
> sys_mgr_base_addr = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
> +
> if (IS_ERR(sys_mgr_base_addr)) {
> dev_info(dev, "No sysmgr-syscon node found\n");
> return PTR_ERR(sys_mgr_base_addr);
> @@ -130,6 +145,74 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
> }
> }
>
> + np_sgmii_adapter = of_parse_phandle(np,
> + "altr,gmii_to_sgmii_converter", 0);
> + if (np_sgmii_adapter) {
> + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> + "hps_emac_interface_splitter_avalon_slave");
> +
> + if (index >= 0) {
> + if (of_address_to_resource(np_sgmii_adapter, index,
> + &res_splitter)) {
> + dev_err(dev, "%s: ERROR: missing emac splitter address\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + dwmac->splitter_base = devm_ioremap_resource(
> + dev, &res_splitter);
> +
> + if (IS_ERR(dwmac->splitter_base)) {
> + dev_err(dev, "%s: ERROR: failed mapping emac splitter\n",
> + __func__);
> + return PTR_ERR(dwmac->splitter_base);
> + }
> + }
> +
> + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> + "gmii_to_sgmii_adapter_avalon_slave");
> +
> + if (index >= 0) {
> + if (of_address_to_resource(np_sgmii_adapter, index,
> + &res_sgmii_adapter)) {
> + dev_err(dev,
> + "%s: ERROR: failed mapping adapter\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + dwmac->pcs.sgmii_adapter_base =
> + devm_ioremap_resource(dev, &res_sgmii_adapter);
> +
> + if (IS_ERR(dwmac->pcs.sgmii_adapter_base)) {
> + dev_err(dev, "%s: failed to mapping adapter\n",
> + __func__);
> + return PTR_ERR(dwmac->pcs.sgmii_adapter_base);
> + }
> + }
> +
> + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> + "eth_tse_control_port");
reg-names looks to be specific and mandatory, IMO they should be
documented in the binding.
> +
> + if (index >= 0) {
> + if (of_address_to_resource(np_sgmii_adapter, index,
> + &res_tse_pcs)) {
> + dev_err(dev,
> + "%s: ERROR: failed mapping tse control port\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + dwmac->pcs.tse_pcs_base = devm_ioremap_resource(
> + dev, &res_tse_pcs);
can you check with indentation (scripts/Lindent)?
> +
> + if (IS_ERR(dwmac->pcs.tse_pcs_base)) {
> + dev_err(dev, "%s: ERROR: failed mapping tse control port\n",
> + __func__);
> + return PTR_ERR(dwmac->pcs.sgmii_adapter_base);
> + }
> + }
> + }
> dwmac->reg_offset = reg_offset;
> dwmac->reg_shift = reg_shift;
> dwmac->sys_mgr_base_addr = sys_mgr_base_addr;
> @@ -153,6 +236,7 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
> break;
> case PHY_INTERFACE_MODE_MII:
> case PHY_INTERFACE_MODE_GMII:
> + case PHY_INTERFACE_MODE_SGMII:
> val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
> break;
> default:
> @@ -172,6 +256,10 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
> ctrl |= val << reg_shift;
>
> regmap_write(sys_mgr_base_addr, reg_offset, ctrl);
> +
> + if (phymode == PHY_INTERFACE_MODE_SGMII)
> + tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs);
> +
> return 0;
> }
>
> @@ -191,6 +279,12 @@ static void *socfpga_dwmac_probe(struct platform_device *pdev)
> return ERR_PTR(ret);
> }
>
> + ret = socfpga_dwmac_setup(dwmac);
> + if (ret) {
> + dev_err(dev, "couldn't setup SoC glue (%d)\n", ret);
> + return ERR_PTR(ret);
> + }
> +
> return dwmac;
> }
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> new file mode 100644
> index 0000000..1989bd1
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> @@ -0,0 +1,261 @@
> +/* Copyright Altera Corporation (C) 2016. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Author: Tien Hock Loh <thloh@altera.com>
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_net.h>
> +#include <linux/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/stmmac.h>
> +
> +#include "stmmac.h"
> +#include "stmmac_platform.h"
> +#include "tse_pcs.h"
> +
> +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
> +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
> +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
> +#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH 2
> +#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK 0x00000003
> +
> +#define EMAC_SPLITTER_CTRL_REG 0x0
> +#define EMAC_SPLITTER_CTRL_SPEED_MASK 0x3
> +#define EMAC_SPLITTER_CTRL_SPEED_10 0x2
> +#define EMAC_SPLITTER_CTRL_SPEED_100 0x3
> +#define EMAC_SPLITTER_CTRL_SPEED_1000 0x0
pls use BIT and GENMASK.
...
> +static void config_tx_buffer(u16 data, void __iomem *base)
> +{
> + writew(data, base + SGMII_ADAPTER_CTRL_REG);
> +}
hmm, is this function really necessary? I think you can use
directly writew or maybe writew_relaxed.
> +
> +static void tse_pcs_reset(void __iomem *base, struct tse_pcs *pcs)
> +{
> + int counter = 0;
> + u16 val;
> +
> + val = readw(base + TSE_PCS_CONTROL_REG);
> + val |= TSE_PCS_SW_RST_MASK;
> + writew(val, base + TSE_PCS_CONTROL_REG);
> +
> + while (counter < TSE_PCS_SW_RESET_TIMEOUT) {
> + val = readw(base + TSE_PCS_CONTROL_REG);
> + val &= TSE_PCS_SW_RST_MASK;
> + if (val == 0)
> + break;
> + counter++;
> + udelay(1);
> + }
> + if (counter >= TSE_PCS_SW_RESET_TIMEOUT)
> + dev_err(pcs->dev, "PCS could not get out of sw reset\n");
> +}
> +
maybe this should return an error to stop the init if reset is NOK.
> +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs)
> +{
> + writew(0x0001, base + TSE_PCS_IF_MODE_REG);
> +
> + writew(TSE_PCS_SGMII_LINK_TIMER_0, base + TSE_PCS_LINK_TIMER_0_REG);
> + writew(TSE_PCS_SGMII_LINK_TIMER_1, base + TSE_PCS_LINK_TIMER_1_REG);
> +
> + tse_pcs_reset(base, pcs);
> + config_tx_buffer(SGMII_ADAPTER_ENABLE,
> + pcs->sgmii_adapter_base);
> +}
> +
> +static void pcs_link_timer_callback(unsigned long data)
> +{
> + u16 val = 0;
> +
> + struct tse_pcs *pcs = (struct tse_pcs *)data;
> + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> +
> + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> + val &= TSE_PCS_STATUS_LINK_MASK;
> +
> + if (val != 0) {
> + dev_dbg(pcs->dev, "Adapter: Link is established\n");
> + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base);
> + } else {
> + mod_timer(&pcs->link_timer, jiffies +
> + msecs_to_jiffies(LINK_TIMER));
> + }
> +}
> +
> +static void auto_nego_timer_callback(unsigned long data)
> +{
> + u16 val = 0;
> + u16 speed = 0;
> + u16 duplex = 0;
> +
> + struct tse_pcs *pcs = (struct tse_pcs *)data;
> + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> +
> + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> + val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
> +
> + if (val != 0) {
> + dev_dbg(pcs->dev, "Adapter: Auto Negotiation is completed\n");
> + val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG);
> + speed = val & TSE_PCS_PARTNER_SPEED_MASK;
> + duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK;
> +
> + if (speed == TSE_PCS_PARTNER_SPEED_10 &&
> + duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> + dev_dbg(pcs->dev,
> + "Adapter: Link Partner is Up - 10/Full\n");
> + else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
> + duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> + dev_dbg(pcs->dev,
> + "Adapter: Link Partner is Up - 100/Full\n");
> + else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
> + duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> + dev_dbg(pcs->dev,
> + "Adapter: Link Partner is Up - 1000/Full\n");
> + else if (speed == TSE_PCS_PARTNER_SPEED_10 &&
> + duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> + dev_err(pcs->dev,
> + "Adapter does not support Half Duplex\n");
> + else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
> + duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> + dev_err(pcs->dev,
> + "Adapter does not support Half Duplex\n");
> + else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
> + duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> + dev_err(pcs->dev,
> + "Adapter does not support Half Duplex\n");
> + else
> + dev_err(pcs->dev,
> + "Adapter: Invalid Partner Speed and Duplex\n");
ANE is completed but speed or duplex is NOK. Any failure to signalling?
I see that you then enable the adpter in any case.
Maybe we could try to restart ANE again or force it (reducing the speed)
I wonder what happens if, for some reason, there is some hw problem. In
that case the timer is always running signalling an invalid Parter
speed. Anyway, this is jus a question... I expect that this error will
always point us to a problem to debug further (if occurs).
> +
> + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base);
> + } else {
> + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> + val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
> + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> +
> + tse_pcs_reset(tse_pcs_base, pcs);
> + mod_timer(&pcs->an_timer, jiffies +
> + msecs_to_jiffies(AUTONEGO_TIMER));
> + }
> +}
> +
> +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
> + unsigned int speed)
> +{
> + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> + u32 val;
> +
> + config_tx_buffer(SGMII_ADAPTER_DISABLE, sgmii_adapter_base);
> +
> + if (phy_dev->autoneg == AUTONEG_ENABLE) {
> + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> + val |= TSE_PCS_CONTROL_AN_EN_MASK;
> + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> +
> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> + val |= TSE_PCS_USE_SGMII_AN_MASK;
> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> +
> + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> + val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
> + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> +
> + tse_pcs_reset(tse_pcs_base, pcs);
why do you need to reset here?
> +
> + setup_timer(&pcs->an_timer,
> + auto_nego_timer_callback,
> + (unsigned long)pcs);
> + mod_timer(&pcs->an_timer, jiffies +
> + msecs_to_jiffies(AUTONEGO_TIMER));
> + } else if (phy_dev->autoneg == AUTONEG_DISABLE) {
> + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> + val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
> + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> +
> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> + val &= ~TSE_PCS_USE_SGMII_AN_MASK;
> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> +
> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> + val &= ~TSE_PCS_SGMII_SPEED_MASK;
> +
> + switch (speed) {
> + case 1000:
> + val |= TSE_PCS_SGMII_SPEED_1000;
> + break;
> + case 100:
> + val |= TSE_PCS_SGMII_SPEED_100;
> + break;
> + case 10:
> + val |= TSE_PCS_SGMII_SPEED_10;
> + break;
> + default:
> + return;
> + }
> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> +
> + tse_pcs_reset(tse_pcs_base, pcs);
> +
> + setup_timer(&pcs->link_timer,
> + pcs_link_timer_callback,
> + (unsigned long)pcs);
> + mod_timer(&pcs->link_timer, jiffies +
> + msecs_to_jiffies(LINK_TIMER));
I wonder if we can have just one timer to manage ANE and LINK.
> + }
> +}
> diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
> new file mode 100644
> index 0000000..e3d4511
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
> @@ -0,0 +1,36 @@
> +/* Copyright Altera Corporation (C) 2016. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Author: Tien Hock Loh <thloh@altera.com>
> + */
> +
> +#ifndef __TSE_PCS_H__
> +#define __TSE_PCS_H__
> +
> +#include <linux/phy.h>
> +#include <linux/timer.h>
> +
> +struct tse_pcs {
> + struct device *dev;
> + void __iomem *tse_pcs_base;
> + void __iomem *sgmii_adapter_base;
> + struct timer_list an_timer;
> + struct timer_list link_timer;
> +};
> +
> +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs);
> +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
> + unsigned int speed);
> +
> +#endif /* __TSE_PCS_H__ */
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
2016-06-07 3:01 [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga thloh
2016-06-07 8:30 ` Giuseppe CAVALLARO
@ 2016-06-08 19:52 ` Rob Herring
1 sibling, 0 replies; 7+ messages in thread
From: Rob Herring @ 2016-06-08 19:52 UTC (permalink / raw)
To: thloh
Cc: pawel.moll, mark.rutland, ijc+devicetree, galak, peppe.cavallaro,
devicetree, linux-kernel, netdev
On Mon, Jun 06, 2016 at 08:01:56PM -0700, thloh@altera.com wrote:
> From: Tien Hock Loh <thloh@altera.com>
>
> This adds support for TSE PCS that uses SGMII adapter when the phy-mode of
> the dwmac is set to sgmii
>
> Signed-off-by: Tien Hock Loh <thloh@altera.com>
>
> ---
> v2:
> - Refactored the TSE PCS out from the dwmac-socfpga.c file
> - Added binding documentation for TSE PCS sgmii adapter
> v3:
> - Added missing license header for new source files
> - Updated tse_pcs.h include headers
> - Standardize if statements
> ---
> .../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
> .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
> drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
> drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
> 5 files changed, 419 insertions(+), 24 deletions(-)
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
>
> diff --git a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> index 3a9d679..2bc39f1 100644
> --- a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> @@ -15,6 +15,8 @@ Required properties:
> Optional properties:
> altr,emac-splitter: Should be the phandle to the emac splitter soft IP node if
> DWMAC controller is connected emac splitter.
> +phy-mode: The phy mode the ethernet operates in
> +altr,sgmii_to_sgmii_converter: phandle to the TSE SGMII converter
Use '-' rather than '_'.
With that, for the binding:
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
2016-06-07 8:30 ` Giuseppe CAVALLARO
@ 2016-06-09 5:48 ` Tien Hock Loh
2016-06-09 6:20 ` Giuseppe CAVALLARO
0 siblings, 1 reply; 7+ messages in thread
From: Tien Hock Loh @ 2016-06-09 5:48 UTC (permalink / raw)
To: Giuseppe CAVALLARO
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
devicetree, linux-kernel, netdev, thloh85, cnphoon, thloh
Hi Peppe,
On Tue, 2016-06-07 at 10:30 +0200, Giuseppe CAVALLARO wrote:
> Hello
>
> On 6/7/2016 5:01 AM, thloh@altera.com wrote:
> > From: Tien Hock Loh <thloh@altera.com>
> >
> > This adds support for TSE PCS that uses SGMII adapter when the phy-mode of
> > the dwmac is set to sgmii
> >
> > Signed-off-by: Tien Hock Loh <thloh@altera.com>
> >
> > ---
> > v2:
> > - Refactored the TSE PCS out from the dwmac-socfpga.c file
> > - Added binding documentation for TSE PCS sgmii adapter
> > v3:
> > - Added missing license header for new source files
> > - Updated tse_pcs.h include headers
> > - Standardize if statements
> > ---
> > .../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
> > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
> > .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
> > drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
> > drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
> > 5 files changed, 419 insertions(+), 24 deletions(-)
> > create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> > create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
>
> I just wonder if it could make sense to rename the
> tse_pcs.[hc] files or creating a sub-directory for altera devel.
> It seems that tse_pcs.[hc] are common files but this support
> is for Altera.
> Anyway, I let you decide and I also ask you to update the stmmac.txt
> file.
Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename
them, would altr_tse_pcs.[hc] be good? I don't think creating a
sub-directory with only 2 files is necessary though.
I see that stmmac.txt is generic, and other vendor's PCS support
documents their specific uses in their own *-dwmac.txt (eg.
rockchip-dwmac.txt). Is this not the case?
>
> > diff --git a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> > index 3a9d679..2bc39f1 100644
> > --- a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> > +++ b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt
> > @@ -15,6 +15,8 @@ Required properties:
> > Optional properties:
> > altr,emac-splitter: Should be the phandle to the emac splitter soft IP node if
> > DWMAC controller is connected emac splitter.
> > +phy-mode: The phy mode the ethernet operates in
> > +altr,sgmii_to_sgmii_converter: phandle to the TSE SGMII converter
> >
> > Example:
> >
> > @@ -28,4 +30,6 @@ gmac0: ethernet@ff700000 {
> > mac-address = [00 00 00 00 00 00];/* Filled in by U-Boot */
> > clocks = <&emac_0_clk>;
> > clock-names = "stmmaceth";
> > + phy-mode = "sgmii";
> > + altr,gmii_to_sgmii_converter = <&sgmii_1_gmii_to_sgmii_converter_0>;
> > };
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > index 73c2715..29c1dee 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \
> >
> > obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
> > stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o \
> > - dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
> > + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o tse_pcs.o
> >
> > obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
> > stmmac-pci-objs:= stmmac_pci.o
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > index 3f9588e..88fba4e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > @@ -27,6 +27,8 @@
> > #include "stmmac.h"
> > #include "stmmac_platform.h"
> >
> > +#include "tse_pcs.h"
> > +
> > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
> > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
> > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
> > @@ -47,48 +49,60 @@ struct socfpga_dwmac {
> > struct regmap *sys_mgr_base_addr;
> > struct reset_control *stmmac_rst;
> > void __iomem *splitter_base;
> > + struct tse_pcs pcs;
> > };
> >
> > static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
> > {
> > struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
> > void __iomem *splitter_base = dwmac->splitter_base;
> > + void __iomem *tse_pcs_base = dwmac->pcs.tse_pcs_base;
> > + void __iomem *sgmii_adapter_base = dwmac->pcs.sgmii_adapter_base;
> > + struct device *dev = dwmac->dev;
> > + struct net_device *ndev = dev_get_drvdata(dev);
> > + struct phy_device *phy_dev = ndev->phydev;
> > u32 val;
> >
> > - if (!splitter_base)
> > - return;
> > -
> > - val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
> > - val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
> > -
> > - switch (speed) {
> > - case 1000:
> > - val |= EMAC_SPLITTER_CTRL_SPEED_1000;
> > - break;
> > - case 100:
> > - val |= EMAC_SPLITTER_CTRL_SPEED_100;
> > - break;
> > - case 10:
> > - val |= EMAC_SPLITTER_CTRL_SPEED_10;
> > - break;
> > - default:
> > - return;
> > + if (splitter_base) {
> > + val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
> > + val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
> > +
> > + switch (speed) {
> > + case 1000:
> > + val |= EMAC_SPLITTER_CTRL_SPEED_1000;
> > + break;
> > + case 100:
> > + val |= EMAC_SPLITTER_CTRL_SPEED_100;
> > + break;
> > + case 10:
> > + val |= EMAC_SPLITTER_CTRL_SPEED_10;
> > + break;
> > + default:
> > + return;
> > + }
> > + writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
> > }
> >
> > - writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
> > + if ((tse_pcs_base) && (sgmii_adapter_base))
> > + tse_pcs_fix_mac_speed(&dwmac->pcs, phy_dev, speed);
> > }
> >
> > -static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *dev)
> > +static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac,
> > + struct device *dev)
> > {
> > struct device_node *np = dev->of_node;
> > struct regmap *sys_mgr_base_addr;
> > u32 reg_offset, reg_shift;
> > - int ret;
> > - struct device_node *np_splitter;
> > + int ret, index;
> > + struct device_node *np_splitter = NULL;
> > + struct device_node *np_sgmii_adapter = NULL;
> > +
> > struct resource res_splitter;
> > + struct resource res_tse_pcs;
> > + struct resource res_sgmii_adapter;
> >
> > dwmac->stmmac_rst = devm_reset_control_get(dev,
> > - STMMAC_RESOURCE_NAME);
> > + STMMAC_RESOURCE_NAME);
> > if (IS_ERR(dwmac->stmmac_rst)) {
> > dev_info(dev, "Could not get reset control!\n");
> > if (PTR_ERR(dwmac->stmmac_rst) == -EPROBE_DEFER)
> > @@ -99,6 +113,7 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
> > dwmac->interface = of_get_phy_mode(np);
> >
> > sys_mgr_base_addr = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
> > +
> > if (IS_ERR(sys_mgr_base_addr)) {
> > dev_info(dev, "No sysmgr-syscon node found\n");
> > return PTR_ERR(sys_mgr_base_addr);
> > @@ -130,6 +145,74 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
> > }
> > }
> >
> > + np_sgmii_adapter = of_parse_phandle(np,
> > + "altr,gmii_to_sgmii_converter", 0);
> > + if (np_sgmii_adapter) {
> > + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> > + "hps_emac_interface_splitter_avalon_slave");
> > +
> > + if (index >= 0) {
> > + if (of_address_to_resource(np_sgmii_adapter, index,
> > + &res_splitter)) {
> > + dev_err(dev, "%s: ERROR: missing emac splitter address\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + dwmac->splitter_base = devm_ioremap_resource(
> > + dev, &res_splitter);
> > +
> > + if (IS_ERR(dwmac->splitter_base)) {
> > + dev_err(dev, "%s: ERROR: failed mapping emac splitter\n",
> > + __func__);
> > + return PTR_ERR(dwmac->splitter_base);
> > + }
> > + }
> > +
> > + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> > + "gmii_to_sgmii_adapter_avalon_slave");
> > +
> > + if (index >= 0) {
> > + if (of_address_to_resource(np_sgmii_adapter, index,
> > + &res_sgmii_adapter)) {
> > + dev_err(dev,
> > + "%s: ERROR: failed mapping adapter\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + dwmac->pcs.sgmii_adapter_base =
> > + devm_ioremap_resource(dev, &res_sgmii_adapter);
> > +
> > + if (IS_ERR(dwmac->pcs.sgmii_adapter_base)) {
> > + dev_err(dev, "%s: failed to mapping adapter\n",
> > + __func__);
> > + return PTR_ERR(dwmac->pcs.sgmii_adapter_base);
> > + }
> > + }
> > +
> > + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> > + "eth_tse_control_port");
>
> reg-names looks to be specific and mandatory, IMO they should be
> documented in the binding.
That's the binding for the adapter (the phandle to the sgmii adapter),
not the stmac binding itself. Do you mean I should document the sgmii
adapter as well?
>
> > +
> > + if (index >= 0) {
> > + if (of_address_to_resource(np_sgmii_adapter, index,
> > + &res_tse_pcs)) {
> > + dev_err(dev,
> > + "%s: ERROR: failed mapping tse control port\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + dwmac->pcs.tse_pcs_base = devm_ioremap_resource(
> > + dev, &res_tse_pcs);
>
> can you check with indentation (scripts/Lindent)?
Noted. For some reason check_patch didn't catch this.
>
>
> > +
> > + if (IS_ERR(dwmac->pcs.tse_pcs_base)) {
> > + dev_err(dev, "%s: ERROR: failed mapping tse control port\n",
> > + __func__);
> > + return PTR_ERR(dwmac->pcs.sgmii_adapter_base);
> > + }
> > + }
> > + }
> > dwmac->reg_offset = reg_offset;
> > dwmac->reg_shift = reg_shift;
> > dwmac->sys_mgr_base_addr = sys_mgr_base_addr;
> > @@ -153,6 +236,7 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
> > break;
> > case PHY_INTERFACE_MODE_MII:
> > case PHY_INTERFACE_MODE_GMII:
> > + case PHY_INTERFACE_MODE_SGMII:
> > val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
> > break;
> > default:
> > @@ -172,6 +256,10 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac)
> > ctrl |= val << reg_shift;
> >
> > regmap_write(sys_mgr_base_addr, reg_offset, ctrl);
> > +
> > + if (phymode == PHY_INTERFACE_MODE_SGMII)
> > + tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs);
> > +
> > return 0;
> > }
> >
> > @@ -191,6 +279,12 @@ static void *socfpga_dwmac_probe(struct platform_device *pdev)
> > return ERR_PTR(ret);
> > }
> >
> > + ret = socfpga_dwmac_setup(dwmac);
> > + if (ret) {
> > + dev_err(dev, "couldn't setup SoC glue (%d)\n", ret);
> > + return ERR_PTR(ret);
> > + }
> > +
> > return dwmac;
> > }
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> > new file mode 100644
> > index 0000000..1989bd1
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> > @@ -0,0 +1,261 @@
> > +/* Copyright Altera Corporation (C) 2016. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2,
> > + * as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Author: Tien Hock Loh <thloh@altera.com>
> > + */
> > +
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_net.h>
> > +#include <linux/phy.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/stmmac.h>
> > +
> > +#include "stmmac.h"
> > +#include "stmmac_platform.h"
> > +#include "tse_pcs.h"
> > +
> > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
> > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
> > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
> > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH 2
> > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK 0x00000003
> > +
> > +#define EMAC_SPLITTER_CTRL_REG 0x0
> > +#define EMAC_SPLITTER_CTRL_SPEED_MASK 0x3
> > +#define EMAC_SPLITTER_CTRL_SPEED_10 0x2
> > +#define EMAC_SPLITTER_CTRL_SPEED_100 0x3
> > +#define EMAC_SPLITTER_CTRL_SPEED_1000 0x0
>
> pls use BIT and GENMASK.
Noted. Will do the next patch.
>
> ...
>
> > +static void config_tx_buffer(u16 data, void __iomem *base)
> > +{
> > + writew(data, base + SGMII_ADAPTER_CTRL_REG);
> > +}
>
> hmm, is this function really necessary? I think you can use
> directly writew or maybe writew_relaxed.
>
OK will do.
> > +
> > +static void tse_pcs_reset(void __iomem *base, struct tse_pcs *pcs)
> > +{
> > + int counter = 0;
> > + u16 val;
> > +
> > + val = readw(base + TSE_PCS_CONTROL_REG);
> > + val |= TSE_PCS_SW_RST_MASK;
> > + writew(val, base + TSE_PCS_CONTROL_REG);
> > +
> > + while (counter < TSE_PCS_SW_RESET_TIMEOUT) {
> > + val = readw(base + TSE_PCS_CONTROL_REG);
> > + val &= TSE_PCS_SW_RST_MASK;
> > + if (val == 0)
> > + break;
> > + counter++;
> > + udelay(1);
> > + }
> > + if (counter >= TSE_PCS_SW_RESET_TIMEOUT)
> > + dev_err(pcs->dev, "PCS could not get out of sw reset\n");
> > +}
> > +
>
> maybe this should return an error to stop the init if reset is NOK.
Yeah you're right. I'll fix this in the next patch.
>
>
> > +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs)
> > +{
> > + writew(0x0001, base + TSE_PCS_IF_MODE_REG);
> > +
> > + writew(TSE_PCS_SGMII_LINK_TIMER_0, base + TSE_PCS_LINK_TIMER_0_REG);
> > + writew(TSE_PCS_SGMII_LINK_TIMER_1, base + TSE_PCS_LINK_TIMER_1_REG);
> > +
> > + tse_pcs_reset(base, pcs);
> > + config_tx_buffer(SGMII_ADAPTER_ENABLE,
> > + pcs->sgmii_adapter_base);
> > +}
> > +
> > +static void pcs_link_timer_callback(unsigned long data)
> > +{
> > + u16 val = 0;
> > +
> > + struct tse_pcs *pcs = (struct tse_pcs *)data;
> > + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> > +
> > + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> > + val &= TSE_PCS_STATUS_LINK_MASK;
> > +
> > + if (val != 0) {
> > + dev_dbg(pcs->dev, "Adapter: Link is established\n");
> > + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base);
> > + } else {
> > + mod_timer(&pcs->link_timer, jiffies +
> > + msecs_to_jiffies(LINK_TIMER));
> > + }
> > +}
> > +
> > +static void auto_nego_timer_callback(unsigned long data)
> > +{
> > + u16 val = 0;
> > + u16 speed = 0;
> > + u16 duplex = 0;
> > +
> > + struct tse_pcs *pcs = (struct tse_pcs *)data;
> > + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> > +
> > + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> > + val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
> > +
> > + if (val != 0) {
> > + dev_dbg(pcs->dev, "Adapter: Auto Negotiation is completed\n");
> > + val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG);
> > + speed = val & TSE_PCS_PARTNER_SPEED_MASK;
> > + duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK;
> > +
> > + if (speed == TSE_PCS_PARTNER_SPEED_10 &&
> > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> > + dev_dbg(pcs->dev,
> > + "Adapter: Link Partner is Up - 10/Full\n");
> > + else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
> > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> > + dev_dbg(pcs->dev,
> > + "Adapter: Link Partner is Up - 100/Full\n");
> > + else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
> > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> > + dev_dbg(pcs->dev,
> > + "Adapter: Link Partner is Up - 1000/Full\n");
> > + else if (speed == TSE_PCS_PARTNER_SPEED_10 &&
> > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> > + dev_err(pcs->dev,
> > + "Adapter does not support Half Duplex\n");
> > + else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
> > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> > + dev_err(pcs->dev,
> > + "Adapter does not support Half Duplex\n");
> > + else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
> > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> > + dev_err(pcs->dev,
> > + "Adapter does not support Half Duplex\n");
> > + else
> > + dev_err(pcs->dev,
> > + "Adapter: Invalid Partner Speed and Duplex\n");
>
> ANE is completed but speed or duplex is NOK. Any failure to signalling?
> I see that you then enable the adpter in any case.
>
> Maybe we could try to restart ANE again or force it (reducing the speed)
> I wonder what happens if, for some reason, there is some hw problem. In
> that case the timer is always running signalling an invalid Parter
> speed. Anyway, this is jus a question... I expect that this error will
> always point us to a problem to debug further (if occurs).
Let me look at how we can handle the case. Perhaps we could do a restart
and register the timer again. I'm just worried it will go into an
infinite timer registering hogging up the kernel if the hardware really
fails. Maybe I can do a n-time retry here. Looking into this. Let me
know if you have any opinions on this.
I haven't been able to check for this behaviour though, negative testing
on this code isn't too easy to simulate.
>
> > +
> > + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base);
> > + } else {
> > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> > + val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
> > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> > +
> > + tse_pcs_reset(tse_pcs_base, pcs);
> > + mod_timer(&pcs->an_timer, jiffies +
> > + msecs_to_jiffies(AUTONEGO_TIMER));
> > + }
> > +}
> > +
> > +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
> > + unsigned int speed)
> > +{
> > + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> > + u32 val;
> > +
> > + config_tx_buffer(SGMII_ADAPTER_DISABLE, sgmii_adapter_base);
> > +
> > + if (phy_dev->autoneg == AUTONEG_ENABLE) {
> > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> > + val |= TSE_PCS_CONTROL_AN_EN_MASK;
> > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> > +
> > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> > + val |= TSE_PCS_USE_SGMII_AN_MASK;
> > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> > +
> > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> > + val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
> > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> > +
> > + tse_pcs_reset(tse_pcs_base, pcs);
>
> why do you need to reset here?
This is done according to the Altera TSE specification, whenever a
configuration changes, it is recommended that the PCS gets reset.
>
> > +
> > + setup_timer(&pcs->an_timer,
> > + auto_nego_timer_callback,
> > + (unsigned long)pcs);
> > + mod_timer(&pcs->an_timer, jiffies +
> > + msecs_to_jiffies(AUTONEGO_TIMER));
> > + } else if (phy_dev->autoneg == AUTONEG_DISABLE) {
> > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> > + val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
> > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> > +
> > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> > + val &= ~TSE_PCS_USE_SGMII_AN_MASK;
> > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> > +
> > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> > + val &= ~TSE_PCS_SGMII_SPEED_MASK;
> > +
> > + switch (speed) {
> > + case 1000:
> > + val |= TSE_PCS_SGMII_SPEED_1000;
> > + break;
> > + case 100:
> > + val |= TSE_PCS_SGMII_SPEED_100;
> > + break;
> > + case 10:
> > + val |= TSE_PCS_SGMII_SPEED_10;
> > + break;
> > + default:
> > + return;
> > + }
> > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> > +
> > + tse_pcs_reset(tse_pcs_base, pcs);
> > +
> > + setup_timer(&pcs->link_timer,
> > + pcs_link_timer_callback,
> > + (unsigned long)pcs);
> > + mod_timer(&pcs->link_timer, jiffies +
> > + msecs_to_jiffies(LINK_TIMER));
>
> I wonder if we can have just one timer to manage ANE and LINK.
>
That would increase the complexity of the code because we would need to
check the callback type on when the callback is triggered and call the
correct function.
> > + }
> > +}
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
> > new file mode 100644
> > index 0000000..e3d4511
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
> > @@ -0,0 +1,36 @@
> > +/* Copyright Altera Corporation (C) 2016. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2,
> > + * as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Author: Tien Hock Loh <thloh@altera.com>
> > + */
> > +
> > +#ifndef __TSE_PCS_H__
> > +#define __TSE_PCS_H__
> > +
> > +#include <linux/phy.h>
> > +#include <linux/timer.h>
> > +
> > +struct tse_pcs {
> > + struct device *dev;
> > + void __iomem *tse_pcs_base;
> > + void __iomem *sgmii_adapter_base;
> > + struct timer_list an_timer;
> > + struct timer_list link_timer;
> > +};
> > +
> > +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs);
> > +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
> > + unsigned int speed);
> > +
> > +#endif /* __TSE_PCS_H__ */
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
2016-06-09 5:48 ` Tien Hock Loh
@ 2016-06-09 6:20 ` Giuseppe CAVALLARO
2016-06-10 6:12 ` Tien Hock Loh
0 siblings, 1 reply; 7+ messages in thread
From: Giuseppe CAVALLARO @ 2016-06-09 6:20 UTC (permalink / raw)
To: Tien Hock Loh
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
devicetree, linux-kernel, netdev, thloh85, cnphoon
Hello Tien Hock
On 6/9/2016 7:48 AM, Tien Hock Loh wrote:
[snip]
>>> .../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
>>> .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
>>> drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
>>> drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
>>> 5 files changed, 419 insertions(+), 24 deletions(-)
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
>>
>> I just wonder if it could make sense to rename the
>> tse_pcs.[hc] files or creating a sub-directory for altera devel.
>> It seems that tse_pcs.[hc] are common files but this support
>> is for Altera.
>> Anyway, I let you decide and I also ask you to update the stmmac.txt
>> file.
>
> Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename
> them, would altr_tse_pcs.[hc] be good? I don't think creating a
> sub-directory with only 2 files is necessary though.
ok for two files w/o sub-dir.
>
> I see that stmmac.txt is generic, and other vendor's PCS support
> documents their specific uses in their own *-dwmac.txt (eg.
> rockchip-dwmac.txt). Is this not the case?
yes you can use this name convention. I let you decide.
[snip]
>>> +
>>> + index = of_property_match_string(np_sgmii_adapter, "reg-names",
>>> + "eth_tse_control_port");
>>
>> reg-names looks to be specific and mandatory, IMO they should be
>> documented in the binding.
>
> That's the binding for the adapter (the phandle to the sgmii adapter),
> not the stmac binding itself. Do you mean I should document the sgmii
> adapter as well?
no I just meant for the adapter binding, I had understood that
eth_tse_control_port and gmii_to_sgmii_adapter_avalon_slave
were not documented and these seem to be mandatory.
[snip]
>>> +
>>> +static void auto_nego_timer_callback(unsigned long data)
>>> +{
>>> + u16 val = 0;
>>> + u16 speed = 0;
>>> + u16 duplex = 0;
>>> +
>>> + struct tse_pcs *pcs = (struct tse_pcs *)data;
>>> + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
>>> + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
>>> +
>>> + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
>>> + val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
[snip]
>>
>> ANE is completed but speed or duplex is NOK. Any failure to signalling?
>> I see that you then enable the adpter in any case.
>>
>> Maybe we could try to restart ANE again or force it (reducing the speed)
>> I wonder what happens if, for some reason, there is some hw problem. In
>> that case the timer is always running signalling an invalid Parter
>> speed. Anyway, this is jus a question... I expect that this error will
>> always point us to a problem to debug further (if occurs).
>
> Let me look at how we can handle the case. Perhaps we could do a restart
> and register the timer again. I'm just worried it will go into an
> infinite timer registering hogging up the kernel if the hardware really
> fails. Maybe I can do a n-time retry here. Looking into this. Let me
> know if you have any opinions on this.
>
> I haven't been able to check for this behaviour though, negative testing
> on this code isn't too easy to simulate.
yes and I expect this can occur on hw / conf problems. Take a look at
how the Physical Abstraction Layer manages this.
Indeed, we can try to restart ANE for a while and then just report the
failure (dev_err). Or we can try to fix other speed or duplex. But not
sure this is a good solution. We just mask a problem.
[snip]
>>> +
>>> + setup_timer(&pcs->an_timer,
>>> + auto_nego_timer_callback,
>>> + (unsigned long)pcs);
>>> + mod_timer(&pcs->an_timer, jiffies +
>>> + msecs_to_jiffies(AUTONEGO_TIMER));
>>> + } else if (phy_dev->autoneg == AUTONEG_DISABLE) {
>>> + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
>>> + val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
>>> + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
>>> +
>>> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
>>> + val &= ~TSE_PCS_USE_SGMII_AN_MASK;
>>> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
>>> +
>>> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
>>> + val &= ~TSE_PCS_SGMII_SPEED_MASK;
>>> +
>>> + switch (speed) {
>>> + case 1000:
>>> + val |= TSE_PCS_SGMII_SPEED_1000;
>>> + break;
>>> + case 100:
>>> + val |= TSE_PCS_SGMII_SPEED_100;
>>> + break;
>>> + case 10:
>>> + val |= TSE_PCS_SGMII_SPEED_10;
>>> + break;
>>> + default:
>>> + return;
>>> + }
>>> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
>>> +
>>> + tse_pcs_reset(tse_pcs_base, pcs);
>>> +
>>> + setup_timer(&pcs->link_timer,
>>> + pcs_link_timer_callback,
>>> + (unsigned long)pcs);
>>> + mod_timer(&pcs->link_timer, jiffies +
>>> + msecs_to_jiffies(LINK_TIMER));
>>
>> I wonder if we can have just one timer to manage ANE and LINK.
>>
>
> That would increase the complexity of the code because we would need to
> check the callback type on when the callback is triggered and call the
> correct function.
hmm, in that case, you have two timers and no lock protection:
I suspect there could be some hidden problem. The link goes down
and a timer polls this then another one try to restart ANE and
both timers read the TSE_PCS_STATUS_REG.
IMO, a timer is enough and you could keep the code to manage ANE and
LINK in two different functions. Pls take a look at if this is feasible.
Peppe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
2016-06-09 6:20 ` Giuseppe CAVALLARO
@ 2016-06-10 6:12 ` Tien Hock Loh
2016-06-10 12:09 ` Giuseppe CAVALLARO
0 siblings, 1 reply; 7+ messages in thread
From: Tien Hock Loh @ 2016-06-10 6:12 UTC (permalink / raw)
To: Giuseppe CAVALLARO
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
devicetree, linux-kernel, netdev, thloh85, Chee Nouk Phoon
Hi Peppe,
On Wed, 2016-06-08 at 23:20 +0000, Giuseppe CAVALLARO wrote:
> Hello Tien Hock
>
> On 6/9/2016 7:48 AM, Tien Hock Loh wrote:
>
> [snip]
>
> >>> .../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
> >>> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
> >>> .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
> >>> drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
> >>> drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
> >>> 5 files changed, 419 insertions(+), 24 deletions(-)
> >>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
> >>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
> >>
> >> I just wonder if it could make sense to rename the
> >> tse_pcs.[hc] files or creating a sub-directory for altera devel.
> >> It seems that tse_pcs.[hc] are common files but this support
> >> is for Altera.
> >> Anyway, I let you decide and I also ask you to update the stmmac.txt
> >> file.
> >
> > Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename
> > them, would altr_tse_pcs.[hc] be good? I don't think creating a
> > sub-directory with only 2 files is necessary though.
>
> ok for two files w/o sub-dir.
>
> >
> > I see that stmmac.txt is generic, and other vendor's PCS support
> > documents their specific uses in their own *-dwmac.txt (eg.
> > rockchip-dwmac.txt). Is this not the case?
>
> yes you can use this name convention. I let you decide.
What I meant was we've documented the bindings in socfpga-dwmac.txt for
platform specific bindings, and I won't be updating stmmac.txt because
that is the generic driver binding. Agree?
>
> [snip]
>
>
> >>> +
> >>> + index = of_property_match_string(np_sgmii_adapter, "reg-names",
> >>> + "eth_tse_control_port");
> >>
> >> reg-names looks to be specific and mandatory, IMO they should be
> >> documented in the binding.
> >
> > That's the binding for the adapter (the phandle to the sgmii adapter),
> > not the stmac binding itself. Do you mean I should document the sgmii
> > adapter as well?
>
> no I just meant for the adapter binding, I had understood that
> eth_tse_control_port and gmii_to_sgmii_adapter_avalon_slave
> were not documented and these seem to be mandatory.
OK noted.
>
> [snip]
>
> >>> +
> >>> +static void auto_nego_timer_callback(unsigned long data)
> >>> +{
> >>> + u16 val = 0;
> >>> + u16 speed = 0;
> >>> + u16 duplex = 0;
> >>> +
> >>> + struct tse_pcs *pcs = (struct tse_pcs *)data;
> >>> + void __iomem *tse_pcs_base = pcs->tse_pcs_base;
> >>> + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
> >>> +
> >>> + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> >>> + val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
>
> [snip]
>
> >>
> >> ANE is completed but speed or duplex is NOK. Any failure to signalling?
> >> I see that you then enable the adpter in any case.
> >>
> >> Maybe we could try to restart ANE again or force it (reducing the speed)
> >> I wonder what happens if, for some reason, there is some hw problem. In
> >> that case the timer is always running signalling an invalid Parter
> >> speed. Anyway, this is jus a question... I expect that this error will
> >> always point us to a problem to debug further (if occurs).
> >
> > Let me look at how we can handle the case. Perhaps we could do a restart
> > and register the timer again. I'm just worried it will go into an
> > infinite timer registering hogging up the kernel if the hardware really
> > fails. Maybe I can do a n-time retry here. Looking into this. Let me
> > know if you have any opinions on this.
> >
> > I haven't been able to check for this behaviour though, negative testing
> > on this code isn't too easy to simulate.
>
> yes and I expect this can occur on hw / conf problems. Take a look at
> how the Physical Abstraction Layer manages this.
> Indeed, we can try to restart ANE for a while and then just report the
> failure (dev_err). Or we can try to fix other speed or duplex. But not
> sure this is a good solution. We just mask a problem.
Done some read up on the generic PHY in Physical Abstraction Layer and
it halts the PHY on aneg failure. I guess we can do do the same then, to
not enable the SGMII adapter.
>
> [snip]
>
> >>> +
> >>> + setup_timer(&pcs->an_timer,
> >>> + auto_nego_timer_callback,
> >>> + (unsigned long)pcs);
> >>> + mod_timer(&pcs->an_timer, jiffies +
> >>> + msecs_to_jiffies(AUTONEGO_TIMER));
> >>> + } else if (phy_dev->autoneg == AUTONEG_DISABLE) {
> >>> + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> >>> + val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
> >>> + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> >>> +
> >>> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> >>> + val &= ~TSE_PCS_USE_SGMII_AN_MASK;
> >>> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> >>> +
> >>> + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
> >>> + val &= ~TSE_PCS_SGMII_SPEED_MASK;
> >>> +
> >>> + switch (speed) {
> >>> + case 1000:
> >>> + val |= TSE_PCS_SGMII_SPEED_1000;
> >>> + break;
> >>> + case 100:
> >>> + val |= TSE_PCS_SGMII_SPEED_100;
> >>> + break;
> >>> + case 10:
> >>> + val |= TSE_PCS_SGMII_SPEED_10;
> >>> + break;
> >>> + default:
> >>> + return;
> >>> + }
> >>> + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
> >>> +
> >>> + tse_pcs_reset(tse_pcs_base, pcs);
> >>> +
> >>> + setup_timer(&pcs->link_timer,
> >>> + pcs_link_timer_callback,
> >>> + (unsigned long)pcs);
> >>> + mod_timer(&pcs->link_timer, jiffies +
> >>> + msecs_to_jiffies(LINK_TIMER));
> >>
> >> I wonder if we can have just one timer to manage ANE and LINK.
> >>
> >
> > That would increase the complexity of the code because we would need to
> > check the callback type on when the callback is triggered and call the
> > correct function.
>
> hmm, in that case, you have two timers and no lock protection:
> I suspect there could be some hidden problem. The link goes down
> and a timer polls this then another one try to restart ANE and
> both timers read the TSE_PCS_STATUS_REG.
> IMO, a timer is enough and you could keep the code to manage ANE and
> LINK in two different functions. Pls take a look at if this is feasible.
Yeah you're right about the lock protection. I'll patch them to use one
timer.
>
> Peppe
Tien Hock
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga
2016-06-10 6:12 ` Tien Hock Loh
@ 2016-06-10 12:09 ` Giuseppe CAVALLARO
0 siblings, 0 replies; 7+ messages in thread
From: Giuseppe CAVALLARO @ 2016-06-10 12:09 UTC (permalink / raw)
To: Tien Hock Loh
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
devicetree, linux-kernel, netdev, thloh85, Chee Nouk Phoon
Hello Tien Hock
On 6/10/2016 8:12 AM, Tien Hock Loh wrote:
>>> Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename
>>> > > them, would altr_tse_pcs.[hc] be good? I don't think creating a
>>> > > sub-directory with only 2 files is necessary though.
>> >
>> > ok for two files w/o sub-dir.
>> >
>>> > >
>>> > > I see that stmmac.txt is generic, and other vendor's PCS support
>>> > > documents their specific uses in their own *-dwmac.txt (eg.
>>> > > rockchip-dwmac.txt). Is this not the case?
>> >
>> > yes you can use this name convention. I let you decide.
> What I meant was we've documented the bindings in socfpga-dwmac.txt for
> platform specific bindings, and I won't be updating stmmac.txt because
> that is the generic driver binding. Agree?
ok if you documented all in socfpga-dwmac.txt
peppe
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-06-10 12:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 3:01 [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga thloh
2016-06-07 8:30 ` Giuseppe CAVALLARO
2016-06-09 5:48 ` Tien Hock Loh
2016-06-09 6:20 ` Giuseppe CAVALLARO
2016-06-10 6:12 ` Tien Hock Loh
2016-06-10 12:09 ` Giuseppe CAVALLARO
2016-06-08 19:52 ` Rob Herring
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).