Dear Wolfgang Denk,

Thank you so much for your comments. I will do the changes carefully.

Thanks & Regards, Ajay Bhargav

----- Original Message ----- From: "Wolfgang Denk" wd@denx.de To: "Ajay Bhargav" ajay.bhargav@einfochips.com Cc: prafulla@marvell.com, u-boot@lists.denx.de Sent: Friday, July 8, 2011 1:17:50 PM Subject: Re: [U-Boot] [PATCH 1/4] Armada100: Ethernet support for Marvell gplugD

Dear Ajay Bhargav,

In message 1310106168-17166-1-git-send-email-ajay.bhargav@einfochips.com you wrote:

This patch adds ethernet support for GuruPlug-Display. tftpboot and and other network related commands now works.

Signed-off-by: Ajay Bhargav ajay.bhargav@einfochips.com

.gitignore | 1 + arch/arm/include/asm/arch-armada100/armada100.h | 39 ++ arch/arm/include/asm/arch-armada100/gpio.h | 81 +++ arch/arm/include/asm/arch-armada100/mfp.h | 19 + board/Marvell/gplugd/gplugd.c | 77 +++ drivers/net/Makefile | 1 + drivers/net/pxa168_eth.c | 815 +++++++++++++++++++++++ drivers/net/pxa168_eth.h | 239 +++++++ include/configs/gplugd.h | 22 +- include/netdev.h | 1 + 10 files changed, 1293 insertions(+), 2 deletions(-) create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h create mode 100644 drivers/net/pxa168_eth.c create mode 100644 drivers/net/pxa168_eth.h

diff --git a/.gitignore b/.gitignore index 8ec3d06..806ab29 100644 --- a/.gitignore +++ b/.gitignore @@ -43,6 +43,7 @@

/include/generated/ /lib/asm-offsets.s +/include/configs/tags

This change is unrelated. Please submit separately.

# stgit generated dirs patches-* diff --git a/arch/arm/include/asm/arch-armada100/armada100.h b/arch/arm/include/asm/arch-armada100/armada100.h index d5d125a..b21e476 100644 --- a/arch/arm/include/asm/arch-armada100/armada100.h +++ b/arch/arm/include/asm/arch-armada100/armada100.h @@ -60,6 +60,9 @@ #define ARMD1_APMU_BASE 0xD4282800 #define ARMD1_CPU_BASE 0xD4282C00

+#define FEC_CLK_ADR 0xD42828FC

Only one blank line, please, at places like that.

--- /dev/null +++ b/arch/arm/include/asm/arch-armada100/gpio.h @@ -0,0 +1,81 @@ +/**************************************************************************

• • Copyright (c) 2009, 2010 Marvell International Ltd.

...

Incorrect multiline comment style.

+#define GPIO_bit(gpio) (1 << ((gpio) & 0x1f))

Macro names must be all caps.

...

+#define GPLR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x00) +#define GPDR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x0c) +#define GPSR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x18) +#define GPCR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x24) +#define GSDR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x54) +#define GCDR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x60)

Please use a C struct to dsescribe the register layout.

+#define GPIO_SET 1 +#define GPIO_CLR 0 + +static inline int gpio_get_value(unsigned gpio)

Don't we have a generic GPIO framework we should use here?

diff --git a/board/Marvell/gplugd/gplugd.c b/board/Marvell/gplugd/gplugd.c index dc7d89d..81770fc 100644 --- a/board/Marvell/gplugd/gplugd.c +++ b/board/Marvell/gplugd/gplugd.c @@ -32,9 +32,24 @@ #include <mvmfp.h> #include <asm/arch/mfp.h> #include <asm/arch/armada100.h> +#include <asm/arch/gpio.h> +#include <miiphy.h>

+#ifdef CONFIG_PXA_ETH +#include <net.h> +#include <netdev.h> +#endif /* CONFIG_PXA_ETH */

DECLARE_GLOBAL_DATA_PTR;

+#define PHY_LED_PAR_SEL_REG 22 +#define PHY_LED_MAN_REG 25 +#define PHY_LED_VAL 0x5b /* LINK LED1, ACT LED2 */ +#define PHY_RST 104 /* GPIO104 - PHY RESET */ +#define RTC_IRQ 102 /* GPIO102 - RTC IRQ Input */ +#define FE_CLK_RST 0x1 +#define FE_CLK_ENA 0x8

Hm... is this cource file the right place for such #defines? Probably some should go into global hader files, other into the board config file?

diff --git a/drivers/net/pxa168_eth.c b/drivers/net/pxa168_eth.c new file mode 100644 index 0000000..f5251e9 --- /dev/null +++ b/drivers/net/pxa168_eth.c

...

This driver looks very much similar to what we already have in drivers/net/mvgbe.c.

Instead of re-implementing the same code again and again, please come up with a unified version.

diff --git a/drivers/net/pxa168_eth.h b/drivers/net/pxa168_eth.h new file mode 100644 index 0000000..29c8673 --- /dev/null +++ b/drivers/net/pxa168_eth.h

...

+struct pxa_reg {

• u32 phyadr;
• u8 pad1[0x010 - 0x00 - 4];
• u32 smi;
• u8 pad2[0x400 - 0x010 - 4];
• u32 pconf;
• u8 pad3[4];
• u32 pconf_ext;
• u8 pad4[4];
• u32 pcmd;
• u8 pad5[4];
• u32 pstatus;
• u8 pad6[4];
• u32 spar;
• u8 pad7[4];
• u32 htpr;
• u8 pad8[4];
• u32 fcsal;
• u8 pad9[4];
• u32 fcsah;
• u8 pad10[4];
• u32 sdma_conf;
• u8 pad11[4];
• u32 sdma_cmd;
• u8 pad12[4];
• u32 ic;
• u32 iwc;
• u32 im;
• u8 pad13[4];
• u32 *eth_idscpp[4];
• u32 eth_vlan_p;
• u8 pad14[0x480 - 0x470 - 4];
• struct rx_desc *rxfdp[4];
• u8 pad15[0x4a0 - 0x48c - 4];
• struct rx_desc *rxcdp[4];
• u8 pad16[0x4e0 - 0x4ac - 4];
• struct tx_desc *txcdp[2];

+};

Would it not be nice to have a few comments here what the entries are?

+struct pxa_device {

• struct eth_device dev;
• struct pxa_reg *regs;
• struct tx_desc *p_txdesc;
• struct rx_desc *p_rxdesc;
• struct rx_desc *p_rxdesc_curr;
• u8 *p_rxbuf;
• u8 *p_aligned_txbuf;
• u8 *htpr; /* hash pointer */

+};

Again, this should porobably be unified with drivers/net/mvgbe.h

+#define CONFIG_IPADDR 192.168.9.51 +#define CONFIG_SERVERIP 192.168.9.91 +#define CONFIG_ETHADDR "F0:AD:4E:00:32:9C"

NAK.

We don't allow such static initializations of network parameters.

Best regards,

Wolfgang Denk