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

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

[Daniel Mack]

v3 -> v4:
	* use IS_ERR() to check for failed devm_ioremap_resource()
	  calls (reported by Sergei Shtylyov)

v2 -> v3:
	* swap "ti,am3352-cpsw" and "ti,cpsw" to work around a matching
	  bug (reported by Sekhar)

v1 -> v2:
	* combine devm_request_mem_region() and devm_ioremap() and use
	  devm_ioremap_resource() (reported by Sergei Shtylyov)
	* fix multi-line comment style (reported by Sergei Shtylyov)
	* fix ti,rmii-clock-ext property name (reported by Sekhar)
	* rebased to net-next (reported by Mugunthan V N, David Miller)
	* add a new compatible type, and handle AM33xx specific
	  registers that way (reported by Sekhar)
	* move gmii_sel_reg modifications to the open routine
	  (reported by Mugunthan V N)


Daniel Mack (5):
  net: ethernet: cpsw: switch to devres allocations
  net: ethernet: cpsw: add optional third memory region for CONTROL
    module
  net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
  net: ethernet: cpsw: add support for hardware interface mode config
  ARM: dts: am33xx: adopt to cpsw changes

 Documentation/devicetree/bindings/net/cpsw.txt |  10 +-
 arch/arm/boot/dts/am33xx.dtsi                  |   5 +-
 drivers/net/ethernet/ti/cpsw.c                 | 254 ++++++++++++++-----------
 drivers/net/ethernet/ti/cpsw.h                 |   2 +
 4 files changed, 159 insertions(+), 112 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/5] 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, 45 insertions(+), 102 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 79974e3..849af52 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;
@@ -1712,62 +1710,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;
 
@@ -1794,8 +1785,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);
@@ -1825,10 +1815,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,
@@ -1870,7 +1856,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;
@@ -1914,8 +1899,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;
 
@@ -1951,7 +1936,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;
 
@@ -1965,11 +1950,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;
@@ -1977,55 +1963,41 @@ 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)) {
-		dev_err(priv->dev, "failed request i/o region\n");
-		ret = -ENXIO;
-		goto clean_clk_ret;
-	}
-	ss_regs = ioremap(priv->cpsw_res->start, resource_size(priv->cpsw_res));
-	if (!ss_regs) {
+	ss_regs = devm_ioremap_resource(&pdev->dev, ss_res);
+	if (IS_ERR(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;
-	}
-	if (!request_mem_region(priv->cpsw_wr_res->start,
-			resource_size(priv->cpsw_wr_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_resource(&pdev->dev, res);
+	if (IS_ERR(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));
@@ -2056,12 +2028,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];
@@ -2089,7 +2061,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),
@@ -2124,8 +2096,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;
 			}
@@ -2147,7 +2119,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,
@@ -2155,44 +2127,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;
 }
@@ -2201,30 +2156,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

Re: [PATCH v4 1/5] net: ethernet: cpsw: switch to devres allocations

[Sergei Shtylyov]

On 08/23/2013 06:16 PM, Daniel Mack wrote:

> 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, 45 insertions(+), 102 deletions(-)

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 79974e3..849af52 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
[...]
> @@ -1977,55 +1963,41 @@ 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) {

    You don't need to check the result of platform_get_resource() if you call
devm_ioremap_resource() right afterwards -- it will check resource ptr for 
NULL the first thing. :-)

>   		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)) {
> -		dev_err(priv->dev, "failed request i/o region\n");
> -		ret = -ENXIO;
> -		goto clean_clk_ret;
> -	}
> -	ss_regs = ioremap(priv->cpsw_res->start, resource_size(priv->cpsw_res));
> -	if (!ss_regs) {
> +	ss_regs = devm_ioremap_resource(&pdev->dev, ss_res);
> +	if (IS_ERR(ss_regs)) {
>   		dev_err(priv->dev, "unable to map i/o region\n");

    Don't need the error message anymore -- devm_ioremap_resource() will print 
it. And you missed:

		ret = PTR_ERR(ss_regs);

> -		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) {

    Same comment about resource check here...

>   		dev_err(priv->dev, "error getting i/o resource\n");
>   		ret = -ENOENT;
> -		goto clean_iomap_ret;
> -	}
> -	if (!request_mem_region(priv->cpsw_wr_res->start,
> -			resource_size(priv->cpsw_wr_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_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->wr_regs)) {
>   		dev_err(priv->dev, "unable to map i/o region\n");

    And same comments about the unneeded error message and missing 'ret' 
assignment here...

> -		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));
[...]

WBR, Sergei

Re: [PATCH v4 1/5] net: ethernet: cpsw: switch to devres allocations

[Daniel Mack]

On 23.08.2013 20:10, Sergei Shtylyov wrote:
> On 08/23/2013 06:16 PM, Daniel Mack wrote:

>>   	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) {
> 
>     You don't need to check the result of platform_get_resource() if you call
> devm_ioremap_resource() right afterwards -- it will check resource ptr for 
> NULL the first thing. :-)

Thanks! Will address that in v5.


Daniel

[PATCH v4 2/5] 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                 | 16 ++++++++++++++++
 2 files changed, 20 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 849af52..7a25ff4 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;
@@ -1999,6 +2000,21 @@ 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_info(priv->dev, "error getting control i/o resource\n");
+		goto no_gmii_sel;
+	}
+	priv->gmii_sel_reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(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

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

[Sergei Shtylyov]

On 23-08-2013 18:16, 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>
[...]

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 849af52..7a25ff4 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
[...]
> @@ -1999,6 +2000,21 @@ 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_info(priv->dev, "error getting control i/o resource\n");
> +		goto no_gmii_sel;
> +	}
> +	priv->gmii_sel_reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->gmii_sel_reg)) {
> +		dev_err(priv->dev, "unable to map control i/o region\n");

    You didn't actually seem to heed my words about error message. And don't 
you want to do:

	res = PTR_ERR(priv->gmii_sel_reg);

> +		goto clean_runtime_disable_ret;
> +	}
> +
> +no_gmii_sel:
>   	memset(&dma_params, 0, sizeof(dma_params));
>   	memset(&ale_params, 0, sizeof(ale_params));

