[PATCH 0/8] Add dwc_eth_qos support for rockchip

[PATCH 0/8] Add dwc_eth_qos support for rockchip

[David Wu]

Rockchip Socs can support two controllers "snps, dwmac-4.20a"
and "snps, dwmac-3.50". In order to support two at gmac-rockchip.c,
export public interface functions and struct data, it will be more
general for others.


David Wu (8):
  net: dwc_eth_qos: Use dev_ functions calls to get FDT data
  net: dwc_eth_qos: Fix the software reset
  net: dwc_eth_qos: Add option "snps,reset-gpio" phy-rst gpio for stm32
  net: dwc_eth_qos: Move interface() to eqos_ops struct
  net: dwc_eth_qos: Make clk_rx and clk_tx optional
  net: dwc_eth_qos: Split eqos_start() to get link speed
  net: dwc_eth_qos: Export common struct and interface at head file
  net: gmac_rockchip: Add dwc_eth_qos support

 drivers/net/Kconfig         |   2 +-
 drivers/net/dwc_eth_qos.c   | 264 +++++++++++++++++-------------------
 drivers/net/gmac_rockchip.c | 160 ++++++++++++++++++----
 3 files changed, 263 insertions(+), 163 deletions(-)

-- 
2.19.1

[PATCH 1/8] net: dwc_eth_qos: Use dev_ functions calls to get FDT data

[David Wu]

It seems dev_ functions are more general than fdt_ functions.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 63f2086dec..a72132cacf 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1728,8 +1728,7 @@ static phy_interface_t eqos_get_interface_stm32(struct udevice *dev)
 
 	debug("%s(dev=%p):\n", __func__, dev);
 
-	phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy-mode",
-			       NULL);
+	phy_mode = dev_read_string(dev, "phy-mode");
 	if (phy_mode)
 		interface = phy_get_interface_by_name(phy_mode);
 
@@ -1788,9 +1787,9 @@ static int eqos_probe(struct udevice *dev)
 	eqos->dev = dev;
 	eqos->config = (void *)dev_get_driver_data(dev);
 
-	eqos->regs = devfdt_get_addr(dev);
+	eqos->regs = dev_read_addr(dev);
 	if (eqos->regs == FDT_ADDR_T_NONE) {
-		pr_err("devfdt_get_addr() failed");
+		pr_err("dev_read_addr() failed");
 		return -ENODEV;
 	}
 	eqos->mac_regs = (void *)(eqos->regs + EQOS_MAC_REGS_BASE);
-- 
2.19.1

[PATCH 2/8] net: dwc_eth_qos: Fix the software reset

[David Wu]

When using rgmii Gigabit mode, the wait_for_bit_le32()
reset method resulting in RX can not receive data, after
this patch, works well.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index a72132cacf..16988f6bdc 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1034,7 +1034,7 @@ static int eqos_write_hwaddr(struct udevice *dev)
 static int eqos_start(struct udevice *dev)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
-	int ret, i;
+	int ret, i, limit;
 	ulong rate;
 	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
 	ulong last_rx_desc;
@@ -1060,12 +1060,21 @@ static int eqos_start(struct udevice *dev)
 
 	eqos->reg_access_ok = true;
 
-	ret = wait_for_bit_le32(&eqos->dma_regs->mode,
-				EQOS_DMA_MODE_SWR, false,
-				eqos->config->swr_wait, false);
-	if (ret) {
+	/* DMA SW reset */
+	val = readl(&eqos->dma_regs->mode);
+	val |= EQOS_DMA_MODE_SWR;
+	writel(val, &eqos->dma_regs->mode);
+	limit = eqos->config->swr_wait / 10;
+	while (limit--) {
+		if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR))
+			break;
+		mdelay(10000);
+	}
+
+	if (limit < 0) {
 		pr_err("EQOS_DMA_MODE_SWR stuck");
-		goto err_stop_resets;
+		goto err_stop_clks;
+		return -ETIMEDOUT;
 	}
 
 	ret = eqos->config->ops->eqos_calibrate_pads(dev);
-- 
2.19.1

[PATCH 2/8] net: dwc_eth_qos: Fix the software reset

[Patrice CHOTARD]

Hi David

On 4/30/20 12:36 PM, David Wu wrote:
> When using rgmii Gigabit mode, the wait_for_bit_le32()
> reset method resulting in RX can not receive data, after
> this patch, works well.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>
>  drivers/net/dwc_eth_qos.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index a72132cacf..16988f6bdc 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1034,7 +1034,7 @@ static int eqos_write_hwaddr(struct udevice *dev)
>  static int eqos_start(struct udevice *dev)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
> -	int ret, i;
> +	int ret, i, limit;
>  	ulong rate;
>  	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
>  	ulong last_rx_desc;
> @@ -1060,12 +1060,21 @@ static int eqos_start(struct udevice *dev)
>  
>  	eqos->reg_access_ok = true;
>  
> -	ret = wait_for_bit_le32(&eqos->dma_regs->mode,
> -				EQOS_DMA_MODE_SWR, false,
> -				eqos->config->swr_wait, false);
> -	if (ret) {
> +	/* DMA SW reset */
> +	val = readl(&eqos->dma_regs->mode);
> +	val |= EQOS_DMA_MODE_SWR;
> +	writel(val, &eqos->dma_regs->mode);
> +	limit = eqos->config->swr_wait / 10;
> +	while (limit--) {
> +		if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR))
> +			break;
> +		mdelay(10000);
> +	}

usage of wait_for_bit_le32() must still work, if the timeout is no enough in you case,

create a dedicated rockchip struct eqos_config


> +
> +	if (limit < 0) {
>  		pr_err("EQOS_DMA_MODE_SWR stuck");
> -		goto err_stop_resets;
why are you updating the error path here ?
> +		goto err_stop_clks;
> +		return -ETIMEDOUT;
the return after the goto is useless
>  	}
>  
>  	ret = eqos->config->ops->eqos_calibrate_pads(dev);

[PATCH 2/8] net: dwc_eth_qos: Fix the software reset

[Stephen Warren]

On 4/30/20 4:36 AM, David Wu wrote:
> When using rgmii Gigabit mode, the wait_for_bit_le32()
> reset method resulting in RX can not receive data, after
> this patch, works well.

> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c

> +	limit = eqos->config->swr_wait / 10;
> +	while (limit--) {
> +		if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR))
> +			break;
> +		mdelay(10000);
> +	}

mdelay()'s parameter is in milliseconds judging by its implementation in
include/linux/delay.h. So, this delays 10 seconds in each loop
iteration. That can't possibly be right.

[PATCH 3/8] net: dwc_eth_qos: Add option "snps, reset-gpio" phy-rst gpio for stm32

[David Wu]

It can be seen that most of the Socs using STM mac, "snps,reset-gpio"
gpio is used, adding this option makes reset function more general.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 40 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 16988f6bdc..06a8d924a7 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -298,6 +298,7 @@ struct eqos_priv {
 	struct eqos_tegra186_regs *tegra186_regs;
 	struct reset_ctl reset_ctl;
 	struct gpio_desc phy_reset_gpio;
+	u32 reset_delays[3];
 	struct clk clk_master_bus;
 	struct clk clk_rx;
 	struct clk clk_ptp_ref;
@@ -701,6 +702,15 @@ static int eqos_start_resets_stm32(struct udevice *dev)
 
 	debug("%s(dev=%p):\n", __func__, dev);
 	if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
+		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
+		if (ret < 0) {
+			pr_err("dm_gpio_set_value(phy_reset, deassert) failed: %d",
+			       ret);
+			return ret;
+		}
+
+		udelay(eqos->reset_delays[0]);
+
 		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1);
 		if (ret < 0) {
 			pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d",
@@ -708,7 +718,7 @@ static int eqos_start_resets_stm32(struct udevice *dev)
 			return ret;
 		}
 
-		udelay(2);
+		udelay(eqos->reset_delays[1]);
 
 		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
 		if (ret < 0) {
@@ -716,6 +726,8 @@ static int eqos_start_resets_stm32(struct udevice *dev)
 			       ret);
 			return ret;
 		}
+
+		udelay(eqos->reset_delays[2]);
 	}
 	debug("%s: OK\n", __func__);
 
@@ -1065,16 +1077,16 @@ static int eqos_start(struct udevice *dev)
 	val |= EQOS_DMA_MODE_SWR;
 	writel(val, &eqos->dma_regs->mode);
 	limit = eqos->config->swr_wait / 10;
-	while (limit--) {
+	do {
 		if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR))
 			break;
 		mdelay(10000);
-	}
+	} while (limit--);
 
 	if (limit < 0) {
 		pr_err("EQOS_DMA_MODE_SWR stuck");
-		goto err_stop_clks;
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
+		goto err_stop_resets;
 	}
 
 	ret = eqos->config->ops->eqos_calibrate_pads(dev);
@@ -1712,11 +1724,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
 		if (ret)
 			pr_warn("gpio_request_by_name(phy reset) not provided %d",
 				ret);
+		else
+			eqos->reset_delays[1] = 2;
 
 		eqos->phyaddr = ofnode_read_u32_default(phandle_args.node,
 							"reg", -1);
 	}
 
+	if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
+		int reset_flags = GPIOD_IS_OUT;
+
+		if (dev_read_bool(dev, "snps,reset-active-low"))
+			reset_flags |= GPIOD_ACTIVE_LOW;
+
+		ret = gpio_request_by_name(dev, "snps,reset-gpio", 0,
+					   &eqos->phy_reset_gpio, reset_flags);
+		if (ret == 0)
+			ret = dev_read_u32_array(dev, "snps,reset-delays-us",
+						 eqos->reset_delays, 3);
+		else
+			pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d",
+				ret);
+	}
+
 	debug("%s: OK\n", __func__);
 	return 0;
 
