Hi Sebastian,
On Mon, May 06, 2013 at 05:33:34PM +0200, Sebastian Hesselbarth wrote:
> From: Florian Fainelli <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> Changelog:
> v2->v3:
> - rebase on top of mv643xx_eth clean-ups
> - do not reparse existing platform_data
> - use managed devm_kzalloc for parsed platform_data
> - use of_property_read_u32 where applicable
> - add phy_node to platform_data
> - use of_connect_phy if DT phy node was found
>
> v1->v2:
> - 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
>
> Cc: Grant Likely <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Rob Landley <[email protected]>
> Cc: Lennert Buytenhek <[email protected]>
> Cc: David Miller <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Arnaud Patard <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: Jean-Francois Moine <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Cc: Simon Guinot <[email protected]>
> Cc: Jamie Lentin <[email protected]>
> Cc: Michael Walle <[email protected]>
> Cc: Eric Hutter <[email protected]>
> Cc: Joshua Coombs <[email protected]>
> Cc: Willy Tarreau <[email protected]>
> Cc: Simon Baatz <[email protected]>
> Cc: Alan M Butler <[email protected]>
> Cc: Nigel Roberts <[email protected]>
> Cc: Valentin Longchamp <[email protected]>
> Cc: Stefan Peter <[email protected]>
> Cc: Arnaud Ebalard <[email protected]>
> Cc: Nobuhiro Iwamatsu <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> Documentation/devicetree/bindings/marvell.txt | 22 +++++-
> drivers/net/ethernet/marvell/mv643xx_eth.c | 108 ++++++++++++++++++++++++-
> include/linux/mv643xx_eth.h | 3 +
> 3 files changed, 128 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/marvell.txt b/Documentation/devicetree/bindings/marvell.txt
> index f7a0da6..73ea12f 100644
> --- a/Documentation/devicetree/bindings/marvell.txt
> +++ b/Documentation/devicetree/bindings/marvell.txt
> @@ -112,11 +112,13 @@ 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,mv643xx-eth-block"
> - reg : Offset and length of the register set for this block
>
> Optional properties:
> - clocks : Phandle to the clock control device and gate bit
> + - tx-csum-limit : Hardware limit above which transmit checksumming
> + is disabled.
>
> Example Discovery Ethernet block node:
> ethernet-block@2000 {
> @@ -133,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.
> @@ -143,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 d0afeea..efa5a2f 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -60,6 +60,11 @@
> #include <linux/types.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_mdio.h>
> +#include <linux/of_net.h>
>
> static char mv643xx_eth_driver_name[] = "mv643xx_eth";
> static char mv643xx_eth_driver_version[] = "1.4";
> @@ -2450,13 +2455,22 @@ 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" },
> + { },
> +};
> +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 tx_csum_limit = 0;
>
> if (!mv643xx_eth_version_printed++)
> pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n",
> @@ -2485,13 +2499,21 @@ 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);
>
> +#ifdef CONFIG_OF
> + return of_platform_bus_probe(np, mv643xx_eth_match, &pdev->dev);
I have tested this on Kirkwood (Sheevaplug eSATA). When using
mv643xx_eth as a module with a built-in mvmdio the GbE port works.
However, when unloading the mv643xx_eth module and loading it again,
the second call to of_platform_bus_probe() results in a warning:
[ 190.542992] WARNING: at fs/sysfs/dir.c:530 sysfs_add_one+0x7c/0xa4()
[ 190.549372] sysfs: cannot create duplicate filename '/devices/ocp.0/f1072000.
ethernet-controller/0.ethernet-port'
(Looks more like a problem of of_platform_bus_probe() than a problem
in the driver?)
> +#else
> return 0;
> +#endif
> }
>
> static int mv643xx_eth_shared_remove(struct platform_device *pdev)
> @@ -2505,12 +2527,20 @@ 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-block" },
> + { .compatible = "marvell,mv643xx-eth-block" },
> + { },
> +};
> +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),
> },
> };
>
> @@ -2669,6 +2699,68 @@ 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);
> + const int *prop;
> + const char *mac_addr;
> +
> + if (pdev->dev.platform_data || !pdev->dev.of_node)
> + return 0;
> +
> + pd = devm_kzalloc(&pdev->dev, 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;
> +
> + if (of_property_read_u32(np, "reg", &pd->port_number))
> + return -EINVAL;
> +
> + pd->phy_node = of_parse_phandle(np, "phy", 0);
> + if (!pd->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);
> + }
> +
> + 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) \
> + do { \
> + prop = of_get_property(np, __stringify(_name), NULL); \
> + if (prop) \
> + pd->_name = be32_to_cpup(prop); \
> + } while (0)
> +
> + 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;
> @@ -2677,6 +2769,10 @@ 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");
If the clock isn't already enabled (mvmdio and mv643xx_eth both built
as modules), a delay seems to be necessary in mv643xx_eth_probe()
after enabling the clock on my hardware. Otherwise the device hangs.
Andrew found the same in the past (see [1]). udelay(50) seems to be
sufficient in my case.
> @@ -2717,7 +2813,12 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
> netif_set_real_num_rx_queues(dev, mp->rxq_count);
>
> if (pd->phy_addr != MV643XX_ETH_PHY_NONE) {
> - mp->phy = phy_scan(mp, pd->phy_addr);
> + if (pd->phy_node)
> + mp->phy = of_phy_connect(mp->dev, pd->phy_node,
> + mv643xx_eth_adjust_link, 0,
> + PHY_INTERFACE_MODE_GMII);
of_phy_connect() returns NULL in case of an error and no ERR_PTR.
- Simon
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/103521.html
On 05/20/2013 11:19 PM, Simon Baatz wrote:
> On Mon, May 06, 2013 at 05:33:34PM +0200, Sebastian Hesselbarth wrote:
>> From: Florian Fainelli<[email protected]>
>>
...
>> @@ -2485,13 +2499,21 @@ 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);
>>
>> +#ifdef CONFIG_OF
>> + return of_platform_bus_probe(np, mv643xx_eth_match,&pdev->dev);
>
> I have tested this on Kirkwood (Sheevaplug eSATA). When using
> mv643xx_eth as a module with a built-in mvmdio the GbE port works.
> However, when unloading the mv643xx_eth module and loading it again,
> the second call to of_platform_bus_probe() results in a warning:
>
> [ 190.542992] WARNING: at fs/sysfs/dir.c:530 sysfs_add_one+0x7c/0xa4()
> [ 190.549372] sysfs: cannot create duplicate filename '/devices/ocp.0/f1072000.
> ethernet-controller/0.ethernet-port'
>
> (Looks more like a problem of of_platform_bus_probe() than a problem
> in the driver?)
Hi Simon,
thanks for the review. I am right now working on a v4 of the DT support
patches for mv643xx_eth and the above will not be there anymore. I will
test v4 for rmmod/modprobe issues before posting.
>> @@ -2677,6 +2769,10 @@ 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");
>
> If the clock isn't already enabled (mvmdio and mv643xx_eth both built
> as modules), a delay seems to be necessary in mv643xx_eth_probe()
> after enabling the clock on my hardware. Otherwise the device hangs.
> Andrew found the same in the past (see [1]). udelay(50) seems to be
> sufficient in my case.
Hmm, I am wondering if that delay shouldn't be in the clock provider
then. I test it on Dove also and look for a way to insert the delay
if neccessary. Maybe Andrew can also comment on this.
>> @@ -2717,7 +2813,12 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
>> netif_set_real_num_rx_queues(dev, mp->rxq_count);
>>
>> if (pd->phy_addr != MV643XX_ETH_PHY_NONE) {
>> - mp->phy = phy_scan(mp, pd->phy_addr);
>> + if (pd->phy_node)
>> + mp->phy = of_phy_connect(mp->dev, pd->phy_node,
>> + mv643xx_eth_adjust_link, 0,
>> + PHY_INTERFACE_MODE_GMII);
>
> of_phy_connect() returns NULL in case of an error and no ERR_PTR.
True, will take care of that.
Sebastian