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

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

[richardretanubun]

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.

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;
+    }
+
     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__);
         return -1;
     }

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

[Ben Warren]

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.
>
>   
Can't you do this by changing the 'ethactive' environment variable?

regards,
Ben

[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:
> 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.
>>
>>   
> Can't you do this by changing the 'ethactive' environment variable?
You are correct,  changing 'ethact' environment variable works if you 
are actually changing the network interface (say from "FSL UEC0" to "FSL 
UEC1")

I'm sorry for not adding this earlier.

The scenario I am trying to handle is if switching network interface fails.
I am trying to switch and activate specific network interfaces for 
testing and
often times the interface will fail to initialize and will have to be 
reinitialized

I am using these function call sequence to do activate the specific eth 
interface repeatedly:

void net_set_eth(char *newEthDev)
{
    ....
    setenv("ethact", newEthDev);
    eth_set_current();
    eth_init(gd->bd);
    ....
}

By adding the patch, I can just keep trying the same eth interface. The 
'ethrotate' environment variable will also work,
but only for rotating once, once a failure occur, there is no way to 
retry the same ethernet connection.

>
> regards,
> Ben

Hope this does not confuse more :)

- Richard.

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

[Ben Warren]

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.
> 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.
>      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.

regards,
Ben

[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
>