Date:   Tue, 27 Jun 2017 19:29:37 +0200
From:   Maxime Ripard <maxime.ripard@...e-electrons.com>
To:     Corentin Labbe <clabbe.montjoie@...il.com>
Cc:     Andre Przywara <andre.przywara@....com>,
        Icenowy Zheng <icenowy@...c.io>, Chen-Yu Tsai <wens@...e.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        alexandre.torgue@...com, devicetree <devicetree@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-sunxi <linux-sunxi@...glegroups.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        david.wu@...k-chips.com, Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiko Stübner <heiko@...ech.de>
Subject: Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

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@....com> 写到:
> > >> 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@...il.com> 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@...0000 {
> > >> 	    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 had
  - it uses an undocumented value, with an unclear implication

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists