From: Marek Vasut <marex@denx.de> Date: 2021-01-11 12:54:57
Unless the internal PHY is connected and started, the phylib will not
poll the PHY for state and produce state updates. Connect the PHY and
start/stop it.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Lukas Wunner <lukas@wunner.de>
---
drivers/net/ethernet/micrel/ks8851.h | 2 ++
drivers/net/ethernet/micrel/ks8851_common.c | 28 +++++++++++++++++++++
2 files changed, 30 insertions(+)
@@ -445,11 +450,22 @@ static int ks8851_net_open(struct net_device *dev)unsignedlongflags;intret;+ret=phy_connect_direct(ks->netdev,ks->phy_dev,+&ks8851_handle_link_change,+PHY_INTERFACE_MODE_INTERNAL);+if(ret){+netdev_err(dev,"failed to attach PHY\n");+returnret;+}++phy_attached_info(ks->phy_dev);+ret=request_threaded_irq(dev->irq,NULL,ks8851_irq,IRQF_TRIGGER_LOW|IRQF_ONESHOT,dev->name,ks);if(ret<0){netdev_err(dev,"failed to get irq\n");+phy_disconnect(ks->phy_dev);returnret;}
@@ -528,6 +545,9 @@ static int ks8851_net_stop(struct net_device *dev)netif_stop_queue(dev);+phy_stop(ks->phy_dev);+phy_disconnect(ks->phy_dev);+ks8851_lock(ks,&flags);/* turn off the IRQs and ack any outstanding */ks8851_wrreg16(ks,KS_IER,0x0000);
@@ -1084,6 +1104,7 @@ int ks8851_resume(struct device *dev)staticintks8851_register_mdiobus(structks8851_net*ks,structdevice*dev){+structphy_device*phy_dev;structmii_bus*mii_bus;intret;
Unless the internal PHY is connected and started, the phylib will not
poll the PHY for state and produce state updates. Connect the PHY and
start/stop it.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Lukas Wunner <lukas@wunner.de>
---
drivers/net/ethernet/micrel/ks8851.h | 2 ++
drivers/net/ethernet/micrel/ks8851_common.c | 28 +++++++++++++++++++++
2 files changed, 30 insertions(+)
@@ -445,11 +450,22 @@ static int ks8851_net_open(struct net_device *dev)unsignedlongflags;intret;+ret=phy_connect_direct(ks->netdev,ks->phy_dev,+&ks8851_handle_link_change,+PHY_INTERFACE_MODE_INTERNAL);+if(ret){+netdev_err(dev,"failed to attach PHY\n");+returnret;+}++phy_attached_info(ks->phy_dev);+ret=request_threaded_irq(dev->irq,NULL,ks8851_irq,IRQF_TRIGGER_LOW|IRQF_ONESHOT,dev->name,ks);if(ret<0){netdev_err(dev,"failed to get irq\n");+phy_disconnect(ks->phy_dev);returnret;}
@@ -528,6 +545,9 @@ static int ks8851_net_stop(struct net_device *dev)netif_stop_queue(dev);+phy_stop(ks->phy_dev);+phy_disconnect(ks->phy_dev);+ks8851_lock(ks,&flags);/* turn off the IRQs and ack any outstanding */ks8851_wrreg16(ks,KS_IER,0x0000);
@@ -1084,6 +1104,7 @@ int ks8851_resume(struct device *dev)staticintks8851_register_mdiobus(structks8851_net*ks,structdevice*dev){+structphy_device*phy_dev;structmii_bus*mii_bus;intret;
LGTM. When having a brief look at the driver I stumbled across two things:
1. Do MAC/PHY support any pause mode? Then a call to
phy_support_(a)sym_pause() would be missing.
2. Don't have the datasheet, but IRQ_LCI seems to be the link change
interrupt. So far it's ignored by the driver. You could configure
it and use phy_mac_interrupt() to operate the internal PHY in
interrupt mode.
From: Marek Vasut <marex@denx.de> Date: 2021-01-11 13:40:48
On 1/11/21 2:26 PM, Heiner Kallweit wrote:
[...]
LGTM. When having a brief look at the driver I stumbled across two things:
1. Do MAC/PHY support any pause mode? Then a call to
phy_support_(a)sym_pause() would be missing.
2. Don't have the datasheet, but IRQ_LCI seems to be the link change
interrupt. So far it's ignored by the driver. You could configure
it and use phy_mac_interrupt() to operate the internal PHY in
interrupt mode.
That's only for link state change, shouldn't the PHY interrupt trigger
on other things as well ?
LGTM. When having a brief look at the driver I stumbled across two things:
1. Do MAC/PHY support any pause mode? Then a call to
phy_support_(a)sym_pause() would be missing.
2. Don't have the datasheet, but IRQ_LCI seems to be the link change
interrupt. So far it's ignored by the driver. You could configure
it and use phy_mac_interrupt() to operate the internal PHY in
interrupt mode.
That's only for link state change, shouldn't the PHY interrupt trigger on other things as well ?
No, it's sufficient if the interrupt can signal link state change.
In r8169 I have exactly that case.
From: Marek Vasut <marex@denx.de> Date: 2021-01-11 14:11:20
On 1/11/21 2:50 PM, Heiner Kallweit wrote:
On 11.01.2021 14:38, Marek Vasut wrote:
quoted
On 1/11/21 2:26 PM, Heiner Kallweit wrote:
[...]
quoted
LGTM. When having a brief look at the driver I stumbled across two things:
1. Do MAC/PHY support any pause mode? Then a call to
phy_support_(a)sym_pause() would be missing.
Based on the datasheet, does it support sym or asym pause ?
quoted
quoted
2. Don't have the datasheet, but IRQ_LCI seems to be the link change
interrupt. So far it's ignored by the driver. You could configure
it and use phy_mac_interrupt() to operate the internal PHY in
interrupt mode.
That's only for link state change, shouldn't the PHY interrupt trigger on other things as well ?
No, it's sufficient if the interrupt can signal link state change.
In r8169 I have exactly that case.
I'll do that in a subsequent patch, once I verify it works as it should.
LGTM. When having a brief look at the driver I stumbled across two things:
1. Do MAC/PHY support any pause mode? Then a call to
phy_support_(a)sym_pause() would be missing.
Based on the datasheet, does it support sym or asym pause ?
According to the description of flow control on p.23 it can support asym pause.
However on the MAC side flow control doesn't seem to be always active, it's
controlled by these two bits:
p.49, TXCR, bit 3
p.50, RXCR1, bit 10
Default seems to be that flow control is disabled.
quoted
quoted
quoted
2. Don't have the datasheet, but IRQ_LCI seems to be the link change
interrupt. So far it's ignored by the driver. You could configure
it and use phy_mac_interrupt() to operate the internal PHY in
interrupt mode.
That's only for link state change, shouldn't the PHY interrupt trigger on other things as well ?
No, it's sufficient if the interrupt can signal link state change.
In r8169 I have exactly that case.
I'll do that in a subsequent patch, once I verify it works as it should.
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-11 14:48:35
On Mon, Jan 11, 2021 at 01:53:37PM +0100, Marek Vasut wrote:
Unless the internal PHY is connected and started, the phylib will not
poll the PHY for state and produce state updates. Connect the PHY and
start/stop it.
Hi Marek
Please continue the conversion and remove all mii_calls.
ks8851_set_link_ksettings() calling mii_ethtool_set_link_ksettings()
is not good, phylib will not know about changes which we made to the
PHY etc.
Andrew
From: Marek Vasut <marex@denx.de> Date: 2021-01-12 22:29:46
On 1/11/21 3:43 PM, Heiner Kallweit wrote:
On 11.01.2021 15:10, Marek Vasut wrote:
quoted
On 1/11/21 2:50 PM, Heiner Kallweit wrote:
quoted
On 11.01.2021 14:38, Marek Vasut wrote:
quoted
On 1/11/21 2:26 PM, Heiner Kallweit wrote:
[...]
quoted
LGTM. When having a brief look at the driver I stumbled across two things:
1. Do MAC/PHY support any pause mode? Then a call to
phy_support_(a)sym_pause() would be missing.
Based on the datasheet, does it support sym or asym pause ?
According to the description of flow control on p.23 it can support asym pause.
However on the MAC side flow control doesn't seem to be always active, it's
controlled by these two bits:
p.49, TXCR, bit 3
p.50, RXCR1, bit 10
Default seems to be that flow control is disabled.
From: Marek Vasut <marex@denx.de> Date: 2021-01-12 22:30:18
On 1/11/21 3:47 PM, Andrew Lunn wrote:
On Mon, Jan 11, 2021 at 01:53:37PM +0100, Marek Vasut wrote:
quoted
Unless the internal PHY is connected and started, the phylib will not
poll the PHY for state and produce state updates. Connect the PHY and
start/stop it.
Hi Marek
Please continue the conversion and remove all mii_calls.
ks8851_set_link_ksettings() calling mii_ethtool_set_link_ksettings()
is not good, phylib will not know about changes which we made to the
PHY etc.
Hi,
I noticed a couple of drivers implement both the mii and mdiobus
options, I was pondering why is that. Is there some legacy backward
compatibility reason for keeping both or is it safe to remove the mii
support completely from the driver?
Either way, I will do that in a separate patch, so it could be reverted
if it breaks something.
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-14 13:54:55
On Tue, Jan 12, 2021 at 11:28:00PM +0100, Marek Vasut wrote:
On 1/11/21 3:47 PM, Andrew Lunn wrote:
quoted
On Mon, Jan 11, 2021 at 01:53:37PM +0100, Marek Vasut wrote:
quoted
Unless the internal PHY is connected and started, the phylib will not
poll the PHY for state and produce state updates. Connect the PHY and
start/stop it.
Hi Marek
Please continue the conversion and remove all mii_calls.
ks8851_set_link_ksettings() calling mii_ethtool_set_link_ksettings()
is not good, phylib will not know about changes which we made to the
PHY etc.
Hi,
I noticed a couple of drivers implement both the mii and mdiobus options.
Which ones?
Simply getting the link status might be safe, but if
set_link_ksettings() or get_link_ksettings() is used, phylib is going
to get confused when the PHY is changed without it knowing.. So please
do remove all the mii calls as part of the patchset.
Andrew
From: Marek Vasut <marex@denx.de> Date: 2021-01-15 12:47:06
On 1/14/21 2:54 PM, Andrew Lunn wrote:
On Tue, Jan 12, 2021 at 11:28:00PM +0100, Marek Vasut wrote:
quoted
On 1/11/21 3:47 PM, Andrew Lunn wrote:
quoted
On Mon, Jan 11, 2021 at 01:53:37PM +0100, Marek Vasut wrote:
quoted
Unless the internal PHY is connected and started, the phylib will not
poll the PHY for state and produce state updates. Connect the PHY and
start/stop it.
Hi Marek
Please continue the conversion and remove all mii_calls.
ks8851_set_link_ksettings() calling mii_ethtool_set_link_ksettings()
is not good, phylib will not know about changes which we made to the
PHY etc.
Hi,
I noticed a couple of drivers implement both the mii and mdiobus options.
Which ones?
boardcom b44.c and bcm63xx_enet.c for example
Simply getting the link status might be safe, but if
set_link_ksettings() or get_link_ksettings() is used, phylib is going
to get confused when the PHY is changed without it knowing.. So please
do remove all the mii calls as part of the patchset.
Isn't that gonna break some ABI ?
Also, is separate patch OK ?
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-15 14:55:50
quoted
quoted
I noticed a couple of drivers implement both the mii and mdiobus options.
Which ones?
boardcom b44.c and bcm63xx_enet.c for example
Thanks. I will take a look at those and maybe ask Florian.
quoted
Simply getting the link status might be safe, but if
set_link_ksettings() or get_link_ksettings() is used, phylib is going
to get confused when the PHY is changed without it knowing.. So please
do remove all the mii calls as part of the patchset.
Isn't that gonna break some ABI ?
I guess not, but i have no definitive answer. It should add more
features, not take any away.
Also, is separate patch OK ?
It obviously works well enough that you have not run into issues. So i
guess anybody doing a git bisect will be O.K. if they land on the
state with both phydev and mii. So yes, a separate patch is O.K.
Andrew