* [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip @ 2009-02-16 20:53 Anatolij Gustschin 2009-02-16 20:53 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Anatolij Gustschin 2009-02-27 6:39 ` [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip David Miller 0 siblings, 2 replies; 6+ messages in thread From: Anatolij Gustschin @ 2009-02-16 20:53 UTC (permalink / raw) To: netdev; +Cc: Sergei Poselenov, Yuri Tikhonov, Anatolij Gustschin From: Sergei Poselenov <sposelenov@emcraft.com> Signed-off-by: Yuri Tikhonov <yur@emcraft.com> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com> Signed-off-by: Anatolij Gustschin <agust@denx.de> --- drivers/net/phy/marvell.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-) diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 4aa5479..1d282b0 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -69,6 +69,12 @@ #define MII_M1111_COPPER 0 #define MII_M1111_FIBER 1 +#define MII_88E1121_PHY_LED_CTRL 16 +#define MII_88E1121_PHY_LED_PAGE 3 +#define MII_88E1121_PHY_LED_DEF 0x0030 + +#define MII_88E1121_PHY_PAGE 22 + #define MII_M1011_PHY_STATUS 0x11 #define MII_M1011_PHY_STATUS_1000 0x8000 #define MII_M1011_PHY_STATUS_100 0x4000 @@ -154,6 +160,30 @@ static int marvell_config_aneg(struct phy_device *phydev) return err; } +static int m88e1121_config_aneg(struct phy_device *phydev) +{ + int err, temp; + + err = phy_write(phydev, MII_BMCR, BMCR_RESET); + if (err < 0) + return err; + + err = phy_write(phydev, MII_M1011_PHY_SCR, + MII_M1011_PHY_SCR_AUTO_CROSS); + if (err < 0) + return err; + + temp = phy_read(phydev, MII_88E1121_PHY_PAGE); + + phy_write(phydev, MII_88E1121_PHY_PAGE, MII_88E1121_PHY_LED_PAGE); + phy_write(phydev, MII_88E1121_PHY_LED_CTRL, MII_88E1121_PHY_LED_DEF); + phy_write(phydev, MII_88E1121_PHY_PAGE, temp); + + err = genphy_config_aneg(phydev); + + return err; +} + static int m88e1111_config_init(struct phy_device *phydev) { int err; @@ -416,6 +446,18 @@ static struct phy_driver marvell_drivers[] = { .driver = { .owner = THIS_MODULE }, }, { + .phy_id = 0x01410cb0, + .phy_id_mask = 0xfffffff0, + .name = "Marvell 88E1121R", + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .config_aneg = &m88e1121_config_aneg, + .read_status = &marvell_read_status, + .ack_interrupt = &marvell_ack_interrupt, + .config_intr = &marvell_config_intr, + .driver = { .owner = THIS_MODULE }, + }, + { .phy_id = 0x01410cd0, .phy_id_mask = 0xfffffff0, .name = "Marvell 88E1145", -- 1.5.4.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] phylib: add support for shared PHY interrupts 2009-02-16 20:53 [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip Anatolij Gustschin @ 2009-02-16 20:53 ` Anatolij Gustschin 2009-02-16 20:53 ` [PATCH 3/3] phylib: shared interrupts support for Marvell 88E1121R PHY Anatolij Gustschin 2009-02-18 0:47 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Andy Fleming 2009-02-27 6:39 ` [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip David Miller 1 sibling, 2 replies; 6+ messages in thread From: Anatolij Gustschin @ 2009-02-16 20:53 UTC (permalink / raw) To: netdev; +Cc: Anatolij Gustschin Marvell 88E1121R PHY device can be hardware-configured to use shared interrupt pin for both PHY ports. This patch adds support for shared PHY interrupts to the PHY framework to enable support for such PHY configurations. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- drivers/net/phy/phy.c | 13 +++++++++++++ drivers/net/phy/phy_device.c | 2 ++ include/linux/phy.h | 4 ++++ 3 files changed, 19 insertions(+), 0 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index df4e625..93d387e 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -657,9 +657,17 @@ EXPORT_SYMBOL(phy_stop_interrupts); static void phy_change(struct work_struct *work) { int err; + int src; struct phy_device *phydev = container_of(work, struct phy_device, phy_queue); + if (phydev->drv->flags & PHY_INTERRUPTS_SHARED + && phydev->drv->interrupt_src) { + src = phydev->drv->interrupt_src(phydev); + if ((src == -1) || (src != phydev->addr)) + goto ignore; + } + err = phy_disable_interrupts(phydev); if (err) @@ -688,6 +696,11 @@ static void phy_change(struct work_struct *work) return; +ignore: + atomic_dec(&phydev->irq_disable); + enable_irq(phydev->irq); + return; + irq_enable_err: disable_irq(phydev->irq); atomic_inc(&phydev->irq_disable); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index e11b03b..dccc445 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -401,6 +401,8 @@ void phy_detach(struct phy_device *phydev) * real driver could be loaded */ if (phydev->dev.driver == &genphy_driver.driver) device_release_driver(&phydev->dev); + else if (phydev->drv->remove) + phydev->drv->remove(phydev); } EXPORT_SYMBOL(phy_detach); diff --git a/include/linux/phy.h b/include/linux/phy.h index 77c4ed6..a844c2a 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -49,6 +49,7 @@ #define PHY_HAS_INTERRUPT 0x00000001 #define PHY_HAS_MAGICANEG 0x00000002 +#define PHY_INTERRUPTS_SHARED 0x00000004 /* Interface Mode definitions */ typedef enum { @@ -389,6 +390,9 @@ struct phy_driver { /* Enables or disables interrupts */ int (*config_intr)(struct phy_device *phydev); + /* Returns interrupt source if interrupt pin is shared */ + int (*interrupt_src)(struct phy_device *phydev); + /* Clears up any memory if needed */ void (*remove)(struct phy_device *phydev); -- 1.5.4.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] phylib: shared interrupts support for Marvell 88E1121R PHY 2009-02-16 20:53 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Anatolij Gustschin @ 2009-02-16 20:53 ` Anatolij Gustschin 2009-02-18 1:12 ` Andy Fleming 2009-02-18 0:47 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Andy Fleming 1 sibling, 1 reply; 6+ messages in thread From: Anatolij Gustschin @ 2009-02-16 20:53 UTC (permalink / raw) To: netdev; +Cc: Anatolij Gustschin If this PHY is hardware-configured for shared MDC/MDIO, P0_INT pin will indicate interrupts for both ports. This patch extends 88E1121R code for handling interrupt events over shared interrupt pin. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- drivers/net/phy/marvell.c | 86 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 86 insertions(+), 0 deletions(-) diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 1d282b0..64f22b9 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -74,6 +74,7 @@ #define MII_88E1121_PHY_LED_DEF 0x0030 #define MII_88E1121_PHY_PAGE 22 +#define MII_88E1121_PHY_GLOBAL_IRQS 23 #define MII_M1011_PHY_STATUS 0x11 #define MII_M1011_PHY_STATUS_1000 0x8000 @@ -88,6 +89,10 @@ MODULE_DESCRIPTION("Marvell PHY driver"); MODULE_AUTHOR("Andy Fleming"); MODULE_LICENSE("GPL"); +struct m88e1121_priv { + unsigned long irq_map; +} m88e1121_drv_data; + static int marvell_ack_interrupt(struct phy_device *phydev) { int err; @@ -257,6 +262,36 @@ static int m88e1111_config_init(struct phy_device *phydev) return 0; } +static int m88e1121_config_init(struct phy_device *phydev) +{ + struct m88e1121_priv *priv; + struct mii_bus *bus = phydev->bus; + int err; + + /* Disable interrupts */ + err = phy_write(phydev, MII_M1011_IMASK, MII_M1011_IMASK_CLEAR); + if (err < 0) + return err; + + /* Clear interrupts */ + err = phy_read(phydev, MII_M1011_IEVENT); + if (err < 0) + return err; + + priv = &m88e1121_drv_data; + phydev->priv = priv; + + if (phydev->drv->flags & PHY_HAS_INTERRUPT) + set_bit(phydev->addr, &priv->irq_map); + + if ((bus->irq[0] != -1) && (bus->irq[1] != -1) + && (bus->irq[0] == bus->irq[1])) { + phydev->drv->flags |= PHY_INTERRUPTS_SHARED; + } + + return 0; +} + static int m88e1145_config_init(struct phy_device *phydev) { int err; @@ -406,6 +441,54 @@ static int marvell_read_status(struct phy_device *phydev) return 0; } +static int m88e1121_interrupt_src(struct phy_device *phydev) +{ + struct m88e1121_priv *priv = phydev->priv; + struct mii_bus *bus = phydev->bus; + int src, port = -1; + + src = phy_read(phydev, MII_88E1121_PHY_GLOBAL_IRQS); + if (src < 0) + return src; + + if (src & 1) + port = 0; + else if (src & 2) + port = 1; + + if (port != -1 && phydev->addr != port + && !(priv->irq_map & (1 << port)) + && bus->phy_map[port]) { + /* + * Interrupt not for this port and + * interface is down, so we disable + * this interrupt and ack it. + */ + BUG_ON(in_interrupt()); + + mutex_lock(&bus->mdio_lock); + /* Disable the interrupts */ + src = bus->write(bus, bus->phy_map[port]->addr, + MII_M1011_IMASK, MII_M1011_IMASK_CLEAR); + + /* Clear the interrupts */ + src = bus->read(bus, bus->phy_map[port]->addr, + MII_M1011_IEVENT); + mutex_unlock(&bus->mdio_lock); + if (src < 0) + return src; + } + + return port; +} + +static void m88e1121_remove(struct phy_device *phydev) +{ + struct m88e1121_priv *priv = phydev->priv; + + clear_bit(phydev->addr, &priv->irq_map); +} + static struct phy_driver marvell_drivers[] = { { .phy_id = 0x01410c60, @@ -451,10 +534,13 @@ static struct phy_driver marvell_drivers[] = { .name = "Marvell 88E1121R", .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, + .config_init = &m88e1121_config_init, .config_aneg = &m88e1121_config_aneg, .read_status = &marvell_read_status, .ack_interrupt = &marvell_ack_interrupt, + .interrupt_src = &m88e1121_interrupt_src, .config_intr = &marvell_config_intr, + .remove = &m88e1121_remove, .driver = { .owner = THIS_MODULE }, }, { -- 1.5.4.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] phylib: shared interrupts support for Marvell 88E1121R PHY 2009-02-16 20:53 ` [PATCH 3/3] phylib: shared interrupts support for Marvell 88E1121R PHY Anatolij Gustschin @ 2009-02-18 1:12 ` Andy Fleming 0 siblings, 0 replies; 6+ messages in thread From: Andy Fleming @ 2009-02-18 1:12 UTC (permalink / raw) To: Anatolij Gustschin; +Cc: netdev On Mon, Feb 16, 2009 at 2:53 PM, Anatolij Gustschin <agust@denx.de> wrote: > If this PHY is hardware-configured for shared MDC/MDIO, > P0_INT pin will indicate interrupts for both ports. > This patch extends 88E1121R code for handling interrupt > events over shared interrupt pin. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > drivers/net/phy/marvell.c | 86 +++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 86 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index 1d282b0..64f22b9 100644 > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -74,6 +74,7 @@ > #define MII_88E1121_PHY_LED_DEF 0x0030 > > #define MII_88E1121_PHY_PAGE 22 > +#define MII_88E1121_PHY_GLOBAL_IRQS 23 > > #define MII_M1011_PHY_STATUS 0x11 > #define MII_M1011_PHY_STATUS_1000 0x8000 > @@ -88,6 +89,10 @@ MODULE_DESCRIPTION("Marvell PHY driver"); > MODULE_AUTHOR("Andy Fleming"); > MODULE_LICENSE("GPL"); > > +struct m88e1121_priv { > + unsigned long irq_map; > +} m88e1121_drv_data; > + This is bad, and doesn't work. This is the sort of reason we may need a container struct for multi-PHY devices. The problem, here, is that you have made it impossible for one system to have more than one m88e1121, and I consider that unacceptable from a design perspective. > static int marvell_ack_interrupt(struct phy_device *phydev) > { > int err; > @@ -257,6 +262,36 @@ static int m88e1111_config_init(struct phy_device *phydev) > return 0; > } > > +static int m88e1121_config_init(struct phy_device *phydev) > +{ [snip] > + > + if (phydev->drv->flags & PHY_HAS_INTERRUPT) > + set_bit(phydev->addr, &priv->irq_map); Also, there's a race condition, here if two things try to modify this value at once. > + > + if ((bus->irq[0] != -1) && (bus->irq[1] != -1) > + && (bus->irq[0] == bus->irq[1])) { This is no good. I feel certain that the Marvell PHY in question doesn't have the PHY addresses hard-coded like this. If some board designer decides not to start the PHY addresses at 0 (as they should, since PHY addr 0 is technically reserved), this code will fail. > + phydev->drv->flags |= PHY_INTERRUPTS_SHARED; You can't do this. You are modifying the *global* driver structure for the entire system based on the runtime parameters of 2 devices. But, as mentioned before, I don't think you need this bit, anyway. > +static int m88e1121_interrupt_src(struct phy_device *phydev) > +{ > + struct m88e1121_priv *priv = phydev->priv; > + struct mii_bus *bus = phydev->bus; > + int src, port = -1; > + > + src = phy_read(phydev, MII_88E1121_PHY_GLOBAL_IRQS); > + if (src < 0) > + return src; > + > + if (src & 1) > + port = 0; > + else if (src & 2) > + port = 1; Again, these are hard-coded, and that seems wrong, to me. I don't know what happens if you pinstrap the 1121 to a different address, but I'm positive it would break this code. If we decide to go with a function that reports whether *this* PHY generated the interrupt, you would want to do something more like: if (src & (1 << phydev->addr)) return 1; else return 0; > + > + if (port != -1 && phydev->addr != port > + && !(priv->irq_map & (1 << port)) > + && bus->phy_map[port]) { > + /* > + * Interrupt not for this port and > + * interface is down, so we disable > + * this interrupt and ack it. > + */ There are other ways to determine whether the PHY has been brought up (eg, reading the state field). However, I'm fairly convinced that if this happens, it's a bug. You've got an interrupt being generated by a device that has no driver running? Please make sure that doesn't happen, instead. > + BUG_ON(in_interrupt()); > + > + mutex_lock(&bus->mdio_lock); > + /* Disable the interrupts */ > + src = bus->write(bus, bus->phy_map[port]->addr, > + MII_M1011_IMASK, MII_M1011_IMASK_CLEAR); > + > + /* Clear the interrupts */ > + src = bus->read(bus, bus->phy_map[port]->addr, > + MII_M1011_IEVENT); > + mutex_unlock(&bus->mdio_lock); > + if (src < 0) > + return src; > + } > + > + return port; > +} > + > +static void m88e1121_remove(struct phy_device *phydev) > +{ > + struct m88e1121_priv *priv = phydev->priv; > + > + clear_bit(phydev->addr, &priv->irq_map); > +} I don't think you need irq_map at all. If you do, you should base it on whether the interrupt is enabled on that PHY, rather than whether the PHY is up (which is determined by phydev->state). However, you will need to come up with some way for the chip-global information to be dealt with. If someone builds a system with multiple 1121s, you need a way for the probe function to allocate global information for each 1121, and then associate each specific PHY with the correct instance. I think, for this problem, we don't need the phylib to have a notion of multi-phy devices, but it will be interesting to see how they evolve as time goes on. Andy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] phylib: add support for shared PHY interrupts 2009-02-16 20:53 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Anatolij Gustschin 2009-02-16 20:53 ` [PATCH 3/3] phylib: shared interrupts support for Marvell 88E1121R PHY Anatolij Gustschin @ 2009-02-18 0:47 ` Andy Fleming 1 sibling, 0 replies; 6+ messages in thread From: Andy Fleming @ 2009-02-18 0:47 UTC (permalink / raw) To: Anatolij Gustschin; +Cc: netdev On Mon, Feb 16, 2009 at 2:53 PM, Anatolij Gustschin <agust@denx.de> wrote: > Marvell 88E1121R PHY device can be hardware-configured > to use shared interrupt pin for both PHY ports. This > patch adds support for shared PHY interrupts to the PHY > framework to enable support for such PHY configurations. Well, technically the phylib already supports shared interrupts (all of my systems have only one interrupt for all of the PHYs). This patch adds support for some PHYs which can report which device caused the interrupt. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > drivers/net/phy/phy.c | 13 +++++++++++++ > drivers/net/phy/phy_device.c | 2 ++ > include/linux/phy.h | 4 ++++ > 3 files changed, 19 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index df4e625..93d387e 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -657,9 +657,17 @@ EXPORT_SYMBOL(phy_stop_interrupts); > static void phy_change(struct work_struct *work) > { > int err; > + int src; > struct phy_device *phydev = > container_of(work, struct phy_device, phy_queue); > > + if (phydev->drv->flags & PHY_INTERRUPTS_SHARED > + && phydev->drv->interrupt_src) { > + src = phydev->drv->interrupt_src(phydev); > + if ((src == -1) || (src != phydev->addr)) > + goto ignore; > + } > + I don't agree with this approach to the problem. First, it would appear that phydev->drv->interrupt_src is redundant with PHY_INTERRUPTS_SHARED. So we probably don't need to add that bit. Next, the result of interrupt_src isn't being used to do anything but determine whether *this* PHY originated the interrupt. So a minor improvement would be to have a function ->did_interrupt(), which returns 1 if the PHY generated an interrupt, and 0 otherwise. For solving this problem, I'd prefer a solution that didn't appear to be so closely tied to the specific hardware you are using. The ->did_interrupt() method would allow any PHY with interrupts to report whether it believes it created the interrupt. It seems to me that this is just creating more overhead, though. There may be a way to consolidate the various interrupt-related functions, and create a new set which allows for this sort of "early-out". Another possibility to consider is some way that each "global" device could only read this register once per interrupt, and report that status through the phy_device struct(s). Maybe we need some sort of container for multi-PHY devices? Andy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip 2009-02-16 20:53 [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip Anatolij Gustschin 2009-02-16 20:53 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Anatolij Gustschin @ 2009-02-27 6:39 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2009-02-27 6:39 UTC (permalink / raw) To: agust; +Cc: netdev, sposelenov, yur From: Anatolij Gustschin <agust@denx.de> Date: Mon, 16 Feb 2009 21:53:49 +0100 > From: Sergei Poselenov <sposelenov@emcraft.com> > > Signed-off-by: Yuri Tikhonov <yur@emcraft.com> > Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com> > Signed-off-by: Anatolij Gustschin <agust@denx.de> This patch doesn't apply, and patches 2 and 3 are still under discussion. So I'm dropping all of them. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-02-27 6:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-02-16 20:53 [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip Anatolij Gustschin 2009-02-16 20:53 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Anatolij Gustschin 2009-02-16 20:53 ` [PATCH 3/3] phylib: shared interrupts support for Marvell 88E1121R PHY Anatolij Gustschin 2009-02-18 1:12 ` Andy Fleming 2009-02-18 0:47 ` [PATCH 2/3] phylib: add support for shared PHY interrupts Andy Fleming 2009-02-27 6:39 ` [PATCH 1/3] phylib: Basic support for the M88E1121R Marvell chip David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).