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

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1238798370-9245-3-git-send-email-prafulla at marvell.com> you wrote:
> From: prafulla_wadaskar <prafulla at marvell.com>
> 
> Contributors:
> Yotam Admon <yotam at marvell.com>
> Michael Blostein <michaelbl at marvell.com
> 
> Signed-off-by: prafulla_wadaskar <prafulla at marvell.com>
> Reviewed by: Ronen Shitrit <rshitrit at marvell.com>
...
> diff --git a/cpu/arm926ejs/kirkwood/Makefile b/cpu/arm926ejs/kirkwood/Makefile
> index 41ac8d7..3e20898 100644
> --- a/cpu/arm926ejs/kirkwood/Makefile
> +++ b/cpu/arm926ejs/kirkwood/Makefile
> @@ -30,6 +30,7 @@ COBJS-y	= timer.o
>  COBJS-y	+= serial.o
>  COBJS-y	+= kwcore.o
>  COBJS-y	+= dram.o
> +COBJS-y	+= egiga.o

Please sort such lists.

...
> +int eth_smi_reg_read(u32 eth_port_num, u32 phy_adr, u32 reg_ofs, u16 * data)
> +{
> +	u32 smi_reg;
> +	volatile u32 timeout;
> +
> +	/* check parameters */
> +	if ((phy_adr << ETH_PHY_SMI_DEV_ADDR_OFFS) & ~ETH_PHY_SMI_DEV_ADDR_MASK) {

Line too long. (Hint: consider using shorter names! :-)

...
> +	/* fill the phy address and regiser offset and read opcode */
> +	smi_reg =
> +	    (phy_adr << ETH_PHY_SMI_DEV_ADDR_OFFS) | (reg_ofs <<
> +						      KW_ETH_SMI_REG_ADDR_OFFS)
> +	    | ETH_PHY_SMI_OPCODE_READ;

Please clean up indentation here.

...
> +static int egiga_free_rx_rings(struct eth_device *dev)
> +{
> +	u32 queue;
> +	ETH_PORT_INFO *ethernet_private;
> +	struct egiga_priv *port_private;
> +	u32 port_num;
> +	volatile ETH_RX_DESC *p_rx_curr_desc;
> +
> +	ethernet_private = (ETH_PORT_INFO *) dev->priv;
> +	port_private = (struct egiga_priv *)ethernet_private->port_private;
> +	port_num = port_private->port_num;
> +
> +	/* Stop RX Queues */
> +	KW_REG_WRITE(KW_ETH_RECEIVE_QUEUE_COMMAND_REG(port_num), 0x0000ff00);
> +
> +	/* Free RX rings */
> +	debug_print("Clearing previously allocated RX queues... ");
> +	for (queue = 0; queue < KW_RX_QUEUE_NUM; queue++) {
> +		/* Free preallocated skb's on RX rings */
> +		for (p_rx_curr_desc =
> +		     ethernet_private->p_rx_desc_area_base[queue];
> +		     (((u32)p_rx_curr_desc <
> +		       ((u32)ethernet_private->
> +			p_rx_desc_area_base[queue] +
> +			ethernet_private->rx_desc_area_size[queue])));
> +		     p_rx_curr_desc =
> +		     (ETH_RX_DESC *) ((u32)p_rx_curr_desc +
> +				      RX_DESC_ALIGNED_SIZE)) {
> +			if (p_rx_curr_desc->return_info != 0) {
> +				p_rx_curr_desc->return_info = 0;
> +				debug_print("freed");
> +			}

... and here. Such code is basicly unreadable.

...
> +static ETH_FUNC_RET_STATUS eth_port_send(ETH_PORT_INFO * p_eth_port_ctrl,
> +					 ETH_QUEUE tx_queue,
> +					 PKT_INFO * p_pkt_info)
> +{
> +	volatile ETH_TX_DESC *p_tx_desc_first;
> +	volatile ETH_TX_DESC *p_tx_desc_curr;
> +	volatile ETH_TX_DESC *p_tx_next_desc_curr;
> +	volatile ETH_TX_DESC *p_tx_desc_used;
> +	u32 command_status;
> +
> +#ifdef CONFIG_TX_PKT_DISPLAY
> +	{
> +		u16 pcnt = p_pkt_info->byte_cnt;
> +		u8 *prndt = (char *)p_pkt_info->buf_ptr;
> +		printf("cnt=%d ", pcnt);

Blank line after declarations, please (check gloabally, please).

Also, pelase try to acoid such nested blocks with additional
declarations.

> +		while (pcnt) {
> +			printf("%02x,", prndt[0]);
> +			prndt++;
> +			pcnt--;
> +		}
> +		printf(" pckend \n");
> +
> +	}
> +#endif
...

> diff --git a/cpu/arm926ejs/kirkwood/egiga.h b/cpu/arm926ejs/kirkwood/egiga.h
> new file mode 100644
> index 0000000..83dc4b3
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/egiga.h
> @@ -0,0 +1,707 @@
...

> +#ifndef TRUE
> +#define TRUE 1
> +#endif
> +#ifndef FALSE
> +#define FALSE 0
> +#endif

Omit?

...
> +struct net_device_stats {
> +	u32 rx_packets;	/* total packets received       */
> +	u32 tx_packets;	/* total packets transmitted    */
> +	u32 rx_bytes;	/* total bytes received         */
> +	u32 tx_bytes;	/* total bytes transmitted      */
> +	u32 rx_errors;	/* bad packets received         */
> +	u32 tx_errors;	/* packet transmit problems     */
> +	u32 rx_dropped;	/* no space in linux buffers    */
> +	u32 tx_dropped;	/* no space available in linux  */
> +	u32 multicast;	/* multicast packets received   */
> +	u32 collisions;
> +	/* detailed rx_errors: */
> +	u32 rx_length_errors;
> +	u32 rx_over_errors;	/* receiver ring buff overflow  */
> +	u32 rx_crc_errors;	/* recved pkt with crc error    */
> +	u32 rx_frame_errors;	/* recv'd frame alignment error */
> +	u32 rx_fifo_errors;	/* recv'r fifo overrun          */
> +	u32 rx_missed_errors;	/* receiver missed packet       */
> +	/* detailed tx_errors */
> +	u32 tx_aborted_errors;
> +	u32 tx_carrier_errors;
> +	u32 tx_fifo_errors;
> +	u32 tx_heartbeat_errors;
> +	u32 tx_window_errors;
> +	/* for cslip etc */
> +	u32 rx_compressed;
> +	u32 tx_compressed;
> +};
> +
> +/* Private data structure used for ethernet device */
> +struct egiga_priv {
> +	u32 port_num;
> +	struct net_device_stats *stats;
> +	/* to buffer area aligned */
> +	char *p_eth_tx_buffer[KW_TX_QUEUE_SIZE + 1];
> +	char *p_eth_rx_buffer[KW_RX_QUEUE_SIZE + 1];
> +	/* Size of Tx Ring per queue */
> +	u32 tx_ring_size[MAX_TX_QUEUE_NUM];
> +	/* Size of Rx Ring per queue */
> +	u32 rx_ring_size[MAX_RX_QUEUE_NUM];
> +	/* Magic Number for Ethernet running */
> +	u32 eth_running;
> +};

Please use TABs for vertical alignment to make the code readable.

...
> +/* Gap define */
> +#define ETH_BAR_GAP			0x8
> +#define ETH_SIZE_REG_GAP		0x8
> +#define ETH_HIGH_ADDR_REMAP_REG_GAP	0x4
> +#define ETH_PORT_ACCESS_CTRL_GAP	0x4
> +/* MIB Counters register definitions */
> +#define ETH_MIB_GOOD_OCTETS_RECEIVED_LOW   0x0
> +#define ETH_MIB_GOOD_OCTETS_RECEIVED_HIGH  0x4
> +#define ETH_MIB_BAD_OCTETS_RECEIVED	   0x8
> +#define ETH_MIB_INTERNAL_MAC_TRANSMIT_ERR  0xc
> +#define ETH_MIB_GOOD_FRAMES_RECEIVED	   0x10
> +#define ETH_MIB_BAD_FRAMES_RECEIVED	   0x14
> +#define ETH_MIB_BROADCAST_FRAMES_RECEIVED  0x18
> +#define ETH_MIB_MULTICAST_FRAMES_RECEIVED  0x1c
... and so on:

Please use TABs for vertical alignment to make the code better readable.

This applies to the rest of file, too.

> diff --git a/cpu/arm926ejs/kirkwood/egiga_regs.h b/cpu/arm926ejs/kirkwood/egiga_regs.h
> new file mode 100644
> index 0000000..a24fa04
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/egiga_regs.h
...
> +/*
> + *Ethernet Controller Registers
> + */
> +#define KW_ETH_PHY_ADDR_REG(port)			(0x72000 + (port<<14))
> +#define KW_ETH_SMI_REG(port)				(0x72004 + (port<<14))
> +#define KW_ETH_UNIT_DEFAULT_ADDR_REG(port)		(0x72008 + (port<<14))
> +#define KW_ETH_UNIT_DEFAULTID_REG(port)			(0x7200c + (port<<14))
> +#define KW_ETH_UNIT_INTERRUPT_CAUSE_REG(port)		(0x72080 + (port<<14))
> +#define KW_ETH_UNIT_INTERRUPT_MASK_REG(port)		(0x72084 + (port<<14))
...

Use C structures to describe the device layout, and you can get rid of
this ugly code.


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
I've got a bad feeling about this.