Re: [RFC PATCH v0.2] net driver: mpc52xx fec

[Domen Puncer <domen@coderock.org>]

On 02/10/07 14:49 +0200, Sascha Hauer wrote:
> 
> Hi Domen,

Hi Sascha!


> 
> On Sun, Sep 02, 2007 at 09:41:43AM +0200, Domen Puncer wrote:
>  + */
> > +static void fec_start(struct net_device *dev)
> > +{
> > +	struct fec_priv *priv = netdev_priv(dev);
> > +	struct mpc52xx_fec __iomem *fec = priv->fec;
> > +	u32 rcntrl;
> > +	u32 tcntrl;
> > +	u32 tmp;
> > +
> > +	/* clear sticky error bits */
> > +	tmp = FEC_FIFO_STATUS_ERR | FEC_FIFO_STATUS_UF | FEC_FIFO_STATUS_OF;
> > +	out_be32(&fec->rfifo_status, in_be32(&fec->rfifo_status) & tmp);
> > +	out_be32(&fec->tfifo_status, in_be32(&fec->tfifo_status) & tmp);
> > +
> > +	/* FIFOs will reset on fec_enable */
> > +	out_be32(&fec->reset_cntrl, FEC_RESET_CNTRL_ENABLE_IS_RESET);
> > +
> > +	/* Set station address. */
> > +	fec_set_paddr(dev, dev->dev_addr);
> > +
> > +	fec_set_multicast_list(dev);
> > +
> > +	/* set max frame len, enable flow control, select mii mode */
> > +	rcntrl = FEC_RX_BUFFER_SIZE << 16;	/* max frame length */
> > +	rcntrl |= FEC_RCNTRL_FCE;
> > +	rcntrl |= MII_RCNTL_MODE;
> > +	if (priv->duplex == DUPLEX_FULL)
> > +		tcntrl = FEC_TCNTRL_FDEN;	/* FD enable */
> > +	else {
> > +		rcntrl |= FEC_RCNTRL_DRT;	/* disable Rx on Tx (HD) */
> > +		tcntrl = 0;
> > +	}
> > +	out_be32(&fec->r_cntrl, rcntrl);
> > +	out_be32(&fec->x_cntrl, tcntrl);
> > +
> > +	/* Clear any outstanding interrupt. */
> > +	out_be32(&fec->ievent, 0xffffffff);
> > +
> > +	/* Enable interrupts we wish to service. */
> > +	out_be32(&fec->imask, FEC_IMASK_ENABLE);
> 
> 
> This disables phy interrupts.

Right, oops.

> 
> 
> > +static int fec_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> > +{
> > +	struct fec_mdio_priv *priv = bus->priv;
> > +	int tries = 100;
> > +
> > +	u32 request = FEC_MII_READ_FRAME;
> > +	request |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
> > +	request |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
> > +
> > +	out_be32(&priv->regs->mii_data, request);
> > +
> > +	/* wait for it to finish, this takes about 23 us on lite5200b */
> > +	while (priv->completed == 0 && tries--)
> > +		udelay(5);
> > +
> > +	priv->completed = 0;
> > +
> > +	if (tries == 0)
> > +		return -ETIMEDOUT;
> 
> This does not work as expected. When a timeout occurs tries is -1 not 0,
> so the test above will never trigger.
> Using --tries instead of tries-- reveals another bug. We get a timeout
> everytime now, because MII interrupts are accidently disabled in
> fec_start().

Oh, double bug made it work! ;-)


> 
> We cannot use a waitqueue or similar for waiting for the mii transfer
> because we are atomic here.
> A simple fix is provided below. It removes the need for the interrupt
> handler in the phy handling routines. Anyway, it might be better to fix
> the phy layer not to use atomic contexts, so this patch might not be the
> way to go.

Doh, looks like this was the problem with wq's, but I forgot to remove
them, when I "fixed" the code.

> 
> 
> Regards, 
>   Sascha
> 
>  +
> > +static int fec_mdio_probe(struct of_device *of, const struct of_device_id *match)
> > +{
> > +	struct device *dev = &of->dev;
> >
> >       [...]
> >
> > +	init_waitqueue_head(&priv->wq);
> 
> This waitqueue is never used. wake_up() is called in the interrupt
> handler, but noone ever sleeps on the queue.
> 
> 
> ---
>  drivers/net/fec_mpc52xx/fec.c     |    7 +---
>  drivers/net/fec_mpc52xx/fec_phy.c |   59 +++++++-------------------------------
>  2 files changed, 15 insertions(+), 51 deletions(-)

The patch looks ok to me.


	Domen