[U-Boot] [PATCH v2 03/12] net: add Faraday FTMAC110 10/100Mbps ethernet support

[Kuo-Jung Su]

2013/4/18 Wolfgang Denk <wd at denx.de>:
> Dear Kuo-Jung Su,
>
> In message <1366277139-29728-4-git-send-email-dantesu at gmail.com> you wrote:
>>
>> +/*******************************************************************/
>> +/*               FTMAC110 DMA design issue                         */
>> +/*                                             Dante Su 2010.02.03 */
>> +/*                                                                 */
>> +/* The DMA engine has a weird restriction that its Rx DMA engine   */
>> +/* accepts only 16-bits aligned address, 32-bits aligned is not    */
>> +/* acceptable. However this restriction does not apply to Tx DMA.  */
>> +/* Conclusion:                                                     */
>> +/* (1) Tx DMA Buffer Address:                                      */
>> +/*     1 bytes aligned: Invalid                                    */
>> +/*     2 bytes aligned: O.K                                        */
>> +/*     4 bytes aligned: O.K (-> u-boot ZeroCopy is possible)       */
>> +/* (2) Rx DMA Buffer Address:                                      */
>> +/*     1 bytes aligned: Invalid                                    */
>> +/*     2 bytes aligned: O.K                                        */
>> +/*     4 bytes aligned: Invalid                                    */
>> +/*******************************************************************/
>
> Incorrect multi-line comment style. Please fix globally.
>

Got it, thanks

>> +/* Register access macros */
>> +#define MAC_READ(r)                  le32_to_cpu(readl(r))
>> +#define MAC_WRITE(v, r)              writel(cpu_to_le32(v), r)
>
> Please drop these and use the real functions instead.
>

Got it, thanks.

>> +static char ftmac110_mac_addr[] = { 0x00, 0x41, 0x71, 0x00, 0x00, 0x52 };
>
> NAK.  We do not allow static configuration of MAC addresses.
>

Got it, thanks.

>> +     ulong ts;
>> +     uint32_t maccr;
>> +     uint16_t pa, tmp;
>
> Are you sure using  uint16_t  gives more efficient code than a plain
> int?
>

No, it won't.
It' would be fixed latter.

>> +     if (pa >= 32) {
>> +             puts("ftmac110: phy device not found!\n");
>
> make this a debug() ?
>
> ...

Got it, thanks

>> +     MAC_WRITE(0x00001010, &regs->itc);
>> +     MAC_WRITE(0x00000001, &regs->aptc);
>> +     MAC_WRITE(0x00000390, &regs->dblac);
>> +     MAC_WRITE(0x000003FF, &regs->isr);
>
> What do these magic numbers mean?
>
>

Got it, thanks.
I'll add comments to these timing control registers

>> +struct ftmac110_rxd {
>> +     /* RXDES0 */
>> +#ifdef __ARMEB__
>> +     uint32_t owner:1; /* BIT: 31 - 1:Hardware, 0: Software */
>> +     uint32_t rsvd3:1;
>> +     uint32_t frs:1;
>> +     uint32_t lrs:1;
>> +     uint32_t rsvd2:5;
>> +     uint32_t error:5;
>> +     uint32_t bcast:1;
>> +     uint32_t mcast:1;
>> +     uint32_t rsvd1:5;
>> +     uint32_t len:11;
>> +#else
>> +     uint32_t len:11;
>> +     uint32_t rsvd1:5;
>> +     uint32_t mcast:1;
>> +     uint32_t bcast:1;
>> +     uint32_t error:5;
>> +     uint32_t rsvd2:5;
>> +     uint32_t lrs:1;
>> +     uint32_t frs:1;
>> +     uint32_t rsvd3:1;
>> +     uint32_t owner:1; /* BIT: 31 - 1:Hardware, 0: Software */
>> +#endif
>
> NAK.  Please do NOT use bit fields.  They are non-portable and
> dangerous.
>
>

Got it, thanks

> 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
> A year spent in artificial intelligence is enough to make one believe
> in God.



--
Best wishes,
Kuo-Jung Su