WBR, Sergei

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

[Daniel Mack]

On 23.08.2013 16:59, Sergei Shtylyov wrote:
> On 23-08-2013 18:16, Daniel Mack wrote:

>> +	priv->gmii_sel_reg = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(priv->gmii_sel_reg)) {
>> +		dev_err(priv->dev, "unable to map control i/o region\n");
> 
>     You didn't actually seem to heed my words about error message.

Well yes I did, but only in the check for platform_get_resource(). As
the comment says - we pass on if that memory region is not given, but if
it is given, it also has to be valid.

> And don't  you want to do:
> 
> 	res = PTR_ERR(priv->gmii_sel_reg);


Erm, of course. Sorry for that.


Daniel

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

[Sergei Shtylyov]

On 08/23/2013 08:21 PM, Daniel Mack wrote:

>>> +	priv->gmii_sel_reg = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(priv->gmii_sel_reg)) {
>>> +		dev_err(priv->dev, "unable to map control i/o region\n");

>>      You didn't actually seem to heed my words about error message.

> Well yes I did, but only in the check for platform_get_resource(). As
> the comment says - we pass on if that memory region is not given, but if
> it is given, it also has to be valid.

    Yes, but what I told you was devm_ioremap_resource() prints the error 
messages itself, so that you don't have to. And you even consented with that. :-)

>> And don't  you want to do:

>> 	res = PTR_ERR(priv->gmii_sel_reg);

     Well, I've messed with the variable name: 'res' is struct resource *', 
what I meant was *int* variable.

> Erm, of course. Sorry for that.

> Daniel

WBR, Sergei

[PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Daniel Mack]

In order to support features that are specific to the AM335x IP, we have
to add hardware types and another compatible string.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
 drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 4e5ca54..b717458 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
 ------------------------------------------------------
 
 Required properties:
-- compatible		: Should be "ti,cpsw"
+- compatible		: Should be "ti,cpsw" for generic cpsw support, or
+			  "ti,am3352-cpsw" for AM3352 SoCs
 - reg			: physical base address and size of the cpsw
 			  registers map.
 			  An optional third memory region can be supplied if
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 7a25ff4..73c44cb6 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -155,6 +155,11 @@ do {								\
 		((priv->data.dual_emac) ? priv->emac_port :	\
 		priv->data.active_slave)
 
+enum {
+	CPSW_TYPE_GENERIC,
+	CPSW_TYPE_AM33XX
+};
+
 static int debug_level;
 module_param(debug_level, int, 0);
 MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
@@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
 	slave->port_vlan = data->dual_emac_res_vlan;
 }
 
+static const struct of_device_id cpsw_of_mtable[] = {
+	{
+		.compatible	= "ti,am3352-cpsw",
+		.data		= (void *) CPSW_TYPE_AM33XX
+	}, {
+		.compatible	= "ti,cpsw",
+		.data		= (void *) CPSW_TYPE_GENERIC
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, cpsw_of_mtable);
+
 static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			 struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct device_node *slave_node;
+	unsigned long match_data;
 	int i = 0, ret;
 	u32 prop;
 
-	if (!node)
+	match = of_match_device(cpsw_of_mtable, &pdev->dev);
+
+	if (!node || !match)
 		return -EINVAL;
 
+	match_data = (unsigned long) match->data;
+	data->hw_type = match_data;
+
 	if (of_property_read_u32(node, "slaves", &prop)) {
 		pr_err("Missing slaves property in the DT.\n");
 		return -EINVAL;
@@ -2228,12 +2252,6 @@ static const struct dev_pm_ops cpsw_pm_ops = {
 	.resume		= cpsw_resume,
 };
 
-static const struct of_device_id cpsw_of_mtable[] = {
-	{ .compatible = "ti,cpsw", },
-	{ /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, cpsw_of_mtable);
-
 static struct platform_driver cpsw_driver = {
 	.driver = {
 		.name	 = "cpsw",
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index eb3e101..96c374a 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -37,6 +37,7 @@ struct cpsw_platform_data {
 	u32	mac_control;	/* Mac control register */
 	u16	default_vlan;	/* Def VLAN for ALE lookup in VLAN aware mode*/
 	bool	dual_emac;	/* Enable Dual EMAC mode */
+	u32	hw_type;	/* hardware type as specified in 'compatible' */
 };
 
 #endif /* __CPSW_H__ */
-- 
1.8.3.1

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
> In order to support features that are specific to the AM335x IP, we have
> to add hardware types and another compatible string.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 4e5ca54..b717458 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>  ------------------------------------------------------
>  
>  Required properties:
> -- compatible		: Should be "ti,cpsw"
> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
> +			  "ti,am3352-cpsw" for AM3352 SoCs
>  - reg			: physical base address and size of the cpsw
>  			  registers map.
>  			  An optional third memory region can be supplied if
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 7a25ff4..73c44cb6 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -155,6 +155,11 @@ do {								\
>  		((priv->data.dual_emac) ? priv->emac_port :	\
>  		priv->data.active_slave)
>  
> +enum {
> +	CPSW_TYPE_GENERIC,
> +	CPSW_TYPE_AM33XX
> +};
> +
>  static int debug_level;
>  module_param(debug_level, int, 0);
>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>  	slave->port_vlan = data->dual_emac_res_vlan;
>  }
>  
> +static const struct of_device_id cpsw_of_mtable[] = {
> +	{
> +		.compatible	= "ti,am3352-cpsw",
I didn't notice this earlier, but can't you use the IP version
as a compatible instead of using a SOC name. Whats really SOC specific
on this IP ? Sorry i have missed any earlier discussion on this but
this approach doesn't seem good. Its like adding SOC checks in the
driver subsystem.

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Benoit Cousson]

Hi Santosh,

[...]

>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.

Hehe, welcome to the club!
I keep arguing about that as well :-)

I think we should create a Facebook group: 
"against-the-soc-version-in-the-compatible-string".

Regards,
Benoit

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 11:22 AM, Benoit Cousson wrote:
> Hi Santosh,
> 
> [...]
> 
>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>> +    {
>>> +        .compatible    = "ti,am3352-cpsw",
>> I didn't notice this earlier, but can't you use the IP version
>> as a compatible instead of using a SOC name. Whats really SOC specific
>> on this IP ? Sorry i have missed any earlier discussion on this but
>> this approach doesn't seem good. Its like adding SOC checks in the
>> driver subsystem.
> 
> Hehe, welcome to the club!
> I keep arguing about that as well :-)
> 
> I think we should create a Facebook group: "against-the-soc-version-in-the-compatible-string".
> 
Indeed !!

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Daniel Mack]

On 23.08.2013 16:23, Santosh Shilimkar wrote:
> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:

>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
>
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.

As I already mentioned in the cover letter and in the commit message, I
just don't know which criteria makes most sense here.

On a general note, I would say that chances that this exactly IP core
with the same version number will appear on some other silicon which
doesn't support the control mode register in an AM33xx fashion, is not
necessarily negligible.

So what that new compatible string denotes is the cpsw in a version as
found on am3352 SoCs, which is actually exactly what it does.

I don't have a strong opinion here, but see your point. I just don't
have a better idea on how to treat that.


Daniel

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
> 
>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>> +	{
>>> +		.compatible	= "ti,am3352-cpsw",
>>
>> I didn't notice this earlier, but can't you use the IP version
>> as a compatible instead of using a SOC name. Whats really SOC specific
>> on this IP ? Sorry i have missed any earlier discussion on this but
>> this approach doesn't seem good. Its like adding SOC checks in the
>> driver subsystem.
> 
> As I already mentioned in the cover letter and in the commit message, I
> just don't know which criteria makes most sense here.
> 
> On a general note, I would say that chances that this exactly IP core
> with the same version number will appear on some other silicon which
> doesn't support the control mode register in an AM33xx fashion, is not
> necessarily negligible.
> 
> So what that new compatible string denotes is the cpsw in a version as
> found on am3352 SoCs, which is actually exactly what it does.
> 
> I don't have a strong opinion here, but see your point. I just don't
> have a better idea on how to treat that.
> 
So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
That way if in future if someone uses those features, they can use
this compatible if they don't they use the one which suites that
SOC.

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Sekhar Nori]

On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
>> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>
>>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>>> +	{
>>>> +		.compatible	= "ti,am3352-cpsw",
>>>
>>> I didn't notice this earlier, but can't you use the IP version
>>> as a compatible instead of using a SOC name. Whats really SOC specific
>>> on this IP ? Sorry i have missed any earlier discussion on this but
>>> this approach doesn't seem good. Its like adding SOC checks in the
>>> driver subsystem.
>>
>> As I already mentioned in the cover letter and in the commit message, I
>> just don't know which criteria makes most sense here.
>>
>> On a general note, I would say that chances that this exactly IP core
>> with the same version number will appear on some other silicon which
>> doesn't support the control mode register in an AM33xx fashion, is not
>> necessarily negligible.
>>
>> So what that new compatible string denotes is the cpsw in a version as
>> found on am3352 SoCs, which is actually exactly what it does.
>>
>> I don't have a strong opinion here, but see your point. I just don't
>> have a better idea on how to treat that.
>>
> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.

If this could be handled using IP version then the right way would be to
just read the IP version from hardware and use it. No need of DT property.

Thanks,
Sekhar

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Daniel Mack]

On 23.08.2013 19:09, Sekhar Nori wrote:
> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:

>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> 
> If this could be handled using IP version then the right way would be to
> just read the IP version from hardware and use it. No need of DT property.

Yes, exactly. The information we need to support the phy interface
setting is completely disconnected from the cpsw core itself.


Daniel

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
> 
> 
> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
>>> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>>
>>>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>>>> +	{
>>>>> +		.compatible	= "ti,am3352-cpsw",
>>>>
>>>> I didn't notice this earlier, but can't you use the IP version
>>>> as a compatible instead of using a SOC name. Whats really SOC specific
>>>> on this IP ? Sorry i have missed any earlier discussion on this but
>>>> this approach doesn't seem good. Its like adding SOC checks in the
>>>> driver subsystem.
>>>
>>> As I already mentioned in the cover letter and in the commit message, I
>>> just don't know which criteria makes most sense here.
>>>
>>> On a general note, I would say that chances that this exactly IP core
>>> with the same version number will appear on some other silicon which
>>> doesn't support the control mode register in an AM33xx fashion, is not
>>> necessarily negligible.
>>>
>>> So what that new compatible string denotes is the cpsw in a version as
>>> found on am3352 SoCs, which is actually exactly what it does.
>>>
>>> I don't have a strong opinion here, but see your point. I just don't
>>> have a better idea on how to treat that.
>>>
>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> 
> If this could be handled using IP version then the right way would be to
> just read the IP version from hardware and use it. No need of DT property.
> 
Thats fine as well but I thought the patch needed additional properties like
CM reg-address come from DT and hence the separate compatible. If you can
manage without that, thats even better.

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Daniel Mack]

On 23.08.2013 19:19, Santosh Shilimkar wrote:
> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:

>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>
>> If this could be handled using IP version then the right way would be to
>> just read the IP version from hardware and use it. No need of DT property.
>>
> Thats fine as well but I thought the patch needed additional properties like
> CM reg-address come from DT and hence the separate compatible. If you can
> manage without that, thats even better.

We can't, that's the whole point :)

Well, theoretically, we could for now, but that's not a clean solution.
Again: the problem here is that the control port is separated from the
cpsw core, and so we have to implement something specific for the AM3352
SoC. I know that's a violation of clean and generic driver ideas, but
there's no way we can assume that every cpsw v2 ip block has a control
port that is compatible to the one found on am335x chips.


Daniel

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
> 
>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>
>>> If this could be handled using IP version then the right way would be to
>>> just read the IP version from hardware and use it. No need of DT property.
>>>
>> Thats fine as well but I thought the patch needed additional properties like
>> CM reg-address come from DT and hence the separate compatible. If you can
>> manage without that, thats even better.
> 
> We can't, that's the whole point :)
> 
I saw that from the patch :)

> Well, theoretically, we could for now, but that's not a clean solution.
> Again: the problem here is that the control port is separated from the
> cpsw core, and so we have to implement something specific for the AM3352
> SoC. I know that's a violation of clean and generic driver ideas, but
> there's no way we can assume that every cpsw v2 ip block has a control
> port that is compatible to the one found on am335x chips.
> 
But there is a possibility that other SOC will just use the same
control module approach. So using a revision IP is just fine. BTW,
CPSW is not the only driver where control module is used. There are
many examples like USB, MMC etc

There is nothing wrong in using the version info and associating
the additional functionality with it.

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Sekhar Nori]

On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>
>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>
>>>> If this could be handled using IP version then the right way would be to
>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>
>>> Thats fine as well but I thought the patch needed additional properties like
>>> CM reg-address come from DT and hence the separate compatible. If you can
>>> manage without that, thats even better.
>>
>> We can't, that's the whole point :)
>>
> I saw that from the patch :)
> 
>> Well, theoretically, we could for now, but that's not a clean solution.
>> Again: the problem here is that the control port is separated from the
>> cpsw core, and so we have to implement something specific for the AM3352
>> SoC. I know that's a violation of clean and generic driver ideas, but
>> there's no way we can assume that every cpsw v2 ip block has a control
>> port that is compatible to the one found on am335x chips.
>>
> But there is a possibility that other SOC will just use the same
> control module approach. So using a revision IP is just fine. BTW,

