On Tue, Jun 27, 2017 at 10:37:34AM -0700, Florian Fainelli wrote: > On 06/27/2017 10:29 AM, Maxime Ripard wrote: > > On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote: > >> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote: > >>> Hi, > >>> > >>> On 27/06/17 11:23, Icenowy Zheng wrote: > >>>> > >>>> > >>>> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <andre.przywara@xxxxxxx> 写到: > >>>>> Hi, > >>>>> > >>>>> On 27/06/17 10:41, Maxime Ripard wrote: > >>>>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> (CC:ing some people from that Rockchip dmwac series) > >>>>>>> > >>>>>>> On 27/06/17 09:21, Corentin Labbe wrote: > >>>>>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote: > >>>>>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe > >>>>>>>>> <clabbe.montjoie@xxxxxxxxx> wrote: > >>>>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote: > >>>>>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote: > >>>>>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by > >>>>>>>>>>>> allwinner. > >>>>>>>>>>>> In fact the only common part is the descriptor management and > >>>>> the first > >>>>>>>>>>>> register function. > >>>>>>>>>>> > >>>>>>>>>>> Hi, > >>>>>>>>>>> > >>>>>>>>>>> I know I am a bit late with this, but while adapting the U-Boot > >>>>> driver > >>>>>>>>>>> to the new binding I was wondering about the internal PHY > >>>>> detection: > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> So here you seem to deduce the usage of the internal PHY by the > >>>>> PHY > >>>>>>>>>>> interface specified in the DT (MII = internal, RGMII = > >>>>> external). > >>>>>>>>>>> I think I raised this question before, but isn't it perfectly > >>>>> legal for > >>>>>>>>>>> a board to use MII with an external PHY even on those SoCs that > >>>>> feature > >>>>>>>>>>> an internal PHY? > >>>>>>>>>>> On the first glance that does not make too much sense, but apart > >>>>> from > >>>>>>>>>>> not being the correct binding to describe all of the SoCs > >>>>> features I see > >>>>>>>>>>> two scenarios: > >>>>>>>>>>> 1) A board vendor might choose to not use the internal PHY > >>>>> because it > >>>>>>>>>>> has bugs, lacks features (configurability) or has other issues. > >>>>> For > >>>>>>>>>>> instance I have heard reports that the internal PHY makes the > >>>>> SoC go > >>>>>>>>>>> rather hot, possibly limiting the CPU frequency. By using an > >>>>> external > >>>>>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be > >>>>> avoided. > >>>>>>>>>>> 2) A PHY does not necessarily need to be directly connected to > >>>>>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a > >>>>> switch > >>>>>>>>>>> IC or some other network circuitry, for instance fibre > >>>>> connectors. > >>>>>>>>>>> > >>>>>>>>>>> So I was wondering if we would need an explicit: > >>>>>>>>>>> allwinner,use-internal-phy; > >>>>>>>>>>> boolean DT property to signal the usage of the internal PHY? > >>>>>>>>>>> Alternatively we could go with the negative version: > >>>>>>>>>>> allwinner,disable-internal-phy; > >>>>>>>>>>> > >>>>>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy" > >>>>> compatible > >>>>>>>>>>> string for the *PHY* node and use that? > >>>>>>>>>>> > >>>>>>>>>>> I just want to avoid that we introduce a binding that causes us > >>>>>>>>>>> headaches later. I think we can still fix this with a followup > >>>>> patch > >>>>>>>>>>> before the driver and its binding hit a release kernel. > >>>>>>>>>>> > >>>>>>>>>>> Cheers, > >>>>>>>>>>> Andre. > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I just see some patch, where "phy-mode = internal" is valid. > >>>>>>>>>> I will try to find a way to use it > >>>>>>>>> > >>>>>>>>> Can you provide a link? > >>>>>>>> > >>>>>>>> https://lkml.org/lkml/2017/6/23/479 > >>>>>>>> > >>>>>>>>> > >>>>>>>>> I'm not a fan of using phy-mode for this. There's no guarantee > >>>>> what > >>>>>>>>> mode the internal PHY uses. That's what phy-mode is for. > >>>>>>> > >>>>>>> I can understand Chen-Yu's concerns, but ... > >>>>>>> > >>>>>>>> For each soc the internal PHY mode is know and setted in > >>>>> emac_variant/internal_phy > >>>>>>>> So its not a problem. > >>>>>>> > >>>>>>> that is true as well, at least for now. > >>>>>>> > >>>>>>> So while I agree that having a separate property to indicate > >>>>>>> the usage of the internal PHY would be nice, I am bit tempted > >>>>>>> to use this easier approach and piggy back on the existing > >>>>>>> phy-mode property. > >>>>>> > >>>>>> We're trying to fix an issue that works for now too. > >>>>>> > >>>>>> If we want to consider future weird cases, then we must > >>>>>> consider all of them. And the phy mode changing is definitely > >>>>>> not really far fetched. > >>>>>> > >>>>>> I agree with Chen-Yu, and I really feel like the compatible > >>>>>> solution you suggested would cover both your concerns, and > >>>>>> ours. > >>>>> > >>>>> So something like this? > >>>>> emac: emac@1c30000 { > >>>>> compatible = "allwinner,sun8i-h3-emac"; > >>>>> ... > >>>>> phy-mode = "mii"; > >>>>> phy-handle = <&int_mii_phy>; > >>>>> ... > >>>>> > >>>>> mdio: mdio { > >>>>> #address-cells = <1>; > >>>>> #size-cells = <0>; > >>>>> int_mii_phy: ethernet-phy@1 { > >>>>> compatible = "allwinner,sun8i-h3-ephy"; > >>>>> syscon = <&syscon>; > >>>> > >>>> The MAC still needs to set some bits of syscon register. > >>> > >>> Yes, the syscon property needs also to be in the MAC node, that > >>> was meant to be somewhere in the second "..." ;-) > >>> > >>> But now since Chen-Yu mentioned that we need to set up the PHY *first* > >>> to make it actually discoverable via MDIO, I wonder if we could change > >>> this to: > >>> 1) have the DT as described here > >>> 2) Let the dwmac-sun8i driver peek into the node referenced by > >>> phy-handle and check the compatible string there. > >>> 3) If that matches some allwinner internal PHY name, it sets up the PHY > >>> to make it respond when the MDIO driver queries its bus. > >>> > >>> Or can a PHY driver set itself up (since we have clocks and resets > >>> properties in there) *before* the MDIO bus gets scanned? > >>> Chen-Yu's comment in the other mail hints at that this is not easily > >>> possible. > >> > >> I think adding phy compatible just make things more complex. > >> > >> I think the patch series I sent early fix all problems without more > >> complexity since: > >> > >> - it does not add more DT stuff > >> - it use an already used in tree DT phy-mode "internal" (and so phy > >> mode PHY_INTERFACE_MODE_INTERNAL) > > > > - it doesn't cover all the concerns we ha> - it uses an undocumented value, with an unclear implication > > No it's no longer undocumented since [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=29b65f5f97632722bb80969377e5b0e2401fb392 > > Due to the timezone difference, you guys have already managed to have > several exchanges, hopefully I will have a chance to review your > discussions a little later today. Hello I wait for your comment before sending my reverts patch for http://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1431579.html Could you confirm that internal is only meant for "non xMII internal protocol" Regards -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
