[PATCH v2 0/8] ARM: Berlin: Ethernet support

[PATCH v2 0/8] ARM: Berlin: Ethernet support

[Antoine Tenart]

Hi all,

This series introduce support for the Ethernet controller on Berlin SoCs,
using the existing pxa168 Ethernet driver. In order to do this, DT
support is added to the driver alongside some other modifications and
fixes.

Version 1 of this series was introducing a new Ethernet driver but thanks
to Sebastian's review it seems we can actually use the existing pxa168
Ethernet driver. This second version uses it.

This has been tested on a Berlin BG2Q DMP board.

Changes since v1:
	- removed custom Berlin Ethernet driver
	- used the pxa168 Ethernet driver instead
	- made modifications to the pxa168 driver (DT support, fixes)

Antoine Tenart (8):
  net: pxa168_eth: clean up
  net: pxa168_eth: add device tree support
  Documentation: bindings: net: add the Marvell PXA168 Ethernet
    controller
  net: pxa168_eth: fix Ethernet flow control status
  net: pxa168_eth: get and set the mac address on the Ethernet
    controller
  net: pxa168_eth: allow Berlin SoCs to use the pxa168_eth driver
  ARM: dts: berlin: add the Ethernet node
  ARM: dts: berlin: enable the Ethernet port on the BG2Q DMP

 .../devicetree/bindings/net/marvell-pxa168.txt     |  22 +++
 arch/arm/boot/dts/berlin2q-marvell-dmp.dts         |   4 +
 arch/arm/boot/dts/berlin2q.dtsi                    |   9 +
 drivers/net/ethernet/marvell/Kconfig               |   2 +-
 drivers/net/ethernet/marvell/pxa168_eth.c          | 207 +++++++++++++--------
 5 files changed, 165 insertions(+), 79 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/marvell-pxa168.txt

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/8] net: pxa168_eth: clean up

[Antoine Tenart]

Clean up a bit the pxa168_eth driver before adding the device tree
support.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 102 +++++++++++++++---------------
 1 file changed, 50 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 8f5aa7c62b18..b370162dbe02 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -22,27 +22,29 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <linux/dma-mapping.h>
-#include <linux/in.h>
-#include <linux/ip.h>
-#include <linux/tcp.h>
-#include <linux/udp.h>
-#include <linux/etherdevice.h>
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/etherdevice.h>
 #include <linux/ethtool.h>
-#include <linux/platform_device.h>
-#include <linux/module.h>
+#include <linux/in.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ip.h>
 #include <linux/kernel.h>
-#include <linux/workqueue.h>
-#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of.h>
 #include <linux/phy.h>
-#include <linux/io.h>
-#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/pxa168_eth.h>
+#include <linux/tcp.h>
 #include <linux/types.h>
+#include <linux/udp.h>
+#include <linux/workqueue.h>
+
 #include <asm/pgtable.h>
 #include <asm/cacheflush.h>
-#include <linux/pxa168_eth.h>
 
 #define DRIVER_NAME	"pxa168-eth"
 #define DRIVER_VERSION	"0.3"
@@ -296,7 +298,7 @@ static void abort_dma(struct pxa168_eth_private *pep)
 	} while (max_retries-- > 0 && delay <= 0);
 
 	if (max_retries <= 0)
-		printk(KERN_ERR "%s : DMA Stuck\n", __func__);
+		netdev_err(pep->dev, "%s : DMA Stuck\n", __func__);
 }
 
 static int ethernet_phy_get(struct pxa168_eth_private *pep)
@@ -507,9 +509,10 @@ static int add_del_hash_entry(struct pxa168_eth_private *pep,
 
 	if (i == HOP_NUMBER) {
 		if (!del) {
-			printk(KERN_INFO "%s: table section is full, need to "
-					"move to 16kB implementation?\n",
-					 __FILE__);
+			netdev_info(pep->dev,
+				    "%s: table section is full, need to "
+				    "move to 16kB implementation?\n",
+				    __FILE__);
 			return -ENOSPC;
 		} else
 			return 0;
@@ -726,7 +729,7 @@ static int txq_reclaim(struct net_device *dev, int force)
 
 		if (cmd_sts & TX_ERROR) {
 			if (net_ratelimit())
-				printk(KERN_ERR "%s: Error in TX\n", dev->name);
+				netdev_err(dev, "Error in TX\n");
 			dev->stats.tx_errors++;
 		}
 		dma_unmap_single(NULL, addr, count, DMA_TO_DEVICE);
@@ -743,8 +746,7 @@ static void pxa168_eth_tx_timeout(struct net_device *dev)
 {
 	struct pxa168_eth_private *pep = netdev_priv(dev);
 
-	printk(KERN_INFO "%s: TX timeout  desc_count %d\n",
-	       dev->name, pep->tx_desc_count);
+	netdev_info(dev, "TX timeout  desc_count %d\n", pep->tx_desc_count);
 
 	schedule_work(&pep->tx_timeout_task);
 }
@@ -814,9 +816,8 @@ static int rxq_process(struct net_device *dev, int budget)
 			if ((cmd_sts & (RX_FIRST_DESC | RX_LAST_DESC)) !=
 			    (RX_FIRST_DESC | RX_LAST_DESC)) {
 				if (net_ratelimit())
-					printk(KERN_ERR
-					       "%s: Rx pkt on multiple desc\n",
-					       dev->name);
+					netdev_err(dev,
+						   "Rx pkt on multiple desc\n");
 			}
 			if (cmd_sts & RX_ERROR)
 				stats->rx_errors++;
@@ -871,7 +872,7 @@ static void handle_link_event(struct pxa168_eth_private *pep)
 	port_status = rdl(pep, PORT_STATUS);
 	if (!(port_status & LINK_UP)) {
 		if (netif_carrier_ok(dev)) {
-			printk(KERN_INFO "%s: link down\n", dev->name);
+			netdev_info(dev, "link down\n");
 			netif_carrier_off(dev);
 			txq_reclaim(dev, 1);
 		}
@@ -884,9 +885,8 @@ static void handle_link_event(struct pxa168_eth_private *pep)
 
 	duplex = (port_status & FULL_DUPLEX) ? 1 : 0;
 	fc = (port_status & FLOW_CONTROL_ENABLED) ? 1 : 0;
-	printk(KERN_INFO "%s: link up, %d Mb/s, %s duplex, "
-	       "flow control %sabled\n", dev->name,
-	       speed, duplex ? "full" : "half", fc ? "en" : "dis");
+	netdev_info(dev, "link up, %d Mb/s, %s duplex, flow control %sabled\n",
+		    speed, duplex ? "full" : "half", fc ? "en" : "dis");
 	if (!netif_carrier_ok(dev))
 		netif_carrier_on(dev);
 }
@@ -1039,9 +1039,8 @@ static void rxq_deinit(struct net_device *dev)
 		}
 	}
 	if (pep->rx_desc_count)