-- 
2.19.1

[PATCH 3/8] net: dwc_eth_qos: Add option "snps,reset-gpio" phy-rst gpio for stm32

[Patrice CHOTARD]

Hi David

On 4/30/20 12:36 PM, David Wu wrote:
> It can be seen that most of the Socs using STM mac, "snps,reset-gpio"
> gpio is used, adding this option makes reset function more general.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>
>  drivers/net/dwc_eth_qos.c | 40 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 16988f6bdc..06a8d924a7 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -298,6 +298,7 @@ struct eqos_priv {
>  	struct eqos_tegra186_regs *tegra186_regs;
>  	struct reset_ctl reset_ctl;
>  	struct gpio_desc phy_reset_gpio;
> +	u32 reset_delays[3];
>  	struct clk clk_master_bus;
>  	struct clk clk_rx;
>  	struct clk clk_ptp_ref;
> @@ -701,6 +702,15 @@ static int eqos_start_resets_stm32(struct udevice *dev)
>  
>  	debug("%s(dev=%p):\n", __func__, dev);
>  	if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
> +		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
> +		if (ret < 0) {
> +			pr_err("dm_gpio_set_value(phy_reset, deassert) failed: %d",
> +			       ret);
> +			return ret;
> +		}
> +
> +		udelay(eqos->reset_delays[0]);
> +
not related to this patch subject
>  		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1);
>  		if (ret < 0) {
>  			pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d",
> @@ -708,7 +718,7 @@ static int eqos_start_resets_stm32(struct udevice *dev)
>  			return ret;
>  		}
>  
> -		udelay(2);
> +		udelay(eqos->reset_delays[1]);
>  
>  		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
>  		if (ret < 0) {
> @@ -716,6 +726,8 @@ static int eqos_start_resets_stm32(struct udevice *dev)
>  			       ret);
>  			return ret;
>  		}
> +
> +		udelay(eqos->reset_delays[2]);
>  	}
>  	debug("%s: OK\n", __func__);
>  
> @@ -1065,16 +1077,16 @@ static int eqos_start(struct udevice *dev)
>  	val |= EQOS_DMA_MODE_SWR;
>  	writel(val, &eqos->dma_regs->mode);
>  	limit = eqos->config->swr_wait / 10;
> -	while (limit--) {
> +	do {
>  		if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR))
>  			break;
>  		mdelay(10000);
> -	}
> +	} while (limit--);
why are you updating this ? it's not related to the purpose of this patch
>  
>  	if (limit < 0) {
>  		pr_err("EQOS_DMA_MODE_SWR stuck");
> -		goto err_stop_clks;
> -		return -ETIMEDOUT;
> +		ret = -ETIMEDOUT;
> +		goto err_stop_resets;
ditto
>  	}
>  
>  	ret = eqos->config->ops->eqos_calibrate_pads(dev);
> @@ -1712,11 +1724,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>  		if (ret)
>  			pr_warn("gpio_request_by_name(phy reset) not provided %d",
>  				ret);
> +		else
> +			eqos->reset_delays[1] = 2;
this is not the correct place to set default value. It must be set in case we can't get value from DT below
>  
>  		eqos->phyaddr = ofnode_read_u32_default(phandle_args.node,
>  							"reg", -1);
>  	}
>  
> +	if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
> +		int reset_flags = GPIOD_IS_OUT;
> +
> +		if (dev_read_bool(dev, "snps,reset-active-low"))
> +			reset_flags |= GPIOD_ACTIVE_LOW;
> +
> +		ret = gpio_request_by_name(dev, "snps,reset-gpio", 0,
> +					   &eqos->phy_reset_gpio, reset_flags);
> +		if (ret == 0)
> +			ret = dev_read_u32_array(dev, "snps,reset-delays-us",
> +						 eqos->reset_delays, 3);
in case "snps,reset-delays-us" is not in present DT, all resets-delays are set to 0, see my remark above
> +		else
> +			pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d",
> +				ret);
> +	}
> +
>  	debug("%s: OK\n", __func__);
>  	return 0;
>  

[PATCH 3/8] net: dwc_eth_qos: Add option "snps,reset-gpio" phy-rst gpio for stm32

[David Wu]

Hi Patrice,

? 2020/4/30 ??11:47, Patrice CHOTARD ??:
>> @@ -701,6 +702,15 @@ static int eqos_start_resets_stm32(struct udevice *dev)
>>   
>>   	debug("%s(dev=%p):\n", __func__, dev);
>>   	if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
>> +		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
>> +		if (ret < 0) {
>> +			pr_err("dm_gpio_set_value(phy_reset, deassert) failed: %d",
>> +			       ret);
>> +			return ret;
>> +		}
>> +
>> +		udelay(eqos->reset_delays[0]);
>> +
> not related to this patch subject
>>   		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1);
>>   		if (ret < 0) {
>>   			pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d",
>> @@ -708,7 +718,7 @@ static int eqos_start_resets_stm32(struct udevice *dev)
>>   			return ret;
>>   		}
>>   
>> -		udelay(2);
>> +		udelay(eqos->reset_delays[1]);
>>   
>>   		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
>>   		if (ret < 0) {

>> @@ -1712,11 +1724,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>>   		if (ret)
>>   			pr_warn("gpio_request_by_name(phy reset) not provided %d",
>>   				ret);
>> +		else
>> +			eqos->reset_delays[1] = 2;
> this is not the correct place to set default value. It must be set in case we can't get value from DT below

No, three cases below, it is second case, and we can see udelay(2) in 
eqos_start_resets_stm32(), here we are to be compatible with the original.

- If there is not phy rst, reset_delays is 0;
- If "reset-gpios exists in phy node, reset_delays [1] = 2;
- "snps, reset-gpio" exists in DT, reset_delays is obtained from DT

>>   
>>   		eqos->phyaddr = ofnode_read_u32_default(phandle_args.node,
>>   							"reg", -1);
>>   	}
>>   
>> +	if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
>> +		int reset_flags = GPIOD_IS_OUT;
>> +
>> +		if (dev_read_bool(dev, "snps,reset-active-low"))
>> +			reset_flags |= GPIOD_ACTIVE_LOW;
>> +
>> +		ret = gpio_request_by_name(dev, "snps,reset-gpio", 0,
>> +					   &eqos->phy_reset_gpio, reset_flags);
>> +		if (ret == 0)
>> +			ret = dev_read_u32_array(dev, "snps,reset-delays-us",
>> +						 eqos->reset_delays, 3);
> in case "snps,reset-delays-us" is not in present DT, all resets-delays are set to 0, see my remark above
>> +		else
>> +			pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d",
>> +				ret);
>> +	}
>> +
>>   	debug("%s: OK\n", __func__);
>>   	return 0;
>>   

[PATCH 3/8] net: dwc_eth_qos: Add option "snps, reset-gpio" phy-rst gpio for stm32

[Stephen Warren]

On 4/30/20 4:36 AM, David Wu wrote:
> It can be seen that most of the Socs using STM mac, "snps,reset-gpio"
> gpio is used, adding this option makes reset function more general.

> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c

> @@ -1712,11 +1724,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>  		if (ret)
>  			pr_warn("gpio_request_by_name(phy reset) not provided %d",
>  				ret);
> +		else
> +			eqos->reset_delays[1] = 2;
>  
>  		eqos->phyaddr = ofnode_read_u32_default(phandle_args.node,
>  							"reg", -1);
>  	}
>  
> +	if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
> +		int reset_flags = GPIOD_IS_OUT;
> +
> +		if (dev_read_bool(dev, "snps,reset-active-low"))
> +			reset_flags |= GPIOD_ACTIVE_LOW;
> +
> +		ret = gpio_request_by_name(dev, "snps,reset-gpio", 0,
> +					   &eqos->phy_reset_gpio, reset_flags);


The kernel's bindings/net/snps,dwmac.yaml does not mention any
reset-gpios property (which is what the existing code parses just above
the portion that is quoted by this patch as context). I suspect that
this patch should simply change the name of the property that this
function parses to align with the binding, and fix any DTs in U-Boot
that also don't match the binding?

[PATCH 3/8] net: dwc_eth_qos: Add option "snps, reset-gpio" phy-rst gpio for stm32

[Stephen Warren]

On 4/30/20 4:36 PM, Stephen Warren wrote:
> On 4/30/20 4:36 AM, David Wu wrote:
>> It can be seen that most of the Socs using STM mac, "snps,reset-gpio"
>> gpio is used, adding this option makes reset function more general.
> 
>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> 
>> @@ -1712,11 +1724,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>>  		if (ret)
>>  			pr_warn("gpio_request_by_name(phy reset) not provided %d",
>>  				ret);
>> +		else
>> +			eqos->reset_delays[1] = 2;
>>  
>>  		eqos->phyaddr = ofnode_read_u32_default(phandle_args.node,
>>  							"reg", -1);
>>  	}
>>  
>> +	if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
>> +		int reset_flags = GPIOD_IS_OUT;
>> +
>> +		if (dev_read_bool(dev, "snps,reset-active-low"))
>> +			reset_flags |= GPIOD_ACTIVE_LOW;
>> +
>> +		ret = gpio_request_by_name(dev, "snps,reset-gpio", 0,
>> +					   &eqos->phy_reset_gpio, reset_flags);
> 
> 
> The kernel's bindings/net/snps,dwmac.yaml does not mention any
> reset-gpios property (which is what the existing code parses just above
> the portion that is quoted by this patch as context). I suspect that
> this patch should simply change the name of the property that this
> function parses to align with the binding, and fix any DTs in U-Boot
> that also don't match the binding?

