| Message ID | 20200225230819.7325-2-michael@walle.cc |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | AT8031 PHY timestamping support | expand |
On 26.02.2020 00:08, Michael Walle wrote: > There are more and more PHY drivers which has more than just the PHY > link change interrupts. For example, temperature thresholds or PTP > interrupts. > > At the moment it is not possible to correctly handle interrupts for PHYs > which has a clear-on-read interrupt status register. It is also likely > that the current approach of the phylib isn't working for all PHYs out > there. > > Therefore, this patch let the PHY driver register its own interrupt > handler. To notify the phylib about a link change, the interrupt handler > has to call the new function phy_drv_interrupt(). > We have phy_driver callback handle_interrupt for custom interrupt handlers. Any specific reason why you can't use it for your purposes? > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/net/phy/phy.c | 15 +++++++++++++++ > drivers/net/phy/phy_device.c | 6 ++++-- > include/linux/phy.h | 2 ++ > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index d76e038cf2cb..f25aacbcf1d9 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -942,6 +942,21 @@ void phy_mac_interrupt(struct phy_device *phydev) > } > EXPORT_SYMBOL(phy_mac_interrupt); > > +/** > + * phy_drv_interrupt - PHY drivers says the link has changed > + * @phydev: phy_device struct with changed link > + * > + * The PHY driver may implement his own interrupt handler. It will call this > + * function to notify us about a link change. Trigger the state machine and > + * work a work queue. > + */ This function would duplicate phy_mac_interrupt(). > +void phy_drv_interrupt(struct phy_device *phydev) > +{ > + /* Trigger a state machine change */ > + phy_trigger_machine(phydev); > +} > +EXPORT_SYMBOL(phy_drv_interrupt); > + > static void mmd_eee_adv_to_linkmode(unsigned long *advertising, u16 eee_adv) > { > linkmode_zero(advertising); > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 6a5056e0ae77..6d8c94e61251 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -965,7 +965,8 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev, > return rc; > > phy_prepare_link(phydev, handler); > - if (phy_interrupt_is_valid(phydev)) > + if (phy_interrupt_is_valid(phydev) && > + phydev->drv->flags & PHY_HAS_OWN_IRQ_HANDLER) > phy_request_interrupt(phydev); Here most likely a ! is missing. because as-is you would break current phylib interrupt mode. Where in the PHY driver (in which callback) do you want to register your own interrupt handler? > > return 0; > @@ -2411,7 +2412,8 @@ EXPORT_SYMBOL(phy_validate_pause); > > static bool phy_drv_supports_irq(struct phy_driver *phydrv) > { > - return phydrv->config_intr && phydrv->ack_interrupt; > + return ((phydrv->config_intr && phydrv->ack_interrupt) || > + phydrv->flags & PHY_HAS_OWN_IRQ_HANDLER); > } > > /** > diff --git a/include/linux/phy.h b/include/linux/phy.h > index c570e162e05e..46f73b94fd60 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -75,6 +75,7 @@ extern const int phy_10gbit_features_array[1]; > > #define PHY_IS_INTERNAL 0x00000001 > #define PHY_RST_AFTER_CLK_EN 0x00000002 > +#define PHY_HAS_OWN_IRQ_HANDLER 0x00000004 > #define MDIO_DEVICE_IS_PHY 0x80000000 > > /* Interface Mode definitions */ > @@ -1235,6 +1236,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int n, > void phy_state_machine(struct work_struct *work); > void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies); > void phy_mac_interrupt(struct phy_device *phydev); > +void phy_drv_interrupt(struct phy_device *phydev); > void phy_start_machine(struct phy_device *phydev); > void phy_stop_machine(struct phy_device *phydev); > void phy_ethtool_ksettings_get(struct phy_device *phydev, >
Am 2020-02-26 08:27, schrieb Heiner Kallweit: > On 26.02.2020 00:08, Michael Walle wrote: >> There are more and more PHY drivers which has more than just the PHY >> link change interrupts. For example, temperature thresholds or PTP >> interrupts. >> >> At the moment it is not possible to correctly handle interrupts for >> PHYs >> which has a clear-on-read interrupt status register. It is also likely >> that the current approach of the phylib isn't working for all PHYs out >> there. >> >> Therefore, this patch let the PHY driver register its own interrupt >> handler. To notify the phylib about a link change, the interrupt >> handler >> has to call the new function phy_drv_interrupt(). >> > > We have phy_driver callback handle_interrupt for custom interrupt > handlers. Any specific reason why you can't use it for your purposes? Yes, as mentioned above this wont work for PHYs which has a clear-on-read status register, because you may loose interrupts between handle_interrupt() and phy_clear_interrupt(). See also https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/ And esp. Russell reply. I tried using handle_interrupt() but it won't work unless you make the ack_interrupt a NOOP. but even then it won't work because the ack_interrupt is also used during setup etc. >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/net/phy/phy.c | 15 +++++++++++++++ >> drivers/net/phy/phy_device.c | 6 ++++-- >> include/linux/phy.h | 2 ++ >> 3 files changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index d76e038cf2cb..f25aacbcf1d9 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -942,6 +942,21 @@ void phy_mac_interrupt(struct phy_device *phydev) >> } >> EXPORT_SYMBOL(phy_mac_interrupt); >> >> +/** >> + * phy_drv_interrupt - PHY drivers says the link has changed >> + * @phydev: phy_device struct with changed link >> + * >> + * The PHY driver may implement his own interrupt handler. It will >> call this >> + * function to notify us about a link change. Trigger the state >> machine and >> + * work a work queue. >> + */ > > This function would duplicate phy_mac_interrupt(). Yes I wasn't sure, if I should just call that function of if it might change in the future. Also the description doesn't fit. Renaming the function sounded also bad, because its an exported symbol. I'm open to suggestions ;) > >> +void phy_drv_interrupt(struct phy_device *phydev) >> +{ >> + /* Trigger a state machine change */ >> + phy_trigger_machine(phydev); >> +} >> +EXPORT_SYMBOL(phy_drv_interrupt); >> + >> static void mmd_eee_adv_to_linkmode(unsigned long *advertising, u16 >> eee_adv) >> { >> linkmode_zero(advertising); >> diff --git a/drivers/net/phy/phy_device.c >> b/drivers/net/phy/phy_device.c >> index 6a5056e0ae77..6d8c94e61251 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -965,7 +965,8 @@ int phy_connect_direct(struct net_device *dev, >> struct phy_device *phydev, >> return rc; >> >> phy_prepare_link(phydev, handler); >> - if (phy_interrupt_is_valid(phydev)) >> + if (phy_interrupt_is_valid(phydev) && >> + phydev->drv->flags & PHY_HAS_OWN_IRQ_HANDLER) >> phy_request_interrupt(phydev); > > Here most likely a ! is missing. because as-is you would break > current phylib interrupt mode. whoops you're right. nice catch. > Where in the PHY driver (in which > callback) do you want to register your own interrupt handler? In the _probe() see patch 2/2. -michael >> >> return 0; >> @@ -2411,7 +2412,8 @@ EXPORT_SYMBOL(phy_validate_pause); >> >> static bool phy_drv_supports_irq(struct phy_driver *phydrv) >> { >> - return phydrv->config_intr && phydrv->ack_interrupt; >> + return ((phydrv->config_intr && phydrv->ack_interrupt) || >> + phydrv->flags & PHY_HAS_OWN_IRQ_HANDLER); >> } >> >> /** >> diff --git a/include/linux/phy.h b/include/linux/phy.h >> index c570e162e05e..46f73b94fd60 100644 >> --- a/include/linux/phy.h >> +++ b/include/linux/phy.h >> @@ -75,6 +75,7 @@ extern const int phy_10gbit_features_array[1]; >> >> #define PHY_IS_INTERNAL 0x00000001 >> #define PHY_RST_AFTER_CLK_EN 0x00000002 >> +#define PHY_HAS_OWN_IRQ_HANDLER 0x00000004 >> #define MDIO_DEVICE_IS_PHY 0x80000000 >> >> /* Interface Mode definitions */ >> @@ -1235,6 +1236,7 @@ int phy_drivers_register(struct phy_driver >> *new_driver, int n, >> void phy_state_machine(struct work_struct *work); >> void phy_queue_state_machine(struct phy_device *phydev, unsigned long >> jiffies); >> void phy_mac_interrupt(struct phy_device *phydev); >> +void phy_drv_interrupt(struct phy_device *phydev); >> void phy_start_machine(struct phy_device *phydev); >> void phy_stop_machine(struct phy_device *phydev); >> void phy_ethtool_ksettings_get(struct phy_device *phydev, >>
On 26.02.2020 12:12, Michael Walle wrote: > Am 2020-02-26 08:27, schrieb Heiner Kallweit: >> On 26.02.2020 00:08, Michael Walle wrote: >>> There are more and more PHY drivers which has more than just the PHY >>> link change interrupts. For example, temperature thresholds or PTP >>> interrupts. >>> >>> At the moment it is not possible to correctly handle interrupts for PHYs >>> which has a clear-on-read interrupt status register. It is also likely >>> that the current approach of the phylib isn't working for all PHYs out >>> there. >>> >>> Therefore, this patch let the PHY driver register its own interrupt >>> handler. To notify the phylib about a link change, the interrupt handler >>> has to call the new function phy_drv_interrupt(). >>> >> >> We have phy_driver callback handle_interrupt for custom interrupt >> handlers. Any specific reason why you can't use it for your purposes? > > Yes, as mentioned above this wont work for PHYs which has a clear-on-read > status register, because you may loose interrupts between handle_interrupt() > and phy_clear_interrupt(). > > See also > > https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/ > > And esp. Russell reply. I tried using handle_interrupt() but it won't work > unless you make the ack_interrupt a NOOP. but even then it won't work because > the ack_interrupt is also used during setup etc. > Right, now I remember .. So far we have only one user of the handle_interrupt callback. Following a proposal from the quoted discussion, can you base your patch on the following and check? Note: Even though you implement handle_interrupt, you still have to implement ack_interrupt too. --- drivers/net/phy/mscc.c | 3 ++- drivers/net/phy/phy.c | 4 ++-- include/linux/phy.h | 4 +++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c index 937ac7da2..20b9d3ef5 100644 --- a/drivers/net/phy/mscc.c +++ b/drivers/net/phy/mscc.c @@ -2868,7 +2868,8 @@ static int vsc8584_handle_interrupt(struct phy_device *phydev) #endif phy_mac_interrupt(phydev); - return 0; + + return vsc85xx_ack_interrupt(phydev); } static int vsc85xx_config_init(struct phy_device *phydev) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index d76e038cf..de52f0e82 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -725,10 +725,10 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat) } else { /* reschedule state queue work to run as soon as possible */ phy_trigger_machine(phydev); + if (phy_clear_interrupt(phydev)) + goto phy_err; } - if (phy_clear_interrupt(phydev)) - goto phy_err; return IRQ_HANDLED; phy_err: diff --git a/include/linux/phy.h b/include/linux/phy.h index 80f8b2158..9e2895ee4 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -560,7 +560,9 @@ struct phy_driver { */ int (*did_interrupt)(struct phy_device *phydev); - /* Override default interrupt handling */ + /* Override default interrupt handling. Handler has to ensure + * that interrupt is ack'ed. + */ int (*handle_interrupt)(struct phy_device *phydev); /* Clears up any memory if needed */
Am 2020-02-26 22:17, schrieb Heiner Kallweit: > On 26.02.2020 12:12, Michael Walle wrote: >> Am 2020-02-26 08:27, schrieb Heiner Kallweit: >>> On 26.02.2020 00:08, Michael Walle wrote: >>>> There are more and more PHY drivers which has more than just the PHY >>>> link change interrupts. For example, temperature thresholds or PTP >>>> interrupts. >>>> >>>> At the moment it is not possible to correctly handle interrupts for >>>> PHYs >>>> which has a clear-on-read interrupt status register. It is also >>>> likely >>>> that the current approach of the phylib isn't working for all PHYs >>>> out >>>> there. >>>> >>>> Therefore, this patch let the PHY driver register its own interrupt >>>> handler. To notify the phylib about a link change, the interrupt >>>> handler >>>> has to call the new function phy_drv_interrupt(). >>>> >>> >>> We have phy_driver callback handle_interrupt for custom interrupt >>> handlers. Any specific reason why you can't use it for your purposes? >> >> Yes, as mentioned above this wont work for PHYs which has a >> clear-on-read >> status register, because you may loose interrupts between >> handle_interrupt() >> and phy_clear_interrupt(). >> >> See also >> >> https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/ >> >> And esp. Russell reply. I tried using handle_interrupt() but it won't >> work >> unless you make the ack_interrupt a NOOP. but even then it won't work >> because >> the ack_interrupt is also used during setup etc. >> > Right, now I remember .. So far we have only one user of the > handle_interrupt > callback. Following a proposal from the quoted discussion, can you base > your > patch on the following and check? > Note: Even though you implement handle_interrupt, you still have to > implement > ack_interrupt too. I guess that should work, I can give that a try. But I'm also preparing support for the BCM54140, a quad PHY. I guess we have the same problem with did_interrupt(). Eg. to tell if there actually was an interrupt from that PHY you have to read the status register which clears the interrupt. So handle_interrupt() will left with no actual interrupt pending bits. I've had a look at how the vsc8584 does it as it also uses did_interrupt() together with handle_interrupt() and it looks like it only works because handle_interrupt() doesn't actually read the pending bits again. Also I guess there is a chance of missing interrupts between vsc8584_did_interrupt() and vsc85xx_ack_interrupt() which both clears interrupts. Although I'm not sure if that matters for the current implementation. -michael > > > --- > drivers/net/phy/mscc.c | 3 ++- > drivers/net/phy/phy.c | 4 ++-- > include/linux/phy.h | 4 +++- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c > index 937ac7da2..20b9d3ef5 100644 > --- a/drivers/net/phy/mscc.c > +++ b/drivers/net/phy/mscc.c > @@ -2868,7 +2868,8 @@ static int vsc8584_handle_interrupt(struct > phy_device *phydev) > #endif > > phy_mac_interrupt(phydev); > - return 0; > + > + return vsc85xx_ack_interrupt(phydev); > } > > static int vsc85xx_config_init(struct phy_device *phydev) > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index d76e038cf..de52f0e82 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -725,10 +725,10 @@ static irqreturn_t phy_interrupt(int irq, void > *phy_dat) > } else { > /* reschedule state queue work to run as soon as possible */ > phy_trigger_machine(phydev); > + if (phy_clear_interrupt(phydev)) > + goto phy_err; > } > > - if (phy_clear_interrupt(phydev)) > - goto phy_err; > return IRQ_HANDLED; > > phy_err: > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 80f8b2158..9e2895ee4 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -560,7 +560,9 @@ struct phy_driver { > */ > int (*did_interrupt)(struct phy_device *phydev); > > - /* Override default interrupt handling */ > + /* Override default interrupt handling. Handler has to ensure > + * that interrupt is ack'ed. > + */ > int (*handle_interrupt)(struct phy_device *phydev); > > /* Clears up any memory if needed */
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index d76e038cf2cb..f25aacbcf1d9 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -942,6 +942,21 @@ void phy_mac_interrupt(struct phy_device *phydev) } EXPORT_SYMBOL(phy_mac_interrupt); +/** + * phy_drv_interrupt - PHY drivers says the link has changed + * @phydev: phy_device struct with changed link + * + * The PHY driver may implement his own interrupt handler. It will call this + * function to notify us about a link change. Trigger the state machine and + * work a work queue. + */ +void phy_drv_interrupt(struct phy_device *phydev) +{ + /* Trigger a state machine change */ + phy_trigger_machine(phydev); +} +EXPORT_SYMBOL(phy_drv_interrupt); + static void mmd_eee_adv_to_linkmode(unsigned long *advertising, u16 eee_adv) { linkmode_zero(advertising); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 6a5056e0ae77..6d8c94e61251 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -965,7 +965,8 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev, return rc; phy_prepare_link(phydev, handler); - if (phy_interrupt_is_valid(phydev)) + if (phy_interrupt_is_valid(phydev) && + phydev->drv->flags & PHY_HAS_OWN_IRQ_HANDLER) phy_request_interrupt(phydev); return 0; @@ -2411,7 +2412,8 @@ EXPORT_SYMBOL(phy_validate_pause); static bool phy_drv_supports_irq(struct phy_driver *phydrv) { - return phydrv->config_intr && phydrv->ack_interrupt; + return ((phydrv->config_intr && phydrv->ack_interrupt) || + phydrv->flags & PHY_HAS_OWN_IRQ_HANDLER); } /** diff --git a/include/linux/phy.h b/include/linux/phy.h index c570e162e05e..46f73b94fd60 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -75,6 +75,7 @@ extern const int phy_10gbit_features_array[1]; #define PHY_IS_INTERNAL 0x00000001 #define PHY_RST_AFTER_CLK_EN 0x00000002 +#define PHY_HAS_OWN_IRQ_HANDLER 0x00000004 #define MDIO_DEVICE_IS_PHY 0x80000000 /* Interface Mode definitions */ @@ -1235,6 +1236,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int n, void phy_state_machine(struct work_struct *work); void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies); void phy_mac_interrupt(struct phy_device *phydev); +void phy_drv_interrupt(struct phy_device *phydev); void phy_start_machine(struct phy_device *phydev); void phy_stop_machine(struct phy_device *phydev); void phy_ethtool_ksettings_get(struct phy_device *phydev,
There are more and more PHY drivers which has more than just the PHY link change interrupts. For example, temperature thresholds or PTP interrupts. At the moment it is not possible to correctly handle interrupts for PHYs which has a clear-on-read interrupt status register. It is also likely that the current approach of the phylib isn't working for all PHYs out there. Therefore, this patch let the PHY driver register its own interrupt handler. To notify the phylib about a link change, the interrupt handler has to call the new function phy_drv_interrupt(). Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/phy/phy.c | 15 +++++++++++++++ drivers/net/phy/phy_device.c | 6 ++++-- include/linux/phy.h | 2 ++ 3 files changed, 21 insertions(+), 2 deletions(-)