[PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[David Wu]

Add constants and callback functions for the dwmac on px30 soc.
The base structure is the same, but registers and the bits in
them moved slightly, and add the clk_mac_speed for the select
of mac speed.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 .../devicetree/bindings/net/rockchip-dwmac.txt     |  1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     | 64 ++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
index 9c16ee2..3b71da7 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
@@ -4,6 +4,7 @@ The device node has following properties.
 
 Required properties:
  - compatible: should be "rockchip,<name>-gamc"
+   "rockchip,px30-gmac":   found on PX30 SoCs
    "rockchip,rk3128-gmac": found on RK312x SoCs
    "rockchip,rk3228-gmac": found on RK322x SoCs
    "rockchip,rk3288-gmac": found on RK3288 SoCs
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 13133b3..4b2ab71 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -61,6 +61,7 @@ struct rk_priv_data {
 	struct clk *mac_clk_tx;
 	struct clk *clk_mac_ref;
 	struct clk *clk_mac_refout;
+	struct clk *clk_mac_speed;
 	struct clk *aclk_mac;
 	struct clk *pclk_mac;
 	struct clk *clk_phy;
@@ -83,6 +84,64 @@ struct rk_priv_data {
 	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
 	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
 
+#define PX30_GRF_GMAC_CON1		0X0904
+
+/* PX30_GRF_GMAC_CON1 */
+#define PX30_GMAC_PHY_INTF_SEL_RMII	(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | \
+					GRF_BIT(6))
+#define PX30_GMAC_SPEED_10M		GRF_CLR_BIT(2)
+#define PX30_GMAC_SPEED_100M		GRF_BIT(2)
+
+static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
+{
+	struct device *dev = &bsp_priv->pdev->dev;
+
+	if (IS_ERR(bsp_priv->grf)) {
+		dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
+		return;
+	}
+
+	regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+		     PX30_GMAC_PHY_INTF_SEL_RMII);
+}
+
+static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
+{
+	struct device *dev = &bsp_priv->pdev->dev;
+	int ret;
+
+	if (IS_ERR(bsp_priv->clk_mac_speed)) {
+		dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
+		return;
+	}
+
+	if (speed == 10) {
+		regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+			     PX30_GMAC_SPEED_10M);
+
+		ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500000);
+		if (ret)
+			dev_err(dev, "%s: set clk_mac_speed rate 2500000 failed: %d\n",
+				__func__, ret);
+	} else if (speed == 100) {
+		regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+			     PX30_GMAC_SPEED_100M);
+
+		ret = clk_set_rate(bsp_priv->clk_mac_speed, 25000000);
+		if (ret)
+			dev_err(dev, "%s: set clk_mac_speed rate 25000000 failed: %d\n",
+				__func__, ret);
+
+	} else {
+		dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
+	}
+}
+
+static const struct rk_gmac_ops px30_ops = {
+	.set_to_rmii = px30_set_to_rmii,
+	.set_rmii_speed = px30_set_rmii_speed,
+};
+
 #define RK3128_GRF_MAC_CON0	0x0168
 #define RK3128_GRF_MAC_CON1	0x016c
 
@@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
 		}
 	}
 
