[U-Boot] [PATCH 2/2] net: Add Xilinx LL Temac driver

[Michal Simek]

Hi Ben,

> Hi Michal,
>
> I'm really sorry for taking so long getting to this. Mostly little things:

it's ok you are busy guy. I removed uninteresting part of code.
I added all your reported things to u-boot-microblaze.git net branch.
Thanks for pull.

>> +#ifdef DEBUG
>> +/* read from phy */
>> +static void read_phy_reg (int phy_addr)
>> +{
>> +	int j, result;
>> +	printf("phy%d ",phy_addr);
>> +	for ( j = 0; j < 32; j++) {
>> +		result = xps_ll_temac_hostif_get(0, phy_addr, j);
>> +		printf("%d: 0x%x ", j, result);
>> +	}
>> +	puts("\n");
>> +}
>> +#endif
>> +
>>
> You only use this function in one place. Why not move this code to that
> place and use debug()


You are right that currently is this function use only once but that's for this
code. When will be new board with different phy, it is easy to use this function
in different place too. The second reason is that eth_init is long and added
this function to it just cause that function will be longer than now. It is
atomic function.


>> +static int phy_addr = -1;
>> +static int link = 0;
>> +
>> +/* setting ll_temac and phy to proper setting */
>> +static int xps_ll_temac_phy_ctrl(void)
>> +{
>> +	int i;
>> +	unsigned int result;
>> +	unsigned retries = 10;
>> +
>> +	if(link == 1)
>> +		return 1; /* link is setup */
>> +
>> +	/* wait for link up */
>> +	while (retries-- &&
>> +		((xps_ll_temac_hostif_get(0, phy_addr, 1) & 0x24) == 0x24))
>> +		;
>> +
>> +	if(phy_addr == -1) {
>> +		for(i = 31; i >= 0; i--) {
>> +			result = xps_ll_temac_hostif_get(0, i, 1);
>> +			if((result & 0x0ffff) != 0x0ffff) {
>> +#ifdef DEBUG
>> +				printf ("phy %x result %x\n", i, result);
>> +#endif

fixed debug here too.

>> +				phy_addr = i;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* get PHY id */
>> +	i = (xps_ll_temac_hostif_get(0, phy_addr, 2) << 16) | \
>> +		xps_ll_temac_hostif_get(0, phy_addr, 3);
>> +#ifdef DEBUG
>> +	printf("LL_TEMAC: Phy ID 0x%x\n", i);
>>
> Please use debug() instead

fixed.

>> +#endif
>> +
>> +#ifdef DEBUG
>> +	xps_ll_temac_hostif_set(0, 0, 0, 0x8000); /* phy reset */
>> +#endif
>> +	/* FIXME this part will be replaced by PHY lib */
>> +	/* s3e boards */
>> +	if (i == 0x7c0a3) {
>> +		xps_ll_temac_indirect_set(0, EMMC, 0x40000000); /* 100BASE-T/FD */
>> +		link = 1;
>> +		return 1;
>> +	}
>> +
>> +	/* Marwell 88e1111 id - ml50x */
>> +	if (i == 0x1410cc2) {
>> +		result = xps_ll_temac_hostif_get(0, phy_addr, 5);
>> +		if((result & 0x8000) == 0x8000) {
>> +			xps_ll_temac_indirect_set(0, EMMC, 0x80000000);
>> +			printf("1000BASE-T/FD\n");
>> +			link = 1;
>> +		} else if((result & 0x4000) == 0x4000) {
>> +			xps_ll_temac_indirect_set(0, EMMC, 0x40000000);
>> +			printf("100BASE-T/FD\n");
>> +			link = 1;
>> +		} else {
>> +			printf("Unsupported mode\n");
>> +			link = 0;
>> +		}
>> +		return 1;
>> +	}
>> +	return 0;
>> +}
>> +

>> +
>> +#ifdef FIFO_MODE
>> +void debugll(int count)
>> +{
>> +	printf ("%d fifo isr 0x%08x, fifo_ier 0x%08x, fifo_rdfr 0x%08x, fifo_rdfo
0x%08x fifo_rlr 0x%08x\n",count, ll_fifo->isr, \
>>
> This line's way too long.

Yes I know about. It was fixed in net branch.


>> +	ll_fifo->ier, ll_fifo->rdfr, ll_fifo->rdfo, ll_fifo->rlf);
>> +
>> +}
>> +

>> +
>> +#ifdef ETH_HALTING
>> +static int xps_ll_temac_halt(void)
>> +{
>> +	xps_ll_temac_indirect_set(0, RCW1, 0x00000000); /* Disable Receiver */
>> +	xps_ll_temac_indirect_set(0, TC, 0x00000000); /* Disable Transmitter */
>> +
>> +#ifdef SDMA_MODE
>> +	out_be32((u32 *)DMA_CONTROL_REG, 0x00000001);
>> +	while(in_be32((u32 *)DMA_CONTROL_REG) & 1);
>> +#endif
>> +#ifdef FIFO_MODE
>> +	/* reset fifos */
>> +#endif
>> +}
>> +#endif
>> +
>> +/* halt device */
>> +void eth_halt(void){
>> +	link = 0;
>> +#ifdef ETH_HALTING
>> +	xps_ll_temac_halt();
>> +#endif
>> +}
>> +
>> +int eth_init(bd_t *bis)
>> +{
>> +	static int first = 1;
>> +	struct eth_device *dev;
>> +	struct xps_ll_temac_private *lp;
>> +#if DEBUG
>> +	int i;
>> +#endif
>> +
>>
> I normally don't like this, but move the variable declaration to where
> it's used to avoid 2 #ifs

I don't like it too but ok.

>> +	if(!first)
>> +		return 0;
>> +	first = 0;
>> +	dev = (struct eth_device *) calloc(1, sizeof(struct eth_device));
>> +	if (NULL == dev)
>> +		return 0;
>> +
>> +	lp = (struct xps_ll_temac_private *) calloc(1, sizeof(struct
xps_ll_temac_private));
>> +	if (lp == NULL)
>> +		return 0;
>> +	dev->priv = lp;
>> +	sprintf(dev->name, "eth0");
>> +
>> +	xps_ll_temac_init(dev, bis);
>> +
>> +	printf("%s: Xilinx XPS LocalLink Tri-Mode Ether MAC #%d at 0x%08X.\n",
>> +		dev->name, 0, XILINX_LLTEMAC_BASEADDR);
>> +
>> +#if DEBUG
>> +	for(i = 0;i < 32;i++) {
>> +		read_phy_reg(i);
>> +	}
>> +#endif
>> +	xps_ll_temac_phy_ctrl();
>> +	return 1;
>> +}
>> +
>>
> regards,
> B en

Thanks,
Michal

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>