mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/2] net: designware: eqos: reset phy
@ 2021-06-07 14:10 Sascha Hauer
  2021-06-07 14:10 ` [PATCH 2/2] net: eqos: Rockchip support Sascha Hauer
  2021-06-07 15:59 ` [PATCH 1/2] net: designware: eqos: reset phy Ahmad Fatoum
  0 siblings, 2 replies; 8+ messages in thread
From: Sascha Hauer @ 2021-06-07 14:10 UTC (permalink / raw)
  To: Barebox List

The designware eqos DT binding has support for specifying reset GPIOs.
Add support for them.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
 drivers/of/of_gpio.c          |  7 +++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
index d2baaeaf63..0321024169 100644
--- a/drivers/net/designware_eqos.c
+++ b/drivers/net/designware_eqos.c
@@ -8,9 +8,11 @@
 
 #include <common.h>
 #include <init.h>
+#include <gpio.h>
 #include <dma.h>
 #include <net.h>
 #include <of_net.h>
+#include <of_gpio.h>
 #include <linux/iopoll.h>
 #include <linux/time.h>
 #include <linux/sizes.h>
@@ -189,6 +191,33 @@ struct eqos_desc {
 
 #define MII_BUSY		(1 << 0)
 
+static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
+{
+	int phy_reset;
+	int ret;
+	u32 delays[3] = { 0, 0, 0 };
+
+	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
+
+        if (!gpio_is_valid(phy_reset))
+		return 0;
+
+	ret = gpio_request(phy_reset, "phy-reset");
+	if (ret)
+		return ret;
+
+	of_property_read_u32_array(dev->device_node,
+				   "snps,reset-delays-us",
+				   delays, ARRAY_SIZE(delays));
+
+	gpio_direction_active(phy_reset, 0);
+	udelay(delays[1]);
+	gpio_set_active(phy_reset, 1);
+	udelay(delays[2]);
+
+	return 0;
+}
+
 static int eqos_mdio_wait_idle(struct eqos *eqos)
 {
 	u32 idle;
@@ -843,6 +872,10 @@ int eqos_probe(struct device_d *dev, const struct eqos_ops *ops, void *priv)
 	if (ret)
 		return ret;
 
+	ret = eqos_phy_reset(dev, eqos);
+	if (ret)
+		return ret;
+
 	ret = mdiobus_register(miibus);
 	if (ret)
 		return ret;
diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
index 7cbeeaf69e..e1cafdc848 100644
--- a/drivers/of/of_gpio.c
+++ b/drivers/of/of_gpio.c
@@ -35,6 +35,13 @@ static void of_gpio_flags_quirks(struct device_node *np,
 		if (active_low)
 			*flags |= OF_GPIO_ACTIVE_LOW;
 	}
+
+	/* Legacy handling of stmmac's active-low PHY reset line */
+	if (IS_ENABLED(CONFIG_DRIVER_NET_DESIGNWARE_EQOS) &&
+	    !strcmp(propname, "snps,reset-gpio") &&
+	    of_property_read_bool(np, "snps,reset-active-low"))
+		*flags |= OF_GPIO_ACTIVE_LOW;
+
 }
 
 /**
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] net: eqos: Rockchip support
  2021-06-07 14:10 [PATCH 1/2] net: designware: eqos: reset phy Sascha Hauer
@ 2021-06-07 14:10 ` Sascha Hauer
  2021-06-07 16:05   ` Ahmad Fatoum
  2021-06-07 15:59 ` [PATCH 1/2] net: designware: eqos: reset phy Ahmad Fatoum
  1 sibling, 1 reply; 8+ messages in thread
From: Sascha Hauer @ 2021-06-07 14:10 UTC (permalink / raw)
  To: Barebox List

A variant of the designware eqos core is used on Rockchip SoCs. Add
support for it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/Kconfig               |   6 +
 drivers/net/Makefile              |   1 +
 drivers/net/designware_rockchip.c | 311 ++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/net/designware_rockchip.c

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 0d55ea7a3b..18931211b5 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -108,6 +108,12 @@ config DRIVER_NET_DESIGNWARE_TEGRA186
 	help
 	  This option enables support for the ethernet MAC on the Tegra186 & 194.
 
+config DRIVER_NET_DESIGNWARE_ROCKCHIP
+	bool "Designware Universal MAC ethernet driver for Rockchip platforms"
+	select MFD_SYSCON
+	help
+	  This option enables support for the ethernet MAC on different Rockchip SoCs
+
 endif
 
 config DRIVER_NET_DM9K
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 656d45a868..1674d53dff 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_DRIVER_NET_DESIGNWARE_SOCFPGA) += designware_socfpga.o
 obj-$(CONFIG_DRIVER_NET_DESIGNWARE_EQOS) += designware_eqos.o
 obj-$(CONFIG_DRIVER_NET_DESIGNWARE_STM32) += designware_stm32.o
 obj-$(CONFIG_DRIVER_NET_DESIGNWARE_TEGRA186) += designware_tegra186.o
+obj-$(CONFIG_DRIVER_NET_DESIGNWARE_ROCKCHIP) += designware_rockchip.o
 obj-$(CONFIG_DRIVER_NET_DM9K)		+= dm9k.o
 obj-$(CONFIG_DRIVER_NET_E1000)		+= e1000/regio.o e1000/main.o e1000/eeprom.o
 obj-$(CONFIG_DRIVER_NET_ENC28J60)	+= enc28j60.o
diff --git a/drivers/net/designware_rockchip.c b/drivers/net/designware_rockchip.c
new file mode 100644
index 0000000000..883c1d806d
--- /dev/null
+++ b/drivers/net/designware_rockchip.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <common.h>
+#include <init.h>
+#include <dma.h>
+#include <net.h>
+#include <regmap.h>
+#include <of_net.h>
+#include <mfd/syscon.h>
+#include <linux/iopoll.h>
+#include <linux/sizes.h>
+#include <linux/time.h>
+#include <linux/clk.h>
+
+#include "designware_eqos.h"
+
+struct rk_gmac_ops {
+	void (*set_to_rgmii)(struct eqos *eqos,
+			     int tx_delay, int rx_delay);
+	void (*set_to_rmii)(struct eqos *eqos);
+	void (*set_speed)(struct eqos *eqos, int speed);
+	void (*integrated_phy_powerup)(struct eqos *eqos);
+};
+
+struct eqos_rk_gmac {
+	struct clk_bulk_data *clks;
+	int num_clks;
+	bool clock_input;
+	const struct rk_gmac_ops *ops;
+	struct regmap *grf;
+	int bus_id;
+	u32 tx_delay;
+	u32 rx_delay;
+	struct device_d *dev;
+};
+
+enum {
+	CLK_STMMACETH,
+	CLK_MAC_RX,
+	CLK_MAC_TX,
+	CLK_MAC_REFOUT,
+	CLK_MAC_ACLK,
+	CLK_MAC_PCLK,
+	CLK_MAC_SPEED,
+	CLK_PTP_REF,
+	CLK_XPCS_PCLK,
+};
+
+static const struct clk_bulk_data rk_gmac_clks[] = {
+	[CLK_STMMACETH]   = { .id = "stmmaceth" },
+	[CLK_MAC_RX]      = { .id = "mac_clk_rx" },
+	[CLK_MAC_TX]      = { .id = "mac_clk_tx" },
+	[CLK_MAC_REFOUT]  = { .id = "clk_mac_refout" },
+	[CLK_MAC_ACLK]    = { .id = "aclk_mac" },
+	[CLK_MAC_PCLK]    = { .id = "pclk_mac" },
+	[CLK_MAC_SPEED]   = { .id = "clk_mac_speed" },
+	[CLK_PTP_REF]     = { .id = "ptp_ref" },
+	[CLK_XPCS_PCLK]   = { .id = "pclk_xpcs" },
+};
+
+static inline struct eqos_rk_gmac *to_rk_gmac(struct eqos *eqos)
+{
+	return eqos->priv;
+}
+
+#define HIWORD_UPDATE(val, mask, shift) \
+		((val) << (shift) | (mask) << ((shift) + 16))
+
+#define GRF_BIT(nr)	(BIT(nr) | BIT((nr) + 16))
+#define GRF_CLR_BIT(nr)	(BIT((nr) + 16))
+
+#define RK3568_GRF_GMAC0_CON0		0X0380
+#define RK3568_GRF_GMAC0_CON1		0X0384
+#define RK3568_GRF_GMAC1_CON0		0X0388
+#define RK3568_GRF_GMAC1_CON1		0X038c
+
+/* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
+#define RK3568_GMAC_PHY_INTF_SEL_RGMII	\
+		(GRF_BIT(4) | GRF_CLR_BIT(5) | GRF_CLR_BIT(6))
+#define RK3568_GMAC_PHY_INTF_SEL_RMII	\
+		(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | GRF_BIT(6))
+#define RK3568_GMAC_FLOW_CTRL			GRF_BIT(3)
+#define RK3568_GMAC_FLOW_CTRL_CLR		GRF_CLR_BIT(3)
+#define RK3568_GMAC_RXCLK_DLY_ENABLE		GRF_BIT(1)
+#define RK3568_GMAC_RXCLK_DLY_DISABLE		GRF_CLR_BIT(1)
+#define RK3568_GMAC_TXCLK_DLY_ENABLE		GRF_BIT(0)
+#define RK3568_GMAC_TXCLK_DLY_DISABLE		GRF_CLR_BIT(0)
+
+/* RK3568_GRF_GMAC0_CON0 && RK3568_GRF_GMAC1_CON0 */
+#define RK3568_GMAC_CLK_RX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 8)
+#define RK3568_GMAC_CLK_TX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 0)
+
+static unsigned long eqos_get_csr_clk_rate_rk_gmac(struct eqos *eqos)
+{
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+
+	return clk_get_rate(priv->clks[CLK_STMMACETH].clk);
+}
+
+static void rk3568_set_to_rgmii(struct eqos *eqos,
+				int tx_delay, int rx_delay)
+{
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+	struct device_d *dev = priv->dev;
+	u32 offset_con0, offset_con1;
+
+	if (IS_ERR(priv->grf)) {
+		dev_err(dev, "Missing rockchip,grf property\n");
+		return;
+	}
+
+	offset_con0 = (priv->bus_id == 1)
+		      ? RK3568_GRF_GMAC1_CON0 : RK3568_GRF_GMAC0_CON0;
+	offset_con1 = (priv->bus_id == 1)
+		      ? RK3568_GRF_GMAC1_CON1 : RK3568_GRF_GMAC0_CON1;
+
+	regmap_write(priv->grf, offset_con1,
+		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
+		     RK3568_GMAC_RXCLK_DLY_ENABLE |
+		     RK3568_GMAC_TXCLK_DLY_ENABLE);
+
+	regmap_write(priv->grf, offset_con0,
+		     RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
+		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
+}
+
+static void rk3568_set_to_rmii(struct eqos *eqos)
+{
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+	struct device_d *dev = priv->dev;
+	u32 offset_con1;
+
+	if (IS_ERR(priv->grf)) {
+		dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
+		return;
+	}
+
+	offset_con1 = (priv->bus_id == 1)
+		      ? RK3568_GRF_GMAC1_CON1 : RK3568_GRF_GMAC0_CON1;
+
+	regmap_write(priv->grf, offset_con1,
+		     RK3568_GMAC_PHY_INTF_SEL_RMII);
+}
+
+static void rk3568_set_gmac_speed(struct eqos *eqos, int speed)
+{
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+	struct device_d *dev = priv->dev;
+	unsigned long rate;
+	int ret;
+
+	switch (speed) {
+	case SPEED_10:
+		rate = 2500000;
+		break;
+	case SPEED_100:
+		rate = 25000000;
+		break;
+	case SPEED_1000:
+		rate = 125000000;
+		break;
+	default:
+		dev_err(dev, "unknown speed value for GMAC speed=%d", speed);
+		return;
+	}
+
+	ret = clk_set_rate(priv->clks[CLK_MAC_SPEED].clk, rate);
+	if (ret)
+		dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n",
+			__func__, rate, ret);
+}
+
+static const struct rk_gmac_ops rk3568_ops = {
+	.set_to_rgmii = rk3568_set_to_rgmii,
+	.set_to_rmii = rk3568_set_to_rmii,
+	.set_speed = rk3568_set_gmac_speed,
+};
+
+static int rk_gmac_powerup(struct eqos *eqos)
+{
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+	struct device_d *dev = priv->dev;
+
+	/*rmii or rgmii*/
+	switch (eqos->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		dev_dbg(dev, "init for RGMII\n");
+		priv->ops->set_to_rgmii(eqos, priv->tx_delay,
+					    priv->rx_delay);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		dev_dbg(dev, "init for RGMII_ID\n");
+		priv->ops->set_to_rgmii(eqos, 0, 0);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		dev_dbg(dev, "init for RGMII_RXID\n");
+		priv->ops->set_to_rgmii(eqos, priv->tx_delay, 0);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		dev_dbg(dev, "init for RGMII_TXID\n");
+		priv->ops->set_to_rgmii(eqos, 0, priv->rx_delay);
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		dev_dbg(dev, "init for RMII\n");
+		priv->ops->set_to_rmii(eqos);
+		break;
+	default:
+		dev_err(dev, "NO interface defined!\n");
+	}
+
+	return 0;
+}
+
+static void eqos_rk_adjust_link(struct eth_device *edev)
+{
+	struct eqos *eqos = edev->priv;
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+
+	priv->ops->set_speed(eqos, edev->phydev->speed);
+
+	eqos_adjust_link(edev);
+}
+
+static int eqos_init_rk_gmac(struct device_d *dev, struct eqos *eqos)
+{
+	struct device_node *np = dev->device_node;
+	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
+	int ret;
+	const char *strings;
+
+	priv->dev = dev;
+
+	ret = of_property_read_string(np, "clock_in_out", &strings);
+	if (ret) {
+                dev_err(dev, "Can not read property: clock_in_out.\n");
+                priv->clock_input = true;
+	} else {
+		dev_dbg(dev, "clock is %s\n", strings);
+		if (!strcmp(strings, "input"))
+			priv->clock_input = true;
+		else
+			priv->clock_input = false;
+	}
+
+	priv->ops = device_get_match_data(dev);
+
+	priv->bus_id = of_alias_get_id(np, "ethernet");
+
+	priv->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
+	if (IS_ERR(priv->grf)) {
+		dev_err(dev, "unable to get grf");
+		return PTR_ERR(priv->grf);
+	}
+
+	priv->tx_delay = 0x30;
+	of_property_read_u32(np, "tx_delay", &priv->tx_delay);
+	priv->rx_delay = 0x10;
+	of_property_read_u32(np, "rx_delay", &priv->rx_delay);
+
+	priv->num_clks = ARRAY_SIZE(rk_gmac_clks);
+	priv->clks = xmalloc(priv->num_clks * sizeof(*priv->clks));
+	memcpy(priv->clks, rk_gmac_clks, sizeof rk_gmac_clks);
+
+	ret = clk_bulk_get(dev, priv->num_clks, priv->clks);
+	if (ret) {
+		dev_err(dev, "Failed to get clks: %s\n", strerror(-ret));
+		return ret;
+	}
+
+	ret = clk_bulk_enable(priv->num_clks, priv->clks);
+	if (ret) {
+		dev_err(dev, "Failed to enable clks: %s\n", strerror(-ret));
+		return ret;
+	}
+
+	rk_gmac_powerup(eqos);
+
+	return 0;
+}
+
+static struct eqos_ops rk_gmac_ops = {
+	.init = eqos_init_rk_gmac,
+	.get_ethaddr = eqos_get_ethaddr,
+	.set_ethaddr = eqos_set_ethaddr,
+	.adjust_link = eqos_rk_adjust_link,
+	.get_csr_clk_rate = eqos_get_csr_clk_rate_rk_gmac,
+
+	.clk_csr = EQOS_MDIO_ADDR_CR_250_300,
+	.config_mac = EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_AV,
+};
+
+static int dwc_ether_probe(struct device_d *dev)
+{
+	return eqos_probe(dev, &rk_gmac_ops, xzalloc(sizeof(struct eqos_rk_gmac)));
+}
+
+static __maybe_unused struct of_device_id dwc_ether_compatible[] = {
+	{
+		.compatible = "rockchip,rk3568-gmac",
+		.data = &rk3568_ops,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct driver_d dwc_ether_driver = {
+	.name = "designware_eqos",
+	.probe = dwc_ether_probe,
+	.of_compatible = DRV_OF_COMPAT(dwc_ether_compatible),
+};
+device_platform_driver(dwc_ether_driver);
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] net: designware: eqos: reset phy
  2021-06-07 14:10 [PATCH 1/2] net: designware: eqos: reset phy Sascha Hauer
  2021-06-07 14:10 ` [PATCH 2/2] net: eqos: Rockchip support Sascha Hauer
@ 2021-06-07 15:59 ` Ahmad Fatoum
  2021-06-07 22:22   ` Sascha Hauer
  1 sibling, 1 reply; 8+ messages in thread
From: Ahmad Fatoum @ 2021-06-07 15:59 UTC (permalink / raw)
  To: Sascha Hauer, Barebox List

Hello Sascha,

On 07.06.21 16:10, Sascha Hauer wrote:
> The designware eqos DT binding has support for specifying reset GPIOs.
> Add support for them.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/of/of_gpio.c          |  7 +++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
> index d2baaeaf63..0321024169 100644
> --- a/drivers/net/designware_eqos.c
> +++ b/drivers/net/designware_eqos.c
> @@ -8,9 +8,11 @@
>  
>  #include <common.h>
>  #include <init.h>
> +#include <gpio.h>
>  #include <dma.h>
>  #include <net.h>
>  #include <of_net.h>
> +#include <of_gpio.h>
>  #include <linux/iopoll.h>
>  #include <linux/time.h>
>  #include <linux/sizes.h>
> @@ -189,6 +191,33 @@ struct eqos_desc {
>  
>  #define MII_BUSY		(1 << 0)
>  
> +static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
> +{
> +	int phy_reset;
> +	int ret;
> +	u32 delays[3] = { 0, 0, 0 };
> +
> +	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
> +
> +        if (!gpio_is_valid(phy_reset))
> +		return 0;

Whitespace is wrong.

> +
> +	ret = gpio_request(phy_reset, "phy-reset");
> +	if (ret)
> +		return ret;

Can you use gpiod_get instead? This would reduce the boilerplate here.

> +
> +	of_property_read_u32_array(dev->device_node,
> +				   "snps,reset-delays-us",
> +				   delays, ARRAY_SIZE(delays));
> +

Looks strange to read out a delay and not act on it. I'd prefer
waiting delays[0] here.

> +	gpio_direction_active(phy_reset, 0);

This always sets logical zero, because gpio_request above doesn't
accept a flag telling it otherwise. You'll need of_get_named_gpio_flags
here, at which point, you'll have basically open-coded gpiod_get(), so
you could use that.

> +	udelay(delays[1]);

Linux rounds up to 1 msec granularity here. We should do likewise.

> +	gpio_set_active(phy_reset, 1);

Nitpick: true/false.

> +	udelay(delays[2]);
> +
> +	return 0;
> +}
> +
>  static int eqos_mdio_wait_idle(struct eqos *eqos)
>  {
>  	u32 idle;
> @@ -843,6 +872,10 @@ int eqos_probe(struct device_d *dev, const struct eqos_ops *ops, void *priv)
>  	if (ret)
>  		return ret;
>  
> +	ret = eqos_phy_reset(dev, eqos);
> +	if (ret)
> +		return ret;
> +
>  	ret = mdiobus_register(miibus);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/of/of_gpio.c b/drivers/of/of_gpio.c
> index 7cbeeaf69e..e1cafdc848 100644
> --- a/drivers/of/of_gpio.c
> +++ b/drivers/of/of_gpio.c
> @@ -35,6 +35,13 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  		if (active_low)
>  			*flags |= OF_GPIO_ACTIVE_LOW;
>  	}
> +
> +	/* Legacy handling of stmmac's active-low PHY reset line */
> +	if (IS_ENABLED(CONFIG_DRIVER_NET_DESIGNWARE_EQOS) &&
> +	    !strcmp(propname, "snps,reset-gpio") &&
> +	    of_property_read_bool(np, "snps,reset-active-low"))
> +		*flags |= OF_GPIO_ACTIVE_LOW;
> +
>  }
>  
>  /**
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] net: eqos: Rockchip support
  2021-06-07 14:10 ` [PATCH 2/2] net: eqos: Rockchip support Sascha Hauer
@ 2021-06-07 16:05   ` Ahmad Fatoum
  0 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-06-07 16:05 UTC (permalink / raw)
  To: Sascha Hauer, Barebox List



On 07.06.21 16:10, Sascha Hauer wrote:
> A variant of the designware eqos core is used on Rockchip SoCs. Add
> support for it.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/Kconfig               |   6 +
>  drivers/net/Makefile              |   1 +
>  drivers/net/designware_rockchip.c | 311 ++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/net/designware_rockchip.c
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 0d55ea7a3b..18931211b5 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -108,6 +108,12 @@ config DRIVER_NET_DESIGNWARE_TEGRA186
>  	help
>  	  This option enables support for the ethernet MAC on the Tegra186 & 194.
>  
> +config DRIVER_NET_DESIGNWARE_ROCKCHIP
> +	bool "Designware Universal MAC ethernet driver for Rockchip platforms"
> +	select MFD_SYSCON
> +	help
> +	  This option enables support for the ethernet MAC on different Rockchip SoCs
> +
>  endif
>  
>  config DRIVER_NET_DM9K
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 656d45a868..1674d53dff 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_DRIVER_NET_DESIGNWARE_SOCFPGA) += designware_socfpga.o
>  obj-$(CONFIG_DRIVER_NET_DESIGNWARE_EQOS) += designware_eqos.o
>  obj-$(CONFIG_DRIVER_NET_DESIGNWARE_STM32) += designware_stm32.o
>  obj-$(CONFIG_DRIVER_NET_DESIGNWARE_TEGRA186) += designware_tegra186.o
> +obj-$(CONFIG_DRIVER_NET_DESIGNWARE_ROCKCHIP) += designware_rockchip.o
>  obj-$(CONFIG_DRIVER_NET_DM9K)		+= dm9k.o
>  obj-$(CONFIG_DRIVER_NET_E1000)		+= e1000/regio.o e1000/main.o e1000/eeprom.o
>  obj-$(CONFIG_DRIVER_NET_ENC28J60)	+= enc28j60.o
> diff --git a/drivers/net/designware_rockchip.c b/drivers/net/designware_rockchip.c
> new file mode 100644
> index 0000000000..883c1d806d
> --- /dev/null
> +++ b/drivers/net/designware_rockchip.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <common.h>
> +#include <init.h>
> +#include <dma.h>
> +#include <net.h>
> +#include <regmap.h>
> +#include <of_net.h>
> +#include <mfd/syscon.h>
> +#include <linux/iopoll.h>
> +#include <linux/sizes.h>
> +#include <linux/time.h>
> +#include <linux/clk.h>
> +
> +#include "designware_eqos.h"
> +
> +struct rk_gmac_ops {
> +	void (*set_to_rgmii)(struct eqos *eqos,
> +			     int tx_delay, int rx_delay);
> +	void (*set_to_rmii)(struct eqos *eqos);
> +	void (*set_speed)(struct eqos *eqos, int speed);
> +	void (*integrated_phy_powerup)(struct eqos *eqos);
> +};
> +
> +struct eqos_rk_gmac {
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +	bool clock_input;
> +	const struct rk_gmac_ops *ops;
> +	struct regmap *grf;
> +	int bus_id;
> +	u32 tx_delay;
> +	u32 rx_delay;
> +	struct device_d *dev;
> +};
> +
> +enum {
> +	CLK_STMMACETH,
> +	CLK_MAC_RX,
> +	CLK_MAC_TX,
> +	CLK_MAC_REFOUT,
> +	CLK_MAC_ACLK,
> +	CLK_MAC_PCLK,
> +	CLK_MAC_SPEED,
> +	CLK_PTP_REF,
> +	CLK_XPCS_PCLK,
> +};
> +
> +static const struct clk_bulk_data rk_gmac_clks[] = {
> +	[CLK_STMMACETH]   = { .id = "stmmaceth" },
> +	[CLK_MAC_RX]      = { .id = "mac_clk_rx" },
> +	[CLK_MAC_TX]      = { .id = "mac_clk_tx" },
> +	[CLK_MAC_REFOUT]  = { .id = "clk_mac_refout" },
> +	[CLK_MAC_ACLK]    = { .id = "aclk_mac" },
> +	[CLK_MAC_PCLK]    = { .id = "pclk_mac" },
> +	[CLK_MAC_SPEED]   = { .id = "clk_mac_speed" },
> +	[CLK_PTP_REF]     = { .id = "ptp_ref" },
> +	[CLK_XPCS_PCLK]   = { .id = "pclk_xpcs" },
> +};
> +
> +static inline struct eqos_rk_gmac *to_rk_gmac(struct eqos *eqos)
> +{
> +	return eqos->priv;
> +}
> +
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> +
> +#define GRF_BIT(nr)	(BIT(nr) | BIT((nr) + 16))
> +#define GRF_CLR_BIT(nr)	(BIT((nr) + 16))
> +
> +#define RK3568_GRF_GMAC0_CON0		0X0380
> +#define RK3568_GRF_GMAC0_CON1		0X0384
> +#define RK3568_GRF_GMAC1_CON0		0X0388
> +#define RK3568_GRF_GMAC1_CON1		0X038c
> +
> +/* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
> +#define RK3568_GMAC_PHY_INTF_SEL_RGMII	\
> +		(GRF_BIT(4) | GRF_CLR_BIT(5) | GRF_CLR_BIT(6))
> +#define RK3568_GMAC_PHY_INTF_SEL_RMII	\
> +		(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | GRF_BIT(6))
> +#define RK3568_GMAC_FLOW_CTRL			GRF_BIT(3)
> +#define RK3568_GMAC_FLOW_CTRL_CLR		GRF_CLR_BIT(3)
> +#define RK3568_GMAC_RXCLK_DLY_ENABLE		GRF_BIT(1)
> +#define RK3568_GMAC_RXCLK_DLY_DISABLE		GRF_CLR_BIT(1)
> +#define RK3568_GMAC_TXCLK_DLY_ENABLE		GRF_BIT(0)
> +#define RK3568_GMAC_TXCLK_DLY_DISABLE		GRF_CLR_BIT(0)
> +
> +/* RK3568_GRF_GMAC0_CON0 && RK3568_GRF_GMAC1_CON0 */
> +#define RK3568_GMAC_CLK_RX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 8)
> +#define RK3568_GMAC_CLK_TX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 0)
> +
> +static unsigned long eqos_get_csr_clk_rate_rk_gmac(struct eqos *eqos)
> +{
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +
> +	return clk_get_rate(priv->clks[CLK_STMMACETH].clk);
> +}
> +
> +static void rk3568_set_to_rgmii(struct eqos *eqos,
> +				int tx_delay, int rx_delay)
> +{
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +	struct device_d *dev = priv->dev;
> +	u32 offset_con0, offset_con1;
> +
> +	if (IS_ERR(priv->grf)) {
> +		dev_err(dev, "Missing rockchip,grf property\n");
> +		return;
> +	}
> +
> +	offset_con0 = (priv->bus_id == 1)
> +		      ? RK3568_GRF_GMAC1_CON0 : RK3568_GRF_GMAC0_CON0;
> +	offset_con1 = (priv->bus_id == 1)
> +		      ? RK3568_GRF_GMAC1_CON1 : RK3568_GRF_GMAC0_CON1;
> +
> +	regmap_write(priv->grf, offset_con1,
> +		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
> +		     RK3568_GMAC_RXCLK_DLY_ENABLE |
> +		     RK3568_GMAC_TXCLK_DLY_ENABLE);
> +
> +	regmap_write(priv->grf, offset_con0,
> +		     RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> +		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> +}
> +
> +static void rk3568_set_to_rmii(struct eqos *eqos)
> +{
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +	struct device_d *dev = priv->dev;
> +	u32 offset_con1;
> +
> +	if (IS_ERR(priv->grf)) {
> +		dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
> +		return;
> +	}
> +
> +	offset_con1 = (priv->bus_id == 1)
> +		      ? RK3568_GRF_GMAC1_CON1 : RK3568_GRF_GMAC0_CON1;
> +
> +	regmap_write(priv->grf, offset_con1,
> +		     RK3568_GMAC_PHY_INTF_SEL_RMII);
> +}
> +
> +static void rk3568_set_gmac_speed(struct eqos *eqos, int speed)
> +{
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +	struct device_d *dev = priv->dev;
> +	unsigned long rate;
> +	int ret;
> +
> +	switch (speed) {
> +	case SPEED_10:
> +		rate = 2500000;
> +		break;
> +	case SPEED_100:
> +		rate = 25000000;
> +		break;
> +	case SPEED_1000:
> +		rate = 125000000;
> +		break;
> +	default:
> +		dev_err(dev, "unknown speed value for GMAC speed=%d", speed);
> +		return;
> +	}
> +
> +	ret = clk_set_rate(priv->clks[CLK_MAC_SPEED].clk, rate);
> +	if (ret)
> +		dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n",
> +			__func__, rate, ret);
> +}
> +
> +static const struct rk_gmac_ops rk3568_ops = {
> +	.set_to_rgmii = rk3568_set_to_rgmii,
> +	.set_to_rmii = rk3568_set_to_rmii,
> +	.set_speed = rk3568_set_gmac_speed,
> +};
> +
> +static int rk_gmac_powerup(struct eqos *eqos)
> +{
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +	struct device_d *dev = priv->dev;
> +
> +	/*rmii or rgmii*/
> +	switch (eqos->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		dev_dbg(dev, "init for RGMII\n");
> +		priv->ops->set_to_rgmii(eqos, priv->tx_delay,
> +					    priv->rx_delay);
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		dev_dbg(dev, "init for RGMII_ID\n");
> +		priv->ops->set_to_rgmii(eqos, 0, 0);
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		dev_dbg(dev, "init for RGMII_RXID\n");
> +		priv->ops->set_to_rgmii(eqos, priv->tx_delay, 0);
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		dev_dbg(dev, "init for RGMII_TXID\n");
> +		priv->ops->set_to_rgmii(eqos, 0, priv->rx_delay);
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		dev_dbg(dev, "init for RMII\n");
> +		priv->ops->set_to_rmii(eqos);
> +		break;
> +	default:
> +		dev_err(dev, "NO interface defined!\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static void eqos_rk_adjust_link(struct eth_device *edev)
> +{
> +	struct eqos *eqos = edev->priv;
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +
> +	priv->ops->set_speed(eqos, edev->phydev->speed);
> +
> +	eqos_adjust_link(edev);
> +}
> +
> +static int eqos_init_rk_gmac(struct device_d *dev, struct eqos *eqos)
> +{
> +	struct device_node *np = dev->device_node;
> +	struct eqos_rk_gmac *priv = to_rk_gmac(eqos);
> +	int ret;
> +	const char *strings;
> +
> +	priv->dev = dev;
> +
> +	ret = of_property_read_string(np, "clock_in_out", &strings);
> +	if (ret) {
> +                dev_err(dev, "Can not read property: clock_in_out.\n");
> +                priv->clock_input = true;
> +	} else {
> +		dev_dbg(dev, "clock is %s\n", strings);
> +		if (!strcmp(strings, "input"))
> +			priv->clock_input = true;
> +		else
> +			priv->clock_input = false;
> +	}
> +
> +	priv->ops = device_get_match_data(dev);
> +
> +	priv->bus_id = of_alias_get_id(np, "ethernet");
> +
> +	priv->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> +	if (IS_ERR(priv->grf)) {
> +		dev_err(dev, "unable to get grf");
> +		return PTR_ERR(priv->grf);
> +	}
> +
> +	priv->tx_delay = 0x30;
> +	of_property_read_u32(np, "tx_delay", &priv->tx_delay);
> +	priv->rx_delay = 0x10;
> +	of_property_read_u32(np, "rx_delay", &priv->rx_delay);
> +
> +	priv->num_clks = ARRAY_SIZE(rk_gmac_clks);
> +	priv->clks = xmalloc(priv->num_clks * sizeof(*priv->clks));
> +	memcpy(priv->clks, rk_gmac_clks, sizeof rk_gmac_clks);
> +
> +	ret = clk_bulk_get(dev, priv->num_clks, priv->clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clks: %s\n", strerror(-ret));
> +		return ret;
> +	}
> +
> +	ret = clk_bulk_enable(priv->num_clks, priv->clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clks: %s\n", strerror(-ret));
> +		return ret;
> +	}
> +
> +	rk_gmac_powerup(eqos);
> +
> +	return 0;
> +}
> +
> +static struct eqos_ops rk_gmac_ops = {
> +	.init = eqos_init_rk_gmac,
> +	.get_ethaddr = eqos_get_ethaddr,
> +	.set_ethaddr = eqos_set_ethaddr,
> +	.adjust_link = eqos_rk_adjust_link,
> +	.get_csr_clk_rate = eqos_get_csr_clk_rate_rk_gmac,
> +
> +	.clk_csr = EQOS_MDIO_ADDR_CR_250_300,
> +	.config_mac = EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_AV,
> +};
> +
> +static int dwc_ether_probe(struct device_d *dev)
> +{
> +	return eqos_probe(dev, &rk_gmac_ops, xzalloc(sizeof(struct eqos_rk_gmac)));
> +}
> +
> +static __maybe_unused struct of_device_id dwc_ether_compatible[] = {
> +	{
> +		.compatible = "rockchip,rk3568-gmac",
> +		.data = &rk3568_ops,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
> +static struct driver_d dwc_ether_driver = {
> +	.name = "designware_eqos",
> +	.probe = dwc_ether_probe,

STM32MP1 and Tegra186 both have a .remove calling eqos_remove here.
I think that's required, because net core doesn't ->halt network
drivers otherwise.

> +	.of_compatible = DRV_OF_COMPAT(dwc_ether_compatible),
> +};
> +device_platform_driver(dwc_ether_driver);

With the above addressed:
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] net: designware: eqos: reset phy
  2021-06-07 15:59 ` [PATCH 1/2] net: designware: eqos: reset phy Ahmad Fatoum
@ 2021-06-07 22:22   ` Sascha Hauer
  2021-06-08  7:31     ` Ahmad Fatoum
  0 siblings, 1 reply; 8+ messages in thread
From: Sascha Hauer @ 2021-06-07 22:22 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: Barebox List

On Mon, Jun 07, 2021 at 05:59:02PM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 07.06.21 16:10, Sascha Hauer wrote:
> > The designware eqos DT binding has support for specifying reset GPIOs.
> > Add support for them.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
> >  drivers/of/of_gpio.c          |  7 +++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
> > index d2baaeaf63..0321024169 100644
> > --- a/drivers/net/designware_eqos.c
> > +++ b/drivers/net/designware_eqos.c
> > @@ -8,9 +8,11 @@
> >  
> >  #include <common.h>
> >  #include <init.h>
> > +#include <gpio.h>
> >  #include <dma.h>
> >  #include <net.h>
> >  #include <of_net.h>
> > +#include <of_gpio.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/time.h>
> >  #include <linux/sizes.h>
> > @@ -189,6 +191,33 @@ struct eqos_desc {
> >  
> >  #define MII_BUSY		(1 << 0)
> >  
> > +static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
> > +{
> > +	int phy_reset;
> > +	int ret;
> > +	u32 delays[3] = { 0, 0, 0 };
> > +
> > +	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
> > +
> > +        if (!gpio_is_valid(phy_reset))
> > +		return 0;
> 
> Whitespace is wrong.
> 
> > +
> > +	ret = gpio_request(phy_reset, "phy-reset");
> > +	if (ret)
> > +		return ret;
> 
> Can you use gpiod_get instead? This would reduce the boilerplate here.

Sure. I didn't realize I don't honour the active high/low flags the way
I did it.

> 
> > +
> > +	of_property_read_u32_array(dev->device_node,
> > +				   "snps,reset-delays-us",
> > +				   delays, ARRAY_SIZE(delays));
> > +
> 
> Looks strange to read out a delay and not act on it. I'd prefer
> waiting delays[0] here.

Yes, it looks strange, but that's because the binding doesn't make much
sense. Why should I insert a delay before doing anything?
I can a delay here, it wouldn't make much difference as all users except
one specify zero delay here anyway. For the one exception I would bet
someone has inserted the first delay without a good reason, they are all
10000.

> 
> > +	gpio_direction_active(phy_reset, 0);
> 
> This always sets logical zero, because gpio_request above doesn't
> accept a flag telling it otherwise. You'll need of_get_named_gpio_flags
> here, at which point, you'll have basically open-coded gpiod_get(), so
> you could use that.

Right.

> 
> > +	udelay(delays[1]);
> 
> Linux rounds up to 1 msec granularity here. We should do likewise.

I don't see a good reason for that. The Linux driver used udelay()
initially, that was changed to msleep as the times were too long for
busy waiting. I have no clue why the author didn't use usleep_range
instead.

> 
> > +	gpio_set_active(phy_reset, 1);
> 
> Nitpick: true/false.

Ok.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] net: designware: eqos: reset phy
  2021-06-07 22:22   ` Sascha Hauer
@ 2021-06-08  7:31     ` Ahmad Fatoum
  2021-06-08  8:58       ` Sascha Hauer
  0 siblings, 1 reply; 8+ messages in thread
From: Ahmad Fatoum @ 2021-06-08  7:31 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Barebox List

Hello Sascha,

On 08.06.21 00:22, Sascha Hauer wrote:
> On Mon, Jun 07, 2021 at 05:59:02PM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 07.06.21 16:10, Sascha Hauer wrote:
>>> The designware eqos DT binding has support for specifying reset GPIOs.
>>> Add support for them.
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>  drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
>>>  drivers/of/of_gpio.c          |  7 +++++++
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
>>> index d2baaeaf63..0321024169 100644
>>> --- a/drivers/net/designware_eqos.c
>>> +++ b/drivers/net/designware_eqos.c
>>> @@ -8,9 +8,11 @@
>>>  
>>>  #include <common.h>
>>>  #include <init.h>
>>> +#include <gpio.h>
>>>  #include <dma.h>
>>>  #include <net.h>
>>>  #include <of_net.h>
>>> +#include <of_gpio.h>
>>>  #include <linux/iopoll.h>
>>>  #include <linux/time.h>
>>>  #include <linux/sizes.h>
>>> @@ -189,6 +191,33 @@ struct eqos_desc {
>>>  
>>>  #define MII_BUSY		(1 << 0)
>>>  
>>> +static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
>>> +{
>>> +	int phy_reset;
>>> +	int ret;
>>> +	u32 delays[3] = { 0, 0, 0 };
>>> +
>>> +	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
>>> +
>>> +        if (!gpio_is_valid(phy_reset))
>>> +		return 0;
>>
>> Whitespace is wrong.
>>
>>> +
>>> +	ret = gpio_request(phy_reset, "phy-reset");
>>> +	if (ret)
>>> +		return ret;
>>
>> Can you use gpiod_get instead? This would reduce the boilerplate here.
> 
> Sure. I didn't realize I don't honour the active high/low flags the way
> I did it.
> 
>>
>>> +
>>> +	of_property_read_u32_array(dev->device_node,
>>> +				   "snps,reset-delays-us",
>>> +				   delays, ARRAY_SIZE(delays));
>>> +
>>
>> Looks strange to read out a delay and not act on it. I'd prefer
>> waiting delays[0] here.
> 
> Yes, it looks strange, but that's because the binding doesn't make much
> sense. Why should I insert a delay before doing anything?

                   .--------.

     POR --------->|R  flip |---- Regulator ----> PHY VDD

                .->|S  flop |

                |  `--------'

                |   

                |   

                |

RESET GPIO -----'`-------------------------------> PHY Reset

(active low)

It's stupid, but it works with Linux and wouldn't with barebox
(if PHY VDD takes too long to stabilize)... ^^'

> I can a delay here, it wouldn't make much difference as all users except
> one specify zero delay here anyway. For the one exception I would bet
> someone has inserted the first delay without a good reason, they are all
> 10000.

That's probably true. I still think mimicking Linux' interpretation
of bindings is a good general rule to follow.

>>
>>> +	gpio_direction_active(phy_reset, 0);
>>
>> This always sets logical zero, because gpio_request above doesn't
>> accept a flag telling it otherwise. You'll need of_get_named_gpio_flags
>> here, at which point, you'll have basically open-coded gpiod_get(), so
>> you could use that.
> 
> Right.
> 
>>
>>> +	udelay(delays[1]);
>>
>> Linux rounds up to 1 msec granularity here. We should do likewise.
> 
> I don't see a good reason for that. The Linux driver used udelay()
> initially, that was changed to msleep as the times were too long for
> busy waiting. I have no clue why the author didn't use usleep_range
> instead.

Same reason: Device trees are tested with Linux. They've a better chance
of just working when we round up wait times the same way.

> 
>>
>>> +	gpio_set_active(phy_reset, 1);
>>
>> Nitpick: true/false.
> 
> Ok.
> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] net: designware: eqos: reset phy
  2021-06-08  7:31     ` Ahmad Fatoum
@ 2021-06-08  8:58       ` Sascha Hauer
  2021-06-09  8:04         ` Ahmad Fatoum
  0 siblings, 1 reply; 8+ messages in thread
From: Sascha Hauer @ 2021-06-08  8:58 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: Barebox List

On Tue, Jun 08, 2021 at 09:31:03AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 08.06.21 00:22, Sascha Hauer wrote:
> > On Mon, Jun 07, 2021 at 05:59:02PM +0200, Ahmad Fatoum wrote:
> >> Hello Sascha,
> >>
> >> On 07.06.21 16:10, Sascha Hauer wrote:
> >>> The designware eqos DT binding has support for specifying reset GPIOs.
> >>> Add support for them.
> >>>
> >>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >>> ---
> >>>  drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
> >>>  drivers/of/of_gpio.c          |  7 +++++++
> >>>  2 files changed, 40 insertions(+)
> >>>
> >>> diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
> >>> index d2baaeaf63..0321024169 100644
> >>> --- a/drivers/net/designware_eqos.c
> >>> +++ b/drivers/net/designware_eqos.c
> >>> @@ -8,9 +8,11 @@
> >>>  
> >>>  #include <common.h>
> >>>  #include <init.h>
> >>> +#include <gpio.h>
> >>>  #include <dma.h>
> >>>  #include <net.h>
> >>>  #include <of_net.h>
> >>> +#include <of_gpio.h>
> >>>  #include <linux/iopoll.h>
> >>>  #include <linux/time.h>
> >>>  #include <linux/sizes.h>
> >>> @@ -189,6 +191,33 @@ struct eqos_desc {
> >>>  
> >>>  #define MII_BUSY		(1 << 0)
> >>>  
> >>> +static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
> >>> +{
> >>> +	int phy_reset;
> >>> +	int ret;
> >>> +	u32 delays[3] = { 0, 0, 0 };
> >>> +
> >>> +	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
> >>> +
> >>> +        if (!gpio_is_valid(phy_reset))
> >>> +		return 0;
> >>
> >> Whitespace is wrong.
> >>
> >>> +
> >>> +	ret = gpio_request(phy_reset, "phy-reset");
> >>> +	if (ret)
> >>> +		return ret;
> >>
> >> Can you use gpiod_get instead? This would reduce the boilerplate here.
> > 
> > Sure. I didn't realize I don't honour the active high/low flags the way
> > I did it.
> > 
> >>
> >>> +
> >>> +	of_property_read_u32_array(dev->device_node,
> >>> +				   "snps,reset-delays-us",
> >>> +				   delays, ARRAY_SIZE(delays));
> >>> +
> >>
> >> Looks strange to read out a delay and not act on it. I'd prefer
> >> waiting delays[0] here.
> > 
> > Yes, it looks strange, but that's because the binding doesn't make much
> > sense. Why should I insert a delay before doing anything?
> 
>                    .--------.
> 
>      POR --------->|R  flip |---- Regulator ----> PHY VDD
> 
>                 .->|S  flop |
> 
>                 |  `--------'
> 
>                 |   
> 
>                 |   
> 
>                 |
> 
> RESET GPIO -----'`-------------------------------> PHY Reset
> 
> (active low)
> 
> It's stupid, but it works with Linux and wouldn't with barebox
> (if PHY VDD takes too long to stabilize)... ^^'

What we are doing is:

1) RESET_GPIO has unknown state
2) gpio_request()
3) RESET_GPIO has the same unknown state
4) udelay(delays[0])
5) RESET_GPIO still has the same unknown state
6) gpio_set_active()
7) RESET_GPIO puts phy into reset

