[v2] lan743x: correctly handle chips with internal PHY

From: Sven Van Asbroeck <thesven73@gmail.com>

Commit 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
assumes that chips with an internal PHY will never have a devicetree
entry. This is incorrect: even for these chips, a devicetree entry
can be useful e.g. to pass the mac address from bootloader to chip:

    &pcie {
            status = "okay";

            host@0 {
                    reg = <0 0 0 0 0>;

                    #address-cells = <3>;
                    #size-cells = <2>;

                    lan7430: ethernet@0 {
                            /* LAN7430 with internal PHY */
                            compatible = "microchip,lan743x";
                            status = "okay";
                            reg = <0 0 0 0 0>;
                            /* filled in by bootloader */
                            local-mac-address = [00 00 00 00 00 00];
                    };
            };
    };

If a devicetree entry is present, the driver will not attach the chip
to its internal phy, and the chip will be non-operational.

Fix by tweaking the phy connection algorithm:
- first try to connect to a phy specified in the devicetree
  (could be 'real' phy, or just a 'fixed-link')
- if that doesn't succeed, try to connect to an internal phy, even
  if the chip has a devnode

Tested on a LAN7430 with internal PHY. I cannot test a device using
fixed-link, as I do not have access to one.

Fixes: 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
Tested-by: Sven Van Asbroeck <thesven73@gmail.com> # lan7430
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
---

v1 -> v2:
    Andrew Lunn: keep patch minimal and correct, so keep open-coded version
    of of_phy_get_and_connect().

    Jakub Kicinski: fix e-mail address case.

Tree: v5.10-rc2

To: Andrew Lunn <andrew@lunn.ch>
To: Bryan Whitehead <bryan.whitehead@microchip.com>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
Cc: Roelof Berg <rberg@berg-solutions.de>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 drivers/net/ethernet/microchip/lan743x_main.c | 21 +++++++++++--------
 drivers/net/ethernet/microchip/lan743x_main.h |  1 -
 2 files changed, 12 insertions(+), 10 deletions(-)

On Fri, Nov 06, 2020 at 08:43:24AM -0500, Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <thesven73@gmail.com>
> 
> Commit 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
> assumes that chips with an internal PHY will never have a devicetree
> entry. This is incorrect: even for these chips, a devicetree entry
> can be useful e.g. to pass the mac address from bootloader to chip:
> 
>     &pcie {
>             status = "okay";
> 
>             host@0 {
>                     reg = <0 0 0 0 0>;
> 
>                     #address-cells = <3>;
>                     #size-cells = <2>;
> 
>                     lan7430: ethernet@0 {
>                             /* LAN7430 with internal PHY */
>                             compatible = "microchip,lan743x";
>                             status = "okay";
>                             reg = <0 0 0 0 0>;
>                             /* filled in by bootloader */
>                             local-mac-address = [00 00 00 00 00 00];
>                     };
>             };
>     };
> 
> If a devicetree entry is present, the driver will not attach the chip
> to its internal phy, and the chip will be non-operational.
> 
> Fix by tweaking the phy connection algorithm:
> - first try to connect to a phy specified in the devicetree
>   (could be 'real' phy, or just a 'fixed-link')
> - if that doesn't succeed, try to connect to an internal phy, even
>   if the chip has a devnode
> 
> Tested on a LAN7430 with internal PHY. I cannot test a device using
> fixed-link, as I do not have access to one.
> 
> Fixes: 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
> Tested-by: Sven Van Asbroeck <thesven73@gmail.com> # lan7430
> Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
> ---
> 
> v1 -> v2:
>     Andrew Lunn: keep patch minimal and correct, so keep open-coded version
>     of of_phy_get_and_connect().

Hi Sven

Why is it required to remove adapter->phy_mode? It is not clear to me
what this change has to do with not looking for an internal PHY.

> @@ -1063,6 +1065,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
>  
>  	phy_start(phydev);
>  	phy_start_aneg(phydev);
> +	phy_attached_info(phydev);

This also has nothing to do with the problem you are fixing. It is a
sensible thing to do, but it should be a separate patch, and target
net-next, since it is not a fix.

	  Andrew

My last customer has a fixed link setup, contact me if you need one. Maybe they let me have it and I can test all future patches on fixed link as well. I tested the current driver version on a Lan7430 EVM in a PC by the way and it seemed to be working. I have the EVM still here if needed.

Greetings,
Roelof

On Sat, Nov 7, 2020 at 11:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> This also has nothing to do with the problem you are fixing. It is a
> sensible thing to do, but it should be a separate patch, and target
> net-next, since it is not a fix.
>

Agreed - I will spin a truly minimal v3.

Hi Roelof, great to meet another user of the lan743x !

On Sun, Nov 8, 2020 at 2:43 AM Roelof Berg <rberg@berg-solutions.de> wrote:
>
> My last customer has a fixed link setup, contact me if you need one. Maybe they let me have it and I can test all future patches on fixed link as well.

Is this a commercially available product? I could test this out if it
can be ordered
inexpensively from my location (Canada).

> I tested the current driver version on a Lan7430 EVM in a PC by the way and it seemed to be working. I have the EVM still here if needed.

Yes it will work on a PC, because the device doesn't have a devicetree
entry there.
In my case I am running on ARM, and I'm manually creating a devicetree
entry in the fdt, so that the bootloader can assign a mac address to the chip.

I must be one of the first to use this chip on ARM, because I have
encountered 2 or 3 issues with the current driver, which crash the
kernel on ARM.

I'll try to contribute those changes once this fix is accepted and merged.

Sven

Hi Roelof,

On Sun, Nov 8, 2020 at 2:57 PM Roelof Berg <rberg@berg-solutions.de> wrote:
>
> Well, it’s not an easy 4 hours attempt between breakfast and lunch, unfortunately, but it’s based on inexpensive off-the-shelf parts and doable in an experienced team.
>

I would love to test this patch on fixed-link, but unfortunately I
don't have the
resources to create a prototype as per your instructions.

Sven

.
> I would love to test this patch on fixed-link, but unfortunately

Yes, understandable, it’s quite dome effort. I will e-mail the company that owns the test setup. They have their own pcba‘s ready meanwhile, so maybe they can give me the EVM setup and I can use it for testing your and other patches.