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

[Andre Przywara]

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>;
                    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.