[PATCH v2 0/6] support GMAC driver for RK3288

[PATCH v2 0/6] support GMAC driver for RK3288

[Roger Chen]

Roger Chen (6):
  patch1: add driver for Rockchip RK3288 SoCs integrated GMAC
  patch2: define clock ID used for GMAC
  patch3: modify CRU config for Rockchip RK3288 SoCs integrated GMAC
  patch4: dts: rockchip: add gmac info for rk3288
  patch5: dts: rockchip: enable gmac on RK3288 evb board
  patch6: add document for Rockchip RK3288 GMAC

Tested on rk3288 evb board:
Execute the following command to enable ethernet,
set local IP and ping a remote host.

busybox ifconfig eth0 up
busybox ifconfig eth0 192.168.1.111
ping 192.168.1.1

-- 
1.7.9.5

[PATCH v2 0/6] support GMAC driver for RK3288

[Roger Chen]

Roger Chen (6):
  patch1: add driver for Rockchip RK3288 SoCs integrated GMAC
  patch2: define clock ID used for GMAC
  patch3: modify CRU config for Rockchip RK3288 SoCs integrated GMAC
  patch4: dts: rockchip: add gmac info for rk3288
  patch5: dts: rockchip: enable gmac on RK3288 evb board
  patch6: add document for Rockchip RK3288 GMAC

Tested on rk3288 evb board:
Execute the following command to enable ethernet,
set local IP and ping a remote host.

busybox ifconfig eth0 up
busybox ifconfig eth0 192.168.1.111
ping 192.168.1.1

-- 
1.7.9.5

[PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC

[Roger Chen]

This driver is based on stmmac driver.

modification based on Giuseppe CAVALLARO's suggestion:
1. use BIT()
	> +/*RK3288_GRF_SOC_CON3*/
	> +#define GMAC_TXCLK_DLY_ENABLE   ((0x4000 << 16) | (0x4000))
	> +#define GMAC_TXCLK_DLY_DISABLE  ((0x4000 << 16) | (0x0000))
	...

	why do not use BIT and BIT_MASK where possible?

	===>after modification:

	#define GRF_BIT(nr)     (BIT(nr) | BIT(nr+16))
	#define GRF_CLR_BIT(nr) (BIT(nr+16))
	#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
	#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
	...
2.
	> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
	> +             GMAC_PHY_INTF_SEL_RGMII);
	> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
	> +             GMAC_RMII_MODE_CLR);
	maybe you could perform just one write unless there is some HW
	constraint.

	===>after modification:

	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
			 GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);

3. use macros
	> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, 0xFFFFFFFF);
	> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
	> +             0x3<<2<<16 | 0x3<<2);

	pls use macros, these shift sequence is really help to decode

	===>after modification:

	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);

4. remove grf fail check in rk_gmac_setup()
	> +    if (IS_ERR(bsp_priv->grf))
	> +        dev_err(&pdev->dev, "Missing rockchip,grf property\n");

	I wonder if you can fail on here and save all the check in
	set_rgmii_speed etc.
	Maybe this can be considered a mandatory property for the glue-logic.

5. remove .tx_coe=1
	> +const struct stmmac_of_data rk_gmac_data = {
	> +    .has_gmac = 1,
	> +    .tx_coe = 1,

	FYI, on new gmac there is the HW capability register to dinamically
	provide you if coe is supported.

	IMO you should add the OF "compatible" string and in case of mac
	newer than the 3.50a you can remove coe.

Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
---
 drivers/net/ethernet/stmicro/stmmac/Makefile       |    2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  636 ++++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |    3 +
 .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |    1 +
 4 files changed, 641 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index ac4d562..73c2715 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o	\
 
 obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
 stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o	\
-		       dwmac-sti.o dwmac-socfpga.o
+		       dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
 
 obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
 stmmac-pci-objs:= stmmac_pci.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
new file mode 100644
index 0000000..870563f
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -0,0 +1,636 @@
+/**
+ * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
+ *
+ * Copyright (C) 2014 Chen-Zhi (Roger Chen)
+ *
+ * Chen-Zhi (Roger Chen)  <roger.chen@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/stmmac.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/phy.h>
+#include <linux/of_net.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+struct rk_priv_data {
+	struct platform_device *pdev;
+	int phy_iface;
+	bool power_ctrl_by_pmu;
+	char pmu_regulator[32];
+	int pmu_enable_level;
+
+	int power_io;
+	int power_io_level;
+	int reset_io;
+	int reset_io_level;
+	int phyirq_io;
+	int phyirq_io_level;
+
+	bool clk_enabled;
+	bool clock_input;
+
+	struct clk *clk_mac;
+	struct clk *clk_mac_pll;
+	struct clk *gmac_clkin;
+	struct clk *mac_clk_rx;
+	struct clk *mac_clk_tx;
+	struct clk *clk_mac_ref;
+	struct clk *clk_mac_refout;
+	struct clk *aclk_mac;
+	struct clk *pclk_mac;
+
+	int tx_delay;
+	int rx_delay;
+
+	struct regmap *grf;
+};
+
+#define RK3288_GRF_SOC_CON1 0x0248
+#define RK3288_GRF_SOC_CON3 0x0250
+#define RK3288_GRF_GPIO3D_E 0x01ec
+#define RK3288_GRF_GPIO4A_E 0x01f0
+#define RK3288_GRF_GPIO4B_E 0x01f4
+
+#define GPIO3D_2MA	0xFFFF0000
+#define GPIO3D_4MA	0xFFFF5555
+#define GPIO3D_8MA	0xFFFFAAAA
+#define GPIO3D_12MA	0xFFFFFFFF
+
+#define GPIO4A_2MA	0xFFFF0000
+#define GPIO4A_4MA	0xFFFF5555
+#define GPIO4A_8MA	0xFFFFAAAA
+#define GPIO4A_12MA	0xFFFFFFFF
+
+#define GRF_BIT(nr)	(BIT(nr) | BIT(nr+16))
+#define GRF_CLR_BIT(nr)	(BIT(nr+16))
+
+#define GPIO4B_2_2MA	(GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
+#define GPIO4B_2_4MA	(GRF_BIT(2) | GRF_CLR_BIT(3))
+#define GPIO4B_2_8MA	(GRF_CLR_BIT(2) | GRF_BIT(3))
+#define GPIO4B_2_12MA	(GRF_BIT(2) | GRF_BIT(3))
+
+/*RK3288_GRF_SOC_CON1*/
+#define GMAC_PHY_INTF_SEL_RGMII	(GRF_BIT(6) | GRF_CLR_BIT(7) | GRF_CLR_BIT(8))
+#define GMAC_PHY_INTF_SEL_RMII  (GRF_CLR_BIT(6) | GRF_CLR_BIT(7) | GRF_BIT(8))
+#define GMAC_FLOW_CTRL          GRF_BIT(9)
+#define GMAC_FLOW_CTRL_CLR      GRF_CLR_BIT(9)
+#define GMAC_SPEED_10M          GRF_CLR_BIT(10)
+#define GMAC_SPEED_100M         GRF_BIT(10)
+#define GMAC_RMII_CLK_25M       GRF_BIT(11)
+#define GMAC_RMII_CLK_2_5M      GRF_CLR_BIT(11)
+#define GMAC_CLK_125M           (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
+#define GMAC_CLK_25M            (GRF_BIT(12) | GRF_BIT(13))
+#define GMAC_CLK_2_5M           (GRF_CLR_BIT(12) | GRF_BIT(13))
+#define GMAC_RMII_MODE          GRF_BIT(14)
+#define GMAC_RMII_MODE_CLR      GRF_CLR_BIT(14)
+
+/*RK3288_GRF_SOC_CON3*/
+#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
+#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
+#define GMAC_RXCLK_DLY_ENABLE   GRF_BIT(15)
+#define GMAC_RXCLK_DLY_DISABLE  GRF_CLR_BIT(15)
+#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
+#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
+
+static void set_to_rgmii(struct rk_priv_data *bsp_priv,
+			 int tx_delay, int rx_delay)
+{
+	if (IS_ERR(bsp_priv->grf)) {
+		pr_err("%s: Missing rockchip,grf property\n", __func__);
+		return;
+	}
+
+	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+		     GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
+	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
+		     GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
+		     GMAC_CLK_RX_DL_CFG(rx_delay) |
+		     GMAC_CLK_TX_DL_CFG(tx_delay));
+	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
+	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
+	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
+
+	pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
+		 __func__, tx_delay, rx_delay);
+}
+
+static void set_to_rmii(struct rk_priv_data *bsp_priv)
+{
+	if (IS_ERR(bsp_priv->grf)) {
+		pr_err("%s: Missing rockchip,grf property\n", __func__);
+		return;
+	}
+
+	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+		     GMAC_PHY_INTF_SEL_RMII);
+	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+		     GMAC_RMII_MODE);
+}
+
+static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
+{
+	if (IS_ERR(bsp_priv->grf)) {
+		pr_err("%s: Missing rockchip,grf property\n", __func__);
+		return;
+	}
+
+	if (speed == 10)
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_2_5M);
+	else if (speed == 100)
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_25M);
+	else if (speed == 1000)
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_125M);
+	else
+		pr_err("unknown speed value for RGMII! speed=%d", speed);
+}
+
+static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
+{
+	if (IS_ERR(bsp_priv->grf)) {
+		pr_err("%s: Missing rockchip,grf property\n", __func__);
+		return;
+	}
+
+	if (speed == 10) {
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+			     GMAC_RMII_CLK_2_5M);
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+			     GMAC_SPEED_10M);
+	} else if (speed == 100) {
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+			     GMAC_RMII_CLK_25M);
+		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
+			     GMAC_SPEED_100M);
+	} else {
+		pr_err("unknown speed value for RMII! speed=%d", speed);
+	}
+}
+
+#define MAC_CLK_RX	"mac_clk_rx"
+#define MAC_CLK_TX	"mac_clk_tx"
+#define CLK_MAC_REF	"clk_mac_ref"
+#define CLK_MAC_REF_OUT	"clk_mac_refout"
+#define CLK_MAC_PLL	"clk_mac_pll"
+#define ACLK_MAC	"aclk_mac"
+#define PCLK_MAC	"pclk_mac"
+#define MAC_CLKIN	"ext_gmac"
+#define CLK_MAC		"stmmaceth"
+
+static int gmac_clk_init(struct rk_priv_data *bsp_priv)
+{
+	struct device *dev = &bsp_priv->pdev->dev;
+
+	bsp_priv->clk_enabled = false;
+
+	bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
+	if (IS_ERR(bsp_priv->mac_clk_rx))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, MAC_CLK_RX);
+
+	bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
+	if (IS_ERR(bsp_priv->mac_clk_tx))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, MAC_CLK_TX);
+
+	bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
+	if (IS_ERR(bsp_priv->clk_mac_ref))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, CLK_MAC_REF);
+
+	bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
+	if (IS_ERR(bsp_priv->clk_mac_refout))
+		pr_warn("%s: warning:cannot get clock %s\n",
+			__func__, CLK_MAC_REF_OUT);
+
+	bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
+	if (IS_ERR(bsp_priv->aclk_mac))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, ACLK_MAC);
+
+	bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
+	if (IS_ERR(bsp_priv->pclk_mac))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, PCLK_MAC);
+
+	bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
+	if (IS_ERR(bsp_priv->clk_mac_pll))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, CLK_MAC_PLL);
+
+	bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
+	if (IS_ERR(bsp_priv->gmac_clkin))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, MAC_CLKIN);
+
+	bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
+	if (IS_ERR(bsp_priv->clk_mac))
+		pr_warn("%s: warning: cannot get clock %s\n",
+			__func__, CLK_MAC);
+
+	if (bsp_priv->clock_input) {
+		pr_info("%s: clock input from PHY\n", __func__);
+	} else {
+		if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
+			clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
+
+		clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
+	}
+
+	return 0;
+}
+
+static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
+{
+	int phy_iface = phy_iface = bsp_priv->phy_iface;
+
+	if (enable) {
+		if (!bsp_priv->clk_enabled) {
+			if (phy_iface == PHY_INTERFACE_MODE_RMII) {
+				if (!IS_ERR(bsp_priv->mac_clk_rx))
+					clk_prepare_enable(
+						bsp_priv->mac_clk_rx);
+
+				if (!IS_ERR(bsp_priv->clk_mac_ref))
+					clk_prepare_enable(
+						bsp_priv->clk_mac_ref);
+
+				if (!IS_ERR(bsp_priv->clk_mac_refout))
+					clk_prepare_enable(
+						bsp_priv->clk_mac_refout);
+			}
+
+			if (!IS_ERR(bsp_priv->aclk_mac))
+				clk_prepare_enable(bsp_priv->aclk_mac);
+
+			if (!IS_ERR(bsp_priv->pclk_mac))
+				clk_prepare_enable(bsp_priv->pclk_mac);
+
+			if (!IS_ERR(bsp_priv->mac_clk_tx))
+				clk_prepare_enable(bsp_priv->mac_clk_tx);
+
+			/**
+			 * if (!IS_ERR(bsp_priv->clk_mac))
+			 *	clk_prepare_enable(bsp_priv->clk_mac);
+			 */
+			mdelay(5);
+			bsp_priv->clk_enabled = true;
+		}
+	} else {
+		if (bsp_priv->clk_enabled) {
+			if (phy_iface == PHY_INTERFACE_MODE_RMII) {
+				if (!IS_ERR(bsp_priv->mac_clk_rx))
+					clk_disable_unprepare(
+						bsp_priv->mac_clk_rx);
+
+				if (!IS_ERR(bsp_priv->clk_mac_ref))
+					clk_disable_unprepare(
+						bsp_priv->clk_mac_ref);
+
+				if (!IS_ERR(bsp_priv->clk_mac_refout))
+					clk_disable_unprepare(
+						bsp_priv->clk_mac_refout);
+			}
+
+			if (!IS_ERR(bsp_priv->aclk_mac))
+				clk_disable_unprepare(bsp_priv->aclk_mac);
+
+			if (!IS_ERR(bsp_priv->pclk_mac))
+				clk_disable_unprepare(bsp_priv->pclk_mac);
+
+			if (!IS_ERR(bsp_priv->mac_clk_tx))
+				clk_disable_unprepare(bsp_priv->mac_clk_tx);
+			/**
+			 * if (!IS_ERR(bsp_priv->clk_mac))
+			 *	clk_disable_unprepare(bsp_priv->clk_mac);
+			 */
+			bsp_priv->clk_enabled = false;
+		}
+	}
+
+	return 0;
+}
+
+static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
+{
+	struct regulator *ldo;
+	char *ldostr = bsp_priv->pmu_regulator;
+	int ret;
+
+	if (!ldostr) {
+		pr_err("%s: no ldo found\n", __func__);
+		return -1;
+	}
+
+	ldo = regulator_get(NULL, ldostr);
+	if (!ldo) {
+		pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
+	} else {
+		if (enable) {
+			if (!regulator_is_enabled(ldo)) {
+				regulator_set_voltage(ldo, 3300000, 3300000);
+				ret = regulator_enable(ldo);
+				if (ret != 0)
+					pr_err("%s: fail to enable %s\n",
+					       __func__, ldostr);
+				else
+					pr_info("turn on ldo done.\n");
+			} else {
+				pr_warn("%s is enabled before enable", ldostr);
+			}
+		} else {
+			if (regulator_is_enabled(ldo)) {
+				ret = regulator_disable(ldo);
+				if (ret != 0)
+					pr_err("%s: fail to disable %s\n",
+					       __func__, ldostr);
+				else
+					pr_info("turn off ldo done.\n");
+			} else {
+				pr_warn("%s is disabled before disable",
+					ldostr);
+			}
+		}
+		regulator_put(ldo);
+	}
+
+	return 0;
+}
+
+static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool enable)
+{
+	if (enable) {
+		/*power on*/
+		if (gpio_is_valid(bsp_priv->power_io))
+			gpio_direction_output(bsp_priv->power_io,
+					      bsp_priv->power_io_level);
+	} else {
+		/*power off*/
+		if (gpio_is_valid(bsp_priv->power_io))
+			gpio_direction_output(bsp_priv->power_io,
+					      !bsp_priv->power_io_level);
+	}
+
+	return 0;
+}
+
+static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
+{
+	int ret = -1;
+
+	pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
+
+	if (bsp_priv->power_ctrl_by_pmu)
+		ret = power_on_by_pmu(bsp_priv, enable);
+	else
+		ret =  power_on_by_gpio(bsp_priv, enable);
+
+	if (enable) {
+		/*reset*/
+		if (gpio_is_valid(bsp_priv->reset_io)) {
+			gpio_direction_output(bsp_priv->reset_io,
+					      bsp_priv->reset_io_level);
+			mdelay(5);
+			gpio_direction_output(bsp_priv->reset_io,
+					      !bsp_priv->reset_io_level);
+		}
+		mdelay(30);
+
+	} else {
+		/*pull down reset*/
+		if (gpio_is_valid(bsp_priv->reset_io)) {
+			gpio_direction_output(bsp_priv->reset_io,
+					      bsp_priv->reset_io_level);
+		}
+	}
+
+	return ret;
+}
+
+#define GPIO_PHY_POWER	"gmac_phy_power"
+#define GPIO_PHY_RESET	"gmac_phy_reset"
+#define GPIO_PHY_IRQ	"gmac_phy_irq"
+
+static void *rk_gmac_setup(struct platform_device *pdev)
+{
+	struct rk_priv_data *bsp_priv;
+	struct device *dev = &pdev->dev;
+	enum of_gpio_flags flags;
+	int ret;
+	const char *strings = NULL;
+	int value;
+	int irq;
+
+	bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
+	if (!bsp_priv)
+		return ERR_PTR(-ENOMEM);
+
+	bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
+
+	ret = of_property_read_string(dev->of_node, "pmu_regulator", &strings);
+	if (ret) {
+		pr_err("%s: Can not read property: pmu_regulator.\n", __func__);
+		bsp_priv->power_ctrl_by_pmu = false;
+	} else {
+		pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
+			__func__, strings);
+		bsp_priv->power_ctrl_by_pmu = true;
+		strcpy(bsp_priv->pmu_regulator, strings);
+	}
+
+	ret = of_property_read_u32(dev->of_node, "pmu_enable_level", &value);
+	if (ret) {
+		pr_err("%s: Can not read property: pmu_enable_level.\n",
+		       __func__);
+		bsp_priv->power_ctrl_by_pmu = false;
+	} else {
+		pr_info("%s: PHY power controlled by pmu(level = %s).\n",
+			__func__, (value == 1) ? "HIGH" : "LOW");
+		bsp_priv->power_ctrl_by_pmu = true;
+		bsp_priv->pmu_enable_level = value;
+	}
+
+	ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);
+	if (ret) {
+		pr_err("%s: Can not read property: clock_in_out.\n", __func__);
+		bsp_priv->clock_input = true;
+	} else {
+		pr_info("%s: clock input or output? (%s).\n",
+			__func__, strings);
+		if (!strcmp(strings, "input"))
+			bsp_priv->clock_input = true;
+		else
+			bsp_priv->clock_input = false;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
+	if (ret) {
+		bsp_priv->tx_delay = 0x30;
+		pr_err("%s: Can not read property: tx_delay.", __func__);
+		pr_err("%s: set tx_delay to 0x%x\n",
+		       __func__, bsp_priv->tx_delay);
+	} else {
+		pr_info("%s: TX delay(0x%x).\n", __func__, value);
+		bsp_priv->tx_delay = value;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
+	if (ret) {
+		bsp_priv->rx_delay = 0x10;
+		pr_err("%s: Can not read property: rx_delay.", __func__);
+		pr_err("%s: set rx_delay to 0x%x\n",
+		       __func__, bsp_priv->rx_delay);
+	} else {
+		pr_info("%s: RX delay(0x%x).\n", __func__, value);
+		bsp_priv->rx_delay = value;
+	}
+
+	bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
+							"rockchip,grf");
+	bsp_priv->phyirq_io =
+		of_get_named_gpio_flags(dev->of_node,
+					"phyirq-gpio", 0, &flags);
+	bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+	bsp_priv->reset_io =
+		of_get_named_gpio_flags(dev->of_node,
+					"reset-gpio", 0, &flags);
+	bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+	bsp_priv->power_io =
+		of_get_named_gpio_flags(dev->of_node, "power-gpio", 0, &flags);
+	bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+	/*power*/
+	if (!gpio_is_valid(bsp_priv->power_io)) {
+		pr_err("%s: Failed to get GPIO %s.\n",
+		       __func__, "power-gpio");
+	} else {
+		ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
+		if (ret)
+			pr_err("%s: ERROR: Failed to request GPIO %s.\n",
+			       __func__, GPIO_PHY_POWER);
+	}
+
+	if (!gpio_is_valid(bsp_priv->reset_io)) {
+		pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
+	} else {
+		ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
+		if (ret)
+			pr_err("%s: ERROR: Failed to request GPIO %s.\n",
+			       __func__, GPIO_PHY_RESET);
+	}
+
+	if (bsp_priv->phyirq_io > 0) {
+		struct plat_stmmacenet_data *plat_dat;
+
+		pr_info("PHY irq in use\n");
+		ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
+		if (ret < 0) {
+			pr_warn("%s: Failed to request GPIO %s\n",
+				__func__, GPIO_PHY_IRQ);
+			goto goon;
+		}
+
+		ret = gpio_direction_input(bsp_priv->phyirq_io);
+		if (ret < 0) {
+			pr_err("%s, Failed to set input for GPIO %s\n",
+			       __func__, GPIO_PHY_IRQ);
+			gpio_free(bsp_priv->phyirq_io);
+			goto goon;
+		}
+
+		irq = gpio_to_irq(bsp_priv->phyirq_io);
+		if (irq < 0) {
+			ret = irq;
+			pr_err("Failed to set irq for %s\n",
+			       GPIO_PHY_IRQ);
+			gpio_free(bsp_priv->phyirq_io);
+			goto goon;
+		}
+
+		plat_dat = dev_get_platdata(&pdev->dev);
+		if (plat_dat)
+			plat_dat->mdio_bus_data->probed_phy_irq = irq;
+		else
+			pr_err("%s: plat_data is NULL\n", __func__);
+	}
+
+goon:
+	/*rmii or rgmii*/
+	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
+		pr_info("%s: init for RGMII\n", __func__);
+		set_to_rgmii(bsp_priv, bsp_priv->tx_delay, bsp_priv->rx_delay);
+	} else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
+		pr_info("%s: init for RMII\n", __func__);
+		set_to_rmii(bsp_priv);
+	} else {
+		pr_err("%s: ERROR: NO interface defined!\n", __func__);
+	}
+
+	bsp_priv->pdev = pdev;
+
+	gmac_clk_init(bsp_priv);
+
+	return bsp_priv;
+}
+
+static int rk_gmac_init(struct platform_device *pdev, void *priv)
+{
+	struct rk_priv_data *bsp_priv = priv;
+	int ret;
+
+	ret = phy_power_on(bsp_priv, true);
+	if (ret)
+		return ret;
+
+	ret = gmac_clk_enable(bsp_priv, true);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void rk_gmac_exit(struct platform_device *pdev, void *priv)
+{
+	struct rk_priv_data *gmac = priv;
+
+	phy_power_on(gmac, false);
+	gmac_clk_enable(gmac, false);
+}
+
+static void rk_fix_speed(void *priv, unsigned int speed)
+{
+	struct rk_priv_data *bsp_priv = priv;
+
+	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
+		set_rgmii_speed(bsp_priv, speed);
+	else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
+		set_rmii_speed(bsp_priv, speed);
+	else
+		pr_err("unsupported interface %d", bsp_priv->phy_iface);
+}
+
+const struct stmmac_of_data rk_gmac_data = {
+	.has_gmac = 1,
+	.fix_mac_speed = rk_fix_speed,
+	.setup = rk_gmac_setup,
+	.init = rk_gmac_init,
+	.exit = rk_gmac_exit,
+};
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 15814b7..b4dee96 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -33,6 +33,7 @@
 
 static const struct of_device_id stmmac_dt_ids[] = {
 	/* SoC specific glue layers should come before generic bindings */
+	{ .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
 	{ .compatible = "amlogic,meson6-dwmac", .data = &meson6_dwmac_data},
 	{ .compatible = "allwinner,sun7i-a20-gmac", .data = &sun7i_gmac_data},
 	{ .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
@@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct platform_device *pdev)
 			return  -ENOMEM;
 		}
 
+		pdev->dev.platform_data = plat_dat;
+
 		ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
 		if (ret) {
 			pr_err("%s: main dt probe failed", __func__);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
index 25dd1f7..32a0516 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
@@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
 extern const struct stmmac_of_data stih4xx_dwmac_data;
 extern const struct stmmac_of_data stid127_dwmac_data;
 extern const struct stmmac_of_data socfpga_gmac_data;
+extern const struct stmmac_of_data rk_gmac_data;
 
 #endif /* __STMMAC_PLATFORM_H__ */
-- 
1.7.9.5

Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC

[Heiko Stübner]

Hi Roger,

the comments inline are a rough first review. I hope to get a clearer picture 
for the stuff I'm not sure about in v3 once the big issues are fixed.


Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
> This driver is based on stmmac driver.
> 
> modification based on Giuseppe CAVALLARO's suggestion:
> 1. use BIT()
> 
> 	> +/*RK3288_GRF_SOC_CON3*/
> 	> +#define GMAC_TXCLK_DLY_ENABLE   ((0x4000 << 16) | (0x4000))
> 	> +#define GMAC_TXCLK_DLY_DISABLE  ((0x4000 << 16) | (0x0000))
> 
> 	...
> 
> 	why do not use BIT and BIT_MASK where possible?
> 
> 	===>after modification:
> 
> 	#define GRF_BIT(nr)     (BIT(nr) | BIT(nr+16))
> 	#define GRF_CLR_BIT(nr) (BIT(nr+16))
> 	#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
> 	#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
> 	...
> 2.
> 
> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> 	> +             GMAC_PHY_INTF_SEL_RGMII);
> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> 	> +             GMAC_RMII_MODE_CLR);
> 
> 	maybe you could perform just one write unless there is some HW
> 	constraint.
> 
> 	===>after modification:
> 
> 	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> 			 GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
> 
> 3. use macros
> 
> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, 0xFFFFFFFF);
> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
> 	> +             0x3<<2<<16 | 0x3<<2);
> 
> 	pls use macros, these shift sequence is really help to decode
> 
> 	===>after modification:
> 
> 	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
> 	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
> 
> 4. remove grf fail check in rk_gmac_setup()
> 
> 	> +    if (IS_ERR(bsp_priv->grf))
> 	> +        dev_err(&pdev->dev, "Missing rockchip,grf property\n");
> 
> 	I wonder if you can fail on here and save all the check in
> 	set_rgmii_speed etc.
> 	Maybe this can be considered a mandatory property for the glue-logic.
> 
> 5. remove .tx_coe=1
> 
> 	> +const struct stmmac_of_data rk_gmac_data = {
> 	> +    .has_gmac = 1,
> 	> +    .tx_coe = 1,
> 
> 	FYI, on new gmac there is the HW capability register to dinamically
> 	provide you if coe is supported.
> 
> 	IMO you should add the OF "compatible" string and in case of mac
> 	newer than the 3.50a you can remove coe.

changelogs like these, should be compact and also not be in the commit message 
itself, but in the "comment"-section below the "---" and before the diffstat.


> 
> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
> ---

changelog here ... the commonly used pattern is something like

changes since v2:
- ...
- ...

changes since v1:
- ...

>  drivers/net/ethernet/stmicro/stmmac/Makefile       |    2 +-
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  636
> ++++++++++++++++++++ .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  
>  3 +
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |    1 +
>  4 files changed, 641 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
> 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o
> ring_mode.o	\
> 
>  obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>  stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o	\
> -		       dwmac-sti.o dwmac-socfpga.o
> +		       dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
> 
>  obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>  stmmac-pci-objs:= stmmac_pci.o
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
> index 0000000..870563f
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -0,0 +1,636 @@
> +/**
> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
> + *
> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
> + *
> + * Chen-Zhi (Roger Chen)  <roger.chen@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/stmmac.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/phy.h>
> +#include <linux/of_net.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +struct rk_priv_data {
> +	struct platform_device *pdev;
> +	int phy_iface;
> +	bool power_ctrl_by_pmu;
> +	char pmu_regulator[32];
> +	int pmu_enable_level;
> +
> +	int power_io;
> +	int power_io_level;
> +	int reset_io;
> +	int reset_io_level;
> +	int phyirq_io;
> +	int phyirq_io_level;
> +
> +	bool clk_enabled;
> +	bool clock_input;
> +
> +	struct clk *clk_mac;
> +	struct clk *clk_mac_pll;
> +	struct clk *gmac_clkin;
> +	struct clk *mac_clk_rx;
> +	struct clk *mac_clk_tx;
> +	struct clk *clk_mac_ref;
> +	struct clk *clk_mac_refout;
> +	struct clk *aclk_mac;
> +	struct clk *pclk_mac;
> +
> +	int tx_delay;
> +	int rx_delay;
> +
> +	struct regmap *grf;
> +};
> +
> +#define RK3288_GRF_SOC_CON1 0x0248
> +#define RK3288_GRF_SOC_CON3 0x0250
> +#define RK3288_GRF_GPIO3D_E 0x01ec
> +#define RK3288_GRF_GPIO4A_E 0x01f0
> +#define RK3288_GRF_GPIO4B_E 0x01f4

here you're using a space instead of a tab, please select one pattern either 
tabs or space but do not mix them.


> +#define GPIO3D_2MA	0xFFFF0000
> +#define GPIO3D_4MA	0xFFFF5555
> +#define GPIO3D_8MA	0xFFFFAAAA
> +#define GPIO3D_12MA	0xFFFFFFFF
> +
> +#define GPIO4A_2MA	0xFFFF0000
> +#define GPIO4A_4MA	0xFFFF5555
> +#define GPIO4A_8MA	0xFFFFAAAA
> +#define GPIO4A_12MA	0xFFFFFFFF

see comment about pin settings below


> +
> +#define GRF_BIT(nr)	(BIT(nr) | BIT(nr+16))
> +#define GRF_CLR_BIT(nr)	(BIT(nr+16))
> +
> +#define GPIO4B_2_2MA	(GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
> +#define GPIO4B_2_4MA	(GRF_BIT(2) | GRF_CLR_BIT(3))
> +#define GPIO4B_2_8MA	(GRF_CLR_BIT(2) | GRF_BIT(3))
> +#define GPIO4B_2_12MA	(GRF_BIT(2) | GRF_BIT(3))
> +
> +/*RK3288_GRF_SOC_CON1*/
> +#define GMAC_PHY_INTF_SEL_RGMII	(GRF_BIT(6) | GRF_CLR_BIT(7) |
> GRF_CLR_BIT(8)) 
> +#define GMAC_PHY_INTF_SEL_RMII  (GRF_CLR_BIT(6) |
> GRF_CLR_BIT(7) | GRF_BIT(8)) 
> +#define GMAC_FLOW_CTRL          GRF_BIT(9)
> +#define GMAC_FLOW_CTRL_CLR      GRF_CLR_BIT(9)
> +#define GMAC_SPEED_10M          GRF_CLR_BIT(10)
> +#define GMAC_SPEED_100M         GRF_BIT(10)
> +#define GMAC_RMII_CLK_25M       GRF_BIT(11)
> +#define GMAC_RMII_CLK_2_5M      GRF_CLR_BIT(11)
> +#define GMAC_CLK_125M           (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
> +#define GMAC_CLK_25M            (GRF_BIT(12) | GRF_BIT(13))
> +#define GMAC_CLK_2_5M           (GRF_CLR_BIT(12) | GRF_BIT(13))
> +#define GMAC_RMII_MODE          GRF_BIT(14)
> +#define GMAC_RMII_MODE_CLR      GRF_CLR_BIT(14)
> +
> +/*RK3288_GRF_SOC_CON3*/
> +#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
> +#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
> +#define GMAC_RXCLK_DLY_ENABLE   GRF_BIT(15)
> +#define GMAC_RXCLK_DLY_DISABLE  GRF_CLR_BIT(15)
> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))

