[U-Boot] FEC and EFI Simple Network

[Joe Hershberger]

Hi Patrick,

On Wed, Mar 28, 2018 at 4:54 PM, Fabio Estevam <festevam at gmail.com> wrote:
> Adding Joe in case he has some ideas.
>
> On Tue, Mar 27, 2018 at 9:12 AM, Patrick Wildt <patrick at blueri.se> wrote:
>> Hi,
>>
>> I have been debugging network issues when running an EFI Application
>> that uses the EFI Simple Network protocol on an i.MX6 machine (FEC).
>>
>> The symptom is that u-boot's (FEC) internal RX ring index is reset to 0,
>> while the controller is still at idx 3 (or something else).  This is
>> caused by the following circumstances:
>>
>> The Simple Network protocol offers methods like Start(), Stop(),
>> Initialize(), Shutdown(), Reset().  Also the protocol has a state.  The
>> states are Stopped, Started, Initialized.  The transition is as follows:
>>
>> Stopped ---Start()---> Started ---Initialize()--> Initialized
>>
>> Start() does some initialization, Initialize() allocates the TX/RX
>> descriptors and actually kicks off the network engine.
>>
>> So far, only Initialize() is implemented in our u-boot interface, and it
>> calls eth_init() which in the end calls fec_init().  Our network state
>> is _always_ Started.  This means that EFI Applications see that that the
>> state is Started and then call Initialize() to start the actual network
>> traffic.  There is no call to Stop()/Shutdown()/Reset() as it's supposed
>> to be in a sane state.
>>
>> In my case the FEC is already initialized since I booted using network
>> and the RX desc index is already non-zero.  Now the EFI Application sees
>> that the state is Started, calls Initialize() which makes u-boot call
>> eth_init() which then calls fec_init().
>>
>> fec_init() does not reset the controller so that the controller-internal
>> RX desc index is not reset to zero.  fec_init() calls fec_open() which
>> then resets the driver-internal RX desc index to zero.  Now they are out
>> of sync, boom.

Would it be reasonable for fec_init to use a state variable to keep
track of if it (and the HW) is already initialized and not call
fec_open in that case? Also, fec_halt would need to update that state
as well.

>>
>> This means that fec_init() without a previous fec_halt() breaks the
>> whole network if it was already running.  The Designware driver as used
>> by some sunxi platforms does a reset of the controller in the init
>> function.  Maybe calling fec_halt() at the start of fec_init() could be
>> a possible solution?
>>
>> Patrick
>>
>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>> index ff7ad91116..ba8bd9920d 100644
>> --- a/drivers/net/fec_mxc.c
>> +++ b/drivers/net/fec_mxc.c
>> @@ -522,6 +522,12 @@ static int fec_open(struct eth_device *edev)
>>         return 0;
>>  }
>>
>> +#ifdef CONFIG_DM_ETH
>> +static void fecmxc_halt(struct udevice *dev);
>> +#else
>> +static void fec_halt(struct eth_device *dev);
>> +#endif
>> +
>>  #ifdef CONFIG_DM_ETH
>>  static int fecmxc_init(struct udevice *dev)
>>  #else
>> @@ -537,6 +543,15 @@ static int fec_init(struct eth_device *dev, bd_t *bd)
>>         u8 *i;
>>         ulong addr;
>>
>> +#ifdef CONFIG_DM_ETH
>> +       fecmxc_halt(dev);
>> +#else
>> +       fec_halt(dev);
>> +#endif
>> +
>> +       writel(~FEC_TCNTRL_GTS & readl(&fec->eth->x_cntrl),
>> +              &fec->eth->x_cntrl);
>> +
>>         /* Initialize MAC address */
>>  #ifdef CONFIG_DM_ETH
>>         fecmxc_set_hwaddr(dev);
>> @@ -825,19 +840,12 @@ static int fec_recv(struct eth_device *dev)
>>         }
>>         if (ievent & FEC_IEVENT_HBERR) {
>>                 /* Heartbeat error */
>> -               writel(0x00000001 | readl(&fec->eth->x_cntrl),
>> +               writel(FEC_TCNTRL_GTS | readl(&fec->eth->x_cntrl),
>>                        &fec->eth->x_cntrl);
>>         }
>>         if (ievent & FEC_IEVENT_GRA) {
>>                 /* Graceful stop complete */
>> -               if (readl(&fec->eth->x_cntrl) & 0x00000001) {
>> -#ifdef CONFIG_DM_ETH
>> -                       fecmxc_halt(dev);
>> -#else
>> -                       fec_halt(dev);
>> -#endif
>> -                       writel(~0x00000001 & readl(&fec->eth->x_cntrl),
>> -                              &fec->eth->x_cntrl);
>> +               if (readl(&fec->eth->x_cntrl) & FEC_TCNTRL_GTS) {
>>  #ifdef CONFIG_DM_ETH
>>                         fecmxc_init(dev);
>>  #else
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot