| Message ID | 20210111125337.36513-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ks8851: Connect and start/stop the internal PHY | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: kuba@kernel.org davem@davemloft.net zhengyongjun3@huawei.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 1 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 87 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 11.01.2021 13:53, 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. > > 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(+) > > diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h > index e2eb0caeac82..ef13929036cf 100644 > --- a/drivers/net/ethernet/micrel/ks8851.h > +++ b/drivers/net/ethernet/micrel/ks8851.h > @@ -359,6 +359,7 @@ union ks8851_tx_hdr { > * @vdd_io: Optional digital power supply for IO > * @gpio: Optional reset_n gpio > * @mii_bus: Pointer to MII bus structure > + * @phy_dev: Pointer to PHY device structure > * @lock: Bus access lock callback > * @unlock: Bus access unlock callback > * @rdreg16: 16bit register read callback > @@ -405,6 +406,7 @@ struct ks8851_net { > struct regulator *vdd_io; > int gpio; > struct mii_bus *mii_bus; > + struct phy_device *phy_dev; > > void (*lock)(struct ks8851_net *ks, > unsigned long *flags); > diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c > index 058fd99bd483..a3716fd2d858 100644 > --- a/drivers/net/ethernet/micrel/ks8851_common.c > +++ b/drivers/net/ethernet/micrel/ks8851_common.c > @@ -432,6 +432,11 @@ static void ks8851_flush_tx_work(struct ks8851_net *ks) > ks->flush_tx_work(ks); > } > > +static void ks8851_handle_link_change(struct net_device *net) > +{ > + phy_print_status(net->phydev); > +} > + > /** > * ks8851_net_open - open network device > * @dev: The network device being opened. > @@ -445,11 +450,22 @@ static int ks8851_net_open(struct net_device *dev) > unsigned long flags; > int ret; > > + 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"); > + return ret; > + } > + > + 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); > return ret; > } > > @@ -507,6 +523,7 @@ static int ks8851_net_open(struct net_device *dev) > netif_dbg(ks, ifup, ks->netdev, "network device up\n"); > > ks8851_unlock(ks, &flags); > + phy_start(ks->phy_dev); > mii_check_link(&ks->mii); > return 0; > } > @@ -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) > > static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev) > { > + struct phy_device *phy_dev; > struct mii_bus *mii_bus; > int ret; > > @@ -1103,10 +1124,17 @@ static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev) > if (ret) > goto err_mdiobus_register; > > + phy_dev = phy_find_first(mii_bus); > + if (!phy_dev) > + goto err_find_phy; > + > ks->mii_bus = mii_bus; > + ks->phy_dev = phy_dev; > > return 0; > > +err_find_phy: > + mdiobus_unregister(mii_bus); > err_mdiobus_register: > mdiobus_free(mii_bus); > return ret; > 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.
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. https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8851-16MLL-Single-Port-Ethernet-MAC-Controller-with-8-Bit-or-16-Bit-Non-PCI-Interface-DS00002357B.pdf page 64 https://www.mouser.com/datasheet/2/268/ksz8851-16mll_ds-776208.pdf page 65 The later is more complete. Apparently it does support pause. > 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 ?
On 11.01.2021 14:38, Marek Vasut wrote: > 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. > > https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8851-16MLL-Single-Port-Ethernet-MAC-Controller-with-8-Bit-or-16-Bit-Non-PCI-Interface-DS00002357B.pdf > page 64 > > https://www.mouser.com/datasheet/2/268/ksz8851-16mll_ds-776208.pdf > page 65 > > The later is more complete. > > Apparently it does support pause. > >> 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.
On 1/11/21 2:50 PM, Heiner Kallweit wrote: > On 11.01.2021 14:38, Marek Vasut wrote: >> 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. >> >> https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8851-16MLL-Single-Port-Ethernet-MAC-Controller-with-8-Bit-or-16-Bit-Non-PCI-Interface-DS00002357B.pdf >> page 64 >> >> https://www.mouser.com/datasheet/2/268/ksz8851-16mll_ds-776208.pdf >> page 65 >> >> The later is more complete. >> >> Apparently it does support pause. Based on the datasheet, does it support sym or asym pause ? >>> 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.
On 11.01.2021 15:10, Marek Vasut wrote: > On 1/11/21 2:50 PM, Heiner Kallweit wrote: >> On 11.01.2021 14:38, Marek Vasut wrote: >>> 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. >>> >>> https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8851-16MLL-Single-Port-Ethernet-MAC-Controller-with-8-Bit-or-16-Bit-Non-PCI-Interface-DS00002357B.pdf >>> page 64 >>> >>> https://www.mouser.com/datasheet/2/268/ksz8851-16mll_ds-776208.pdf >>> page 65 >>> >>> The later is more complete. >>> >>> Apparently it does support pause. > > 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. >>>> 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.
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
On 1/11/21 3:47 PM, Andrew Lunn wrote: > 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. 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.
On 1/11/21 3:43 PM, Heiner Kallweit wrote: > On 11.01.2021 15:10, Marek Vasut wrote: >> On 1/11/21 2:50 PM, Heiner Kallweit wrote: >>> On 11.01.2021 14:38, Marek Vasut wrote: >>>> 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. >>>> >>>> https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8851-16MLL-Single-Port-Ethernet-MAC-Controller-with-8-Bit-or-16-Bit-Non-PCI-Interface-DS00002357B.pdf >>>> page 64 >>>> >>>> https://www.mouser.com/datasheet/2/268/ksz8851-16mll_ds-776208.pdf >>>> page 65 >>>> >>>> The later is more complete. >>>> >>>> Apparently it does support pause. >> >> 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. So I guess this patch is OK as-is ?
On Tue, Jan 12, 2021 at 11:28:00PM +0100, Marek Vasut wrote: > On 1/11/21 3:47 PM, Andrew Lunn wrote: > > 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. > > 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
On 1/14/21 2:54 PM, Andrew Lunn wrote: > On Tue, Jan 12, 2021 at 11:28:00PM +0100, Marek Vasut wrote: >> On 1/11/21 3:47 PM, Andrew Lunn wrote: >>> 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. >> >> 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 ?
> > > 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. > > 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
diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h index e2eb0caeac82..ef13929036cf 100644 --- a/drivers/net/ethernet/micrel/ks8851.h +++ b/drivers/net/ethernet/micrel/ks8851.h @@ -359,6 +359,7 @@ union ks8851_tx_hdr { * @vdd_io: Optional digital power supply for IO * @gpio: Optional reset_n gpio * @mii_bus: Pointer to MII bus structure + * @phy_dev: Pointer to PHY device structure * @lock: Bus access lock callback * @unlock: Bus access unlock callback * @rdreg16: 16bit register read callback @@ -405,6 +406,7 @@ struct ks8851_net { struct regulator *vdd_io; int gpio; struct mii_bus *mii_bus; + struct phy_device *phy_dev; void (*lock)(struct ks8851_net *ks, unsigned long *flags); diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c index 058fd99bd483..a3716fd2d858 100644 --- a/drivers/net/ethernet/micrel/ks8851_common.c +++ b/drivers/net/ethernet/micrel/ks8851_common.c @@ -432,6 +432,11 @@ static void ks8851_flush_tx_work(struct ks8851_net *ks) ks->flush_tx_work(ks); } +static void ks8851_handle_link_change(struct net_device *net) +{ + phy_print_status(net->phydev); +} + /** * ks8851_net_open - open network device * @dev: The network device being opened. @@ -445,11 +450,22 @@ static int ks8851_net_open(struct net_device *dev) unsigned long flags; int ret; + 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"); + return ret; + } + + 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); return ret; } @@ -507,6 +523,7 @@ static int ks8851_net_open(struct net_device *dev) netif_dbg(ks, ifup, ks->netdev, "network device up\n"); ks8851_unlock(ks, &flags); + phy_start(ks->phy_dev); mii_check_link(&ks->mii); return 0; } @@ -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) static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev) { + struct phy_device *phy_dev; struct mii_bus *mii_bus; int ret; @@ -1103,10 +1124,17 @@ static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev) if (ret) goto err_mdiobus_register; + phy_dev = phy_find_first(mii_bus); + if (!phy_dev) + goto err_find_phy; + ks->mii_bus = mii_bus; + ks->phy_dev = phy_dev; return 0; +err_find_phy: + mdiobus_unregister(mii_bus); err_mdiobus_register: mdiobus_free(mii_bus); return ret;
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(+)