* [v5, PATCH 0/2] add Ethernet driver support for mt2712
@ 2018-11-22 10:28 Biao Huang
2018-11-22 10:28 ` [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add " Biao Huang
2018-11-22 10:28 ` [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC Biao Huang
0 siblings, 2 replies; 11+ messages in thread
From: Biao Huang @ 2018-11-22 10:28 UTC (permalink / raw)
To: davem, robh+dt
Cc: honghui.zhang, yt.shen, liguo.zhang, mark.rutland, nelson.chang,
matthias.bgg, biao.huang, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek, joabreu, andrew
Changes in v5:
1. fix dma bit mask setting.
2. remove one training whitespace.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
2018-11-22 10:28 [v5, PATCH 0/2] add Ethernet driver support for mt2712 Biao Huang
@ 2018-11-22 10:28 ` Biao Huang
2018-11-23 0:54 ` Sean Wang
2018-11-22 10:28 ` [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC Biao Huang
1 sibling, 1 reply; 11+ messages in thread
From: Biao Huang @ 2018-11-22 10:28 UTC (permalink / raw)
To: davem, robh+dt
Cc: honghui.zhang, yt.shen, liguo.zhang, mark.rutland, nelson.chang,
matthias.bgg, biao.huang, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek, joabreu, andrew
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 | 364 ++++++++++++++++++++
3 files changed, 373 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..dd8d4cc
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
@@ -0,0 +1,364 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ */
+#include <linux/io.h>
+#include <linux/mfd/syscon.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_GMII 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_PHY_DLY 0x428
+#define PHY_DLY_GTXC_INV BIT(6)
+#define PHY_DLY_GTXC_ENABLE BIT(5)
+#define PHY_DLY_GTXC_STAGES GENMASK(4, 0)
+#define PHY_DLY_TXC_INV BIT(20)
+#define PHY_DLY_TXC_ENABLE BIT(19)
+#define PHY_DLY_TXC_STAGES GENMASK(18, 14)
+#define PHY_DLY_TXC_SHIFT 14
+#define PHY_DLY_RXC_INV BIT(13)
+#define PHY_DLY_RXC_ENABLE BIT(12)
+#define PHY_DLY_RXC_STAGES GENMASK(11, 7)
+#define PHY_DLY_RXC_SHIFT 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;
+ u32 tx_inv;
+ u32 rx_inv;
+};
+
+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 fine_tune;
+ int phy_mode;
+ int 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_GMII;
+ break;
+ case PHY_INTERFACE_MODE_RMII:
+ intf_val |= PHY_INTF_RMII;
+ intf_val |= 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 int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
+{
+ struct mac_delay_struct *mac_delay = &plat->mac_delay;
+ u32 delay_val = 0;
+ u32 fine_val = 0;
+
+ switch (plat->phy_mode) {
+ case PHY_INTERFACE_MODE_MII:
+ delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
+ delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
+ PHY_DLY_TXC_STAGES;
+ delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
+ delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
+ delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
+ PHY_DLY_RXC_STAGES;
+ delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
+ break;
+ case PHY_INTERFACE_MODE_RMII:
+ if (plat->rmii_rxc) {
+ delay_val |= mac_delay->rx_delay ?
+ PHY_DLY_RXC_ENABLE : 0;
+ delay_val |= (mac_delay->rx_delay <<
+ PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
+ delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
+ fine_val |= mac_delay->tx_inv ?
+ ETH_RMII_DLY_TX_INV : 0;
+ } else {
+ delay_val |= mac_delay->rx_delay ?
+ PHY_DLY_TXC_ENABLE : 0;
+ delay_val |= (mac_delay->rx_delay <<
+ PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
+ delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
+ fine_val |= mac_delay->tx_inv ?
+ ETH_RMII_DLY_TX_INV : 0;
+ }
+ break;
+ case PHY_INTERFACE_MODE_RGMII:
+ fine_val = plat->fine_tune ?
+ (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
+ delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
+ delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
+ delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
+ delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
+ delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
+ PHY_DLY_RXC_STAGES;
+ delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
+ delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
+ delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
+ PHY_DLY_RXC_STAGES;
+ delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
+ delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
+ delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
+ delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ break;
+ default:
+ dev_err(plat->dev, "phy interface not supported\n");
+ return -EINVAL;
+ }
+ regmap_write(plat->peri_regmap, PERI_ETH_PHY_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 = 32,
+ .tx_delay_max = 32,
+};
+
+static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
+{
+ u32 tx_delay, rx_delay;
+
+ 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", &tx_delay)) {
+ if (tx_delay < plat->variant->tx_delay_max) {
+ plat->mac_delay.tx_delay = tx_delay;
+ } else {
+ dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
+ return -EINVAL;
+ }
+ }
+
+ if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
+ if (rx_delay < plat->variant->rx_delay_max) {
+ plat->mac_delay.rx_delay = rx_delay;
+ } else {
+ dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
+ return -EINVAL;
+ }
+ }
+
+ plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
+ plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
+ plat->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 num = variant->num_clks;
+ int i;
+
+ 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 = 0;
+
+ 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 = 0;
+
+ 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);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC
2018-11-22 10:28 [v5, PATCH 0/2] add Ethernet driver support for mt2712 Biao Huang
2018-11-22 10:28 ` [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add " Biao Huang
@ 2018-11-22 10:28 ` Biao Huang
2018-11-22 15:19 ` Andrew Lunn
1 sibling, 1 reply; 11+ messages in thread
From: Biao Huang @ 2018-11-22 10:28 UTC (permalink / raw)
To: davem, robh+dt
Cc: honghui.zhang, yt.shen, liguo.zhang, mark.rutland, nelson.chang,
matthias.bgg, biao.huang, netdev, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek, joabreu, andrew
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 | 78 ++++++++++++++++++++
1 file changed, 78 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..0f8a915
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mediatek-dwmac.txt
@@ -0,0 +1,78 @@
+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
+
+Optional properties:
+- mediatek,tx-delay: TX clock delay macro value. Range is 0~31. Default is 0.
+ It should be defined for rgmii/rgmii-rxid/mii interface.
+- mediatek,rx-delay: RX clock delay macro value. Range is 0~31. Default is 0.
+ It should be defined for rgmii/rgmii-txid/mii/rmii interface.
+- mediatek,fine-tune: boolean property, if present indicates that fine delay
+ is selected for rgmii interface.
+ If present, tx-delay/rx-delay is 170+/-50ps per stage.
+ Else tx-delay/rx-delay 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-id";
+ 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 = <9>;
+ mediatek,rx-delay = <9>;
+ 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC
2018-11-22 10:28 ` [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC Biao Huang
@ 2018-11-22 15:19 ` Andrew Lunn
2018-11-23 1:31 ` biao huang
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2018-11-22 15:19 UTC (permalink / raw)
To: Biao Huang
Cc: davem, robh+dt, honghui.zhang, yt.shen, liguo.zhang,
mark.rutland, nelson.chang, matthias.bgg, netdev, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, joabreu
On Thu, Nov 22, 2018 at 06:28:41PM +0800, Biao Huang wrote:
> 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>
> +Optional properties:
> +- mediatek,tx-delay: TX clock delay macro value. Range is 0~31. Default is 0.
> + It should be defined for rgmii/rgmii-rxid/mii interface.
> +- mediatek,rx-delay: RX clock delay macro value. Range is 0~31. Default is 0.
> + It should be defined for rgmii/rgmii-txid/mii/rmii interface.
You have received the same feedback at least twice now, from two
different maintainers, that the delay should be specified in pS, and
the driver should figure out what values to place into registers.
You should not ignore feedback like that. If you don't understand the
feedback, please ask us to explain it. If you don't agree with the
feedback, you need to argue why you think it is wrong, or why what you
are doing is better, etc.
We are here to help, but just ignoring us won't get you anywhere.
For the moment:
NACK
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
2018-11-22 10:28 ` [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add " Biao Huang
@ 2018-11-23 0:54 ` Sean Wang
2018-11-23 2:17 ` biao huang
0 siblings, 1 reply; 11+ messages in thread
From: Sean Wang @ 2018-11-23 0:54 UTC (permalink / raw)
To: biao.huang
Cc: davem, robh+dt, mark.rutland, devicetree, nelson.chang,
Andrew Lunn, netdev, Liguo Zhang, linux-kernel, Matthias Brugger,
joabreu, linux-mediatek, honghui.zhang, linux-arm-kernel
On Thu, Nov 22, 2018 at 2:29 AM Biao Huang <biao.huang@mediatek.com> wrote:
>
> 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 | 364 ++++++++++++++++++++
> 3 files changed, 373 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..dd8d4cc
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> @@ -0,0 +1,364 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + */
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.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_GMII 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_PHY_DLY 0x428
> +#define PHY_DLY_GTXC_INV BIT(6)
> +#define PHY_DLY_GTXC_ENABLE BIT(5)
> +#define PHY_DLY_GTXC_STAGES GENMASK(4, 0)
> +#define PHY_DLY_TXC_INV BIT(20)
> +#define PHY_DLY_TXC_ENABLE BIT(19)
> +#define PHY_DLY_TXC_STAGES GENMASK(18, 14)
> +#define PHY_DLY_TXC_SHIFT 14
> +#define PHY_DLY_RXC_INV BIT(13)
> +#define PHY_DLY_RXC_ENABLE BIT(12)
> +#define PHY_DLY_RXC_STAGES GENMASK(11, 7)
> +#define PHY_DLY_RXC_SHIFT 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;
> + u32 tx_inv;
bool should be enough
> + u32 rx_inv;
bool should be enough
> +};
> +
> +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 fine_tune;
bool
> + int phy_mode;
> + int rmii_rxc;
bool
> +};
> +
> +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_GMII;
> + break;
> + case PHY_INTERFACE_MODE_RMII:
> + intf_val |= PHY_INTF_RMII;
> + intf_val |= rmii_rxc;
how about putting into one line such as 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 int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
> +{
> + struct mac_delay_struct *mac_delay = &plat->mac_delay;
> + u32 delay_val = 0;
> + u32 fine_val = 0;
The same type declaration can be put into the one line
> +
> + switch (plat->phy_mode) {
There exists some room for code optimization in the switch statement
such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are
almost the same and even the configuration for the other PHY modes can
reuse their partial setup. It appears to be better using a common way
to set up various PHY modes.
> + case PHY_INTERFACE_MODE_MII:
> + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
> + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
> + PHY_DLY_TXC_STAGES;
> + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
> + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> + PHY_DLY_RXC_STAGES;
> + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> + break;
> + case PHY_INTERFACE_MODE_RMII:
> + if (plat->rmii_rxc) {
> + delay_val |= mac_delay->rx_delay ?
> + PHY_DLY_RXC_ENABLE : 0;
> + delay_val |= (mac_delay->rx_delay <<
> + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
> + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> + fine_val |= mac_delay->tx_inv ?
> + ETH_RMII_DLY_TX_INV : 0;
why is fine_val got from tx_inv?
> + } else {
> + delay_val |= mac_delay->rx_delay ?
> + PHY_DLY_TXC_ENABLE : 0;
> + delay_val |= (mac_delay->rx_delay <<
> + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
> + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
> + fine_val |= mac_delay->tx_inv ?
> + ETH_RMII_DLY_TX_INV : 0;
ditto, why is fine_val got from tx_inv?
> + }
> + break;
> + case PHY_INTERFACE_MODE_RGMII:
> + fine_val = plat->fine_tune ?
> + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
> + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> + PHY_DLY_RXC_STAGES;
> + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> + break;
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> + PHY_DLY_RXC_STAGES;
> + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register
bits, not with *_TXC_* bits? I'm a little confused about what path the
register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC?
> + break;
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
ditto, as the above quetion
> + break;
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + break;
> + default:
> + dev_err(plat->dev, "phy interface not supported\n");
> + return -EINVAL;
> + }
> + regmap_write(plat->peri_regmap, PERI_ETH_PHY_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 = 32,
> + .tx_delay_max = 32,
> +};
> +
> +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
> +{
> + u32 tx_delay, rx_delay;
> +
> + 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", &tx_delay)) {
> + if (tx_delay < plat->variant->tx_delay_max) {
> + plat->mac_delay.tx_delay = tx_delay;
> + } else {
> + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
> + return -EINVAL;
> + }
> + }
> +
> + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
> + if (rx_delay < plat->variant->rx_delay_max) {
> + plat->mac_delay.rx_delay = rx_delay;
> + } else {
> + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
> + return -EINVAL;
> + }
> + }
> +
> + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> + plat->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 num = variant->num_clks;
> + int i;
put into the same line seems good
> +
> + 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 = 0;
zero initialized seems unnecessary
> +
> + 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 = 0;
zero initialized seems unnecessary
> +
> + 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);
> --
> 1.7.9.5
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC
2018-11-22 15:19 ` Andrew Lunn
@ 2018-11-23 1:31 ` biao huang
2018-11-26 21:46 ` Rob Herring
0 siblings, 1 reply; 11+ messages in thread
From: biao huang @ 2018-11-23 1:31 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, robh+dt, honghui.zhang, yt.shen, liguo.zhang,
mark.rutland, nelson.chang, matthias.bgg, netdev, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, joabreu
Dear Andrew,
Thanks for you remind.
Sincerely, I respect any comment from any reviewer. If I didn't reply
for any comment, really sorry for that.
As to this "tx-delay" issue, the following reply in v3 maybe ignored.
https://lkml.org/lkml/2018/11/19/158
"the delay time in mediatek dwmac design is not so accurate,
the current mt2712 and the following ICs will not use the
same delay design, but will use stages to indicate different
delay time.
so maybe "mediatek.tx-delay" represent the delay stage is a
good choice"
And to make it clearer here.
In mt2712, there are two delay macro circuit: named fine-tune and
coarse-tune.
a. fine-tune, 170+/-50ps per stage, total 32 stages
b. coarse-tune, 0.55+/-0.2ns per stage, total 32 stages
If we only consider mt2712, delay in fine-tune select a integer
multiple of 170ps, delay in coarse-tune select a integer multiple of
550ps, for stage 0~31, the delay in fine-tune will not have the same
value with that in coarse-tune.
OK, It seems the property "fine-tune" can be eliminated .
But the following ic will not have the same accuracy as mt2712,
and maybe will not have two delay macro circuit to be selected.
1. assume two delay macro circuit in the following ic,
fine-tune, 100ps per stage, coarse-tune, 0.55ns per stage,
if we want delay 2.2ns, fine-tune will get a 22, and coarse-tune get a
4. We can't distinguish which delay macro we are choosing.
2. assume only one delay macro circuit is used, a similar case as 1
will also increase the complexity of driver.
Then, we need define more flag property to know which delay macro we
are handling.
The common things for all delay macro circuit in MediaTek mac design is
the stages, not the accuracy. so if we maintain stage info in "mediatek,
tx-delay", we only need care which stage we should choose.
And for each IC, we will recommend a best stage as a candidate.
Above is my personal opinion, may be my understanding is wrong,
welcome for further discussion.
Thanks a lot.
On Thu, 2018-11-22 at 16:19 +0100, Andrew Lunn wrote:
> On Thu, Nov 22, 2018 at 06:28:41PM +0800, Biao Huang wrote:
> > 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>
>
> > +Optional properties:
> > +- mediatek,tx-delay: TX clock delay macro value. Range is 0~31. Default is 0.
> > + It should be defined for rgmii/rgmii-rxid/mii interface.
> > +- mediatek,rx-delay: RX clock delay macro value. Range is 0~31. Default is 0.
> > + It should be defined for rgmii/rgmii-txid/mii/rmii interface.
>
> You have received the same feedback at least twice now, from two
> different maintainers, that the delay should be specified in pS, and
> the driver should figure out what values to place into registers.
>
> You should not ignore feedback like that. If you don't understand the
> feedback, please ask us to explain it. If you don't agree with the
> feedback, you need to argue why you think it is wrong, or why what you
> are doing is better, etc.
>
> We are here to help, but just ignoring us won't get you anywhere.
>
> For the moment:
>
> NACK
>
> Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
2018-11-23 0:54 ` Sean Wang
@ 2018-11-23 2:17 ` biao huang
2018-11-23 7:04 ` Sean Wang
0 siblings, 1 reply; 11+ messages in thread
From: biao huang @ 2018-11-23 2:17 UTC (permalink / raw)
To: Sean Wang
Cc: davem, robh+dt, mark.rutland, devicetree, nelson.chang,
Andrew Lunn, netdev, Liguo Zhang, linux-kernel, Matthias Brugger,
joabreu, linux-mediatek, honghui.zhang, linux-arm-kernel
Dear Sean,
Thanks for your comments.
On Thu, 2018-11-22 at 16:54 -0800, Sean Wang wrote:
> On Thu, Nov 22, 2018 at 2:29 AM Biao Huang <biao.huang@mediatek.com> wrote:
> >
> > 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 | 364 ++++++++++++++++++++
> > 3 files changed, 373 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..dd8d4cc
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> > @@ -0,0 +1,364 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + */
> > +#include <linux/io.h>
> > +#include <linux/mfd/syscon.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_GMII 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_PHY_DLY 0x428
> > +#define PHY_DLY_GTXC_INV BIT(6)
> > +#define PHY_DLY_GTXC_ENABLE BIT(5)
> > +#define PHY_DLY_GTXC_STAGES GENMASK(4, 0)
> > +#define PHY_DLY_TXC_INV BIT(20)
> > +#define PHY_DLY_TXC_ENABLE BIT(19)
> > +#define PHY_DLY_TXC_STAGES GENMASK(18, 14)
> > +#define PHY_DLY_TXC_SHIFT 14
> > +#define PHY_DLY_RXC_INV BIT(13)
> > +#define PHY_DLY_RXC_ENABLE BIT(12)
> > +#define PHY_DLY_RXC_STAGES GENMASK(11, 7)
> > +#define PHY_DLY_RXC_SHIFT 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;
> > + u32 tx_inv;
> bool should be enough
yes
>
> > + u32 rx_inv;
> bool should be enough
yes
>
> > +};
> > +
> > +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 fine_tune;
> bool
>
yes
> > + int phy_mode;
> > + int rmii_rxc;
> bool
yes
>
> > +};
> > +
> > +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_GMII;
> > + break;
> > + case PHY_INTERFACE_MODE_RMII:
> > + intf_val |= PHY_INTF_RMII;
> > + intf_val |= rmii_rxc;
> how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ?
>
ok, will change in next version.
> > + 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 int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
> > +{
> > + struct mac_delay_struct *mac_delay = &plat->mac_delay;
> > + u32 delay_val = 0;
> > + u32 fine_val = 0;
> The same type declaration can be put into the one line
>
Got it.
> > +
> > + switch (plat->phy_mode) {
>
> There exists some room for code optimization in the switch statement
> such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are
> almost the same and even the configuration for the other PHY modes can
> reuse their partial setup. It appears to be better using a common way
> to set up various PHY modes.
>
I'll try define a function to handle it.
And how about like this:
static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32
clk_shift, u32 clk_mask, u32 inv_bit) {
u32 value = 0
value |= delay ? en_bit : 0;
value |= (delay << clk_shift) & clk_mask;
value |= inv ? inv_bit : 0;
}
case PHY_INTERFACE_MODE_MII:
delay_value |= delay_setting(mac_delay->tx_delay,
mac_delay->tx_inv,
PHY_DLY_TXC_ENABLE,
PHY_DLY_TXC_SHIFT,
PHY_DLY_TXC_STAGES,
PHY_DLY_TXC_INV);
delay_value |= delay_setting(mac_delay->rx_delay,
mac_delay->rx_inv,
PHY_DLY_RXC_ENABLE,
PHY_DLY_RXC_SHIFT,
PHY_DLY_RXC_STAGES,
PHY_DLY_RXC_INV);
case PHY_INTERFACE_MODE_RMII:
if (plat->rmii_rxc) {
delay_value |= delay_setting(mac_delay->rx_delay,
mac_delay->rx_inv,
PHY_DLY_RXC_ENABLE,
PHY_DLY_RXC_SHIFT,
PHY_DLY_RXC_STAGES,
PHY_DLY_RXC_INV);
fine_val |= mac_delay->tx_inv ?
ETH_RMII_DLY_TX_INV : 0;
} else {
delay_value |= delay_setting(mac_delay->rx_delay,
mac_delay->rx_inv,
PHY_DLY_TXC_ENABLE,
PHY_DLY_TXC_SHIFT,
PHY_DLY_TXC_STAGES,
PHY_DLY_TXC_INV);
fine_val |= mac_delay->tx_inv ?
ETH_RMII_DLY_TX_INV : 0;
}
case PHY_INTERFACE_MODE_RGMII:
fine_val = plat->fine_tune ?
(ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
delay_value |= delay_setting(mac_delay->tx_delay,
mac_delay->tx_inv,
PHY_DLY_GTXC_ENABLE,
PHY_DLY_GTXC_SHIFT,
PHY_DLY_GTXC_STAGES,
PHY_DLY_GTXC_INV);
delay_value |= delay_setting(mac_delay->rx_delay,
mac_delay->rx_inv,
PHY_DLY_RXC_ENABLE,
PHY_DLY_RXC_SHIFT,
PHY_DLY_RXC_STAGES,
PHY_DLY_RXC_INV);
case PHY_INTERFACE_MODE_RGMII_TXID:
fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
delay_value |= delay_setting(mac_delay->rx_delay,
mac_delay->rx_inv,
PHY_DLY_RXC_ENABLE,
PHY_DLY_RXC_SHIFT,
PHY_DLY_RXC_STAGES,
PHY_DLY_RXC_INV);
case PHY_INTERFACE_MODE_RGMII_RXID:
fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
delay_value |= delay_setting(mac_delay->tx_delay,
mac_delay->tx_inv,
PHY_DLY_GTXC_ENABLE,
PHY_DLY_GTXC_SHIFT,
PHY_DLY_GTXC_STAGES,
PHY_DLY_GTXC_INV);
> > + case PHY_INTERFACE_MODE_MII:
> > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
> > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
> > + PHY_DLY_TXC_STAGES;
> > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
> > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > + PHY_DLY_RXC_STAGES;
> > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > + break;
> > + case PHY_INTERFACE_MODE_RMII:
> > + if (plat->rmii_rxc) {
> > + delay_val |= mac_delay->rx_delay ?
> > + PHY_DLY_RXC_ENABLE : 0;
> > + delay_val |= (mac_delay->rx_delay <<
> > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
> > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > + fine_val |= mac_delay->tx_inv ?
> > + ETH_RMII_DLY_TX_INV : 0;
> why is fine_val got from tx_inv?
>
becase the tx inv will inverse the tx clock inside mac relative to
reference clock from external phy, and this bit is located in the same
register with fine-tune.
maybe I should rename fine_val to avoid misunderstanding.
> > + } else {
> > + delay_val |= mac_delay->rx_delay ?
> > + PHY_DLY_TXC_ENABLE : 0;
> > + delay_val |= (mac_delay->rx_delay <<
> > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
> > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
> > + fine_val |= mac_delay->tx_inv ?
> > + ETH_RMII_DLY_TX_INV : 0;
> ditto, why is fine_val got from tx_inv?
>
same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the
same register with fine-tune.
> > + }
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII:
> > + fine_val = plat->fine_tune ?
> > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
> > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > + PHY_DLY_RXC_STAGES;
> > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > + PHY_DLY_RXC_STAGES;
> > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register
> bits, not with *_TXC_* bits? I'm a little confused about what path the
> register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC?
>
The PHY_INTERFACE_MODE_RGMII_TXID is defined in
Documentation/networking/phy.txt
means phy will handle delay in tx path, so mac need handle delay in rx
path here.
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> ditto, as the above quetion
>
Similar answer as above.
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + break;
> > + default:
> > + dev_err(plat->dev, "phy interface not supported\n");
> > + return -EINVAL;
> > + }
> > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_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 = 32,
> > + .tx_delay_max = 32,
> > +};
> > +
> > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
> > +{
> > + u32 tx_delay, rx_delay;
> > +
> > + 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", &tx_delay)) {
> > + if (tx_delay < plat->variant->tx_delay_max) {
> > + plat->mac_delay.tx_delay = tx_delay;
> > + } else {
> > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
> > + if (rx_delay < plat->variant->rx_delay_max) {
> > + plat->mac_delay.rx_delay = rx_delay;
> > + } else {
> > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> > + plat->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 num = variant->num_clks;
> > + int i;
> put into the same line seems good
>
ok
> > +
> > + 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 = 0;
> zero initialized seems unnecessary
>
ok, will not initialized here
> > +
> > + 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 = 0;
> zero initialized seems unnecessary
>
ok, will not initialized here
> > +
> > + 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);
> > --
> > 1.7.9.5
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
2018-11-23 2:17 ` biao huang
@ 2018-11-23 7:04 ` Sean Wang
2018-11-23 9:09 ` biao huang
0 siblings, 1 reply; 11+ messages in thread
From: Sean Wang @ 2018-11-23 7:04 UTC (permalink / raw)
To: biao.huang
Cc: davem, robh+dt, mark.rutland, devicetree, nelson.chang,
Andrew Lunn, netdev, Liguo Zhang, linux-kernel, Matthias Brugger,
joabreu, linux-mediatek, honghui.zhang, linux-arm-kernel
< ... >
> > > + /* select phy interface in top control domain */
> > > + switch (plat->phy_mode) {
> > > + case PHY_INTERFACE_MODE_MII:
> > > + intf_val |= PHY_INTF_MII_GMII;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RMII:
> > > + intf_val |= PHY_INTF_RMII;
> > > + intf_val |= rmii_rxc;
> > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ?
> >
> ok, will change in next version.
> > > + 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 int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
> > > +{
> > > + struct mac_delay_struct *mac_delay = &plat->mac_delay;
> > > + u32 delay_val = 0;
> > > + u32 fine_val = 0;
> > The same type declaration can be put into the one line
> >
> Got it.
> > > +
> > > + switch (plat->phy_mode) {
> >
> > There exists some room for code optimization in the switch statement
> > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are
> > almost the same and even the configuration for the other PHY modes can
> > reuse their partial setup. It appears to be better using a common way
> > to set up various PHY modes.
> >
> I'll try define a function to handle it.
> And how about like this:
> static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32
> clk_shift, u32 clk_mask, u32 inv_bit) {
> u32 value = 0
>
> value |= delay ? en_bit : 0;
> value |= (delay << clk_shift) & clk_mask;
> value |= inv ? inv_bit : 0;
> }
>
> case PHY_INTERFACE_MODE_MII:
> delay_value |= delay_setting(mac_delay->tx_delay,
> mac_delay->tx_inv,
> PHY_DLY_TXC_ENABLE,
> PHY_DLY_TXC_SHIFT,
> PHY_DLY_TXC_STAGES,
> PHY_DLY_TXC_INV);
We can reuse FIELD_PREP defined in include/linux/bitfield.h to make up
of the value instead of creating your own function delay_setting here,
and also PHY_DLY_TXC_SHIFT macro can be trimmed while you're using
FIED_PREP
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
>
> case PHY_INTERFACE_MODE_RMII:
> if (plat->rmii_rxc) {
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
> fine_val |= mac_delay->tx_inv ?
> ETH_RMII_DLY_TX_INV : 0;
> } else {
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
shoudn't the parametors be mac_delay->tx_delay and mac_delay->tx_inv?
> PHY_DLY_TXC_ENABLE,
> PHY_DLY_TXC_SHIFT,
> PHY_DLY_TXC_STAGES,
> PHY_DLY_TXC_INV);
> fine_val |= mac_delay->tx_inv ?
> ETH_RMII_DLY_TX_INV : 0;
if (plat->tx_inv)
fine_val = ETH_RMII_DLY_TX_INV;
the default fine_val is zero so zero assignement can be trimmed when
!plat-> tx_inv
> }
> case PHY_INTERFACE_MODE_RGMII:
> fine_val = plat->fine_tune ?
> (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
if (plat->fine_tune)
fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC;
the default fine_val is zero so zero assignement can be trimmed when
!plat->fine_tune
> delay_value |= delay_setting(mac_delay->tx_delay,
> mac_delay->tx_inv,
> PHY_DLY_GTXC_ENABLE,
> PHY_DLY_GTXC_SHIFT,
> PHY_DLY_GTXC_STAGES,
> PHY_DLY_GTXC_INV);
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
> case PHY_INTERFACE_MODE_RGMII_TXID:
> fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
> case PHY_INTERFACE_MODE_RGMII_RXID:
> fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> delay_value |= delay_setting(mac_delay->tx_delay,
> mac_delay->tx_inv,
> PHY_DLY_GTXC_ENABLE,
> PHY_DLY_GTXC_SHIFT,
> PHY_DLY_GTXC_STAGES,
> PHY_DLY_GTXC_INV);
>
phy_mode is used to indicate what phy mode would be tweaked when mac
is connected to the phy so I thought mac delay can be independent from
phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and
PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as
PHY_INTERFACE_MODE_RGMII does.
> > > + case PHY_INTERFACE_MODE_MII:
> > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
> > > + PHY_DLY_TXC_STAGES;
> > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
> > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > + PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RMII:
> > > + if (plat->rmii_rxc) {
> > > + delay_val |= mac_delay->rx_delay ?
> > > + PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay <<
> > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > + fine_val |= mac_delay->tx_inv ?
> > > + ETH_RMII_DLY_TX_INV : 0;
> > why is fine_val got from tx_inv?
> >
> becase the tx inv will inverse the tx clock inside mac relative to
> reference clock from external phy, and this bit is located in the same
> register with fine-tune.
> maybe I should rename fine_val to avoid misunderstanding.
If you add more comments to say that, fine_val remains would be okay
> > > + } else {
> > > + delay_val |= mac_delay->rx_delay ?
> > > + PHY_DLY_TXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay <<
> > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
> > > + fine_val |= mac_delay->tx_inv ?
> > > + ETH_RMII_DLY_TX_INV : 0;
> > ditto, why is fine_val got from tx_inv?
> >
> same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the
> same register with fine-tune.
adding a fewer comments helps to avoid some confusion
> > > + }
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII:
> > > + fine_val = plat->fine_tune ?
> > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
> > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > + PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > + PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register
> > bits, not with *_TXC_* bits? I'm a little confused about what path the
> > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC?
> >
> The PHY_INTERFACE_MODE_RGMII_TXID is defined in
> Documentation/networking/phy.txt
> means phy will handle delay in tx path, so mac need handle delay in rx
> path here.
See the above explains: phy_mode is used to indicate what phy mode
would be tweaked when mac is connected to the phy so I thought mac
delay can be independent of phy internal delay that means
PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can
apply the same setting as PHY_INTERFACE_MODE_RGMII does.
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > ditto, as the above quetion
> >
> Similar answer as above.
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > + break;
> > > + default:
> > > + dev_err(plat->dev, "phy interface not supported\n");
> > > + return -EINVAL;
> > > + }
> > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_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 = 32,
> > > + .tx_delay_max = 32,
> > > +};
> > > +
> > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
> > > +{
> > > + u32 tx_delay, rx_delay;
> > > +
> > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg");
you're also missing the property definition in dt-binding.
> > > + 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", &tx_delay)) {
> > > + if (tx_delay < plat->variant->tx_delay_max) {
> > > + plat->mac_delay.tx_delay = tx_delay;
> > > + } else {
> > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
> > > + if (rx_delay < plat->variant->rx_delay_max) {
> > > + plat->mac_delay.rx_delay = rx_delay;
> > > + } else {
> > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> > > + plat->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 num = variant->num_clks;
> > > + int i;
> > put into the same line seems good
> >
> ok
> > > +
> > > + 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 = 0;
> > zero initialized seems unnecessary
> >
> ok, will not initialized here
> > > +
> > > + 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 = 0;
> > zero initialized seems unnecessary
> >
> ok, will not initialized here
> > > +
> > > + 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;
> > > +
< ... >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
2018-11-23 7:04 ` Sean Wang
@ 2018-11-23 9:09 ` biao huang
0 siblings, 0 replies; 11+ messages in thread
From: biao huang @ 2018-11-23 9:09 UTC (permalink / raw)
To: Sean Wang
Cc: davem, robh+dt, mark.rutland, devicetree, nelson.chang,
Andrew Lunn, netdev, Liguo Zhang, linux-kernel, Matthias Brugger,
joabreu, linux-mediatek, honghui.zhang, linux-arm-kernel
Dear Sean,
Thanks for your comments.
If any misunderstanding, please correct me.
On Thu, 2018-11-22 at 23:04 -0800, Sean Wang wrote:
> < ... >
>
> > > > + /* select phy interface in top control domain */
> > > > + switch (plat->phy_mode) {
> > > > + case PHY_INTERFACE_MODE_MII:
> > > > + intf_val |= PHY_INTF_MII_GMII;
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RMII:
> > > > + intf_val |= PHY_INTF_RMII;
> > > > + intf_val |= rmii_rxc;
> > > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ?
> > >
> > ok, will change in next version.
> > > > + 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 int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
> > > > +{
> > > > + struct mac_delay_struct *mac_delay = &plat->mac_delay;
> > > > + u32 delay_val = 0;
> > > > + u32 fine_val = 0;
> > > The same type declaration can be put into the one line
> > >
> > Got it.
> > > > +
> > > > + switch (plat->phy_mode) {
> > >
> > > There exists some room for code optimization in the switch statement
> > > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are
> > > almost the same and even the configuration for the other PHY modes can
> > > reuse their partial setup. It appears to be better using a common way
> > > to set up various PHY modes.
> > >
> > I'll try define a function to handle it.
> > And how about like this:
> > static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32
> > clk_shift, u32 clk_mask, u32 inv_bit) {
> > u32 value = 0
> >
> > value |= delay ? en_bit : 0;
> > value |= (delay << clk_shift) & clk_mask;
> > value |= inv ? inv_bit : 0;
> > }
> >
> > case PHY_INTERFACE_MODE_MII:
> > delay_value |= delay_setting(mac_delay->tx_delay,
> > mac_delay->tx_inv,
> > PHY_DLY_TXC_ENABLE,
> > PHY_DLY_TXC_SHIFT,
> > PHY_DLY_TXC_STAGES,
> > PHY_DLY_TXC_INV);
>
> We can reuse FIELD_PREP defined in include/linux/bitfield.h to make up
> of the value instead of creating your own function delay_setting here,
> and also PHY_DLY_TXC_SHIFT macro can be trimmed while you're using
> FIED_PREP
>
ok, so it should like this:
delay_value |= FIELD_PREP(PHY_DLY_TXC_ENABLE, !!mac_delay->delay);
delay_value |= FIELD_PREP(PHY_DLY_TXC_STAGES, mac_delay->delay);
delay_value |= FIELD_PREP(PHY_DLY_TXC_INV, mac_delay->inv);
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> >
> > case PHY_INTERFACE_MODE_RMII:
> > if (plat->rmii_rxc) {
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> > fine_val |= mac_delay->tx_inv ?
> > ETH_RMII_DLY_TX_INV : 0;
> > } else {
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
>
> shoudn't the parametors be mac_delay->tx_delay and mac_delay->tx_inv?
>
in this case, the rmii reference clk from external phy is connected to
TXC pin, and property "rmii_rxc" will handle which pin the reference clk
is connected to. after that, the reference clk is only a received clk
from outside, so use rx_delay/rx_inv may be much better.
> > PHY_DLY_TXC_ENABLE,
> > PHY_DLY_TXC_SHIFT,
> > PHY_DLY_TXC_STAGES,
> > PHY_DLY_TXC_INV);
> > fine_val |= mac_delay->tx_inv ?
> > ETH_RMII_DLY_TX_INV : 0;
>
> if (plat->tx_inv)
> fine_val = ETH_RMII_DLY_TX_INV;
> the default fine_val is zero so zero assignement can be trimmed when
> !plat-> tx_inv
>
> > }
> > case PHY_INTERFACE_MODE_RGMII:
> > fine_val = plat->fine_tune ?
> > (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
>
> if (plat->fine_tune)
> fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC;
> the default fine_val is zero so zero assignement can be trimmed when
> !plat->fine_tune
>
ok, I'll not initialize fine_val.
> > delay_value |= delay_setting(mac_delay->tx_delay,
> > mac_delay->tx_inv,
> > PHY_DLY_GTXC_ENABLE,
> > PHY_DLY_GTXC_SHIFT,
> > PHY_DLY_GTXC_STAGES,
> > PHY_DLY_GTXC_INV);
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> > case PHY_INTERFACE_MODE_RGMII_TXID:
> > fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> > case PHY_INTERFACE_MODE_RGMII_RXID:
> > fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > delay_value |= delay_setting(mac_delay->tx_delay,
> > mac_delay->tx_inv,
> > PHY_DLY_GTXC_ENABLE,
> > PHY_DLY_GTXC_SHIFT,
> > PHY_DLY_GTXC_STAGES,
> > PHY_DLY_GTXC_INV);
> >
>
> phy_mode is used to indicate what phy mode would be tweaked when mac
> is connected to the phy so I thought mac delay can be independent from
> phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and
> PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as
> PHY_INTERFACE_MODE_RGMII does.
>
no, when phy adjust the tx delay, mac should not adjust tx delay at the
same time. ex: phy will delay 2ns, and mac delay 2ns, the total delay
will be 2+2=4ns.the whole delay will not meet the rgmii requirement.
And PHY_INTERFACE_MODE_RGMII_TXID/RXID/ID is clarified clearly in
phy.txt, I think it should be ok here.
More, I'll add comments here to avoid confusion.
> > > > + case PHY_INTERFACE_MODE_MII:
> > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
> > > > + PHY_DLY_TXC_STAGES;
> > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
> > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > > + PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RMII:
> > > > + if (plat->rmii_rxc) {
> > > > + delay_val |= mac_delay->rx_delay ?
> > > > + PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay <<
> > > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > > + fine_val |= mac_delay->tx_inv ?
> > > > + ETH_RMII_DLY_TX_INV : 0;
> > > why is fine_val got from tx_inv?
> > >
> > becase the tx inv will inverse the tx clock inside mac relative to
> > reference clock from external phy, and this bit is located in the same
> > register with fine-tune.
> > maybe I should rename fine_val to avoid misunderstanding.
>
> If you add more comments to say that, fine_val remains would be okay
>
OK, I'll add comments.
> > > > + } else {
> > > > + delay_val |= mac_delay->rx_delay ?
> > > > + PHY_DLY_TXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay <<
> > > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
> > > > + fine_val |= mac_delay->tx_inv ?
> > > > + ETH_RMII_DLY_TX_INV : 0;
> > > ditto, why is fine_val got from tx_inv?
> > >
> > same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the
> > same register with fine-tune.
>
> adding a fewer comments helps to avoid some confusion
>
OK.
> > > > + }
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII:
> > > > + fine_val = plat->fine_tune ?
> > > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
> > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > > + PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > > + PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register
> > > bits, not with *_TXC_* bits? I'm a little confused about what path the
> > > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC?
> > >
> > The PHY_INTERFACE_MODE_RGMII_TXID is defined in
> > Documentation/networking/phy.txt
> > means phy will handle delay in tx path, so mac need handle delay in rx
> > path here.
>
> See the above explains: phy_mode is used to indicate what phy mode
> would be tweaked when mac is connected to the phy so I thought mac
> delay can be independent of phy internal delay that means
> PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can
> apply the same setting as PHY_INTERFACE_MODE_RGMII does.
>
Maybe more comments will avoid confusion.
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > > ditto, as the above quetion
> > >
> > Similar answer as above.
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > > + break;
> > > > + default:
> > > > + dev_err(plat->dev, "phy interface not supported\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_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 = 32,
> > > > + .tx_delay_max = 32,
> > > > +};
> > > > +
> > > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
> > > > +{
> > > > + u32 tx_delay, rx_delay;
> > > > +
> > > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg");
>
> you're also missing the property definition in dt-binding.
>
thanks for remind. I'll add in next patch.
> > > > + 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", &tx_delay)) {
> > > > + if (tx_delay < plat->variant->tx_delay_max) {
> > > > + plat->mac_delay.tx_delay = tx_delay;
> > > > + } else {
> > > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
> > > > + if (rx_delay < plat->variant->rx_delay_max) {
> > > > + plat->mac_delay.rx_delay = rx_delay;
> > > > + } else {
> > > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> > > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> > > > + plat->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 num = variant->num_clks;
> > > > + int i;
> > > put into the same line seems good
> > >
> > ok
> > > > +
> > > > + 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 = 0;
> > > zero initialized seems unnecessary
> > >
> > ok, will not initialized here
> > > > +
> > > > + 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 = 0;
> > > zero initialized seems unnecessary
> > >
> > ok, will not initialized here
> > > > +
> > > > + 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;
> > > > +
>
> < ... >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC
2018-11-23 1:31 ` biao huang
@ 2018-11-26 21:46 ` Rob Herring
2018-11-27 1:39 ` biao huang
0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2018-11-26 21:46 UTC (permalink / raw)
To: biao huang
Cc: Andrew Lunn, davem, honghui.zhang, yt.shen, liguo.zhang,
mark.rutland, nelson.chang, matthias.bgg, netdev, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, joabreu
On Fri, Nov 23, 2018 at 09:31:16AM +0800, biao huang wrote:
> Dear Andrew,
>
> Thanks for you remind.
>
> Sincerely, I respect any comment from any reviewer. If I didn't reply
> for any comment, really sorry for that.
>
> As to this "tx-delay" issue, the following reply in v3 maybe ignored.
> https://lkml.org/lkml/2018/11/19/158
>
> "the delay time in mediatek dwmac design is not so accurate,
> the current mt2712 and the following ICs will not use the
> same delay design, but will use stages to indicate different
> delay time.
> so maybe "mediatek.tx-delay" represent the delay stage is a
> good choice"
>
> And to make it clearer here.
>
> In mt2712, there are two delay macro circuit: named fine-tune and
> coarse-tune.
> a. fine-tune, 170+/-50ps per stage, total 32 stages
> b. coarse-tune, 0.55+/-0.2ns per stage, total 32 stages
> If we only consider mt2712, delay in fine-tune select a integer
> multiple of 170ps, delay in coarse-tune select a integer multiple of
> 550ps, for stage 0~31, the delay in fine-tune will not have the same
> value with that in coarse-tune.
> OK, It seems the property "fine-tune" can be eliminated .
>
> But the following ic will not have the same accuracy as mt2712,
> and maybe will not have two delay macro circuit to be selected.
New IC will have new compatible string then. If it is different, then
likely these properties would have to change or have different meaning
unless you use time.
> 1. assume two delay macro circuit in the following ic,
> fine-tune, 100ps per stage, coarse-tune, 0.55ns per stage,
> if we want delay 2.2ns, fine-tune will get a 22, and coarse-tune get a
> 4. We can't distinguish which delay macro we are choosing.
Why wouldn't you just choose fine-tune for anything less than the max
range (3200ps in this example) and course for greater than 3100ps.
> 2. assume only one delay macro circuit is used, a similar case as 1
> will also increase the complexity of driver.
> Then, we need define more flag property to know which delay macro we
> are handling.
>
> The common things for all delay macro circuit in MediaTek mac design is
> the stages, not the accuracy. so if we maintain stage info in "mediatek,
> tx-delay", we only need care which stage we should choose.
> And for each IC, we will recommend a best stage as a candidate.
What if you had a 3rd delay circuit?
> Above is my personal opinion, may be my understanding is wrong,
> welcome for further discussion.
>
> Thanks a lot.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC
2018-11-26 21:46 ` Rob Herring
@ 2018-11-27 1:39 ` biao huang
0 siblings, 0 replies; 11+ messages in thread
From: biao huang @ 2018-11-27 1:39 UTC (permalink / raw)
To: Rob Herring
Cc: Andrew Lunn, davem, honghui.zhang, yt.shen, liguo.zhang,
mark.rutland, nelson.chang, matthias.bgg, netdev, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, joabreu
Dear Rob,
Thanks for your comments.
On Mon, 2018-11-26 at 15:46 -0600, Rob Herring wrote:
> On Fri, Nov 23, 2018 at 09:31:16AM +0800, biao huang wrote:
> > Dear Andrew,
> >
> > Thanks for you remind.
> >
> > Sincerely, I respect any comment from any reviewer. If I didn't reply
> > for any comment, really sorry for that.
> >
> > As to this "tx-delay" issue, the following reply in v3 maybe ignored.
> > https://lkml.org/lkml/2018/11/19/158
> >
> > "the delay time in mediatek dwmac design is not so accurate,
> > the current mt2712 and the following ICs will not use the
> > same delay design, but will use stages to indicate different
> > delay time.
> > so maybe "mediatek.tx-delay" represent the delay stage is a
> > good choice"
> >
> > And to make it clearer here.
> >
> > In mt2712, there are two delay macro circuit: named fine-tune and
> > coarse-tune.
> > a. fine-tune, 170+/-50ps per stage, total 32 stages
> > b. coarse-tune, 0.55+/-0.2ns per stage, total 32 stages
> > If we only consider mt2712, delay in fine-tune select a integer
> > multiple of 170ps, delay in coarse-tune select a integer multiple of
> > 550ps, for stage 0~31, the delay in fine-tune will not have the same
> > value with that in coarse-tune.
> > OK, It seems the property "fine-tune" can be eliminated .
> >
> > But the following ic will not have the same accuracy as mt2712,
> > and maybe will not have two delay macro circuit to be selected.
>
> New IC will have new compatible string then. If it is different, then
> likely these properties would have to change or have different meaning
> unless you use time.
>
OK, I'll use tx-delay-ps instead of tx-delay.
> > 1. assume two delay macro circuit in the following ic,
> > fine-tune, 100ps per stage, coarse-tune, 0.55ns per stage,
> > if we want delay 2.2ns, fine-tune will get a 22, and coarse-tune get a
> > 4. We can't distinguish which delay macro we are choosing.
>
> Why wouldn't you just choose fine-tune for anything less than the max
> range (3200ps in this example) and course for greater than 3100ps.
>
The fine-tune circuit and coarse-tune circuit are parallel, and
fine-tuen is a select switch.
It depends on users to choose which circuit is take effect.
I shouldn't assume users would choose fine-tune when delay < 3200ps, and
coarse for > 3100ps.
so, tx-delay-ps will be chosen, and "fine-tune" boolean property should
be remained as a indicator.
> > 2. assume only one delay macro circuit is used, a similar case as 1
> > will also increase the complexity of driver.
> > Then, we need define more flag property to know which delay macro we
> > are handling.
> >
> > The common things for all delay macro circuit in MediaTek mac design is
> > the stages, not the accuracy. so if we maintain stage info in "mediatek,
> > tx-delay", we only need care which stage we should choose.
> > And for each IC, we will recommend a best stage as a candidate.
>
> What if you had a 3rd delay circuit?
>
OK, tx-delay-ps will be a meaningful property.
If more delay circuit is added, fine-tune property(boolean --> u32) can
still be a indicator.
> > Above is my personal opinion, may be my understanding is wrong,
> > welcome for further discussion.
> >
> > Thanks a lot.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-11-27 1:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-22 10:28 [v5, PATCH 0/2] add Ethernet driver support for mt2712 Biao Huang
2018-11-22 10:28 ` [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add " Biao Huang
2018-11-23 0:54 ` Sean Wang
2018-11-23 2:17 ` biao huang
2018-11-23 7:04 ` Sean Wang
2018-11-23 9:09 ` biao huang
2018-11-22 10:28 ` [v5, PATCH 2/2] dt-binding: mediatek-dwmac: add binding document for MediaTek MT2712 DWMAC Biao Huang
2018-11-22 15:19 ` Andrew Lunn
2018-11-23 1:31 ` biao huang
2018-11-26 21:46 ` Rob Herring
2018-11-27 1:39 ` biao huang
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).