Dear Prafulla Wadaskar,
In message <1238798370-9245-3-git-send-email-prafu...@marvell.com> you wrote:
> From: prafulla_wadaskar <prafu...@marvell.com>
>
> Contributors:
> Yotam Admon <yo...@marvell.com>
> Michael Blostein <michae...@marvell.com
>
> Signed-off-by: prafulla_wadaskar <prafu...@marvell.com>
> Reviewed by: Ronen Shitrit <rshit...@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: w...@denx.de
I've got a bad feeling about this.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
