Re: [RFC PATCH] net:Add basic DWC Ethernet QoS Driver

[Andreas Irestål]

> > Signed-off-by: Andreas Irestaal <Andreas.Irestal@xxxxxxxx>
> > ---
> >  drivers/net/ethernet/Kconfig                |    1 +
> >  drivers/net/ethernet/Makefile               |    1 +
> >  drivers/net/ethernet/synopsys/Kconfig       |   24 +
> >  drivers/net/ethernet/synopsys/Makefile      |    5 +
> >  drivers/net/ethernet/synopsys/dwc_eth_qos.c |  710 +++++++++++++++++++++++++++
> >  drivers/net/ethernet/synopsys/dwc_eth_qos.h |  308 ++++++++++++
> 
> Should we maybe move the dw_mac driver out of the stmicro directory into
> synopsys as a preparation, to keep them in the same place?
> 

That sounds like a good idea since that driver uses the same IP. I'm not the
person to take that decision though.

> > +
> > +	/* Set poll wait timeout to 2 seconds */
> > +	dwc_wait = 200;
> > +
> > +	while (lp->tx_descs[i].tdes3.wr.own) {
> > +		mdelay(10);
> > +		if (!dwc_wait--)
> > +			break;
> > +	}
> 
> This is really evil: you are blocking the CPU for up to two seconds!
> You already mentioned that this is work-in-progress, but I guess it has
> to be a little better than this and do something that doesn't block
> out the CPU during TX.
> 

It really is, but a 2s lockout is only happening upon TX failure. Anyway, this
won't be an issue in the final version, since it won't use polling for TX.

> 
> > +/* Fields/constants */
> > +#define DWCEQOS_DMA_MODE_SWR			1
> > +#define DWCEQOS_DMA_MODE_TXPR			(1 << 11)
> > +#define DWCEQOS_DMA_MODE_DA			(1 << 1)
> > +#define DWCEQOS_DMA_SYSBUS_MB			(1 << 14)
> 
> I find it more readable to use hexadecimal notation here
> 
> #define DWCEQOS_DMA_MODE_SWR		0x0001
> #define DWCEQOS_DMA_MODE_DA		0x0002
> #define DWCEQOS_DMA_MODE_TXPR		0x0800
> #define DWCEQOS_DMA_SYSBUS_MB		0x4000
> 

I agree, but it's not 100% clear which one is preferred as there exist drivers
using the first notation style. Anyway, I'll use the hex notation in future
patches since you prefer it.

Thanks for all valuable input. This was exactly the kind of feedback i was looking for

/Andreas


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/