[U-Boot] [PATCH 1/4] Armada100: Ethernet support for Marvell gplugD

[Ajay Bhargav <ajay.bhargav@einfochips.com>]

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 at marvell.com, u-boot at 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Don't worry about people stealing your ideas. If your ideas are  any
good, you'll have to ram them down people's throats."  - Howard Aiken