[PATCH 0/4] cpsw: support for control module register

[PATCH 0/4] cpsw: support for control module register

[Daniel Mack]

In order to make the cpsw driver work in for example RMII mode with
external clock the driver needs to learn about the control module
register that is at least available on AM33xx SoCs. I'm not sure
whether other chips have similar or compatible bits in such a register,
hence I limited the code paths to that familiy only.

This series is necessary to make the cpsw driver work after resume on
my systems, because that control register is only written from the
bootloader at startup, and its content is altered after resume, so it
needs reprogramming.

The first patch is actually just a cleanup to make the additional
memory resource handling cleaner.

Patches 1-3 are for the net tree and apply to net-next. Patch 4/4 is
for the ARM/dts tree, but can be merged independently.

Thanks,
Daniel

Daniel Mack (4):
  net: ethernet: cpsw: switch to devres allocations
  net: ethernet: cpsw: add optional third memory region for CONTROL
    module
  net: ethernet: cpsw: add support for hardware interface mode config
  ARM: dts: am33xx: add third memory region to cpsw block

 Documentation/devicetree/bindings/net/cpsw.txt |   7 +-
 arch/arm/boot/dts/am33xx.dtsi                  |   3 +-
 drivers/net/ethernet/ti/cpsw.c                 | 218 ++++++++++++++-----------
 include/linux/platform_data/cpsw.h             |   1 +
 4 files changed, 132 insertions(+), 97 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] net: ethernet: cpsw: switch to devres allocations

[Daniel Mack]

