From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corentin Labbe Subject: Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i Date: Sat, 1 Jul 2017 08:53:58 +0200 Message-ID: <20170701065358.GC23363@Red> References: <20170627082103.GB2468@Red> <530f7b3e-247d-2e4d-8174-ce6195bacf5b@arm.com> <20170627094111.supxmzf2k3n3ewec@flea.lan> <44afb371-44d4-dbb5-0aac-4bbc55269344@arm.com> <700A8221-3CEC-4657-900D-183398D25AE7@aosc.io> <858b5361-fde5-8ab4-b9ba-aeb7859b9a7d@arm.com> <20170627123748.GA8403@Red> <20170627172937.qrbcie347wlbo3z3@flea> <42c51c03-2247-2d75-8534-51a96a92875c@gmail.com> Reply-To: clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: Maxime Ripard , Andre Przywara , Icenowy Zheng , Chen-Yu Tsai , Rob Herring , Mark Rutland , Russell King , Catalin Marinas , Will Deacon , Giuseppe Cavallaro , alexandre.torgue-qxv4g6HH51o@public.gmane.org, devicetree , netdev , linux-kernel , linux-sunxi , linux-arm-kernel , david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org, Andrew Lunn , Heiko =?iso-8859-1?Q?St=FCbner?= To: Florian Fainelli Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Content-Disposition: inline In-Reply-To: <42c51c03-2247-2d75-8534-51a96a92875c-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , List-Id: netdev.vger.kernel.org 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: > >>>> > >>>> > >>>> =E4=BA=8E 2017=E5=B9=B46=E6=9C=8827=E6=97=A5 GMT+08:00 =E4=B8=8B=E5= =8D=886:15:58, Andre Przywara =E5=86=99=E5=88=B0: > >>>>> 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 > >>>>>>>>> wrote: > >>>>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, Andr=C3=A9 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 an= d > >>>>> the first > >>>>>>>>>>>> register function. > >>>>>>>>>>> > >>>>>>>>>>> Hi, > >>>>>>>>>>> > >>>>>>>>>>> I know I am a bit late with this, but while adapting the U-Bo= ot > >>>>> 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 t= he > >>>>> PHY > >>>>>>>>>>> interface specified in the DT (MII =3D internal, RGMII =3D > >>>>> 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 th= at > >>>>> feature > >>>>>>>>>>> an internal PHY? > >>>>>>>>>>> On the first glance that does not make too much sense, but ap= art > >>>>> 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 issue= s. > >>>>> 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 t= o > >>>>>>>>>>> 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 followu= p > >>>>> patch > >>>>>>>>>>> before the driver and its binding hit a release kernel. > >>>>>>>>>>> > >>>>>>>>>>> Cheers, > >>>>>>>>>>> Andre. > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I just see some patch, where "phy-mode =3D 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 =3D "allwinner,sun8i-h3-emac"; > >>>>> ... > >>>>> phy-mode =3D "mii"; > >>>>> phy-handle =3D <&int_mii_phy>; > >>>>> ... > >>>>> > >>>>> mdio: mdio { > >>>>> #address-cells =3D <1>; > >>>>> #size-cells =3D <0>; > >>>>> int_mii_phy: ethernet-phy@1 { > >>>>> compatible =3D "allwinner,sun8i-h3-ephy"; > >>>>> syscon =3D <&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 chang= e > >>> 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 P= HY > >>> 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) > >=20 > > - it doesn't cover all the concerns we ha> - it uses an undocumente= d value, with an unclear implication >=20 > No it's no longer undocumented since [1] >=20 > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit= /?id=3D29b65f5f97632722bb80969377e5b0e2401fb392 >=20 > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg1431579.html Could you confirm that internal is only meant for "non xMII internal protoc= ol" Regards --=20 You received this message because you are subscribed to the Google Groups "= linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout.