* [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.