| Message ID | fd9674b3-4033-95c6-a93e-10c8b5ba2e6a@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt | expand |
Hi Heiner > +static bool phy_drv_supports_irq(struct phy_driver *phydrv) > +{ > + return phydrv->config_intr || phydrv->ack_interrupt; > +} Should this be && not || ? I thought both needed to be provided for interrupts to work. Andrew
On 09.11.2018 21:13, Andrew Lunn wrote: > Hi Heiner > >> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) >> +{ >> + return phydrv->config_intr || phydrv->ack_interrupt; >> +} > > Should this be && not || ? I thought both needed to be provided for > interrupts to work. > > Andrew > I've seen at least one driver which configures interrupts in config_init and doesn't define a config_intr callback (ack_interrupt callback is there) Intention of this check is not to ensure that the driver defines everything to make interrupts work. All it states: If at least one of the irq-related callbacks is defined, then we interpret this as indicator that the PHY supports interrupts. Heiner
On 11/9/18 12:22 PM, Heiner Kallweit wrote: > On 09.11.2018 21:13, Andrew Lunn wrote: >> Hi Heiner >> >>> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) >>> +{ >>> + return phydrv->config_intr || phydrv->ack_interrupt; >>> +} >> >> Should this be && not || ? I thought both needed to be provided for >> interrupts to work. >> >> Andrew >> > I've seen at least one driver which configures interrupts in > config_init and doesn't define a config_intr callback > (ack_interrupt callback is there) That driver should probably be fixed, while it most likely does not make any significant difference during probe/connect, since config_init() and config_intr() are virtually happening at the same time, this is not necessarily true when disconnecting from the PHY where we really want config_intr() to effectively disable the interrupts and not leaving something enabled that would now become unmaskable, because no more driver attached. > Intention of this check is not to ensure that the driver defines > everything to make interrupts work. All it states: > If at least one of the irq-related callbacks is defined, then > we interpret this as indicator that the PHY supports interrupts. I agree with Andrew here, that this should be an AND here, both callbacks must be implemented for interrupts to work correctly.
On Fri, Nov 09, 2018 at 09:22:55PM +0100, Heiner Kallweit wrote: > On 09.11.2018 21:13, Andrew Lunn wrote: > > Hi Heiner > > > >> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) > >> +{ > >> + return phydrv->config_intr || phydrv->ack_interrupt; > >> +} > > > > Should this be && not || ? I thought both needed to be provided for > > interrupts to work. > > > > Andrew > > > I've seen at least one driver which configures interrupts in > config_init and doesn't define a config_intr callback > (ack_interrupt callback is there) > Intention of this check is not to ensure that the driver defines > everything to make interrupts work. All it states: > If at least one of the irq-related callbacks is defined, then > we interpret this as indicator that the PHY supports interrupts. I'm just wondering if that driver is broken if it enables interrupts in config_init()? phylib deliberately enable/disable interrupts. If we cannot do that, can we get an interrupt when we don't expect it? Can we miss a state transition which would be reported when interrupts would be re-enabled immediately triggering an interrupt? Well, the current code does not seem to care if one is missing. So i doubt this is making it more broken. So, Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 09.11.2018 21:33, Florian Fainelli wrote: > On 11/9/18 12:22 PM, Heiner Kallweit wrote: >> On 09.11.2018 21:13, Andrew Lunn wrote: >>> Hi Heiner >>> >>>> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) >>>> +{ >>>> + return phydrv->config_intr || phydrv->ack_interrupt; >>>> +} >>> >>> Should this be && not || ? I thought both needed to be provided for >>> interrupts to work. >>> >>> Andrew >>> >> I've seen at least one driver which configures interrupts in >> config_init and doesn't define a config_intr callback >> (ack_interrupt callback is there) > > That driver should probably be fixed, while it most likely does not make > any significant difference during probe/connect, since config_init() and > config_intr() are virtually happening at the same time, this is not > necessarily true when disconnecting from the PHY where we really want > config_intr() to effectively disable the interrupts and not leaving > something enabled that would now become unmaskable, because no more > driver attached. > Found the driver: It's the IP101A/G in icplus.c It should be easy to fix the behavior and move the interrupt config to a config_intr callback. But the last real changes to the driver have been done 6 years ago, so I'm not sure there's anybody out there who can test. >> Intention of this check is not to ensure that the driver defines >> everything to make interrupts work. All it states: >> If at least one of the irq-related callbacks is defined, then >> we interpret this as indicator that the PHY supports interrupts. > > I agree with Andrew here, that this should be an AND here, both > callbacks must be implemented for interrupts to work correctly. >
Hi Heiner, On Fri, Nov 9, 2018 at 9:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 09.11.2018 21:33, Florian Fainelli wrote: > > On 11/9/18 12:22 PM, Heiner Kallweit wrote: > >> On 09.11.2018 21:13, Andrew Lunn wrote: > >>> Hi Heiner > >>> > >>>> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) > >>>> +{ > >>>> + return phydrv->config_intr || phydrv->ack_interrupt; > >>>> +} > >>> > >>> Should this be && not || ? I thought both needed to be provided for > >>> interrupts to work. > >>> > >>> Andrew > >>> > >> I've seen at least one driver which configures interrupts in > >> config_init and doesn't define a config_intr callback > >> (ack_interrupt callback is there) > > > > That driver should probably be fixed, while it most likely does not make > > any significant difference during probe/connect, since config_init() and > > config_intr() are virtually happening at the same time, this is not > > necessarily true when disconnecting from the PHY where we really want > > config_intr() to effectively disable the interrupts and not leaving > > something enabled that would now become unmaskable, because no more > > driver attached. > > > Found the driver: It's the IP101A/G in icplus.c > It should be easy to fix the behavior and move the interrupt config > to a config_intr callback. But the last real changes to the driver > have been done 6 years ago, so I'm not sure there's anybody out > there who can test. if you want I can take care of the IP101A/G code. I have at least one board with an IP101A/G (PHY ID: 0x02430c54, according to the schematics it's an IP101GR-GP) where the interrupt is routed to the SoC. please let me know whether you'd like to work on it or if I should give it a try. Regards Martin
On 12.11.2018 21:32, Martin Blumenstingl wrote: > Hi Heiner, > > On Fri, Nov 9, 2018 at 9:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> On 09.11.2018 21:33, Florian Fainelli wrote: >>> On 11/9/18 12:22 PM, Heiner Kallweit wrote: >>>> On 09.11.2018 21:13, Andrew Lunn wrote: >>>>> Hi Heiner >>>>> >>>>>> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) >>>>>> +{ >>>>>> + return phydrv->config_intr || phydrv->ack_interrupt; >>>>>> +} >>>>> >>>>> Should this be && not || ? I thought both needed to be provided for >>>>> interrupts to work. >>>>> >>>>> Andrew >>>>> >>>> I've seen at least one driver which configures interrupts in >>>> config_init and doesn't define a config_intr callback >>>> (ack_interrupt callback is there) >>> >>> That driver should probably be fixed, while it most likely does not make >>> any significant difference during probe/connect, since config_init() and >>> config_intr() are virtually happening at the same time, this is not >>> necessarily true when disconnecting from the PHY where we really want >>> config_intr() to effectively disable the interrupts and not leaving >>> something enabled that would now become unmaskable, because no more >>> driver attached. >>> >> Found the driver: It's the IP101A/G in icplus.c >> It should be easy to fix the behavior and move the interrupt config >> to a config_intr callback. But the last real changes to the driver >> have been done 6 years ago, so I'm not sure there's anybody out >> there who can test. > if you want I can take care of the IP101A/G code. > I have at least one board with an IP101A/G (PHY ID: 0x02430c54, > according to the schematics it's an IP101GR-GP) where the interrupt is > routed to the SoC. > > please let me know whether you'd like to work on it or if I should > give it a try. > I made the change already based on the datasheet of IP101A LF which is supposed to be register-compatible with IP101A/G. The patch is not applied yet, you can find it in the mailing list archive or in patchwork. Would be great if you could test it and report problems or add a Tested-by. > > Regards > Martin > Rgds, Heiner
On Mon, Nov 12, 2018 at 9:38 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 12.11.2018 21:32, Martin Blumenstingl wrote: > > Hi Heiner, > > > > On Fri, Nov 9, 2018 at 9:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> On 09.11.2018 21:33, Florian Fainelli wrote: > >>> On 11/9/18 12:22 PM, Heiner Kallweit wrote: > >>>> On 09.11.2018 21:13, Andrew Lunn wrote: > >>>>> Hi Heiner > >>>>> > >>>>>> +static bool phy_drv_supports_irq(struct phy_driver *phydrv) > >>>>>> +{ > >>>>>> + return phydrv->config_intr || phydrv->ack_interrupt; > >>>>>> +} > >>>>> > >>>>> Should this be && not || ? I thought both needed to be provided for > >>>>> interrupts to work. > >>>>> > >>>>> Andrew > >>>>> > >>>> I've seen at least one driver which configures interrupts in > >>>> config_init and doesn't define a config_intr callback > >>>> (ack_interrupt callback is there) > >>> > >>> That driver should probably be fixed, while it most likely does not make > >>> any significant difference during probe/connect, since config_init() and > >>> config_intr() are virtually happening at the same time, this is not > >>> necessarily true when disconnecting from the PHY where we really want > >>> config_intr() to effectively disable the interrupts and not leaving > >>> something enabled that would now become unmaskable, because no more > >>> driver attached. > >>> > >> Found the driver: It's the IP101A/G in icplus.c > >> It should be easy to fix the behavior and move the interrupt config > >> to a config_intr callback. But the last real changes to the driver > >> have been done 6 years ago, so I'm not sure there's anybody out > >> there who can test. > > if you want I can take care of the IP101A/G code. > > I have at least one board with an IP101A/G (PHY ID: 0x02430c54, > > according to the schematics it's an IP101GR-GP) where the interrupt is > > routed to the SoC. > > > > please let me know whether you'd like to work on it or if I should > > give it a try. > > > I made the change already based on the datasheet of IP101A LF which > is supposed to be register-compatible with IP101A/G. > The patch is not applied yet, you can find it in the mailing > list archive or in patchwork. Would be great if you could test it > and report problems or add a Tested-by. I will test it but I doubt it works out-of-the-box in my setup: my board routes the RXER/INTR_32 pin of the IP101GR to the SoC. that pin defaults to "RXER" (receive error signal). so I need to come up with some extra patches which toggle the bit to output the interrupt signal on that pin in other words: I'll give your patches a try as soon as I have time and give my Tested-by along with my extra patches on top of yours. Regards Martin
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index ab33d1777..00a46218c 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2074,6 +2074,11 @@ static void of_set_phy_eee_broken(struct phy_device *phydev) phydev->eee_broken_modes = broken; } +static bool phy_drv_supports_irq(struct phy_driver *phydrv) +{ + return phydrv->config_intr || phydrv->ack_interrupt; +} + /** * phy_probe - probe and init a PHY device * @dev: device to probe and init @@ -2095,8 +2100,7 @@ static int phy_probe(struct device *dev) /* Disable the interrupt if the PHY doesn't support it * but the interrupt is still a valid one */ - if (!(phydrv->flags & PHY_HAS_INTERRUPT) && - phy_interrupt_is_valid(phydev)) + if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev)) phydev->irq = PHY_POLL; if (phydrv->flags & PHY_IS_INTERNAL)
Flag PHY_HAS_INTERRUPT is used only here for this small check. I think using interrupts isn't possible if a driver defines neither config_intr nor ack_interrupts callback. So we can replace checking flag PHY_HAS_INTERRUPT with checking for these callbacks. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - add helper for the check v3: - no changes --- drivers/net/phy/phy_device.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)