Re: [U-Boot] [PATCH v3 03/11] net: add Faraday FTMAC110 10/100Mbps ethernet support

2013-05-03 Thread Kuo-Jung Su 
2013/5/3 Tom Rini tr...@ti.com:
 On Fri, Apr 26, 2013 at 04:02:32PM +0800, Kuo-Jung Su wrote:

 From: Kuo-Jung Su dant...@faraday-tech.com
 [snip]
 + | (phyaddr  PHYCR_ADDR_SHIFT)
 + | (phyreg   PHYCR_REG_SHIFT)
 + | 0x3000;

 Magic number.

It's the HW debug function, it would be removed at next version.

 +
 + writel(tmp, regs-phycr);
 +
 + for (ts = get_timer(0); get_timer(ts)  1000; ) {

 Please define a TIMEOUT and use that insteadof 1000 all the time.


Got it, thanks

 [snip]
 + /* interrupt at every packet transmit/receive */
 + writel(0x1010, regs-itc);
 + /* tx/rx poll interval=5.12us; rx_poll_cnt=1 */
 + writel(0x0001, regs-aptc);
 + /* rx fifo: high=1536, low=512 */
 + writel(0x0390, regs-dblac);
 + /* clear all interrupt status */
 + writel(0x03FF, regs-isr);

 More magic numbers.  Please fix globally.  Thanks!


Got it, thanks

 --
 Tom



--
Best wishes,
Kuo-Jung Su
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 03/11] net: add Faraday FTMAC110 10/100Mbps ethernet support

2013-05-02 Thread Tom Rini 
On Fri, Apr 26, 2013 at 04:02:32PM +0800, Kuo-Jung Su wrote:

 From: Kuo-Jung Su dant...@faraday-tech.com
[snip]
 + | (phyaddr  PHYCR_ADDR_SHIFT)
 + | (phyreg   PHYCR_REG_SHIFT)
 + | 0x3000;

Magic number.

 +
 + writel(tmp, regs-phycr);
 +
 + for (ts = get_timer(0); get_timer(ts)  1000; ) {

Please define a TIMEOUT and use that insteadof 1000 all the time.

[snip]
 + /* interrupt at every packet transmit/receive */
 + writel(0x1010, regs-itc);
 + /* tx/rx poll interval=5.12us; rx_poll_cnt=1 */
 + writel(0x0001, regs-aptc);
 + /* rx fifo: high=1536, low=512 */
 + writel(0x0390, regs-dblac);
 + /* clear all interrupt status */
 + writel(0x03FF, regs-isr);

More magic numbers.  Please fix globally.  Thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot