Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Icenowy Zheng <icenowy@aosc.io>]

于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <andre.przywara@arm.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@gmail.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@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.

>                    reg = <1>;
>                    clocks = <&ccu CLK_BUS_EPHY>;
>                    resets = <&ccu RST_BUS_EPHY>;
>                };
>            };
>        };
>
>And then move the internal-PHY setup code into a separate PHY driver?
>
>That looks like the architecturally best solution to me, but is
>probably
>also a bit involved since it would require a separate PHY driver.
>Or can we make it simpler, but still use this binding?
>
>Cheers,
>Andre.