-		printk(KERN_ERR
-		       "Error in freeing Rx Ring. %d skb's still\n",
-		       pep->rx_desc_count);
+		netdev_err(dev, "Error in freeing Rx Ring. %d skb's still\n",
+			   pep->rx_desc_count);
 	/* Free RX ring */
 	if (pep->p_rx_desc_area)
 		dma_free_coherent(pep->dev->dev.parent, pep->rx_desc_area_size,
@@ -1280,15 +1279,15 @@ static int pxa168_smi_read(struct mii_bus *bus, int phy_addr, int regnum)
 	int val;
 
 	if (smi_wait_ready(pep)) {
-		printk(KERN_WARNING "pxa168_eth: SMI bus busy timeout\n");
+		netdev_warn(pep->dev, "pxa168_eth: SMI bus busy timeout\n");
 		return -ETIMEDOUT;
 	}
 	wrl(pep, SMI, (phy_addr << 16) | (regnum << 21) | SMI_OP_R);
 	/* now wait for the data to be valid */
 	for (i = 0; !((val = rdl(pep, SMI)) & SMI_R_VALID); i++) {
 		if (i == PHY_WAIT_ITERATIONS) {
-			printk(KERN_WARNING
-				"pxa168_eth: SMI bus read not valid\n");
+			netdev_warn(pep->dev,
+				    "pxa168_eth: SMI bus read not valid\n");
 			return -ENODEV;
 		}
 		msleep(10);
@@ -1303,7 +1302,7 @@ static int pxa168_smi_write(struct mii_bus *bus, int phy_addr, int regnum,
 	struct pxa168_eth_private *pep = bus->priv;
 
 	if (smi_wait_ready(pep)) {
-		printk(KERN_WARNING "pxa168_eth: SMI bus busy timeout\n");
+		netdev_warn(pep->dev, "pxa168_eth: SMI bus busy timeout\n");
 		return -ETIMEDOUT;
 	}
 
@@ -1311,7 +1310,7 @@ static int pxa168_smi_write(struct mii_bus *bus, int phy_addr, int regnum,
 	    SMI_OP_W | (value & 0xffff));
 
 	if (smi_wait_ready(pep)) {
-		printk(KERN_ERR "pxa168_eth: SMI bus busy timeout\n");
+		netdev_err(pep->dev, "pxa168_eth: SMI bus busy timeout\n");
 		return -ETIMEDOUT;
 	}
 
@@ -1425,23 +1424,23 @@ static void pxa168_get_drvinfo(struct net_device *dev,
 }
 
 static const struct ethtool_ops pxa168_ethtool_ops = {
-	.get_settings = pxa168_get_settings,
-	.set_settings = pxa168_set_settings,
-	.get_drvinfo = pxa168_get_drvinfo,
-	.get_link = ethtool_op_get_link,
-	.get_ts_info = ethtool_op_get_ts_info,
+	.get_settings	= pxa168_get_settings,
+	.set_settings	= pxa168_set_settings,
+	.get_drvinfo	= pxa168_get_drvinfo,
+	.get_link	= ethtool_op_get_link,
+	.get_ts_info	= ethtool_op_get_ts_info,
 };
 
 static const struct net_device_ops pxa168_eth_netdev_ops = {
-	.ndo_open = pxa168_eth_open,
-	.ndo_stop = pxa168_eth_stop,
-	.ndo_start_xmit = pxa168_eth_start_xmit,
-	.ndo_set_rx_mode = pxa168_eth_set_rx_mode,
-	.ndo_set_mac_address = pxa168_eth_set_mac_address,
-	.ndo_validate_addr = eth_validate_addr,
-	.ndo_do_ioctl = pxa168_eth_do_ioctl,
-	.ndo_change_mtu = pxa168_eth_change_mtu,
-	.ndo_tx_timeout = pxa168_eth_tx_timeout,
+	.ndo_open		= pxa168_eth_open,
+	.ndo_stop		= pxa168_eth_stop,
+	.ndo_start_xmit		= pxa168_eth_start_xmit,
+	.ndo_set_rx_mode	= pxa168_eth_set_rx_mode,
+	.ndo_set_mac_address	= pxa168_eth_set_mac_address,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_do_ioctl		= pxa168_eth_do_ioctl,
+	.ndo_change_mtu		= pxa168_eth_change_mtu,
+	.ndo_tx_timeout		= pxa168_eth_tx_timeout,
 };
 
 static int pxa168_eth_probe(struct platform_device *pdev)
@@ -1456,8 +1455,7 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 
 	clk = clk_get(&pdev->dev, "MFUCLK");
 	if (IS_ERR(clk)) {
-		printk(KERN_ERR "%s: Fast Ethernet failed to get clock\n",
-			DRIVER_NAME);
+		dev_err(&pdev->dev, "Fast Ethernet failed to get clock\n");
 		return -ENODEV;
 	}
 	clk_enable(clk);
@@ -1492,7 +1490,7 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 
 	INIT_WORK(&pep->tx_timeout_task, pxa168_eth_tx_timeout_task);
 
-	printk(KERN_INFO "%s:Using random mac address\n", DRIVER_NAME);
+	dev_info(&pdev->dev, "Using random mac address\n");
 	eth_hw_addr_random(dev);
 
 	pep->pd = dev_get_platdata(&pdev->dev);
-- 
1.9.1

[PATCH v2 2/8] net: pxa168_eth: add device tree support

[Antoine Tenart]

Add the device tree support to the pxa168_eth driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 67 ++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index b370162dbe02..953112f87c5f 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -193,6 +193,7 @@ struct tx_desc {
 
 struct pxa168_eth_private {
 	int port_num;		/* User Ethernet port number    */
+	int phy_addr;
 
 	int rx_resource_err;	/* Rx ring resource error flag */
 
@@ -1360,24 +1361,25 @@ static struct phy_device *phy_scan(struct pxa168_eth_private *pep, int phy_addr)
 	return phydev;
 }
 
-static void phy_init(struct pxa168_eth_private *pep, int speed, int duplex)
+static void phy_init(struct pxa168_eth_private *pep)
 {
 	struct phy_device *phy = pep->phy;
 
 	phy_attach(pep->dev, dev_name(&phy->dev), PHY_INTERFACE_MODE_MII);
 
-	if (speed == 0) {
+	if (pep->pd && pep->pd->speed != 0) {
+		phy->autoneg = AUTONEG_DISABLE;
+		phy->advertising = 0;
+		phy->speed = pep->pd->speed;
+		phy->duplex = pep->pd->duplex;
+	} else {
 		phy->autoneg = AUTONEG_ENABLE;
 		phy->speed = 0;
 		phy->duplex = 0;
 		phy->supported &= PHY_BASIC_FEATURES;
 		phy->advertising = phy->supported | ADVERTISED_Autoneg;
-	} else {
-		phy->autoneg = AUTONEG_DISABLE;
-		phy->advertising = 0;
-		phy->speed = speed;
-		phy->duplex = duplex;
 	}
+
 	phy_start_aneg(phy);
 }
 
@@ -1385,11 +1387,13 @@ static int ethernet_phy_setup(struct net_device *dev)
 {
 	struct pxa168_eth_private *pep = netdev_priv(dev);
 
-	if (pep->pd->init)
+	if (pep->pd && pep->pd->init)
 		pep->pd->init();
-	pep->phy = phy_scan(pep, pep->pd->phy_addr & 0x1f);
+
+	pep->phy = phy_scan(pep, pep->phy_addr & 0x1f);
 	if (pep->phy != NULL)
-		phy_init(pep, pep->pd->speed, pep->pd->duplex);
+		phy_init(pep);
+
 	update_hash_table_mac_address(pep, NULL, dev->dev_addr);
 
 	return 0;
@@ -1453,12 +1457,12 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 
 	printk(KERN_NOTICE "PXA168 10/100 Ethernet Driver\n");
 
-	clk = clk_get(&pdev->dev, "MFUCLK");
+	clk = devm_clk_get(&pdev->dev, "MFUCLK");
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "Fast Ethernet failed to get clock\n");
 		return -ENODEV;
 	}
-	clk_enable(clk);
+	clk_prepare_enable(clk);
 
 	dev = alloc_etherdev(sizeof(struct pxa168_eth_private));
 	if (!dev) {
@@ -1475,8 +1479,8 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 		err = -ENODEV;
 		goto err_netdev;
 	}
-	pep->base = ioremap(res->start, resource_size(res));
-	if (pep->base == NULL) {
+	pep->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pep->base)) {
 		err = -ENOMEM;
 		goto err_netdev;
 	}
@@ -1493,16 +1497,26 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "Using random mac address\n");
 	eth_hw_addr_random(dev);
 
-	pep->pd = dev_get_platdata(&pdev->dev);
 	pep->rx_ring_size = NUM_RX_DESCS;
-	if (pep->pd->rx_queue_size)
-		pep->rx_ring_size = pep->pd->rx_queue_size;
-
 	pep->tx_ring_size = NUM_TX_DESCS;
-	if (pep->pd->tx_queue_size)
-		pep->tx_ring_size = pep->pd->tx_queue_size;
 
-	pep->port_num = pep->pd->port_number;
+	pep->pd = dev_get_platdata(&pdev->dev);
+	if (pep->pd) {
+		if (pep->pd->rx_queue_size)
+			pep->rx_ring_size = pep->pd->rx_queue_size;
+
+		if (pep->pd->tx_queue_size)
+			pep->tx_ring_size = pep->pd->tx_queue_size;
+
+		pep->port_num = pep->pd->port_number;
+		pep->phy_addr = pep->pd->phy_addr;
+	} else if (pdev->dev.of_node) {
+		of_property_read_u32(pdev->dev.of_node, "port-id",
+				     &pep->port_num);
+		of_property_read_u32(pdev->dev.of_node, "phy-addr",
+				     &pep->phy_addr);
+	}
+
 	/* Hardware supports only 3 ports */
 	BUG_ON(pep->port_num > 2);
 	netif_napi_add(dev, &pep->napi, pxa168_rx_poll, pep->rx_ring_size);
@@ -1603,6 +1617,12 @@ static int pxa168_eth_suspend(struct platform_device *pdev, pm_message_t state)
 #define pxa168_eth_suspend NULL
 #endif
 
+static const struct of_device_id pxa168_eth_of_match[] = {
+	{ .compatible = "marvell,pxa168-eth" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pxa168_eth_of_match)
+
 static struct platform_driver pxa168_eth_driver = {
 	.probe = pxa168_eth_probe,
 	.remove = pxa168_eth_remove,
@@ -1610,8 +1630,9 @@ static struct platform_driver pxa168_eth_driver = {
 	.resume = pxa168_eth_resume,
 	.suspend = pxa168_eth_suspend,
 	.driver = {
-		   .name = DRIVER_NAME,
-		   },
+		.name		= DRIVER_NAME,
+		.of_match_table	= of_match_ptr(pxa168_eth_of_match),
+	},
 };
 
 module_platform_driver(pxa168_eth_driver);
-- 
1.9.1

[PATCH v2 3/8] Documentation: bindings: net: add the Marvell PXA168 Ethernet controller

[Antoine Tenart]

This adds the binding documentation for the Marvell PXA168 Ethernet
controller, following its DT support.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/net/marvell-pxa168.txt     | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/marvell-pxa168.txt

diff --git a/Documentation/devicetree/bindings/net/marvell-pxa168.txt b/Documentation/devicetree/bindings/net/marvell-pxa168.txt
new file mode 100644
index 000000000000..3a5320ad09a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell-pxa168.txt
@@ -0,0 +1,22 @@
+* Marvell PXA168 Ethernet Controller
+
+Required properties:
+- compatible: should be "marvell,pxa168-eth".
+- reg: address and length of the register set for the device.
+- interrupts: interrupt for the device.
+- clocks: pointer to the clock for the device.
+- clock-names: should be "MFUCLK".
+
+Optional properties:
+- port-id: should be '0','1' or '2'.
+- phy-addr: MDIO address of the PHY.
+
+Example:
+
+	eth0: ethernet@f7b90000 {
+		compatible = "marvell,pxa168-eth";
+		reg = <0xf7b90000 0x10000>;
+		clocks = <&chip CLKID_GETH0>;
+		clock-names = "MFUCLK";
+		interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
+	};
-- 
1.9.1

Re: [PATCH v2 3/8] Documentation: bindings: net: add the Marvell PXA168 Ethernet controller

[Arnd Bergmann]

On Tuesday 09 September 2014 16:44:03 Antoine Tenart wrote:
> +- clocks: pointer to the clock for the device.
> +- clock-names: should be "MFUCLK".

Clock names are normally not capitalized. Are you able to change
that name when providing a binding or make it an anoymous clock?
What does MFU stand for anyway?

	Arnd

Re: [PATCH v2 3/8] Documentation: bindings: net: add the Marvell PXA168 Ethernet controller

[Antoine Tenart]

Arnd,

On Tue, Sep 09, 2014 at 05:58:12PM +0200, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 16:44:03 Antoine Tenart wrote:
> > +- clocks: pointer to the clock for the device.
> > +- clock-names: should be "MFUCLK".
> 
> Clock names are normally not capitalized. Are you able to change
> that name when providing a binding or make it an anoymous clock?
> What does MFU stand for anyway?

Sure. I could have make it an anonymous clock but the name "MFUCLK" was
already used by the pxa168 Ethernet driver so I didn't wanted to change
that.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] Documentation: bindings: net: add the Marvell PXA168 Ethernet controller

[Arnd Bergmann]

On Tuesday 09 September 2014 18:01:36 Antoine Tenart wrote:
> 
> On Tue, Sep 09, 2014 at 05:58:12PM +0200, Arnd Bergmann wrote:
> > On Tuesday 09 September 2014 16:44:03 Antoine Tenart wrote:
> > > +- clocks: pointer to the clock for the device.
> > > +- clock-names: should be "MFUCLK".
> > 
> > Clock names are normally not capitalized. Are you able to change
> > that name when providing a binding or make it an anoymous clock?
> > What does MFU stand for anyway?
> 
> Sure. I could have make it an anonymous clock but the name "MFUCLK" was
> already used by the pxa168 Ethernet driver so I didn't wanted to change
> that.

I believe you can just ask for an anonymous clock anyway and get the
first one even if it has a name, but I didn't check.

In any case, we should not just take a clock name in a DT binding
because it happened to be used by platform code. It's easy fix the
platform code when someone makes a mistake there, but very hard to
fix DT strings once there are users relying on a particular convention.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] Documentation: bindings: net: add the Marvell PXA168 Ethernet controller

[Mark Rutland]

On Tue, Sep 09, 2014 at 06:47:35PM +0100, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 18:01:36 Antoine Tenart wrote:
> > 
> > On Tue, Sep 09, 2014 at 05:58:12PM +0200, Arnd Bergmann wrote:
> > > On Tuesday 09 September 2014 16:44:03 Antoine Tenart wrote:
> > > > +- clocks: pointer to the clock for the device.
> > > > +- clock-names: should be "MFUCLK".
> > > 
> > > Clock names are normally not capitalized. Are you able to change
> > > that name when providing a binding or make it an anoymous clock?
> > > What does MFU stand for anyway?
> > 
> > Sure. I could have make it an anonymous clock but the name "MFUCLK" was
> > already used by the pxa168 Ethernet driver so I didn't wanted to change
> > that.
> 
> I believe you can just ask for an anonymous clock anyway and get the
> first one even if it has a name, but I didn't check.
> 
> In any case, we should not just take a clock name in a DT binding
> because it happened to be used by platform code. It's easy fix the
> platform code when someone makes a mistake there, but very hard to
> fix DT strings once there are users relying on a particular convention.

At least with names it's easy to add/remove abitrary sets of clocks. We
can be stuck with bad names but I don't think that's as big a concern.

So if there is an actual name for the clock input line (from the pov of
the consumer), I'd recommend using it. Note that this does not have to
be the current "MFUCLK" as used by platform data.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/8] net: pxa168_eth: fix Ethernet flow control status