But this is misleading because it makes appear like the same compatible
can be used on on another SoC like DRA7 which probably has the same
version of IP but a different control module implementation, when in
practice it cannot.

The fact is we are doing something SoC specific in the driver and we
cannot hide that behind IP versions. If really in practice there comes
another SoC with the same control module definition then it can always
use ti,am3352-cpsw compatible as well. The compatible name does not
preclude its usage.

Thanks,
Sekhar

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote:
> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>>
>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>>
>>>>> If this could be handled using IP version then the right way would be to
>>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>>
>>>> Thats fine as well but I thought the patch needed additional properties like
>>>> CM reg-address come from DT and hence the separate compatible. If you can
>>>> manage without that, thats even better.
>>>
>>> We can't, that's the whole point :)
>>>
>> I saw that from the patch :)
>>
>>> Well, theoretically, we could for now, but that's not a clean solution.
>>> Again: the problem here is that the control port is separated from the
>>> cpsw core, and so we have to implement something specific for the AM3352
>>> SoC. I know that's a violation of clean and generic driver ideas, but
>>> there's no way we can assume that every cpsw v2 ip block has a control
>>> port that is compatible to the one found on am335x chips.
>>>
>> But there is a possibility that other SOC will just use the same
>> control module approach. So using a revision IP is just fine. BTW,
> 
> But this is misleading because it makes appear like the same compatible
> can be used on on another SoC like DRA7 which probably has the same
> version of IP but a different control module implementation, when in
> practice it cannot.
> 
> The fact is we are doing something SoC specific in the driver and we
> cannot hide that behind IP versions. If really in practice there comes
> another SoC with the same control module definition then it can always
> use ti,am3352-cpsw compatible as well. The compatible name does not
> preclude its usage.
> 
My point was the CPSW needs a feature which is implemented using
control module rather than within the IP itself. Its an implementation
detail. As such the additional feature makes sense for that IP. O.w
there was no need to do any monkeying with control module.