Your FLIPFLOP will also only set its output at step 7), so your
regulator stabilization time begins at 7), you would still have
to make delays[1] long enough for the regulator to stabilize and
the phy come up.
What matters here is the point where we put RESET_GPIO into a known
state. Whether you do step 4) or leave it out makes no difference.

> 
> > I can a delay here, it wouldn't make much difference as all users except
> > one specify zero delay here anyway. For the one exception I would bet
> > someone has inserted the first delay without a good reason, they are all
> > 10000.
> 
> That's probably true. I still think mimicking Linux' interpretation
> of bindings is a good general rule to follow.

As long as they make sense, yes.

> 
> >>
> >>> +	gpio_direction_active(phy_reset, 0);
> >>
> >> This always sets logical zero, because gpio_request above doesn't
> >> accept a flag telling it otherwise. You'll need of_get_named_gpio_flags
> >> here, at which point, you'll have basically open-coded gpiod_get(), so
> >> you could use that.
> > 
> > Right.
> > 
> >>
> >>> +	udelay(delays[1]);
> >>
> >> Linux rounds up to 1 msec granularity here. We should do likewise.
> > 
> > I don't see a good reason for that. The Linux driver used udelay()
> > initially, that was changed to msleep as the times were too long for
> > busy waiting. I have no clue why the author didn't use usleep_range
> > instead.
> 
> Same reason: Device trees are tested with Linux. They've a better chance
> of just working when we round up wait times the same way.

