Message ID | 20201106134324.20656-1-TheSven73@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [v2] lan743x: correctly handle chips with internal PHY | expand |
Context | Check | Description |
---|---|---|
jkicinski/cover_letter | success | Link |
jkicinski/fixes_present | success | Link |
jkicinski/patch_count | success | Link |
jkicinski/tree_selection | success | Guessed tree name to be net-next |
jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
jkicinski/source_inline | success | Was 0 now: 0 |
jkicinski/verify_signedoff | success | Link |
jkicinski/module_param | success | Was 0 now: 0 |
jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/verify_fixes | success | Link |
jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 67 lines checked |
jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/header_inline | success | Link |
jkicinski/stable | success | Stable not CCed |
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.
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index f2d13e8d20f0..65e80d41c9bc 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -957,7 +957,7 @@ static void lan743x_phy_link_status_change(struct net_device *netdev) data = lan743x_csr_read(adapter, MAC_CR); /* set interface mode */ - if (phy_interface_mode_is_rgmii(adapter->phy_mode)) + if (phy_interface_is_rgmii(phydev)) /* RGMII */ data &= ~MAC_CR_MII_EN_; else @@ -1014,17 +1014,18 @@ static void lan743x_phy_close(struct lan743x_adapter *adapter) static int lan743x_phy_open(struct lan743x_adapter *adapter) { struct lan743x_phy *phy = &adapter->phy; + struct phy_device *phydev = NULL; struct device_node *phynode; - struct phy_device *phydev; struct net_device *netdev; + phy_interface_t phy_mode; int ret = -EIO; netdev = adapter->netdev; phynode = of_node_get(adapter->pdev->dev.of_node); - adapter->phy_mode = PHY_INTERFACE_MODE_GMII; if (phynode) { - of_get_phy_mode(phynode, &adapter->phy_mode); + /* try devicetree phy, or fixed link */ + of_get_phy_mode(phynode, &phy_mode); if (of_phy_is_fixed_link(phynode)) { ret = of_phy_register_fixed_link(phynode); @@ -1037,18 +1038,19 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter) } phydev = of_phy_connect(netdev, phynode, lan743x_phy_link_status_change, 0, - adapter->phy_mode); + phy_mode); of_node_put(phynode); - if (!phydev) - goto return_error; - } else { + } + + if (!phydev) { + /* try internal phy */ phydev = phy_find_first(adapter->mdiobus); if (!phydev) goto return_error; ret = phy_connect_direct(netdev, phydev, lan743x_phy_link_status_change, - adapter->phy_mode); + PHY_INTERFACE_MODE_GMII); if (ret) goto return_error; } @@ -1063,6 +1065,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter) phy_start(phydev); phy_start_aneg(phydev); + phy_attached_info(phydev); return 0; return_error: diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h index a536f4a4994d..3a0e70daa88f 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.h +++ b/drivers/net/ethernet/microchip/lan743x_main.h @@ -703,7 +703,6 @@ struct lan743x_rx { struct lan743x_adapter { struct net_device *netdev; struct mii_bus *mdiobus; - phy_interface_t phy_mode; int msg_enable; #ifdef CONFIG_PM u32 wolopts;