+	bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
+	if (IS_ERR(bsp_priv->clk_mac_speed))
+		dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
+
 	if (bsp_priv->clock_input) {
 		dev_info(dev, "clock input from PHY\n");
 	} else {
@@ -1424,6 +1487,7 @@ static int rk_gmac_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(rk_gmac_pm_ops, rk_gmac_suspend, rk_gmac_resume);
 
 static const struct of_device_id rk_gmac_dwmac_match[] = {
+	{ .compatible = "rockchip,px30-gmac",	.data = &px30_ops   },
 	{ .compatible = "rockchip,rk3128-gmac", .data = &rk3128_ops },
 	{ .compatible = "rockchip,rk3228-gmac", .data = &rk3228_ops },
 	{ .compatible = "rockchip,rk3288-gmac", .data = &rk3288_ops },
-- 
2.7.4

Re: [PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[Shawn Lin]

Hi David,

On 2018/5/16 11:38, David Wu wrote:
> Add constants and callback functions for the dwmac on px30 soc.

s/soc/SoC

> The base structure is the same, but registers and the bits in
> them moved slightly, and add the clk_mac_speed for the select

s/moved/are moved

> of mac speed.

for selecting mas speed.

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

git log --oneline  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

shows very inconsistent format wrt. commit title, so please
follow the exsiting exsamples as possible.

> ---
>   .../devicetree/bindings/net/rockchip-dwmac.txt     |  1 +
>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     | 64 ++++++++++++++++++++++
>   2 files changed, 65 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> index 9c16ee2..3b71da7 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt

It'd be better to split doc changes into a separate patch.

> @@ -4,6 +4,7 @@ The device node has following properties.
>   
>   Required properties:
>    - compatible: should be "rockchip,<name>-gamc"
> +   "rockchip,px30-gmac":   found on PX30 SoCs
>      "rockchip,rk3128-gmac": found on RK312x SoCs
>      "rockchip,rk3228-gmac": found on RK322x SoCs
>      "rockchip,rk3288-gmac": found on RK3288 SoCs
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 13133b3..4b2ab71 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -61,6 +61,7 @@ struct rk_priv_data {
>   	struct clk *mac_clk_tx;
>   	struct clk *clk_mac_ref;
>   	struct clk *clk_mac_refout;
> +	struct clk *clk_mac_speed;

No need to do anything now but it seems you could consider doing some
cleanup by using clk bulk APIs in the future.

>   	struct clk *aclk_mac;
>   	struct clk *pclk_mac;
>   	struct clk *clk_phy;
> @@ -83,6 +84,64 @@ struct rk_priv_data {
>   	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
>   	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
>   
> +#define PX30_GRF_GMAC_CON1		0X0904

s/0X0904/0x0904 , since the other constants in this file follow the
same format.

> +
> +/* PX30_GRF_GMAC_CON1 */
> +#define PX30_GMAC_PHY_INTF_SEL_RMII	(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | \
> +					GRF_BIT(6))
> +#define PX30_GMAC_SPEED_10M		GRF_CLR_BIT(2)
> +#define PX30_GMAC_SPEED_100M		GRF_BIT(2)
> +
> +static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
> +{
> +	struct device *dev = &bsp_priv->pdev->dev;
> +
> +	if (IS_ERR(bsp_priv->grf)) {
> +		dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
> +		return;
> +	}
> +
> +	regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> +		     PX30_GMAC_PHY_INTF_SEL_RMII);
> +}
> +
> +static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
> +{
> +	struct device *dev = &bsp_priv->pdev->dev;
> +	int ret;
> +
> +	if (IS_ERR(bsp_priv->clk_mac_speed)) {
> +		dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
> +		return;
> +	}
> +
> +	if (speed == 10) {
> +		regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> +			     PX30_GMAC_SPEED_10M);
> +
> +		ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500000);
> +		if (ret)
> +			dev_err(dev, "%s: set clk_mac_speed rate 2500000 failed: %d\n",
> +				__func__, ret);
> +	} else if (speed == 100) {
> +		regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> +			     PX30_GMAC_SPEED_100M);
> +
> +		ret = clk_set_rate(bsp_priv->clk_mac_speed, 25000000);
> +		if (ret)
> +			dev_err(dev, "%s: set clk_mac_speed rate 25000000 failed: %d\n",
> +				__func__, ret);

I know it follows the existing examples, but IMHO it duplicates
unnecessary code as all the difference is PX30_GMAC_SPEED_*

> +
> +	} else {
> +		dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
> +	}
> +}
> +
> +static const struct rk_gmac_ops px30_ops = {
> +	.set_to_rmii = px30_set_to_rmii,
> +	.set_rmii_speed = px30_set_rmii_speed,
> +};
> +
>   #define RK3128_GRF_MAC_CON0	0x0168
>   #define RK3128_GRF_MAC_CON1	0x016c
>   
> @@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
>   		}
>   	}
>   
> +	bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");

Mightbe it'd be better to use "mac-speed" in DT bindings.

> +	if (IS_ERR(bsp_priv->clk_mac_speed))
> +		dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
> +

Would you like to handle deferred probe?