again mixed tabs and spaces as delimiters.

Also the _CFG macros are not well abstracted. You could take a look at the 
HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:

#define GMAC_CLK_DL_MASK	0x7f
#define GMAC_CLK_RX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)  
#define GMAC_CLK_TX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)  


> +
> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
> +			 int tx_delay, int rx_delay)
> +{
> +	if (IS_ERR(bsp_priv->grf)) {
> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
> +		return;
> +	}
> +
> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +		     GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
> +		     GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
> +		     GMAC_CLK_RX_DL_CFG(rx_delay) |
> +		     GMAC_CLK_TX_DL_CFG(tx_delay));
> +	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
> +	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
> +	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);

please don't write to parts controlled by other drivers - here the drive 
strength settings of pins is controlled by the pinctrl driver. Instead you can 
just set the drive-strength in the pinctrl settings.


> +
> +	pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
> +		 __func__, tx_delay, rx_delay);
> +}
> +
> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
> +{
> +	if (IS_ERR(bsp_priv->grf)) {
> +		pr_err("%s: Missing rockchip,grf property\n", __func__);

you have a device-reference in rk_priv_data, so you could use dev_err here. 
Same for all other pr_err/pr_debug/pr_* calls in this file.


> +		return;
> +	}
> +
> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +		     GMAC_PHY_INTF_SEL_RMII);
> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +		     GMAC_RMII_MODE);

these two could be combined?


> +}
> +
> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
> +{
> +	if (IS_ERR(bsp_priv->grf)) {
> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
> +		return;
> +	}
> +
> +	if (speed == 10)
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_2_5M);
> +	else if (speed == 100)
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_25M);
> +	else if (speed == 1000)
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_125M);
> +	else
> +		pr_err("unknown speed value for RGMII! speed=%d", speed);
> +}
> +
> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
> +{
> +	if (IS_ERR(bsp_priv->grf)) {
> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
> +		return;
> +	}
> +
> +	if (speed == 10) {
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +			     GMAC_RMII_CLK_2_5M);
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +			     GMAC_SPEED_10M);

combine into one write?


> +	} else if (speed == 100) {
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +			     GMAC_RMII_CLK_25M);
> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> +			     GMAC_SPEED_100M);

combine into one write?


> +	} else {
> +		pr_err("unknown speed value for RMII! speed=%d", speed);
> +	}
> +}
> +
> +#define MAC_CLK_RX	"mac_clk_rx"
> +#define MAC_CLK_TX	"mac_clk_tx"
> +#define CLK_MAC_REF	"clk_mac_ref"
> +#define CLK_MAC_REF_OUT	"clk_mac_refout"
> +#define CLK_MAC_PLL	"clk_mac_pll"
> +#define ACLK_MAC	"aclk_mac"
> +#define PCLK_MAC	"pclk_mac"
> +#define MAC_CLKIN	"ext_gmac"
> +#define CLK_MAC		"stmmaceth"

why the need to extra constants for the clock names and not use the real names 
directly like most other drivers do?


> +
> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
> +{
> +	struct device *dev = &bsp_priv->pdev->dev;
> +
> +	bsp_priv->clk_enabled = false;
> +
> +	bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
> +	if (IS_ERR(bsp_priv->mac_clk_rx))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, MAC_CLK_RX);
> +
> +	bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
> +	if (IS_ERR(bsp_priv->mac_clk_tx))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, MAC_CLK_TX);
> +
> +	bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
> +	if (IS_ERR(bsp_priv->clk_mac_ref))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, CLK_MAC_REF);
> +
> +	bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
> +	if (IS_ERR(bsp_priv->clk_mac_refout))
> +		pr_warn("%s: warning:cannot get clock %s\n",
> +			__func__, CLK_MAC_REF_OUT);
> +
> +	bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
> +	if (IS_ERR(bsp_priv->aclk_mac))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, ACLK_MAC);
> +
> +	bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
> +	if (IS_ERR(bsp_priv->pclk_mac))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, PCLK_MAC);
> +
> +	bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
> +	if (IS_ERR(bsp_priv->clk_mac_pll))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, CLK_MAC_PLL);
> +
> +	bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
> +	if (IS_ERR(bsp_priv->gmac_clkin))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, MAC_CLKIN);
> +
> +	bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
> +	if (IS_ERR(bsp_priv->clk_mac))
> +		pr_warn("%s: warning: cannot get clock %s\n",
> +			__func__, CLK_MAC);

there is not clk_put in the _remove case ... maybe you could simply use 
devm_clk_get here so that all clocks are put on device removal.

Also you're warning on every missing clock. Below it looks like you need a 
different set of them for rgmii and rmii, so maybe you should simply error out 
when core clocks for the selected phy-mode are missing.


> +
> +	if (bsp_priv->clock_input) {
> +		pr_info("%s: clock input from PHY\n", __func__);
> +	} else {
> +		if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
> +			clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
> +
> +		clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);

why the explicit reparenting. The common clock-framework is intelligent enough 
to select the best suitable parent.

In general I'm thinking the clock-handling inside this driver should be 
simplyfied, as the common-clock framework can handle most cases itself. I.e. if 
a 125MHz external clock is present and so on. But haven't looked to deep yet.



