Message-ID: <CAGb2v64jrDPSff+QL_PQBMaQJ+CcTVbskjuOUd1OqR4smbu+ig@mail.gmail.com>
Date:   Fri, 25 Aug 2017 10:54:11 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Corentin Labbe <clabbe.montjoie@...il.com>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Chen-Yu Tsai <wens@...e.org>, Andrew Lunn <andrew@...n.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@...il.com> wrote:
> On 08/24/2017 01:21 AM, Corentin Labbe wrote:
>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>>> Hi Florian,
>>>>
>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>>> with the internal PHY.
>>>>>>>>
>>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>>
>>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>>> used, then just put the external PHY as a child node there.
>>>>>>>
>>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>>
>>>>>>> Works for everyone?
>>>>>>
>>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>>> il will be merged with internal PHY node and get
>>>>>> phy-is-integrated.
>>>>>
>>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>>> external PHY and push all the internal and external PHY node definition
>>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>>
>>>> If possible, I'd really like to have the internal PHY in the
>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>>> with its clock, reset line, and whatever info we might need in the
>>>> future in each and every board DTS that uses it will be very error
>>>> prone and we will have the usual bunch of issues that come up with
>>>> duplication.
>>>
>>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>>> status = "disabled" property, and have the per-board DTS put a status =
>>> "okay" property along with a "phy-is-integrated" boolean property? Would
>>> that work?
>>
>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
>
> Is not there is a mistake in the unit address not matching the "reg"
> property, or am I not looking at the right tree?
>
> &mdio {
>         ext_rgmii_phy: ethernet-phy@1 {
>                 compatible = "ethernet-phy-ieee802.3-c22";
>                 reg = <0>;
>         };
> };
>
> If the PHY is really at MDIO address 0, then it should be
> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
> merging?

That is wrong. The board described in the example likely has a Realtek
RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address,
so it still works, but is the wrong representation.

>
>
>> So that adding a 'status = "disabled"' does not bring anything.
>>
>>>
>>> What I really don't think is necessary is:
>>>
>>> - duplicating the "mdio" controller node for external vs. internal PHY,
>>> because this is not accurate, there is just one MDIO controller, but
>>> there may be different kinds of MDIO/PHY devices attached
>>
>> For me, if we want to represent the reality, we need two MDIO:
>> - since two PHY at the same address could co-exists
>> - since they are isolated so not on the same MDIO bus
>
> Is that really true? It might be, but from experience with e.g:
> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
> bus, which is convenient, except when you have an address conflict.

There's a mux in the hardware: either the internal MDIO+MII lines
from the internal PHY are connected to the MAC, or the external
MDIO+MII lines from the pin controller are connected. I believe
this was already mentioned?

>
>>
>>> - having the STMMAC driver MDIO probing code having to deal with a
>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>>> and requiring more driver-level changes that are error prone
>>
>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
>> have to be changed to something like "register_parent_mdio"
>>
>>
>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
>> Really having two MDIO seems cleaner.
>
> The only valid thing that you have provided so far is this merging
> problem. Anything else ranging from "we will face with lots of small
> patch for adding phy-is-integrated" to "Really having two MDIO seems
> cleaner." are hard to receive as technical arguments for correctness.
>
> What happens if someone connects an external PHY at the same MDIO
> address than the internal PHY, which one do you get responses from? If
> you shutdown the internal PHY and it stops responding, then this
> probably becomes deterministic, but it still supports the fact there is
> just one MDIO bus controller per MAC.

Depends on whichever set of pins/lines are selected. But yeah, there's
only one MDIO bus controller in the MAC.

ChenYu

> Anyway, do whatever works best for you I guess.
> --
> Florian

Powered by blists - more mailing lists