[Antoine Tenart]

IEEE 802.3x Ethernet flow control is disabled when bit (1 << 2) is set
in the port status register. Fix the flow control detection in the link
event handling function which was relying on the opposite assumption.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 953112f87c5f..10422f2df6cc 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -163,7 +163,7 @@
 /* Bit definitions for Port status */
 #define PORT_SPEED_100		(1 << 0)
 #define FULL_DUPLEX		(1 << 1)
-#define FLOW_CONTROL_ENABLED	(1 << 2)
+#define FLOW_CONTROL_DISABLED	(1 << 2)
 #define LINK_UP			(1 << 3)
 
 /* Bit definitions for work to be done */
@@ -885,7 +885,7 @@ static void handle_link_event(struct pxa168_eth_private *pep)
 		speed = 10;
 
 	duplex = (port_status & FULL_DUPLEX) ? 1 : 0;
-	fc = (port_status & FLOW_CONTROL_ENABLED) ? 1 : 0;
+	fc = (port_status & FLOW_CONTROL_DISABLED) ? 0 : 1;
 	netdev_info(dev, "link up, %d Mb/s, %s duplex, flow control %sabled\n",
 		    speed, duplex ? "full" : "half", fc ? "en" : "dis");
 	if (!netif_carrier_ok(dev))