Oops, the relevant YAML file is probably
./bindings/net/rockchip-dwmac.txt, although this makes no difference to
my statement luckily.

[PATCH 3/8] net: dwc_eth_qos: Add option "snps, reset-gpio" phy-rst gpio for stm32

[David Wu]

Hi Stephen,

? 2020/5/1 ??6:36, Stephen Warren ??:
> The kernel's bindings/net/snps,dwmac.yaml does not mention any
> reset-gpios property (which is what the existing code parses just above
> the portion that is quoted by this patch as context). I suspect that
> this patch should simply change the name of the property that this
> function parses to align with the binding, and fix any DTs in U-Boot
> that also don't match the binding?

The kernel's ./Documentation/devicetree/bindings/net/stmmac.txt mentions
that Required properties:

- phy-mode: See ethernet.txt file in the same directory.
- snps,reset-gpio       gpio number for phy reset.
- snps,reset-active-low boolean flag to indicate if phy reset is active low.
- snps,reset-delays-us  is triplet of delays
         The 1st cell is reset pre-delay in micro seconds.
         The 2nd cell is reset pulse in micro seconds.
         The 3rd cell is reset post-delay in micro seconds.

[PATCH 3/8] net: dwc_eth_qos: Add option "snps, reset-gpio" phy-rst gpio for stm32

[David Wu]

Hi Stephen,

? 2020/5/9 ??10:41, David Wu ??:
> 
> The kernel's ./Documentation/devicetree/bindings/net/stmmac.txt mentions
> that Required properties:
> 
> - phy-mode: See ethernet.txt file in the same directory.
> - snps,reset-gpio?????? gpio number for phy reset.
> - snps,reset-active-low boolean flag to indicate if phy reset is active 
> low.
> - snps,reset-delays-us? is triplet of delays
>  ??????? The 1st cell is reset pre-delay in micro seconds.
>  ??????? The 2nd cell is reset pulse in micro seconds.
>  ??????? The 3rd cell is reset post-delay in micro seconds.

Sorry, I just saw you replying again before, stmmac.txt was found, this 
reply email please discard.

[PATCH 4/8] net: dwc_eth_qos: Move interface() to eqos_ops struct

[David Wu]

After moving to eqos_ops, if eqos_config is defined
outside, can not export interface() definition.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 06a8d924a7..fbd6caf85b 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -267,7 +267,6 @@ struct eqos_config {
 	int swr_wait;
 	int config_mac;
 	int config_mac_mdio;
-	phy_interface_t (*interface)(struct udevice *dev);
 	struct eqos_ops *ops;
 };
 