E.g
MMC card detect is a basic functionality, implemented by various types
like control module, PMIC or MMC IP itself. As such the driver need
that support and all the implementation details needs to still handled
to make that part work.

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Mugunthan V N]

On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote:
>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>>> If this could be handled using IP version then the right way would be to
>>>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>>>
>>>>> Thats fine as well but I thought the patch needed additional properties like
>>>>> CM reg-address come from DT and hence the separate compatible. If you can
>>>>> manage without that, thats even better.
>>>> We can't, that's the whole point :)
>>>>
>>> I saw that from the patch :)
>>>
>>>> Well, theoretically, we could for now, but that's not a clean solution.
>>>> Again: the problem here is that the control port is separated from the
>>>> cpsw core, and so we have to implement something specific for the AM3352
>>>> SoC. I know that's a violation of clean and generic driver ideas, but
>>>> there's no way we can assume that every cpsw v2 ip block has a control
>>>> port that is compatible to the one found on am335x chips.
>>>>
>>> But there is a possibility that other SOC will just use the same
>>> control module approach. So using a revision IP is just fine. BTW,
>> But this is misleading because it makes appear like the same compatible
>> can be used on on another SoC like DRA7 which probably has the same
>> version of IP but a different control module implementation, when in
>> practice it cannot.
>>
>> The fact is we are doing something SoC specific in the driver and we
>> cannot hide that behind IP versions. If really in practice there comes
>> another SoC with the same control module definition then it can always
>> use ti,am3352-cpsw compatible as well. The compatible name does not
>> preclude its usage.
>>
> My point was the CPSW needs a feature which is implemented using
> control module rather than within the IP itself. Its an implementation
> detail. As such the additional feature makes sense for that IP. O.w
> there was no need to do any monkeying with control module.
>
> E.g
> MMC card detect is a basic functionality, implemented by various types
> like control module, PMIC or MMC IP itself. As such the driver need
> that support and all the implementation details needs to still handled
> to make that part work.
>
>