This patch cleans up the allocation and error unwind paths, which
allows us to carry less information in struct cpsw_priv and reduce the
amount of jump labels in the probe functions.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/net/ethernet/ti/cpsw.c | 147 +++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 95 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 5d673bf..63feaae 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -367,8 +367,6 @@ struct cpsw_priv {
 	spinlock_t			lock;
 	struct platform_device		*pdev;
 	struct net_device		*ndev;
-	struct resource			*cpsw_res;
-	struct resource			*cpsw_wr_res;
 	struct napi_struct		napi;
 	struct device			*dev;
 	struct cpsw_platform_data	data;
@@ -1711,62 +1709,55 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 
 	if (of_property_read_u32(node, "active_slave", &prop)) {
 		pr_err("Missing active_slave property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->active_slave = prop;
 
 	if (of_property_read_u32(node, "cpts_clock_mult", &prop)) {
 		pr_err("Missing cpts_clock_mult property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->cpts_clock_mult = prop;
 
 	if (of_property_read_u32(node, "cpts_clock_shift", &prop)) {
 		pr_err("Missing cpts_clock_shift property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->cpts_clock_shift = prop;
 
-	data->slave_data = kcalloc(data->slaves, sizeof(struct cpsw_slave_data),
-				   GFP_KERNEL);
+	data->slave_data = devm_kzalloc(&pdev->dev, data->slaves
+					* sizeof(struct cpsw_slave_data),
+					GFP_KERNEL);
 	if (!data->slave_data)
-		return -EINVAL;
+		return -ENOMEM;
 
 	if (of_property_read_u32(node, "cpdma_channels", &prop)) {
 		pr_err("Missing cpdma_channels property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->channels = prop;
 
 	if (of_property_read_u32(node, "ale_entries", &prop)) {
 		pr_err("Missing ale_entries property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->ale_entries = prop;
 
 	if (of_property_read_u32(node, "bd_ram_size", &prop)) {
 		pr_err("Missing bd_ram_size property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->bd_ram_size = prop;
 
 	if (of_property_read_u32(node, "rx_descs", &prop)) {
 		pr_err("Missing rx_descs property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->rx_descs = prop;
 
 	if (of_property_read_u32(node, "mac_control", &prop)) {
 		pr_err("Missing mac_control property in the DT.\n");
-		ret = -EINVAL;
-		goto error_ret;
+		return -EINVAL;
 	}
 	data->mac_control = prop;
 
@@ -1793,8 +1784,7 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 		parp = of_get_property(slave_node, "phy_id", &lenp);
 		if ((parp == NULL) || (lenp != (sizeof(void *) * 2))) {
 			pr_err("Missing slave[%d] phy_id property\n", i);
-			ret = -EINVAL;
-			goto error_ret;
+			return -EINVAL;
 		}
 		mdio_node = of_find_node_by_phandle(be32_to_cpup(parp));
 		phyid = be32_to_cpup(parp+1);
@@ -1824,10 +1814,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 	}
 
 	return 0;
-
-error_ret:
-	kfree(data->slave_data);
-	return ret;
 }
 
 static int cpsw_probe_dual_emac(struct platform_device *pdev,
@@ -1869,7 +1855,6 @@ static int cpsw_probe_dual_emac(struct platform_device *pdev,
 	priv_sl2->coal_intvl = 0;
 	priv_sl2->bus_freq_mhz = priv->bus_freq_mhz;
 
-	priv_sl2->cpsw_res = priv->cpsw_res;
 	priv_sl2->regs = priv->regs;
 	priv_sl2->host_port = priv->host_port;
 	priv_sl2->host_port_regs = priv->host_port_regs;
@@ -1913,8 +1898,8 @@ static int cpsw_probe(struct platform_device *pdev)
 	struct cpsw_priv		*priv;
 	struct cpdma_params		dma_params;
 	struct cpsw_ale_params		ale_params;
-	void __iomem			*ss_regs, *wr_regs;
-	struct resource			*res;
+	void __iomem			*ss_regs;
+	struct resource			*res, *ss_res;
 	u32 slave_offset, sliver_offset, slave_size;
 	int ret = 0, i, k = 0;
 
@@ -1950,7 +1935,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (cpsw_probe_dt(&priv->data, pdev)) {
 		pr_err("cpsw: platform data missing\n");
 		ret = -ENODEV;
-		goto clean_ndev_ret;
+		goto clean_runtime_disable_ret;
 	}
 	data = &priv->data;
 
@@ -1964,11 +1949,12 @@ static int cpsw_probe(struct platform_device *pdev)
 
 	memcpy(ndev->dev_addr, priv->mac_addr, ETH_ALEN);
 
-	priv->slaves = kzalloc(sizeof(struct cpsw_slave) * data->slaves,
-			       GFP_KERNEL);
+	priv->slaves =
+		devm_kzalloc(&pdev->dev, sizeof(struct cpsw_slave) * data->slaves,
+			     GFP_KERNEL);
 	if (!priv->slaves) {
-		ret = -EBUSY;
-		goto clean_ndev_ret;
+		ret = -ENOMEM;
+		goto clean_runtime_disable_ret;
 	}
 	for (i = 0; i < data->slaves; i++)
 		priv->slaves[i].slave_num = i;
@@ -1976,55 +1962,55 @@ static int cpsw_probe(struct platform_device *pdev)
 	priv->slaves[0].ndev = ndev;
 	priv->emac_port = 0;
 
-	priv->clk = clk_get(&pdev->dev, "fck");
+	priv->clk = devm_clk_get(&pdev->dev, "fck");
 	if (IS_ERR(priv->clk)) {
-		dev_err(&pdev->dev, "fck is not found\n");
+		dev_err(priv->dev, "fck is not found\n");
 		ret = -ENODEV;
-		goto clean_slave_ret;
+		goto clean_runtime_disable_ret;
 	}
 	priv->coal_intvl = 0;
 	priv->bus_freq_mhz = clk_get_rate(priv->clk) / 1000000;
 
-	priv->cpsw_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!priv->cpsw_res) {
+	ss_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!ss_res) {
 		dev_err(priv->dev, "error getting i/o resource\n");
 		ret = -ENOENT;
-		goto clean_clk_ret;
+		goto clean_runtime_disable_ret;
 	}
-	if (!request_mem_region(priv->cpsw_res->start,
-				resource_size(priv->cpsw_res), ndev->name)) {
+	if (!devm_request_mem_region(&pdev->dev, ss_res->start,
+				     resource_size(ss_res), ndev->name)) {
 		dev_err(priv->dev, "failed request i/o region\n");
 		ret = -ENXIO;
-		goto clean_clk_ret;
+		goto clean_runtime_disable_ret;
 	}
-	ss_regs = ioremap(priv->cpsw_res->start, resource_size(priv->cpsw_res));
+	ss_regs = devm_ioremap(&pdev->dev, ss_res->start,
+			       resource_size(ss_res));
 	if (!ss_regs) {
 		dev_err(priv->dev, "unable to map i/o region\n");
-		goto clean_cpsw_iores_ret;
+		goto clean_runtime_disable_ret;
 	}
 	priv->regs = ss_regs;
 	priv->version = __raw_readl(&priv->regs->id_ver);
 	priv->host_port = HOST_PORT_NUM;
 
-	priv->cpsw_wr_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!priv->cpsw_wr_res) {
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
 		dev_err(priv->dev, "error getting i/o resource\n");
 		ret = -ENOENT;
-		goto clean_iomap_ret;
+		goto clean_runtime_disable_ret;
 	}
-	if (!request_mem_region(priv->cpsw_wr_res->start,
-			resource_size(priv->cpsw_wr_res), ndev->name)) {
+	if (!devm_request_mem_region(&pdev->dev, res->start,
+				     resource_size(res), ndev->name)) {
 		dev_err(priv->dev, "failed request i/o region\n");
 		ret = -ENXIO;
-		goto clean_iomap_ret;
+		goto clean_runtime_disable_ret;
 	}
-	wr_regs = ioremap(priv->cpsw_wr_res->start,
-				resource_size(priv->cpsw_wr_res));
-	if (!wr_regs) {
+	priv->wr_regs = devm_ioremap(&pdev->dev, res->start,
+				     resource_size(res));
+	if (!priv->wr_regs) {
 		dev_err(priv->dev, "unable to map i/o region\n");
-		goto clean_cpsw_wr_iores_ret;
+		goto clean_runtime_disable_ret;
 	}
-	priv->wr_regs = wr_regs;
 
 	memset(&dma_params, 0, sizeof(dma_params));
 	memset(&ale_params, 0, sizeof(ale_params));
@@ -2055,12 +2041,12 @@ static int cpsw_probe(struct platform_device *pdev)
 		slave_size           = CPSW2_SLAVE_SIZE;
 		sliver_offset        = CPSW2_SLIVER_OFFSET;
 		dma_params.desc_mem_phys =
-			(u32 __force) priv->cpsw_res->start + CPSW2_BD_OFFSET;
+			(u32 __force) ss_res->start + CPSW2_BD_OFFSET;
 		break;
 	default:
 		dev_err(priv->dev, "unknown version 0x%08x\n", priv->version);
 		ret = -ENODEV;
-		goto clean_cpsw_wr_iores_ret;
+		goto clean_runtime_disable_ret;
 	}
 	for (i = 0; i < priv->data.slaves; i++) {
 		struct cpsw_slave *slave = &priv->slaves[i];
@@ -2088,7 +2074,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (!priv->dma) {
 		dev_err(priv->dev, "error initializing dma\n");
 		ret = -ENOMEM;
-		goto clean_wr_iomap_ret;
+		goto clean_runtime_disable_ret;
 	}
 
 	priv->txch = cpdma_chan_create(priv->dma, tx_chan_num(0),
@@ -2123,8 +2109,8 @@ static int cpsw_probe(struct platform_device *pdev)
 
 	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
 		for (i = res->start; i <= res->end; i++) {
-			if (request_irq(i, cpsw_interrupt, 0,
-					dev_name(&pdev->dev), priv)) {
+			if (devm_request_irq(&pdev->dev, i, cpsw_interrupt, 0,
+					     dev_name(priv->dev), priv)) {
 				dev_err(priv->dev, "error attaching irq\n");
 				goto clean_ale_ret;
 			}
@@ -2146,7 +2132,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(priv->dev, "error registering net device\n");
 		ret = -ENODEV;
-		goto clean_irq_ret;
+		goto clean_ale_ret;
 	}
 
 	if (cpts_register(&pdev->dev, priv->cpts,
@@ -2154,44 +2140,27 @@ static int cpsw_probe(struct platform_device *pdev)
 		dev_err(priv->dev, "error registering cpts device\n");
 
 	cpsw_notice(priv, probe, "initialized device (regs %x, irq %d)\n",
-		  priv->cpsw_res->start, ndev->irq);
+		    ss_res->start, ndev->irq);
 
 	if (priv->data.dual_emac) {
 		ret = cpsw_probe_dual_emac(pdev, priv);
 		if (ret) {
 			cpsw_err(priv, probe, "error probe slave 2 emac interface\n");
-			goto clean_irq_ret;
+			goto clean_ale_ret;
 		}
 	}
 
 	return 0;
 
-clean_irq_ret:
-	for (i = 0; i < priv->num_irqs; i++)
-		free_irq(priv->irqs_table[i], priv);
 clean_ale_ret:
 	cpsw_ale_destroy(priv->ale);
 clean_dma_ret:
 	cpdma_chan_destroy(priv->txch);
 	cpdma_chan_destroy(priv->rxch);
 	cpdma_ctlr_destroy(priv->dma);
-clean_wr_iomap_ret:
-	iounmap(priv->wr_regs);
-clean_cpsw_wr_iores_ret:
-	release_mem_region(priv->cpsw_wr_res->start,
-			   resource_size(priv->cpsw_wr_res));
-clean_iomap_ret:
-	iounmap(priv->regs);
-clean_cpsw_iores_ret:
-	release_mem_region(priv->cpsw_res->start,
-			   resource_size(priv->cpsw_res));
-clean_clk_ret:
-	clk_put(priv->clk);
-clean_slave_ret:
+clean_runtime_disable_ret:
 	pm_runtime_disable(&pdev->dev);
-	kfree(priv->slaves);
 clean_ndev_ret:
-	kfree(priv->data.slave_data);
 	free_netdev(priv->ndev);
 	return ret;
 }
@@ -2200,30 +2169,18 @@ static int cpsw_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct cpsw_priv *priv = netdev_priv(ndev);
-	int i;
 
 	if (priv->data.dual_emac)
 		unregister_netdev(cpsw_get_slave_ndev(priv, 1));
 	unregister_netdev(ndev);
 
 	cpts_unregister(priv->cpts);
-	for (i = 0; i < priv->num_irqs; i++)
-		free_irq(priv->irqs_table[i], priv);
 
 	cpsw_ale_destroy(priv->ale);
 	cpdma_chan_destroy(priv->txch);
 	cpdma_chan_destroy(priv->rxch);
 	cpdma_ctlr_destroy(priv->dma);
-	iounmap(priv->regs);
-	release_mem_region(priv->cpsw_res->start,
-			   resource_size(priv->cpsw_res));
-	iounmap(priv->wr_regs);
-	release_mem_region(priv->cpsw_wr_res->start,
-			   resource_size(priv->cpsw_wr_res));
 	pm_runtime_disable(&pdev->dev);
-	clk_put(priv->clk);
-	kfree(priv->slaves);
-	kfree(priv->data.slave_data);
 	if (priv->data.dual_emac)
 		free_netdev(cpsw_get_slave_ndev(priv, 1));
 	free_netdev(ndev);
-- 
1.8.3.1

[PATCH 2/4] net: ethernet: cpsw: add optional third memory region for CONTROL module

[Daniel Mack]

At least the AM33xx SoC has a control module register to configure
details such as the hardware ethernet interface mode.

I'm not sure whether all SoCs which feature the cpsw block have such a
register, so that third memory region is considered optional for now.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt |  5 ++++-
 drivers/net/ethernet/ti/cpsw.c                 | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 05d660e..4e5ca54 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -4,7 +4,10 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
 Required properties:
 - compatible		: Should be "ti,cpsw"
 - reg			: physical base address and size of the cpsw
-			  registers map
+			  registers map.
+			  An optional third memory region can be supplied if
+			  the platform has a control module register to
+			  configure phy interface details
 - interrupts		: property with a value describing the interrupt
 			  number
 - interrupt-parent	: The parent interrupt controller
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 63feaae..4855d8e 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -372,6 +372,7 @@ struct cpsw_priv {
 	struct cpsw_platform_data	data;
 	struct cpsw_ss_regs __iomem	*regs;
 	struct cpsw_wr_regs __iomem	*wr_regs;
+	u32 __iomem			*gmii_sel_reg;
 	u8 __iomem			*hw_stats;
 	struct cpsw_host_regs __iomem	*host_port_regs;
 	u32				msg_enable;
@@ -2012,6 +2013,27 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_runtime_disable_ret;
 	}
 
+	/* If the control memory region is unspecified, continue without it.
+	 * If it is specified, but we're unable to reserve it, bail. */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	if (!res) {
+		dev_err(priv->dev, "error getting control i/o resource\n");
+		goto no_gmii_sel;
+	}
+	if (!devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
+				     ndev->name)) {
+		dev_err(priv->dev, "failed request control i/o region\n");
+		ret = -ENXIO;
+		goto clean_runtime_disable_ret;
+	}
+	priv->gmii_sel_reg = devm_ioremap(&pdev->dev, res->start,
+					  resource_size(res));
+	if (!priv->gmii_sel_reg) {
+		dev_err(priv->dev, "unable to map control i/o region\n");
+		goto clean_runtime_disable_ret;
+	}
+
+no_gmii_sel:
 	memset(&dma_params, 0, sizeof(dma_params));
 	memset(&ale_params, 0, sizeof(ale_params));
 
-- 
1.8.3.1

[PATCH 3/4] net: ethernet: cpsw: add support for hardware interface mode config

[Daniel Mack]

The cpsw currently lacks code to properly set up the hardware interface
mode on AM33xx. Other platforms might be equally affected.

Usually, the bootloader will configure the control module register, so
probably that's why such support wasn't needed in the past. In suspend
mode though, this register is modified, and so it needs reprogramming
after resume.

This patch adds code that makes use of the previously added and optional
support for passing the control mode register, and configures the
correct register bits from _cpsw_adjust_link().

The AM33xx also has a bit for each slave to configure the RMII reference
clock direction. Setting it is now supported by a per-slave DT property.

This code path introducted by this patch is currently exclusive for
am33xx.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt |  2 ++
 drivers/net/ethernet/ti/cpsw.c                 | 49 ++++++++++++++++++++++++++
 include/linux/platform_data/cpsw.h             |  1 +
 3 files changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 4e5ca54..0ccf01f 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -33,6 +33,8 @@ Required properties:
 - phy_id		: Specifies slave phy id
 - phy-mode		: The interface between the SoC and the PHY (a string
 			  that of_get_phy_mode() can understand)
+- ti,rmii-clock-ext	: If present, the driver will configure the RMII
+			  interface to external clock usage
 - mac-address		: Specifies slave MAC address
 
 Optional properties:
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 4855d8e..d18ae43 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -138,6 +138,14 @@ do {								\
 #define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
 #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
 
+#define AM33XX_GMII_SEL_MODE_MII	(0)
+#define AM33XX_GMII_SEL_MODE_RMII	(1)
+#define AM33XX_GMII_SEL_MODE_RGMII	(2)
+#define AM33XX_GMII_SEL_MODE_UNUSED	(3)
+
+#define AM33XX_GMII_SEL_RMII2_IO_CLK_EN	BIT(7)
+#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)
+
 #define cpsw_enable_irq(priv)	\
 	do {			\
 		u32 i;		\
@@ -728,6 +736,44 @@ static void _cpsw_adjust_link(struct cpsw_slave *slave,
 	u32			mac_control = 0;
 	u32			slave_port;
 
+	if (priv->gmii_sel_reg && of_machine_is_compatible("ti,am33xx")) {
+		u32 reg = __raw_readl(priv->gmii_sel_reg);
+		u32 mode = AM33XX_GMII_SEL_MODE_UNUSED;
+		u32 mask;
+
+		if (phy) {
+			switch (phy->interface) {
+			case PHY_INTERFACE_MODE_MII:
+				mode = AM33XX_GMII_SEL_MODE_MII;
+				break;
+			case PHY_INTERFACE_MODE_RMII:
+				mode = AM33XX_GMII_SEL_MODE_RMII;
+				break;
+			case PHY_INTERFACE_MODE_RGMII:
+				mode = AM33XX_GMII_SEL_MODE_RGMII;
+				break;
+			default:
+				break;
+			};
+		}
+
+		mask = 0x3 << (slave->slave_num * 2) |
+		       BIT(slave->slave_num + 6);
+		mode <<= slave->slave_num * 2;
+
+		if (slave->data->rmii_clock_external) {
+			if (slave->slave_num == 0)
+				mode |= AM33XX_GMII_SEL_RMII1_IO_CLK_EN;
+			else
+				mode |= AM33XX_GMII_SEL_RMII2_IO_CLK_EN;
+		}
+
+		reg &= ~mask;
+		reg |= mode;
+
+		__raw_writel(reg, priv->gmii_sel_reg);
+	}
+
 	if (!phy)
 		return;
 
@@ -1798,6 +1844,9 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			memcpy(slave_data->mac_addr, mac_addr, ETH_ALEN);
 
 		slave_data->phy_if = of_get_phy_mode(slave_node);
+		if (of_find_property(slave_node, "ti,rmii-clock-external",
+				     NULL))
+			slave_data->rmii_clock_external = true;
 
 		if (data->dual_emac) {
 			if (of_property_read_u32(slave_node, "dual_emac_res_vlan",
diff --git a/include/linux/platform_data/cpsw.h b/include/linux/platform_data/cpsw.h
index bb3cd58..a29c48b 100644
--- a/include/linux/platform_data/cpsw.h
+++ b/include/linux/platform_data/cpsw.h
@@ -20,6 +20,7 @@
 struct cpsw_slave_data {
 	char		phy_id[MII_BUS_ID_SIZE];
 	int		phy_if;
+	bool		rmii_clock_external;
 	u8		mac_addr[ETH_ALEN];
 	u16		dual_emac_res_vlan;	/* Reserved VLAN for DualEMAC */
 
-- 
1.8.3.1

[PATCH 4/4] ARM: dts: am33xx: add third memory region to cpsw block

[Daniel Mack]

This third memory region just denotes one single register in the CONTROL
module block. The driver uses that in order to set the correct physical
ethernet interface modes.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index a785b95..4ac4e4c 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -459,7 +459,8 @@
 			cpts_clock_mult = <0x80000000>;
 			cpts_clock_shift = <29>;
 			reg = <0x4a100000 0x800
-			       0x4a101200 0x100>;
+			       0x4a101200 0x100
+			       0x44e10650 0x4>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			interrupt-parent = <&intc>;
-- 
1.8.3.1

Re: [PATCH 2/4] net: ethernet: cpsw: add optional third memory region for CONTROL module

[Sergei Shtylyov]

Hello.

On 08/22/2013 03:37 PM, Daniel Mack wrote:

> At least the AM33xx SoC has a control module register to configure
> details such as the hardware ethernet interface mode.

> I'm not sure whether all SoCs which feature the cpsw block have such a
> register, so that third memory region is considered optional for now.

> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>   Documentation/devicetree/bindings/net/cpsw.txt |  5 ++++-
>   drivers/net/ethernet/ti/cpsw.c                 | 22 ++++++++++++++++++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)

> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 05d660e..4e5ca54 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
[...]
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 63feaae..4855d8e 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
[...]
> @@ -2012,6 +2013,27 @@ static int cpsw_probe(struct platform_device *pdev)
>   		goto clean_runtime_disable_ret;
>   	}
>
> +	/* If the control memory region is unspecified, continue without it.
> +	 * If it is specified, but we're unable to reserve it, bail. */

    According to Documentation/CodingStyle, the networking code's preferred 
style of multi-line comments is this:

/* Bla
  * bla
  */

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	if (!res) {
> +		dev_err(priv->dev, "error getting control i/o resource\n");
> +		goto no_gmii_sel;
> +	}
> +	if (!devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
> +				     ndev->name)) {

    Not dev_name(&pdev->dev)?

> +		dev_err(priv->dev, "failed request control i/o region\n");
> +		ret = -ENXIO;

    Rather -EBUSY.

> +		goto clean_runtime_disable_ret;
> +	}
> +	priv->gmii_sel_reg = devm_ioremap(&pdev->dev, res->start,
> +					  resource_size(res));
> +	if (!priv->gmii_sel_reg) {
> +		dev_err(priv->dev, "unable to map control i/o region\n");
> +		goto clean_runtime_disable_ret;
> +	}

    Why not use devm_ioremap_resource() instead of the above sequence?

> +
> +no_gmii_sel:

WBR, Sergei

Re: [PATCH 2/4] net: ethernet: cpsw: add optional third memory region for CONTROL module

[Daniel Mack]

Hi Sergei,

On 22.08.2013 20:12, Sergei Shtylyov wrote:
>> @@ -2012,6 +2013,27 @@ static int cpsw_probe(struct platform_device *pdev)
>>   		goto clean_runtime_disable_ret;
>>   	}
>>
>> +	/* If the control memory region is unspecified, continue without it.
>> +	 * If it is specified, but we're unable to reserve it, bail. */
> 
>     According to Documentation/CodingStyle, the networking code's preferred 
> style of multi-line comments is this:
> 
> /* Bla
>   * bla
>   */

Ok, thanks. Will fix.

>> +		goto clean_runtime_disable_ret;
>> +	}
>> +	priv->gmii_sel_reg = devm_ioremap(&pdev->dev, res->start,
>> +					  resource_size(res));
>> +	if (!priv->gmii_sel_reg) {
>> +		dev_err(priv->dev, "unable to map control i/o region\n");
>> +		goto clean_runtime_disable_ret;
>> +	}
> 
>     Why not use devm_ioremap_resource() instead of the above sequence?

Right, that's much nicer. I'll also do that for the devm rework patch.


Thank you for your review!


Daniel

Re: [PATCH 0/4] cpsw: support for control module register

[David Miller]

Patch #3 doesn't apply to net-next because it makes changes to
a file "include/linux/platform_data/cpsw.h" which doesn't exist.

Re: [PATCH 3/4] net: ethernet: cpsw: add support for hardware interface mode config

[Sekhar Nori]

On Thursday 22 August 2013 05:07 PM, Daniel Mack wrote:
> The cpsw currently lacks code to properly set up the hardware interface
> mode on AM33xx. Other platforms might be equally affected.
> 
> Usually, the bootloader will configure the control module register, so
> probably that's why such support wasn't needed in the past. In suspend
> mode though, this register is modified, and so it needs reprogramming
> after resume.
> 
> This patch adds code that makes use of the previously added and optional
> support for passing the control mode register, and configures the
> correct register bits from _cpsw_adjust_link().
> 
> The AM33xx also has a bit for each slave to configure the RMII reference
> clock direction. Setting it is now supported by a per-slave DT property.
> 
> This code path introducted by this patch is currently exclusive for
> am33xx.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt |  2 ++
>  drivers/net/ethernet/ti/cpsw.c                 | 49 ++++++++++++++++++++++++++
>  include/linux/platform_data/cpsw.h             |  1 +
>  3 files changed, 52 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 4e5ca54..0ccf01f 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -33,6 +33,8 @@ Required properties:
>  - phy_id		: Specifies slave phy id
>  - phy-mode		: The interface between the SoC and the PHY (a string
>  			  that of_get_phy_mode() can understand)
> +- ti,rmii-clock-ext	: If present, the driver will configure the RMII
> +			  interface to external clock usage
>  - mac-address		: Specifies slave MAC address
>  
>  Optional properties:
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 4855d8e..d18ae43 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -138,6 +138,14 @@ do {								\
>  #define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>  
> +#define AM33XX_GMII_SEL_MODE_MII	(0)
> +#define AM33XX_GMII_SEL_MODE_RMII	(1)
> +#define AM33XX_GMII_SEL_MODE_RGMII	(2)
> +#define AM33XX_GMII_SEL_MODE_UNUSED	(3)
> +
> +#define AM33XX_GMII_SEL_RMII2_IO_CLK_EN	BIT(7)
> +#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)
> +
>  #define cpsw_enable_irq(priv)	\
>  	do {			\
>  		u32 i;		\
> @@ -728,6 +736,44 @@ static void _cpsw_adjust_link(struct cpsw_slave *slave,
>  	u32			mac_control = 0;
>  	u32			slave_port;
>  
> +	if (priv->gmii_sel_reg && of_machine_is_compatible("ti,am33xx")) {

This sounds like the DT version of cpu_is_am335x()! Looks like you need
to introduce a new compatible "ti,am3352-cpsw" to indicate the AM3352
specific features of the CPSW IP (yeah, control module is not really
part of the IP, but by introducing it in the driver, we are treating it
such anyway. And you can see this register as extension of IP since its
not shared for any other purpose).

> +		u32 reg = __raw_readl(priv->gmii_sel_reg);

readl() and writel() instead of the __raw_ versions? Even if the
existing driver uses these, lets not have new code use them.

> +		u32 mode = AM33XX_GMII_SEL_MODE_UNUSED;
> +		u32 mask;
> +
> +		if (phy) {
> +			switch (phy->interface) {
> +			case PHY_INTERFACE_MODE_MII:
> +				mode = AM33XX_GMII_SEL_MODE_MII;
> +				break;
> +			case PHY_INTERFACE_MODE_RMII:
> +				mode = AM33XX_GMII_SEL_MODE_RMII;
> +				break;
> +			case PHY_INTERFACE_MODE_RGMII:
> +				mode = AM33XX_GMII_SEL_MODE_RGMII;
> +				break;
> +			default:
> +				break;
> +			};
> +		}
> +
> +		mask = 0x3 << (slave->slave_num * 2) |
> +		       BIT(slave->slave_num + 6);
> +		mode <<= slave->slave_num * 2;
> +
> +		if (slave->data->rmii_clock_external) {
> +			if (slave->slave_num == 0)
> +				mode |= AM33XX_GMII_SEL_RMII1_IO_CLK_EN;
> +			else
> +				mode |= AM33XX_GMII_SEL_RMII2_IO_CLK_EN;
> +		}
> +
> +		reg &= ~mask;
> +		reg |= mode;
> +
> +		__raw_writel(reg, priv->gmii_sel_reg);
> +	}
> +
>  	if (!phy)
>  		return;
>  
> @@ -1798,6 +1844,9 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
>  			memcpy(slave_data->mac_addr, mac_addr, ETH_ALEN);
>  
>  		slave_data->phy_if = of_get_phy_mode(slave_node);
> +		if (of_find_property(slave_node, "ti,rmii-clock-external",

Hey, your documentation says this should be "ti,rmii-clock-ext"

Thanks,
Sekhar

Re: [PATCH 3/4] net: ethernet: cpsw: add support for hardware interface mode config

[Mugunthan V N]

On Thursday 22 August 2013 05:07 PM, Daniel Mack wrote:
> The cpsw currently lacks code to properly set up the hardware interface
> mode on AM33xx. Other platforms might be equally affected.
>
> Usually, the bootloader will configure the control module register, so
> probably that's why such support wasn't needed in the past. In suspend
> mode though, this register is modified, and so it needs reprogramming
> after resume.
>
> This patch adds code that makes use of the previously added and optional
> support for passing the control mode register, and configures the
> correct register bits from _cpsw_adjust_link().
>
> The AM33xx also has a bit for each slave to configure the RMII reference
> clock direction. Setting it is now supported by a per-slave DT property.
>
> This code path introducted by this patch is currently exclusive for
> am33xx.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt |  2 ++
>  drivers/net/ethernet/ti/cpsw.c                 | 49 ++++++++++++++++++++++++++
>  include/linux/platform_data/cpsw.h             |  1 +
>  3 files changed, 52 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 4e5ca54..0ccf01f 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -33,6 +33,8 @@ Required properties:
>  - phy_id		: Specifies slave phy id
>  - phy-mode		: The interface between the SoC and the PHY (a string
>  			  that of_get_phy_mode() can understand)
> +- ti,rmii-clock-ext	: If present, the driver will configure the RMII
> +			  interface to external clock usage
>  - mac-address		: Specifies slave MAC address
>  
>  Optional properties:
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 4855d8e..d18ae43 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -138,6 +138,14 @@ do {								\
>  #define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>  
> +#define AM33XX_GMII_SEL_MODE_MII	(0)
> +#define AM33XX_GMII_SEL_MODE_RMII	(1)
> +#define AM33XX_GMII_SEL_MODE_RGMII	(2)
> +#define AM33XX_GMII_SEL_MODE_UNUSED	(3)
> +
> +#define AM33XX_GMII_SEL_RMII2_IO_CLK_EN	BIT(7)
> +#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)
> +
>  #define cpsw_enable_irq(priv)	\
>  	do {			\
>  		u32 i;		\
> @@ -728,6 +736,44 @@ static void _cpsw_adjust_link(struct cpsw_slave *slave,
>  	u32			mac_control = 0;
>  	u32			slave_port;
>  
> +	if (priv->gmii_sel_reg && of_machine_is_compatible("ti,am33xx")) {
> +		u32 reg = __raw_readl(priv->gmii_sel_reg);
> +		u32 mode = AM33XX_GMII_SEL_MODE_UNUSED;

If phy interface is other than the below handled case then you will be
writing UNUSED bit to gmii sel bits which is wrong as it should not be
used as per hardware design guys. Please refer the following thread in
U-Boot which had a discussion about the UNUSED bit combination usage.
http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/166689

> +		u32 mask;
> +
> +		if (phy) {
> +			switch (phy->interface) {
> +			case PHY_INTERFACE_MODE_MII:
> +				mode = AM33XX_GMII_SEL_MODE_MII;
> +				break;
> +			case PHY_INTERFACE_MODE_RMII:
> +				mode = AM33XX_GMII_SEL_MODE_RMII;
> +				break;
> +			case PHY_INTERFACE_MODE_RGMII:
> +				mode = AM33XX_GMII_SEL_MODE_RGMII;
> +				break;
> +			default:
> +				break;
> +			};
> +		}
> +
> +		mask = 0x3 << (slave->slave_num * 2) |
> +		       BIT(slave->slave_num + 6);
> +		mode <<= slave->slave_num * 2;
> +
> +		if (slave->data->rmii_clock_external) {
> +			if (slave->slave_num == 0)
> +				mode |= AM33XX_GMII_SEL_RMII1_IO_CLK_EN;
> +			else
> +				mode |= AM33XX_GMII_SEL_RMII2_IO_CLK_EN;
> +		}
> +
> +		reg &= ~mask;
> +		reg |= mode;
> +
> +		__raw_writel(reg, priv->gmii_sel_reg);
> +	}
> +

This is not the proper location to add phy interface enable and rmii
external clock enable. This should be moved to open as this api is
called from phy poll work queue at constant interval to detect phy link
changes. The above init has required only once and not to be done regularly.

>  	if (!phy)
>  		return;
>  
> @@ -1798,6 +1844,9 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
>  			memcpy(slave_data->mac_addr, mac_addr, ETH_ALEN);
>  
>  		slave_data->phy_if = of_get_phy_mode(slave_node);
> +		if (of_find_property(slave_node, "ti,rmii-clock-external",
> +				     NULL))
> +			slave_data->rmii_clock_external = true;
>  
>  		if (data->dual_emac) {
>  			if (of_property_read_u32(slave_node, "dual_emac_res_vlan",
> diff --git a/include/linux/platform_data/cpsw.h b/include/linux/platform_data/cpsw.h
> index bb3cd58..a29c48b 100644
> --- a/include/linux/platform_data/cpsw.h
> +++ b/include/linux/platform_data/cpsw.h

In net-next this file is moved to drivers/net/ethernet/ti folder itself
as this driver only supports DT boot.

Regards
Mugunthan V N

Re: [PATCH 3/4] net: ethernet: cpsw: add support for hardware interface mode config

[Mugunthan V N]

On Friday 23 August 2013 11:00 AM, Sekhar Nori wrote:
>> @@ -728,6 +736,44 @@ static void _cpsw_adjust_link(struct cpsw_slave *slave,
>> >  	u32			mac_control = 0;
>> >  	u32			slave_port;
>> >  
>> > +	if (priv->gmii_sel_reg && of_machine_is_compatible("ti,am33xx")) {
> This sounds like the DT version of cpu_is_am335x()! Looks like you need
> to introduce a new compatible "ti,am3352-cpsw" to indicate the AM3352
> specific features of the CPSW IP (yeah, control module is not really
> part of the IP, but by introducing it in the driver, we are treating it
> such anyway. And you can see this register as extension of IP since its
> not shared for any other purpose).
>

I also agree but it should be ti,am3352-cpsw or ti.am33xx-cpsw?

Regards
Mugunthan V N

Re: [PATCH 3/4] net: ethernet: cpsw: add support for hardware interface mode config

[Daniel Mack]

On 23.08.2013 08:14, Mugunthan V N wrote:
> On Friday 23 August 2013 11:00 AM, Sekhar Nori wrote:
>>> @@ -728,6 +736,44 @@ static void _cpsw_adjust_link(struct cpsw_slave *slave,
>>>>  	u32			mac_control = 0;
>>>>  	u32			slave_port;
>>>>  
>>>> +	if (priv->gmii_sel_reg && of_machine_is_compatible("ti,am33xx")) {
>> This sounds like the DT version of cpu_is_am335x()! Looks like you need
>> to introduce a new compatible "ti,am3352-cpsw" to indicate the AM3352
>> specific features of the CPSW IP (yeah, control module is not really
>> part of the IP, but by introducing it in the driver, we are treating it
>> such anyway. And you can see this register as extension of IP since its
>> not shared for any other purpose).
>>
> 
> I also agree but it should be ti,am3352-cpsw or ti.am33xx-cpsw?

I think the rule is to be as specific as possible, so I'll take
ti,am3352-cpsw.

Thank you very much for the review, everyone. I'll respin a new series
with all issues addressed.


Daniel