I am generally with you, but in this case the binding is very clear and
if you find a bug in the dts with this case, then be happy and fix it.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] net: designware: eqos: reset phy
  2021-06-08  8:58       ` Sascha Hauer
@ 2021-06-09  8:04         ` Ahmad Fatoum
  0 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-06-09  8:04 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Barebox List



On 08.06.21 10:58, Sascha Hauer wrote:
> On Tue, Jun 08, 2021 at 09:31:03AM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 08.06.21 00:22, Sascha Hauer wrote:
>>> On Mon, Jun 07, 2021 at 05:59:02PM +0200, Ahmad Fatoum wrote:
>>>> Hello Sascha,
>>>>
>>>> On 07.06.21 16:10, Sascha Hauer wrote:
>>>>> The designware eqos DT binding has support for specifying reset GPIOs.
>>>>> Add support for them.
>>>>>
>>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>>> ---
>>>>>  drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
>>>>>  drivers/of/of_gpio.c          |  7 +++++++
>>>>>  2 files changed, 40 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
>>>>> index d2baaeaf63..0321024169 100644
>>>>> --- a/drivers/net/designware_eqos.c
>>>>> +++ b/drivers/net/designware_eqos.c
>>>>> @@ -8,9 +8,11 @@
>>>>>  
>>>>>  #include <common.h>
>>>>>  #include <init.h>
>>>>> +#include <gpio.h>
>>>>>  #include <dma.h>
>>>>>  #include <net.h>
>>>>>  #include <of_net.h>
>>>>> +#include <of_gpio.h>
>>>>>  #include <linux/iopoll.h>
>>>>>  #include <linux/time.h>
>>>>>  #include <linux/sizes.h>
>>>>> @@ -189,6 +191,33 @@ struct eqos_desc {
>>>>>  
>>>>>  #define MII_BUSY		(1 << 0)
>>>>>  
>>>>> +static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
>>>>> +{
>>>>> +	int phy_reset;
>>>>> +	int ret;
>>>>> +	u32 delays[3] = { 0, 0, 0 };
>>>>> +
>>>>> +	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
>>>>> +
>>>>> +        if (!gpio_is_valid(phy_reset))
>>>>> +		return 0;
>>>>
>>>> Whitespace is wrong.
>>>>
>>>>> +
>>>>> +	ret = gpio_request(phy_reset, "phy-reset");
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>
>>>> Can you use gpiod_get instead? This would reduce the boilerplate here.
>>>
>>> Sure. I didn't realize I don't honour the active high/low flags the way
>>> I did it.
>>>
>>>>
>>>>> +
>>>>> +	of_property_read_u32_array(dev->device_node,
>>>>> +				   "snps,reset-delays-us",
>>>>> +				   delays, ARRAY_SIZE(delays));
>>>>> +
>>>>
>>>> Looks strange to read out a delay and not act on it. I'd prefer
>>>> waiting delays[0] here.
>>>
>>> Yes, it looks strange, but that's because the binding doesn't make much
>>> sense. Why should I insert a delay before doing anything?
>>
>>                    .--------.
>>
>>      POR --------->|R  flip |---- Regulator ----> PHY VDD
>>
>>                 .->|S  flop |
>>
>>                 |  `--------'
>>
>>                 |   
>>
>>                 |   
>>
>>                 |
>>
>> RESET GPIO -----'`-------------------------------> PHY Reset
>>
>> (active low)
>>
>> It's stupid, but it works with Linux and wouldn't with barebox
>> (if PHY VDD takes too long to stabilize)... ^^'
> 
> What we are doing is:
> 
> 1) RESET_GPIO has unknown state
> 2) gpio_request()
> 3) RESET_GPIO has the same unknown state