CPSW core as such understands only GMII/MII signals, there is an
additional module which converts GMII/MII signals to RGMII/RMII signals
respectively which is called as CPRGMII/CPRMII as specified in the
AM335x TRM in Figure 14-1. Ethernet Switch Integration.

So to control this sub-module, the control register is used and this has
to be configured according to the EVM design like what mode of phy is
connected. CPRGMII and CPRMII is no way related to CPSW core.

Regards
Mugunthan V N

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 02:29 PM, Mugunthan V N wrote:
> On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote:
>>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
>>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>>>> If this could be handled using IP version then the right way would be to
>>>>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>>>>
>>>>>> Thats fine as well but I thought the patch needed additional properties like
>>>>>> CM reg-address come from DT and hence the separate compatible. If you can
>>>>>> manage without that, thats even better.
>>>>> We can't, that's the whole point :)
>>>>>
>>>> I saw that from the patch :)
>>>>
>>>>> Well, theoretically, we could for now, but that's not a clean solution.
>>>>> Again: the problem here is that the control port is separated from the
>>>>> cpsw core, and so we have to implement something specific for the AM3352
>>>>> SoC. I know that's a violation of clean and generic driver ideas, but
>>>>> there's no way we can assume that every cpsw v2 ip block has a control
>>>>> port that is compatible to the one found on am335x chips.
>>>>>
>>>> But there is a possibility that other SOC will just use the same
>>>> control module approach. So using a revision IP is just fine. BTW,
>>> But this is misleading because it makes appear like the same compatible
>>> can be used on on another SoC like DRA7 which probably has the same
>>> version of IP but a different control module implementation, when in
>>> practice it cannot.
>>>
>>> The fact is we are doing something SoC specific in the driver and we
>>> cannot hide that behind IP versions. If really in practice there comes
>>> another SoC with the same control module definition then it can always
>>> use ti,am3352-cpsw compatible as well. The compatible name does not
>>> preclude its usage.
>>>
>> My point was the CPSW needs a feature which is implemented using
>> control module rather than within the IP itself. Its an implementation
>> detail. As such the additional feature makes sense for that IP. O.w
>> there was no need to do any monkeying with control module.
>>
>> E.g
>> MMC card detect is a basic functionality, implemented by various types
>> like control module, PMIC or MMC IP itself. As such the driver need
>> that support and all the implementation details needs to still handled
>> to make that part work.
>>
>>
> 
> CPSW core as such understands only GMII/MII signals, there is an
> additional module which converts GMII/MII signals to RGMII/RMII signals
> respectively which is called as CPRGMII/CPRMII as specified in the
> AM335x TRM in Figure 14-1. Ethernet Switch Integration.
> 
> So to control this sub-module, the control register is used and this has
> to be configured according to the EVM design like what mode of phy is
> connected. CPRGMII and CPRMII is no way related to CPSW core.
> 
Ok then why are you polluting cpsw driver with that code which
not realted to CPSW as you said above. You are contradicting what
you said by supporting the SOC usage in the core CPSW driver.

Regards,
Santosh

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Mugunthan V N]

On Saturday 24 August 2013 01:24 AM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 02:29 PM, Mugunthan V N wrote:
>> On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote:
>>>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
>>>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>>>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>>>>> If this could be handled using IP version then the right way would be to
>>>>>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>>>>>
>>>>>>> Thats fine as well but I thought the patch needed additional properties like
>>>>>>> CM reg-address come from DT and hence the separate compatible. If you can
>>>>>>> manage without that, thats even better.
>>>>>> We can't, that's the whole point :)
>>>>>>
>>>>> I saw that from the patch :)
>>>>>
>>>>>> Well, theoretically, we could for now, but that's not a clean solution.
>>>>>> Again: the problem here is that the control port is separated from the
>>>>>> cpsw core, and so we have to implement something specific for the AM3352
>>>>>> SoC. I know that's a violation of clean and generic driver ideas, but
>>>>>> there's no way we can assume that every cpsw v2 ip block has a control
>>>>>> port that is compatible to the one found on am335x chips.
>>>>>>
>>>>> But there is a possibility that other SOC will just use the same
>>>>> control module approach. So using a revision IP is just fine. BTW,
>>>> But this is misleading because it makes appear like the same compatible
>>>> can be used on on another SoC like DRA7 which probably has the same
>>>> version of IP but a different control module implementation, when in
>>>> practice it cannot.
>>>>
>>>> The fact is we are doing something SoC specific in the driver and we
>>>> cannot hide that behind IP versions. If really in practice there comes
>>>> another SoC with the same control module definition then it can always
>>>> use ti,am3352-cpsw compatible as well. The compatible name does not
>>>> preclude its usage.
>>>>
>>> My point was the CPSW needs a feature which is implemented using
>>> control module rather than within the IP itself. Its an implementation
>>> detail. As such the additional feature makes sense for that IP. O.w
>>> there was no need to do any monkeying with control module.
>>>
>>> E.g
>>> MMC card detect is a basic functionality, implemented by various types
>>> like control module, PMIC or MMC IP itself. As such the driver need
>>> that support and all the implementation details needs to still handled
>>> to make that part work.
>>>
>>>
>> CPSW core as such understands only GMII/MII signals, there is an
>> additional module which converts GMII/MII signals to RGMII/RMII signals
>> respectively which is called as CPRGMII/CPRMII as specified in the
>> AM335x TRM in Figure 14-1. Ethernet Switch Integration.
>>
>> So to control this sub-module, the control register is used and this has
>> to be configured according to the EVM design like what mode of phy is
>> connected. CPRGMII and CPRMII is no way related to CPSW core.
>>
> Ok then why are you polluting cpsw driver with that code which
> not realted to CPSW as you said above. You are contradicting what
> you said by supporting the SOC usage in the core CPSW driver.
This patch series is not from me and because of the reason I mentioned
about control module driver, so that cpsw driver can make use control
module apis to select phy mode and control module driver takes care of
SoC specific register offsets and definitions, but now it is not
possible as it is not acceptable in mainline. So other way is to keep
these in driver itself as it is done in this patch series with SoC
compatibilities.

