[v7, PATCH 0/2] add Ethernet driver support for mt2712

[v7, PATCH 0/2] add Ethernet driver support for mt2712

[Biao Huang]

Changes in v7:
Add MODULE_LICENSE info in driver.

Biao Huang (2):
  net:stmmac: dwmac-mediatek: add support for mt2712
  dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712
    DWMAC

 .../devicetree/bindings/net/mediatek-dwmac.txt     |   87 +++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |    8 +
 drivers/net/ethernet/stmicro/stmmac/Makefile       |    1 +
 .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c   |  413 ++++++++++++++++++++
 4 files changed, 509 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek-dwmac.txt
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

-- 
1.7.9.5

[v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712

[Biao Huang]

Add Ethernet support for MediaTek SoCs from the mt2712 family

Signed-off-by: Biao Huang <biao.huang@mediatek.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |    8 +
 drivers/net/ethernet/stmicro/stmmac/Makefile       |    1 +
 .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c   |  413 ++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 324049e..6209cc1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -75,6 +75,14 @@ config DWMAC_LPC18XX
 	---help---
 	  Support for NXP LPC18xx/43xx DWMAC Ethernet.
 
+config DWMAC_MEDIATEK
+	tristate "MediaTek MT27xx GMAC support"
+	depends on OF && (ARCH_MEDIATEK || COMPILE_TEST)
+	help
+	  Support for MediaTek GMAC Ethernet controller.
+
+	  This selects the MT2712 SoC support for the stmmac driver.
+
 config DWMAC_MESON
 	tristate "Amlogic Meson dwmac support"
 	default ARCH_MESON
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 99967a8..bf09701 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_STMMAC_PLATFORM)	+= stmmac-platform.o
 obj-$(CONFIG_DWMAC_ANARION)	+= dwmac-anarion.o
 obj-$(CONFIG_DWMAC_IPQ806X)	+= dwmac-ipq806x.o
 obj-$(CONFIG_DWMAC_LPC18XX)	+= dwmac-lpc18xx.o
+obj-$(CONFIG_DWMAC_MEDIATEK)	+= dwmac-mediatek.o
 obj-$(CONFIG_DWMAC_MESON)	+= dwmac-meson.o dwmac-meson8b.o
 obj-$(CONFIG_DWMAC_OXNAS)	+= dwmac-oxnas.o
 obj-$(CONFIG_DWMAC_ROCKCHIP)	+= dwmac-rk.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