Linux requests with devm_gpiod_get_optional(..., GPIOD_OUT_LOW),

so it's inactive here (== HIGH, which enables flip flop)

> 4) udelay(delays[0])
> 5) RESET_GPIO still has the same unknown state

Voltage has a while to stabilize while RESET GPIO is inactive here.

> 6) gpio_set_active()
> 7) RESET_GPIO puts phy into reset
> 
> Your FLIPFLOP will also only set its output at step 7), so your
> regulator stabilization time begins at 7), you would still have
> to make delays[1] long enough for the regulator to stabilize and
> the phy come up.
> What matters here is the point where we put RESET_GPIO into a known
> state. Whether you do step 4) or leave it out makes no difference.
> 
>>
>>> I can a delay here, it wouldn't make much difference as all users except
>>> one specify zero delay here anyway. For the one exception I would bet
>>> someone has inserted the first delay without a good reason, they are all
>>> 10000.
>>
>> That's probably true. I still think mimicking Linux' interpretation
>> of bindings is a good general rule to follow.
> 
> As long as they make sense, yes.
> 
>>
>>>>
>>>>> +	gpio_direction_active(phy_reset, 0);
>>>>
>>>> This always sets logical zero, because gpio_request above doesn't
>>>> accept a flag telling it otherwise. You'll need of_get_named_gpio_flags
>>>> here, at which point, you'll have basically open-coded gpiod_get(), so
>>>> you could use that.
>>>
>>> Right.
>>>
>>>>
>>>>> +	udelay(delays[1]);
>>>>
>>>> Linux rounds up to 1 msec granularity here. We should do likewise.
>>>
>>> I don't see a good reason for that. The Linux driver used udelay()
>>> initially, that was changed to msleep as the times were too long for
>>> busy waiting. I have no clue why the author didn't use usleep_range
>>> instead.
>>
>> Same reason: Device trees are tested with Linux. They've a better chance
>> of just working when we round up wait times the same way.
> 
> I am generally with you, but in this case the binding is very clear and
> if you find a bug in the dts with this case, then be happy and fix it.
> 
> Sascha
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-06-09  8:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-07 14:10 [PATCH 1/2] net: designware: eqos: reset phy Sascha Hauer
2021-06-07 14:10 ` [PATCH 2/2] net: eqos: Rockchip support Sascha Hauer
2021-06-07 16:05   ` Ahmad Fatoum
2021-06-07 15:59 ` [PATCH 1/2] net: designware: eqos: reset phy Ahmad Fatoum
2021-06-07 22:22   ` Sascha Hauer
2021-06-08  7:31     ` Ahmad Fatoum
2021-06-08  8:58       ` Sascha Hauer
2021-06-09  8:04         ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox