[U-Boot] [PATCH 1/4] Armada100: Ethernet support for Marvell gplugD
Ajay Bhargav
ajay.bhargav at einfochips.com
Fri Jul 8 10:17:44 CEST 2011
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 at denx.de>
To: "Ajay Bhargav" <ajay.bhargav at 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 at 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 at 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
More information about the U-Boot
mailing list