Patchwork [1/5,v2] mv643xx_eth: add Device Tree bindings

login
register
mail settings
Submitter Florian Fainelli
Date April 4, 2013, 10:27 a.m.
Message ID <1365071235-11611-2-git-send-email-florian@openwrt.org>
Download mbox | patch
Permalink /patch/233707/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Florian Fainelli - April 4, 2013, 10:27 a.m.
This patch adds Device Tree bindings following the already defined
bindings at Documentation/devicetree/bindings/marvell.txt. The binding
documentation is also enhanced with new optionnal properties required
for supporting certain devices (RX/TX queue and SRAM). Since we now have
proper support for the orion MDIO bus driver, there is no need to fiddle
around with device tree phandles. PHY-less (MAC connected to switch)
configurations are supported by not specifying any phy phandle for an
ethernet node.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
- properly ifdef of_platform_bus_probe with CONFIG_OF
- handle of_platform_bus_probe errors and cleanup accordingly
- use of_property_read_u32 where applicable
- parse "duplex" and "speed" property in PHY-less configuration

 Documentation/devicetree/bindings/marvell.txt |   25 +++++-
 drivers/net/ethernet/marvell/mv643xx_eth.c    |  120 ++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 5 deletions(-)
Simon Baatz - April 4, 2013, 9:29 p.m.
Hi Florian

