netdev - Re: [PATCH 1/5 v2] mv643xx_eth: add Device Tree bindings

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <515E9FE0.8050402@openwrt.org>
Date:	Fri, 05 Apr 2013 11:56:48 +0200
From:	Florian Fainelli <florian@...nwrt.org>
To:	Simon Baatz <gmbnomis@...il.com>
CC:	davem@...emloft.net, thomas.petazzoni@...e-electrons.com,
	moinejf@...e.fr, jason@...edaemon.net, andrew@...n.ch,
	netdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	rob.herring@...xeda.com, grant.likely@...retlab.ca,
	jogo@...nwrt.org, buytenh@...tstofly.org, jm@...tin.co.uk,
	linux-arm-kernel@...ts.infradead.org,
	sebastian.hesselbarth@...il.com
Subject: Re: [PATCH 1/5 v2] mv643xx_eth: add Device Tree bindings

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html