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 = _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 = _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, 250);
+    if (ret)
+    dev_err(dev, "%s: set clk_mac_speed rate 250 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, 2500);
+    if (ret)
+    dev_err(dev, "%s: set clk_mac_speed rate 2500 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 = _ops   },
  { .compatible = "rockchip,rk3128-gmac", .data = _ops },
  { .compatible = "rockchip,rk3228-gmac", .data = _ops },
  { .compatible = "rockchip,rk3288-gmac", .data = _ops },

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 


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,-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_10MGRF_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 = _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 = _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, 250);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 250 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, 2500);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 2500 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, 

[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 
---
 .../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,-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_10MGRF_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 = _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 = _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, 250);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 250 
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, 2500);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 2500 
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_CON00x0168
 #define RK3128_GRF_MAC_CON10x016c
 
@@ -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 = _ops   },
{ .compatible = "rockchip,rk3128-gmac", .data = _ops },
{ .compatible = "rockchip,rk3228-gmac", .data = _ops },
{ .compatible = "rockchip,rk3288-gmac", .data = _ops },
-- 
2.7.4