-- 
1.9.1

Re: [PATCH v2 4/8] net: pxa168_eth: fix Ethernet flow control status

[Andrew Lunn]

On Tue, Sep 09, 2014 at 04:44:04PM +0200, Antoine Tenart wrote:
> IEEE 802.3x Ethernet flow control is disabled when bit (1 << 2) is set
> in the port status register. Fix the flow control detection in the link
> event handling function which was relying on the opposite assumption.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/net/ethernet/marvell/pxa168_eth.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> index 953112f87c5f..10422f2df6cc 100644
> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> @@ -163,7 +163,7 @@
>  /* Bit definitions for Port status */
>  #define PORT_SPEED_100		(1 << 0)
>  #define FULL_DUPLEX		(1 << 1)
> -#define FLOW_CONTROL_ENABLED	(1 << 2)
> +#define FLOW_CONTROL_DISABLED	(1 << 2)
>  #define LINK_UP			(1 << 3)
>  
>  /* Bit definitions for work to be done */
> @@ -885,7 +885,7 @@ static void handle_link_event(struct pxa168_eth_private *pep)
>  		speed = 10;
>  
>  	duplex = (port_status & FULL_DUPLEX) ? 1 : 0;
> -	fc = (port_status & FLOW_CONTROL_ENABLED) ? 1 : 0;
> +	fc = (port_status & FLOW_CONTROL_DISABLED) ? 0 : 1;
>  	netdev_info(dev, "link up, %d Mb/s, %s duplex, flow control %sabled\n",
>  		    speed, duplex ? "full" : "half", fc ? "en" : "dis");
>  	if (!netif_carrier_ok(dev))