Regards
Mugunthan V N

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Sekhar Nori]

On Monday 26 August 2013 11:29 AM, Mugunthan V N wrote:
> On Saturday 24 August 2013 01:24 AM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 02:29 PM, Mugunthan V N wrote:
>>> On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote:
>>>> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote:
>>>>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
>>>>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>>>>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>>>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>>>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>>>>>> If this could be handled using IP version then the right way would be to
>>>>>>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>>>>>>
>>>>>>>> Thats fine as well but I thought the patch needed additional properties like
>>>>>>>> CM reg-address come from DT and hence the separate compatible. If you can
>>>>>>>> manage without that, thats even better.
>>>>>>> We can't, that's the whole point :)
>>>>>>>
>>>>>> I saw that from the patch :)
>>>>>>
>>>>>>> Well, theoretically, we could for now, but that's not a clean solution.
>>>>>>> Again: the problem here is that the control port is separated from the
>>>>>>> cpsw core, and so we have to implement something specific for the AM3352
>>>>>>> SoC. I know that's a violation of clean and generic driver ideas, but
>>>>>>> there's no way we can assume that every cpsw v2 ip block has a control
>>>>>>> port that is compatible to the one found on am335x chips.
>>>>>>>
>>>>>> But there is a possibility that other SOC will just use the same
>>>>>> control module approach. So using a revision IP is just fine. BTW,
>>>>> But this is misleading because it makes appear like the same compatible
>>>>> can be used on on another SoC like DRA7 which probably has the same
>>>>> version of IP but a different control module implementation, when in
>>>>> practice it cannot.
>>>>>
>>>>> The fact is we are doing something SoC specific in the driver and we
>>>>> cannot hide that behind IP versions. If really in practice there comes
>>>>> another SoC with the same control module definition then it can always
>>>>> use ti,am3352-cpsw compatible as well. The compatible name does not
>>>>> preclude its usage.
>>>>>
>>>> My point was the CPSW needs a feature which is implemented using
>>>> control module rather than within the IP itself. Its an implementation
>>>> detail. As such the additional feature makes sense for that IP. O.w
>>>> there was no need to do any monkeying with control module.
>>>>
>>>> E.g
>>>> MMC card detect is a basic functionality, implemented by various types
>>>> like control module, PMIC or MMC IP itself. As such the driver need
>>>> that support and all the implementation details needs to still handled
>>>> to make that part work.
>>>>
>>>>
>>> CPSW core as such understands only GMII/MII signals, there is an
>>> additional module which converts GMII/MII signals to RGMII/RMII signals
>>> respectively which is called as CPRGMII/CPRMII as specified in the
>>> AM335x TRM in Figure 14-1. Ethernet Switch Integration.
>>>
>>> So to control this sub-module, the control register is used and this has
>>> to be configured according to the EVM design like what mode of phy is
>>> connected. CPRGMII and CPRMII is no way related to CPSW core.
>>>
>> Ok then why are you polluting cpsw driver with that code which
>> not realted to CPSW as you said above. You are contradicting what
>> you said by supporting the SOC usage in the core CPSW driver.
> This patch series is not from me and because of the reason I mentioned
> about control module driver, so that cpsw driver can make use control
> module apis to select phy mode and control module driver takes care of
> SoC specific register offsets and definitions, but now it is not
> possible as it is not acceptable in mainline. So other way is to keep
> these in driver itself as it is done in this patch series with SoC
> compatibilities.

What is done in this patch is _not_ "SoC compatibilities". SoC
compatibility would be what was done in v1 of this patch ie, explicit
check for

	of_machine_is_compatible("ti,am33xx")

"ti,am3352-cpsw" says "CPSW as implemented on AM3352". This is not the
same as checking if SoC is AM3352.

The example quoted on ePAPR spec for a compatible string is:

	compatible = “fsl,mpc8641-uart”, “ns16550";

MPC8641 is freescale PowerPC based SoC[1]. This shows that it is not
unnatural to use SoC names in compatibles for IPs. That, or the ePAPR
specification needs to be updated to show the right example of how a
compatible could be defined. Until then I see no reason of changing what
is implemented in this patch.

In short, even if there was no control module handling in the driver,
using "ti,am3352-cpsw" would be just fine.

I have also CCed the DT maintainers for their opinion. They should have
been explicitly CCed anyway.

Thanks,
Sekhar

[1] http://www.freescale.com/files/32bit/doc/data_sheet/MPC8641DEC.pdf

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Mugunthan V N]

On Friday 23 August 2013 10:26 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
>> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>>> +	{
>>>> +		.compatible	= "ti,am3352-cpsw",
>>> I didn't notice this earlier, but can't you use the IP version
>>> as a compatible instead of using a SOC name. Whats really SOC specific
>>> on this IP ? Sorry i have missed any earlier discussion on this but
>>> this approach doesn't seem good. Its like adding SOC checks in the
>>> driver subsystem.
>> As I already mentioned in the cover letter and in the commit message, I
>> just don't know which criteria makes most sense here.
>>
>> On a general note, I would say that chances that this exactly IP core
>> with the same version number will appear on some other silicon which
>> doesn't support the control mode register in an AM33xx fashion, is not
>> necessarily negligible.
>>
>> So what that new compatible string denotes is the cpsw in a version as
>> found on am3352 SoCs, which is actually exactly what it does.
>>
>> I don't have a strong opinion here, but see your point. I just don't
>> have a better idea on how to treat that.
>>
> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> That way if in future if someone uses those features, they can use
> this compatible if they don't they use the one which suites that
> SOC.
>
We cannot map control module register with CPSW IP version as both comes
from different design team and CPSW ip version can be same across SoC
and gmii sel register definition can be different. Control module
defines may vary in different SoC as per SoC requirements.

Adding Pekon Gupta who had worked in Silicon team before.

Regards
Mugunthan V N

RE: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Gupta, Pekon]

