[U-Boot] [PATCH 1/4] Armada100: Ethernet support for Marvell gplugD
Ajay Bhargav
ajay.bhargav at einfochips.com
Fri Jul 8 11:32:17 CEST 2011
Dear Wolfgang,
I need some more clarification on the GPIO part. Most of the architectures i see define their own way of working with GPIO. There is no generic framework for GPIO as i see. I followed similar style used in drivers/gpio/kw_gpio.c they however used different names so its not so easy to make out that they are actually GPIO registers.
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
--
This message has been scanned for viruses and
dangerous content by Clean Mail Gateway, and is
believed to be clean.
*************************************************************************************************************************************************************
einfochips Business Disclaimer : This e-mail message and all attachments transmitted with it are intended solely for the use of the addressee and may contain legally privileged and confidential information. If the reader of this message is not the intended recipient, or an employee or agent responsible for delivering this message to the intended recipient, you are hereby notified that any dissemination, distribution, copying, or other use of this message or its attachments is strictly prohibited. If you have received this message in error, please notify the sender immediately by replying to this message and please delete it from your computer. Any views expressed in this message are those of the individual sender unless otherwise stated. Company has taken enough precautions to prevent the spread of viruses. However the company accepts no liability for any damage caused by any virus transmitted by this email.
*************************************************************************************************************************************************************
More information about the U-Boot
mailing list