On Thu, Apr 04, 2013 at 12:27:11PM +0200, Florian Fainelli wrote:
> This patch adds Device Tree bindings following the already defined
> bindings at Documentation/devicetree/bindings/marvell.txt. The binding
> documentation is also enhanced with new optionnal properties required
> for supporting certain devices (RX/TX queue and SRAM). Since we now have
> proper support for the orion MDIO bus driver, there is no need to fiddle
> around with device tree phandles. PHY-less (MAC connected to switch)
> configurations are supported by not specifying any phy phandle for an
> ethernet node.
> 
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
> - properly ifdef of_platform_bus_probe with CONFIG_OF
> - handle of_platform_bus_probe errors and cleanup accordingly
> - use of_property_read_u32 where applicable
> - parse "duplex" and "speed" property in PHY-less configuration
> 
>  Documentation/devicetree/bindings/marvell.txt |   25 +++++-
>  drivers/net/ethernet/marvell/mv643xx_eth.c    |  120 ++++++++++++++++++++++++-
>  2 files changed, 140 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/marvell.txt b/Documentation/devicetree/bindings/marvell.txt
> index f1533d9..e70a013 100644
> --- a/Documentation/devicetree/bindings/marvell.txt
> +++ b/Documentation/devicetree/bindings/marvell.txt
> @@ -112,9 +112,14 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd.
>     Required properties:
>       - #address-cells : <1>
>       - #size-cells : <0>
> -     - compatible : "marvell,mv64360-eth-block"
> +     - compatible : "marvell,mv64360-eth-block", "marvell,mv64360-eth-group",
> +		    "marvell,mv643xx-eth-block"
>       - reg : Offset and length of the register set for this block
>  
> +   Optional properties:
> +     - tx-csum-limit : Hardware limit above which transmit checksumming
> +                       is disabled.
> +
>     Example Discovery Ethernet block node:
>       ethernet-block@2000 {
>  	     #address-cells = <1>;
> @@ -130,7 +135,7 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd.
>  
>     Required properties:
>       - device_type : Should be "network".
> -     - compatible : Should be "marvell,mv64360-eth".
> +     - compatible : Should be "marvell,mv64360-eth", "marvell,mv643xx-eth".
>       - reg : Should be <0>, <1>, or <2>, according to which registers
>         within the silicon block the device uses.
>       - interrupts : <a> where a is the interrupt number for the port.
> @@ -140,6 +145,22 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd.
>         controller.
>       - local-mac-address : 6 bytes, MAC address
>  
> +   Optional properties:
> +     - clocks : Phandle to the clock control device and gate bit
> +     - clock-names : String describing the clock gate bit
> +     - speed : Speed to force the link (10, 100, 1000), used when no
> +               phy property is defined
> +     - duplex : Duplex to force the link (0: half, 1: full), used when no
> +               phy property is defined
> +     - rx-queue-count : number of RX queues to use
> +     - tx-queue-count : number of TX queues to use
> +     - rx-queue-size : size of the RX queue (in bytes)
> +     - tx-queue-size : size of the TX queue (in bytes)
> +     - rx-sram-addr : address of the SRAM for RX path (non 0 means used)
> +     - rx-sram-size : size of the SRAM for RX path (non 0 means used)
> +     - tx-sram-addr : address of the SRAM for TX path (non 0 means used)
> +     - tx-sram-size : size of the SRAM for TX path (non 0 means used)
> +
>     Example Discovery Ethernet port node:
>       ethernet@0 {
>  	     device_type = "network";
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index aedbd82..75599a8 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -60,6 +60,10 @@
>  #include <linux/inet_lro.h>
>  #include <linux/slab.h>
>  #include <linux/clk.h>
> +#include <linux/stringify.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_net.h>
>  
>  static char mv643xx_eth_driver_name[] = "mv643xx_eth";
>  static char mv643xx_eth_driver_version[] = "1.4";
> @@ -2542,14 +2546,23 @@ static void infer_hw_params(struct mv643xx_eth_shared_private *msp)
>  	}
>  }
>  
> +static const struct of_device_id mv643xx_eth_match[] = {
> +	{ .compatible = "marvell,mv64360-eth" },
> +	{ .compatible = "marvell,mv643xx-eth" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mv643xx_eth_match);
> +
>  static int mv643xx_eth_shared_probe(struct platform_device *pdev)
>  {
>  	static int mv643xx_eth_version_printed;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct mv643xx_eth_shared_platform_data *pd = pdev->dev.platform_data;
>  	struct mv643xx_eth_shared_private *msp;
>  	const struct mbus_dram_target_info *dram;
>  	struct resource *res;
>  	int ret;
> +	int tx_csum_limit = 0;
>  
>  	if (!mv643xx_eth_version_printed++)
>  		pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n",

This is not related to your change, but there is a problem in this
function that has already been discussed in the past if I remember
correctly: The respective clock needs to be enabled here (at least
on Kirkwood), since accesses to the hardware are done below. 
Enabling the clock only in mv643xx_eth_probe() is too late.

As said, this is not a problem introduced by your changes (and which
is currently circumvented by enabling the respective clocks in
kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might
want to fix this now to get rid of unconditionally enabling the GE
clocks in the DT case.


> @@ -2576,13 +2589,23 @@ static int mv643xx_eth_shared_probe(struct platform_device *pdev)
>  	if (dram)
>  		mv643xx_eth_conf_mbus_windows(msp, dram);
>  
> -	msp->tx_csum_limit = (pd != NULL && pd->tx_csum_limit) ?
> -					pd->tx_csum_limit : 9 * 1024;
> +	if (np)
> +		of_property_read_u32(np, "tx-csum-limit", &tx_csum_limit);
> +	else
> +		tx_csum_limit = pd->tx_csum_limit;
> +
> +	msp->tx_csum_limit = tx_csum_limit ? tx_csum_limit : 9 * 1024;
>  	infer_hw_params(msp);
>  
>  	platform_set_drvdata(pdev, msp);
> +	ret = 0;
>  
> -	return 0;
> +#ifdef CONFIG_OF
> +	ret = of_platform_bus_probe(np, mv643xx_eth_match, &pdev->dev);
> +	if (ret)
> +		goto out_free;
> +#endif
> +	return ret;
>  
>  out_free:
>  	kfree(msp);
> @@ -2600,12 +2623,22 @@ static int mv643xx_eth_shared_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id mv643xx_eth_shared_match[] = {
> +	{ .compatible = "marvell,mv64360-eth-group" },
> +	{ .compatible = "marvell,mv64360-eth-block" },
> +	{ .compatible = "marvell,mv643xx-eth-group" },
> +	{ .compatible = "marvell,mv643xx-eth-block" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mv643xx_eth_shared_match);
> +
>  static struct platform_driver mv643xx_eth_shared_driver = {
>  	.probe		= mv643xx_eth_shared_probe,
>  	.remove		= mv643xx_eth_shared_remove,
>  	.driver = {
>  		.name	= MV643XX_ETH_SHARED_NAME,
>  		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(mv643xx_eth_shared_match),
>  	},
>  };
>  
> @@ -2764,6 +2797,74 @@ static const struct net_device_ops mv643xx_eth_netdev_ops = {
>  #endif
>  };
>  
> +#ifdef CONFIG_OF
> +static int mv643xx_eth_of_probe(struct platform_device *pdev)
> +{
> +	struct mv643xx_eth_platform_data *pd;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *shared = of_get_parent(np);
> +	struct device_node *phy_node;
> +	const int *prop;
> +	const char *mac_addr;
> +
> +	if (!pdev->dev.of_node)
> +		return 0;
> +
> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	pdev->dev.platform_data = pd;
> +
> +	pd->shared = of_find_device_by_node(shared);
> +	if (!pd->shared)
> +		return -ENODEV;
> +
> +	prop = of_get_property(np, "reg", NULL);
> +	if (!prop)
> +		return -EINVAL;
> +
> +	pd->port_number = be32_to_cpup(prop);
> +
> +	phy_node = of_parse_phandle(np, "phy", 0);
> +	if (!phy_node) {
> +		pd->phy_addr = MV643XX_ETH_PHY_NONE;
> +
> +		of_property_read_u32(np, "speed", &pd->speed);
> +		of_property_read_u32(np, "duplex", &pd->duplex);
> +	} else {
> +		prop = of_get_property(phy_node, "reg", NULL);
> +		if (prop)
> +			pd->phy_addr = be32_to_cpup(prop);
> +	}
> +
> +	mac_addr = of_get_mac_address(np);
> +	if (mac_addr)
> +		memcpy(pd->mac_addr, mac_addr, ETH_ALEN);
> +
> +#define rx_tx_queue_sram_property(_name)			\
> +	prop = of_get_property(np, __stringify(_name), NULL);	\
> +	if (prop)						\
> +		pd->_name = be32_to_cpup(prop);
> +
> +	rx_tx_queue_sram_property(rx_queue_count);
> +	rx_tx_queue_sram_property(tx_queue_count);
> +	rx_tx_queue_sram_property(rx_queue_size);
> +	rx_tx_queue_sram_property(tx_queue_size);
> +	rx_tx_queue_sram_property(rx_sram_addr);
> +	rx_tx_queue_sram_property(rx_sram_size);
> +	rx_tx_queue_sram_property(tx_sram_addr);
> +	rx_tx_queue_sram_property(rx_sram_size);
> +
> +	return 0;
> +}
> +#else
> +static inline int mv643xx_eth_of_probe(struct platform_device *dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int mv643xx_eth_probe(struct platform_device *pdev)
>  {
>  	struct mv643xx_eth_platform_data *pd;
> @@ -2772,7 +2873,12 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	int err;
>  
> +	err = mv643xx_eth_of_probe(pdev);
> +	if (err)
> +		return err;
> +
>  	pd = pdev->dev.platform_data;
> +
>  	if (pd == NULL) {
>  		dev_err(&pdev->dev, "no mv643xx_eth_platform_data\n");
>  		return -ENODEV;

You don't change the clk initialization here:

#if defined(CONFIG_HAVE_CLK)
	mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0"));
	if (!IS_ERR(mp->clk)) {
		clk_prepare_enable(mp->clk);
		mp->t_clk = clk_get_rate(mp->clk);
	}
#endif

Which, if I understand correctly, works in the DT case because you
assign "clock-names" to the clocks in the DTS. However, I wonder
whether this works for any but the first Ethernet device.

In the old platform device setup, the pdev->id was set when
initialiazing the platform_device structure in common.c.  Where is
this done in the DT case?


In phy_scan(), the phy is searched like this:

		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
				"orion-mdio-mii", addr);

		phydev = phy_connect(mp->dev, phy_id, mv643xx_eth_adjust_link,
				PHY_INTERFACE_MODE_GMII);

But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a
platform_device. I could not get this to work if the MDIO device is
setup via DT. Am I doing something wrong?


Additionally, in phy_scan() there is this:

	if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) {
		start = phy_addr_get(mp) & 0x1f;
		num = 32;
	} else {
	...

MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood
devices use "MV643XX_ETH_PHY_ADDR(0)".  If the module probe is
deferred in mv643xx_eth because the MDIO driver is not yet loaded,
all 32 PHY addresses are scanned without success.  This is not needed
and clutters the log.


- Simon




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli - April 5, 2013, 9:56 a.m.
Hello Simon,

First of all, thanks for getting these patches a try!

Le 04/04/13 23:29, Simon Baatz a écrit :
> Hi Florian
>

[snip]

>>   	if (!mv643xx_eth_version_printed++)
>>   		pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n",
>
> This is not related to your change, but there is a problem in this
> function that has already been discussed in the past if I remember
> correctly: The respective clock needs to be enabled here (at least
> on Kirkwood), since accesses to the hardware are done below.
> Enabling the clock only in mv643xx_eth_probe() is too late.
>
> As said, this is not a problem introduced by your changes (and which
> is currently circumvented by enabling the respective clocks in
> kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might
> want to fix this now to get rid of unconditionally enabling the GE
> clocks in the DT case.

I think there may have been some confusion between the "ethernet-group" 
clock and the actual Ethernet port inside the "ethernet-group". The 
mv643xx_eth driver assumes we have a per-port clock gating scheme, while 
I think we have a per "ethernet-group" clock gating scheme instead. Like 
you said, I think this should be addressed separately.

[snip]

>
> You don't change the clk initialization here:
>
> #if defined(CONFIG_HAVE_CLK)
> 	mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0"));
> 	if (!IS_ERR(mp->clk)) {
> 		clk_prepare_enable(mp->clk);
> 		mp->t_clk = clk_get_rate(mp->clk);
> 	}
> #endif
>
> Which, if I understand correctly, works in the DT case because you
> assign "clock-names" to the clocks in the DTS. However, I wonder
> whether this works for any but the first Ethernet device.
>
> In the old platform device setup, the pdev->id was set when
> initialiazing the platform_device structure in common.c.  Where is
> this done in the DT case?

Looks like you are right, in the DT case, I assume that we should lookup 
the clock using NULL instead of "1" or "0" so we match any clock instead 
of a specific one.

[snip]

>
>
> In phy_scan(), the phy is searched like this:
>
> 		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
> 				"orion-mdio-mii", addr);
>
> 		phydev = phy_connect(mp->dev, phy_id, mv643xx_eth_adjust_link,
> 				PHY_INTERFACE_MODE_GMII);
>
> But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a
> platform_device. I could not get this to work if the MDIO device is
> setup via DT. Am I doing something wrong?

I just missed updating this part of the code to probe for PHYs. The 
board I tested with uses a "PHY_NONE" configuration. I will add the 
missing bits for of_phy_connect() to be called here.

>
>
> Additionally, in phy_scan() there is this:
>
> 	if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) {
> 		start = phy_addr_get(mp) & 0x1f;
> 		num = 32;
> 	} else {
> 	...
>
> MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood
> devices use "MV643XX_ETH_PHY_ADDR(0)".  If the module probe is
> deferred in mv643xx_eth because the MDIO driver is not yet loaded,
> all 32 PHY addresses are scanned without success.  This is not needed
> and clutters the log.

Ok, I am not sure how we can circumvent the log cluttering that happens, 
what would be your suggestion?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Hesselbarth - April 5, 2013, 1:58 p.m.
On Fri, Apr 5, 2013 at 11:56 AM, Florian Fainelli <florian@openwrt.org> wrote:
> [snip]

Florian,

took me a while to try you patches out on Dove but now I fixed all
issues. I will
comment on all related patches but first I want to comment here.

One general note for Dove related patches: You didn't remove the registration of
ge platform_device from mach-dove/board-dt.c. That will lead to double
registration
of mdio and mv643xx_eth/shared, so you'll never be sure if DT or non-DT code is
executed. I haven't checked mach-kirkwood/board-dt.c or orion5x code.

>>>         if (!mv643xx_eth_version_printed++)
>>>                 pr_notice("MV-643xx 10/100/1000 ethernet driver version
>>> %s\n",
>>
>>
>> This is not related to your change, but there is a problem in this
>> function that has already been discussed in the past if I remember
>> correctly: The respective clock needs to be enabled here (at least
>> on Kirkwood), since accesses to the hardware are done below.
>> Enabling the clock only in mv643xx_eth_probe() is too late.
>>
>> As said, this is not a problem introduced by your changes (and which
>> is currently circumvented by enabling the respective clocks in
>> kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might
>> want to fix this now to get rid of unconditionally enabling the GE
>> clocks in the DT case.
>
>
> I think there may have been some confusion between the "ethernet-group"
> clock and the actual Ethernet port inside the "ethernet-group". The
> mv643xx_eth driver assumes we have a per-port clock gating scheme, while I
> think we have a per "ethernet-group" clock gating scheme instead. Like you
> said, I think this should be addressed separately.

IMHO, there should be a clocks property where ever you try to access registers,
i.e. in all three "parts" mv643xx_eth_shared (group), mv643xx_eth
(port) and mdio.
Since port depends on shared it would be ok to have it per group but that may
collide with other SoCs than Dove/Kirkwood that have per port clocks.

Is that separation (group/port) really required for any SoC?

>
> [snip]
>>
>> You don't change the clk initialization here:
>>
>> #if defined(CONFIG_HAVE_CLK)
>>         mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0"));
>>         if (!IS_ERR(mp->clk)) {
>>                 clk_prepare_enable(mp->clk);
>>                 mp->t_clk = clk_get_rate(mp->clk);
>>         }
>> #endif
>>
>> Which, if I understand correctly, works in the DT case because you
>> assign "clock-names" to the clocks in the DTS. However, I wonder
>> whether this works for any but the first Ethernet device.

Yes, it does. Assigned clocks from clocks property get a clock alias for
that device name (node name). Using anything else than NULL here is
IMHO just wrong. We should rather provide proper clock aliases for non-DT case.

>> In the old platform device setup, the pdev->id was set when
>> initialiazing the platform_device structure in common.c.  Where is
>> this done in the DT case?
>
> Looks like you are right, in the DT case, I assume that we should lookup the
> clock using NULL instead of "1" or "0" so we match any clock instead of a
> specific one.

Yes.

> [snip]
>>
>>
>> In phy_scan(), the phy is searched like this:
>>
>>                 snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
>>                                 "orion-mdio-mii", addr);
>>
>>                 phydev = phy_connect(mp->dev, phy_id,
>> mv643xx_eth_adjust_link,
>>                                 PHY_INTERFACE_MODE_GMII);
>>
>> But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a
>> platform_device. I could not get this to work if the MDIO device is
>> setup via DT. Am I doing something wrong?
>
> I just missed updating this part of the code to probe for PHYs. The board I
> tested with uses a "PHY_NONE" configuration. I will add the missing bits for
> of_phy_connect() to be called here.

I don't think that the ethernet controller should probe the PHY's on mdio-bus
at all. At least not for DT enabled platforms. I had a look at DT and non-DT
mdio-bus sources, and realized that there is a bus scan for non-DT only.
of_mdiobus_register requires you to set (and know) the PHY address.

I prepared a patch for of_mdio_register that will allow you to probe mdio and
assign phy addresses to each node found. Currently, the heuristic for probing
is: assign each phy node the next probed phy_addr starting with 0. But that
will not allow to e.g. set some PHY addresses and probe the rest.

We had a similar discussion whether to probe or not for DT nodes, and I guess
there also will be some discussion about the above patch. OTOH we could just
(again) ask users of every kirkwood/orion5x/dove board to tell their
phy addresses
and fail to probe the phy for new boards...

I will prepare a proper patch soon and post it on the corresponding lists.

>> Additionally, in phy_scan() there is this:
>>
>>         if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) {
>>                 start = phy_addr_get(mp) & 0x1f;
>>                 num = 32;
>>         } else {
>>         ...
>>
>> MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood
>> devices use "MV643XX_ETH_PHY_ADDR(0)".  If the module probe is
>> deferred in mv643xx_eth because the MDIO driver is not yet loaded,
>> all 32 PHY addresses are scanned without success.  This is not needed
>> and clutters the log.
>
>
> Ok, I am not sure how we can circumvent the log cluttering that happens,
> what would be your suggestion?

My suggestion is to change MV643XX_ETH_PHY_ADDR_DEFAULT from a valid
phy address (0)
to something invalid (32). I understand that using 0 helps if you
don't want to set it in mv643xx's platform_data
but it is always difficult to rely on that if 0 is a valid number.

Changing the above to 32 should just work because most (all?) boards
using phy_scan should also
already use MV643XX_ETH_PHY_ADDR_DEFAULT. I also suggest to rename
current define to a
better name, e.g. MV643XX_ETH_PHY_ADDR_AUTOSCAN.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli - April 5, 2013, 2:23 p.m.
Hello Sebastian,

Le 04/05/13 15:58, Sebastian Hesselbarth a écrit :
> On Fri, Apr 5, 2013 at 11:56 AM, Florian Fainelli <florian@openwrt.org> wrote:
>> [snip]
>
> Florian,
>
> took me a while to try you patches out on Dove but now I fixed all
> issues. I will
> comment on all related patches but first I want to comment here.
>
> One general note for Dove related patches: You didn't remove the registration of
> ge platform_device from mach-dove/board-dt.c. That will lead to double
> registration
> of mdio and mv643xx_eth/shared, so you'll never be sure if DT or non-DT code is
> executed. I haven't checked mach-kirkwood/board-dt.c or orion5x code.

This was intentional, this patchset is just preparatory in the sense 
that it does no conversion of the existing users of the mv643xx_eth 
platform driver over DT (have some patches to that though). I wanted to 
resume the discussion on these bindings first, then proceed with the 
conversion.

>
>>>>          if (!mv643xx_eth_version_printed++)
>>>>                  pr_notice("MV-643xx 10/100/1000 ethernet driver version
>>>> %s\n",
>>>
>>>
>>> This is not related to your change, but there is a problem in this
>>> function that has already been discussed in the past if I remember
>>> correctly: The respective clock needs to be enabled here (at least
>>> on Kirkwood), since accesses to the hardware are done below.
>>> Enabling the clock only in mv643xx_eth_probe() is too late.
>>>
>>> As said, this is not a problem introduced by your changes (and which
>>> is currently circumvented by enabling the respective clocks in
>>> kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might
>>> want to fix this now to get rid of unconditionally enabling the GE
>>> clocks in the DT case.
>>
>>
>> I think there may have been some confusion between the "ethernet-group"
>> clock and the actual Ethernet port inside the "ethernet-group". The
>> mv643xx_eth driver assumes we have a per-port clock gating scheme, while I
>> think we have a per "ethernet-group" clock gating scheme instead. Like you
>> said, I think this should be addressed separately.
>
> IMHO, there should be a clocks property where ever you try to access registers,
> i.e. in all three "parts" mv643xx_eth_shared (group), mv643xx_eth
> (port) and mdio.
> Since port depends on shared it would be ok to have it per group but that may
> collide with other SoCs than Dove/Kirkwood that have per port clocks.

Ok, which means that we should also teach mv643xx_eth_shared_probe() 
about it, as well as the orion-mdio driver. I don't have any particular 
objections since it should just make things safer with respect to clocking.

>
> Is that separation (group/port) really required for any SoC?

Probably not, it was not clear when I looked at mv78xx0 if it uses two 
ports per group or 4 groups and 1 port. Anyway, since we are re-using 
the existing Device Tree binding definition and that the hardware 
present itself as ethernet groups and ports, I don't see any problem 
with keeping that difference since it allows for fine-grained 
representation of the hardware.

>
>>
>> [snip]
>>>
>>> You don't change the clk initialization here:
>>>
>>> #if defined(CONFIG_HAVE_CLK)
>>>          mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0"));
>>>          if (!IS_ERR(mp->clk)) {
>>>                  clk_prepare_enable(mp->clk);
>>>                  mp->t_clk = clk_get_rate(mp->clk);
>>>          }
>>> #endif
>>>
>>> Which, if I understand correctly, works in the DT case because you
>>> assign "clock-names" to the clocks in the DTS. However, I wonder
>>> whether this works for any but the first Ethernet device.
>
> Yes, it does. Assigned clocks from clocks property get a clock alias for
> that device name (node name). Using anything else than NULL here is
> IMHO just wrong. We should rather provide proper clock aliases for non-DT case.
>
>>> In the old platform device setup, the pdev->id was set when
>>> initialiazing the platform_device structure in common.c.  Where is
>>> this done in the DT case?
>>
>> Looks like you are right, in the DT case, I assume that we should lookup the
>> clock using NULL instead of "1" or "0" so we match any clock instead of a
>> specific one.
>
> Yes.
>
>> [snip]
>>>
>>>
>>> In phy_scan(), the phy is searched like this:
>>>
>>>                  snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
>>>                                  "orion-mdio-mii", addr);
>>>
>>>                  phydev = phy_connect(mp->dev, phy_id,
>>> mv643xx_eth_adjust_link,
>>>                                  PHY_INTERFACE_MODE_GMII);
>>>
>>> But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a
>>> platform_device. I could not get this to work if the MDIO device is
>>> setup via DT. Am I doing something wrong?
>>
>> I just missed updating this part of the code to probe for PHYs. The board I
>> tested with uses a "PHY_NONE" configuration. I will add the missing bits for
>> of_phy_connect() to be called here.
>
> I don't think that the ethernet controller should probe the PHY's on mdio-bus
> at all. At least not for DT enabled platforms. I had a look at DT and non-DT
> mdio-bus sources, and realized that there is a bus scan for non-DT only.
> of_mdiobus_register requires you to set (and know) the PHY address.

One reason the Ethernet controller could do the probing is in the case 
we need to apply quirks (e.g: using phydev->flags) for instance. This 
can be done even after the MDIO bus driver did probe PHY devices though.

>
> I prepared a patch for of_mdio_register that will allow you to probe mdio and
> assign phy addresses to each node found. Currently, the heuristic for probing
> is: assign each phy node the next probed phy_addr starting with 0. But that
> will not allow to e.g. set some PHY addresses and probe the rest.

Ok, we just need to make sure that this does not break any specific use 
case, I don't think it does, since it seems to be more accurate or 
equivalent to Ethernet driver doing the probing.

>
> We had a similar discussion whether to probe or not for DT nodes, and I guess
> there also will be some discussion about the above patch. OTOH we could just
> (again) ask users of every kirkwood/orion5x/dove board to tell their
> phy addresses
> and fail to probe the phy for new boards...
>
> I will prepare a proper patch soon and post it on the corresponding lists.

Cool, thanks!

>
>>> Additionally, in phy_scan() there is this:
>>>
>>>          if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) {
>>>                  start = phy_addr_get(mp) & 0x1f;
>>>                  num = 32;
>>>          } else {
>>>          ...
>>>
>>> MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood
>>> devices use "MV643XX_ETH_PHY_ADDR(0)".  If the module probe is
>>> deferred in mv643xx_eth because the MDIO driver is not yet loaded,
>>> all 32 PHY addresses are scanned without success.  This is not needed
>>> and clutters the log.
>>
>>
>> Ok, I am not sure how we can circumvent the log cluttering that happens,
>> what would be your suggestion?
>
> My suggestion is to change MV643XX_ETH_PHY_ADDR_DEFAULT from a valid
> phy address (0)
> to something invalid (32). I understand that using 0 helps if you
> don't want to set it in mv643xx's platform_data
> but it is always difficult to rely on that if 0 is a valid number.
>
> Changing the above to 32 should just work because most (all?) boards
> using phy_scan should also
> already use MV643XX_ETH_PHY_ADDR_DEFAULT. I also suggest to rename
> current define to a
> better name, e.g. MV643XX_ETH_PHY_ADDR_AUTOSCAN.

Sounds good to me.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe - April 5, 2013, 6:04 p.m.
On Fri, Apr 05, 2013 at 03:58:03PM +0200, Sebastian Hesselbarth wrote:

> I don't think that the ethernet controller should probe the PHY's on mdio-bus
> at all. At least not for DT enabled platforms. I had a look at DT and non-DT
> mdio-bus sources, and realized that there is a bus scan for non-DT only.
> of_mdiobus_register requires you to set (and know) the PHY address.

DT platforms should have the option to use the standard phy-phandle
connection:


		mdio@72004 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "marvell,orion-mdio";
			reg = <0x72004 0x84>;
			status = "disabled";

+                        PHY1: ethernet-phy@1 {
+                                reg = <1>;
+                                device_type = "ethernet-phy";
+                        };
		};

		ethernet-group@72000 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "marvell,mv643xx-eth-block";
			reg = <0x72000 0x4000>;
			tx-csum-limit = <1600>;
			status = "disabled";

			egiga0: ethernet@0 {
				device_type = "network";
				compatible = "marvell,mv643xx-eth";
				reg = <0>;
				interrupts = <29>;
				clocks = <&gate_clk 2>;
+	                        phy-handle = <&PHY1>;
			};
		};

When phy-handle is present the ethernet driver should not probe/scan for
phys.

There is standard code to handle all of this - an important gain is
that the phy driver now has access to a DT node and can apply
phy-specific properties.

> We had a similar discussion whether to probe or not for DT nodes,
> and I guess there also will be some discussion about the above
> patch. OTOH we could just (again) ask users of every
> kirkwood/orion5x/dove board to tell their phy addresses and fail to
> probe the phy for new boards...

Maybe print a warning and call the no-DT phy probe code if phy-handle
is nor present?

Not sure this should be in the common code, phy probing is sketchy, it
shouldn't be encouraged, IMHO..

Jason
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Hesselbarth - April 5, 2013, 8:35 p.m.
On 04/05/2013 08:04 PM, Jason Gunthorpe wrote:
> On Fri, Apr 05, 2013 at 03:58:03PM +0200, Sebastian Hesselbarth wrote:
>> I don't think that the ethernet controller should probe the PHY's on mdio-bus
>> at all. At least not for DT enabled platforms. I had a look at DT and non-DT
>> mdio-bus sources, and realized that there is a bus scan for non-DT only.
>> of_mdiobus_register requires you to set (and know) the PHY address.
>
> DT platforms should have the option to use the standard phy-phandle
> connection:
>
>
> 		mdio@72004 {
> 			#address-cells =<1>;
> 			#size-cells =<0>;
> 			compatible = "marvell,orion-mdio";
> 			reg =<0x72004 0x84>;
> 			status = "disabled";
>
> +                        PHY1: ethernet-phy@1 {
> +                                reg =<1>;
> +                                device_type = "ethernet-phy";
> +                        };
> 		};
>
> 		ethernet-group@72000 {
> 			#address-cells =<1>;
> 			#size-cells =<0>;
> 			compatible = "marvell,mv643xx-eth-block";
> 			reg =<0x72000 0x4000>;
> 			tx-csum-limit =<1600>;
> 			status = "disabled";
>
> 			egiga0: ethernet@0 {
> 				device_type = "network";
> 				compatible = "marvell,mv643xx-eth";
> 				reg =<0>;
> 				interrupts =<29>;
> 				clocks =<&gate_clk 2>;
> +	                        phy-handle =<&PHY1>;
> 			};
> 		};
>
> When phy-handle is present the ethernet driver should not probe/scan for
> phys.
>
> There is standard code to handle all of this - an important gain is
> that the phy driver now has access to a DT node and can apply
> phy-specific properties.

The above is what I use as a modification of Florian's patches.
I compared of_mdiobus_register against mdiobus_register. The difference
is that DT version does not scan the bus for attached PHYs. That is ok,
if you know the phy_address of the PHY that you pass the handle of. But 
in most cases there will be only one PHY on the mdiobus and especially
for new boards probing will help here.

>> We had a similar discussion whether to probe or not for DT nodes,
>> and I guess there also will be some discussion about the above
>> patch. OTOH we could just (again) ask users of every
>> kirkwood/orion5x/dove board to tell their phy addresses and fail to
>> probe the phy for new boards...
>
> Maybe print a warning and call the no-DT phy probe code if phy-handle
> is nor present?

I think it would be best to spam each probing result during mdiobus
scan to encourage people to edit the DT and put in the phy_addr
directly. IMHO it will be best to have bus scan on mdiobus rather than
ethernet driver.

> Not sure this should be in the common code, phy probing is sketchy, it
> shouldn't be encouraged, IMHO..

Actually, it is in common code (non-DT case) and I think passing the
phy_addr by DT node like above is best. But for boards you don't
know the address (yet) probing helps a lot. If all phy nodes have their
reg property set, no probing will be performed.

For testing mdio bus probe, I used

mdio {
	...
	ethphy: ethernet-phy {
		reg = <32>;
	};
};

And 32 should be defined as OF_PHY_ADDR_AUTOSCAN. It is an invalid
address as max phy_addr is 31. I also thought about a bool property
but passing an invalid reg property in SoC file allows to overwrite
it in board file with the actual address easily.
And AFAIK bool properties cannot be removed later on by board specific
nodes.

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

Patch

diff --git a/Documentation/devicetree/bindings/marvell.txt b/Documentation/devicetree/bindings/marvell.txt
index f1533d9..e70a013 100644
--- a/Documentation/devicetree/bindings/marvell.txt
+++ b/Documentation/devicetree/bindings/marvell.txt
@@ -112,9 +112,14 @@  prefixed with the string "marvell,", for Marvell Technology Group Ltd.
    Required properties:
      - #address-cells : <1>
      - #size-cells : <0>
-     - compatible : "marvell,mv64360-eth-block"
+     - compatible : "marvell,mv64360-eth-block", "marvell,mv64360-eth-group",
+		    "marvell,mv643xx-eth-block"
      - reg : Offset and length of the register set for this block
 
+   Optional properties:
+     - tx-csum-limit : Hardware limit above which transmit checksumming
+                       is disabled.
+
    Example Discovery Ethernet block node:
      ethernet-block@2000 {
 	     #address-cells = <1>;
@@ -130,7 +135,7 @@  prefixed with the string "marvell,", for Marvell Technology Group Ltd.
 
    Required properties:
      - device_type : Should be "network".
-     - compatible : Should be "marvell,mv64360-eth".
+     - compatible : Should be "marvell,mv64360-eth", "marvell,mv643xx-eth".
      - reg : Should be <0>, <1>, or <2>, according to which registers
        within the silicon block the device uses.
      - interrupts : <a> where a is the interrupt number for the port.
@@ -140,6 +145,22 @@  prefixed with the string "marvell,", for Marvell Technology Group Ltd.
        controller.
      - local-mac-address : 6 bytes, MAC address
 
+   Optional properties:
+     - clocks : Phandle to the clock control device and gate bit
+     - clock-names : String describing the clock gate bit
+     - speed : Speed to force the link (10, 100, 1000), used when no
+               phy property is defined
+     - duplex : Duplex to force the link (0: half, 1: full), used when no
+               phy property is defined
+     - rx-queue-count : number of RX queues to use
+     - tx-queue-count : number of TX queues to use
+     - rx-queue-size : size of the RX queue (in bytes)
+     - tx-queue-size : size of the TX queue (in bytes)
+     - rx-sram-addr : address of the SRAM for RX path (non 0 means used)
+     - rx-sram-size : size of the SRAM for RX path (non 0 means used)
+     - tx-sram-addr : address of the SRAM for TX path (non 0 means used)
+     - tx-sram-size : size of the SRAM for TX path (non 0 means used)
+
    Example Discovery Ethernet port node:
      ethernet@0 {
 	     device_type = "network";
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index aedbd82..75599a8 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -60,6 +60,10 @@ 
 #include <linux/inet_lro.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
+#include <linux/stringify.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_net.h>
 
 static char mv643xx_eth_driver_name[] = "mv643xx_eth";
 static char mv643xx_eth_driver_version[] = "1.4";
@@ -2542,14 +2546,23 @@  static void infer_hw_params(struct mv643xx_eth_shared_private *msp)
 	}
 }
 
+static const struct of_device_id mv643xx_eth_match[] = {
+	{ .compatible = "marvell,mv64360-eth" },
+	{ .compatible = "marvell,mv643xx-eth" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mv643xx_eth_match);
+
 static int mv643xx_eth_shared_probe(struct platform_device *pdev)
 {
 	static int mv643xx_eth_version_printed;
+	struct device_node *np = pdev->dev.of_node;
 	struct mv643xx_eth_shared_platform_data *pd = pdev->dev.platform_data;
 	struct mv643xx_eth_shared_private *msp;
 	const struct mbus_dram_target_info *dram;
 	struct resource *res;
 	int ret;
+	int tx_csum_limit = 0;
 
 	if (!mv643xx_eth_version_printed++)
 		pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n",
@@ -2576,13 +2589,23 @@  static int mv643xx_eth_shared_probe(struct platform_device *pdev)
 	if (dram)
 		mv643xx_eth_conf_mbus_windows(msp, dram);
 
-	msp->tx_csum_limit = (pd != NULL && pd->tx_csum_limit) ?
-					pd->tx_csum_limit : 9 * 1024;
+	if (np)
+		of_property_read_u32(np, "tx-csum-limit", &tx_csum_limit);
+	else
+		tx_csum_limit = pd->tx_csum_limit;
+
+	msp->tx_csum_limit = tx_csum_limit ? tx_csum_limit : 9 * 1024;
 	infer_hw_params(msp);
 
 	platform_set_drvdata(pdev, msp);
+	ret = 0;
 
-	return 0;
+#ifdef CONFIG_OF
+	ret = of_platform_bus_probe(np, mv643xx_eth_match, &pdev->dev);
+	if (ret)
+		goto out_free;
+#endif
+	return ret;
 
 out_free:
 	kfree(msp);
@@ -2600,12 +2623,22 @@  static int mv643xx_eth_shared_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id mv643xx_eth_shared_match[] = {
+	{ .compatible = "marvell,mv64360-eth-group" },
+	{ .compatible = "marvell,mv64360-eth-block" },
+	{ .compatible = "marvell,mv643xx-eth-group" },
+	{ .compatible = "marvell,mv643xx-eth-block" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mv643xx_eth_shared_match);
+
 static struct platform_driver mv643xx_eth_shared_driver = {
 	.probe		= mv643xx_eth_shared_probe,
 	.remove		= mv643xx_eth_shared_remove,
 	.driver = {
 		.name	= MV643XX_ETH_SHARED_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mv643xx_eth_shared_match),
 	},
 };
 
@@ -2764,6 +2797,74 @@  static const struct net_device_ops mv643xx_eth_netdev_ops = {
 #endif
 };
 
+#ifdef CONFIG_OF
+static int mv643xx_eth_of_probe(struct platform_device *pdev)
+{
+	struct mv643xx_eth_platform_data *pd;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *shared = of_get_parent(np);
+	struct device_node *phy_node;
+	const int *prop;
+	const char *mac_addr;
+
+	if (!pdev->dev.of_node)
+		return 0;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	pdev->dev.platform_data = pd;
+
+	pd->shared = of_find_device_by_node(shared);
+	if (!pd->shared)
+		return -ENODEV;
+
+	prop = of_get_property(np, "reg", NULL);
+	if (!prop)
+		return -EINVAL;
+
+	pd->port_number = be32_to_cpup(prop);
+
+	phy_node = of_parse_phandle(np, "phy", 0);
+	if (!phy_node) {
+		pd->phy_addr = MV643XX_ETH_PHY_NONE;
+
+		of_property_read_u32(np, "speed", &pd->speed);
+		of_property_read_u32(np, "duplex", &pd->duplex);
+	} else {
+		prop = of_get_property(phy_node, "reg", NULL);
+		if (prop)
+			pd->phy_addr = be32_to_cpup(prop);
+	}
+
+	mac_addr = of_get_mac_address(np);
+	if (mac_addr)
+		memcpy(pd->mac_addr, mac_addr, ETH_ALEN);
+
+#define rx_tx_queue_sram_property(_name)			\
+	prop = of_get_property(np, __stringify(_name), NULL);	\
+	if (prop)						\
+		pd->_name = be32_to_cpup(prop);
+
+	rx_tx_queue_sram_property(rx_queue_count);
+	rx_tx_queue_sram_property(tx_queue_count);
+	rx_tx_queue_sram_property(rx_queue_size);
+	rx_tx_queue_sram_property(tx_queue_size);
+	rx_tx_queue_sram_property(rx_sram_addr);
+	rx_tx_queue_sram_property(rx_sram_size);
+	rx_tx_queue_sram_property(tx_sram_addr);
+	rx_tx_queue_sram_property(rx_sram_size);
+
+	return 0;
+}
+#else
+static inline int mv643xx_eth_of_probe(struct platform_device *dev)
+{
+	return 0;
+}
+#endif
+
 static int mv643xx_eth_probe(struct platform_device *pdev)
 {
 	struct mv643xx_eth_platform_data *pd;
@@ -2772,7 +2873,12 @@  static int mv643xx_eth_probe(struct platform_device *pdev)
 	struct resource *res;
 	int err;
 
+	err = mv643xx_eth_of_probe(pdev);
+	if (err)
+		return err;
+
 	pd = pdev->dev.platform_data;
+
 	if (pd == NULL) {
 		dev_err(&pdev->dev, "no mv643xx_eth_platform_data\n");
 		return -ENODEV;
@@ -2896,6 +3002,8 @@  out:
 	}
 #endif
 	free_netdev(dev);
+	if (pdev->dev.of_node)
+		kfree(pd);
 
 	return err;
 }
@@ -2903,6 +3011,7 @@  out:
 static int mv643xx_eth_remove(struct platform_device *pdev)
 {
 	struct mv643xx_eth_private *mp = platform_get_drvdata(pdev);
+	struct mv643xx_eth_platform_data *pd = pdev->dev.platform_data;
 
 	unregister_netdev(mp->dev);
 	if (mp->phy != NULL)
@@ -2918,6 +3027,9 @@  static int mv643xx_eth_remove(struct platform_device *pdev)
 
 	free_netdev(mp->dev);
 
+	if (pdev->dev.of_node)
+		kfree(pd);
+
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
@@ -2935,6 +3047,7 @@  static void mv643xx_eth_shutdown(struct platform_device *pdev)
 		port_reset(mp);
 }
 
+
 static struct platform_driver mv643xx_eth_driver = {
 	.probe		= mv643xx_eth_probe,
 	.remove		= mv643xx_eth_remove,
@@ -2942,6 +3055,7 @@  static struct platform_driver mv643xx_eth_driver = {
 	.driver = {
 		.name	= MV643XX_ETH_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mv643xx_eth_match),
 	},
 };