new file mode 100644
index 0000000..e400cbd
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
@@ -0,0 +1,413 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/regmap.h>
+#include <linux/stmmac.h>
+
+#include "stmmac.h"
+#include "stmmac_platform.h"
+
+/* Peri Configuration register for mt2712 */
+#define PERI_ETH_PHY_INTF_SEL	0x418
+#define PHY_INTF_MII		0
+#define PHY_INTF_RGMII		1
+#define PHY_INTF_RMII		4
+#define RMII_CLK_SRC_RXC	BIT(4)
+#define RMII_CLK_SRC_INTERNAL	BIT(5)
+
+#define PERI_ETH_DLY	0x428
+#define ETH_DLY_GTXC_INV	BIT(6)
+#define ETH_DLY_GTXC_ENABLE	BIT(5)
+#define ETH_DLY_GTXC_STAGES	GENMASK(4, 0)
+#define ETH_DLY_TXC_INV		BIT(20)
+#define ETH_DLY_TXC_ENABLE	BIT(19)
+#define ETH_DLY_TXC_STAGES	GENMASK(18, 14)
+#define ETH_DLY_RXC_INV		BIT(13)
+#define ETH_DLY_RXC_ENABLE	BIT(12)
+#define ETH_DLY_RXC_STAGES	GENMASK(11, 7)
+
+#define PERI_ETH_DLY_FINE	0x800
+#define ETH_RMII_DLY_TX_INV	BIT(2)
+#define ETH_FINE_DLY_GTXC	BIT(1)
+#define ETH_FINE_DLY_RXC	BIT(0)
+
+struct mac_delay_struct {
+	u32 tx_delay;
+	u32 rx_delay;
+	bool tx_inv;
+	bool rx_inv;
+	bool fine_tune;
+};
+
+struct mediatek_dwmac_plat_data {
+	const struct mediatek_dwmac_variant *variant;
+	struct mac_delay_struct mac_delay;
+	struct clk_bulk_data *clks;
+	struct device_node *np;
+	struct regmap *peri_regmap;
+	struct device *dev;
+	int phy_mode;
+	bool rmii_rxc;
+};
+
+struct mediatek_dwmac_variant {
+	int (*dwmac_set_phy_interface)(struct mediatek_dwmac_plat_data *plat);
+	int (*dwmac_set_delay)(struct mediatek_dwmac_plat_data *plat);
+
+	/* clock ids to be requested */
+	const char * const *clk_list;
+	int num_clks;
+
+	u32 dma_bit_mask;
+	u32 rx_delay_max;
+	u32 tx_delay_max;
+};
+
+/* list of clocks required for mac */
+static const char * const mt2712_dwmac_clk_l[] = {
+	"axi", "apb", "mac_main", "ptp_ref"
+};
+
+static int mt2712_set_interface(struct mediatek_dwmac_plat_data *plat)
+{
+	int rmii_rxc = plat->rmii_rxc ? RMII_CLK_SRC_RXC : 0;
+	u32 intf_val = 0;
+
+	/* select phy interface in top control domain */
+	switch (plat->phy_mode) {
+	case PHY_INTERFACE_MODE_MII:
+		intf_val |= PHY_INTF_MII;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		intf_val |= (PHY_INTF_RMII | rmii_rxc);
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		intf_val |= PHY_INTF_RGMII;
+		break;
+	default:
+		dev_err(plat->dev, "phy interface not supported\n");
+		return -EINVAL;
+	}
+
+	regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val);
+
+	return 0;
+}
+
+static void mt2712_delay_ps2stage(struct mac_delay_struct *mac_delay)
+{
+	if (mac_delay->fine_tune) {
+		/* 170ps per stage for fine tune delay macro circuit*/
+		mac_delay->tx_delay /= 170;
+		mac_delay->rx_delay /= 170;
+	} else {
+		/* 550ps per stage for coarse tune delay macro circuit*/
+		mac_delay->tx_delay /= 550;
+		mac_delay->rx_delay /= 550;
+	}
+}
+
+static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
+{
+	struct mac_delay_struct *mac_delay = &plat->mac_delay;
+	u32 delay_val = 0, fine_val = 0;
+
+	mt2712_delay_ps2stage(mac_delay);
+
+	switch (plat->phy_mode) {
+	case PHY_INTERFACE_MODE_MII:
+		delay_val |= FIELD_PREP(ETH_DLY_TXC_ENABLE, !!mac_delay->tx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_TXC_STAGES, mac_delay->tx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_TXC_INV, mac_delay->tx_inv);
+
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_ENABLE, !!mac_delay->rx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_STAGES, mac_delay->rx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_INV, mac_delay->rx_inv);
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		/* the rmii reference clock is from external phy,
+		 * and the property "rmii_rxc" indicates which pin(TXC/RXC)
+		 * the reference clk is connected to. The reference clock is a
+		 * received signal, so rx_delay/rx_inv are used to indicate
+		 * the reference clock timing adjustment
+		 */
+		if (plat->rmii_rxc) {
+			/* the rmii reference clock from outside is connected
+			 * to RXC pin, the reference clock will be adjusted
+			 * by RXC delay macro circuit.
+			 */
+			delay_val |= FIELD_PREP(ETH_DLY_RXC_ENABLE, !!mac_delay->rx_delay);
+			delay_val |= FIELD_PREP(ETH_DLY_RXC_STAGES, mac_delay->rx_delay);
+			delay_val |= FIELD_PREP(ETH_DLY_RXC_INV, mac_delay->rx_inv);
+		} else {
+			/* the rmii reference clock from outside is connected
+			 * to TXC pin, the reference clock will be adjusted
+			 * by TXC delay macro circuit.
+			 */
+			delay_val |= FIELD_PREP(ETH_DLY_TXC_ENABLE, !!mac_delay->rx_delay);
+			delay_val |= FIELD_PREP(ETH_DLY_TXC_STAGES, mac_delay->rx_delay);
+			delay_val |= FIELD_PREP(ETH_DLY_TXC_INV, mac_delay->rx_inv);
+		}
+		/* tx_inv will inverse the tx clock inside mac relateive to
+		 * reference clock from external phy,
+		 * and this bit is located in the same register with fine-tune
+		 */
+		if (mac_delay->tx_inv)
+			fine_val = ETH_RMII_DLY_TX_INV;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+		/* the PHY is not responsible for inserting any internal
+		 * delay by itself in PHY_INTERFACE_MODE_RGMII case,
+		 * so Ethernet MAC will insert delays for both transmit
+		 * and receive path here.
+		 */
+		if (mac_delay->fine_tune)
+			fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC;
+
+		delay_val |= FIELD_PREP(ETH_DLY_GTXC_ENABLE, !!mac_delay->tx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_GTXC_STAGES, mac_delay->tx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_GTXC_INV, mac_delay->tx_inv);
+
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_ENABLE, !!mac_delay->rx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_STAGES, mac_delay->rx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_INV, mac_delay->rx_inv);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		/* the PHY should insert an internal delay for the transmit
+		 * path in PHY_INTERFACE_MODE_RGMII_TXID case,
+		 * so Ethernet MAC will insert the delay for receive path here.
+		 */
+		if (mac_delay->fine_tune)
+			fine_val = ETH_FINE_DLY_RXC;
+
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_ENABLE, !!mac_delay->rx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_STAGES, mac_delay->rx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_RXC_INV, mac_delay->rx_inv);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		/* the PHY should insert an internal delay for the receive
+		 * path in PHY_INTERFACE_MODE_RGMII_RXID case,
+		 * so Ethernet MAC will insert the delay for transmit path here.
+		 */
+		if (mac_delay->fine_tune)
+			fine_val = ETH_FINE_DLY_GTXC;
+
+		delay_val |= FIELD_PREP(ETH_DLY_GTXC_ENABLE, !!mac_delay->tx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_GTXC_STAGES, mac_delay->tx_delay);
+		delay_val |= FIELD_PREP(ETH_DLY_GTXC_INV, mac_delay->tx_inv);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		/* the PHY should insert internal delays for both transmit
+		 * and receive path in PHY_INTERFACE_MODE_RGMII_RXID case,
+		 * so Ethernet MAC will NOT insert any delay here.
+		 */
+		break;
+	default:
+		dev_err(plat->dev, "phy interface not supported\n");
+		return -EINVAL;
+	}
+	regmap_write(plat->peri_regmap, PERI_ETH_DLY, delay_val);
+	regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val);
+
+	return 0;
+}
+
+static const struct mediatek_dwmac_variant mt2712_gmac_variant = {
+		.dwmac_set_phy_interface = mt2712_set_interface,
+		.dwmac_set_delay = mt2712_set_delay,
+		.clk_list = mt2712_dwmac_clk_l,
+		.num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l),
+		.dma_bit_mask = 33,
+		.rx_delay_max = 17600,
+		.tx_delay_max = 17600,
+};
+
+static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
+{
+	struct mac_delay_struct *mac_delay = &plat->mac_delay;
+	u32 tx_delay_ps, rx_delay_ps;
+
+	plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg");
+	if (IS_ERR(plat->peri_regmap)) {
+		dev_err(plat->dev, "Failed to get pericfg syscon\n");
+		return PTR_ERR(plat->peri_regmap);
+	}
+
+	plat->phy_mode = of_get_phy_mode(plat->np);
+	if (plat->phy_mode < 0) {
+		dev_err(plat->dev, "not find phy-mode\n");
+		return -EINVAL;
+	}
+
+	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
+		if (tx_delay_ps < plat->variant->tx_delay_max) {
+			mac_delay->tx_delay = tx_delay_ps;
+		} else {
+			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
+			return -EINVAL;
+		}
+	}
+
+	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
+		if (rx_delay_ps < plat->variant->rx_delay_max) {
+			mac_delay->rx_delay = rx_delay_ps;
+		} else {
+			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
+			return -EINVAL;
+		}
+	}
+
+	mac_delay->tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
+	mac_delay->rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
+	mac_delay->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");
+	plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc");
+
+	return 0;
+}
+
+static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat)
+{
+	const struct mediatek_dwmac_variant *variant = plat->variant;
+	int i, num = variant->num_clks;
+
+	plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL);
+	if (!plat->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++)
+		plat->clks[i].id = variant->clk_list[i];
+
+	return devm_clk_bulk_get(plat->dev, num, plat->clks);
+}
+
+static int mediatek_dwmac_init(struct platform_device *pdev, void *priv)
+{
+	struct mediatek_dwmac_plat_data *plat = priv;
+	const struct mediatek_dwmac_variant *variant = plat->variant;
+	int ret;
+
+	ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask));
+	if (ret) {
+		dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret);
+		return ret;
+	}
+
+	ret = variant->dwmac_set_phy_interface(plat);
+	if (ret) {
+		dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret);
+		return ret;
+	}
+
+	ret = variant->dwmac_set_delay(plat);
+	if (ret) {
+		dev_err(plat->dev, "failed to set delay value, err = %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks);
+	if (ret) {
+		dev_err(plat->dev, "failed to enable clks, err = %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv)
+{
+	struct mediatek_dwmac_plat_data *plat = priv;
+	const struct mediatek_dwmac_variant *variant = plat->variant;
+
+	clk_bulk_disable_unprepare(variant->num_clks, plat->clks);
+}
+
+static int mediatek_dwmac_probe(struct platform_device *pdev)
+{
+	struct mediatek_dwmac_plat_data *priv_plat;
+	struct plat_stmmacenet_data *plat_dat;
+	struct stmmac_resources stmmac_res;
+	int ret;
+
+	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL);
+	if (!priv_plat)
+		return -ENOMEM;
+
+	priv_plat->variant = of_device_get_match_data(&pdev->dev);
+	if (!priv_plat->variant) {
+		dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n");
+		return -EINVAL;
+	}
+
+	priv_plat->dev = &pdev->dev;
+	priv_plat->np = pdev->dev.of_node;
+
+	ret = mediatek_dwmac_config_dt(priv_plat);
+	if (ret)
+		return ret;
+
+	ret = mediatek_dwmac_clk_init(priv_plat);
+	if (ret)
+		return ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return ret;
+
+	plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac);
+	if (IS_ERR(plat_dat))
+		return PTR_ERR(plat_dat);
+
+	plat_dat->interface = priv_plat->phy_mode;
+	/* clk_csr_i = 250-300MHz & MDC = clk_csr_i/124 */
+	plat_dat->clk_csr = 5;
+	plat_dat->has_gmac4 = 1;
+	plat_dat->has_gmac = 0;
+	plat_dat->pmt = 0;
+	plat_dat->maxmtu = ETH_DATA_LEN;
+	plat_dat->bsp_priv = priv_plat;
+	plat_dat->init = mediatek_dwmac_init;
+	plat_dat->exit = mediatek_dwmac_exit;
+	mediatek_dwmac_init(pdev, priv_plat);
+
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (ret) {
+		stmmac_remove_config_dt(pdev, plat_dat);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id mediatek_dwmac_match[] = {
+	{ .compatible = "mediatek,mt2712-gmac",
+	  .data = &mt2712_gmac_variant },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, mediatek_dwmac_match);
+
+static struct platform_driver mediatek_dwmac_driver = {
+	.probe  = mediatek_dwmac_probe,
+	.remove = stmmac_pltfr_remove,
+	.driver = {
+		.name           = "dwmac-mediatek",
+		.pm		= &stmmac_pltfr_pm_ops,
+		.of_match_table = mediatek_dwmac_match,
+	},
+};
+module_platform_driver(mediatek_dwmac_driver);
+
+MODULE_AUTHOR("Biao Huang <biao.huang@mediatek.com>");
+MODULE_DESCRIPTION("MediaTek DWMAC specific glue layer");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712

[Andrew Lunn]

Hi Biao

> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* the PHY is not responsible for inserting any internal
> +		 * delay by itself in PHY_INTERFACE_MODE_RGMII case,
> +		 * so Ethernet MAC will insert delays for both transmit
> +		 * and receive path here.
> +		 */

What if the PCB designed has decided to do a kink in the clock to add
the delays? I don't think any of these delays should depend on the PHY
interface mode. It is up to the device tree writer to set both the PHY
delay and the MAC delay, based on knowledge of the board, including
any kicks in the tracks. The driver should then do what it is told.

> +	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
> +		if (tx_delay_ps < plat->variant->tx_delay_max) {
> +			mac_delay->tx_delay = tx_delay_ps;
> +		} else {
> +			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
> +		if (rx_delay_ps < plat->variant->rx_delay_max) {
> +			mac_delay->rx_delay = rx_delay_ps;
> +		} else {
> +			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	mac_delay->tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> +	mac_delay->rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> +	mac_delay->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");

Why is fine tune needed? If the requested delay can be done using fine
tune, it should use fine tune. If not, it should use rough tune. The
driver can work this out itself.

       Thanks
	Andrew

Re: [v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712

[biao huang]

Dear Andrew,
	Thanks for your comments.

On Thu, 2018-12-13 at 13:33 +0100, Andrew Lunn wrote:
> Hi Biao
> 
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +		/* the PHY is not responsible for inserting any internal
> > +		 * delay by itself in PHY_INTERFACE_MODE_RGMII case,
> > +		 * so Ethernet MAC will insert delays for both transmit
> > +		 * and receive path here.
> > +		 */
> 
> What if the PCB designed has decided to do a kink in the clock to add
> the delays? I don't think any of these delays should depend on the PHY
> interface mode. It is up to the device tree writer to set both the PHY
> delay and the MAC delay, based on knowledge of the board, including
> any kicks in the tracks. The driver should then do what it is told.
> 
Originally, we recommend equal trace length on PCB, which means that
RGMII delay by PCB traces is not recommended. so only PHY/MAC delay is
taken into account in the transmit/receive path.

as you described above, maybe the equal PCB trace length assumption is
not reasonable, and we'll only handle MAC delay-ps in our driver based
on the device tree information no matter which rgmii is selected.

Since David already applied this patch, I'll send another patch to fix
this issue.
> > +	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
> > +		if (tx_delay_ps < plat->variant->tx_delay_max) {
> > +			mac_delay->tx_delay = tx_delay_ps;
> > +		} else {
> > +			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
> > +		if (rx_delay_ps < plat->variant->rx_delay_max) {
> > +			mac_delay->rx_delay = rx_delay_ps;
> > +		} else {
> > +			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	mac_delay->tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> > +	mac_delay->rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> > +	mac_delay->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");
> 
> Why is fine tune needed? If the requested delay can be done using fine
> tune, it should use fine tune. If not, it should use rough tune. The
> driver can work this out itself.

find tune here represents a more accurate delay circuit than coarse
tune, and it's a parallel circuit of coarse tune.
For most delay, both fine and coarse tune can meet the requirement.
It's up to the user to select which one.

But only one of them can work at the same time, so we need a switch
flag(fine_tune here) to indicate which one is valid.
Driver can hardly work out which one is working according to delay-ps.

Please correct me if any misunderstanding.

> 
>        Thanks
> 	Andrew

Re: [v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712

[Florian Fainelli]

Le 12/13/18 à 7:01 PM, biao huang a écrit :
> Dear Andrew,
> 	Thanks for your comments.
> 
> On Thu, 2018-12-13 at 13:33 +0100, Andrew Lunn wrote:
>> Hi Biao
>>
>>> +	case PHY_INTERFACE_MODE_RGMII:
>>> +		/* the PHY is not responsible for inserting any internal
>>> +		 * delay by itself in PHY_INTERFACE_MODE_RGMII case,
>>> +		 * so Ethernet MAC will insert delays for both transmit
>>> +		 * and receive path here.
>>> +		 */
>>
>> What if the PCB designed has decided to do a kink in the clock to add
>> the delays? I don't think any of these delays should depend on the PHY
>> interface mode. It is up to the device tree writer to set both the PHY
>> delay and the MAC delay, based on knowledge of the board, including
>> any kicks in the tracks. The driver should then do what it is told.
>>
> Originally, we recommend equal trace length on PCB, which means that
> RGMII delay by PCB traces is not recommended. so only PHY/MAC delay is
> taken into account in the transmit/receive path.
> 
> as you described above, maybe the equal PCB trace length assumption is
> not reasonable, and we'll only handle MAC delay-ps in our driver based
> on the device tree information no matter which rgmii is selected.

Expecting identical PCB traces is something that is hard to enforce with
external customers, for internal reference boards, absolutely they
should have those traces of equal length.

> 
> Since David already applied this patch, I'll send another patch to fix
> this issue.
>>> +	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
>>> +		if (tx_delay_ps < plat->variant->tx_delay_max) {
>>> +			mac_delay->tx_delay = tx_delay_ps;
>>> +		} else {
>>> +			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>> +	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
>>> +		if (rx_delay_ps < plat->variant->rx_delay_max) {
>>> +			mac_delay->rx_delay = rx_delay_ps;
>>> +		} else {
>>> +			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>> +	mac_delay->tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
>>> +	mac_delay->rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
>>> +	mac_delay->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");
>>
>> Why is fine tune needed? If the requested delay can be done using fine
>> tune, it should use fine tune. If not, it should use rough tune. The
>> driver can work this out itself.
> 
> find tune here represents a more accurate delay circuit than coarse
> tune, and it's a parallel circuit of coarse tune.
> For most delay, both fine and coarse tune can meet the requirement.
> It's up to the user to select which one.
> 
> But only one of them can work at the same time, so we need a switch
> flag(fine_tune here) to indicate which one is valid.
> Driver can hardly work out which one is working according to delay-ps.
> 
> Please correct me if any misunderstanding.

You are giving a lot of options for users of this Ethernet controller to
shoot themselves in the feet and spend a good amount of time debugging
why their RGMII connection is not reliable or have timing violations.
-- 
Florian

Re: [v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712

[biao huang]

Dear Florian,
	Thanks for your comments.

On Thu, 2018-12-13 at 21:11 -0800, Florian Fainelli wrote:
> Le 12/13/18 à 7:01 PM, biao huang a écrit :
> > Dear Andrew,
> > 	Thanks for your comments.
> > 
> > On Thu, 2018-12-13 at 13:33 +0100, Andrew Lunn wrote:
> >> Hi Biao
> >>
> >>> +	case PHY_INTERFACE_MODE_RGMII:
> >>> +		/* the PHY is not responsible for inserting any internal
> >>> +		 * delay by itself in PHY_INTERFACE_MODE_RGMII case,
> >>> +		 * so Ethernet MAC will insert delays for both transmit
> >>> +		 * and receive path here.
> >>> +		 */
> >>
> >> What if the PCB designed has decided to do a kink in the clock to add
> >> the delays? I don't think any of these delays should depend on the PHY
> >> interface mode. It is up to the device tree writer to set both the PHY
> >> delay and the MAC delay, based on knowledge of the board, including
> >> any kicks in the tracks. The driver should then do what it is told.
> >>
> > Originally, we recommend equal trace length on PCB, which means that
> > RGMII delay by PCB traces is not recommended. so only PHY/MAC delay is
> > taken into account in the transmit/receive path.
> > 
> > as you described above, maybe the equal PCB trace length assumption is
> > not reasonable, and we'll only handle MAC delay-ps in our driver based
> > on the device tree information no matter which rgmii is selected.
> 
> Expecting identical PCB traces is something that is hard to enforce with
> external customers, for internal reference boards, absolutely they
> should have those traces of equal length.
> 
yes, we'll set the corresponding register based on the
tx-delay-ps/rx-delay-ps in device tree for rgmii interface.
the PHY_INTERFACE_MODE_RGMII/-RXID/-TXID/-ID share the same flow in
Ethernet driver.

A new patch will be sent to fix this issue.
> > 
> > Since David already applied this patch, I'll send another patch to fix
> > this issue.
> >>> +	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
> >>> +		if (tx_delay_ps < plat->variant->tx_delay_max) {
> >>> +			mac_delay->tx_delay = tx_delay_ps;
> >>> +		} else {
> >>> +			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
> >>> +		if (rx_delay_ps < plat->variant->rx_delay_max) {
> >>> +			mac_delay->rx_delay = rx_delay_ps;
> >>> +		} else {
> >>> +			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	mac_delay->tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> >>> +	mac_delay->rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> >>> +	mac_delay->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");
> >>
> >> Why is fine tune needed? If the requested delay can be done using fine
> >> tune, it should use fine tune. If not, it should use rough tune. The
> >> driver can work this out itself.
> > 
> > find tune here represents a more accurate delay circuit than coarse
> > tune, and it's a parallel circuit of coarse tune.
> > For most delay, both fine and coarse tune can meet the requirement.
> > It's up to the user to select which one.
> > 
> > But only one of them can work at the same time, so we need a switch
> > flag(fine_tune here) to indicate which one is valid.
> > Driver can hardly work out which one is working according to delay-ps.
> > 
> > Please correct me if any misunderstanding.
> 
> You are giving a lot of options for users of this Ethernet controller to
> shoot themselves in the feet and spend a good amount of time debugging
> why their RGMII connection is not reliable or have timing violations.
yes, since fine tune is more accurate, and can meet customer's
requirement, we'll remove the 'fine-tune' property in device tree,
enable fine-tune circuit by default in Ethernet driver, and abandon the
coarse delay mechanism. so customer will not be confused by the options.

I'll send a new patch to fix this issue.

[v7, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC

[Biao Huang]

The commit adds the device tree binding documentation for the MediaTek DWMAC
found on MediaTek MT2712.

Signed-off-by: Biao Huang <biao.huang@mediatek.com>
---
 .../devicetree/bindings/net/mediatek-dwmac.txt     |   87 ++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek-dwmac.txt

diff --git a/Documentation/devicetree/bindings/net/mediatek-dwmac.txt b/Documentation/devicetree/bindings/net/mediatek-dwmac.txt
new file mode 100644
index 0000000..4de479b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mediatek-dwmac.txt
@@ -0,0 +1,87 @@
+MediaTek DWMAC glue layer controller
+
+This file documents platform glue layer for stmmac.
+Please see stmmac.txt for the other unchanged properties.
+
+The device node has following properties.
+
+Required properties:
+- compatible:  Should be "mediatek,mt2712-gmac" for MT2712 SoC
+- reg:  Address and length of the register set for the device
+- interrupts:  Should contain the MAC interrupts
+- interrupt-names: Should contain a list of interrupt names corresponding to
+	the interrupts in the interrupts property, if available.
+	Should be "macirq" for the main MAC IRQ
+- clocks: Must contain a phandle for each entry in clock-names.
+- clock-names: The name of the clock listed in the clocks property. These are
+	"axi", "apb", "mac_main", "ptp_ref" for MT2712 SoC
+- mac-address: See ethernet.txt in the same directory
+- phy-mode: See ethernet.txt in the same directory
+- mediatek,pericfg: A phandle to the syscon node that control ethernet
+	interface and timing delay.
+
+Optional properties:
+- mediatek,tx-delay-ps: TX clock delay macro value. Default is 0.
+	It should be defined for rgmii/rgmii-rxid/mii interface.
+- mediatek,rx-delay-ps: RX clock delay macro value. Default is 0.
+	It should be defined for rgmii/rgmii-txid/mii/rmii interface.
+Both delay properties need to be a multiple of 170 for fine-tune rgmii,
+range 0~31*170.
+Both delay properties need to be a multiple of 550 for coarse-tune rgmii,
+range 0~31*550.
+Both delay properties need to be a multiple of 550 for mii/rmii,
+range 0~31*550.
+
+- mediatek,fine-tune: boolean property, if present indicates that fine delay
+	is selected for rgmii interface.
+	If present, tx-delay-ps/rx-delay-ps is 170+/-50ps per stage.
+	Else tx-delay-ps/rx-delay-ps of coarse delay macro is 0.55+/-0.2ns per stage.
+	This property do not apply to non-rgmii PHYs.
+	Only coarse-tune delay is supported for mii/rmii PHYs.
+- mediatek,rmii-rxc: boolean property, if present indicates that the rmii
+	reference clock, which is from external PHYs, is connected to RXC pin
+	on MT2712 SoC.
+	Otherwise, is connected to TXC pin.
+- mediatek,txc-inverse: boolean property, if present indicates that
+	1. tx clock will be inversed in mii/rgmii case,
+	2. tx clock inside MAC will be inversed relative to reference clock
+	   which is from external PHYs in rmii case, and it rarely happen.
+- mediatek,rxc-inverse: boolean property, if present indicates that
+	1. rx clock will be inversed in mii/rgmii case.
+	2. reference clock will be inversed when arrived at MAC in rmii case.
+- assigned-clocks: mac_main and ptp_ref clocks
+- assigned-clock-parents: parent clocks of the assigned clocks
+
+Example:
+	eth: ethernet@1101c000 {
+		compatible = "mediatek,mt2712-gmac";
+		reg = <0 0x1101c000 0 0x1300>;
+		interrupts = <GIC_SPI 237 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-names = "macirq";
+		phy-mode ="rgmii";
+		mac-address = [00 55 7b b5 7d f7];
+		clock-names = "axi",
+			      "apb",
+			      "mac_main",
+			      "ptp_ref",
+			      "ptp_top";
+		clocks = <&pericfg CLK_PERI_GMAC>,
+			 <&pericfg CLK_PERI_GMAC_PCLK>,
+			 <&topckgen CLK_TOP_ETHER_125M_SEL>,
+			 <&topckgen CLK_TOP_ETHER_50M_SEL>;
+		assigned-clocks = <&topckgen CLK_TOP_ETHER_125M_SEL>,
+				  <&topckgen CLK_TOP_ETHER_50M_SEL>;
+		assigned-clock-parents = <&topckgen CLK_TOP_ETHERPLL_125M>,
+					 <&topckgen CLK_TOP_APLL1_D3>;
+		mediatek,pericfg = <&pericfg>;
+		mediatek,tx-delay-ps = <1530>;
+		mediatek,rx-delay-ps = <1530>;
+		mediatek,fine-tune;
+		mediatek,rmii-rxc;
+		mediatek,txc-inverse;
+		mediatek,rxc-inverse;
+		snps,txpbl = <32>;
+		snps,rxpbl = <32>;
+		snps,reset-gpio = <&pio 87 GPIO_ACTIVE_LOW>;
+		snps,reset-active-low;
+	};
-- 
1.7.9.5

Re: [v7, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC

[Andrew Lunn]

> +- mediatek,rx-delay-ps: RX clock delay macro value. Default is 0.
> +	It should be defined for rgmii/rgmii-txid/mii/rmii interface.
> +Both delay properties need to be a multiple of 170 for fine-tune rgmii,
> +range 0~31*170.
> +Both delay properties need to be a multiple of 550 for coarse-tune rgmii,
> +range 0~31*550.
> +Both delay properties need to be a multiple of 550 for mii/rmii,
> +range 0~31*550.

You see "need to be" here, but the driver does not care. It will round
down. Please either make the driver check, or change the text to
rather say it will round down.

       Andrew

Re: [v7, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC

[biao huang]

Dear Andrew,

	Thanks for your comments.
On Thu, 2018-12-13 at 13:36 +0100, Andrew Lunn wrote:
> > +- mediatek,rx-delay-ps: RX clock delay macro value. Default is 0.
> > +	It should be defined for rgmii/rgmii-txid/mii/rmii interface.
> > +Both delay properties need to be a multiple of 170 for fine-tune rgmii,
> > +range 0~31*170.
> > +Both delay properties need to be a multiple of 550 for coarse-tune rgmii,
> > +range 0~31*550.
> > +Both delay properties need to be a multiple of 550 for mii/rmii,
> > +range 0~31*550.
> 
> You see "need to be" here, but the driver does not care. It will round
> down. Please either make the driver check, or change the text to
> rather say it will round down.
OK, will mark "round down" in the binding file.
> 
>        Andrew

Re: [v7, PATCH 0/2] add Ethernet driver support for mt2712

[biao huang]

Dear All,
	Sorry, I misunderstood David's reply mail in v6 series, please ignore
v7 series.

Dear David,
	I send you another follow-up patch to fix up build warning, please
check it.

Thanks!

On Thu, 2018-12-13 at 09:56 +0800, Biao Huang wrote:
> Changes in v7:
> Add MODULE_LICENSE info in driver.
> 
> Biao Huang (2):
>   net:stmmac: dwmac-mediatek: add support for mt2712
>   dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712
>     DWMAC
> 
>  .../devicetree/bindings/net/mediatek-dwmac.txt     |   87 +++++
>  drivers/net/ethernet/stmicro/stmmac/Kconfig        |    8 +
>  drivers/net/ethernet/stmicro/stmmac/Makefile       |    1 +
>  .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c   |  413 ++++++++++++++++++++
>  4 files changed, 509 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/mediatek-dwmac.txt
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> 

Re: [v7, PATCH 0/2] add Ethernet driver support for mt2712

[David Miller]

From: Biao Huang <biao.huang@mediatek.com>
Date: Thu, 13 Dec 2018 09:56:11 +0800

> Changes in v7:
> Add MODULE_LICENSE info in driver.

Biao, I said I need a "fixup" patch, not the whole series again.

I've already applied your patches, and you need to send me a small
fix on top.

This shows that you are not generating your patches against my
net-next tree properly.  If you were, you would see that I applied
your changes already.