All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
@ 2021-01-11 12:53 Marek Vasut
  2021-01-11 13:26 ` Heiner Kallweit
  2021-01-11 14:47 ` Andrew Lunn
  0 siblings, 2 replies; 13+ messages in thread
From: Marek Vasut @ 2021-01-11 12:53 UTC (permalink / raw)
  To: netdev; +Cc: Marek Vasut, Andrew Lunn, Heiner Kallweit, Lukas Wunner

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 12:53 [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY Marek Vasut
@ 2021-01-11 13:26 ` Heiner Kallweit
  2021-01-11 13:38   ` Marek Vasut
  2021-01-11 14:47 ` Andrew Lunn
  1 sibling, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2021-01-11 13:26 UTC (permalink / raw)
  To: Marek Vasut, netdev; +Cc: Andrew Lunn, Lukas Wunner

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 13:26 ` Heiner Kallweit
@ 2021-01-11 13:38   ` Marek Vasut
  2021-01-11 13:50     ` Heiner Kallweit
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2021-01-11 13:38 UTC (permalink / raw)
  To: Heiner Kallweit, netdev; +Cc: Andrew Lunn, Lukas Wunner

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 ?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 13:38   ` Marek Vasut
@ 2021-01-11 13:50     ` Heiner Kallweit
  2021-01-11 14:10       ` Marek Vasut
  0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2021-01-11 13:50 UTC (permalink / raw)
  To: Marek Vasut, netdev; +Cc: Andrew Lunn, Lukas Wunner

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 13:50     ` Heiner Kallweit
@ 2021-01-11 14:10       ` Marek Vasut
  2021-01-11 14:43         ` Heiner Kallweit
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2021-01-11 14:10 UTC (permalink / raw)
  To: Heiner Kallweit, netdev; +Cc: Andrew Lunn, Lukas Wunner

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 14:10       ` Marek Vasut
@ 2021-01-11 14:43         ` Heiner Kallweit
  2021-01-12 22:28           ` Marek Vasut
  0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2021-01-11 14:43 UTC (permalink / raw)
  To: Marek Vasut, netdev; +Cc: Andrew Lunn, Lukas Wunner

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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 12:53 [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY Marek Vasut
  2021-01-11 13:26 ` Heiner Kallweit
@ 2021-01-11 14:47 ` Andrew Lunn
  2021-01-12 22:28   ` Marek Vasut
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2021-01-11 14:47 UTC (permalink / raw)
  To: Marek Vasut; +Cc: netdev, Heiner Kallweit, Lukas Wunner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 14:47 ` Andrew Lunn
@ 2021-01-12 22:28   ` Marek Vasut
  2021-01-14 13:54     ` Andrew Lunn
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2021-01-12 22:28 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev, Heiner Kallweit, Lukas Wunner

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-11 14:43         ` Heiner Kallweit
@ 2021-01-12 22:28           ` Marek Vasut
  0 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2021-01-12 22:28 UTC (permalink / raw)
  To: Heiner Kallweit, netdev; +Cc: Andrew Lunn, Lukas Wunner

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 ?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-12 22:28   ` Marek Vasut
@ 2021-01-14 13:54     ` Andrew Lunn
  2021-01-15 12:45       ` Marek Vasut
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2021-01-14 13:54 UTC (permalink / raw)
  To: Marek Vasut; +Cc: netdev, Heiner Kallweit, Lukas Wunner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-14 13:54     ` Andrew Lunn
@ 2021-01-15 12:45       ` Marek Vasut
  2021-01-15 14:55         ` Andrew Lunn
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2021-01-15 12:45 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev, Heiner Kallweit, Lukas Wunner

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 ?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
  2021-01-15 12:45       ` Marek Vasut
@ 2021-01-15 14:55         ` Andrew Lunn
  0 siblings, 0 replies; 13+ messages in thread
From: Andrew Lunn @ 2021-01-15 14:55 UTC (permalink / raw)
  To: Marek Vasut; +Cc: netdev, Heiner Kallweit, Lukas Wunner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY
@ 2021-01-13  6:49 kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-01-13  6:49 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 5991 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210111125337.36513-1-marex@denx.de>
References: <20210111125337.36513-1-marex@denx.de>
TO: Marek Vasut <marex@denx.de>
TO: netdev(a)vger.kernel.org
CC: Marek Vasut <marex@denx.de>
CC: Andrew Lunn <andrew@lunn.ch>
CC: Heiner Kallweit <hkallweit1@gmail.com>
CC: Lukas Wunner <lukas@wunner.de>

Hi Marek,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/net-ks8851-Connect-and-start-stop-the-internal-PHY/20210111-210055
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 73b7a6047971aa6ce4a70fc4901964d14f077171
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: x86_64-randconfig-m001-20210113 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/net/ethernet/micrel/ks8851_common.c:1129 ks8851_register_mdiobus() warn: missing error code 'ret'

vim +/ret +1129 drivers/net/ethernet/micrel/ks8851_common.c

d5b40921aa09a0cc drivers/net/ethernet/micrel/ks8851.c        Lars-Peter Clausen 2013-04-06  1104  
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1105  static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev)
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1106  {
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1107  	struct phy_device *phy_dev;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1108  	struct mii_bus *mii_bus;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1109  	int ret;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1110  
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1111  	mii_bus = mdiobus_alloc();
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1112  	if (!mii_bus)
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1113  		return -ENOMEM;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1114  
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1115  	mii_bus->name = "ks8851_eth_mii";
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1116  	mii_bus->read = ks8851_mdio_read;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1117  	mii_bus->write = ks8851_mdio_write;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1118  	mii_bus->priv = ks;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1119  	mii_bus->parent = dev;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1120  	mii_bus->phy_mask = ~((u32)BIT(0));
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1121  	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1122  
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1123  	ret = mdiobus_register(mii_bus);
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1124  	if (ret)
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1125  		goto err_mdiobus_register;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1126  
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1127  	phy_dev = phy_find_first(mii_bus);
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1128  	if (!phy_dev)
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11 @1129  		goto err_find_phy;
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1130  
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1131  	ks->mii_bus = mii_bus;
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1132  	ks->phy_dev = phy_dev;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1133  
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1134  	return 0;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1135  
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1136  err_find_phy:
c135c36e0a3639c9 drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-11  1137  	mdiobus_unregister(mii_bus);
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1138  err_mdiobus_register:
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1139  	mdiobus_free(mii_bus);
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1140  	return ret;
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1141  }
ef3631220d2b3d8d drivers/net/ethernet/micrel/ks8851_common.c Marek Vasut        2021-01-05  1142  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32434 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-01-15 14:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 12:53 [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY Marek Vasut
2021-01-11 13:26 ` Heiner Kallweit
2021-01-11 13:38   ` Marek Vasut
2021-01-11 13:50     ` Heiner Kallweit
2021-01-11 14:10       ` Marek Vasut
2021-01-11 14:43         ` Heiner Kallweit
2021-01-12 22:28           ` Marek Vasut
2021-01-11 14:47 ` Andrew Lunn
2021-01-12 22:28   ` Marek Vasut
2021-01-14 13:54     ` Andrew Lunn
2021-01-15 12:45       ` Marek Vasut
2021-01-15 14:55         ` Andrew Lunn
2021-01-13  6:49 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.