>   	if (bsp_priv->clock_input) {
>   		dev_info(dev, "clock input from PHY\n");
>   	} else {
> @@ -1424,6 +1487,7 @@ static int rk_gmac_resume(struct device *dev)
>   static SIMPLE_DEV_PM_OPS(rk_gmac_pm_ops, rk_gmac_suspend, rk_gmac_resume);
>   
>   static const struct of_device_id rk_gmac_dwmac_match[] = {
> +	{ .compatible = "rockchip,px30-gmac",	.data = &px30_ops   },
>   	{ .compatible = "rockchip,rk3128-gmac", .data = &rk3128_ops },
>   	{ .compatible = "rockchip,rk3228-gmac", .data = &rk3228_ops },
>   	{ .compatible = "rockchip,rk3288-gmac", .data = &rk3288_ops },
> 

Re: [PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[David Wu]

Hi Shawn,

Thanks for the suggestion, the most is okay.

在 2018年05月16日 14:34, Shawn Lin 写道:
> Hi David,
> 
> On 2018/5/16 11:38, David Wu wrote:
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> index 13133b3..4b2ab71 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> @@ -61,6 +61,7 @@ struct rk_priv_data {
>>       struct clk *mac_clk_tx;
>>       struct clk *clk_mac_ref;
>>       struct clk *clk_mac_refout;
>> +    struct clk *clk_mac_speed;
> 
> No need to do anything now but it seems you could consider doing some
> cleanup by using clk bulk APIs in the future.

The use of this may seem to be less applicable because there are many 
scenarios using different clocks.

> 
>>       struct clk *aclk_mac;
>>       struct clk *pclk_mac;
>>       struct clk *clk_phy;
>> @@ -83,6 +84,64 @@ struct rk_priv_data {
>>       (((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : 
>> soc##_GMAC_TXCLK_DLY_DISABLE) | \
>>        ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : 
>> soc##_GMAC_RXCLK_DLY_DISABLE))
>> +#define PX30_GRF_GMAC_CON1        0X0904
> 
> s/0X0904/0x0904 , since the other constants in this file follow the
> same format.
> 
>> +
>> +/* PX30_GRF_GMAC_CON1 */
>> +#define PX30_GMAC_PHY_INTF_SEL_RMII    (GRF_CLR_BIT(4) | 
>> GRF_CLR_BIT(5) | \
>> +                    GRF_BIT(6))
>> +#define PX30_GMAC_SPEED_10M        GRF_CLR_BIT(2)
>> +#define PX30_GMAC_SPEED_100M        GRF_BIT(2)
>> +
>> +static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
>> +{
>> +    struct device *dev = &bsp_priv->pdev->dev;
>> +
>> +    if (IS_ERR(bsp_priv->grf)) {
>> +        dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
>> +        return;
>> +    }
>> +
>> +    regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
>> +             PX30_GMAC_PHY_INTF_SEL_RMII);
>> +}
>> +
>> +static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int 
>> speed)
>> +{
>> +    struct device *dev = &bsp_priv->pdev->dev;
>> +    int ret;
>> +
>> +    if (IS_ERR(bsp_priv->clk_mac_speed)) {
>> +        dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
>> +        return;
>> +    }
>> +
>> +    if (speed == 10) {
>> +        regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
>> +                 PX30_GMAC_SPEED_10M);
>> +
>> +        ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500000);
>> +        if (ret)
>> +            dev_err(dev, "%s: set clk_mac_speed rate 2500000 failed: 
>> %d\n",
>> +                __func__, ret);
>> +    } else if (speed == 100) {
>> +        regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
>> +                 PX30_GMAC_SPEED_100M);
>> +
>> +        ret = clk_set_rate(bsp_priv->clk_mac_speed, 25000000);
>> +        if (ret)
>> +            dev_err(dev, "%s: set clk_mac_speed rate 25000000 failed: 
>> %d\n",
>> +                __func__, ret);
> 
> I know it follows the existing examples, but IMHO it duplicates
> unnecessary code as all the difference is PX30_GMAC_SPEED_*
> 

i think the difference is the register offset and bits.

>> +
>> +    } else {
>> +        dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
>> +    }
>> +}
>> +
>> +static const struct rk_gmac_ops px30_ops = {
>> +    .set_to_rmii = px30_set_to_rmii,
>> +    .set_rmii_speed = px30_set_rmii_speed,
>> +};
>> +
>>   #define RK3128_GRF_MAC_CON0    0x0168
>>   #define RK3128_GRF_MAC_CON1    0x016c
>> @@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct 
>> plat_stmmacenet_data *plat)
>>           }
>>       }
>> +    bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
> 
> Mightbe it'd be better to use "mac-speed" in DT bindings.
> 
>> +    if (IS_ERR(bsp_priv->clk_mac_speed))
>> +        dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
>> +
> 
> Would you like to handle deferred probe >

No,

>>       if (bsp_priv->clock_input) {
>>           dev_info(dev, "clock input from PHY\n");
>>       } else {
>> @@ -1424,6 +1487,7 @@ static int rk_gmac_resume(struct device *dev)
>>   static SIMPLE_DEV_PM_OPS(rk_gmac_pm_ops, rk_gmac_suspend, 
>> rk_gmac_resume);
>>   static const struct of_device_id rk_gmac_dwmac_match[] = {
>> +    { .compatible = "rockchip,px30-gmac",    .data = &px30_ops   },
>>       { .compatible = "rockchip,rk3128-gmac", .data = &rk3128_ops },
>>       { .compatible = "rockchip,rk3228-gmac", .data = &rk3228_ops },
>>       { .compatible = "rockchip,rk3288-gmac", .data = &rk3288_ops },
>>
> 
> 
>