> 
> On Friday 23 August 2013 10:26 PM, Santosh Shilimkar wrote:
> > On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
> >> On 23.08.2013 16:23, Santosh Shilimkar wrote:
> >>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
> >>>> +static const struct of_device_id cpsw_of_mtable[] = {
> >>>> +	{
> >>>> +		.compatible	= "ti,am3352-cpsw",
> >>> I didn't notice this earlier, but can't you use the IP version
> >>> as a compatible instead of using a SOC name. Whats really SOC specific
> >>> on this IP ? Sorry i have missed any earlier discussion on this but
> >>> this approach doesn't seem good. Its like adding SOC checks in the
> >>> driver subsystem.
> >> As I already mentioned in the cover letter and in the commit message, I
> >> just don't know which criteria makes most sense here.
> >>
> >> On a general note, I would say that chances that this exactly IP core
> >> with the same version number will appear on some other silicon which
> >> doesn't support the control mode register in an AM33xx fashion, is not
> >> necessarily negligible.
> >>
> >> So what that new compatible string denotes is the cpsw in a version as
> >> found on am3352 SoCs, which is actually exactly what it does.
> >>
> >> I don't have a strong opinion here, but see your point. I just don't
> >> have a better idea on how to treat that.
> >>
> > So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> > That way if in future if someone uses those features, they can use
> > this compatible if they don't they use the one which suites that
> > SOC.
> >
> We cannot map control module register with CPSW IP version as both comes
> from different design team and CPSW ip version can be same across SoC
> and gmii sel register definition can be different. Control module
> defines may vary in different SoC as per SoC requirements.
> 
> Adding Pekon Gupta who had worked in Silicon team before.
> 
[Pekon]: My opinion here..
Uniformity in control module cannot be guaranteed, neither in register
offsets nor their functionality. Uniformity is usually maintained till the
point same person is writing the spec, or it’s a derivative device.
Control-module should not be categorized as IP, instead it’s a group of
miscellaneous logic usually consisting of following:
(a) SoC bug-fixes across silicon revisions.
(b) SoC specific logic like device_type, JTAG-ID.
(c) IP bug fixes which could not be accommodated in IP address-map.
(d) IP logic which depends on SoC configurations.

Due to this un-deterministic composition of control-module, having 
a dedicated driver for control module might not work either, as it
has to be updated | re-written for every new device.

So, Following can be used as guideline to determine compatibility
string for DT bindings..
- If binding maps to (a) and (c), .i.e., bug-fixes for SoC or IP, 
 then its most likely that these would change either in next silicon
 revision or in next devices. Hence such bindings should use *not*
 use IP compatibility strings, they _may_ use SoC-name based
 compatibility string.

- Else if binding maps to (b),.i.e., SoC specific configurations.
 then their offsets and functionality should remain same across the
 family of devices at-least, so it should use SoC-name based
 compatibility string.
- And if binding maps to (d), .i.e., IP feature but depending on SoC
 Then it can use IP based compatibility string, along with IP version.

(same approach can be used for CPSW binding used here,

with regards, pekon
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Sekhar Nori]

On 8/23/2013 7:53 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>> In order to support features that are specific to the AM335x IP, we have
>> to add hardware types and another compatible string.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 4e5ca54..b717458 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>  ------------------------------------------------------
>>  
>>  Required properties:
>> -- compatible		: Should be "ti,cpsw"
>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>  - reg			: physical base address and size of the cpsw
>>  			  registers map.
>>  			  An optional third memory region can be supplied if
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 7a25ff4..73c44cb6 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -155,6 +155,11 @@ do {								\
>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>  		priv->data.active_slave)
>>  
>> +enum {
>> +	CPSW_TYPE_GENERIC,
>> +	CPSW_TYPE_AM33XX
>> +};
>> +
>>  static int debug_level;
>>  module_param(debug_level, int, 0);
>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>  }
>>  
>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.

The objection then should not just be to the name of compatible string,
but to handling AM335x control module register in CPSW driver. What
would be the alternate method of doing what Daniel is doing? I vaguely
remember there were attempts to develop an independent control module
driver that were abandoned.

Thanks,
Sekhar

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Mugunthan V N]

On Friday 23 August 2013 07:53 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>> In order to support features that are specific to the AM335x IP, we have
>> to add hardware types and another compatible string.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 4e5ca54..b717458 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>  ------------------------------------------------------
>>  
>>  Required properties:
>> -- compatible		: Should be "ti,cpsw"
>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>  - reg			: physical base address and size of the cpsw
>>  			  registers map.
>>  			  An optional third memory region can be supplied if
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 7a25ff4..73c44cb6 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -155,6 +155,11 @@ do {								\
>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>  		priv->data.active_slave)
>>  
>> +enum {
>> +	CPSW_TYPE_GENERIC,
>> +	CPSW_TYPE_AM33XX
>> +};
>> +
>>  static int debug_level;
>>  module_param(debug_level, int, 0);
>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>  }
>>  
>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.
>
>
But the same IP can be used in different SoC as well where the control
register may be different as per the Silicon Integration team's decision?

Ideally there should be a separate control module driver so that it can
take care of different SoC related needs.

Regards
Mugunthan V N

Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar]