@@ -286,6 +285,7 @@ struct eqos_ops {
 	int (*eqos_disable_calibration)(struct udevice *dev);
 	int (*eqos_set_tx_clk_speed)(struct udevice *dev);
 	ulong (*eqos_get_tick_clk_rate)(struct udevice *dev);
+	phy_interface_t (*eqos_get_interface)(struct udevice *dev);
 };
 
 struct eqos_priv {
@@ -1105,7 +1105,7 @@ static int eqos_start(struct udevice *dev)
 	 */
 	if (!eqos->phy) {
 		eqos->phy = phy_connect(eqos->mii, eqos->phyaddr, dev,
-					eqos->config->interface(dev));
+			  eqos->config->ops->eqos_get_interface(dev));
 		if (!eqos->phy) {
 			pr_err("phy_connect() failed");
 			goto err_stop_resets;
@@ -1675,7 +1675,7 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
 
 	debug("%s(dev=%p):\n", __func__, dev);
 
-	interface = eqos->config->interface(dev);
+	interface = eqos->config->ops->eqos_get_interface(dev);
 
 	if (interface == PHY_INTERFACE_MODE_NONE) {
 		pr_err("Invalid PHY interface\n");
@@ -1918,7 +1918,8 @@ static struct eqos_ops eqos_tegra186_ops = {
 	.eqos_calibrate_pads = eqos_calibrate_pads_tegra186,
 	.eqos_disable_calibration = eqos_disable_calibration_tegra186,
 	.eqos_set_tx_clk_speed = eqos_set_tx_clk_speed_tegra186,
-	.eqos_get_tick_clk_rate = eqos_get_tick_clk_rate_tegra186
+	.eqos_get_tick_clk_rate = eqos_get_tick_clk_rate_tegra186,
+	.eqos_get_interface = eqos_get_interface_tegra186
 };
 
 static const struct eqos_config eqos_tegra186_config = {
@@ -1927,7 +1928,6 @@ static const struct eqos_config eqos_tegra186_config = {
 	.swr_wait = 10,
 	.config_mac = EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_DCB,
 	.config_mac_mdio = EQOS_MAC_MDIO_ADDRESS_CR_20_35,
-	.interface = eqos_get_interface_tegra186,
 	.ops = &eqos_tegra186_ops
 };
 
@@ -1945,7 +1945,8 @@ static struct eqos_ops eqos_stm32_ops = {
 	.eqos_calibrate_pads = eqos_calibrate_pads_stm32,
 	.eqos_disable_calibration = eqos_disable_calibration_stm32,
 	.eqos_set_tx_clk_speed = eqos_set_tx_clk_speed_stm32,
-	.eqos_get_tick_clk_rate = eqos_get_tick_clk_rate_stm32
+	.eqos_get_tick_clk_rate = eqos_get_tick_clk_rate_stm32,
+	.eqos_get_interface = eqos_get_interface_stm32
 };
 
 static const struct eqos_config eqos_stm32_config = {
@@ -1954,7 +1955,6 @@ static const struct eqos_config eqos_stm32_config = {
 	.swr_wait = 50,
 	.config_mac = EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_AV,
 	.config_mac_mdio = EQOS_MAC_MDIO_ADDRESS_CR_250_300,
-	.interface = eqos_get_interface_stm32,
 	.ops = &eqos_stm32_ops
 };
 
-- 
2.19.1

[PATCH 4/8] net: dwc_eth_qos: Move interface() to eqos_ops struct

[Stephen Warren]

On 4/30/20 4:36 AM, David Wu wrote:
> After moving to eqos_ops, if eqos_config is defined
> outside, can not export interface() definition.

Looking at the patch itself, I think this patch just moves a function
pointer from the config to the ops structure which makes sense. However,
I can't understand the patch description at all, so I worry there's
intended to be some other justification/implication for this patch, and
that may not be correct...

In particular, defined outside of what, and what does this have to do
with exporting things?

[PATCH 4/8] net: dwc_eth_qos: Move interface() to eqos_ops struct

[David Wu]

Hi Stephen,

? 2020/5/1 ??6:39, Stephen Warren ??:
> On 4/30/20 4:36 AM, David Wu wrote:
>> After moving to eqos_ops, if eqos_config is defined
>> outside, can not export interface() definition.
> 
> Looking at the patch itself, I think this patch just moves a function
> pointer from the config to the ops structure which makes sense. However,
> I can't understand the patch description at all, so I worry there's
> intended to be some other justification/implication for this patch, and
> that may not be correct...
> 
> In particular, defined outside of what, and what does this have to do
> with exporting things
Yes, if define eqos_config structure in gmac_rockchip.c, need to export 
an eqos_get_interface function, or redefine a similar function in 
gmac_rockchip.c, but this function is the same implementation as 
eqos_get_interface_stm32(), so we can share this function. Move 
interface() to eqos_ops structure, no need to export interface() in the 
head file. I lost a patch to define eqos_ops structure at curent file, 
then only exprot eqos_rockchip_ops, so it would be simpler?

> 
> 
> 

[PATCH 5/8] net: dwc_eth_qos: Make clk_rx and clk_tx optional

[David Wu]

For others using, clk_rx and clk_tx may not be necessary,
and their clock names are different.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 65 +++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index fbd6caf85b..b5d5156292 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -592,16 +592,20 @@ static int eqos_start_clks_stm32(struct udevice *dev)
 		goto err;
 	}
 
-	ret = clk_enable(&eqos->clk_rx);
-	if (ret < 0) {
-		pr_err("clk_enable(clk_rx) failed: %d", ret);
-		goto err_disable_clk_master_bus;
+	if (clk_valid(&eqos->clk_rx)) {
+		ret = clk_enable(&eqos->clk_rx);
+		if (ret < 0) {
+			pr_err("clk_enable(clk_rx) failed: %d", ret);
+			goto err_disable_clk_master_bus;
+		}
 	}
 
-	ret = clk_enable(&eqos->clk_tx);
-	if (ret < 0) {
-		pr_err("clk_enable(clk_tx) failed: %d", ret);
-		goto err_disable_clk_rx;
+	if (clk_valid(&eqos->clk_tx)) {
+		ret = clk_enable(&eqos->clk_tx);
+		if (ret < 0) {
+			pr_err("clk_enable(clk_tx) failed: %d", ret);
+			goto err_disable_clk_rx;
+		}
 	}
 
 	if (clk_valid(&eqos->clk_ck)) {
@@ -616,9 +620,11 @@ static int eqos_start_clks_stm32(struct udevice *dev)
 	return 0;
 
 err_disable_clk_tx:
-	clk_disable(&eqos->clk_tx);
+	if (clk_valid(&eqos->clk_tx))
+		clk_disable(&eqos->clk_tx);
 err_disable_clk_rx:
-	clk_disable(&eqos->clk_rx);
+	if (clk_valid(&eqos->clk_rx))
+		clk_disable(&eqos->clk_rx);
 err_disable_clk_master_bus:
 	clk_disable(&eqos->clk_master_bus);
 err:
@@ -647,8 +653,10 @@ static void eqos_stop_clks_stm32(struct udevice *dev)
 
 	debug("%s(dev=%p):\n", __func__, dev);
 
-	clk_disable(&eqos->clk_tx);
-	clk_disable(&eqos->clk_rx);
+	if (clk_valid(&eqos->clk_tx))
+		clk_disable(&eqos->clk_tx);
+	if (clk_valid(&eqos->clk_rx))
+		clk_disable(&eqos->clk_rx);
 	clk_disable(&eqos->clk_master_bus);
 	if (clk_valid(&eqos->clk_ck))
 		clk_disable(&eqos->clk_ck);
@@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
 	ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus);
 	if (ret) {
 		pr_err("clk_get_by_name(master_bus) failed: %d", ret);
-		goto err_probe;
+		return ret;
 	}
 
-	ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx);
-	if (ret) {
-		pr_err("clk_get_by_name(rx) failed: %d", ret);
-		goto err_free_clk_master_bus;
-	}
+	ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx);
+	if (ret)
+		pr_warn("clk_get_by_name(rx) failed: %d", ret);
 
-	ret = clk_get_by_name(dev, "mac-clk-tx", &eqos->clk_tx);
-	if (ret) {
-		pr_err("clk_get_by_name(tx) failed: %d", ret);
-		goto err_free_clk_rx;
-	}
+	ret = clk_get_by_name(dev, "mac_clk_tx", &eqos->clk_tx);
+	if (ret)
+		pr_warn("clk_get_by_name(tx) failed: %d", ret);
 
 	/*  Get ETH_CLK clocks (optional) */
 	ret = clk_get_by_name(dev, "eth-ck", &eqos->clk_ck);
@@ -1749,15 +1753,6 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
 
 	debug("%s: OK\n", __func__);
 	return 0;
-
-err_free_clk_rx:
-	clk_free(&eqos->clk_rx);
-err_free_clk_master_bus:
-	clk_free(&eqos->clk_master_bus);
-err_probe:
-
-	debug("%s: returns %d\n", __func__, ret);
-	return ret;
 }
 
 static phy_interface_t eqos_get_interface_stm32(struct udevice *dev)
@@ -1803,8 +1798,10 @@ static int eqos_remove_resources_stm32(struct udevice *dev)
 
 	debug("%s(dev=%p):\n", __func__, dev);
 
-	clk_free(&eqos->clk_tx);
-	clk_free(&eqos->clk_rx);
+	if (clk_valid(&eqos->clk_tx))
+		clk_free(&eqos->clk_tx);
+	if (clk_valid(&eqos->clk_rx))
+		clk_free(&eqos->clk_rx);
 	clk_free(&eqos->clk_master_bus);
 	if (clk_valid(&eqos->clk_ck))
 		clk_free(&eqos->clk_ck);
-- 
2.19.1

[PATCH 5/8] net: dwc_eth_qos: Make clk_rx and clk_tx optional

[Patrice CHOTARD]

Hi David

On 4/30/20 12:43 PM, David Wu wrote:
> For others using, clk_rx and clk_tx may not be necessary,
> and their clock names are different.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>
>  drivers/net/dwc_eth_qos.c | 65 +++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index fbd6caf85b..b5d5156292 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -592,16 +592,20 @@ static int eqos_start_clks_stm32(struct udevice *dev)
>  		goto err;
>  	}
>  
> -	ret = clk_enable(&eqos->clk_rx);
> -	if (ret < 0) {
> -		pr_err("clk_enable(clk_rx) failed: %d", ret);
> -		goto err_disable_clk_master_bus;
> +	if (clk_valid(&eqos->clk_rx)) {
> +		ret = clk_enable(&eqos->clk_rx);
> +		if (ret < 0) {
> +			pr_err("clk_enable(clk_rx) failed: %d", ret);
> +			goto err_disable_clk_master_bus;
> +		}
>  	}
>  
> -	ret = clk_enable(&eqos->clk_tx);
> -	if (ret < 0) {
> -		pr_err("clk_enable(clk_tx) failed: %d", ret);
> -		goto err_disable_clk_rx;
> +	if (clk_valid(&eqos->clk_tx)) {
> +		ret = clk_enable(&eqos->clk_tx);
> +		if (ret < 0) {
> +			pr_err("clk_enable(clk_tx) failed: %d", ret);
> +			goto err_disable_clk_rx;
> +		}
>  	}
>  
>  	if (clk_valid(&eqos->clk_ck)) {
> @@ -616,9 +620,11 @@ static int eqos_start_clks_stm32(struct udevice *dev)
>  	return 0;
>  
>  err_disable_clk_tx:
> -	clk_disable(&eqos->clk_tx);
> +	if (clk_valid(&eqos->clk_tx))
> +		clk_disable(&eqos->clk_tx);
>  err_disable_clk_rx:
> -	clk_disable(&eqos->clk_rx);
> +	if (clk_valid(&eqos->clk_rx))
> +		clk_disable(&eqos->clk_rx);
>  err_disable_clk_master_bus:
>  	clk_disable(&eqos->clk_master_bus);
>  err:
> @@ -647,8 +653,10 @@ static void eqos_stop_clks_stm32(struct udevice *dev)
>  
>  	debug("%s(dev=%p):\n", __func__, dev);
>  
> -	clk_disable(&eqos->clk_tx);
> -	clk_disable(&eqos->clk_rx);
> +	if (clk_valid(&eqos->clk_tx))
> +		clk_disable(&eqos->clk_tx);
> +	if (clk_valid(&eqos->clk_rx))
> +		clk_disable(&eqos->clk_rx);
>  	clk_disable(&eqos->clk_master_bus);
>  	if (clk_valid(&eqos->clk_ck))
>  		clk_disable(&eqos->clk_ck);
> @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>  	ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus);
>  	if (ret) {
>  		pr_err("clk_get_by_name(master_bus) failed: %d", ret);
> -		goto err_probe;
> +		return ret;
>  	}
why are you changing the error path here ?
>  
> -	ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx);
> -	if (ret) {
> -		pr_err("clk_get_by_name(rx) failed: %d", ret);
> -		goto err_free_clk_master_bus;
> -	}
> +	ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx);
> +	if (ret)
> +		pr_warn("clk_get_by_name(rx) failed: %d", ret);
>  
> -	ret = clk_get_by_name(dev, "mac-clk-tx", &eqos->clk_tx);
> -	if (ret) {
> -		pr_err("clk_get_by_name(tx) failed: %d", ret);
> -		goto err_free_clk_rx;
> -	}
> +	ret = clk_get_by_name(dev, "mac_clk_tx", &eqos->clk_tx);
> +	if (ret)
> +		pr_warn("clk_get_by_name(tx) failed: %d", ret);

Nak

Why are you changing the Rx and Tx clock names ?

for information, check with the kernel dt bindings regarding this driver:

Documentation/devicetree/bindings/net/stm32-dwmac.txt

This patch is breaking ethernet on STM32MP1 boards

>  
>  	/*  Get ETH_CLK clocks (optional) */
>  	ret = clk_get_by_name(dev, "eth-ck", &eqos->clk_ck);
> @@ -1749,15 +1753,6 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>  
>  	debug("%s: OK\n", __func__);
>  	return 0;
> -
> -err_free_clk_rx:
> -	clk_free(&eqos->clk_rx);
> -err_free_clk_master_bus:
> -	clk_free(&eqos->clk_master_bus);
> -err_probe:
> -
> -	debug("%s: returns %d\n", __func__, ret);
> -	return ret;
>  }
>  
>  static phy_interface_t eqos_get_interface_stm32(struct udevice *dev)
> @@ -1803,8 +1798,10 @@ static int eqos_remove_resources_stm32(struct udevice *dev)
>  
>  	debug("%s(dev=%p):\n", __func__, dev);
>  
> -	clk_free(&eqos->clk_tx);
> -	clk_free(&eqos->clk_rx);
> +	if (clk_valid(&eqos->clk_tx))
> +		clk_free(&eqos->clk_tx);
> +	if (clk_valid(&eqos->clk_rx))
> +		clk_free(&eqos->clk_rx);
>  	clk_free(&eqos->clk_master_bus);
>  	if (clk_valid(&eqos->clk_ck))
>  		clk_free(&eqos->clk_ck);

[PATCH 5/8] net: dwc_eth_qos: Make clk_rx and clk_tx optional

[David Wu]

Hi Patrice,

? 2020/4/30 ??10:00, Patrice CHOTARD ??:
>> @@ -647,8 +653,10 @@ static void eqos_stop_clks_stm32(struct udevice *dev)
>>   
>>   	debug("%s(dev=%p):\n", __func__, dev);
>>   
>> -	clk_disable(&eqos->clk_tx);
>> -	clk_disable(&eqos->clk_rx);
>> +	if (clk_valid(&eqos->clk_tx))
>> +		clk_disable(&eqos->clk_tx);
>> +	if (clk_valid(&eqos->clk_rx))
>> +		clk_disable(&eqos->clk_rx);
>>   	clk_disable(&eqos->clk_master_bus);
>>   	if (clk_valid(&eqos->clk_ck))
>>   		clk_disable(&eqos->clk_ck);
>> @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>>   	ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus);
>>   	if (ret) {
>>   		pr_err("clk_get_by_name(master_bus) failed: %d", ret);
>> -		goto err_probe;
>> +		return ret;
>>   	}
> why are you changing the error path here ?

The following code has not returned, so for the sake of simpler code, 
return directly.

>>   
>> -	ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx);
>> -	if (ret) {
>> -		pr_err("clk_get_by_name(rx) failed: %d", ret);
>> -		goto err_free_clk_master_bus;
>> -	}
>> +	ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx);
>> +	if (ret)
>> +		pr_warn("clk_get_by_name(rx) failed: %d", ret);
>>   
>> -	ret = clk_get_by_name(dev, "mac-clk-tx", &eqos->clk_tx);
>> -	if (ret) {
>> -		pr_err("clk_get_by_name(tx) failed: %d", ret);
>> -		goto err_free_clk_rx;
>> -	}
>> +	ret = clk_get_by_name(dev, "mac_clk_tx", &eqos->clk_tx);
>> +	if (ret)
>> +		pr_warn("clk_get_by_name(tx) failed: %d", ret);
> Nak
> 
> Why are you changing the Rx and Tx clock names ?
> 
> for information, check with the kernel dt bindings regarding this driver:
> 
> Documentation/devicetree/bindings/net/stm32-dwmac.txt
> 
> This patch is breaking ethernet on STM32MP1 boards

I should have made a mistake here. In fact, for Rockchip, there is no 
need to obtain this two clock. My intention is to make these two clocks 
optional.

> 

[PATCH 5/8] net: dwc_eth_qos: Make clk_rx and clk_tx optional

[Stephen Warren]

On 4/30/20 4:43 AM, David Wu wrote:
> For others using, clk_rx and clk_tx may not be necessary,
> and their clock names are different.

> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c

> @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev)
>  	ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus);
>  	if (ret) {
>  		pr_err("clk_get_by_name(master_bus) failed: %d", ret);
> -		goto err_probe;
> +		return ret;
>  	}
>  
> -	ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx);
> -	if (ret) {
> -		pr_err("clk_get_by_name(rx) failed: %d", ret);
> -		goto err_free_clk_master_bus;
> -	}
> +	ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx);
> +	if (ret)
> +		pr_warn("clk_get_by_name(rx) failed: %d", ret);

Oh... Judging by your email, you're trying to make this driver work on a
Rockchip system. However, you're editing an STM32-specific probe
function. You should introduce a new probe function for Rockchip if it
needs to work differently to the existing STM32 code.

Also, mac_clk_rx isn't a valid DT property name; they aren't supposed to
have _ in them. I don't see mac_clk_rx or mac-clk-rx in the DT binding
file in Documentation/bindings/net/rockchip-dwmac.txt the kernel. That
should probably be submitted/reviewed/applied before using the binding...

[PATCH 5/8] net: dwc_eth_qos: Make clk_rx and clk_tx optional

[David Wu]

Hi Stephen,

? 2020/5/1 ??6:45, Stephen Warren ??:
> Oh... Judging by your email, you're trying to make this driver work on a
> Rockchip system. However, you're editing an STM32-specific probe
> function. You should introduce a new probe function for Rockchip if it
> needs to work differently to the existing STM32 code.
> 
> Also, mac_clk_rx isn't a valid DT property name; they aren't supposed to
> have _ in them. I don't see mac_clk_rx or mac-clk-rx in the DT binding
> file in Documentation/bindings/net/rockchip-dwmac.txt the kernel. That
> should probably be submitted/reviewed/applied before using the binding...

If necessary, I can rewrite a function to obtain resources, but I think 
the function of rockchip and stm should be very similar, and can be shared.

[PATCH 6/8] net: dwc_eth_qos: Split eqos_start() to get link speed

[David Wu]

Before enabling mac and mac working, we need to obtain
the current link speed to configure the clock, so split
eqos_start into two functions.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 56 ++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index b5d5156292..25b3449047 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1051,19 +1051,15 @@ static int eqos_write_hwaddr(struct udevice *dev)
 	return 0;
 }
 
-static int eqos_start(struct udevice *dev)
+static int eqos_init(struct udevice *dev)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
-	int ret, i, limit;
+	int ret, limit;
 	ulong rate;
-	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
-	ulong last_rx_desc;
+	u32 val;
 
 	debug("%s(dev=%p):\n", __func__, dev);
 
-	eqos->tx_desc_idx = 0;
-	eqos->rx_desc_idx = 0;
-
 	ret = eqos->config->ops->eqos_start_clks(dev);
 	if (ret < 0) {
 		pr_err("eqos_start_clks() failed: %d", ret);
@@ -1151,6 +1147,30 @@ static int eqos_start(struct udevice *dev)
 		goto err_shutdown_phy;
 	}
 