Could this be a hardware bug which has been fixed in a newer version
of the IP? It would be good to have this tested on an old platform
using this driver.

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] net: pxa168_eth: fix Ethernet flow control status

[Antoine Tenart]

Andrew,

On Wed, Sep 10, 2014 at 05:39:02PM +0200, Andrew Lunn wrote:
> On Tue, Sep 09, 2014 at 04:44:04PM +0200, Antoine Tenart wrote:
> > IEEE 802.3x Ethernet flow control is disabled when bit (1 << 2) is set
> > in the port status register. Fix the flow control detection in the link
> > event handling function which was relying on the opposite assumption.
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/net/ethernet/marvell/pxa168_eth.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> > index 953112f87c5f..10422f2df6cc 100644
> > --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> > +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> > @@ -163,7 +163,7 @@
> >  /* Bit definitions for Port status */
> >  #define PORT_SPEED_100		(1 << 0)
> >  #define FULL_DUPLEX		(1 << 1)
> > -#define FLOW_CONTROL_ENABLED	(1 << 2)
> > +#define FLOW_CONTROL_DISABLED	(1 << 2)
> >  #define LINK_UP			(1 << 3)
> >  
> >  /* Bit definitions for work to be done */
> > @@ -885,7 +885,7 @@ static void handle_link_event(struct pxa168_eth_private *pep)
> >  		speed = 10;
> >  
> >  	duplex = (port_status & FULL_DUPLEX) ? 1 : 0;
> > -	fc = (port_status & FLOW_CONTROL_ENABLED) ? 1 : 0;
> > +	fc = (port_status & FLOW_CONTROL_DISABLED) ? 0 : 1;
> >  	netdev_info(dev, "link up, %d Mb/s, %s duplex, flow control %sabled\n",
> >  		    speed, duplex ? "full" : "half", fc ? "en" : "dis");
> >  	if (!netif_carrier_ok(dev))
> 
> Could this be a hardware bug which has been fixed in a newer version
> of the IP? It would be good to have this tested on an old platform
> using this driver.

This driver seems to be used by the pxa168 GuruPlug board, and according
to the pxa168 datasheet the flow control status is reported as above (I
checked before sending this patch). I used the software manual
referenced in Documentation/arm/Marvell/README.

This is also the case for the Ethernet controller of the Berlin SoCs.

I could only test on the Berlin BG2Q. I can't be sure this was not a
hardware bug, but at least this is consistent with the pxa168 platform
using the driver. All other registers controlling this flow control
capability also enable on 0.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/8] net: pxa168_eth: get and set the mac address on the Ethernet controller

[Antoine Tenart]

When changing the MAC address, in addition to updating the dev_addr in
the net_device structure, this patch also update the MAC address
registers (high and low) of the Ethernet controller with the new MAC.
The address stored in these registers is used for IEEE 802.3x Ethernet
flow control, which is already enabled.

This patch also tries reading the MAC address stored in these registers
when probing the driver, to use the MAC address set by the bootloader
and avoid using a random one.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 36 +++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 10422f2df6cc..0ecbb3903b71 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -60,6 +60,8 @@
 #define PORT_COMMAND		0x0410
 #define PORT_STATUS		0x0418
 #define HTPR			0x0428
+#define MAC_ADDR_LOW		0x0430
+#define MAC_ADDR_HIGH		0x0438
 #define SDMA_CONFIG		0x0440
 #define SDMA_CMD		0x0448
 #define INT_CAUSE		0x0450
@@ -604,16 +606,42 @@ static void pxa168_eth_set_rx_mode(struct net_device *dev)
 		update_hash_table_mac_address(pep, NULL, ha->addr);
 }
 
