[U-Boot] [U-boot] [PATCH 2/2] NET: QE: UEC: Allow uec re-initialization based on netretry environment variable.

[richardretanubun]

Hi Ben,

Ben Warren wrote:

> Hi Richard,
>
>
> richardretanubun wrote:
>
>> Allow uec_init to run more than once, based on the netretry environment 
>> variable.
>>
>> This allows for manual (back and forth) switching between network 
>> interfaces.
>>
>>
>>   
> My general issue with this patch is that if this functionality is really 
> useful, it should be in the main device control flow (net/eth.c), not in 
> a single driver.
>
Understood, I am still new with u-boot and not that familiar with network device drivers. I will look into this file

and resubmit something when I think I have something that can apply to all devices.

>> Signed-off-by: Richard Retanubun <RichardRetanubun_at_ruggedcom.com>
>>
>>
>> diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
>>
>> index 344c649..88402ca 100644
>>
>> --- a/drivers/qe/uec.c
>>
>> +++ b/drivers/qe/uec.c
>>
>> @@ -1192,9 +1192,17 @@ static int uec_init(struct eth_device* dev, bd_t *bd)
>>
>>      uec_private_t        *uec;
>>
>>      int            err, i;
>>
>>      struct phy_info         *curphy;
>>
>> +    char *netretry = NULL;
>>
>>  
>>
>>      uec = (uec_private_t *)dev->priv;
>>
>>  
>>
>> +
>>
>> +    /* Allow for net re-initialization based on netretry environment 
>> setting */
>>
>> +    netretry = getenv("netretry");
>>
>> +    if (netretry != NULL && (strcmp(netretry, "yes") == 0)) {
>>
>> +        uec->the_first_run = 0;
>>
>> +    }
>>
>> +
>>
>>   
> I'm sure you know that "netretry" already exists, and has a different 
> meaning.  It's not a good idea for an environment variable to do 
> different things depending on context.
>
I do, I thought the purpose was to retry a network operation that failed. Since a network operation can
fail because of many causes, I choose to 'broaden' the scope to include bad PHY configurations.

If I have to choose a different environment variable name to control this what would be a good one? (miiretry)

>>      if (uec->the_first_run == 0) {
>>
>>          err = init_phy(dev);
>>
>>          if (err) {
>>
>> @@ -1223,12 +1231,16 @@ static int uec_init(struct eth_device* dev, bd_t 
>> *bd)
>>
>>          if (err || i <= 0)
>>
>>              printf("warning: %s: timeout on PHY link\n", dev->name);
>>
>>  
>>
>> -        uec->the_first_run = 1;
>>
>> +        /* If netretry is not set, or not set to yes, assume no retry */
>>
>> +        netretry = getenv("netretry");
>>
>> +        if (netretry == NULL || (strcmp(netretry, "yes") != 0)) {
>>
>> +            uec->the_first_run = 1;
>>
>> +        }
>>
>>      }
>>
>>  
>>
>>      /* Set up the MAC address */
>>
>>      if (dev->enetaddr[0] & 0x01) {
>>
>> -        printf("%s: MacAddress is multcast address\n",
>>
>> +        printf("%s: MacAddress is multicast address\n",
>>
>>               __FUNCTION__);
>>
>>   
> OK, good catch.
>
>
> Sorry again for taking so long to respond properly.
>
Don't worry, as you can see I am just as capable of responding (even) later :)

Thank you for the advice. 

I will study the way making a more "generic" form of this patch.

I'm afraid It won't happen soon though, the hardware I'm supposed to be booting is arriving soon. 

I will submit an updated patch when I got something working on this.

>
> regards,
>
> Ben
>