Re: [PATCH net-next v1 1/4] net: phy: mxl-gpy: add MDINT workaround

[Michael Walle]

Am 2022-12-02 19:23, schrieb Andrew Lunn:
> On Fri, Dec 02, 2022 at 04:12:01PM +0100, Michael Walle wrote:
> > At least the GPY215B and GPY215C has a bug where it is still driving 
> > the
> > interrupt line (MDINT) even after the interrupt status register is 
> > read
> > and its bits are cleared. This will cause an interrupt storm.
> >
> > Although the MDINT is multiplexed with a GPIO pin and theoretically we
> > could switch the pinmux to GPIO input mode, this isn't possible 
> > because
> > the access to this register will stall exactly as long as the 
> > interrupt
> > line is asserted. We exploit this very fact and just read a random
> > internal register in our interrupt handler. This way, it will be 
> > delayed
> > until the external interrupt line is released and an interrupt storm 
> > is
> > avoided.
> >
> > The internal register access via the mailbox was deduced by looking at
> > the downstream PHY API because the datasheet doesn't mention any of
> > this.
> >
> > Signed-off-by: Michael Walle <michael@xxxxxxxx>
> > ---
> >  drivers/net/phy/mxl-gpy.c | 83 
> > +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >
> > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> > index 0ff7ef076072..20e610dda891 100644
> > --- a/drivers/net/phy/mxl-gpy.c
> > +++ b/drivers/net/phy/mxl-gpy.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/module.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/hwmon.h>
> > +#include <linux/mutex.h>
> >  #include <linux/phy.h>
> >  #include <linux/polynomial.h>
> >  #include <linux/netdevice.h>
> > @@ -81,6 +82,14 @@
> >  #define VSPEC1_TEMP_STA	0x0E
> >  #define VSPEC1_TEMP_STA_DATA	GENMASK(9, 0)
> >
> > +/* Mailbox */
> > +#define VSPEC1_MBOX_DATA	0x5
> > +#define VSPEC1_MBOX_ADDRLO	0x6
> > +#define VSPEC1_MBOX_CMD		0x7
> > +#define VSPEC1_MBOX_CMD_ADDRHI	GENMASK(7, 0)
> > +#define VSPEC1_MBOX_CMD_RD	(0 << 8)
> > +#define VSPEC1_MBOX_CMD_READY	BIT(15)
> > +
> >  /* WoL */
> >  #define VPSPEC2_WOL_CTL		0x0E06
> >  #define VPSPEC2_WOL_AD01	0x0E08
> > @@ -88,7 +97,15 @@
> >  #define VPSPEC2_WOL_AD45	0x0E0A
> >  #define WOL_EN			BIT(0)
> >
> > +/* Internal registers, access via mbox */
> > +#define REG_GPIO0_OUT		0xd3ce00
> > +
> >  struct gpy_priv {
> > +	struct phy_device *phydev;
> > +
> > +	/* serialize mailbox acesses */
> > +	struct mutex mbox_lock;
> > +
>
> >  static int gpy_probe(struct phy_device *phydev)
> >  {
> >  	struct device *dev = &phydev->mdio.dev;
> > @@ -228,7 +286,9 @@ static int gpy_probe(struct phy_device *phydev)
> >  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> >  		return -ENOMEM;
> > +	priv->phydev = phydev;
>
> I don't think you use this anywhere. Maybe in one of the following
> patches?

Arg. Yes, it's an leftover from when I was using a workqueue to
reenable the interrupts again.

Any opinion whether this patch should be net or net-next?

-michael