> +	}
> +
> +	return 0;
> +}
> +
> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
> +{
> +	int phy_iface = phy_iface = bsp_priv->phy_iface;
> +
> +	if (enable) {
> +		if (!bsp_priv->clk_enabled) {
> +			if (phy_iface == PHY_INTERFACE_MODE_RMII) {
> +				if (!IS_ERR(bsp_priv->mac_clk_rx))
> +					clk_prepare_enable(
> +						bsp_priv->mac_clk_rx);
> +
> +				if (!IS_ERR(bsp_priv->clk_mac_ref))
> +					clk_prepare_enable(
> +						bsp_priv->clk_mac_ref);
> +
> +				if (!IS_ERR(bsp_priv->clk_mac_refout))
> +					clk_prepare_enable(
> +						bsp_priv->clk_mac_refout);
> +			}
> +
> +			if (!IS_ERR(bsp_priv->aclk_mac))
> +				clk_prepare_enable(bsp_priv->aclk_mac);
> +
> +			if (!IS_ERR(bsp_priv->pclk_mac))
> +				clk_prepare_enable(bsp_priv->pclk_mac);
> +
> +			if (!IS_ERR(bsp_priv->mac_clk_tx))
> +				clk_prepare_enable(bsp_priv->mac_clk_tx);
> +
> +			/**
> +			 * if (!IS_ERR(bsp_priv->clk_mac))
> +			 *	clk_prepare_enable(bsp_priv->clk_mac);
> +			 */
> +			mdelay(5);
> +			bsp_priv->clk_enabled = true;
> +		}
> +	} else {
> +		if (bsp_priv->clk_enabled) {
> +			if (phy_iface == PHY_INTERFACE_MODE_RMII) {
> +				if (!IS_ERR(bsp_priv->mac_clk_rx))
> +					clk_disable_unprepare(
> +						bsp_priv->mac_clk_rx);
> +
> +				if (!IS_ERR(bsp_priv->clk_mac_ref))
> +					clk_disable_unprepare(
> +						bsp_priv->clk_mac_ref);
> +
> +				if (!IS_ERR(bsp_priv->clk_mac_refout))
> +					clk_disable_unprepare(
> +						bsp_priv->clk_mac_refout);
> +			}
> +
> +			if (!IS_ERR(bsp_priv->aclk_mac))
> +				clk_disable_unprepare(bsp_priv->aclk_mac);
> +
> +			if (!IS_ERR(bsp_priv->pclk_mac))
> +				clk_disable_unprepare(bsp_priv->pclk_mac);
> +
> +			if (!IS_ERR(bsp_priv->mac_clk_tx))
> +				clk_disable_unprepare(bsp_priv->mac_clk_tx);
> +			/**
> +			 * if (!IS_ERR(bsp_priv->clk_mac))
> +			 *	clk_disable_unprepare(bsp_priv->clk_mac);
> +			 */
> +			bsp_priv->clk_enabled = false;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
> +{
> +	struct regulator *ldo;
> +	char *ldostr = bsp_priv->pmu_regulator;
> +	int ret;
> +
> +	if (!ldostr) {
> +		pr_err("%s: no ldo found\n", __func__);
> +		return -1;
> +	}
> +
> +	ldo = regulator_get(NULL, ldostr);
> +	if (!ldo) {
> +		pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
> +	} else {
> +		if (enable) {
> +			if (!regulator_is_enabled(ldo)) {
> +				regulator_set_voltage(ldo, 3300000, 3300000);
> +				ret = regulator_enable(ldo);
> +				if (ret != 0)
> +					pr_err("%s: fail to enable %s\n",
> +					       __func__, ldostr);
> +				else
> +					pr_info("turn on ldo done.\n");
> +			} else {
> +				pr_warn("%s is enabled before enable", ldostr);
> +			}
> +		} else {
> +			if (regulator_is_enabled(ldo)) {
> +				ret = regulator_disable(ldo);
> +				if (ret != 0)
> +					pr_err("%s: fail to disable %s\n",
> +					       __func__, ldostr);
> +				else
> +					pr_info("turn off ldo done.\n");
> +			} else {
> +				pr_warn("%s is disabled before disable",
> +					ldostr);
> +			}
> +		}
> +		regulator_put(ldo);
> +	}
> +
> +	return 0;
> +}
> +
> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool enable)
> +{
> +	if (enable) {
> +		/*power on*/
> +		if (gpio_is_valid(bsp_priv->power_io))
> +			gpio_direction_output(bsp_priv->power_io,
> +					      bsp_priv->power_io_level);
> +	} else {
> +		/*power off*/
> +		if (gpio_is_valid(bsp_priv->power_io))
> +			gpio_direction_output(bsp_priv->power_io,
> +					      !bsp_priv->power_io_level);
> +	}
> +
> +	return 0;
> +}
> +
> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> +{
> +	int ret = -1;
> +
> +	pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
> +
> +	if (bsp_priv->power_ctrl_by_pmu)
> +		ret = power_on_by_pmu(bsp_priv, enable);
> +	else
> +		ret =  power_on_by_gpio(bsp_priv, enable);

this looks wrong. This should always be a regulator. Even a regulator + switch 
controlled by a gpio can still be modelled as regulator, so that you don't 
need this switch and assorted special handling - so just use the regulator API


> +
> +	if (enable) {
> +		/*reset*/
> +		if (gpio_is_valid(bsp_priv->reset_io)) {
> +			gpio_direction_output(bsp_priv->reset_io,
> +					      bsp_priv->reset_io_level);
> +			mdelay(5);
> +			gpio_direction_output(bsp_priv->reset_io,
> +					      !bsp_priv->reset_io_level);
> +		}
> +		mdelay(30);
> +
> +	} else {
> +		/*pull down reset*/
> +		if (gpio_is_valid(bsp_priv->reset_io)) {
> +			gpio_direction_output(bsp_priv->reset_io,
> +					      bsp_priv->reset_io_level);
> +		}
> +	}

I'm not sure yet if it would be better to use the reset framework for this. 
While it says it is also meant for reset-gpios, there does not seem a driver 
for this to exist yet.



> +
> +	return ret;
> +}
> +
> +#define GPIO_PHY_POWER	"gmac_phy_power"
> +#define GPIO_PHY_RESET	"gmac_phy_reset"
> +#define GPIO_PHY_IRQ	"gmac_phy_irq"

again I don't understand why these constants are necessary

> +
> +static void *rk_gmac_setup(struct platform_device *pdev)
> +{
> +	struct rk_priv_data *bsp_priv;
> +	struct device *dev = &pdev->dev;
> +	enum of_gpio_flags flags;
> +	int ret;
> +	const char *strings = NULL;
> +	int value;
> +	int irq;
> +
> +	bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
> +	if (!bsp_priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
> +
> +	ret = of_property_read_string(dev->of_node, "pmu_regulator", &strings);
> +	if (ret) {
> +		pr_err("%s: Can not read property: pmu_regulator.\n", __func__);
> +		bsp_priv->power_ctrl_by_pmu = false;
> +	} else {
> +		pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
> +			__func__, strings);
> +		bsp_priv->power_ctrl_by_pmu = true;
> +		strcpy(bsp_priv->pmu_regulator, strings);
> +	}

There is a generic regulator-dt-binding for regulator-consumers available 
which you should of course use.


> +
> +	ret = of_property_read_u32(dev->of_node, "pmu_enable_level", &value);
> +	if (ret) {
> +		pr_err("%s: Can not read property: pmu_enable_level.\n",
> +		       __func__);
> +		bsp_priv->power_ctrl_by_pmu = false;
> +	} else {
> +		pr_info("%s: PHY power controlled by pmu(level = %s).\n",
> +			__func__, (value == 1) ? "HIGH" : "LOW");
> +		bsp_priv->power_ctrl_by_pmu = true;
> +		bsp_priv->pmu_enable_level = value;
> +	}

What is this used for? Enabling should of course be done via regulator_enable 
and disabling using regulator_disable.


> +
> +	ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);
> +	if (ret) {
> +		pr_err("%s: Can not read property: clock_in_out.\n", __func__);
> +		bsp_priv->clock_input = true;
> +	} else {
> +		pr_info("%s: clock input or output? (%s).\n",
> +			__func__, strings);
> +		if (!strcmp(strings, "input"))
> +			bsp_priv->clock_input = true;
> +		else
> +			bsp_priv->clock_input = false;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
> +	if (ret) {
> +		bsp_priv->tx_delay = 0x30;
> +		pr_err("%s: Can not read property: tx_delay.", __func__);
> +		pr_err("%s: set tx_delay to 0x%x\n",
> +		       __func__, bsp_priv->tx_delay);
> +	} else {
> +		pr_info("%s: TX delay(0x%x).\n", __func__, value);
> +		bsp_priv->tx_delay = value;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
> +	if (ret) {
> +		bsp_priv->rx_delay = 0x10;
> +		pr_err("%s: Can not read property: rx_delay.", __func__);
> +		pr_err("%s: set rx_delay to 0x%x\n",
> +		       __func__, bsp_priv->rx_delay);
> +	} else {
> +		pr_info("%s: RX delay(0x%x).\n", __func__, value);
> +		bsp_priv->rx_delay = value;
> +	}
> +
> +	bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							"rockchip,grf");
> +	bsp_priv->phyirq_io =
> +		of_get_named_gpio_flags(dev->of_node,
> +					"phyirq-gpio", 0, &flags);
> +	bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> +
> +	bsp_priv->reset_io =
> +		of_get_named_gpio_flags(dev->of_node,
> +					"reset-gpio", 0, &flags);
> +	bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> +
> +	bsp_priv->power_io =
> +		of_get_named_gpio_flags(dev->of_node, "power-gpio", 0, &flags);
> +	bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> +
> +	/*power*/
> +	if (!gpio_is_valid(bsp_priv->power_io)) {
> +		pr_err("%s: Failed to get GPIO %s.\n",
> +		       __func__, "power-gpio");
> +	} else {
> +		ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
> +		if (ret)
> +			pr_err("%s: ERROR: Failed to request GPIO %s.\n",
> +			       __func__, GPIO_PHY_POWER);
> +	}

When everything power-related is handled using the regulator api, you don't 
need this


> +
> +	if (!gpio_is_valid(bsp_priv->reset_io)) {
> +		pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
> +	} else {
> +		ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
> +		if (ret)
> +			pr_err("%s: ERROR: Failed to request GPIO %s.\n",
> +			       __func__, GPIO_PHY_RESET);
> +	}
> +
> +	if (bsp_priv->phyirq_io > 0) {

This is more for my understanding: why does the mac driver need to handle the 
phy interrupt - but I might be overlooking something.


> +		struct plat_stmmacenet_data *plat_dat;
> +
> +		pr_info("PHY irq in use\n");
> +		ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
> +		if (ret < 0) {
> +			pr_warn("%s: Failed to request GPIO %s\n",
> +				__func__, GPIO_PHY_IRQ);
> +			goto goon;
> +		}
> +
> +		ret = gpio_direction_input(bsp_priv->phyirq_io);
> +		if (ret < 0) {
> +			pr_err("%s, Failed to set input for GPIO %s\n",
> +			       __func__, GPIO_PHY_IRQ);
> +			gpio_free(bsp_priv->phyirq_io);
> +			goto goon;
> +		}
> +
> +		irq = gpio_to_irq(bsp_priv->phyirq_io);
> +		if (irq < 0) {
> +			ret = irq;
> +			pr_err("Failed to set irq for %s\n",
> +			       GPIO_PHY_IRQ);
> +			gpio_free(bsp_priv->phyirq_io);
> +			goto goon;
> +		}
> +
> +		plat_dat = dev_get_platdata(&pdev->dev);
> +		if (plat_dat)
> +			plat_dat->mdio_bus_data->probed_phy_irq = irq;
> +		else
> +			pr_err("%s: plat_data is NULL\n", __func__);
> +	}
> +
> +goon:
> +	/*rmii or rgmii*/
> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
> +		pr_info("%s: init for RGMII\n", __func__);
> +		set_to_rgmii(bsp_priv, bsp_priv->tx_delay, bsp_priv->rx_delay);
> +	} else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
> +		pr_info("%s: init for RMII\n", __func__);
> +		set_to_rmii(bsp_priv);
> +	} else {
> +		pr_err("%s: ERROR: NO interface defined!\n", __func__);
> +	}
> +
> +	bsp_priv->pdev = pdev;
> +
> +	gmac_clk_init(bsp_priv);
> +
> +	return bsp_priv;
> +}
> +
> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct rk_priv_data *bsp_priv = priv;
> +	int ret;
> +
> +	ret = phy_power_on(bsp_priv, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = gmac_clk_enable(bsp_priv, true);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
> +{
> +	struct rk_priv_data *gmac = priv;
> +
> +	phy_power_on(gmac, false);
> +	gmac_clk_enable(gmac, false);
> +}
> +
> +static void rk_fix_speed(void *priv, unsigned int speed)
> +{
> +	struct rk_priv_data *bsp_priv = priv;
> +
> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
> +		set_rgmii_speed(bsp_priv, speed);
> +	else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
> +		set_rmii_speed(bsp_priv, speed);
> +	else
> +		pr_err("unsupported interface %d", bsp_priv->phy_iface);
> +}
> +
> +const struct stmmac_of_data rk_gmac_data = {
> +	.has_gmac = 1,
> +	.fix_mac_speed = rk_fix_speed,
> +	.setup = rk_gmac_setup,
> +	.init = rk_gmac_init,
> +	.exit = rk_gmac_exit,
> +};
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
> 15814b7..b4dee96 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -33,6 +33,7 @@
> 
>  static const struct of_device_id stmmac_dt_ids[] = {
>  	/* SoC specific glue layers should come before generic bindings */
> +	{ .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},

please name that rk3288_gmac_data [of course the other occurences too] 
It makes it easier to see which soc it is meant for and it's also not save to 
assume that the next one will use the same register + bit positions in the 
grf.


>  	{ .compatible = "amlogic,meson6-dwmac", .data = &meson6_dwmac_data},
>  	{ .compatible = "allwinner,sun7i-a20-gmac", .data = &sun7i_gmac_data},
>  	{ .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct platform_device
> *pdev) return  -ENOMEM;
>  		}
> 
> +		pdev->dev.platform_data = plat_dat;
> +
>  		ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>  		if (ret) {
>  			pr_err("%s: main dt probe failed", __func__);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
> 25dd1f7..32a0516 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>  extern const struct stmmac_of_data stih4xx_dwmac_data;
>  extern const struct stmmac_of_data stid127_dwmac_data;
>  extern const struct stmmac_of_data socfpga_gmac_data;
> +extern const struct stmmac_of_data rk_gmac_data;
> 
>  #endif /* __STMMAC_PLATFORM_H__ */

Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC

[Roger]

Hi! Heiko

about regulator, power gpio,  reset gpio and irq gpio
please refer to my comment inline, tks.

On 2014/12/2 7:44, Heiko Stübner wrote:
> Hi Roger,
>
> the comments inline are a rough first review. I hope to get a clearer picture
> for the stuff I'm not sure about in v3 once the big issues are fixed.
>
>
> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
>> This driver is based on stmmac driver.
>>
>> modification based on Giuseppe CAVALLARO's suggestion:
>> 1. use BIT()
>>
>> 	> +/*RK3288_GRF_SOC_CON3*/
>> 	> +#define GMAC_TXCLK_DLY_ENABLE   ((0x4000 << 16) | (0x4000))
>> 	> +#define GMAC_TXCLK_DLY_DISABLE  ((0x4000 << 16) | (0x0000))
>>
>> 	...
>>
>> 	why do not use BIT and BIT_MASK where possible?
>>
>> 	===>after modification:
>>
>> 	#define GRF_BIT(nr)     (BIT(nr) | BIT(nr+16))
>> 	#define GRF_CLR_BIT(nr) (BIT(nr+16))
>> 	#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
>> 	#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
>> 	...
>> 2.
>>
>> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> 	> +             GMAC_PHY_INTF_SEL_RGMII);
>> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> 	> +             GMAC_RMII_MODE_CLR);
>>
>> 	maybe you could perform just one write unless there is some HW
>> 	constraint.
>>
>> 	===>after modification:
>>
>> 	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> 			 GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>
>> 3. use macros
>>
>> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, 0xFFFFFFFF);
>> 	> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>> 	> +             0x3<<2<<16 | 0x3<<2);
>>
>> 	pls use macros, these shift sequence is really help to decode
>>
>> 	===>after modification:
>>
>> 	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>> 	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>
>> 4. remove grf fail check in rk_gmac_setup()
>>
>> 	> +    if (IS_ERR(bsp_priv->grf))
>> 	> +        dev_err(&pdev->dev, "Missing rockchip,grf property\n");
>>
>> 	I wonder if you can fail on here and save all the check in
>> 	set_rgmii_speed etc.
>> 	Maybe this can be considered a mandatory property for the glue-logic.
>>
>> 5. remove .tx_coe=1
>>
>> 	> +const struct stmmac_of_data rk_gmac_data = {
>> 	> +    .has_gmac = 1,
>> 	> +    .tx_coe = 1,
>>
>> 	FYI, on new gmac there is the HW capability register to dinamically
>> 	provide you if coe is supported.
>>
>> 	IMO you should add the OF "compatible" string and in case of mac
>> 	newer than the 3.50a you can remove coe.
> changelogs like these, should be compact and also not be in the commit message
> itself, but in the "comment"-section below the "---" and before the diffstat.
>
>
>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
>> ---
> changelog here ... the commonly used pattern is something like
>
> changes since v2:
> - ...
> - ...
>
> changes since v1:
> - ...
>
>>   drivers/net/ethernet/stmicro/stmmac/Makefile       |    2 +-
>>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  636
>> ++++++++++++++++++++ .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |
>>   3 +
>>   .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |    1 +
>>   4 files changed, 641 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
>> 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o
>> ring_mode.o	\
>>
>>   obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>   stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o	\
>> -		       dwmac-sti.o dwmac-socfpga.o
>> +		       dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>
>>   obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>   stmmac-pci-objs:= stmmac_pci.o
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
>> index 0000000..870563f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> @@ -0,0 +1,636 @@
>> +/**
>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>> + *
>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>> + *
>> + * Chen-Zhi (Roger Chen)  <roger.chen@rock-chips.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/stmmac.h>
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/phy.h>
>> +#include <linux/of_net.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/delay.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +
>> +struct rk_priv_data {
>> +	struct platform_device *pdev;
>> +	int phy_iface;
>> +	bool power_ctrl_by_pmu;
>> +	char pmu_regulator[32];
>> +	int pmu_enable_level;
>> +
>> +	int power_io;
>> +	int power_io_level;
>> +	int reset_io;
>> +	int reset_io_level;
>> +	int phyirq_io;
>> +	int phyirq_io_level;
>> +
>> +	bool clk_enabled;
>> +	bool clock_input;
>> +
>> +	struct clk *clk_mac;
>> +	struct clk *clk_mac_pll;
>> +	struct clk *gmac_clkin;
>> +	struct clk *mac_clk_rx;
>> +	struct clk *mac_clk_tx;
>> +	struct clk *clk_mac_ref;
>> +	struct clk *clk_mac_refout;
>> +	struct clk *aclk_mac;
>> +	struct clk *pclk_mac;
>> +
>> +	int tx_delay;
>> +	int rx_delay;
>> +
>> +	struct regmap *grf;
>> +};
>> +
>> +#define RK3288_GRF_SOC_CON1 0x0248
>> +#define RK3288_GRF_SOC_CON3 0x0250
>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>> +#define RK3288_GRF_GPIO4B_E 0x01f4
> here you're using a space instead of a tab, please select one pattern either
> tabs or space but do not mix them.
>
>
>> +#define GPIO3D_2MA	0xFFFF0000
>> +#define GPIO3D_4MA	0xFFFF5555
>> +#define GPIO3D_8MA	0xFFFFAAAA
>> +#define GPIO3D_12MA	0xFFFFFFFF
>> +
>> +#define GPIO4A_2MA	0xFFFF0000
>> +#define GPIO4A_4MA	0xFFFF5555
>> +#define GPIO4A_8MA	0xFFFFAAAA
>> +#define GPIO4A_12MA	0xFFFFFFFF
> see comment about pin settings below
>
>
>> +
>> +#define GRF_BIT(nr)	(BIT(nr) | BIT(nr+16))
>> +#define GRF_CLR_BIT(nr)	(BIT(nr+16))
>> +
>> +#define GPIO4B_2_2MA	(GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
>> +#define GPIO4B_2_4MA	(GRF_BIT(2) | GRF_CLR_BIT(3))
>> +#define GPIO4B_2_8MA	(GRF_CLR_BIT(2) | GRF_BIT(3))
>> +#define GPIO4B_2_12MA	(GRF_BIT(2) | GRF_BIT(3))
>> +
>> +/*RK3288_GRF_SOC_CON1*/
>> +#define GMAC_PHY_INTF_SEL_RGMII	(GRF_BIT(6) | GRF_CLR_BIT(7) |
>> GRF_CLR_BIT(8))
>> +#define GMAC_PHY_INTF_SEL_RMII  (GRF_CLR_BIT(6) |
>> GRF_CLR_BIT(7) | GRF_BIT(8))
>> +#define GMAC_FLOW_CTRL          GRF_BIT(9)
>> +#define GMAC_FLOW_CTRL_CLR      GRF_CLR_BIT(9)
>> +#define GMAC_SPEED_10M          GRF_CLR_BIT(10)
>> +#define GMAC_SPEED_100M         GRF_BIT(10)
>> +#define GMAC_RMII_CLK_25M       GRF_BIT(11)
>> +#define GMAC_RMII_CLK_2_5M      GRF_CLR_BIT(11)
>> +#define GMAC_CLK_125M           (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
>> +#define GMAC_CLK_25M            (GRF_BIT(12) | GRF_BIT(13))
>> +#define GMAC_CLK_2_5M           (GRF_CLR_BIT(12) | GRF_BIT(13))
>> +#define GMAC_RMII_MODE          GRF_BIT(14)
>> +#define GMAC_RMII_MODE_CLR      GRF_CLR_BIT(14)
>> +
>> +/*RK3288_GRF_SOC_CON3*/
>> +#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
>> +#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
>> +#define GMAC_RXCLK_DLY_ENABLE   GRF_BIT(15)
>> +#define GMAC_RXCLK_DLY_DISABLE  GRF_CLR_BIT(15)
>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
> again mixed tabs and spaces as delimiters.
>
> Also the _CFG macros are not well abstracted. You could take a look at the
> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
>
> #define GMAC_CLK_DL_MASK	0x7f
> #define GMAC_CLK_RX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
> #define GMAC_CLK_TX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
>
>
>> +
>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>> +			 int tx_delay, int rx_delay)
>> +{
>> +	if (IS_ERR(bsp_priv->grf)) {
>> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
>> +		return;
>> +	}
>> +
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +		     GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>> +		     GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
>> +		     GMAC_CLK_RX_DL_CFG(rx_delay) |
>> +		     GMAC_CLK_TX_DL_CFG(tx_delay));
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
> please don't write to parts controlled by other drivers - here the drive
> strength settings of pins is controlled by the pinctrl driver. Instead you can
> just set the drive-strength in the pinctrl settings.
>
>
>> +
>> +	pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
>> +		 __func__, tx_delay, rx_delay);
>> +}
>> +
>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>> +{
>> +	if (IS_ERR(bsp_priv->grf)) {
>> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
> you have a device-reference in rk_priv_data, so you could use dev_err here.
> Same for all other pr_err/pr_debug/pr_* calls in this file.
>
>
>> +		return;
>> +	}
>> +
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +		     GMAC_PHY_INTF_SEL_RMII);
>> +	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +		     GMAC_RMII_MODE);
> these two could be combined?
>
>
>> +}
>> +
>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>> +{
>> +	if (IS_ERR(bsp_priv->grf)) {
>> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
>> +		return;
>> +	}
>> +
>> +	if (speed == 10)
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_2_5M);
>> +	else if (speed == 100)
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_25M);
>> +	else if (speed == 1000)
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, GMAC_CLK_125M);
>> +	else
>> +		pr_err("unknown speed value for RGMII! speed=%d", speed);
>> +}
>> +
>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>> +{
>> +	if (IS_ERR(bsp_priv->grf)) {
>> +		pr_err("%s: Missing rockchip,grf property\n", __func__);
>> +		return;
>> +	}
>> +
>> +	if (speed == 10) {
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +			     GMAC_RMII_CLK_2_5M);
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +			     GMAC_SPEED_10M);
> combine into one write?
>
>
>> +	} else if (speed == 100) {
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +			     GMAC_RMII_CLK_25M);
>> +		regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>> +			     GMAC_SPEED_100M);
> combine into one write?
>
>
>> +	} else {
>> +		pr_err("unknown speed value for RMII! speed=%d", speed);
>> +	}
>> +}
>> +
>> +#define MAC_CLK_RX	"mac_clk_rx"
>> +#define MAC_CLK_TX	"mac_clk_tx"
>> +#define CLK_MAC_REF	"clk_mac_ref"
>> +#define CLK_MAC_REF_OUT	"clk_mac_refout"
>> +#define CLK_MAC_PLL	"clk_mac_pll"
>> +#define ACLK_MAC	"aclk_mac"
>> +#define PCLK_MAC	"pclk_mac"
>> +#define MAC_CLKIN	"ext_gmac"
>> +#define CLK_MAC		"stmmaceth"
> why the need to extra constants for the clock names and not use the real names
> directly like most other drivers do?
>
>
>> +
>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
>> +{
>> +	struct device *dev = &bsp_priv->pdev->dev;
>> +
>> +	bsp_priv->clk_enabled = false;
>> +
>> +	bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>> +	if (IS_ERR(bsp_priv->mac_clk_rx))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, MAC_CLK_RX);
>> +
>> +	bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>> +	if (IS_ERR(bsp_priv->mac_clk_tx))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, MAC_CLK_TX);
>> +
>> +	bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>> +	if (IS_ERR(bsp_priv->clk_mac_ref))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, CLK_MAC_REF);
>> +
>> +	bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>> +	if (IS_ERR(bsp_priv->clk_mac_refout))
>> +		pr_warn("%s: warning:cannot get clock %s\n",
>> +			__func__, CLK_MAC_REF_OUT);
>> +
>> +	bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>> +	if (IS_ERR(bsp_priv->aclk_mac))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, ACLK_MAC);
>> +
>> +	bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>> +	if (IS_ERR(bsp_priv->pclk_mac))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, PCLK_MAC);
>> +
>> +	bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>> +	if (IS_ERR(bsp_priv->clk_mac_pll))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, CLK_MAC_PLL);
>> +
>> +	bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>> +	if (IS_ERR(bsp_priv->gmac_clkin))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, MAC_CLKIN);
>> +
>> +	bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>> +	if (IS_ERR(bsp_priv->clk_mac))
>> +		pr_warn("%s: warning: cannot get clock %s\n",
>> +			__func__, CLK_MAC);
> there is not clk_put in the _remove case ... maybe you could simply use
> devm_clk_get here so that all clocks are put on device removal.
>
> Also you're warning on every missing clock. Below it looks like you need a
> different set of them for rgmii and rmii, so maybe you should simply error out
> when core clocks for the selected phy-mode are missing.
>
>
>> +
>> +	if (bsp_priv->clock_input) {
>> +		pr_info("%s: clock input from PHY\n", __func__);
>> +	} else {
>> +		if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>> +			clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>> +
>> +		clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
> why the explicit reparenting. The common clock-framework is intelligent enough
> to select the best suitable parent.
>
> In general I'm thinking the clock-handling inside this driver should be
> simplyfied, as the common-clock framework can handle most cases itself. I.e. if
> a 125MHz external clock is present and so on. But haven't looked to deep yet.
>
>
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>> +{
>> +	int phy_iface = phy_iface = bsp_priv->phy_iface;
>> +
>> +	if (enable) {
>> +		if (!bsp_priv->clk_enabled) {
>> +			if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>> +				if (!IS_ERR(bsp_priv->mac_clk_rx))
>> +					clk_prepare_enable(
>> +						bsp_priv->mac_clk_rx);
>> +
>> +				if (!IS_ERR(bsp_priv->clk_mac_ref))
>> +					clk_prepare_enable(
>> +						bsp_priv->clk_mac_ref);
>> +
>> +				if (!IS_ERR(bsp_priv->clk_mac_refout))
>> +					clk_prepare_enable(
>> +						bsp_priv->clk_mac_refout);
>> +			}
>> +
>> +			if (!IS_ERR(bsp_priv->aclk_mac))
>> +				clk_prepare_enable(bsp_priv->aclk_mac);
>> +
>> +			if (!IS_ERR(bsp_priv->pclk_mac))
>> +				clk_prepare_enable(bsp_priv->pclk_mac);
>> +
>> +			if (!IS_ERR(bsp_priv->mac_clk_tx))
>> +				clk_prepare_enable(bsp_priv->mac_clk_tx);
>> +
>> +			/**
>> +			 * if (!IS_ERR(bsp_priv->clk_mac))
>> +			 *	clk_prepare_enable(bsp_priv->clk_mac);
>> +			 */
>> +			mdelay(5);
>> +			bsp_priv->clk_enabled = true;
>> +		}
>> +	} else {
>> +		if (bsp_priv->clk_enabled) {
>> +			if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>> +				if (!IS_ERR(bsp_priv->mac_clk_rx))
>> +					clk_disable_unprepare(
>> +						bsp_priv->mac_clk_rx);
>> +
>> +				if (!IS_ERR(bsp_priv->clk_mac_ref))
>> +					clk_disable_unprepare(
>> +						bsp_priv->clk_mac_ref);
>> +
>> +				if (!IS_ERR(bsp_priv->clk_mac_refout))
>> +					clk_disable_unprepare(
>> +						bsp_priv->clk_mac_refout);
>> +			}
>> +
>> +			if (!IS_ERR(bsp_priv->aclk_mac))
>> +				clk_disable_unprepare(bsp_priv->aclk_mac);
>> +
>> +			if (!IS_ERR(bsp_priv->pclk_mac))
>> +				clk_disable_unprepare(bsp_priv->pclk_mac);
>> +
>> +			if (!IS_ERR(bsp_priv->mac_clk_tx))
>> +				clk_disable_unprepare(bsp_priv->mac_clk_tx);
>> +			/**
>> +			 * if (!IS_ERR(bsp_priv->clk_mac))
>> +			 *	clk_disable_unprepare(bsp_priv->clk_mac);
>> +			 */
>> +			bsp_priv->clk_enabled = false;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>> +{
>> +	struct regulator *ldo;
>> +	char *ldostr = bsp_priv->pmu_regulator;
>> +	int ret;
>> +
>> +	if (!ldostr) {
>> +		pr_err("%s: no ldo found\n", __func__);
>> +		return -1;
>> +	}
>> +
>> +	ldo = regulator_get(NULL, ldostr);
>> +	if (!ldo) {
>> +		pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>> +	} else {
>> +		if (enable) {
>> +			if (!regulator_is_enabled(ldo)) {
>> +				regulator_set_voltage(ldo, 3300000, 3300000);
>> +				ret = regulator_enable(ldo);
>> +				if (ret != 0)
>> +					pr_err("%s: fail to enable %s\n",
>> +					       __func__, ldostr);
>> +				else
>> +					pr_info("turn on ldo done.\n");
>> +			} else {
>> +				pr_warn("%s is enabled before enable", ldostr);
>> +			}
>> +		} else {
>> +			if (regulator_is_enabled(ldo)) {
>> +				ret = regulator_disable(ldo);
>> +				if (ret != 0)
>> +					pr_err("%s: fail to disable %s\n",
>> +					       __func__, ldostr);
>> +				else
>> +					pr_info("turn off ldo done.\n");
>> +			} else {
>> +				pr_warn("%s is disabled before disable",
>> +					ldostr);
>> +			}
>> +		}
>> +		regulator_put(ldo);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool enable)
>> +{
>> +	if (enable) {
>> +		/*power on*/
>> +		if (gpio_is_valid(bsp_priv->power_io))
>> +			gpio_direction_output(bsp_priv->power_io,
>> +					      bsp_priv->power_io_level);
>> +	} else {
>> +		/*power off*/
>> +		if (gpio_is_valid(bsp_priv->power_io))
>> +			gpio_direction_output(bsp_priv->power_io,
>> +					      !bsp_priv->power_io_level);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>> +{
>> +	int ret = -1;
>> +
>> +	pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>> +
>> +	if (bsp_priv->power_ctrl_by_pmu)
>> +		ret = power_on_by_pmu(bsp_priv, enable);
>> +	else
>> +		ret =  power_on_by_gpio(bsp_priv, enable);
> this looks wrong. This should always be a regulator. Even a regulator + switch
> controlled by a gpio can still be modelled as regulator, so that you don't
> need this switch and assorted special handling - so just use the regulator API
>
In some case, it would be a switching circuit to control the power for PHY.
All I need to do is to control a GPIO to make switch on/off.  So...
>> +
>> +	if (enable) {
>> +		/*reset*/
>> +		if (gpio_is_valid(bsp_priv->reset_io)) {
>> +			gpio_direction_output(bsp_priv->reset_io,
>> +					      bsp_priv->reset_io_level);
>> +			mdelay(5);
>> +			gpio_direction_output(bsp_priv->reset_io,
>> +					      !bsp_priv->reset_io_level);
>> +		}
>> +		mdelay(30);
>> +
>> +	} else {
>> +		/*pull down reset*/
>> +		if (gpio_is_valid(bsp_priv->reset_io)) {
>> +			gpio_direction_output(bsp_priv->reset_io,
>> +					      bsp_priv->reset_io_level);
>> +		}
>> +	}
> I'm not sure yet if it would be better to use the reset framework for this.
> While it says it is also meant for reset-gpios, there does not seem a driver
> for this to exist yet.
>
What should I do?
>
>> +
>> +	return ret;
>> +}
>> +
>> +#define GPIO_PHY_POWER	"gmac_phy_power"
>> +#define GPIO_PHY_RESET	"gmac_phy_reset"
>> +#define GPIO_PHY_IRQ	"gmac_phy_irq"
> again I don't understand why these constants are necessary
>
>> +
>> +static void *rk_gmac_setup(struct platform_device *pdev)
>> +{
>> +	struct rk_priv_data *bsp_priv;
>> +	struct device *dev = &pdev->dev;
>> +	enum of_gpio_flags flags;
>> +	int ret;
>> +	const char *strings = NULL;
>> +	int value;
>> +	int irq;
>> +
>> +	bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>> +	if (!bsp_priv)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>> +
>> +	ret = of_property_read_string(dev->of_node, "pmu_regulator", &strings);
>> +	if (ret) {
>> +		pr_err("%s: Can not read property: pmu_regulator.\n", __func__);
>> +		bsp_priv->power_ctrl_by_pmu = false;
>> +	} else {
>> +		pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
>> +			__func__, strings);
>> +		bsp_priv->power_ctrl_by_pmu = true;
>> +		strcpy(bsp_priv->pmu_regulator, strings);
>> +	}
> There is a generic regulator-dt-binding for regulator-consumers available
> which you should of course use.
>
The same explanation as above
>> +
>> +	ret = of_property_read_u32(dev->of_node, "pmu_enable_level", &value);
>> +	if (ret) {
>> +		pr_err("%s: Can not read property: pmu_enable_level.\n",
>> +		       __func__);
>> +		bsp_priv->power_ctrl_by_pmu = false;
>> +	} else {
>> +		pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>> +			__func__, (value == 1) ? "HIGH" : "LOW");
>> +		bsp_priv->power_ctrl_by_pmu = true;
>> +		bsp_priv->pmu_enable_level = value;
>> +	}
> What is this used for? Enabling should of course be done via regulator_enable
> and disabling using regulator_disable.
>
>
>> +
>> +	ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);
>> +	if (ret) {
>> +		pr_err("%s: Can not read property: clock_in_out.\n", __func__);
>> +		bsp_priv->clock_input = true;
>> +	} else {
>> +		pr_info("%s: clock input or output? (%s).\n",
>> +			__func__, strings);
>> +		if (!strcmp(strings, "input"))
>> +			bsp_priv->clock_input = true;
>> +		else
>> +			bsp_priv->clock_input = false;
>> +	}
>> +
>> +	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>> +	if (ret) {
>> +		bsp_priv->tx_delay = 0x30;
>> +		pr_err("%s: Can not read property: tx_delay.", __func__);
>> +		pr_err("%s: set tx_delay to 0x%x\n",
>> +		       __func__, bsp_priv->tx_delay);
>> +	} else {
>> +		pr_info("%s: TX delay(0x%x).\n", __func__, value);
>> +		bsp_priv->tx_delay = value;
>> +	}
>> +
>> +	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>> +	if (ret) {
>> +		bsp_priv->rx_delay = 0x10;
>> +		pr_err("%s: Can not read property: rx_delay.", __func__);
>> +		pr_err("%s: set rx_delay to 0x%x\n",
>> +		       __func__, bsp_priv->rx_delay);
>> +	} else {
>> +		pr_info("%s: RX delay(0x%x).\n", __func__, value);
>> +		bsp_priv->rx_delay = value;
>> +	}
>> +
>> +	bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +							"rockchip,grf");
>> +	bsp_priv->phyirq_io =
>> +		of_get_named_gpio_flags(dev->of_node,
>> +					"phyirq-gpio", 0, &flags);
>> +	bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>> +
>> +	bsp_priv->reset_io =
>> +		of_get_named_gpio_flags(dev->of_node,
>> +					"reset-gpio", 0, &flags);
>> +	bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>> +
>> +	bsp_priv->power_io =
>> +		of_get_named_gpio_flags(dev->of_node, "power-gpio", 0, &flags);
>> +	bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>> +
>> +	/*power*/
>> +	if (!gpio_is_valid(bsp_priv->power_io)) {
>> +		pr_err("%s: Failed to get GPIO %s.\n",
>> +		       __func__, "power-gpio");
>> +	} else {
>> +		ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>> +		if (ret)
>> +			pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>> +			       __func__, GPIO_PHY_POWER);
>> +	}
> When everything power-related is handled using the regulator api, you don't
> need this
The same explanation as above
>
>> +
>> +	if (!gpio_is_valid(bsp_priv->reset_io)) {
>> +		pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>> +	} else {
>> +		ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>> +		if (ret)
>> +			pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>> +			       __func__, GPIO_PHY_RESET);
>> +	}
>> +
>> +	if (bsp_priv->phyirq_io > 0) {
> This is more for my understanding: why does the mac driver need to handle the
> phy interrupt - but I might be overlooking something.
>
phy interrupt is not mandatory.  In most of the time,  in order to find 
something happen in PHY, for example,
link is up or down,  we just use polling method to read the phy's 
register in a timer.
Buf if phy interrupt is in use, when link is up or down,  phy interrupt 
pin will be assert to inform the CPU.
I just implement the driver for phy interrupt gpio,  not enable it as 
default.

>> +		struct plat_stmmacenet_data *plat_dat;
>> +
>> +		pr_info("PHY irq in use\n");
>> +		ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>> +		if (ret < 0) {
>> +			pr_warn("%s: Failed to request GPIO %s\n",
>> +				__func__, GPIO_PHY_IRQ);
>> +			goto goon;
>> +		}
>> +
>> +		ret = gpio_direction_input(bsp_priv->phyirq_io);
>> +		if (ret < 0) {
>> +			pr_err("%s, Failed to set input for GPIO %s\n",
>> +			       __func__, GPIO_PHY_IRQ);
>> +			gpio_free(bsp_priv->phyirq_io);
>> +			goto goon;
>> +		}
>> +
>> +		irq = gpio_to_irq(bsp_priv->phyirq_io);
>> +		if (irq < 0) {
>> +			ret = irq;
>> +			pr_err("Failed to set irq for %s\n",
>> +			       GPIO_PHY_IRQ);
>> +			gpio_free(bsp_priv->phyirq_io);
>> +			goto goon;
>> +		}
>> +
>> +		plat_dat = dev_get_platdata(&pdev->dev);
>> +		if (plat_dat)
>> +			plat_dat->mdio_bus_data->probed_phy_irq = irq;
>> +		else
>> +			pr_err("%s: plat_data is NULL\n", __func__);
>> +	}
>> +
>> +goon:
>> +	/*rmii or rgmii*/
>> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>> +		pr_info("%s: init for RGMII\n", __func__);
>> +		set_to_rgmii(bsp_priv, bsp_priv->tx_delay, bsp_priv->rx_delay);
>> +	} else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>> +		pr_info("%s: init for RMII\n", __func__);
>> +		set_to_rmii(bsp_priv);
>> +	} else {
>> +		pr_err("%s: ERROR: NO interface defined!\n", __func__);
>> +	}
>> +
>> +	bsp_priv->pdev = pdev;
>> +
>> +	gmac_clk_init(bsp_priv);
>> +
>> +	return bsp_priv;
>> +}
>> +
>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>> +{
>> +	struct rk_priv_data *bsp_priv = priv;
>> +	int ret;
>> +
>> +	ret = phy_power_on(bsp_priv, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = gmac_clk_enable(bsp_priv, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>> +{
>> +	struct rk_priv_data *gmac = priv;
>> +
>> +	phy_power_on(gmac, false);
>> +	gmac_clk_enable(gmac, false);
>> +}
>> +
>> +static void rk_fix_speed(void *priv, unsigned int speed)
>> +{
>> +	struct rk_priv_data *bsp_priv = priv;
>> +
>> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>> +		set_rgmii_speed(bsp_priv, speed);
>> +	else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>> +		set_rmii_speed(bsp_priv, speed);
>> +	else
>> +		pr_err("unsupported interface %d", bsp_priv->phy_iface);
>> +}
>> +
>> +const struct stmmac_of_data rk_gmac_data = {
>> +	.has_gmac = 1,
>> +	.fix_mac_speed = rk_fix_speed,
>> +	.setup = rk_gmac_setup,
>> +	.init = rk_gmac_init,
>> +	.exit = rk_gmac_exit,
>> +};
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
>> 15814b7..b4dee96 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -33,6 +33,7 @@
>>
>>   static const struct of_device_id stmmac_dt_ids[] = {
>>   	/* SoC specific glue layers should come before generic bindings */
>> +	{ .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
> please name that rk3288_gmac_data [of course the other occurences too]
> It makes it easier to see which soc it is meant for and it's also not save to
> assume that the next one will use the same register + bit positions in the
> grf.
>
>
>>   	{ .compatible = "amlogic,meson6-dwmac", .data = &meson6_dwmac_data},
>>   	{ .compatible = "allwinner,sun7i-a20-gmac", .data = &sun7i_gmac_data},
>>   	{ .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct platform_device
>> *pdev) return  -ENOMEM;
>>   		}
>>
>> +		pdev->dev.platform_data = plat_dat;
>> +
>>   		ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>   		if (ret) {
>>   			pr_err("%s: main dt probe failed", __func__);
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
>> 25dd1f7..32a0516 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>   extern const struct stmmac_of_data stih4xx_dwmac_data;
>>   extern const struct stmmac_of_data stid127_dwmac_data;
>>   extern const struct stmmac_of_data socfpga_gmac_data;
>> +extern const struct stmmac_of_data rk_gmac_data;
>>
>>   #endif /* __STMMAC_PLATFORM_H__ */
>
>

Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC

[Roger]

Hi! Heiko

Any suggestion?

On 2014/12/3 15:57, Roger wrote:
> Hi! Heiko
>
> about regulator, power gpio,  reset gpio and irq gpio
> please refer to my comment inline, tks.
>
> On 2014/12/2 7:44, Heiko Stübner wrote:
>> Hi Roger,
>>
>> the comments inline are a rough first review. I hope to get a clearer 
>> picture
>> for the stuff I'm not sure about in v3 once the big issues are fixed.
>>
>>
>> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
>>> This driver is based on stmmac driver.
>>>
>>> modification based on Giuseppe CAVALLARO's suggestion:
>>> 1. use BIT()
>>>
>>>     > +/*RK3288_GRF_SOC_CON3*/
>>>     > +#define GMAC_TXCLK_DLY_ENABLE   ((0x4000 << 16) | (0x4000))
>>>     > +#define GMAC_TXCLK_DLY_DISABLE  ((0x4000 << 16) | (0x0000))
>>>
>>>     ...
>>>
>>>     why do not use BIT and BIT_MASK where possible?
>>>
>>>     ===>after modification:
>>>
>>>     #define GRF_BIT(nr)     (BIT(nr) | BIT(nr+16))
>>>     #define GRF_CLR_BIT(nr) (BIT(nr+16))
>>>     #define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
>>>     #define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
>>>     ...
>>> 2.
>>>
>>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>>     > +             GMAC_PHY_INTF_SEL_RGMII);
>>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>>     > +             GMAC_RMII_MODE_CLR);
>>>
>>>     maybe you could perform just one write unless there is some HW
>>>     constraint.
>>>
>>>     ===>after modification:
>>>
>>>     regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>>              GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>>
>>> 3. use macros
>>>
>>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, 
>>> 0xFFFFFFFF);
>>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>>>     > +             0x3<<2<<16 | 0x3<<2);
>>>
>>>     pls use macros, these shift sequence is really help to decode
>>>
>>>     ===>after modification:
>>>
>>>     regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>>     regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>>
>>> 4. remove grf fail check in rk_gmac_setup()
>>>
>>>     > +    if (IS_ERR(bsp_priv->grf))
>>>     > +        dev_err(&pdev->dev, "Missing rockchip,grf property\n");
>>>
>>>     I wonder if you can fail on here and save all the check in
>>>     set_rgmii_speed etc.
>>>     Maybe this can be considered a mandatory property for the 
>>> glue-logic.
>>>
>>> 5. remove .tx_coe=1
>>>
>>>     > +const struct stmmac_of_data rk_gmac_data = {
>>>     > +    .has_gmac = 1,
>>>     > +    .tx_coe = 1,
>>>
>>>     FYI, on new gmac there is the HW capability register to dinamically
>>>     provide you if coe is supported.
>>>
>>>     IMO you should add the OF "compatible" string and in case of mac
>>>     newer than the 3.50a you can remove coe.
>> changelogs like these, should be compact and also not be in the 
>> commit message
>> itself, but in the "comment"-section below the "---" and before the 
>> diffstat.
>>
>>
>>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
>>> ---
>> changelog here ... the commonly used pattern is something like
>>
>> changes since v2:
>> - ...
>> - ...
>>
>> changes since v1:
>> - ...
>>
>>> drivers/net/ethernet/stmicro/stmmac/Makefile       |    2 +-
>>>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  636
>>> ++++++++++++++++++++ 
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |
>>>   3 +
>>>   .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |    1 +
>>>   4 files changed, 641 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
>>> 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o 
>>> stmmac_mdio.o
>>> ring_mode.o    \
>>>
>>>   obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>>   stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o 
>>> dwmac-sunxi.o    \
>>> -               dwmac-sti.o dwmac-socfpga.o
>>> +               dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>>
>>>   obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>>   stmmac-pci-objs:= stmmac_pci.o
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
>>> index 0000000..870563f
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> @@ -0,0 +1,636 @@
>>> +/**
>>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>>> + *
>>> + * Chen-Zhi (Roger Chen)  <roger.chen@rock-chips.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or 
>>> modify
>>> + * it under the terms of the GNU General Public License as 
>>> published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/stmmac.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +struct rk_priv_data {
>>> +    struct platform_device *pdev;
>>> +    int phy_iface;
>>> +    bool power_ctrl_by_pmu;
>>> +    char pmu_regulator[32];
>>> +    int pmu_enable_level;
>>> +
>>> +    int power_io;
>>> +    int power_io_level;
>>> +    int reset_io;
>>> +    int reset_io_level;
>>> +    int phyirq_io;
>>> +    int phyirq_io_level;
>>> +
>>> +    bool clk_enabled;
>>> +    bool clock_input;
>>> +
>>> +    struct clk *clk_mac;
>>> +    struct clk *clk_mac_pll;
>>> +    struct clk *gmac_clkin;
>>> +    struct clk *mac_clk_rx;
>>> +    struct clk *mac_clk_tx;
>>> +    struct clk *clk_mac_ref;
>>> +    struct clk *clk_mac_refout;
>>> +    struct clk *aclk_mac;
>>> +    struct clk *pclk_mac;
>>> +
>>> +    int tx_delay;
>>> +    int rx_delay;
>>> +
>>> +    struct regmap *grf;
>>> +};
>>> +
>>> +#define RK3288_GRF_SOC_CON1 0x0248
>>> +#define RK3288_GRF_SOC_CON3 0x0250
>>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>>> +#define RK3288_GRF_GPIO4B_E 0x01f4
>> here you're using a space instead of a tab, please select one pattern 
>> either
>> tabs or space but do not mix them.
>>
>>
>>> +#define GPIO3D_2MA    0xFFFF0000
>>> +#define GPIO3D_4MA    0xFFFF5555
>>> +#define GPIO3D_8MA    0xFFFFAAAA
>>> +#define GPIO3D_12MA    0xFFFFFFFF
>>> +
>>> +#define GPIO4A_2MA    0xFFFF0000
>>> +#define GPIO4A_4MA    0xFFFF5555
>>> +#define GPIO4A_8MA    0xFFFFAAAA
>>> +#define GPIO4A_12MA    0xFFFFFFFF
>> see comment about pin settings below
>>
>>
>>> +
>>> +#define GRF_BIT(nr)    (BIT(nr) | BIT(nr+16))
>>> +#define GRF_CLR_BIT(nr)    (BIT(nr+16))
>>> +
>>> +#define GPIO4B_2_2MA    (GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_4MA    (GRF_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_8MA    (GRF_CLR_BIT(2) | GRF_BIT(3))
>>> +#define GPIO4B_2_12MA    (GRF_BIT(2) | GRF_BIT(3))
>>> +
>>> +/*RK3288_GRF_SOC_CON1*/
>>> +#define GMAC_PHY_INTF_SEL_RGMII    (GRF_BIT(6) | GRF_CLR_BIT(7) |
>>> GRF_CLR_BIT(8))
>>> +#define GMAC_PHY_INTF_SEL_RMII  (GRF_CLR_BIT(6) |
>>> GRF_CLR_BIT(7) | GRF_BIT(8))
>>> +#define GMAC_FLOW_CTRL          GRF_BIT(9)
>>> +#define GMAC_FLOW_CTRL_CLR      GRF_CLR_BIT(9)
>>> +#define GMAC_SPEED_10M          GRF_CLR_BIT(10)
>>> +#define GMAC_SPEED_100M         GRF_BIT(10)
>>> +#define GMAC_RMII_CLK_25M       GRF_BIT(11)
>>> +#define GMAC_RMII_CLK_2_5M      GRF_CLR_BIT(11)
>>> +#define GMAC_CLK_125M           (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
>>> +#define GMAC_CLK_25M            (GRF_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_CLK_2_5M           (GRF_CLR_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_RMII_MODE          GRF_BIT(14)
>>> +#define GMAC_RMII_MODE_CLR      GRF_CLR_BIT(14)
>>> +
>>> +/*RK3288_GRF_SOC_CON3*/
>>> +#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
>>> +#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
>>> +#define GMAC_RXCLK_DLY_ENABLE   GRF_BIT(15)
>>> +#define GMAC_RXCLK_DLY_DISABLE  GRF_CLR_BIT(15)
>>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
>>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
>> again mixed tabs and spaces as delimiters.
>>
>> Also the _CFG macros are not well abstracted. You could take a look 
>> at the
>> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
>>
>> #define GMAC_CLK_DL_MASK    0x7f
>> #define GMAC_CLK_RX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
>> #define GMAC_CLK_TX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
>>
>>
>>> +
>>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>>> +             int tx_delay, int rx_delay)
>>> +{
>>> +    if (IS_ERR(bsp_priv->grf)) {
>>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +             GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> +             GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
>>> +             GMAC_CLK_RX_DL_CFG(rx_delay) |
>>> +             GMAC_CLK_TX_DL_CFG(tx_delay));
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>> please don't write to parts controlled by other drivers - here the drive
>> strength settings of pins is controlled by the pinctrl driver. 
>> Instead you can
>> just set the drive-strength in the pinctrl settings.
>>
>>
>>> +
>>> +    pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
>>> +         __func__, tx_delay, rx_delay);
>>> +}
>>> +
>>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>>> +{
>>> +    if (IS_ERR(bsp_priv->grf)) {
>>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
>> you have a device-reference in rk_priv_data, so you could use dev_err 
>> here.
>> Same for all other pr_err/pr_debug/pr_* calls in this file.
>>
>>
>>> +        return;
>>> +    }
>>> +
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +             GMAC_PHY_INTF_SEL_RMII);
>>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +             GMAC_RMII_MODE);
>> these two could be combined?
>>
>>
>>> +}
>>> +
>>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> +    if (IS_ERR(bsp_priv->grf)) {
>>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    if (speed == 10)
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, 
>>> GMAC_CLK_2_5M);
>>> +    else if (speed == 100)
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, 
>>> GMAC_CLK_25M);
>>> +    else if (speed == 1000)
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1, 
>>> GMAC_CLK_125M);
>>> +    else
>>> +        pr_err("unknown speed value for RGMII! speed=%d", speed);
>>> +}
>>> +
>>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> +    if (IS_ERR(bsp_priv->grf)) {
>>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    if (speed == 10) {
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                 GMAC_RMII_CLK_2_5M);
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                 GMAC_SPEED_10M);
>> combine into one write?
>>
>>
>>> +    } else if (speed == 100) {
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                 GMAC_RMII_CLK_25M);
>>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                 GMAC_SPEED_100M);
>> combine into one write?
>>
>>
>>> +    } else {
>>> +        pr_err("unknown speed value for RMII! speed=%d", speed);
>>> +    }
>>> +}
>>> +
>>> +#define MAC_CLK_RX    "mac_clk_rx"
>>> +#define MAC_CLK_TX    "mac_clk_tx"
>>> +#define CLK_MAC_REF    "clk_mac_ref"
>>> +#define CLK_MAC_REF_OUT    "clk_mac_refout"
>>> +#define CLK_MAC_PLL    "clk_mac_pll"
>>> +#define ACLK_MAC    "aclk_mac"
>>> +#define PCLK_MAC    "pclk_mac"
>>> +#define MAC_CLKIN    "ext_gmac"
>>> +#define CLK_MAC        "stmmaceth"
>> why the need to extra constants for the clock names and not use the 
>> real names
>> directly like most other drivers do?
>>
>>
>>> +
>>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
>>> +{
>>> +    struct device *dev = &bsp_priv->pdev->dev;
>>> +
>>> +    bsp_priv->clk_enabled = false;
>>> +
>>> +    bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>>> +    if (IS_ERR(bsp_priv->mac_clk_rx))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, MAC_CLK_RX);
>>> +
>>> +    bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>>> +    if (IS_ERR(bsp_priv->mac_clk_tx))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, MAC_CLK_TX);
>>> +
>>> +    bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>>> +    if (IS_ERR(bsp_priv->clk_mac_ref))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, CLK_MAC_REF);
>>> +
>>> +    bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>>> +    if (IS_ERR(bsp_priv->clk_mac_refout))
>>> +        pr_warn("%s: warning:cannot get clock %s\n",
>>> +            __func__, CLK_MAC_REF_OUT);
>>> +
>>> +    bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>>> +    if (IS_ERR(bsp_priv->aclk_mac))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, ACLK_MAC);
>>> +
>>> +    bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>>> +    if (IS_ERR(bsp_priv->pclk_mac))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, PCLK_MAC);
>>> +
>>> +    bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>>> +    if (IS_ERR(bsp_priv->clk_mac_pll))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, CLK_MAC_PLL);
>>> +
>>> +    bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>>> +    if (IS_ERR(bsp_priv->gmac_clkin))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, MAC_CLKIN);
>>> +
>>> +    bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>>> +    if (IS_ERR(bsp_priv->clk_mac))
>>> +        pr_warn("%s: warning: cannot get clock %s\n",
>>> +            __func__, CLK_MAC);
>> there is not clk_put in the _remove case ... maybe you could simply use
>> devm_clk_get here so that all clocks are put on device removal.
>>
>> Also you're warning on every missing clock. Below it looks like you 
>> need a
>> different set of them for rgmii and rmii, so maybe you should simply 
>> error out
>> when core clocks for the selected phy-mode are missing.
>>
>>
>>> +
>>> +    if (bsp_priv->clock_input) {
>>> +        pr_info("%s: clock input from PHY\n", __func__);
>>> +    } else {
>>> +        if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> +            clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>>> +
>>> +        clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
>> why the explicit reparenting. The common clock-framework is 
>> intelligent enough
>> to select the best suitable parent.
>>
>> In general I'm thinking the clock-handling inside this driver should be
>> simplyfied, as the common-clock framework can handle most cases 
>> itself. I.e. if
>> a 125MHz external clock is present and so on. But haven't looked to 
>> deep yet.
>>
>>
>>
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +    int phy_iface = phy_iface = bsp_priv->phy_iface;
>>> +
>>> +    if (enable) {
>>> +        if (!bsp_priv->clk_enabled) {
>>> +            if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> +                if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> +                    clk_prepare_enable(
>>> +                        bsp_priv->mac_clk_rx);
>>> +
>>> +                if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> +                    clk_prepare_enable(
>>> +                        bsp_priv->clk_mac_ref);
>>> +
>>> +                if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> +                    clk_prepare_enable(
>>> +                        bsp_priv->clk_mac_refout);
>>> +            }
>>> +
>>> +            if (!IS_ERR(bsp_priv->aclk_mac))
>>> +                clk_prepare_enable(bsp_priv->aclk_mac);
>>> +
>>> +            if (!IS_ERR(bsp_priv->pclk_mac))
>>> +                clk_prepare_enable(bsp_priv->pclk_mac);
>>> +
>>> +            if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> +                clk_prepare_enable(bsp_priv->mac_clk_tx);
>>> +
>>> +            /**
>>> +             * if (!IS_ERR(bsp_priv->clk_mac))
>>> +             *    clk_prepare_enable(bsp_priv->clk_mac);
>>> +             */
>>> +            mdelay(5);
>>> +            bsp_priv->clk_enabled = true;
>>> +        }
>>> +    } else {
>>> +        if (bsp_priv->clk_enabled) {
>>> +            if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> +                if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> +                    clk_disable_unprepare(
>>> +                        bsp_priv->mac_clk_rx);
>>> +
>>> +                if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> +                    clk_disable_unprepare(
>>> +                        bsp_priv->clk_mac_ref);
>>> +
>>> +                if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> +                    clk_disable_unprepare(
>>> +                        bsp_priv->clk_mac_refout);
>>> +            }
>>> +
>>> +            if (!IS_ERR(bsp_priv->aclk_mac))
>>> +                clk_disable_unprepare(bsp_priv->aclk_mac);
>>> +
>>> +            if (!IS_ERR(bsp_priv->pclk_mac))
>>> +                clk_disable_unprepare(bsp_priv->pclk_mac);
>>> +
>>> +            if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_disable_unprepare(bsp_priv->mac_clk_tx);
>>> +            /**
>>> +             * if (!IS_ERR(bsp_priv->clk_mac))
>>> +             * clk_disable_unprepare(bsp_priv->clk_mac);
>>> +             */
>>> +            bsp_priv->clk_enabled = false;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +    struct regulator *ldo;
>>> +    char *ldostr = bsp_priv->pmu_regulator;
>>> +    int ret;
>>> +
>>> +    if (!ldostr) {
>>> +        pr_err("%s: no ldo found\n", __func__);
>>> +        return -1;
>>> +    }
>>> +
>>> +    ldo = regulator_get(NULL, ldostr);
>>> +    if (!ldo) {
>>> +        pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>>> +    } else {
>>> +        if (enable) {
>>> +            if (!regulator_is_enabled(ldo)) {
>>> +                regulator_set_voltage(ldo, 3300000, 3300000);
>>> +                ret = regulator_enable(ldo);
>>> +                if (ret != 0)
>>> +                    pr_err("%s: fail to enable %s\n",
>>> +                           __func__, ldostr);
>>> +                else
>>> +                    pr_info("turn on ldo done.\n");
>>> +            } else {
>>> +                pr_warn("%s is enabled before enable", ldostr);
>>> +            }
>>> +        } else {
>>> +            if (regulator_is_enabled(ldo)) {
>>> +                ret = regulator_disable(ldo);
>>> +                if (ret != 0)
>>> +                    pr_err("%s: fail to disable %s\n",
>>> +                           __func__, ldostr);
>>> +                else
>>> +                    pr_info("turn off ldo done.\n");
>>> +            } else {
>>> +                pr_warn("%s is disabled before disable",
>>> +                    ldostr);
>>> +            }
>>> +        }
>>> +        regulator_put(ldo);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool 
>>> enable)
>>> +{
>>> +    if (enable) {
>>> +        /*power on*/
>>> +        if (gpio_is_valid(bsp_priv->power_io))
>>> +            gpio_direction_output(bsp_priv->power_io,
>>> +                          bsp_priv->power_io_level);
>>> +    } else {
>>> +        /*power off*/
>>> +        if (gpio_is_valid(bsp_priv->power_io))
>>> +            gpio_direction_output(bsp_priv->power_io,
>>> +                          !bsp_priv->power_io_level);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +    int ret = -1;
>>> +
>>> +    pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>>> +
>>> +    if (bsp_priv->power_ctrl_by_pmu)
>>> +        ret = power_on_by_pmu(bsp_priv, enable);
>>> +    else
>>> +        ret =  power_on_by_gpio(bsp_priv, enable);
>> this looks wrong. This should always be a regulator. Even a regulator 
>> + switch
>> controlled by a gpio can still be modelled as regulator, so that you 
>> don't
>> need this switch and assorted special handling - so just use the 
>> regulator API
>>
> In some case, it would be a switching circuit to control the power for 
> PHY.
> All I need to do is to control a GPIO to make switch on/off. So...
>>> +
>>> +    if (enable) {
>>> +        /*reset*/
>>> +        if (gpio_is_valid(bsp_priv->reset_io)) {
>>> +            gpio_direction_output(bsp_priv->reset_io,
>>> +                          bsp_priv->reset_io_level);
>>> +            mdelay(5);
>>> +            gpio_direction_output(bsp_priv->reset_io,
>>> +                          !bsp_priv->reset_io_level);
>>> +        }
>>> +        mdelay(30);
>>> +
>>> +    } else {
>>> +        /*pull down reset*/
>>> +        if (gpio_is_valid(bsp_priv->reset_io)) {
>>> +            gpio_direction_output(bsp_priv->reset_io,
>>> +                          bsp_priv->reset_io_level);
>>> +        }
>>> +    }
>> I'm not sure yet if it would be better to use the reset framework for 
>> this.
>> While it says it is also meant for reset-gpios, there does not seem a 
>> driver
>> for this to exist yet.
>>
> What should I do?
>>
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +#define GPIO_PHY_POWER    "gmac_phy_power"
>>> +#define GPIO_PHY_RESET    "gmac_phy_reset"
>>> +#define GPIO_PHY_IRQ    "gmac_phy_irq"
>> again I don't understand why these constants are necessary
>>
>>> +
>>> +static void *rk_gmac_setup(struct platform_device *pdev)
>>> +{
>>> +    struct rk_priv_data *bsp_priv;
>>> +    struct device *dev = &pdev->dev;
>>> +    enum of_gpio_flags flags;
>>> +    int ret;
>>> +    const char *strings = NULL;
>>> +    int value;
>>> +    int irq;
>>> +
>>> +    bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>>> +    if (!bsp_priv)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>> +
>>> +    ret = of_property_read_string(dev->of_node, "pmu_regulator", 
>>> &strings);
>>> +    if (ret) {
>>> +        pr_err("%s: Can not read property: pmu_regulator.\n", 
>>> __func__);
>>> +        bsp_priv->power_ctrl_by_pmu = false;
>>> +    } else {
>>> +        pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
>>> +            __func__, strings);
>>> +        bsp_priv->power_ctrl_by_pmu = true;
>>> +        strcpy(bsp_priv->pmu_regulator, strings);
>>> +    }
>> There is a generic regulator-dt-binding for regulator-consumers 
>> available
>> which you should of course use.
>>
> The same explanation as above
>>> +
>>> +    ret = of_property_read_u32(dev->of_node, "pmu_enable_level", 
>>> &value);
>>> +    if (ret) {
>>> +        pr_err("%s: Can not read property: pmu_enable_level.\n",
>>> +               __func__);
>>> +        bsp_priv->power_ctrl_by_pmu = false;
>>> +    } else {
>>> +        pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>>> +            __func__, (value == 1) ? "HIGH" : "LOW");
>>> +        bsp_priv->power_ctrl_by_pmu = true;
>>> +        bsp_priv->pmu_enable_level = value;
>>> +    }
>> What is this used for? Enabling should of course be done via 
>> regulator_enable
>> and disabling using regulator_disable.
>>
>>
>>> +
>>> +    ret = of_property_read_string(dev->of_node, "clock_in_out", 
>>> &strings);
>>> +    if (ret) {
>>> +        pr_err("%s: Can not read property: clock_in_out.\n", 
>>> __func__);
>>> +        bsp_priv->clock_input = true;
>>> +    } else {
>>> +        pr_info("%s: clock input or output? (%s).\n",
>>> +            __func__, strings);
>>> +        if (!strcmp(strings, "input"))
>>> +            bsp_priv->clock_input = true;
>>> +        else
>>> +            bsp_priv->clock_input = false;
>>> +    }
>>> +
>>> +    ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>>> +    if (ret) {
>>> +        bsp_priv->tx_delay = 0x30;
>>> +        pr_err("%s: Can not read property: tx_delay.", __func__);
>>> +        pr_err("%s: set tx_delay to 0x%x\n",
>>> +               __func__, bsp_priv->tx_delay);
>>> +    } else {
>>> +        pr_info("%s: TX delay(0x%x).\n", __func__, value);
>>> +        bsp_priv->tx_delay = value;
>>> +    }
>>> +
>>> +    ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>>> +    if (ret) {
>>> +        bsp_priv->rx_delay = 0x10;
>>> +        pr_err("%s: Can not read property: rx_delay.", __func__);
>>> +        pr_err("%s: set rx_delay to 0x%x\n",
>>> +               __func__, bsp_priv->rx_delay);
>>> +    } else {
>>> +        pr_info("%s: RX delay(0x%x).\n", __func__, value);
>>> +        bsp_priv->rx_delay = value;
>>> +    }
>>> +
>>> +    bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> +                            "rockchip,grf");
>>> +    bsp_priv->phyirq_io =
>>> +        of_get_named_gpio_flags(dev->of_node,
>>> +                    "phyirq-gpio", 0, &flags);
>>> +    bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +    bsp_priv->reset_io =
>>> +        of_get_named_gpio_flags(dev->of_node,
>>> +                    "reset-gpio", 0, &flags);
>>> +    bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +    bsp_priv->power_io =
>>> +        of_get_named_gpio_flags(dev->of_node, "power-gpio", 0, 
>>> &flags);
>>> +    bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +    /*power*/
>>> +    if (!gpio_is_valid(bsp_priv->power_io)) {
>>> +        pr_err("%s: Failed to get GPIO %s.\n",
>>> +               __func__, "power-gpio");
>>> +    } else {
>>> +        ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>>> +        if (ret)
>>> +            pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> +                   __func__, GPIO_PHY_POWER);
>>> +    }
>> When everything power-related is handled using the regulator api, you 
>> don't
>> need this
> The same explanation as above
>>
>>> +
>>> +    if (!gpio_is_valid(bsp_priv->reset_io)) {
>>> +        pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>>> +    } else {
>>> +        ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>>> +        if (ret)
>>> +            pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> +                   __func__, GPIO_PHY_RESET);
>>> +    }
>>> +
>>> +    if (bsp_priv->phyirq_io > 0) {
>> This is more for my understanding: why does the mac driver need to 
>> handle the
>> phy interrupt - but I might be overlooking something.
>>
> phy interrupt is not mandatory.  In most of the time,  in order to 
> find something happen in PHY, for example,
> link is up or down,  we just use polling method to read the phy's 
> register in a timer.
> Buf if phy interrupt is in use, when link is up or down,  phy 
> interrupt pin will be assert to inform the CPU.
> I just implement the driver for phy interrupt gpio,  not enable it as 
> default.
>
>>> +        struct plat_stmmacenet_data *plat_dat;
>>> +
>>> +        pr_info("PHY irq in use\n");
>>> +        ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>>> +        if (ret < 0) {
>>> +            pr_warn("%s: Failed to request GPIO %s\n",
>>> +                __func__, GPIO_PHY_IRQ);
>>> +            goto goon;
>>> +        }
>>> +
>>> +        ret = gpio_direction_input(bsp_priv->phyirq_io);
>>> +        if (ret < 0) {
>>> +            pr_err("%s, Failed to set input for GPIO %s\n",
>>> +                   __func__, GPIO_PHY_IRQ);
>>> +            gpio_free(bsp_priv->phyirq_io);
>>> +            goto goon;
>>> +        }
>>> +
>>> +        irq = gpio_to_irq(bsp_priv->phyirq_io);
>>> +        if (irq < 0) {
>>> +            ret = irq;
>>> +            pr_err("Failed to set irq for %s\n",
>>> +                   GPIO_PHY_IRQ);
>>> +            gpio_free(bsp_priv->phyirq_io);
>>> +            goto goon;
>>> +        }
>>> +
>>> +        plat_dat = dev_get_platdata(&pdev->dev);
>>> +        if (plat_dat)
>>> +            plat_dat->mdio_bus_data->probed_phy_irq = irq;
>>> +        else
>>> +            pr_err("%s: plat_data is NULL\n", __func__);
>>> +    }
>>> +
>>> +goon:
>>> +    /*rmii or rgmii*/
>>> +    if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>>> +        pr_info("%s: init for RGMII\n", __func__);
>>> +        set_to_rgmii(bsp_priv, bsp_priv->tx_delay, 
>>> bsp_priv->rx_delay);
>>> +    } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> +        pr_info("%s: init for RMII\n", __func__);
>>> +        set_to_rmii(bsp_priv);
>>> +    } else {
>>> +        pr_err("%s: ERROR: NO interface defined!\n", __func__);
>>> +    }
>>> +
>>> +    bsp_priv->pdev = pdev;
>>> +
>>> +    gmac_clk_init(bsp_priv);
>>> +
>>> +    return bsp_priv;
>>> +}
>>> +
>>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>>> +{
>>> +    struct rk_priv_data *bsp_priv = priv;
>>> +    int ret;
>>> +
>>> +    ret = phy_power_on(bsp_priv, true);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = gmac_clk_enable(bsp_priv, true);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>>> +{
>>> +    struct rk_priv_data *gmac = priv;
>>> +
>>> +    phy_power_on(gmac, false);
>>> +    gmac_clk_enable(gmac, false);
>>> +}
>>> +
>>> +static void rk_fix_speed(void *priv, unsigned int speed)
>>> +{
>>> +    struct rk_priv_data *bsp_priv = priv;
>>> +
>>> +    if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>>> +        set_rgmii_speed(bsp_priv, speed);
>>> +    else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> +        set_rmii_speed(bsp_priv, speed);
>>> +    else
>>> +        pr_err("unsupported interface %d", bsp_priv->phy_iface);
>>> +}
>>> +
>>> +const struct stmmac_of_data rk_gmac_data = {
>>> +    .has_gmac = 1,
>>> +    .fix_mac_speed = rk_fix_speed,
>>> +    .setup = rk_gmac_setup,
>>> +    .init = rk_gmac_init,
>>> +    .exit = rk_gmac_exit,
>>> +};
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
>>> 15814b7..b4dee96 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -33,6 +33,7 @@
>>>
>>>   static const struct of_device_id stmmac_dt_ids[] = {
>>>       /* SoC specific glue layers should come before generic 
>>> bindings */
>>> +    { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
>> please name that rk3288_gmac_data [of course the other occurences too]
>> It makes it easier to see which soc it is meant for and it's also not 
>> save to
>> assume that the next one will use the same register + bit positions 
>> in the
>> grf.
>>
>>
>>>       { .compatible = "amlogic,meson6-dwmac", .data = 
>>> &meson6_dwmac_data},
>>>       { .compatible = "allwinner,sun7i-a20-gmac", .data = 
>>> &sun7i_gmac_data},
>>>       { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct 
>>> platform_device
>>> *pdev) return  -ENOMEM;
>>>           }
>>>
>>> +        pdev->dev.platform_data = plat_dat;
>>> +
>>>           ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>>           if (ret) {
>>>               pr_err("%s: main dt probe failed", __func__);
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
>>> 25dd1f7..32a0516 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>>   extern const struct stmmac_of_data stih4xx_dwmac_data;
>>>   extern const struct stmmac_of_data stid127_dwmac_data;
>>>   extern const struct stmmac_of_data socfpga_gmac_data;
>>> +extern const struct stmmac_of_data rk_gmac_data;
>>>
>>>   #endif /* __STMMAC_PLATFORM_H__ */
>>
>>
>

Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC

[Heiko Stübner]

Am Donnerstag, 25. Dezember 2014, 08:46:08 schrieb Roger:
> Hi! Heiko
> 
> Any suggestion?

Follow the suggestions from Chen-Yu Tsai (and me) :-) .

I.e. use the phy-reset support the stmmac already provides and control power 
stuff using the regulator framework even if it's only a gpio-controlled switch.

See the vcc_host regulator in rk3288-evb.dtsi as an example on how these 
switches should normally be handled.

Heiko


> 
> On 2014/12/3 15:57, Roger wrote:
> > Hi! Heiko
> > 
> > about regulator, power gpio,  reset gpio and irq gpio
> > please refer to my comment inline, tks.
> > 
> > On 2014/12/2 7:44, Heiko Stübner wrote:
> >> Hi Roger,
> >> 
> >> the comments inline are a rough first review. I hope to get a clearer
> >> picture
> >> for the stuff I'm not sure about in v3 once the big issues are fixed.
> >> 
> >> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
> >>> This driver is based on stmmac driver.
> >>> 
> >>> modification based on Giuseppe CAVALLARO's suggestion:
> >>> 1. use BIT()
> >>> 
> >>>     > +/*RK3288_GRF_SOC_CON3*/
> >>>     > +#define GMAC_TXCLK_DLY_ENABLE   ((0x4000 << 16) | (0x4000))
> >>>     > +#define GMAC_TXCLK_DLY_DISABLE  ((0x4000 << 16) | (0x0000))
> >>>     
> >>>     ...
> >>>     
> >>>     why do not use BIT and BIT_MASK where possible?
> >>>     
> >>>     ===>after modification:
> >>>     
> >>>     #define GRF_BIT(nr)     (BIT(nr) | BIT(nr+16))
> >>>     #define GRF_CLR_BIT(nr) (BIT(nr+16))
> >>>     #define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
> >>>     #define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
> >>>     ...
> >>> 
> >>> 2.
> >>> 
> >>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>>     > +             GMAC_PHY_INTF_SEL_RGMII);
> >>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>>     > +             GMAC_RMII_MODE_CLR);
> >>>     
> >>>     maybe you could perform just one write unless there is some HW
> >>>     constraint.
> >>>     
> >>>     ===>after modification:
> >>>     
> >>>     regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>>     
> >>>              GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
> >>> 
> >>> 3. use macros
> >>> 
> >>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E,
> >>> 
> >>> 0xFFFFFFFF);
> >>> 
> >>>     > +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
> >>>     > +             0x3<<2<<16 | 0x3<<2);
> >>>     
> >>>     pls use macros, these shift sequence is really help to decode
> >>>     
> >>>     ===>after modification:
> >>>     
> >>>     regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
> >>>     regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
> >>> 
> >>> 4. remove grf fail check in rk_gmac_setup()
> >>> 
> >>>     > +    if (IS_ERR(bsp_priv->grf))
> >>>     > +        dev_err(&pdev->dev, "Missing rockchip,grf property\n");
> >>>     
> >>>     I wonder if you can fail on here and save all the check in
> >>>     set_rgmii_speed etc.
> >>>     Maybe this can be considered a mandatory property for the
> >>> 
> >>> glue-logic.
> >>> 
> >>> 5. remove .tx_coe=1
> >>> 
> >>>     > +const struct stmmac_of_data rk_gmac_data = {
> >>>     > +    .has_gmac = 1,
> >>>     > +    .tx_coe = 1,
> >>>     
> >>>     FYI, on new gmac there is the HW capability register to dinamically
> >>>     provide you if coe is supported.
> >>>     
> >>>     IMO you should add the OF "compatible" string and in case of mac
> >>>     newer than the 3.50a you can remove coe.
> >> 
> >> changelogs like these, should be compact and also not be in the
> >> commit message
> >> itself, but in the "comment"-section below the "---" and before the
> >> diffstat.
> >> 
> >>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
> >>> ---
> >> 
> >> changelog here ... the commonly used pattern is something like
> >> 
> >> changes since v2:
> >> - ...
> >> - ...
> >> 
> >> changes since v1:
> >> - ...
> >> 
> >>> drivers/net/ethernet/stmicro/stmmac/Makefile       |    2 +-
> >>> 
> >>>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  636
> >>> 
> >>> ++++++++++++++++++++
> >>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |
> >>> 
> >>>   3 +
> >>>   .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |    1 +
> >>>   4 files changed, 641 insertions(+), 1 deletion(-)
> >>>   create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> >>> 
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
> >>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
> >>> 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> >>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o
> >>> stmmac_mdio.o
> >>> ring_mode.o    \
> >>> 
> >>>   obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
> >>>   stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o
> >>> 
> >>> dwmac-sunxi.o    \
> >>> -               dwmac-sti.o dwmac-socfpga.o
> >>> +               dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
> >>> 
> >>>   obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
> >>>   stmmac-pci-objs:= stmmac_pci.o
> >>> 
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> >>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
> >>> index 0000000..870563f
> >>> --- /dev/null
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> >>> @@ -0,0 +1,636 @@
> >>> +/**
> >>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
> >>> + *
> >>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
> >>> + *
> >>> + * Chen-Zhi (Roger Chen)  <roger.chen@rock-chips.com>
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or
> >>> modify
> >>> + * it under the terms of the GNU General Public License as
> >>> published by
> >>> + * the Free Software Foundation; either version 2 of the License, or
> >>> + * (at your option) any later version.
> >>> + *
> >>> + * This program is distributed in the hope that it will be useful,
> >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>> + * GNU General Public License for more details.
> >>> + */
> >>> +
> >>> +#include <linux/stmmac.h>
> >>> +#include <linux/bitops.h>
> >>> +#include <linux/clk.h>
> >>> +#include <linux/phy.h>
> >>> +#include <linux/of_net.h>
> >>> +#include <linux/gpio.h>
> >>> +#include <linux/of_gpio.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/regulator/consumer.h>
> >>> +#include <linux/delay.h>
> >>> +#include <linux/mfd/syscon.h>
> >>> +#include <linux/regmap.h>
> >>> +
> >>> +struct rk_priv_data {
> >>> +    struct platform_device *pdev;
> >>> +    int phy_iface;
> >>> +    bool power_ctrl_by_pmu;
> >>> +    char pmu_regulator[32];
> >>> +    int pmu_enable_level;
> >>> +
> >>> +    int power_io;
> >>> +    int power_io_level;
> >>> +    int reset_io;
> >>> +    int reset_io_level;
> >>> +    int phyirq_io;
> >>> +    int phyirq_io_level;
> >>> +
> >>> +    bool clk_enabled;
> >>> +    bool clock_input;
> >>> +
> >>> +    struct clk *clk_mac;
> >>> +    struct clk *clk_mac_pll;
> >>> +    struct clk *gmac_clkin;
> >>> +    struct clk *mac_clk_rx;
> >>> +    struct clk *mac_clk_tx;
> >>> +    struct clk *clk_mac_ref;
> >>> +    struct clk *clk_mac_refout;
> >>> +    struct clk *aclk_mac;
> >>> +    struct clk *pclk_mac;
> >>> +
> >>> +    int tx_delay;
> >>> +    int rx_delay;
> >>> +
> >>> +    struct regmap *grf;
> >>> +};
> >>> +
> >>> +#define RK3288_GRF_SOC_CON1 0x0248
> >>> +#define RK3288_GRF_SOC_CON3 0x0250
> >>> +#define RK3288_GRF_GPIO3D_E 0x01ec
> >>> +#define RK3288_GRF_GPIO4A_E 0x01f0
> >>> +#define RK3288_GRF_GPIO4B_E 0x01f4
> >> 
> >> here you're using a space instead of a tab, please select one pattern
> >> either
> >> tabs or space but do not mix them.
> >> 
> >>> +#define GPIO3D_2MA    0xFFFF0000
> >>> +#define GPIO3D_4MA    0xFFFF5555
> >>> +#define GPIO3D_8MA    0xFFFFAAAA
> >>> +#define GPIO3D_12MA    0xFFFFFFFF
> >>> +
> >>> +#define GPIO4A_2MA    0xFFFF0000
> >>> +#define GPIO4A_4MA    0xFFFF5555
> >>> +#define GPIO4A_8MA    0xFFFFAAAA
> >>> +#define GPIO4A_12MA    0xFFFFFFFF
> >> 
> >> see comment about pin settings below
> >> 
> >>> +
> >>> +#define GRF_BIT(nr)    (BIT(nr) | BIT(nr+16))
> >>> +#define GRF_CLR_BIT(nr)    (BIT(nr+16))
> >>> +
> >>> +#define GPIO4B_2_2MA    (GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
> >>> +#define GPIO4B_2_4MA    (GRF_BIT(2) | GRF_CLR_BIT(3))
> >>> +#define GPIO4B_2_8MA    (GRF_CLR_BIT(2) | GRF_BIT(3))
> >>> +#define GPIO4B_2_12MA    (GRF_BIT(2) | GRF_BIT(3))
> >>> +
> >>> +/*RK3288_GRF_SOC_CON1*/
> >>> +#define GMAC_PHY_INTF_SEL_RGMII    (GRF_BIT(6) | GRF_CLR_BIT(7) |
> >>> GRF_CLR_BIT(8))
> >>> +#define GMAC_PHY_INTF_SEL_RMII  (GRF_CLR_BIT(6) |
> >>> GRF_CLR_BIT(7) | GRF_BIT(8))
> >>> +#define GMAC_FLOW_CTRL          GRF_BIT(9)
> >>> +#define GMAC_FLOW_CTRL_CLR      GRF_CLR_BIT(9)
> >>> +#define GMAC_SPEED_10M          GRF_CLR_BIT(10)
> >>> +#define GMAC_SPEED_100M         GRF_BIT(10)
> >>> +#define GMAC_RMII_CLK_25M       GRF_BIT(11)
> >>> +#define GMAC_RMII_CLK_2_5M      GRF_CLR_BIT(11)
> >>> +#define GMAC_CLK_125M           (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
> >>> +#define GMAC_CLK_25M            (GRF_BIT(12) | GRF_BIT(13))
> >>> +#define GMAC_CLK_2_5M           (GRF_CLR_BIT(12) | GRF_BIT(13))
> >>> +#define GMAC_RMII_MODE          GRF_BIT(14)
> >>> +#define GMAC_RMII_MODE_CLR      GRF_CLR_BIT(14)
> >>> +
> >>> +/*RK3288_GRF_SOC_CON3*/
> >>> +#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
> >>> +#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
> >>> +#define GMAC_RXCLK_DLY_ENABLE   GRF_BIT(15)
> >>> +#define GMAC_RXCLK_DLY_DISABLE  GRF_CLR_BIT(15)
> >>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
> >>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
> >> 
> >> again mixed tabs and spaces as delimiters.
> >> 
> >> Also the _CFG macros are not well abstracted. You could take a look
> >> at the
> >> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
> >> 
> >> #define GMAC_CLK_DL_MASK    0x7f
> >> #define GMAC_CLK_RX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
> >> #define GMAC_CLK_TX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
> >> 
> >>> +
> >>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
> >>> +             int tx_delay, int rx_delay)
> >>> +{
> >>> +    if (IS_ERR(bsp_priv->grf)) {
> >>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +             GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
> >>> +             GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
> >>> +             GMAC_CLK_RX_DL_CFG(rx_delay) |
> >>> +             GMAC_CLK_TX_DL_CFG(tx_delay));
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
> >> 
> >> please don't write to parts controlled by other drivers - here the drive
> >> strength settings of pins is controlled by the pinctrl driver.
> >> Instead you can
> >> just set the drive-strength in the pinctrl settings.
> >> 
> >>> +
> >>> +    pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
> >>> +         __func__, tx_delay, rx_delay);
> >>> +}
> >>> +
> >>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
> >>> +{
> >>> +    if (IS_ERR(bsp_priv->grf)) {
> >>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
> >> 
> >> you have a device-reference in rk_priv_data, so you could use dev_err
> >> here.
> >> Same for all other pr_err/pr_debug/pr_* calls in this file.
> >> 
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +             GMAC_PHY_INTF_SEL_RMII);
> >>> +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +             GMAC_RMII_MODE);
> >> 
> >> these two could be combined?
> >> 
> >>> +}
> >>> +
> >>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
> >>> +{
> >>> +    if (IS_ERR(bsp_priv->grf)) {
> >>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    if (speed == 10)
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> GMAC_CLK_2_5M);
> >>> +    else if (speed == 100)
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> GMAC_CLK_25M);
> >>> +    else if (speed == 1000)
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> GMAC_CLK_125M);
> >>> +    else
> >>> +        pr_err("unknown speed value for RGMII! speed=%d", speed);
> >>> +}
> >>> +
> >>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
> >>> +{
> >>> +    if (IS_ERR(bsp_priv->grf)) {
> >>> +        pr_err("%s: Missing rockchip,grf property\n", __func__);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    if (speed == 10) {
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +                 GMAC_RMII_CLK_2_5M);
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +                 GMAC_SPEED_10M);
> >> 
> >> combine into one write?
> >> 
> >>> +    } else if (speed == 100) {
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +                 GMAC_RMII_CLK_25M);
> >>> +        regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
> >>> +                 GMAC_SPEED_100M);
> >> 
> >> combine into one write?
> >> 
> >>> +    } else {
> >>> +        pr_err("unknown speed value for RMII! speed=%d", speed);
> >>> +    }
> >>> +}
> >>> +
> >>> +#define MAC_CLK_RX    "mac_clk_rx"
> >>> +#define MAC_CLK_TX    "mac_clk_tx"
> >>> +#define CLK_MAC_REF    "clk_mac_ref"
> >>> +#define CLK_MAC_REF_OUT    "clk_mac_refout"
> >>> +#define CLK_MAC_PLL    "clk_mac_pll"
> >>> +#define ACLK_MAC    "aclk_mac"
> >>> +#define PCLK_MAC    "pclk_mac"
> >>> +#define MAC_CLKIN    "ext_gmac"
> >>> +#define CLK_MAC        "stmmaceth"
> >> 
> >> why the need to extra constants for the clock names and not use the
> >> real names
> >> directly like most other drivers do?
> >> 
> >>> +
> >>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
> >>> +{
> >>> +    struct device *dev = &bsp_priv->pdev->dev;
> >>> +
> >>> +    bsp_priv->clk_enabled = false;
> >>> +
> >>> +    bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
> >>> +    if (IS_ERR(bsp_priv->mac_clk_rx))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, MAC_CLK_RX);
> >>> +
> >>> +    bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
> >>> +    if (IS_ERR(bsp_priv->mac_clk_tx))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, MAC_CLK_TX);
> >>> +
> >>> +    bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
> >>> +    if (IS_ERR(bsp_priv->clk_mac_ref))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, CLK_MAC_REF);
> >>> +
> >>> +    bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
> >>> +    if (IS_ERR(bsp_priv->clk_mac_refout))
> >>> +        pr_warn("%s: warning:cannot get clock %s\n",
> >>> +            __func__, CLK_MAC_REF_OUT);
> >>> +
> >>> +    bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
> >>> +    if (IS_ERR(bsp_priv->aclk_mac))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, ACLK_MAC);
> >>> +
> >>> +    bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
> >>> +    if (IS_ERR(bsp_priv->pclk_mac))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, PCLK_MAC);
> >>> +
> >>> +    bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
> >>> +    if (IS_ERR(bsp_priv->clk_mac_pll))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, CLK_MAC_PLL);
> >>> +
> >>> +    bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
> >>> +    if (IS_ERR(bsp_priv->gmac_clkin))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, MAC_CLKIN);
> >>> +
> >>> +    bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
> >>> +    if (IS_ERR(bsp_priv->clk_mac))
> >>> +        pr_warn("%s: warning: cannot get clock %s\n",
> >>> +            __func__, CLK_MAC);
> >> 
> >> there is not clk_put in the _remove case ... maybe you could simply use
> >> devm_clk_get here so that all clocks are put on device removal.
> >> 
> >> Also you're warning on every missing clock. Below it looks like you
> >> need a
> >> different set of them for rgmii and rmii, so maybe you should simply
> >> error out
> >> when core clocks for the selected phy-mode are missing.
> >> 
> >>> +
> >>> +    if (bsp_priv->clock_input) {
> >>> +        pr_info("%s: clock input from PHY\n", __func__);
> >>> +    } else {
> >>> +        if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
> >>> +            clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
> >>> +
> >>> +        clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
> >> 
> >> why the explicit reparenting. The common clock-framework is
> >> intelligent enough
> >> to select the best suitable parent.
> >> 
> >> In general I'm thinking the clock-handling inside this driver should be
> >> simplyfied, as the common-clock framework can handle most cases
> >> itself. I.e. if
> >> a 125MHz external clock is present and so on. But haven't looked to
> >> deep yet.
> >> 
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
> >>> +{
> >>> +    int phy_iface = phy_iface = bsp_priv->phy_iface;
> >>> +
> >>> +    if (enable) {
> >>> +        if (!bsp_priv->clk_enabled) {
> >>> +            if (phy_iface == PHY_INTERFACE_MODE_RMII) {
> >>> +                if (!IS_ERR(bsp_priv->mac_clk_rx))
> >>> +                    clk_prepare_enable(
> >>> +                        bsp_priv->mac_clk_rx);
> >>> +
> >>> +                if (!IS_ERR(bsp_priv->clk_mac_ref))
> >>> +                    clk_prepare_enable(
> >>> +                        bsp_priv->clk_mac_ref);
> >>> +
> >>> +                if (!IS_ERR(bsp_priv->clk_mac_refout))
> >>> +                    clk_prepare_enable(
> >>> +                        bsp_priv->clk_mac_refout);
> >>> +            }
> >>> +
> >>> +            if (!IS_ERR(bsp_priv->aclk_mac))
> >>> +                clk_prepare_enable(bsp_priv->aclk_mac);
> >>> +
> >>> +            if (!IS_ERR(bsp_priv->pclk_mac))
> >>> +                clk_prepare_enable(bsp_priv->pclk_mac);
> >>> +
> >>> +            if (!IS_ERR(bsp_priv->mac_clk_tx))
> >>> +                clk_prepare_enable(bsp_priv->mac_clk_tx);
> >>> +
> >>> +            /**
> >>> +             * if (!IS_ERR(bsp_priv->clk_mac))
> >>> +             *    clk_prepare_enable(bsp_priv->clk_mac);
> >>> +             */
> >>> +            mdelay(5);
> >>> +            bsp_priv->clk_enabled = true;
> >>> +        }
> >>> +    } else {
> >>> +        if (bsp_priv->clk_enabled) {
> >>> +            if (phy_iface == PHY_INTERFACE_MODE_RMII) {
> >>> +                if (!IS_ERR(bsp_priv->mac_clk_rx))
> >>> +                    clk_disable_unprepare(
> >>> +                        bsp_priv->mac_clk_rx);
> >>> +
> >>> +                if (!IS_ERR(bsp_priv->clk_mac_ref))
> >>> +                    clk_disable_unprepare(
> >>> +                        bsp_priv->clk_mac_ref);
> >>> +
> >>> +                if (!IS_ERR(bsp_priv->clk_mac_refout))
> >>> +                    clk_disable_unprepare(
> >>> +                        bsp_priv->clk_mac_refout);
> >>> +            }
> >>> +
> >>> +            if (!IS_ERR(bsp_priv->aclk_mac))
> >>> +                clk_disable_unprepare(bsp_priv->aclk_mac);
> >>> +
> >>> +            if (!IS_ERR(bsp_priv->pclk_mac))
> >>> +                clk_disable_unprepare(bsp_priv->pclk_mac);
> >>> +
> >>> +            if (!IS_ERR(bsp_priv->mac_clk_tx))
> >>> + clk_disable_unprepare(bsp_priv->mac_clk_tx);
> >>> +            /**
> >>> +             * if (!IS_ERR(bsp_priv->clk_mac))
> >>> +             * clk_disable_unprepare(bsp_priv->clk_mac);
> >>> +             */
> >>> +            bsp_priv->clk_enabled = false;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
> >>> +{
> >>> +    struct regulator *ldo;
> >>> +    char *ldostr = bsp_priv->pmu_regulator;
> >>> +    int ret;
> >>> +
> >>> +    if (!ldostr) {
> >>> +        pr_err("%s: no ldo found\n", __func__);
> >>> +        return -1;
> >>> +    }
> >>> +
> >>> +    ldo = regulator_get(NULL, ldostr);
> >>> +    if (!ldo) {
> >>> +        pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
> >>> +    } else {
> >>> +        if (enable) {
> >>> +            if (!regulator_is_enabled(ldo)) {
> >>> +                regulator_set_voltage(ldo, 3300000, 3300000);
> >>> +                ret = regulator_enable(ldo);
> >>> +                if (ret != 0)
> >>> +                    pr_err("%s: fail to enable %s\n",
> >>> +                           __func__, ldostr);
> >>> +                else
> >>> +                    pr_info("turn on ldo done.\n");
> >>> +            } else {
> >>> +                pr_warn("%s is enabled before enable", ldostr);
> >>> +            }
> >>> +        } else {
> >>> +            if (regulator_is_enabled(ldo)) {
> >>> +                ret = regulator_disable(ldo);
> >>> +                if (ret != 0)
> >>> +                    pr_err("%s: fail to disable %s\n",
> >>> +                           __func__, ldostr);
> >>> +                else
> >>> +                    pr_info("turn off ldo done.\n");
> >>> +            } else {
> >>> +                pr_warn("%s is disabled before disable",
> >>> +                    ldostr);
> >>> +            }
> >>> +        }
> >>> +        regulator_put(ldo);
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool
> >>> enable)
> >>> +{
> >>> +    if (enable) {
> >>> +        /*power on*/
> >>> +        if (gpio_is_valid(bsp_priv->power_io))
> >>> +            gpio_direction_output(bsp_priv->power_io,
> >>> +                          bsp_priv->power_io_level);
> >>> +    } else {
> >>> +        /*power off*/
> >>> +        if (gpio_is_valid(bsp_priv->power_io))
> >>> +            gpio_direction_output(bsp_priv->power_io,
> >>> +                          !bsp_priv->power_io_level);
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> >>> +{
> >>> +    int ret = -1;
> >>> +
> >>> +    pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
> >>> +
> >>> +    if (bsp_priv->power_ctrl_by_pmu)
> >>> +        ret = power_on_by_pmu(bsp_priv, enable);
> >>> +    else
> >>> +        ret =  power_on_by_gpio(bsp_priv, enable);
> >> 
> >> this looks wrong. This should always be a regulator. Even a regulator
> >> + switch
> >> controlled by a gpio can still be modelled as regulator, so that you
> >> don't
> >> need this switch and assorted special handling - so just use the
> >> regulator API
> > 
> > In some case, it would be a switching circuit to control the power for
> > PHY.
> > All I need to do is to control a GPIO to make switch on/off. So...
> > 
> >>> +
> >>> +    if (enable) {
> >>> +        /*reset*/
> >>> +        if (gpio_is_valid(bsp_priv->reset_io)) {
> >>> +            gpio_direction_output(bsp_priv->reset_io,
> >>> +                          bsp_priv->reset_io_level);
> >>> +            mdelay(5);
> >>> +            gpio_direction_output(bsp_priv->reset_io,
> >>> +                          !bsp_priv->reset_io_level);
> >>> +        }
> >>> +        mdelay(30);
> >>> +
> >>> +    } else {
> >>> +        /*pull down reset*/
> >>> +        if (gpio_is_valid(bsp_priv->reset_io)) {
> >>> +            gpio_direction_output(bsp_priv->reset_io,
> >>> +                          bsp_priv->reset_io_level);
> >>> +        }
> >>> +    }
> >> 
> >> I'm not sure yet if it would be better to use the reset framework for
> >> this.
> >> While it says it is also meant for reset-gpios, there does not seem a
> >> driver
> >> for this to exist yet.
> > 
> > What should I do?
> > 
> >>> +
> >>> +    return ret;
> >>> +}
> >>> +
> >>> +#define GPIO_PHY_POWER    "gmac_phy_power"
> >>> +#define GPIO_PHY_RESET    "gmac_phy_reset"
> >>> +#define GPIO_PHY_IRQ    "gmac_phy_irq"
> >> 
> >> again I don't understand why these constants are necessary
> >> 
> >>> +
> >>> +static void *rk_gmac_setup(struct platform_device *pdev)
> >>> +{
> >>> +    struct rk_priv_data *bsp_priv;
> >>> +    struct device *dev = &pdev->dev;
> >>> +    enum of_gpio_flags flags;
> >>> +    int ret;
> >>> +    const char *strings = NULL;
> >>> +    int value;
> >>> +    int irq;
> >>> +
> >>> +    bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
> >>> +    if (!bsp_priv)
> >>> +        return ERR_PTR(-ENOMEM);
> >>> +
> >>> +    bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
> >>> +
> >>> +    ret = of_property_read_string(dev->of_node, "pmu_regulator",
> >>> &strings);
> >>> +    if (ret) {
> >>> +        pr_err("%s: Can not read property: pmu_regulator.\n",
> >>> __func__);
> >>> +        bsp_priv->power_ctrl_by_pmu = false;
> >>> +    } else {
> >>> +        pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
> >>> +            __func__, strings);
> >>> +        bsp_priv->power_ctrl_by_pmu = true;
> >>> +        strcpy(bsp_priv->pmu_regulator, strings);
> >>> +    }
> >> 
> >> There is a generic regulator-dt-binding for regulator-consumers
> >> available
> >> which you should of course use.
> > 
> > The same explanation as above
> > 
> >>> +
> >>> +    ret = of_property_read_u32(dev->of_node, "pmu_enable_level",
> >>> &value);
> >>> +    if (ret) {
> >>> +        pr_err("%s: Can not read property: pmu_enable_level.\n",
> >>> +               __func__);
> >>> +        bsp_priv->power_ctrl_by_pmu = false;
> >>> +    } else {
> >>> +        pr_info("%s: PHY power controlled by pmu(level = %s).\n",
> >>> +            __func__, (value == 1) ? "HIGH" : "LOW");
> >>> +        bsp_priv->power_ctrl_by_pmu = true;
> >>> +        bsp_priv->pmu_enable_level = value;
> >>> +    }
> >> 
> >> What is this used for? Enabling should of course be done via
> >> regulator_enable
> >> and disabling using regulator_disable.
> >> 
> >>> +
> >>> +    ret = of_property_read_string(dev->of_node, "clock_in_out",
> >>> &strings);
> >>> +    if (ret) {
> >>> +        pr_err("%s: Can not read property: clock_in_out.\n",
> >>> __func__);
> >>> +        bsp_priv->clock_input = true;
> >>> +    } else {
> >>> +        pr_info("%s: clock input or output? (%s).\n",
> >>> +            __func__, strings);
> >>> +        if (!strcmp(strings, "input"))
> >>> +            bsp_priv->clock_input = true;
> >>> +        else
> >>> +            bsp_priv->clock_input = false;
> >>> +    }
> >>> +
> >>> +    ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
> >>> +    if (ret) {
> >>> +        bsp_priv->tx_delay = 0x30;
> >>> +        pr_err("%s: Can not read property: tx_delay.", __func__);
> >>> +        pr_err("%s: set tx_delay to 0x%x\n",
> >>> +               __func__, bsp_priv->tx_delay);
> >>> +    } else {
> >>> +        pr_info("%s: TX delay(0x%x).\n", __func__, value);
> >>> +        bsp_priv->tx_delay = value;
> >>> +    }
> >>> +
> >>> +    ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
> >>> +    if (ret) {
> >>> +        bsp_priv->rx_delay = 0x10;
> >>> +        pr_err("%s: Can not read property: rx_delay.", __func__);
> >>> +        pr_err("%s: set rx_delay to 0x%x\n",
> >>> +               __func__, bsp_priv->rx_delay);
> >>> +    } else {
> >>> +        pr_info("%s: RX delay(0x%x).\n", __func__, value);
> >>> +        bsp_priv->rx_delay = value;
> >>> +    }
> >>> +
> >>> +    bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>> +                            "rockchip,grf");
> >>> +    bsp_priv->phyirq_io =
> >>> +        of_get_named_gpio_flags(dev->of_node,
> >>> +                    "phyirq-gpio", 0, &flags);
> >>> +    bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> >>> +
> >>> +    bsp_priv->reset_io =
> >>> +        of_get_named_gpio_flags(dev->of_node,
> >>> +                    "reset-gpio", 0, &flags);
> >>> +    bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> >>> +
> >>> +    bsp_priv->power_io =
> >>> +        of_get_named_gpio_flags(dev->of_node, "power-gpio", 0,
> >>> &flags);
> >>> +    bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> >>> +
> >>> +    /*power*/
> >>> +    if (!gpio_is_valid(bsp_priv->power_io)) {
> >>> +        pr_err("%s: Failed to get GPIO %s.\n",
> >>> +               __func__, "power-gpio");
> >>> +    } else {
> >>> +        ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
> >>> +        if (ret)
> >>> +            pr_err("%s: ERROR: Failed to request GPIO %s.\n",
> >>> +                   __func__, GPIO_PHY_POWER);
> >>> +    }
> >> 
> >> When everything power-related is handled using the regulator api, you
> >> don't
> >> need this
> > 
> > The same explanation as above
> > 
> >>> +
> >>> +    if (!gpio_is_valid(bsp_priv->reset_io)) {
> >>> +        pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
> >>> +    } else {
> >>> +        ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
> >>> +        if (ret)
> >>> +            pr_err("%s: ERROR: Failed to request GPIO %s.\n",
> >>> +                   __func__, GPIO_PHY_RESET);
> >>> +    }
> >>> +
> >>> +    if (bsp_priv->phyirq_io > 0) {
> >> 
> >> This is more for my understanding: why does the mac driver need to
> >> handle the
> >> phy interrupt - but I might be overlooking something.
> > 
> > phy interrupt is not mandatory.  In most of the time,  in order to
> > find something happen in PHY, for example,
> > link is up or down,  we just use polling method to read the phy's
> > register in a timer.
> > Buf if phy interrupt is in use, when link is up or down,  phy
> > interrupt pin will be assert to inform the CPU.
> > I just implement the driver for phy interrupt gpio,  not enable it as
> > default.
> > 
> >>> +        struct plat_stmmacenet_data *plat_dat;
> >>> +
> >>> +        pr_info("PHY irq in use\n");
> >>> +        ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
> >>> +        if (ret < 0) {
> >>> +            pr_warn("%s: Failed to request GPIO %s\n",
> >>> +                __func__, GPIO_PHY_IRQ);
> >>> +            goto goon;
> >>> +        }
> >>> +
> >>> +        ret = gpio_direction_input(bsp_priv->phyirq_io);
> >>> +        if (ret < 0) {
> >>> +            pr_err("%s, Failed to set input for GPIO %s\n",
> >>> +                   __func__, GPIO_PHY_IRQ);
> >>> +            gpio_free(bsp_priv->phyirq_io);
> >>> +            goto goon;
> >>> +        }
> >>> +
> >>> +        irq = gpio_to_irq(bsp_priv->phyirq_io);
> >>> +        if (irq < 0) {
> >>> +            ret = irq;
> >>> +            pr_err("Failed to set irq for %s\n",
> >>> +                   GPIO_PHY_IRQ);
> >>> +            gpio_free(bsp_priv->phyirq_io);
> >>> +            goto goon;
> >>> +        }
> >>> +
> >>> +        plat_dat = dev_get_platdata(&pdev->dev);
> >>> +        if (plat_dat)
> >>> +            plat_dat->mdio_bus_data->probed_phy_irq = irq;
> >>> +        else
> >>> +            pr_err("%s: plat_data is NULL\n", __func__);
> >>> +    }
> >>> +
> >>> +goon:
> >>> +    /*rmii or rgmii*/
> >>> +    if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
> >>> +        pr_info("%s: init for RGMII\n", __func__);
> >>> +        set_to_rgmii(bsp_priv, bsp_priv->tx_delay,
> >>> bsp_priv->rx_delay);
> >>> +    } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
> >>> +        pr_info("%s: init for RMII\n", __func__);
> >>> +        set_to_rmii(bsp_priv);
> >>> +    } else {
> >>> +        pr_err("%s: ERROR: NO interface defined!\n", __func__);
> >>> +    }
> >>> +
> >>> +    bsp_priv->pdev = pdev;
> >>> +
> >>> +    gmac_clk_init(bsp_priv);
> >>> +
> >>> +    return bsp_priv;
> >>> +}
> >>> +
> >>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
> >>> +{
> >>> +    struct rk_priv_data *bsp_priv = priv;
> >>> +    int ret;
> >>> +
> >>> +    ret = phy_power_on(bsp_priv, true);
> >>> +    if (ret)
> >>> +        return ret;
> >>> +
> >>> +    ret = gmac_clk_enable(bsp_priv, true);
> >>> +    if (ret)
> >>> +        return ret;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
> >>> +{
> >>> +    struct rk_priv_data *gmac = priv;
> >>> +
> >>> +    phy_power_on(gmac, false);
> >>> +    gmac_clk_enable(gmac, false);
> >>> +}
> >>> +
> >>> +static void rk_fix_speed(void *priv, unsigned int speed)
> >>> +{
> >>> +    struct rk_priv_data *bsp_priv = priv;
> >>> +
> >>> +    if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
> >>> +        set_rgmii_speed(bsp_priv, speed);
> >>> +    else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
> >>> +        set_rmii_speed(bsp_priv, speed);
> >>> +    else
> >>> +        pr_err("unsupported interface %d", bsp_priv->phy_iface);
> >>> +}
> >>> +
> >>> +const struct stmmac_of_data rk_gmac_data = {
> >>> +    .has_gmac = 1,
> >>> +    .fix_mac_speed = rk_fix_speed,
> >>> +    .setup = rk_gmac_setup,
> >>> +    .init = rk_gmac_init,
> >>> +    .exit = rk_gmac_exit,
> >>> +};
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
> >>> 15814b7..b4dee96 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> @@ -33,6 +33,7 @@
> >>> 
> >>>   static const struct of_device_id stmmac_dt_ids[] = {
> >>>   
> >>>       /* SoC specific glue layers should come before generic
> >>> 
> >>> bindings */
> >>> +    { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
> >> 
> >> please name that rk3288_gmac_data [of course the other occurences too]
> >> It makes it easier to see which soc it is meant for and it's also not
> >> save to
> >> assume that the next one will use the same register + bit positions
> >> in the
> >> grf.
> >> 
> >>>       { .compatible = "amlogic,meson6-dwmac", .data =
> >>> 
> >>> &meson6_dwmac_data},
> >>> 
> >>>       { .compatible = "allwinner,sun7i-a20-gmac", .data =
> >>> 
> >>> &sun7i_gmac_data},
> >>> 
> >>>       { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
> >>> 
> >>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct
> >>> platform_device
> >>> *pdev) return  -ENOMEM;
> >>> 
> >>>           }
> >>> 
> >>> +        pdev->dev.platform_data = plat_dat;
> >>> +
> >>> 
> >>>           ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
> >>>           if (ret) {
> >>>           
> >>>               pr_err("%s: main dt probe failed", __func__);
> >>> 
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
> >>> 25dd1f7..32a0516 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> >>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
> >>> 
> >>>   extern const struct stmmac_of_data stih4xx_dwmac_data;
> >>>   extern const struct stmmac_of_data stid127_dwmac_data;
> >>>   extern const struct stmmac_of_data socfpga_gmac_data;
> >>> 
> >>> +extern const struct stmmac_of_data rk_gmac_data;
> >>> 
> >>>   #endif /* __STMMAC_PLATFORM_H__ */

Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC

[Chen-Yu Tsai]

Hi,

On Wed, Dec 3, 2014 at 3:57 PM, Roger <roger.chen@rock-chips.com> wrote:
> Hi! Heiko
>
> about regulator, power gpio,  reset gpio and irq gpio
> please refer to my comment inline, tks.
>
>
> On 2014/12/2 7:44, Heiko Stübner wrote:
>>
>> Hi Roger,
>>
>> the comments inline are a rough first review. I hope to get a clearer
>> picture
>> for the stuff I'm not sure about in v3 once the big issues are fixed.
>>
>>
>> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
>>>
>>> This driver is based on stmmac driver.
>>>
>>> modification based on Giuseppe CAVALLARO's suggestion:
>>> 1. use BIT()
>>>
>>>         > +/*RK3288_GRF_SOC_CON3*/
>>>         > +#define GMAC_TXCLK_DLY_ENABLE   ((0x4000 << 16) | (0x4000))
>>>         > +#define GMAC_TXCLK_DLY_DISABLE  ((0x4000 << 16) | (0x0000))
>>>
>>>         ...
>>>
>>>         why do not use BIT and BIT_MASK where possible?
>>>
>>>         ===>after modification:
>>>
>>>         #define GRF_BIT(nr)     (BIT(nr) | BIT(nr+16))
>>>         #define GRF_CLR_BIT(nr) (BIT(nr+16))
>>>         #define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
>>>         #define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
>>>         ...
>>> 2.
>>>
>>>         > +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>>         > +             GMAC_PHY_INTF_SEL_RGMII);
>>>         > +    regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>>         > +             GMAC_RMII_MODE_CLR);
>>>
>>>         maybe you could perform just one write unless there is some HW
>>>         constraint.
>>>
>>>         ===>after modification:
>>>
>>>         regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>>                          GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>>
>>> 3. use macros
>>>
>>>         > +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E,
>>> 0xFFFFFFFF);
>>>         > +    regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>>>         > +             0x3<<2<<16 | 0x3<<2);
>>>
>>>         pls use macros, these shift sequence is really help to decode
>>>
>>>         ===>after modification:
>>>
>>>         regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>>         regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>>
>>> 4. remove grf fail check in rk_gmac_setup()
>>>
>>>         > +    if (IS_ERR(bsp_priv->grf))
>>>         > +        dev_err(&pdev->dev, "Missing rockchip,grf
>>> property\n");
>>>
>>>         I wonder if you can fail on here and save all the check in
>>>         set_rgmii_speed etc.
>>>         Maybe this can be considered a mandatory property for the
>>> glue-logic.
>>>
>>> 5. remove .tx_coe=1
>>>
>>>         > +const struct stmmac_of_data rk_gmac_data = {
>>>         > +    .has_gmac = 1,
>>>         > +    .tx_coe = 1,
>>>
>>>         FYI, on new gmac there is the HW capability register to
>>> dinamically
>>>         provide you if coe is supported.
>>>
>>>         IMO you should add the OF "compatible" string and in case of mac
>>>         newer than the 3.50a you can remove coe.
>>
>> changelogs like these, should be compact and also not be in the commit
>> message
>> itself, but in the "comment"-section below the "---" and before the
>> diffstat.
>>
>>
>>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
>>> ---
>>
>> changelog here ... the commonly used pattern is something like
>>
>> changes since v2:
>> - ...
>> - ...
>>
>> changes since v1:
>> - ...
>>
>>>   drivers/net/ethernet/stmicro/stmmac/Makefile       |    2 +-
>>>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  636
>>> ++++++++++++++++++++ .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |
>>>   3 +
>>>   .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |    1 +
>>>   4 files changed, 641 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
>>> 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o
>>> stmmac_mdio.o
>>> ring_mode.o     \
>>>
>>>   obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>>   stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o  \
>>> -                      dwmac-sti.o dwmac-socfpga.o
>>> +                      dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>>
>>>   obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>>   stmmac-pci-objs:= stmmac_pci.o
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
>>> index 0000000..870563f
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> @@ -0,0 +1,636 @@
>>> +/**
>>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>>> + *
>>> + * Chen-Zhi (Roger Chen)  <roger.chen@rock-chips.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/stmmac.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +struct rk_priv_data {
>>> +       struct platform_device *pdev;
>>> +       int phy_iface;
>>> +       bool power_ctrl_by_pmu;
>>> +       char pmu_regulator[32];
>>> +       int pmu_enable_level;
>>> +
>>> +       int power_io;
>>> +       int power_io_level;
>>> +       int reset_io;
>>> +       int reset_io_level;
>>> +       int phyirq_io;
>>> +       int phyirq_io_level;
>>> +
>>> +       bool clk_enabled;
>>> +       bool clock_input;
>>> +
>>> +       struct clk *clk_mac;
>>> +       struct clk *clk_mac_pll;
>>> +       struct clk *gmac_clkin;
>>> +       struct clk *mac_clk_rx;
>>> +       struct clk *mac_clk_tx;
>>> +       struct clk *clk_mac_ref;
>>> +       struct clk *clk_mac_refout;
>>> +       struct clk *aclk_mac;
>>> +       struct clk *pclk_mac;
>>> +
>>> +       int tx_delay;
>>> +       int rx_delay;
>>> +
>>> +       struct regmap *grf;
>>> +};
>>> +
>>> +#define RK3288_GRF_SOC_CON1 0x0248
>>> +#define RK3288_GRF_SOC_CON3 0x0250
>>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>>> +#define RK3288_GRF_GPIO4B_E 0x01f4
>>
>> here you're using a space instead of a tab, please select one pattern
>> either
>> tabs or space but do not mix them.
>>
>>
>>> +#define GPIO3D_2MA     0xFFFF0000
>>> +#define GPIO3D_4MA     0xFFFF5555
>>> +#define GPIO3D_8MA     0xFFFFAAAA
>>> +#define GPIO3D_12MA    0xFFFFFFFF
>>> +
>>> +#define GPIO4A_2MA     0xFFFF0000
>>> +#define GPIO4A_4MA     0xFFFF5555
>>> +#define GPIO4A_8MA     0xFFFFAAAA
>>> +#define GPIO4A_12MA    0xFFFFFFFF
>>
>> see comment about pin settings below
>>
>>
>>> +
>>> +#define GRF_BIT(nr)    (BIT(nr) | BIT(nr+16))
>>> +#define GRF_CLR_BIT(nr)        (BIT(nr+16))
>>> +
>>> +#define GPIO4B_2_2MA   (GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_4MA   (GRF_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_8MA   (GRF_CLR_BIT(2) | GRF_BIT(3))
>>> +#define GPIO4B_2_12MA  (GRF_BIT(2) | GRF_BIT(3))
>>> +
>>> +/*RK3288_GRF_SOC_CON1*/
>>> +#define GMAC_PHY_INTF_SEL_RGMII        (GRF_BIT(6) | GRF_CLR_BIT(7) |
>>> GRF_CLR_BIT(8))
>>> +#define GMAC_PHY_INTF_SEL_RMII  (GRF_CLR_BIT(6) |
>>> GRF_CLR_BIT(7) | GRF_BIT(8))
>>> +#define GMAC_FLOW_CTRL          GRF_BIT(9)
>>> +#define GMAC_FLOW_CTRL_CLR      GRF_CLR_BIT(9)
>>> +#define GMAC_SPEED_10M          GRF_CLR_BIT(10)
>>> +#define GMAC_SPEED_100M         GRF_BIT(10)
>>> +#define GMAC_RMII_CLK_25M       GRF_BIT(11)
>>> +#define GMAC_RMII_CLK_2_5M      GRF_CLR_BIT(11)
>>> +#define GMAC_CLK_125M           (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
>>> +#define GMAC_CLK_25M            (GRF_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_CLK_2_5M           (GRF_CLR_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_RMII_MODE          GRF_BIT(14)
>>> +#define GMAC_RMII_MODE_CLR      GRF_CLR_BIT(14)
>>> +
>>> +/*RK3288_GRF_SOC_CON3*/
>>> +#define GMAC_TXCLK_DLY_ENABLE   GRF_BIT(14)
>>> +#define GMAC_TXCLK_DLY_DISABLE  GRF_CLR_BIT(14)
>>> +#define GMAC_RXCLK_DLY_ENABLE   GRF_BIT(15)
>>> +#define GMAC_RXCLK_DLY_DISABLE  GRF_CLR_BIT(15)
>>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
>>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
>>
>> again mixed tabs and spaces as delimiters.
>>
>> Also the _CFG macros are not well abstracted. You could take a look at the
>> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
>>
>> #define GMAC_CLK_DL_MASK        0x7f
>> #define GMAC_CLK_RX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
>> #define GMAC_CLK_TX_DL_CFG(val)  HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
>>
>>
>>> +
>>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>>> +                        int tx_delay, int rx_delay)
>>> +{
>>> +       if (IS_ERR(bsp_priv->grf)) {
>>> +               pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> +               return;
>>> +       }
>>> +
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                    GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> +                    GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
>>> +                    GMAC_CLK_RX_DL_CFG(rx_delay) |
>>> +                    GMAC_CLK_TX_DL_CFG(tx_delay));
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>
>> please don't write to parts controlled by other drivers - here the drive
>> strength settings of pins is controlled by the pinctrl driver. Instead you
>> can
>> just set the drive-strength in the pinctrl settings.
>>
>>
>>> +
>>> +       pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
>>> +                __func__, tx_delay, rx_delay);
>>> +}
>>> +
>>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>>> +{
>>> +       if (IS_ERR(bsp_priv->grf)) {
>>> +               pr_err("%s: Missing rockchip,grf property\n", __func__);
>>
>> you have a device-reference in rk_priv_data, so you could use dev_err
>> here.
>> Same for all other pr_err/pr_debug/pr_* calls in this file.
>>
>>
>>> +               return;
>>> +       }
>>> +
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                    GMAC_PHY_INTF_SEL_RMII);
>>> +       regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                    GMAC_RMII_MODE);
>>
>> these two could be combined?
>>
>>
>>> +}
>>> +
>>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> +       if (IS_ERR(bsp_priv->grf)) {
>>> +               pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> +               return;
>>> +       }
>>> +
>>> +       if (speed == 10)
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_2_5M);
>>> +       else if (speed == 100)
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_25M);
>>> +       else if (speed == 1000)
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_125M);
>>> +       else
>>> +               pr_err("unknown speed value for RGMII! speed=%d", speed);
>>> +}
>>> +
>>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> +       if (IS_ERR(bsp_priv->grf)) {
>>> +               pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> +               return;
>>> +       }
>>> +
>>> +       if (speed == 10) {
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                            GMAC_RMII_CLK_2_5M);
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                            GMAC_SPEED_10M);
>>
>> combine into one write?
>>
>>
>>> +       } else if (speed == 100) {
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                            GMAC_RMII_CLK_25M);
>>> +               regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> +                            GMAC_SPEED_100M);
>>
>> combine into one write?
>>
>>
>>> +       } else {
>>> +               pr_err("unknown speed value for RMII! speed=%d", speed);
>>> +       }
>>> +}
>>> +
>>> +#define MAC_CLK_RX     "mac_clk_rx"
>>> +#define MAC_CLK_TX     "mac_clk_tx"
>>> +#define CLK_MAC_REF    "clk_mac_ref"
>>> +#define CLK_MAC_REF_OUT        "clk_mac_refout"
>>> +#define CLK_MAC_PLL    "clk_mac_pll"
>>> +#define ACLK_MAC       "aclk_mac"
>>> +#define PCLK_MAC       "pclk_mac"
>>> +#define MAC_CLKIN      "ext_gmac"
>>> +#define CLK_MAC                "stmmaceth"
>>
>> why the need to extra constants for the clock names and not use the real
>> names
>> directly like most other drivers do?
>>
>>
>>> +
>>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
>>> +{
>>> +       struct device *dev = &bsp_priv->pdev->dev;
>>> +
>>> +       bsp_priv->clk_enabled = false;
>>> +
>>> +       bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>>> +       if (IS_ERR(bsp_priv->mac_clk_rx))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, MAC_CLK_RX);
>>> +
>>> +       bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>>> +       if (IS_ERR(bsp_priv->mac_clk_tx))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, MAC_CLK_TX);
>>> +
>>> +       bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>>> +       if (IS_ERR(bsp_priv->clk_mac_ref))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, CLK_MAC_REF);
>>> +
>>> +       bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>>> +       if (IS_ERR(bsp_priv->clk_mac_refout))
>>> +               pr_warn("%s: warning:cannot get clock %s\n",
>>> +                       __func__, CLK_MAC_REF_OUT);
>>> +
>>> +       bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>>> +       if (IS_ERR(bsp_priv->aclk_mac))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, ACLK_MAC);
>>> +
>>> +       bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>>> +       if (IS_ERR(bsp_priv->pclk_mac))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, PCLK_MAC);
>>> +
>>> +       bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>>> +       if (IS_ERR(bsp_priv->clk_mac_pll))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, CLK_MAC_PLL);
>>> +
>>> +       bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>>> +       if (IS_ERR(bsp_priv->gmac_clkin))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, MAC_CLKIN);
>>> +
>>> +       bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>>> +       if (IS_ERR(bsp_priv->clk_mac))
>>> +               pr_warn("%s: warning: cannot get clock %s\n",
>>> +                       __func__, CLK_MAC);
>>
>> there is not clk_put in the _remove case ... maybe you could simply use
>> devm_clk_get here so that all clocks are put on device removal.
>>
>> Also you're warning on every missing clock. Below it looks like you need a
>> different set of them for rgmii and rmii, so maybe you should simply error
>> out
>> when core clocks for the selected phy-mode are missing.
>>
>>
>>> +
>>> +       if (bsp_priv->clock_input) {
>>> +               pr_info("%s: clock input from PHY\n", __func__);
>>> +       } else {
>>> +               if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> +                       clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>>> +
>>> +               clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
>>
>> why the explicit reparenting. The common clock-framework is intelligent
>> enough
>> to select the best suitable parent.
>>
>> In general I'm thinking the clock-handling inside this driver should be
>> simplyfied, as the common-clock framework can handle most cases itself.
>> I.e. if
>> a 125MHz external clock is present and so on. But haven't looked to deep
>> yet.
>>
>>
>>
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +       int phy_iface = phy_iface = bsp_priv->phy_iface;
>>> +
>>> +       if (enable) {
>>> +               if (!bsp_priv->clk_enabled) {
>>> +                       if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> +                               if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> +                                       clk_prepare_enable(
>>> +                                               bsp_priv->mac_clk_rx);
>>> +
>>> +                               if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> +                                       clk_prepare_enable(
>>> +                                               bsp_priv->clk_mac_ref);
>>> +
>>> +                               if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> +                                       clk_prepare_enable(
>>> +
>>> bsp_priv->clk_mac_refout);
>>> +                       }
>>> +
>>> +                       if (!IS_ERR(bsp_priv->aclk_mac))
>>> +                               clk_prepare_enable(bsp_priv->aclk_mac);
>>> +
>>> +                       if (!IS_ERR(bsp_priv->pclk_mac))
>>> +                               clk_prepare_enable(bsp_priv->pclk_mac);
>>> +
>>> +                       if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> +                               clk_prepare_enable(bsp_priv->mac_clk_tx);
>>> +
>>> +                       /**
>>> +                        * if (!IS_ERR(bsp_priv->clk_mac))
>>> +                        *      clk_prepare_enable(bsp_priv->clk_mac);
>>> +                        */
>>> +                       mdelay(5);
>>> +                       bsp_priv->clk_enabled = true;
>>> +               }
>>> +       } else {
>>> +               if (bsp_priv->clk_enabled) {
>>> +                       if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> +                               if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> +                                       clk_disable_unprepare(
>>> +                                               bsp_priv->mac_clk_rx);
>>> +
>>> +                               if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> +                                       clk_disable_unprepare(
>>> +                                               bsp_priv->clk_mac_ref);
>>> +
>>> +                               if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> +                                       clk_disable_unprepare(
>>> +
>>> bsp_priv->clk_mac_refout);
>>> +                       }
>>> +
>>> +                       if (!IS_ERR(bsp_priv->aclk_mac))
>>> +
>>> clk_disable_unprepare(bsp_priv->aclk_mac);
>>> +
>>> +                       if (!IS_ERR(bsp_priv->pclk_mac))
>>> +
>>> clk_disable_unprepare(bsp_priv->pclk_mac);
>>> +
>>> +                       if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> +
>>> clk_disable_unprepare(bsp_priv->mac_clk_tx);
>>> +                       /**
>>> +                        * if (!IS_ERR(bsp_priv->clk_mac))
>>> +                        *      clk_disable_unprepare(bsp_priv->clk_mac);
>>> +                        */
>>> +                       bsp_priv->clk_enabled = false;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +       struct regulator *ldo;
>>> +       char *ldostr = bsp_priv->pmu_regulator;
>>> +       int ret;
>>> +
>>> +       if (!ldostr) {
>>> +               pr_err("%s: no ldo found\n", __func__);
>>> +               return -1;
>>> +       }
>>> +
>>> +       ldo = regulator_get(NULL, ldostr);
>>> +       if (!ldo) {
>>> +               pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>>> +       } else {
>>> +               if (enable) {
>>> +                       if (!regulator_is_enabled(ldo)) {
>>> +                               regulator_set_voltage(ldo, 3300000,
>>> 3300000);
>>> +                               ret = regulator_enable(ldo);
>>> +                               if (ret != 0)
>>> +                                       pr_err("%s: fail to enable %s\n",
>>> +                                              __func__, ldostr);
>>> +                               else
>>> +                                       pr_info("turn on ldo done.\n");
>>> +                       } else {
>>> +                               pr_warn("%s is enabled before enable",
>>> ldostr);
>>> +                       }
>>> +               } else {
>>> +                       if (regulator_is_enabled(ldo)) {
>>> +                               ret = regulator_disable(ldo);
>>> +                               if (ret != 0)
>>> +                                       pr_err("%s: fail to disable
>>> %s\n",
>>> +                                              __func__, ldostr);
>>> +                               else
>>> +                                       pr_info("turn off ldo done.\n");
>>> +                       } else {
>>> +                               pr_warn("%s is disabled before disable",
>>> +                                       ldostr);
>>> +                       }
>>> +               }
>>> +               regulator_put(ldo);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +       if (enable) {
>>> +               /*power on*/
>>> +               if (gpio_is_valid(bsp_priv->power_io))
>>> +                       gpio_direction_output(bsp_priv->power_io,
>>> +                                             bsp_priv->power_io_level);
>>> +       } else {
>>> +               /*power off*/
>>> +               if (gpio_is_valid(bsp_priv->power_io))
>>> +                       gpio_direction_output(bsp_priv->power_io,
>>> +                                             !bsp_priv->power_io_level);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> +       int ret = -1;
>>> +
>>> +       pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>>> +
>>> +       if (bsp_priv->power_ctrl_by_pmu)
>>> +               ret = power_on_by_pmu(bsp_priv, enable);
>>> +       else
>>> +               ret =  power_on_by_gpio(bsp_priv, enable);
>>
>> this looks wrong. This should always be a regulator. Even a regulator +
>> switch
>> controlled by a gpio can still be modelled as regulator, so that you don't
>> need this switch and assorted special handling - so just use the regulator
>> API
>>
> In some case, it would be a switching circuit to control the power for PHY.
> All I need to do is to control a GPIO to make switch on/off.  So...

A regulator would probably be a better choice. You can use fixed-regulator.
The regulator framework should also take care of enabling any upstream
regulators, such as an LDO output from the PMIC that powers some external
pull-ups or what not.

The sunxi glue driver has this support. Maybe you could move that to the
stmmac platform core.

>>>
>>> +
>>> +       if (enable) {
>>> +               /*reset*/
>>> +               if (gpio_is_valid(bsp_priv->reset_io)) {
>>> +                       gpio_direction_output(bsp_priv->reset_io,
>>> +                                             bsp_priv->reset_io_level);
>>> +                       mdelay(5);
>>> +                       gpio_direction_output(bsp_priv->reset_io,
>>> +                                             !bsp_priv->reset_io_level);
>>> +               }
>>> +               mdelay(30);
>>> +
>>> +       } else {
>>> +               /*pull down reset*/
>>> +               if (gpio_is_valid(bsp_priv->reset_io)) {
>>> +                       gpio_direction_output(bsp_priv->reset_io,
>>> +                                             bsp_priv->reset_io_level);
>>> +               }
>>> +       }
>>
>> I'm not sure yet if it would be better to use the reset framework for
>> this.
>> While it says it is also meant for reset-gpios, there does not seem a
>> driver
>> for this to exist yet.
>>
> What should I do?

The stmmac driver has support for handling phy resets using gpios.
Please use it. See Documentation/devicetree/bindings/net/stmmac.txt
for details. I think this was discussed some time ago, though
probably in a different context. If it's just a gpio that needs to
be poked, it should stay a gpio, at least for external resets.

>
>>
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +#define GPIO_PHY_POWER "gmac_phy_power"
>>> +#define GPIO_PHY_RESET "gmac_phy_reset"
>>> +#define GPIO_PHY_IRQ   "gmac_phy_irq"
>>
>> again I don't understand why these constants are necessary
>>
>>> +
>>> +static void *rk_gmac_setup(struct platform_device *pdev)
>>> +{
>>> +       struct rk_priv_data *bsp_priv;
>>> +       struct device *dev = &pdev->dev;
>>> +       enum of_gpio_flags flags;
>>> +       int ret;
>>> +       const char *strings = NULL;
>>> +       int value;
>>> +       int irq;
>>> +
>>> +       bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>>> +       if (!bsp_priv)
>>> +               return ERR_PTR(-ENOMEM);
>>> +
>>> +       bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>> +
>>> +       ret = of_property_read_string(dev->of_node, "pmu_regulator",
>>> &strings);
>>> +       if (ret) {
>>> +               pr_err("%s: Can not read property: pmu_regulator.\n",
>>> __func__);
>>> +               bsp_priv->power_ctrl_by_pmu = false;
>>> +       } else {
>>> +               pr_info("%s: ethernet phy power controlled by
>>> pmu(%s).\n",
>>> +                       __func__, strings);
>>> +               bsp_priv->power_ctrl_by_pmu = true;
>>> +               strcpy(bsp_priv->pmu_regulator, strings);
>>> +       }
>>
>> There is a generic regulator-dt-binding for regulator-consumers available
>> which you should of course use.
>>
> The same explanation as above
>
>>> +
>>> +       ret = of_property_read_u32(dev->of_node, "pmu_enable_level",
>>> &value);
>>> +       if (ret) {
>>> +               pr_err("%s: Can not read property: pmu_enable_level.\n",
>>> +                      __func__);
>>> +               bsp_priv->power_ctrl_by_pmu = false;
>>> +       } else {
>>> +               pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>>> +                       __func__, (value == 1) ? "HIGH" : "LOW");
>>> +               bsp_priv->power_ctrl_by_pmu = true;
>>> +               bsp_priv->pmu_enable_level = value;
>>> +       }
>>
>> What is this used for? Enabling should of course be done via
>> regulator_enable
>> and disabling using regulator_disable.

Second. Even if it's handled by the PMU, you should also model the
PMU as a set of regulators. That will get rid of all the ifs in
this section.

>>
>>
>>> +
>>> +       ret = of_property_read_string(dev->of_node, "clock_in_out",
>>> &strings);
>>> +       if (ret) {
>>> +               pr_err("%s: Can not read property: clock_in_out.\n",
>>> __func__);
>>> +               bsp_priv->clock_input = true;
>>> +       } else {
>>> +               pr_info("%s: clock input or output? (%s).\n",
>>> +                       __func__, strings);
>>> +               if (!strcmp(strings, "input"))
>>> +                       bsp_priv->clock_input = true;
>>> +               else
>>> +                       bsp_priv->clock_input = false;
>>> +       }
>>> +
>>> +       ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>>> +       if (ret) {
>>> +               bsp_priv->tx_delay = 0x30;
>>> +               pr_err("%s: Can not read property: tx_delay.", __func__);
>>> +               pr_err("%s: set tx_delay to 0x%x\n",
>>> +                      __func__, bsp_priv->tx_delay);
>>> +       } else {
>>> +               pr_info("%s: TX delay(0x%x).\n", __func__, value);
>>> +               bsp_priv->tx_delay = value;
>>> +       }
>>> +
>>> +       ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>>> +       if (ret) {
>>> +               bsp_priv->rx_delay = 0x10;
>>> +               pr_err("%s: Can not read property: rx_delay.", __func__);
>>> +               pr_err("%s: set rx_delay to 0x%x\n",
>>> +                      __func__, bsp_priv->rx_delay);
>>> +       } else {
>>> +               pr_info("%s: RX delay(0x%x).\n", __func__, value);
>>> +               bsp_priv->rx_delay = value;
>>> +       }
>>> +
>>> +       bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> +                                                       "rockchip,grf");
>>> +       bsp_priv->phyirq_io =
>>> +               of_get_named_gpio_flags(dev->of_node,
>>> +                                       "phyirq-gpio", 0, &flags);
>>> +       bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +       bsp_priv->reset_io =
>>> +               of_get_named_gpio_flags(dev->of_node,
>>> +                                       "reset-gpio", 0, &flags);
>>> +       bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +       bsp_priv->power_io =
>>> +               of_get_named_gpio_flags(dev->of_node, "power-gpio", 0,
>>> &flags);
>>> +       bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +       /*power*/
>>> +       if (!gpio_is_valid(bsp_priv->power_io)) {
>>> +               pr_err("%s: Failed to get GPIO %s.\n",
>>> +                      __func__, "power-gpio");
>>> +       } else {
>>> +               ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>>> +               if (ret)
>>> +                       pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> +                              __func__, GPIO_PHY_POWER);
>>> +       }
>>
>> When everything power-related is handled using the regulator api, you
>> don't
>> need this
>
> The same explanation as above
>>
>>
>>> +
>>> +       if (!gpio_is_valid(bsp_priv->reset_io)) {
>>> +               pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>>> +       } else {
>>> +               ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>>> +               if (ret)
>>> +                       pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> +                              __func__, GPIO_PHY_RESET);
>>> +       }
>>> +
>>> +       if (bsp_priv->phyirq_io > 0) {
>>
>> This is more for my understanding: why does the mac driver need to handle
>> the
>> phy interrupt - but I might be overlooking something.

stmmac does not have a separate mdio bus driver. They are combined.

>>
> phy interrupt is not mandatory.  In most of the time,  in order to find
> something happen in PHY, for example,
> link is up or down,  we just use polling method to read the phy's register
> in a timer.
> Buf if phy interrupt is in use, when link is up or down,  phy interrupt pin
> will be assert to inform the CPU.
> I just implement the driver for phy interrupt gpio,  not enable it as
> default.

You should probably do it in the stmmac platform/core driver.

>
>
>>> +               struct plat_stmmacenet_data *plat_dat;
>>> +
>>> +               pr_info("PHY irq in use\n");
>>> +               ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>>> +               if (ret < 0) {
>>> +                       pr_warn("%s: Failed to request GPIO %s\n",
>>> +                               __func__, GPIO_PHY_IRQ);
>>> +                       goto goon;
>>> +               }
>>> +
>>> +               ret = gpio_direction_input(bsp_priv->phyirq_io);
>>> +               if (ret < 0) {
>>> +                       pr_err("%s, Failed to set input for GPIO %s\n",
>>> +                              __func__, GPIO_PHY_IRQ);
>>> +                       gpio_free(bsp_priv->phyirq_io);
>>> +                       goto goon;
>>> +               }
>>> +
>>> +               irq = gpio_to_irq(bsp_priv->phyirq_io);
>>> +               if (irq < 0) {
>>> +                       ret = irq;
>>> +                       pr_err("Failed to set irq for %s\n",
>>> +                              GPIO_PHY_IRQ);
>>> +                       gpio_free(bsp_priv->phyirq_io);
>>> +                       goto goon;
>>> +               }
>>> +
>>> +               plat_dat = dev_get_platdata(&pdev->dev);
>>> +               if (plat_dat)
>>> +                       plat_dat->mdio_bus_data->probed_phy_irq = irq;
>>> +               else
>>> +                       pr_err("%s: plat_data is NULL\n", __func__);

The glue layer should never ever touch the platform data. If you need
to add interrupts for the phy, please do so in the stmmac driver proper,
and add the proper DT bindings/

>>> +       }
>>> +
>>> +goon:
>>> +       /*rmii or rgmii*/
>>> +       if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>>> +               pr_info("%s: init for RGMII\n", __func__);
>>> +               set_to_rgmii(bsp_priv, bsp_priv->tx_delay,
>>> bsp_priv->rx_delay);
>>> +       } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> +               pr_info("%s: init for RMII\n", __func__);
>>> +               set_to_rmii(bsp_priv);
>>> +       } else {
>>> +               pr_err("%s: ERROR: NO interface defined!\n", __func__);
>>> +       }
>>> +
>>> +       bsp_priv->pdev = pdev;
>>> +
>>> +       gmac_clk_init(bsp_priv);
>>> +
>>> +       return bsp_priv;
>>> +}
>>> +
>>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>>> +{
>>> +       struct rk_priv_data *bsp_priv = priv;
>>> +       int ret;
>>> +
>>> +       ret = phy_power_on(bsp_priv, true);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       ret = gmac_clk_enable(bsp_priv, true);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>>> +{
>>> +       struct rk_priv_data *gmac = priv;
>>> +
>>> +       phy_power_on(gmac, false);
>>> +       gmac_clk_enable(gmac, false);
>>> +}
>>> +
>>> +static void rk_fix_speed(void *priv, unsigned int speed)
>>> +{
>>> +       struct rk_priv_data *bsp_priv = priv;
>>> +
>>> +       if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>>> +               set_rgmii_speed(bsp_priv, speed);
>>> +       else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> +               set_rmii_speed(bsp_priv, speed);
>>> +       else
>>> +               pr_err("unsupported interface %d", bsp_priv->phy_iface);
>>> +}
>>> +
>>> +const struct stmmac_of_data rk_gmac_data = {
>>> +       .has_gmac = 1,
>>> +       .fix_mac_speed = rk_fix_speed,
>>> +       .setup = rk_gmac_setup,
>>> +       .init = rk_gmac_init,
>>> +       .exit = rk_gmac_exit,
>>> +};
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
>>> 15814b7..b4dee96 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -33,6 +33,7 @@
>>>
>>>   static const struct of_device_id stmmac_dt_ids[] = {
>>>         /* SoC specific glue layers should come before generic bindings
>>> */
>>> +       { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
>>
>> please name that rk3288_gmac_data [of course the other occurences too]
>> It makes it easier to see which soc it is meant for and it's also not save
>> to
>> assume that the next one will use the same register + bit positions in the
>> grf.
>>
>>
>>>         { .compatible = "amlogic,meson6-dwmac", .data =
>>> &meson6_dwmac_data},
>>>         { .compatible = "allwinner,sun7i-a20-gmac", .data =
>>> &sun7i_gmac_data},
>>>         { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct platform_device
>>> *pdev) return  -ENOMEM;
>>>                 }
>>>
>>> +               pdev->dev.platform_data = plat_dat;
>>> +

When we re-did the DT layer for stmmac, there was consensus to _not_
use platform_data. Please do not use it.

>>>                 ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>>                 if (ret) {
>>>                         pr_err("%s: main dt probe failed", __func__);
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
>>> 25dd1f7..32a0516 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>>   extern const struct stmmac_of_data stih4xx_dwmac_data;
>>>   extern const struct stmmac_of_data stid127_dwmac_data;
>>>   extern const struct stmmac_of_data socfpga_gmac_data;
>>> +extern const struct stmmac_of_data rk_gmac_data;
>>>
>>>   #endif /* __STMMAC_PLATFORM_H__ */
>>
>>
>>

I may have repeated myself a few times. Please take a look at how
the other glue layers are written. This one is larger than it needs
to be.

Regards,
ChenYu

[PATCH v2 2/6] GMAC: define clock ID used for GMAC

[Roger Chen]

Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
---
 include/dt-bindings/clock/rk3288-cru.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/clock/rk3288-cru.h b/include/dt-bindings/clock/rk3288-cru.h
index 100a08c..f9496f5 100644
--- a/include/dt-bindings/clock/rk3288-cru.h
+++ b/include/dt-bindings/clock/rk3288-cru.h
@@ -72,6 +72,10 @@
 #define SCLK_HEVC_CABAC		111
 #define SCLK_HEVC_CORE		112
 
+#define SCLK_MAC_PLL		150
+#define SCLK_MAC		151
+#define SCLK_MACREF_OUT		152
+
 #define DCLK_VOP0		190
 #define DCLK_VOP1		191
 
-- 
1.7.9.5

Re: [PATCH v2 2/6] GMAC: define clock ID used for GMAC

[Heiko Stübner]

Hi Roger,

patches modifying the rockchip clock parts should have a subject like
	clk: rockchip: .....

Same for the following patch

Am Donnerstag, 27. November 2014, 10:52:46 schrieb Roger Chen:
> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
> ---
>  include/dt-bindings/clock/rk3288-cru.h |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/dt-bindings/clock/rk3288-cru.h
> b/include/dt-bindings/clock/rk3288-cru.h index 100a08c..f9496f5 100644
> --- a/include/dt-bindings/clock/rk3288-cru.h
> +++ b/include/dt-bindings/clock/rk3288-cru.h
> @@ -72,6 +72,10 @@
>  #define SCLK_HEVC_CABAC		111
>  #define SCLK_HEVC_CORE		112
> 
> +#define SCLK_MAC_PLL		150

Why do you need to export the mac pll source selection?
I understand SCLK_MAC below, as you will want to select ext_gmac explicitly if 
available. But otherwise, why would you need to explicitly select the pll 
source?

The clock framework is intelligent enough that when you do a 
clk_set_rate(sclk_mac, 50000000) for example that it will select the best pll 
source + best divider to provide the most accurate frequency for this, so 
there should be no need to handle the pll source manually.


Heiko

> +#define SCLK_MAC		151
> +#define SCLK_MACREF_OUT		152
> +
>  #define DCLK_VOP0		190
>  #define DCLK_VOP1		191

[PATCH v2 3/6] GMAC: modify CRU config for Rockchip RK3288 SoCs integrated GMAC

[Roger Chen]

modify CRU config for GMAC driver

Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
---
 drivers/clk/rockchip/clk-rk3288.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 2327829..60237dc 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -187,7 +187,7 @@ PNAME(mux_uart2_p)	= { "uart2_src", "uart2_frac", "xin24m" };
 PNAME(mux_uart3_p)	= { "uart3_src", "uart3_frac", "xin24m" };
 PNAME(mux_uart4_p)	= { "uart4_src", "uart4_frac", "xin24m" };
 PNAME(mux_cif_out_p)	= { "cif_src", "xin24m" };
-PNAME(mux_macref_p)	= { "mac_src", "ext_gmac" };
+PNAME(mux_mac_p)	= { "mac_pll_src", "ext_gmac" };
 PNAME(mux_hsadcout_p)	= { "hsadc_src", "ext_hsadc" };
 PNAME(mux_edp_24m_p)	= { "ext_edp_24m", "xin24m" };
 PNAME(mux_tspout_p)	= { "cpll", "gpll", "npll", "xin27m" };
@@ -560,18 +560,18 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 	MUX(SCLK_UART4, "sclk_uart4", mux_uart4_p, 0,
 			RK3288_CLKSEL_CON(3), 8, 2, MFLAGS),
 
-	COMPOSITE(0, "mac_src", mux_pll_src_npll_cpll_gpll_p, 0,
+	COMPOSITE(SCLK_MAC_PLL, "mac_pll_src", mux_pll_src_npll_cpll_gpll_p, 0,
 			RK3288_CLKSEL_CON(21), 0, 2, MFLAGS, 8, 5, DFLAGS,
 			RK3288_CLKGATE_CON(2), 5, GFLAGS),
-	MUX(0, "macref", mux_macref_p, 0,
+	MUX(SCLK_MAC, "mac_clk", mux_mac_p, 0,
 			RK3288_CLKSEL_CON(21), 4, 1, MFLAGS),
-	GATE(0, "sclk_macref_out", "macref", 0,
+	GATE(SCLK_MACREF_OUT, "sclk_macref_out", "mac_clk", 0,
 			RK3288_CLKGATE_CON(5), 3, GFLAGS),
-	GATE(SCLK_MACREF, "sclk_macref", "macref", 0,
+	GATE(SCLK_MACREF, "sclk_macref", "mac_clk", 0,
 			RK3288_CLKGATE_CON(5), 2, GFLAGS),
-	GATE(SCLK_MAC_RX, "sclk_mac_rx", "macref", 0,
+	GATE(SCLK_MAC_RX, "sclk_mac_rx", "mac_clk", 0,
 			RK3288_CLKGATE_CON(5), 0, GFLAGS),
-	GATE(SCLK_MAC_TX, "sclk_mac_tx", "macref", 0,
+	GATE(SCLK_MAC_TX, "sclk_mac_tx", "mac_clk", 0,
 			RK3288_CLKGATE_CON(5), 1, GFLAGS),
 
 	COMPOSITE(0, "hsadc_src", mux_pll_src_cpll_gpll_p, 0,
-- 
1.7.9.5

[PATCH v2 4/6] ARM: dts: rockchip: add gmac info for rk3288

[Roger Chen]

add gmac info in rk3288.dtsi for GMAC driver

Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
---
 arch/arm/boot/dts/rk3288.dtsi |   49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 0f50d5d..22f7019 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -358,6 +358,22 @@
 		status = "disabled";
 	};
 
+	gmac: ethernet@ff290000 {
+		compatible = "rockchip,rk3288-gmac";
+		reg = <0xff290000 0x10000>;
+		interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "macirq";
+		rockchip,grf = <&grf>;
+		clocks = <&cru SCLK_MAC>, <&cru SCLK_MAC_PLL>,
+			<&cru SCLK_MAC_RX>, <&cru SCLK_MAC_TX>,
+			<&cru SCLK_MACREF>, <&cru SCLK_MACREF_OUT>,
+			<&cru ACLK_GMAC>, <&cru PCLK_GMAC>;
+		clock-names = "stmmaceth", "clk_mac_pll",
+			"mac_clk_rx", "mac_clk_tx",
+			"clk_mac_ref", "clk_mac_refout",
+			"aclk_mac", "pclk_mac";
+	};
+
 	usb_host0_ehci: usb@ff500000 {
 		compatible = "generic-ehci";
 		reg = <0xff500000 0x100>;
@@ -1040,5 +1056,38 @@
 				rockchip,pins = <7 23 3 &pcfg_pull_none>;
 			};
 		};
+
+		gmac {
+			rgmii_pins: rgmii-pins {
+				rockchip,pins = <3 30 3 &pcfg_pull_none>,
+						<3 31 3 &pcfg_pull_none>,
+						<3 26 3 &pcfg_pull_none>,
+						<3 27 3 &pcfg_pull_none>,
+						<3 28 3 &pcfg_pull_none>,
+						<3 29 3 &pcfg_pull_none>,
+						<3 24 3 &pcfg_pull_none>,
+						<3 25 3 &pcfg_pull_none>,
+						<4 0 3 &pcfg_pull_none>,
+						<4 5 3 &pcfg_pull_none>,
+						<4 6 3 &pcfg_pull_none>,
+						<4 9 3 &pcfg_pull_none>,
+						<4 4 3 &pcfg_pull_none>,
+						<4 1 3 &pcfg_pull_none>,
+						<4 3 3 &pcfg_pull_none>;
+			};
+
+			rmii_pins: rmii-pins {
+				rockchip,pins = <3 30 3 &pcfg_pull_none>,
+						<3 31 3 &pcfg_pull_none>,
+						<3 28 3 &pcfg_pull_none>,
+						<3 29 3 &pcfg_pull_none>,
+						<4 0 3 &pcfg_pull_none>,
+						<4 5 3 &pcfg_pull_none>,
+						<4 4 3 &pcfg_pull_none>,
+						<4 1 3 &pcfg_pull_none>,
+						<4 2 3 &pcfg_pull_none>,
+						<4 3 3 &pcfg_pull_none>;
+			};
+		};
 	};
 };
-- 
1.7.9.5

[PATCH v2 5/6] ARM: dts: rockchip: enable gmac on RK3288 evb board

[Roger Chen]

enable gmac in rk3288-evb-rk808.dts

Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
---
 arch/arm/boot/dts/rk3288-evb-rk808.dts |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb-rk808.dts b/arch/arm/boot/dts/rk3288-evb-rk808.dts
index d8c775e6..13ad2d2 100644
--- a/arch/arm/boot/dts/rk3288-evb-rk808.dts
+++ b/arch/arm/boot/dts/rk3288-evb-rk808.dts
@@ -15,6 +15,13 @@
 
 / {
 	compatible = "rockchip,rk3288-evb-rk808", "rockchip,rk3288";
+
+	ext_gmac: external-gmac-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "ext_gmac";
+		#clock-cells = <0>;
+	};
 };
 
 &cpu0 {
@@ -152,3 +159,20 @@
 		};
 	};
 };
+
+&gmac {
+	//pmu_regulator = "act_ldo5";
+	//pmu_enable_level = <1>; //1->HIGH, 0->LOW
+	power-gpio = <&gpio0 6 GPIO_ACTIVE_HIGH>;
+	reset-gpio = <&gpio4 7 GPIO_ACTIVE_LOW>;
+	//phyirq-gpio = <&gpio4 2 GPIO_ACTIVE_LOW>;
+	phy-mode = "rgmii";
+	clock_in_out = "input";
+	assigned-clocks = <&cru SCLK_MAC>;
+	assigned-clock-parents = <&ext_gmac>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmii_pins>;
+	tx_delay = <0x30>;
+	rx_delay = <0x10>;
+	status = "ok";
+};
-- 
1.7.9.5

[PATCH v2 6/6] GMAC: add document for Rockchip RK3288 GMAC

[Roger Chen]

The document descripts how to add properties for GMAC in device tree.

Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
---
 .../devicetree/bindings/net/rockchip-dwmac.txt     |   71 ++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/rockchip-dwmac.txt

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
new file mode 100644
index 0000000..237442b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
@@ -0,0 +1,71 @@
+Rockchip SoC RK3288 10/100/1000 Ethernet driver(GMAC)
+
+The device node has following properties.
+
+Required properties:
+ - compatible: Can be "rockchip,rk3288-gmac".
+ - reg: addresses and length of the register sets for the device.
+ - interrupts: Should contain the GMAC interrupts.
+ - interrupt-names: Should contain the interrupt names "macirq".
+ - rockchip,grf: phandle to the syscon grf used to control speed and mode.
+ - clocks: <&cru SCLK_MAC>: clock selector for main clock, from PLL or PHY.
+	   <&cru SCLK_MAC_PLL>: PLL clock for SCLK_MAC
+	   <&cru SCLK_MAC_RX>: clock gate for RX
+	   <&cru SCLK_MAC_TX>: clock gate for TX
+	   <&cru SCLK_MACREF>: clock gate for RMII referce clock
+	   <&cru SCLK_MACREF_OUT> clock gate for RMII reference clock output
+	   <&cru ACLK_GMAC>: AXI clock gate for GMAC 
+	   <&cru PCLK_GMAC>: APB clock gate for GMAC 
+ - clock-names: One name for each entry in the clocks property.
+ - phy-mode: See ethernet.txt file in the same directory.
+ - pinctrl-names: Names corresponding to the numbered pinctrl states.
+ - pinctrl-0: pin-control mode. can be <&rgmii_pins> or <&rmii_pins>.
+ - clock_in_out: For RGMII, it must be "input", means main clock(125MHz)
+   is not sourced from SoC's PLL, but input from PHY; For RMII, "input" means
+   PHY provides the reference clock(50MHz), "output" means GMAC provides the
+   reference clock. 
+ - assigned-clocks: main clock, should be <&cru SCLK_MAC>;
+ - assigned-clock-parents = parent of main clock.
+   can be <&ext_gmac> or <&cru SCLK_MAC_PLL>.
+ - reset-gpio: GPIO for reset
+
+Optional properties:
+ - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as default.
+ - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as default.
+ - pmu_regulator: PMIC's integrated LDO power for PHY. Can be "act_ldo5".
+ - pmu_enable_level: Enable level of LDO. Can be <1> or <0>. 1->HIGH, 0->LOW.
+ - power-gpio: GPIO used to control PHY power. Normally,
+   power-gpio and pmu_regulator can not be used at the same time.
+ - phyirq-gpio: GPIO used as PHY irq.
+
+Example:
+
+gmac: ethernet@ff290000 {
+	compatible = "rockchip,rk3288-gmac";
+	reg = <0xff290000 0x10000>;
+	interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+	interrupt-names = "macirq";
+	rockchip,grf = <&grf>;
+	clocks = <&cru SCLK_MAC>, <&cru SCLK_MAC_PLL>,
+		<&cru SCLK_MAC_RX>, <&cru SCLK_MAC_TX>,
+		<&cru SCLK_MACREF>, <&cru SCLK_MACREF_OUT>,
+		<&cru ACLK_GMAC>, <&cru PCLK_GMAC>;
+	clock-names = "stmmaceth", "clk_mac_pll",
+		"mac_clk_rx", "mac_clk_tx",
+		"clk_mac_ref", "clk_mac_refout",
+		"aclk_mac", "pclk_mac";
+	phy-mode = "rgmii";
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmii_pins /*&rmii_pins*/>;
+
+	clock_in_out = "input";
+	assigned-clocks = <&cru SCLK_MAC>;
+	assigned-clock-parents = <&ext_gmac>;
+	tx_delay = <0x30>;
+	rx_delay = <0x10>;
+
+        power-gpio = <&gpio0 6 GPIO_ACTIVE_HIGH>;
+        reset-gpio = <&gpio4 7 GPIO_ACTIVE_LOW>;
+
+	status = "ok";
+};
-- 
1.7.9.5