On Fri, Aug 14, 2015 at 12:59 PM, York Sun <york...@freescale.com> wrote:
>
>
> On 08/14/2015 01:28 AM, Ciubotariu Codrin Constantin-B43658 wrote:
>> Hi York,
>>
>>> -----Original Message-----
>>> From: Sun York-R58495
>>> Sent: Thursday, August 13, 2015 6:55 PM
>>> To: Ciubotariu Codrin Constantin-B43658
>>> Cc: Joe Hershberger; u-boot@lists.denx.de; joe.hershber...@ni.com
>>> Subject: Re: [U-Boot] [PATCH v3 11/16] net/eth.c: Add function to validate
>>> a MAC
>>> address
>>>
>>>
>>>
>>> On 08/13/2015 08:42 AM, Ciubotariu Codrin Constantin-B43658 wrote:
>>>> Hi Joe, York,
>>>>
>>>>> -----Original Message-----
>>>>> From: Sun York-R58495
>>>>> Sent: Wednesday, August 12, 2015 10:59 PM
>>>>> To: Ciubotariu Codrin Constantin-B43658
>>>>> Cc: Joe Hershberger; u-boot@lists.denx.de; joe.hershber...@ni.com
>>>>> Subject: Re: [U-Boot] [PATCH v3 11/16] net/eth.c: Add function to
>>>>> validate a MAC address
>>>>>
>>>>> +Codrin
>>>>>
>>>>> Somehow I dropped Codrin in last reply.
>>>>>
>>>>> On 08/10/2015 01:45 PM, York Sun wrote:
>>>>>>
>>>>>>
>>>>>> On 08/10/2015 01:05 PM, Joe Hershberger wrote:
>>>>>>> Hi York,
>>>>>>>
>>>>>>> On Mon, Aug 10, 2015 at 3:03 PM, York Sun <york...@freescale.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08/10/2015 12:57 PM, Joe Hershberger wrote:
>>>>>>>>> Too much top-posting.
>>>>>>>>>
>>>>>>>>> On Mon, Aug 10, 2015 at 2:41 PM, York Sun <york...@freescale.com>
>>>>>>>>> wrote:
>>>>>>>>>> SPL doesn't use net/eth.c. You add a call in env_flags.c.
>>>>>>>>>>
>>>>>>>>>> I think you can put it in header file and use static inline, or
>>>>>>>>>> keep it in the same file where it is called.
>>>>>>>>>
>>>>>>>>> That is probably fine.
>>>>>>>>>
>>>>>>>>>> Another way is to undef CONFIG_CMD_NET for SPL part. It is
>>>>>>>>>> default to 'y' in Kconfig. Joe may have some good suggestion.
>>>>>>>>>
>>>>>>>>> I don't think this is the reason. The problem is that net is
>>>>>>>>> *not* build for SPL, but env is.
>>>>>>>>
>>>>>>>> Yes, env is built. The offending lines in common/env_flags.c are
>>>>>>>> gated by "#ifdef CONFIG_CMD_NET". That's why I say it could be another
>>> way.
>>>>>>>
>>>>>>> OK, sure... but that breaks intended behavior, I think.
>>>>>>>
>>>>>>
>>>>>> I see. The CONFIG_CMD_NET is not evaluated separated for SPL build.
>>>>>> So I guess the fix can be either to put the common function in
>>>>>> header file after making it really simple to reduce dependency, or
>>>>>> to keep the
>>>>> original code in env_flag.c.
>>>>>>
>>>>>
>>>>> Codrin,
>>>>>
>>>>> Can you prepare a new patch? You don't have to send the whole set.
>>>>> All but one have been acked by Joe.
>>>>>
>>>>> York
>>>>
>>>> I can't inline eth_validate_ethaddr_str in eth.h since it depends on
>>> simple_strtoul and tolower. Also, I can't let it in common/env_flags.c
>>> because I
>>> need to access if from drivers/net/vsc9953.c . I guess it has to be in a .c
>>> file
>>> that is also used by SPL targets. Could you please suggest such a file?
>>>>
>>>> SPL targets make use of CONFIG_CMD_NET? It seems strange that ETH env is
>>> built, but net/net.c or net/eth.c not.
>>>>
>>>
>>> I was discussing with Joe about the possibility to deselect CONFIG_CMD_NET
>>> for
>>> SPL build. The issue here is Kconfig is not re-evaluated for the SPL part.
>>> If
>>> you can experiment it, you can try to gate the code in env_flags.c with
>>> CONFIG_SPL_BUILD. It sounds reasonable to me.
>>>
>>> York
>>
>> Something like
>>
>> #if defined(CONFIG_CMD_NET) && !defined(CONFIG_SPL_BUILD)
>> case env_flags_vartype_ipaddr:
>> cur = value;
>> for (i = 0; i < 4; i++) {
>> skip_num(0, cur, &end, 3);
>> if (cur == end)
>> return -1;
>> if (i != 3 && *end != '.')
>> return -1;
>> if (i == 3 && *end != '\0')
>> return -1;
>> cur = end + 1;
>> }
>> break;
>> case env_flags_vartype_macaddr:
>> if (eth_validate_ethaddr_str(value))
>> return -1;
>> break;
>> #endif
>>
>> ?
>>
>> I get two warnings on this:
>> ../common/env_flags.c: In function '_env_flags_validate_type':
>> ../common/env_flags.c:203:2: warning: enumeration value
>> 'env_flags_vartype_ipaddr' not handled in switch [-Wswitch]
>> switch (type) {
>> ^
>> ../common/env_flags.c:203:2: warning: enumeration value
>> 'env_flags_vartype_macaddr' not handled in switch [-Wswitch]
>>
>> Unless I guard these values in env_flags.h:
>> #if defined(CONFIG_CMD_NET) && !defined(CONFIG_SPL_BUILD)
>> env_flags_vartype_ipaddr,
>> env_flags_vartype_macaddr,
>> #endif
>>
>
> It makes sense to me to take out these two for SPL build. The whole purpose of
> SPL is to load the final image. I don't see a chance users would be able to
> type
> any command.
>
> Joe, do you agree the CMD_NET shouldn't be used for SPL?
I tend to agree, but I have a vague recollection that there is at
least one SPL that uses net. Sorry I don't recall what board it was
that I'm thinking about.
-Joe
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