+static void pxa168_eth_get_mac_address(struct net_device *dev,
+				       unsigned char *addr)
+{
+	struct pxa168_eth_private *pep = netdev_priv(dev);
+	unsigned int mac_h = rdl(pep, MAC_ADDR_HIGH);
+	unsigned int mac_l = rdl(pep, MAC_ADDR_LOW);
+
+	addr[0] = (mac_h >> 24) & 0xff;
+	addr[1] = (mac_h >> 16) & 0xff;
+	addr[2] = (mac_h >> 8) & 0xff;
+	addr[3] = mac_h & 0xff;
+	addr[4] = (mac_l >> 8) & 0xff;
+	addr[5] = mac_l & 0xff;
+}
+
 static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
 {
 	struct sockaddr *sa = addr;
 	struct pxa168_eth_private *pep = netdev_priv(dev);
 	unsigned char oldMac[ETH_ALEN];
+	u32 mac_h, mac_l;
 
 	if (!is_valid_ether_addr(sa->sa_data))
 		return -EADDRNOTAVAIL;
 	memcpy(oldMac, dev->dev_addr, ETH_ALEN);
 	memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
+
+	mac_h = sa->sa_data[0] << 24;
+	mac_h |= sa->sa_data[1] << 16;
+	mac_h |= sa->sa_data[2] << 8;
+	mac_h |= sa->sa_data[3];
+	mac_l = sa->sa_data[4] << 8;
+	mac_l |= sa->sa_data[5];
+	wrl(pep, MAC_ADDR_HIGH, mac_h);
+	wrl(pep, MAC_ADDR_LOW, mac_l);
+
 	netif_addr_lock_bh(dev);
 	update_hash_table_mac_address(pep, oldMac, dev->dev_addr);
 	netif_addr_unlock_bh(dev);
@@ -1494,8 +1522,12 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 
 	INIT_WORK(&pep->tx_timeout_task, pxa168_eth_tx_timeout_task);
 
-	dev_info(&pdev->dev, "Using random mac address\n");
-	eth_hw_addr_random(dev);
+	/* try reading the mac address, if set by the bootloader */
+	pxa168_eth_get_mac_address(dev, dev->dev_addr);
+	if (!is_valid_ether_addr(dev->dev_addr)) {
+		dev_info(&pdev->dev, "Using random mac address\n");
+		eth_hw_addr_random(dev);
+	}
 
 	pep->rx_ring_size = NUM_RX_DESCS;
 	pep->tx_ring_size = NUM_TX_DESCS;
-- 
1.9.1

Re: [PATCH v2 5/8] net: pxa168_eth: get and set the mac address on the Ethernet controller

[Arnd Bergmann]

On Tuesday 09 September 2014 16:44:05 Antoine Tenart wrote:
> When changing the MAC address, in addition to updating the dev_addr in
> the net_device structure, this patch also update the MAC address
> registers (high and low) of the Ethernet controller with the new MAC.
> The address stored in these registers is used for IEEE 802.3x Ethernet
> flow control, which is already enabled.
> 
> This patch also tries reading the MAC address stored in these registers
> when probing the driver, to use the MAC address set by the bootloader
> and avoid using a random one.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> 

I think it would be good to allow overriding the address using a
'mac-address' property from DT. It's very easy to call
of_get_mac_address().

	Arnd

Re: [PATCH v2 5/8] net: pxa168_eth: get and set the mac address on the Ethernet controller

[Jason Cooper]

On Tue, Sep 09, 2014 at 04:44:05PM +0200, Antoine Tenart wrote:
> When changing the MAC address, in addition to updating the dev_addr in
> the net_device structure, this patch also update the MAC address
> registers (high and low) of the Ethernet controller with the new MAC.
> The address stored in these registers is used for IEEE 802.3x Ethernet
> flow control, which is already enabled.
> 
> This patch also tries reading the MAC address stored in these registers
> when probing the driver, to use the MAC address set by the bootloader
> and avoid using a random one.

Hmm, the wording here seems odd.  I think the preference should be:

 1) bootloader-supplied addr via DT
 2) addr read from device
 3) randomly generated one.

