From: Sven Van Asbroeck <[email protected]>
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 <[email protected]> # lan7430
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
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 <[email protected]>
To: Bryan Whitehead <[email protected]>
To: "David S. Miller" <[email protected]>
To: Jakub Kicinski <[email protected]>
Cc: Microchip Linux Driver Support <[email protected]>
Cc: Roelof Berg <[email protected]>
Cc: [email protected]
Cc: [email protected]
drivers/net/ethernet/microchip/lan743x_main.c | 21 +++++++++++--------
drivers/net/ethernet/microchip/lan743x_main.h | 1 -
2 files changed, 12 insertions(+), 10 deletions(-)
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;
--
2.17.1
On Fri, Nov 06, 2020 at 08:43:24AM -0500, Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <[email protected]>
>
> 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 <[email protected]> # lan7430
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
>
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.