Message-ID: <450d262e-242c-77f1-9f06-e25943cc595c@gmail.com>
Date:   Fri, 23 Oct 2020 11:21:52 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Cc:     Sekhar Nori <nsekhar@...com>, linux-kernel@...r.kernel.org,
        Vignesh Raghavendra <vigneshr@...com>,
        Roger Quadros <rogerq@...com>,
        Russell King <linux@...linux.org.uk>,
        Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Subject: Re: [PATCH] RFC: net: phy: of phys probe/reset issue

On 10/23/20 10:47 AM, Grygorii Strashko wrote:
> Hi All,
> 
> The main intention of this mail is to trigger discussion to find a proper
> solution. All code is hackish and based on v5.9.
> 
> Problem statement:
> 
> There is an issue observed with MDIO OF PHYs discover/reset sequence in
> case PHY has reset line with default state is (1).
> In this case, when Linux boots PHY is in reset and following code fails:
> 
> of_mdiobus_register()
> |- for_each_available_child_of_node(np, child)
>    |- of_mdiobus_register_phy
>       |- get_phy_device
>          |- get_phy_c22_id ---- > *fails as PHY is in reset*
>       ...
>       |- of_mdiobus_phy_device_register() --> can't be reached
> 
> The current PHY allows to specify PHY reset line for PHY:
> &mdio {
>         phy0: ethernet-phy@0 {
>                 reg = <0>;
>                 ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>                 ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> +               reset-gpios = <&pca9555 4 GPIO_ACTIVE_LOW>;
>                 ti,dp83867-rxctrl-strap-quirk;
>         };
>  };
> 
> But it doesn't help in this case, as PHY's reset code is initialized when
> PHY's mdio_device is registered, which, in turn, happens after
> get_phy_device() call (and get_phy_device() is failed).
> 
> of_mdiobus_phy_device_register()
>  |-phy_device_register
>    |-mdiobus_register_device()
>      |-mdiobus_register_gpiod(mdiodev);
>      |-mdiobus_register_reset(mdiodev);
> 
> There is also possibility to add GPIO reset line to MDIO node itself, but
> It also doesn't help when there are >1 PHY and every PHY has it own reset
> line.
> 
> Only one possible W/A now is to use GPIO HOG, with drawback that PHY will
> stay active always.
> 
> Some history:
> 
> - commit 69226896ad63 ("mdio_bus: Issue GPIO RESET to PHYs") from Roger
> Quadros <rogerq@...com> originally added possibility to specify >1 GPIO
> reset line in MDIO node, which allowed to solve such issues.
>  - commit 4c5e7a2c0501 ("dt-bindings: mdio: Clarify binding document") and
> follow up commit d396e84c5604 ("mdio_bus: handle only single PHY reset
> GPIO") rolled back original solution to only one GPIO reset line, which
> causes problems now.
> 
> Possible solutions I come up with:
>  1) Try to add PHY reset code around get_phy_device() call in of_mdiobus_register_phy()
>   cons:
>    - need to extract/share mdio_device reset code as PHY may have not only GPIO,
>      but also reset_control object assigned.
>      And all current mdio_device rest code expected to have mdio_device already initialized.
>    - There 12 calls to get_phy_device() in v5.9 Kernel
>  2) Try to consolidate OF mdio_device/PHY initialization in one place, as
> illustrated by of_phy_device_create() function (marked by "// option 2" in
> code).
>  3) Return back possibility to use >1 GPIO reset line in MDIO node. Even if
> It seems right thing to do by itself (Devices attached to MDIO bus may have
> any combination of shared reset lines - not always "one for all"), there
> are more things to consider:
>    - PHY reset_control objects handling
>    - the fact that MDIO reset will put PHY out of reset and not allow to
>      reset it again (more like gpio-hog)
> 
> I'd be appreciated for any comments to help resolve it. May be there is
> better way to handle this?

Yes there is: have your Ethernet PHY compatible string be of the form
"ethernetAAAA.BBBB" and then there is no need for such hacking.
of_get_phy_id() will parse that compatible and that will trigger
of_mdiobus_register_phy() to take the phy_device_create() path.

The other advantage I see to the explicit PHY ID in compatible string
approach is that it is easier to scale to other bus subsystems that have
similar requirements like PCI, USB, SPI, I2C etc.

Your solution is not inelegant in that you took care of parsing the
Ethernet PHY (child) device_node, however I see two problems with it:

- it deals only with reset, but there are other essential resources such
as clocks and regulators that would need an equivalent treatment. While
the CLK API has support for device_node references, the regulator API
does not AFAICT.

0 in a world where we move towards supporting ACPI (there is work in
progress on this). We would need a solution that scales to both
"firmware" implementations and using APIs that expect a device_node
reference does not really make that possible. You could argue that the
solution I am offering is currently OF centric and that is true, however
it is easily extensible to ACPI as well (if the ACPI folks and kernel
developers manage to settle on what an appropriate representation for
MDIO/PHY/SFPs looks like).

Since I am officially no longer a PHY library maintainer, you would need
someone more authoritative to weigh in the approach you have taken.
-- 
Florian

Powered by blists - more mailing lists