> 
> Signed-off-by: Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/net/ethernet/marvell/pxa168_eth.c | 36 +++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> index 10422f2df6cc..0ecbb3903b71 100644
> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> @@ -60,6 +60,8 @@
>  #define PORT_COMMAND		0x0410
>  #define PORT_STATUS		0x0418
>  #define HTPR			0x0428
> +#define MAC_ADDR_LOW		0x0430
> +#define MAC_ADDR_HIGH		0x0438
>  #define SDMA_CONFIG		0x0440
>  #define SDMA_CMD		0x0448
>  #define INT_CAUSE		0x0450
> @@ -604,16 +606,42 @@ static void pxa168_eth_set_rx_mode(struct net_device *dev)
>  		update_hash_table_mac_address(pep, NULL, ha->addr);
>  }
>  
> +static void pxa168_eth_get_mac_address(struct net_device *dev,
> +				       unsigned char *addr)
> +{
> +	struct pxa168_eth_private *pep = netdev_priv(dev);
> +	unsigned int mac_h = rdl(pep, MAC_ADDR_HIGH);
> +	unsigned int mac_l = rdl(pep, MAC_ADDR_LOW);
> +
> +	addr[0] = (mac_h >> 24) & 0xff;
> +	addr[1] = (mac_h >> 16) & 0xff;
> +	addr[2] = (mac_h >> 8) & 0xff;
> +	addr[3] = mac_h & 0xff;
> +	addr[4] = (mac_l >> 8) & 0xff;
> +	addr[5] = mac_l & 0xff;
> +}
> +
>  static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
>  {
>  	struct sockaddr *sa = addr;
>  	struct pxa168_eth_private *pep = netdev_priv(dev);
>  	unsigned char oldMac[ETH_ALEN];
> +	u32 mac_h, mac_l;
>  
>  	if (!is_valid_ether_addr(sa->sa_data))
>  		return -EADDRNOTAVAIL;
>  	memcpy(oldMac, dev->dev_addr, ETH_ALEN);
>  	memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
> +
> +	mac_h = sa->sa_data[0] << 24;
> +	mac_h |= sa->sa_data[1] << 16;
> +	mac_h |= sa->sa_data[2] << 8;
> +	mac_h |= sa->sa_data[3];
> +	mac_l = sa->sa_data[4] << 8;
> +	mac_l |= sa->sa_data[5];
> +	wrl(pep, MAC_ADDR_HIGH, mac_h);
> +	wrl(pep, MAC_ADDR_LOW, mac_l);
> +
>  	netif_addr_lock_bh(dev);
>  	update_hash_table_mac_address(pep, oldMac, dev->dev_addr);
>  	netif_addr_unlock_bh(dev);
> @@ -1494,8 +1522,12 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>  
>  	INIT_WORK(&pep->tx_timeout_task, pxa168_eth_tx_timeout_task);
>  
> -	dev_info(&pdev->dev, "Using random mac address\n");
> -	eth_hw_addr_random(dev);
> +	/* try reading the mac address, if set by the bootloader */
> +	pxa168_eth_get_mac_address(dev, dev->dev_addr);
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		dev_info(&pdev->dev, "Using random mac address\n");
> +		eth_hw_addr_random(dev);
> +	}

I would do the above iff there was no valid addr supplied by the DT.

thx,

Jason.
>  
>  	pep->rx_ring_size = NUM_RX_DESCS;
>  	pep->tx_ring_size = NUM_TX_DESCS;
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] net: pxa168_eth: get and set the mac address on the Ethernet controller

[Antoine Tenart]

Jason,

On Tue, Sep 09, 2014 at 12:29:58PM -0400, Jason Cooper wrote:
> On Tue, Sep 09, 2014 at 04:44:05PM +0200, Antoine Tenart wrote:
> > When changing the MAC address, in addition to updating the dev_addr in
> > the net_device structure, this patch also update the MAC address
> > registers (high and low) of the Ethernet controller with the new MAC.
> > The address stored in these registers is used for IEEE 802.3x Ethernet
> > flow control, which is already enabled.
> > 
> > This patch also tries reading the MAC address stored in these registers
> > when probing the driver, to use the MAC address set by the bootloader
> > and avoid using a random one.
> 
> Hmm, the wording here seems odd.  I think the preference should be:
> 
>  1) bootloader-supplied addr via DT
>  2) addr read from device
>  3) randomly generated one.

I agree. I'll update that.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 6/8] net: pxa168_eth: allow Berlin SoCs to use the pxa168_eth driver

[Antoine Tenart]

Berlin SoCs have an Ethernet controller compatible with the pxa168.
Allow these SoCs to use the pxa168_eth driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 1b4fc7c639e6..48b9466d1781 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -64,7 +64,7 @@ config MVPP2
 
 config PXA168_ETH
 	tristate "Marvell pxa168 ethernet support"
-	depends on CPU_PXA168
+	depends on CPU_PXA168 || ARCH_BERLIN
 	select PHYLIB
 	---help---
 	  This driver supports the pxa168 Ethernet ports.
-- 
1.9.1

[PATCH v2 7/8] ARM: dts: berlin: add the Ethernet node

[Antoine Tenart]

This patch adds the Ethernet node, enabling the network unit on Berlin
BG2Q SoCs.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index 400c40fceccc..a216cb621ebc 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -114,6 +114,15 @@
 			#interrupt-cells = <3>;
 		};
 
+		eth0: ethernet@b90000 {
+			compatible = "marvell,pxa168-eth";
+			reg = <0xb90000 0x10000>;
+			clocks = <&chip CLKID_GETH0>;
+			clock-names = "MFUCLK";
+			interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
 		cpu-ctrl@dd0000 {
 			compatible = "marvell,berlin-cpu-ctrl";
 			reg = <0xdd0000 0x10000>;
-- 
1.9.1

[PATCH v2 8/8] ARM: dts: berlin: enable the Ethernet port on the BG2Q DMP

[Antoine Tenart]

This patch enables the Ethernet port on the Marvell Berlin2Q DMP board.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q-marvell-dmp.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q-marvell-dmp.dts b/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
index a357ce02a64e..ea1f99b8eed6 100644
--- a/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
+++ b/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
@@ -45,3 +45,7 @@
 &uart0 {
 	status = "okay";
 };
+
+&eth0 {
+	status = "okay";
+};
-- 
1.9.1