netdev - Re: [PATCH v2 3/4] net: ethoc: set up MII management bus clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <52E8A74F.9060603@gmail.com>
Date:	Tue, 28 Jan 2014 23:01:35 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Max Filippov <jcmvbkbc@...il.com>, linux-xtensa@...ux-xtensa.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
CC:	Chris Zankel <chris@...kel.net>, Marc Gauthier <marc@...ence.com>,
	"David S. Miller" <davem@...emloft.net>,
	Ben Hutchings <ben@...adent.org.uk>
Subject: Re: [PATCH v2 3/4] net: ethoc: set up MII management bus clock

Le 28/01/2014 22:00, Max Filippov a écrit :
> MII management bus clock is derived from the MAC clock by dividing it by
> MIIMODER register CLKDIV field value. This value may need to be set up
> in case it is undefined or its default value is too high (and
> communication with PHY is too slow) or too low (and communication with
> PHY is impossible). The value of CLKDIV is not specified directly, but
> is derived from the MAC clock for the default MII management bus frequency
> of 2.5MHz. The MAC clock may be specified in the platform data, or as
> either 'clock-frequency' or 'clocks' device tree attribute.
>
> Signed-off-by: Max Filippov <jcmvbkbc@...il.com>
> ---
> Changes v1->v2:
> - drop MDIO bus frequency configurability, always configure for standard
>    2.5MHz;
> - allow using common clock framework to provide ethoc clock.
>
>   drivers/net/ethernet/ethoc.c | 37 +++++++++++++++++++++++++++++++++++--
>   include/net/ethoc.h          |  1 +
>   2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 5643b2d..5854d41 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -13,6 +13,7 @@
>
>   #include <linux/dma-mapping.h>
>   #include <linux/etherdevice.h>
> +#include <linux/clk.h>
>   #include <linux/crc32.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
> @@ -216,6 +217,7 @@ struct ethoc {
>
>   	struct phy_device *phy;
>   	struct mii_bus *mdio;
> +	struct clk *clk;
>   	s8 phy_id;
>   };
>
> @@ -925,6 +927,8 @@ static int ethoc_probe(struct platform_device *pdev)
>   	int num_bd;
>   	int ret = 0;
>   	bool random_mac = false;
> +	u32 eth_clkfreq = 0;
> +	struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
>
>   	/* allocate networking device */
>   	netdev = alloc_etherdev(sizeof(struct ethoc));
> @@ -1038,8 +1042,7 @@ static int ethoc_probe(struct platform_device *pdev)
>   	}
>
>   	/* Allow the platform setup code to pass in a MAC address. */
> -	if (dev_get_platdata(&pdev->dev)) {
> -		struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	if (pdata) {
>   		memcpy(netdev->dev_addr, pdata->hwaddr, IFHWADDRLEN);
>   		priv->phy_id = pdata->phy_id;
>   	} else {
> @@ -1077,6 +1080,32 @@ static int ethoc_probe(struct platform_device *pdev)
>   	if (random_mac)
>   		netdev->addr_assign_type = NET_ADDR_RANDOM;
>
> +	/* Allow the platform setup code to adjust MII management bus clock. */
> +	if (pdata)
> +		eth_clkfreq = pdata->eth_clkfreq;

Since this is a new member, why not make it a struct clk pointer 
directly so you could simplify the code path?

> +	else
> +		of_property_read_u32(pdev->dev.of_node,
> +				     "clock-frequency", &eth_clkfreq);
> +	if (!eth_clkfreq) {
> +		struct clk *clk = clk_get(&pdev->dev, NULL);
> +
> +		if (!IS_ERR(clk)) {
> +			priv->clk = clk;
> +			clk_prepare_enable(clk);
> +			eth_clkfreq = clk_get_rate(clk);
> +		}
> +	}
> +	if (eth_clkfreq) {
> +		u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / 2500000 + 1);
> +
> +		if (!clkdiv)
> +			clkdiv = 2;
> +		dev_dbg(&pdev->dev, "setting MII clkdiv to %u\n", clkdiv);
> +		ethoc_write(priv, MIIMODER,
> +			    (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE) |
> +			    clkdiv);
> +	}

This does look a bit convoluted, and it looks like the clk_get() or 
getting the clock-frequency property should boil down to being the same 
thing with of_clk_get() as it should resolve all clocks phandles and 
fetch their frequencies appropriately.

> +
>   	/* register MII bus */
>   	priv->mdio = mdiobus_alloc();
>   	if (!priv->mdio) {
> @@ -1141,6 +1170,8 @@ free_mdio:
>   	kfree(priv->mdio->irq);
>   	mdiobus_free(priv->mdio);
>   free:
> +	if (priv->clk)
> +		clk_disable_unprepare(priv->clk);

This should probbaly be if (!IS_ERR(priv-clk)).

>   	free_netdev(netdev);
>   out:
>   	return ret;
> @@ -1165,6 +1196,8 @@ static int ethoc_remove(struct platform_device *pdev)
>   			kfree(priv->mdio->irq);
>   			mdiobus_free(priv->mdio);
>   		}
> +		if (priv->clk)
> +			clk_disable_unprepare(priv->clk);

Same here

>   		unregister_netdev(netdev);
>   		free_netdev(netdev);
>   	}
> diff --git a/include/net/ethoc.h b/include/net/ethoc.h
> index 96f3789..2a2d6bb 100644
> --- a/include/net/ethoc.h
> +++ b/include/net/ethoc.h
> @@ -16,6 +16,7 @@
>   struct ethoc_platform_data {
>   	u8 hwaddr[IFHWADDRLEN];
>   	s8 phy_id;
> +	u32 eth_clkfreq;
>   };
>
>   #endif /* !LINUX_NET_ETHOC_H */
>

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