On Friday 23 August 2013 12:45 PM, Mugunthan V N wrote:
> On Friday 23 August 2013 07:53 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>> In order to support features that are specific to the AM335x IP, we have
>>> to add hardware types and another compatible string.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>> index 4e5ca54..b717458 100644
>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>>  ------------------------------------------------------
>>>  
>>>  Required properties:
>>> -- compatible		: Should be "ti,cpsw"
>>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>>  - reg			: physical base address and size of the cpsw
>>>  			  registers map.
>>>  			  An optional third memory region can be supplied if
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>>> index 7a25ff4..73c44cb6 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -155,6 +155,11 @@ do {								\
>>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>>  		priv->data.active_slave)
>>>  
>>> +enum {
>>> +	CPSW_TYPE_GENERIC,
>>> +	CPSW_TYPE_AM33XX
>>> +};
>>> +
>>>  static int debug_level;
>>>  module_param(debug_level, int, 0);
>>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>>  }
>>>  
>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>> +	{
>>> +		.compatible	= "ti,am3352-cpsw",
>> I didn't notice this earlier, but can't you use the IP version
>> as a compatible instead of using a SOC name. Whats really SOC specific
>> on this IP ? Sorry i have missed any earlier discussion on this but
>> this approach doesn't seem good. Its like adding SOC checks in the
>> driver subsystem.
>>
>>
> But the same IP can be used in different SoC as well where the control
> register may be different as per the Silicon Integration team's decision?
> 
> Ideally there should be a separate control module driver so that it can
> take care of different SoC related needs.
> 
Instead of having one control module driver to address all the requirements
in various IP's, you could specifically address that in subsystem driver layer.
SOC guys invariably use control modules for certain functionality since it
is easy and quick fix. And their is uniformity in the way they are
implemented for different IP needs.

In this specific case, if the CPSW IP version has some part of the
functionality implemented using control module that is just harfaware
choice. Its not mandatory to partition that in two sub-system. Rather
consider it as a special IP version and treat it accordingly in the
driver.

Regards,
Santosh

[PATCH v4 4/5] 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 when the slave is opened.

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                 | 61 ++++++++++++++++++++++++++
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 3 files changed, 64 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index b717458..0895a51 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -34,6 +34,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 73c44cb6..86b8f7a 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -138,6 +138,13 @@ 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_RMII2_IO_CLK_EN	BIT(7)
+#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)
+
 #define cpsw_enable_irq(priv)	\
 	do {			\
 		u32 i;		\
@@ -980,6 +987,56 @@ static inline void cpsw_add_dual_emac_def_ale_entries(
 		priv->host_port, ALE_VLAN, slave->port_vlan);
 }
 
+static void cpsw_set_phy_interface_mode(struct cpsw_slave *slave,
+					struct cpsw_priv *priv)
+{
+	u32 reg, mask, mode = 0;
+
+	switch (priv->data.hw_type) {
+	case CPSW_TYPE_AM33XX:
+		if (!priv->gmii_sel_reg)
+			break;
+
+		reg = readl(priv->gmii_sel_reg);
+
+		if (slave->phy) {
+			switch (slave->phy->interface) {
+			case PHY_INTERFACE_MODE_MII:
+			default:
+				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;
+			};
+		}
+
+		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;
+
+		writel(reg, priv->gmii_sel_reg);
+		break;
+
+	default:
+		break;
+
+	}
+}
+
 static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 {
 	char name[32];
@@ -1028,6 +1085,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 			 slave->phy->phy_id);
 		phy_start(slave->phy);
 	}
+
+	cpsw_set_phy_interface_mode(slave, priv);
 }
 
 static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
@@ -1823,6 +1882,8 @@ 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-ext", 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/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 96c374a..3baa2350 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -19,6 +19,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

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

[Mugunthan V N]

On Friday 23 August 2013 07:46 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 when the slave is opened.
>
> 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                 | 61 ++++++++++++++++++++++++++
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 64 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index b717458..0895a51 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -34,6 +34,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 73c44cb6..86b8f7a 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -138,6 +138,13 @@ 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_RMII2_IO_CLK_EN	BIT(7)
> +#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)

These defined can go into cpsw.h so that we can maintain platform
defines in one file.


> +
>  #define cpsw_enable_irq(priv)	\
>  	do {			\
>  		u32 i;		\
> @@ -980,6 +987,56 @@ static inline void cpsw_add_dual_emac_def_ale_entries(
>  		priv->host_port, ALE_VLAN, slave->port_vlan);
>  }
>  
> +static void cpsw_set_phy_interface_mode(struct cpsw_slave *slave,
> +					struct cpsw_priv *priv)
> +{
> +	u32 reg, mask, mode = 0;

Please define each variable in separate line to make it clean.
Please check for IS_ERR(gmii_sel_reg) and return if gmii register is not
specified in DT then the below code will crash when trying to access
gmii_sel_reg.


> +
> +	switch (priv->data.hw_type) {
> +	case CPSW_TYPE_AM33XX:
> +		if (!priv->gmii_sel_reg)
> +			break;
> +
> +		reg = readl(priv->gmii_sel_reg);
> +
> +		if (slave->phy) {
> +			switch (slave->phy->interface) {
> +			case PHY_INTERFACE_MODE_MII:
> +			default:
> +				mode = AM33XX_GMII_SEL_MODE_MII;
> +				break;
> +			case PHY_INTERFACE_MODE_RMII:
> +				mode = AM33XX_GMII_SEL_MODE_RMII;
> +				break;
> +			case PHY_INTERFACE_MODE_RGMII:

You need to take care of other RGMII modes as well, for your info
AM335xevm phy mode is "rgmii-txid"

Regards
Mugunthan V N

[PATCH v4 5/5] ARM: dts: am33xx: adopt to cpsw changes

[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.

Also update the compatible string to make use of the am335x specific
features of the cpsw driver.

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

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index a785b95..c7b41ae 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -446,7 +446,7 @@
 		};
 
 		mac: ethernet@4a100000 {
-			compatible = "ti,cpsw";
+			compatible = "ti,am3352-cpsw", "ti,cpsw";
 			ti,hwmods = "cpgmac0";
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
@@ -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