+	debug("%s: OK\n", __func__);
+	return 0;
+
+err_shutdown_phy:
+	phy_shutdown(eqos->phy);
+err_stop_resets:
+	eqos->config->ops->eqos_stop_resets(dev);
+err_stop_clks:
+	eqos->config->ops->eqos_stop_clks(dev);
+err:
+	pr_err("FAILED: %d", ret);
+	return ret;
+}
+
+static void eqos_enable(struct udevice *dev)
+{
+	struct eqos_priv *eqos = dev_get_priv(dev);
+	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
+	ulong last_rx_desc;
+	int i;
+
+	eqos->tx_desc_idx = 0;
+	eqos->rx_desc_idx = 0;
+
 	/* Configure MTL */
 
 	/* Enable Store and Forward mode for TX */
@@ -1352,19 +1372,19 @@ static int eqos_start(struct udevice *dev)
 	writel(last_rx_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
 
 	eqos->started = true;
+}
 
-	debug("%s: OK\n", __func__);
-	return 0;
+static int eqos_start(struct udevice *dev)
+{
+	int ret;
 
-err_shutdown_phy:
-	phy_shutdown(eqos->phy);
-err_stop_resets:
-	eqos->config->ops->eqos_stop_resets(dev);
-err_stop_clks:
-	eqos->config->ops->eqos_stop_clks(dev);
-err:
-	pr_err("FAILED: %d", ret);
-	return ret;
+	ret = eqos_init(dev);
+	if (ret)
+		return ret;
+
+	eqos_enable(dev);
+
+	return 0;
 }
 
 static void eqos_stop(struct udevice *dev)
-- 
2.19.1

[PATCH 6/8] net: dwc_eth_qos: Split eqos_start() to get link speed

[Patrice CHOTARD]

Hi David

On 4/30/20 12:44 PM, David Wu wrote:
> Before enabling mac and mac working, we need to obtain
> the current link speed to configure the clock, so split
> eqos_start into two functions.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>
>  drivers/net/dwc_eth_qos.c | 56 ++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index b5d5156292..25b3449047 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1051,19 +1051,15 @@ static int eqos_write_hwaddr(struct udevice *dev)
>  	return 0;
>  }
>  
> -static int eqos_start(struct udevice *dev)
> +static int eqos_init(struct udevice *dev)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
> -	int ret, i, limit;
> +	int ret, limit;
>  	ulong rate;
> -	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
> -	ulong last_rx_desc;
> +	u32 val;
>  
>  	debug("%s(dev=%p):\n", __func__, dev);
>  
> -	eqos->tx_desc_idx = 0;
> -	eqos->rx_desc_idx = 0;
> -
>  	ret = eqos->config->ops->eqos_start_clks(dev);
>  	if (ret < 0) {
>  		pr_err("eqos_start_clks() failed: %d", ret);
> @@ -1151,6 +1147,30 @@ static int eqos_start(struct udevice *dev)
>  		goto err_shutdown_phy;
>  	}
>  
> +	debug("%s: OK\n", __func__);
> +	return 0;
> +
> +err_shutdown_phy:
> +	phy_shutdown(eqos->phy);
> +err_stop_resets:
> +	eqos->config->ops->eqos_stop_resets(dev);
> +err_stop_clks:
> +	eqos->config->ops->eqos_stop_clks(dev);
> +err:
> +	pr_err("FAILED: %d", ret);
> +	return ret;
> +}
> +
> +static void eqos_enable(struct udevice *dev)
> +{
> +	struct eqos_priv *eqos = dev_get_priv(dev);
> +	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
> +	ulong last_rx_desc;
> +	int i;
> +
> +	eqos->tx_desc_idx = 0;
> +	eqos->rx_desc_idx = 0;
> +
>  	/* Configure MTL */
>  
>  	/* Enable Store and Forward mode for TX */
> @@ -1352,19 +1372,19 @@ static int eqos_start(struct udevice *dev)
>  	writel(last_rx_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
>  
>  	eqos->started = true;
> +}
>  
> -	debug("%s: OK\n", __func__);
> -	return 0;
> +static int eqos_start(struct udevice *dev)
> +{
> +	int ret;
>  
> -err_shutdown_phy:
> -	phy_shutdown(eqos->phy);
> -err_stop_resets:
> -	eqos->config->ops->eqos_stop_resets(dev);
> -err_stop_clks:
> -	eqos->config->ops->eqos_stop_clks(dev);
> -err:
> -	pr_err("FAILED: %d", ret);
> -	return ret;
> +	ret = eqos_init(dev);
> +	if (ret)
> +		return ret;
> +
> +	eqos_enable(dev);
> +
> +	return 0;

Can you explain why you are splitting this function in 2 parts and calling these parts sequentially ?

Please elaborate

Thanks

Patrice


>  }
>  
>  static void eqos_stop(struct udevice *dev)

[PATCH 6/8] net: dwc_eth_qos: Split eqos_start() to get link speed

[David Wu]

Hi Patrice,

? 2020/4/30 ??11:33, Patrice CHOTARD ??:
> Can you explain why you are splitting this function in 2 parts and calling these parts sequentially ?

For rockchip, need to obtain the current link speed to configure the tx 
clocks, (for example, in rgmii mode, 1000M link: 125M, 100M link: 25M, 
10M link is 2.5M rate) and then enable gmac. So after the 
adjust_link(), before the start gamc, this intermediate stage needs to 
configure the clock according to the current link speed.

[PATCH 6/8] net: dwc_eth_qos: Split eqos_start() to get link speed

[Patrice CHOTARD]

Hi David

On 5/9/20 8:42 AM, David Wu wrote:
> Hi Patrice,
>
> ? 2020/4/30 ??11:33, Patrice CHOTARD ??:
>> Can you explain why you are splitting this function in 2 parts and calling these parts sequentially ?
>
> For rockchip, need to obtain the current link speed to configure the tx clocks, (for example, in rgmii mode, 1000M link: 125M, 100M link: 25M, 10M link is 2.5M rate) and then enable gmac. So after the adjust_link(), before the start gamc, this intermediate stage needs to configure the clock according to the current link speed.
>
>
Please, add these informations in the commit message

Thanks

Patrice

[PATCH 6/8] net: dwc_eth_qos: Split eqos_start() to get link speed

[David Wu]

Hi Patrice,

? 2020/5/11 ??8:48, Patrice CHOTARD ??:
> Hi David
> 
> On 5/9/20 8:42 AM, David Wu wrote:
>> Hi Patrice,
>>
>> ? 2020/4/30 ??11:33, Patrice CHOTARD ??:
>>> Can you explain why you are splitting this function in 2 parts and calling these parts sequentially ?
>>
>> For rockchip, need to obtain the current link speed to configure the tx clocks, (for example, in rgmii mode, 1000M link: 125M, 100M link: 25M, 10M link is 2.5M rate) and then enable gmac. So after the adjust_link(), before the start gamc, this intermediate stage needs to configure the clock according to the current link speed.
>>
>>
> Please, add these informations in the commit message

I will add it at the next version, Thanks.

> 
> Thanks
> 
> Patrice
> 

[PATCH 7/8] net: dwc_eth_qos: Export common struct and interface at head file

[David Wu]

Open structure data and interface, so that Soc using dw_eth_qos
controller can reference.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 81 +++++----------------------------------
 1 file changed, 9 insertions(+), 72 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 25b3449047..7f47e5f505 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -41,6 +41,7 @@
 #include <wait_bit.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
+#include "dwc_eth_qos.h"
 
 /* Core registers */
 
@@ -94,9 +95,6 @@ struct eqos_mac_regs {
 
 #define EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT			0
 #define EQOS_MAC_RXQ_CTRL0_RXQ0EN_MASK			3
-#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_NOT_ENABLED		0
-#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_DCB		2
-#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_AV		1
 
 #define EQOS_MAC_RXQ_CTRL2_PSRQ0_SHIFT			0
 #define EQOS_MAC_RXQ_CTRL2_PSRQ0_MASK			0xff
@@ -109,8 +107,6 @@ struct eqos_mac_regs {
 #define EQOS_MAC_MDIO_ADDRESS_PA_SHIFT			21
 #define EQOS_MAC_MDIO_ADDRESS_RDA_SHIFT			16
 #define EQOS_MAC_MDIO_ADDRESS_CR_SHIFT			8
-#define EQOS_MAC_MDIO_ADDRESS_CR_20_35			2
-#define EQOS_MAC_MDIO_ADDRESS_CR_250_300		5
 #define EQOS_MAC_MDIO_ADDRESS_SKAP			BIT(4)
 #define EQOS_MAC_MDIO_ADDRESS_GOC_SHIFT			2
 #define EQOS_MAC_MDIO_ADDRESS_GOC_READ			3
@@ -261,65 +257,6 @@ struct eqos_desc {
 #define EQOS_DESC3_LD		BIT(28)
 #define EQOS_DESC3_BUF1V	BIT(24)
 
-struct eqos_config {
-	bool reg_access_always_ok;
-	int mdio_wait;
-	int swr_wait;
-	int config_mac;
-	int config_mac_mdio;
-	struct eqos_ops *ops;
-};
-
-struct eqos_ops {
-	void (*eqos_inval_desc)(void *desc);
-	void (*eqos_flush_desc)(void *desc);
-	void (*eqos_inval_buffer)(void *buf, size_t size);
-	void (*eqos_flush_buffer)(void *buf, size_t size);
-	int (*eqos_probe_resources)(struct udevice *dev);
-	int (*eqos_remove_resources)(struct udevice *dev);
-	int (*eqos_stop_resets)(struct udevice *dev);
-	int (*eqos_start_resets)(struct udevice *dev);
-	void (*eqos_stop_clks)(struct udevice *dev);
-	int (*eqos_start_clks)(struct udevice *dev);
-	int (*eqos_calibrate_pads)(struct udevice *dev);
-	int (*eqos_disable_calibration)(struct udevice *dev);
-	int (*eqos_set_tx_clk_speed)(struct udevice *dev);
-	ulong (*eqos_get_tick_clk_rate)(struct udevice *dev);
-	phy_interface_t (*eqos_get_interface)(struct udevice *dev);
-};
-
-struct eqos_priv {
-	struct udevice *dev;
-	const struct eqos_config *config;
-	fdt_addr_t regs;
-	struct eqos_mac_regs *mac_regs;
-	struct eqos_mtl_regs *mtl_regs;
-	struct eqos_dma_regs *dma_regs;
-	struct eqos_tegra186_regs *tegra186_regs;
-	struct reset_ctl reset_ctl;
-	struct gpio_desc phy_reset_gpio;
-	u32 reset_delays[3];
-	struct clk clk_master_bus;
-	struct clk clk_rx;
-	struct clk clk_ptp_ref;
-	struct clk clk_tx;
-	struct clk clk_ck;
-	struct clk clk_slave_bus;
-	struct mii_dev *mii;
-	struct phy_device *phy;
-	int phyaddr;
-	u32 max_speed;
-	void *descs;
-	struct eqos_desc *tx_descs;
-	struct eqos_desc *rx_descs;
-	int tx_desc_idx, rx_desc_idx;
-	void *tx_dma_buf;
-	void *rx_dma_buf;
-	void *rx_pkt;
-	bool started;
-	bool reg_access_ok;
-};
-
 /*
  * TX and RX descriptors are 16 bytes. This causes problems with the cache
  * maintenance on CPUs where the cache-line size exceeds the size of these
@@ -1007,7 +944,7 @@ static int eqos_adjust_link(struct udevice *dev)
 	return 0;
 }
 
-static int eqos_write_hwaddr(struct udevice *dev)
+int eqos_write_hwaddr(struct udevice *dev)
 {
 	struct eth_pdata *plat = dev_get_platdata(dev);
 	struct eqos_priv *eqos = dev_get_priv(dev);
@@ -1051,7 +988,7 @@ static int eqos_write_hwaddr(struct udevice *dev)
 	return 0;
 }
 
-static int eqos_init(struct udevice *dev)
+int eqos_init(struct udevice *dev)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	int ret, limit;
@@ -1161,7 +1098,7 @@ err:
 	return ret;
 }
 
-static void eqos_enable(struct udevice *dev)
+void eqos_enable(struct udevice *dev)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
@@ -1387,7 +1324,7 @@ static int eqos_start(struct udevice *dev)
 	return 0;
 }
 
-static void eqos_stop(struct udevice *dev)
+void eqos_stop(struct udevice *dev)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	int i;
@@ -1441,7 +1378,7 @@ static void eqos_stop(struct udevice *dev)
 	debug("%s: OK\n", __func__);
 }
 
-static int eqos_send(struct udevice *dev, void *packet, int length)
+int eqos_send(struct udevice *dev, void *packet, int length)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	struct eqos_desc *tx_desc;
@@ -1482,7 +1419,7 @@ static int eqos_send(struct udevice *dev, void *packet, int length)
 	return -ETIMEDOUT;
 }
 
-static int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
+int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	struct eqos_desc *rx_desc;
@@ -1506,7 +1443,7 @@ static int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
 	return length;
 }
 
-static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
+int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	uchar *packet_expected;
@@ -1833,7 +1770,7 @@ static int eqos_remove_resources_stm32(struct udevice *dev)
 	return 0;
 }
 
-static int eqos_probe(struct udevice *dev)
+int eqos_probe(struct udevice *dev)
 {
 	struct eqos_priv *eqos = dev_get_priv(dev);
 	int ret;
-- 
2.19.1

[PATCH 7/8] net: dwc_eth_qos: Export common struct and interface at head file

[Patrice CHOTARD]

Hi David

On 4/30/20 12:45 PM, David Wu wrote:
> Open structure data and interface, so that Soc using dw_eth_qos
> controller can reference.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>
>  drivers/net/dwc_eth_qos.c | 81 +++++----------------------------------
>  1 file changed, 9 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 25b3449047..7f47e5f505 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -41,6 +41,7 @@
>  #include <wait_bit.h>
>  #include <asm/gpio.h>
>  #include <asm/io.h>
> +#include "dwc_eth_qos.h"

The new dwc_eth_qos.h file is missing in your series and can't compile

Patrice

>  
>  /* Core registers */
>  
> @@ -94,9 +95,6 @@ struct eqos_mac_regs {
>  
>  #define EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT			0
>  #define EQOS_MAC_RXQ_CTRL0_RXQ0EN_MASK			3
> -#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_NOT_ENABLED		0
> -#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_DCB		2
> -#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_AV		1
>  
>  #define EQOS_MAC_RXQ_CTRL2_PSRQ0_SHIFT			0
>  #define EQOS_MAC_RXQ_CTRL2_PSRQ0_MASK			0xff
> @@ -109,8 +107,6 @@ struct eqos_mac_regs {
>  #define EQOS_MAC_MDIO_ADDRESS_PA_SHIFT			21
>  #define EQOS_MAC_MDIO_ADDRESS_RDA_SHIFT			16
>  #define EQOS_MAC_MDIO_ADDRESS_CR_SHIFT			8
> -#define EQOS_MAC_MDIO_ADDRESS_CR_20_35			2
> -#define EQOS_MAC_MDIO_ADDRESS_CR_250_300		5
>  #define EQOS_MAC_MDIO_ADDRESS_SKAP			BIT(4)
>  #define EQOS_MAC_MDIO_ADDRESS_GOC_SHIFT			2
>  #define EQOS_MAC_MDIO_ADDRESS_GOC_READ			3
> @@ -261,65 +257,6 @@ struct eqos_desc {
>  #define EQOS_DESC3_LD		BIT(28)
>  #define EQOS_DESC3_BUF1V	BIT(24)
>  
> -struct eqos_config {
> -	bool reg_access_always_ok;
> -	int mdio_wait;
> -	int swr_wait;
> -	int config_mac;
> -	int config_mac_mdio;
> -	struct eqos_ops *ops;
> -};
> -
> -struct eqos_ops {
> -	void (*eqos_inval_desc)(void *desc);
> -	void (*eqos_flush_desc)(void *desc);
> -	void (*eqos_inval_buffer)(void *buf, size_t size);
> -	void (*eqos_flush_buffer)(void *buf, size_t size);
> -	int (*eqos_probe_resources)(struct udevice *dev);
> -	int (*eqos_remove_resources)(struct udevice *dev);
> -	int (*eqos_stop_resets)(struct udevice *dev);
> -	int (*eqos_start_resets)(struct udevice *dev);
> -	void (*eqos_stop_clks)(struct udevice *dev);
> -	int (*eqos_start_clks)(struct udevice *dev);
> -	int (*eqos_calibrate_pads)(struct udevice *dev);
> -	int (*eqos_disable_calibration)(struct udevice *dev);
> -	int (*eqos_set_tx_clk_speed)(struct udevice *dev);
> -	ulong (*eqos_get_tick_clk_rate)(struct udevice *dev);
> -	phy_interface_t (*eqos_get_interface)(struct udevice *dev);
> -};
> -
> -struct eqos_priv {
> -	struct udevice *dev;
> -	const struct eqos_config *config;
> -	fdt_addr_t regs;
> -	struct eqos_mac_regs *mac_regs;
> -	struct eqos_mtl_regs *mtl_regs;
> -	struct eqos_dma_regs *dma_regs;
> -	struct eqos_tegra186_regs *tegra186_regs;
> -	struct reset_ctl reset_ctl;
> -	struct gpio_desc phy_reset_gpio;
> -	u32 reset_delays[3];
> -	struct clk clk_master_bus;
> -	struct clk clk_rx;
> -	struct clk clk_ptp_ref;
> -	struct clk clk_tx;
> -	struct clk clk_ck;
> -	struct clk clk_slave_bus;
> -	struct mii_dev *mii;
> -	struct phy_device *phy;
> -	int phyaddr;
> -	u32 max_speed;
> -	void *descs;
> -	struct eqos_desc *tx_descs;
> -	struct eqos_desc *rx_descs;
> -	int tx_desc_idx, rx_desc_idx;
> -	void *tx_dma_buf;
> -	void *rx_dma_buf;
> -	void *rx_pkt;
> -	bool started;
> -	bool reg_access_ok;
> -};
> -
>  /*
>   * TX and RX descriptors are 16 bytes. This causes problems with the cache
>   * maintenance on CPUs where the cache-line size exceeds the size of these
> @@ -1007,7 +944,7 @@ static int eqos_adjust_link(struct udevice *dev)
>  	return 0;
>  }
>  
> -static int eqos_write_hwaddr(struct udevice *dev)
> +int eqos_write_hwaddr(struct udevice *dev)
>  {
>  	struct eth_pdata *plat = dev_get_platdata(dev);
>  	struct eqos_priv *eqos = dev_get_priv(dev);
> @@ -1051,7 +988,7 @@ static int eqos_write_hwaddr(struct udevice *dev)
>  	return 0;
>  }
>  
> -static int eqos_init(struct udevice *dev)
> +int eqos_init(struct udevice *dev)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	int ret, limit;
> @@ -1161,7 +1098,7 @@ err:
>  	return ret;
>  }
>  
> -static void eqos_enable(struct udevice *dev)
> +void eqos_enable(struct udevice *dev)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
> @@ -1387,7 +1324,7 @@ static int eqos_start(struct udevice *dev)
>  	return 0;
>  }
>  
> -static void eqos_stop(struct udevice *dev)
> +void eqos_stop(struct udevice *dev)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	int i;
> @@ -1441,7 +1378,7 @@ static void eqos_stop(struct udevice *dev)
>  	debug("%s: OK\n", __func__);
>  }
>  
> -static int eqos_send(struct udevice *dev, void *packet, int length)
> +int eqos_send(struct udevice *dev, void *packet, int length)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	struct eqos_desc *tx_desc;
> @@ -1482,7 +1419,7 @@ static int eqos_send(struct udevice *dev, void *packet, int length)
>  	return -ETIMEDOUT;
>  }
>  
> -static int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
> +int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	struct eqos_desc *rx_desc;
> @@ -1506,7 +1443,7 @@ static int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
>  	return length;
>  }
>  
> -static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
> +int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	uchar *packet_expected;
> @@ -1833,7 +1770,7 @@ static int eqos_remove_resources_stm32(struct udevice *dev)
>  	return 0;
>  }
>  
> -static int eqos_probe(struct udevice *dev)
> +int eqos_probe(struct udevice *dev)
>  {
>  	struct eqos_priv *eqos = dev_get_priv(dev);
>  	int ret;

[PATCH 7/8] net: dwc_eth_qos: Export common struct and interface at head file

[Stephen Warren]

On 4/30/20 4:45 AM, David Wu wrote:
> Open structure data and interface, so that Soc using dw_eth_qos
> controller can reference.

This shouldn't be necessary; nothing outside of this driver should need
access to the registers.

At most, perhaps implement some additional functions so that other code
can call into the driver to perform specific actions. However, even
that's probably not a good idea.

[PATCH 8/8] net: gmac_rockchip: Add dwc_eth_qos support

[David Wu]

Change the original data structure so that Rockchip's Soc
gmac controller can support the designware.c and dwc_eth_qos.c
drivers, a Soc can only support one.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---

 drivers/net/Kconfig         |   2 +-
 drivers/net/gmac_rockchip.c | 160 ++++++++++++++++++++++++++++++------
 2 files changed, 135 insertions(+), 27 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 4d1013c984..07d2b0787c 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -482,7 +482,7 @@ config PIC32_ETH
 
 config GMAC_ROCKCHIP
 	bool "Rockchip Synopsys Designware Ethernet MAC"
-	depends on DM_ETH && ETH_DESIGNWARE
+	depends on DM_ETH && (ETH_DESIGNWARE || DWC_ETH_QOS)
 	help
 	  This driver provides Rockchip SoCs network support based on the
 	  Synopsys Designware driver.
diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
index e152faf083..aa2bab4203 100644
--- a/drivers/net/gmac_rockchip.c
+++ b/drivers/net/gmac_rockchip.c
@@ -25,26 +25,39 @@
 #include <dm/pinctrl.h>
 #include <dt-bindings/clock/rk3288-cru.h>
 #include "designware.h"
+#include "dwc_eth_qos.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 #define DELAY_ENABLE(soc, tx, rx) \
 	(((tx) ? soc##_TXCLK_DLY_ENA_GMAC_ENABLE : soc##_TXCLK_DLY_ENA_GMAC_DISABLE) | \
 	((rx) ? soc##_RXCLK_DLY_ENA_GMAC_ENABLE : soc##_RXCLK_DLY_ENA_GMAC_DISABLE))
 
+struct rockchip_eth_dev {
+	union {
+		struct eqos_priv eqos;
+		struct dw_eth_dev dw;
+	};
+};
+
 /*
  * Platform data for the gmac
  *
  * dw_eth_pdata: Required platform data for designware driver (must be first)
  */
 struct gmac_rockchip_platdata {
-	struct dw_eth_pdata dw_eth_pdata;
+	union {
+		struct dw_eth_pdata dw_eth_pdata;
+		struct eth_pdata eth_pdata;
+	};
+	bool has_gmac4;
 	bool clock_input;
 	int tx_delay;
 	int rx_delay;
 };
 
 struct rk_gmac_ops {
-	int (*fix_mac_speed)(struct dw_eth_dev *priv);
+	const struct eqos_config config;
+	int (*fix_mac_speed)(struct rockchip_eth_dev *dev);
 	void (*set_to_rmii)(struct gmac_rockchip_platdata *pdata);
 	void (*set_to_rgmii)(struct gmac_rockchip_platdata *pdata);
 };
@@ -55,6 +68,9 @@ static int gmac_rockchip_ofdata_to_platdata(struct udevice *dev)
 	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
 	const char *string;
 
+	if (device_is_compatible(dev, "snps,dwmac-4.20a"))
+		pdata->has_gmac4 = true;
+
 	string = dev_read_string(dev, "clock_in_out");
 	if (!strcmp(string, "input"))
 		pdata->clock_input = true;
@@ -71,11 +87,15 @@ static int gmac_rockchip_ofdata_to_platdata(struct udevice *dev)
 	if (pdata->rx_delay == -ENOENT)
 		pdata->rx_delay = dev_read_u32_default(dev, "rx-delay", 0x10);
 
-	return designware_eth_ofdata_to_platdata(dev);
+	if (!pdata->has_gmac4)
+		return designware_eth_ofdata_to_platdata(dev);
+
+	return 0;
 }
 
-static int px30_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int px30_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct px30_grf *grf;
 	struct clk clk_speed;
 	int speed, ret;
@@ -115,8 +135,9 @@ static int px30_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rk3228_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int rk3228_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rk322x_grf *grf;
 	int clk;
 	enum {
@@ -148,8 +169,9 @@ static int rk3228_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rk3288_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int rk3288_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rk3288_grf *grf;
 	int clk;
 
@@ -174,8 +196,9 @@ static int rk3288_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rk3308_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int rk3308_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rk3308_grf *grf;
 	struct clk clk_speed;
 	int speed, ret;
@@ -215,8 +238,9 @@ static int rk3308_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rk3328_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int rk3328_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rk3328_grf_regs *grf;
 	int clk;
 	enum {
@@ -248,8 +272,9 @@ static int rk3328_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rk3368_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int rk3368_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rk3368_grf *grf;
 	int clk;
 	enum {
@@ -280,8 +305,9 @@ static int rk3368_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rk3399_gmac_fix_mac_speed(struct dw_eth_dev *priv)
+static int rk3399_gmac_fix_mac_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rk3399_grf_regs *grf;
 	int clk;
 
@@ -306,8 +332,9 @@ static int rk3399_gmac_fix_mac_speed(struct dw_eth_dev *priv)
 	return 0;
 }
 
-static int rv1108_set_rmii_speed(struct dw_eth_dev *priv)
+static int rv1108_set_rmii_speed(struct rockchip_eth_dev *dev)
 {
+	struct dw_eth_dev *priv = &dev->dw;
 	struct rv1108_grf *grf;
 	int clk, speed;
 	enum {
@@ -555,12 +582,22 @@ static int gmac_rockchip_probe(struct udevice *dev)
 	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
 	struct rk_gmac_ops *ops =
 		(struct rk_gmac_ops *)dev_get_driver_data(dev);
-	struct dw_eth_pdata *dw_pdata = dev_get_platdata(dev);
-	struct eth_pdata *eth_pdata = &dw_pdata->eth_pdata;
+	struct dw_eth_pdata *dw_pdata;
+	struct eth_pdata *eth_pdata;
+	struct eqos_config *config;
 	struct clk clk;
 	ulong rate;
 	int ret;
 
+	if (pdata->has_gmac4) {
+		eth_pdata = &pdata->eth_pdata;
+		config = (struct eqos_config *)&ops->config;
+		eth_pdata->phy_interface = config->ops->eqos_get_interface(dev);
+	} else {
+		dw_pdata = &pdata->dw_eth_pdata;
+		eth_pdata = &dw_pdata->eth_pdata;
+	}
+
 	ret = clk_set_defaults(dev, 0);
 	if (ret)
 		debug("%s clk_set_defaults failed %d\n", __func__, ret);
@@ -656,37 +693,108 @@ static int gmac_rockchip_probe(struct udevice *dev)
 		return -ENXIO;
 	}
 
-	return designware_eth_probe(dev);
+	if (pdata->has_gmac4)
+		return eqos_probe(dev);
+	else
+		return designware_eth_probe(dev);
+}
+
+static int gmac_rockchip_eth_write_hwaddr(struct udevice *dev)
+{
+	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->has_gmac4)
+		return eqos_write_hwaddr(dev);
+	else
+		return designware_eth_write_hwaddr(dev);
+}
+
+static int gmac_rockchip_eth_free_pkt(struct udevice *dev, uchar *packet,
+				      int length)
+{
+	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->has_gmac4)
+		return eqos_free_pkt(dev, packet, length);
+	else
+		return designware_eth_free_pkt(dev, packet, length);
+}
+
+static int gmac_rockchip_eth_send(struct udevice *dev, void *packet,
+				  int length)
+{
+	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->has_gmac4)
+		return eqos_send(dev, packet, length);
+	else
+		return designware_eth_send(dev, packet, length);
+}
+
+static int gmac_rockchip_eth_recv(struct udevice *dev, int flags,
+				  uchar **packetp)
+{
+	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->has_gmac4)
+		return eqos_recv(dev, flags, packetp);
+	else
+		return designware_eth_recv(dev, flags, packetp);
 }
 
 static int gmac_rockchip_eth_start(struct udevice *dev)
 {
-	struct eth_pdata *pdata = dev_get_platdata(dev);
-	struct dw_eth_dev *priv = dev_get_priv(dev);
+	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
+	struct rockchip_eth_dev *priv = dev_get_priv(dev);
 	struct rk_gmac_ops *ops =
 		(struct rk_gmac_ops *)dev_get_driver_data(dev);
+	struct dw_eth_pdata *dw_pdata;
+	struct eth_pdata *eth_pdata;
 	int ret;
 
-	ret = designware_eth_init(priv, pdata->enetaddr);
+	if (pdata->has_gmac4) {
+		ret = eqos_init(dev);
+	} else {
+		dw_pdata = &pdata->dw_eth_pdata;
+		eth_pdata = &dw_pdata->eth_pdata;
+		ret = designware_eth_init((struct dw_eth_dev *)priv,
+					  eth_pdata->enetaddr);
+	}
 	if (ret)
 		return ret;
+
 	ret = ops->fix_mac_speed(priv);
 	if (ret)
 		return ret;
-	ret = designware_eth_enable(priv);
-	if (ret)
-		return ret;
+
+	if (pdata->has_gmac4) {
+		eqos_enable(dev);
+	} else {
+		ret = designware_eth_enable((struct dw_eth_dev *)priv);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
 
+static void gmac_rockchip_eth_stop(struct udevice *dev)
+{
+	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->has_gmac4)
+		eqos_stop(dev);
+	else
+		designware_eth_stop(dev);
+}
+
 const struct eth_ops gmac_rockchip_eth_ops = {
 	.start			= gmac_rockchip_eth_start,
-	.send			= designware_eth_send,
-	.recv			= designware_eth_recv,
-	.free_pkt		= designware_eth_free_pkt,
-	.stop			= designware_eth_stop,
-	.write_hwaddr		= designware_eth_write_hwaddr,
+	.send			= gmac_rockchip_eth_send,
+	.recv			= gmac_rockchip_eth_recv,
+	.free_pkt		= gmac_rockchip_eth_free_pkt,
+	.stop			= gmac_rockchip_eth_stop,
+	.write_hwaddr		= gmac_rockchip_eth_write_hwaddr,
 };
 
 const struct rk_gmac_ops px30_gmac_ops = {
@@ -756,7 +864,7 @@ U_BOOT_DRIVER(eth_gmac_rockchip) = {
 	.ofdata_to_platdata = gmac_rockchip_ofdata_to_platdata,
 	.probe	= gmac_rockchip_probe,
 	.ops	= &gmac_rockchip_eth_ops,
-	.priv_auto_alloc_size = sizeof(struct dw_eth_dev),
+	.priv_auto_alloc_size = sizeof(struct rockchip_eth_dev),
 	.platdata_auto_alloc_size = sizeof(struct gmac_rockchip_platdata),
 	.flags = DM_FLAG_ALLOC_PRIV_DMA,
 };
-- 
2.19.1

[PATCH 8/8] net: gmac_rockchip: Add dwc_eth_qos support

[Stephen Warren]

On 4/30/20 4:45 AM, David Wu wrote:
> Change the original data structure so that Rockchip's Soc
> gmac controller can support the designware.c and dwc_eth_qos.c
> drivers, a Soc can only support one.

I'm really confused; with a filename like gmac_rockchip.c that sounds
like it's driver for a MAC device. DWC EQoS is also a MAC device. The
two shouldn't be related or coupled in any way.

I think what you need is to completely drop this patch (and the patch
which creates dwc_eth_qos.h), and instead make the DWC EQoS driver
itself directly support the Rockchip SoC by adding RK's compatible value
to the list of compatible values that the EQoS driver supports, along
with new probe functions etc.

Maybe this requires splitting some PHY code out of gmac_rockchip into a
common/separate PHY driver? I haven't looked at the code to know if
that's required.

[PATCH 8/8] net: gmac_rockchip: Add dwc_eth_qos support

[David Wu]

Hi Stephen,

? 2020/5/1 ??6:52, Stephen Warren ??:
> I'm really confused; with a filename like gmac_rockchip.c that sounds
> like it's driver for a MAC device. DWC EQoS is also a MAC device. The
> two shouldn't be related or coupled in any way.
> 
> I think what you need is to completely drop this patch (and the patch
> which creates dwc_eth_qos.h), and instead make the DWC EQoS driver
> itself directly support the Rockchip SoC by adding RK's compatible value
> to the list of compatible values that the EQoS driver supports, along
> with new probe functions etc.
> 
> Maybe this requires splitting some PHY code out of gmac_rockchip into a
> common/separate PHY driver? I haven't looked at the code to know if
> that's required.

I think this relationship is like the current designware.c and 
gmac_rockchip.c, except that the designware driver becomes DWC EQoS. In 
fact, most of the code is the same, because it is the same controller, 
and there are a few differences related to Soc implemented in 
gmac_rockchip.c, this is the purpose of my series of patches, and the 
code must be compatible with the previous Rockchip Socs.