RE: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

Hi Nicolas Ferre,
	
	Thanks for the quick review...

Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :

quoted

This patch adds support for gmii2rgmii phy converter in the macb
driver.

Okay, I'd like more explanation here.
Hints & key words:
- dt property
- mdio bus
- mac speed

Sure will fix in the next version...

quoted

Signed-off-by: Kedareswara rao Appana <redacted>
---
 drivers/net/ethernet/cadence/macb.c |   21 ++++++++++++++++++++-
 drivers/net/ethernet/cadence/macb.h |    3 +++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c

b/drivers/net/ethernet/cadence/macb.c
index cb07d95..de0ad71 100644

--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c

@@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int

mii_id, int regnum,

quoted

 	return 0;
 }

+static inline void macb_hw_fix_mac_speed(struct macb *bp,
+					 struct phy_device *phydev)
+{
+	if (likely(bp->converter_phy.fix_mac_speed))

What is the purpose of this branch bias? The code isn't in some hot path, so I
suspect that its not needed.

If we won't put this check driver will crash with NULL pointer dereference for the below cases
---> Converter driver is disabled
---> Converter driver is enabled but the converter probe is not called from the macb driver.

quoted

+		bp->converter_phy.fix_mac_speed(&bp->converter_phy,
+						phydev->speed);
+}
+
 /**
  * macb_set_tx_clk() - Set a clock to a new frequency
  * @clk		Pointer to the clock to change

@@ -329,6 +337,7 @@ static void macb_handle_link_change(struct

net_device *dev)

quoted

 				reg |= GEM_BIT(GBE);

 			macb_or_gem_writel(bp, NCFGR, reg);
+			macb_hw_fix_mac_speed(bp, phydev);

 			bp->speed = phydev->speed;
 			bp->duplex = phydev->duplex;

@@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device

*pdev)

quoted

 			struct clk **, struct clk **)
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node, *np1, *np11;

Nitpicking:
Be more explicit on variable names. Simple name for the iterator is okay, the
other is better if changed.
I also like to see variable on separated lines.

Ok sure will fix in the next version...

quoted

 	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL; @@ -3011,6 +3020,16

@@ static int macb_probe(struct platform_device *pdev)
 		goto err_out_free_netdev;

 	phydev = bp->phy_dev;
+	np1 = of_get_next_child(np, NULL);
+	for_each_child_of_node(np1, np11) {
+		if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {

This would definitively need documentation and at least link in the macb binding
to show how this emulated phy connect to the mdio bus...

I'm not able to judge if the node arrangement is okay: I let Florian tell his view
on this...

Ok will document in the macb binding doc.

quoted

+			bp->converter_phy.dev = dev;
+			bp->converter_phy.mii_bus = bp->mii_bus;
+			bp->converter_phy.mdio_write = macb_mdio_write;
+			bp->converter_phy.platform_data = bp->pdev-
dev.of_node;
+			gmii2rgmii_phyprobe(&bp->converter_phy);
+		}
+	}

 	netif_carrier_off(dev);

diff --git a/drivers/net/ethernet/cadence/macb.h

b/drivers/net/ethernet/cadence/macb.h
index 8a13824..735bce2 100644

--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h

@@ -10,6 +10,8 @@
 #ifndef _MACB_H
 #define _MACB_H

+#include <linux/xilinx_gmii2rgmii.h>

No, put it in the macb.c.

Ok will fix in v2...

quoted

+
 #define MACB_GREGS_NBR 16
 #define MACB_GREGS_VERSION 2
 #define MACB_MAX_QUEUES 8

@@ -846,6 +848,7 @@ struct macb {
 	unsigned int		jumbo_max_len;

 	u32			wol;
+	struct	gmii2rgmii	converter_phy;
 };

 static inline bool macb_is_gem(struct macb *bp)

If Florian and phy guys are okay with the approach, I'm fine with this patch, once
corrected.

Ok thanks...

Regards,
Kedar.

Thanks, bye,
--
Nicolas Ferre