[U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

>>>>>> +
>>>>>> +/* Axi Ethernet registers offset */
>>>>>> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
>>>>>> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
>>>>>> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
>>>>>> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
>>>>>> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
>>>>>> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
>>>>>> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
>>>>>> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data
>>>>>> */ +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read
>>>>>> Data */
>>>>> Please use struct xae_regs {...} as the rest of the u-boot.
>>>> struct is not useful here because it will be big with a lot of reserved
>>>> values.
>>> I see like 10 registers here, you can use "uint32_t reserved[n];"
>> + UAW0 and UAW1 with 0x700 offset.
>>
>> struct axi_ethernet {
>> 	u32 reserved[3];
>> 	u32 is; /* 0xC: Interrupt status */
>> 	u32 reserved2;
>> 	u32 ie; /* 0x14: Interrupt enable */
>> 	u32 reserved3[251];
>> 	u32 rcw1; /* 0x404: Rx Configuration Word 1 */
>> 	u32 tc; /* 0x408: Tx Configuration */
>> 	u32 reserved4;
>> 	u32 emmc; /* 0x410: EMAC mode configuration */
>> 	u32 reserved5[59];
>> 	u32 mdio_mc; /* 0x500: MII Management Config */
>> 	u32 mdio_mcr; /* 0x504: MII Management Control */
>> 	u32 mdio_mwd; /* 0x508: MII Management Write Data */
>> 	u32 mdio_mrd; /* 0x50C: MII Management Read Data */
>> 	u32 reserved6[124];
>> 	u32 uaw0; /* 0x700: Unicast address word 0 */
>> 	u32 uaw1; /* 0x704: Unicast address word 1 */
>> };
>>
>> Are you really sure that this is nice?
>>
> 
> Yep, definitelly great.

ok.

> 
>>>>>> +
>>>>>> +/* Link setup */
>>>>>> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
>>>>>> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 
> Mbit
>>>>>> */ +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100
>>>>>> Mbit */ +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask
>>>>>> for 1000 Mbit */ +
>>>>> Use (1 << n) ?
>>>> just another solution - I prefer to use 32bit value - easier when you
>>>> decode it by md.
>>> It's hard to read, really.
>> At offset 40b40410. It is really hard to decode it if values are (1 << n).
>> With defined macro you directly see that this is on 100Mbit/s LAN.
>>
>> U-Boot-mONStR> md 40b40400
>> 40b40400: ddccbbaa 6800ffee 4a000000 60000000    .......h...J...`
>> 40b40410: 40000000 00000000 000ce000 000ce000    ... at ............
>> 40b40420: 000ce000 000ce000 000ce000 000ce000    ................
>> 40b40430: 000ce000 000ce000 000ce000 000ce000    ................
>>
>> I am fine to use 1 << n solution but definitely in our repo I will use in
>> way I like.
> 
> Well I see it both ways ... 0x40000000 == 1 << 30 ... it's the same thing. On 
> the other note, it's hard to count the zeroes in there AND you can mistake 0 and 
> 8 in a huge series of those.
> 
> Also, you can have whatever you want in your repo if you seriously care to 
> invest the energy into maintaining it just because you need to be stubborn. But 
> it'd really be great if you invested that energy in a more productive manner ;-)

There are two points of view. And both have con & pro.
I don't want to argue. Net custodian should decided if is OK or not.

Look at tsec.h and probably others.


> 
>>>>>> +/* Interrupt Status/Enable/Mask Registers bit definitions */
>>>>>> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
>>>>>> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
>>>>>> +
>>>>> [...]
>>>> same here..
> 
> Yep, 1 << 3 and 1 << 7.
> 
>>>>>> +#define DMAALIGN	128
>>>>>> +
>>>>>> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
>>>>> Don't use cammelcase, all lowcase please.
>>>> Agree - will be done in v2.
>>>>
>>>>   Also, can't you allocate this with
>>>>> memalign and hide it in axidma_priv or something ?
>>>> There are two things in one.
>>>> 1. Adding it to axidma_priv means that I will need to dynamically
>>>> allocate big private structure which is bad thing to do for several eth
>>>> devices. This is FPGA you can create a lot of MACs that's why I think
>>>> it is better not to do so.
>>>> 2. Current style is sharing this rxframe buffer among all controllers
>>>> because only one MAC works. Others are stopped which means that no
>>>> packet come to them.
>>> Ok, I don't think I understand pt. 1. -- you need to keep the state for
>>> each of the ethernet devices on the FPGA separate, don't you.
>> Just the whole private structure with addresses, + phy.
>>
> 
> Ok, now after reading your explanation for pt.2 below, I'm starting to follow.
> 
>>   As for pt. 2. --
>>
>>> "currently", so there's possibility, in future this won't hold?
>> BTW: I am also sharing rx/tx buffer descriptors for dma.
>>
>> When do you expect that u-boot will be able to use several MACs in one
>> time?
> 
> It's not a matter of when, but -- write a correct code, it's much less burden to 
> fix it later.

Agree in general.
It is always question of when. You can always do it in better way. The question is if
someone will pay you for doing it in better way. If this feature is not important
for us, make no sense to invest our time/money to it.


>>>>>> +	/* Write new speed setting out to Axi Ethernet */
>>>>>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
>>>>> Use clrsetbits() here.
>>>> Not defined for Microblaze - just for ARM/PPC.
>>>> Not going to use it.
>>> Please fix then. You're the microblaze maintainer, right?
>> Custodian.
> 
> Oh come on ...
> 
>> But I won't do that.
> 
> I think you should.
> 
>> If you think that all archs should have it then move it to generic location
>> which clean code duplication and I will include it.
> 
> That's not the point, it's platform specific.

Just compare ppc/arm implementation and they are the same. It is even pure generic code.
Adding it to microblaze is just code duplication which is also not good way to go.
Then in future someone will move it to generic location.
There were a lot of examples in linux kernel and includes.

+ network driver should be platform independent which is exactly how it is written.
Writing in the pure C should be fine.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian