[U-Boot] [PATCH v4] Gbe Controller driver support for kirkwood SOCs

[Prafulla Wadaskar]

Thanks a lot Stefan for your review feedback...
You are thinking more similar to Linux Implementation :-)

I will try to address your suggestions, it provides more flexibility and readability.
On the other hand I am currently putting my efforts to reduce patch size cutting some features.
I will do it step-by-step.

Regards..
Prafulla . . .

> -----Original Message-----
> From: Stefan Roese [mailto:sr at denx.de] 
> Sent: Tuesday, May 26, 2009 8:26 PM
> To: u-boot at lists.denx.de
> Cc: Prafulla Wadaskar; Manas Saksena; Ronen Shitrit; Nicolas 
> Pitre; Ashish Karkare; Prabhanjan Sarnaik; Lennert Buijtenhek
> Subject: Re: [U-Boot] [PATCH v4] Gbe Controller driver 
> support for kirkwood SOCs
> 
> Hi Prafulla,
> 
> On Thursday 21 May 2009 22:24:32 Prafulla Wadaskar wrote:
> > Contributors:
> > Yotam Admon <yotam at marvell.com>
> > Michael Blostein <michaelbl at marvell.com
> >
> > Reviewed by: Ronen Shitrit <rshitrit at marvell.com>
> > Signed-off-by: Prafulla Wadaskar <prafulla at marvell.com>
> > ---
> > Change log:
> > v2: entire rewrite/restructure of v1
> > used small names for variable/function names readl/writel used to 
> > access SoC registers Soc registers accssed using pointers 
> through net 
> > device struct miiphy registration done for external smi read/write 
> > access miiphy_link used to detect phy link presence cleaned for 
> > cosmetic changes
> 
> Please find a comment below.
> 
> > diff --git a/drivers/net/kirkwood_egiga.h 
> > b/drivers/net/kirkwood_egiga.h new file mode 100644 index 
> > 0000000..9f3908e
> > --- /dev/null
> > +++ b/drivers/net/kirkwood_egiga.h
> > @@ -0,0 +1,828 @@
> > +/*
> > + * (C) Copyright 2009
> > + * Marvell Semiconductor <www.marvell.com>
> > + * Prafulla Wadaskar <prafulla at marvell.com>
> > + *
> > + * based on - Driver for MV64360X ethernet ports
> > + * Copyright (C) 2002 rabeeh at galileo.co.il
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General 
> Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> > + * MA 02110-1301 USA
> > + */
> > +
> > +#ifndef __EGIGA_H__
> > +#define __EGIGA_H__
> > +
> > +#define MAX_KWGBE_DEVS	2	/*controller has two ports */
> 
> <snip>
> 
> > +/* Default port serial control value */
> > +#define PORT_SERIAL_CONTROL_VALUE_TMP		  \
> > +	KWGBE_FORCE_LINK_PASS			| \
> > +	KWGBE_DIS_AUTO_NEG_FOR_DUPLX		| \
> > +	KWGBE_DIS_AUTO_NEG_FOR_FLOW_CTRL	| \
> > +	KWGBE_ADV_NO_FLOW_CTRL			| \
> > +	KWGBE_FORCE_FC_MODE_NO_PAUSE_DIS_TX	| \
> > +	KWGBE_FORCE_BP_MODE_NO_JAM		| \
> > +	(1 << 9)					| \
> > +	KWGBE_DO_NOT_FORCE_LINK_FAIL		| \
> > +	KWGBE_DIS_AUTO_NEG_SPEED_GMII		| \
> 
> You disable speed auto-negotiation here. Why are you doing 
> this? Using auto- negotiation should be the default operation 
> mode from my point of view.
> 
> > +	KWGBE_DTE_ADV_0				| \
> > +	KWGBE_MIIPHY_MAC_MODE			| \
> > +	KWGBE_AUTO_NEG_NO_CHANGE			| \
> > +	KWGBE_MAX_RX_PACKET_1552BYTE		| \
> > +	KWGBE_CLR_EXT_LOOPBACK			| \
> > +	KWGBE_SET_FULL_DUPLEX_MODE		| \
> > +	KWGBE_DIS_FLOW_CTRL_TX_RX_IN_FULL_DUPLEX | \
> > +	KWGBE_SET_MII_SPEED_TO_100
> > +
> > +#ifdef CONFIG_SYS_MII_MODE
> > +#define PORT_SERIAL_CONTROL_VALUE	\
> > +	PORT_SERIAL_CONTROL_VALUE_TMP |	\
> > +	KWGBE_SET_GMII_SPEED_TO_10_100
> > +#else
> > +#define PORT_SERIAL_CONTROL_VALUE	\
> > +	PORT_SERIAL_CONTROL_VALUE_TMP |	\
> > +	KWGBE_SET_GMII_SPEED_TO_1000
> > +#endif
> 
> If you need a fixed link speed configuration we should 
> perhaps add a configuration option for this. What do you think?
> 
> Best regards,
> Stefan
> 
> =====================================================================
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: 
> office at denx.de 
> =====================================================================
>