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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot