Date:   Tue, 27 Jun 2017 18:20:04 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Icenowy Zheng <icenowy@...c.io>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Andre Przywara <andre.przywara@....com>,
        Corentin Labbe <clabbe.montjoie@...il.com>,
        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: [linux-sunxi] Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

On Tue, Jun 27, 2017 at 6:17 PM, Icenowy Zheng <icenowy@...c.io> wrote:
>
>
> 于 2017年6月27日 GMT+08:00 下午6:11:47, Chen-Yu Tsai <wens@...e.org> 写到:
>>On Tue, Jun 27, 2017 at 5:41 PM, Maxime Ripard
>><maxime.ripard@...e-electrons.com> 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 guess the issue is whether it's likely that the vendor puts 2
>>internal
>>PHYs in one SoC, and they use different modes and can be switched
>>around.
>>Otherwise it's fixed for a given SoC, and we can just handle that with
>>the per-SoC GMAC compatible.
>>
>>Maybe Florian could tell us if this was one of the intended use cases
>>for the "internal" phy mode.
>>
>>As for Rockchip, AFAIK they have 2 MACs, one is connected to the
>>internal
>>PHY, while the other is connected to the external interface, and there
>>is
>>no muxing involved, unlike Allwinner's solution.
>>
>>> I agree with Chen-Yu, and I really feel like the compatible solution
>>> you suggested would cover both your concerns, and ours.
>>
>>If using a PHY compatible is the solution, we could just use the
>>"ethernet-phy-idAAAA.BBBB" style one, and put in the bogus ID that
>>Allwinner used.
>>
>>Care must be taken to put this at the board level for boards using
>>the internal PHY, or we'd have to delete or override the property
>>in all other boards.
>>
>>Ideally I think the internal PHY device node should _not_ be in
>>the SoC level .dtsi file. If we select the external interface, then
>>there's no connection to the internal PHY, and that device node becomes
>>unusable and bogus. This is something I think should be fixed
>>regardless
>>of the phy-mode discussion above.
>
> I think it should be in the SoC DTSI, as it's part of the SoC.
>
> But it makes sense to set status to disabled defaultly.

Then you end up stepping on it when you add an external PHY
with the same address, and start wondering why things don't
work.

ChenYu

Powered by blists - more mailing lists