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: <515F359D.7000306@gmail.com>
Date:	Fri, 05 Apr 2013 22:35:41 +0200
From:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
CC:	Florian Fainelli <florian@...nwrt.org>,
	thomas.petazzoni@...e-electrons.com, moinejf@...e.fr,
	Simon Baatz <gmbnomis@...il.com>, andrew@...n.ch,
	netdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	rob.herring@...xeda.com, grant.likely@...retlab.ca,
	jogo@...nwrt.org, linux-arm-kernel@...ts.infradead.org,
	jm@...tin.co.uk, davem@...emloft.net, buytenh@...tstofly.org,
	jason@...edaemon.net
Subject: Re: [PATCH 1/5 v2] mv643xx_eth: add Device Tree bindings

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