[PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Marek Vasut]

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;
-- 
2.29.2

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Heiner Kallweit]

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.

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Marek Vasut]

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 ?

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Heiner Kallweit]

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.

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Marek Vasut]

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.

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Heiner Kallweit]

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.

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Marek Vasut]

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 ?

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Andrew Lunn]

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

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Marek Vasut]

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.

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Andrew Lunn]

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

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Marek Vasut]

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 ?

Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

[Andrew Lunn]

> > > 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