[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC
Olliver Schinagl
o.schinagl at ultimaker.com
Mon Apr 3 14:32:30 UTC 2017
Hey Joe,
On 26-03-17 16:10, Joe Hershberger wrote:
> Hi Oliver,
>
> On Sun, Dec 11, 2016 at 3:27 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Oliver,
>>
>> On 9 December 2016 at 02:25, Olliver Schinagl <oliver at schinagl.nl> wrote:
>>> Hey simon
>>>
>>> On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Oliver,
>>>>
>>>> On 7 December 2016 at 02:26, Olliver Schinagl <oliver at schinagl.nl>
>>>> wrote:
>>>>>
>>>>>
>>>>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg at chromium.org>
>>>> wrote:
>>>>>> Hi Oliver,
>>>>>>
>>>>>> On 5 December 2016 at 03:28, Olliver Schinagl <oliver at schinagl.nl>
>>>>>> wrote:
>>>>>>> Hey Simon,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 05-12-16 07:24, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Oliver,
>>>>>>>>
>>>>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver at schinagl.nl>
>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hey Joe,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
>>>>>> <oliver at schinagl.nl>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> This patch adds a method for the board to set the MAC address
>>>> if
>>>>>> the
>>>>>>>>>>> environment is not yet set. The environment based MAC addresses
>>>>>> are not
>>>>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>>>>>> into the
>>>>>>>>>>> environment.
>>>>>>>>>>>
>>>>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>>>>>>> contains
>>>>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>>>>>> however, it
>>>>>>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Olliver Schinagl <oliver at schinagl.nl>
>>>>>>>>>>> ---
>>>>>>>>>>> common/fdt_support.c | 8 +++++++-
>>>>>>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>>>> index c34a13c..f127392 100644
>>>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64
>>>> start,
>>>>>> u64
>>>>>>>>>>> size)
>>>>>>>>>>> return fdt_fixup_memory_banks(blob, &start, &size,
>>>> 1);
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>>>>>> *mac_addr)
>>>>>>>>>>
>>>>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and
>>>> in
>>>>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>>>>>>
>>>>>>>>>> Also, it's so generic, and only gets called by the fdt fixup
>>>>>> stuff...
>>>>>>>>>> This function should probably be named in such a way that its
>>>>>>>>>> association with fdt is clear.
>>>>>>>>>
>>>>>>>>> I did not notice that, sorry! But naming suggestions are welcome
>>>> :)
>>>>>>>>>
>>>>>>>>> Right now, I use it in two unrelated spots however.
>>>>>>>>>
>>>>>>>>> from the fdt as seen above and in a subclass driver (which will
>>>>>> come up
>>>>>>>>> for
>>>>>>>>> review) as suggested by Simon.
>>>>>>>>>
>>>>>>>>> There I do:
>>>>>>>>>
>>>>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>>>>>>> +{
>>>>>>>>> + struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>>>>> +
>>>>>>>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>>> .start = designware_eth_start,
>>>>>>>>> .send = designware_eth_send,
>>>>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>>> .free_pkt = designware_eth_free_pkt,
>>>>>>>>> .stop = designware_eth_stop,
>>>>>>>>> .write_hwaddr = designware_eth_write_hwaddr,
>>>>>>>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr,
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> which is completly unrelated to the fdt.
>>>>>>>>>
>>>>>>>>> So naming suggestion or overal suggestion how to handle this nice
>>>>>> and
>>>>>>>>> generically?
>>>>>>>>>
>>>>>>>>> Based from the name however I would think that all
>>>>>> board_get_enetaddr's
>>>>>>>>> work
>>>>>>>>> the same however so should have been interchangeable? Or was that
>>>>>> silly
>>>>>>>>> thinking?
>>>>>>>>
>>>>>>>> Would it be possible to use a name without 'board' in it? I think
>>>>>> this
>>>>>>>> / hope is actually sunxi-specific code, not board-specific?
>>>>>>>
>>>>>>> You are actually correct, we take the serial number of the SoC
>>>> (sunxi
>>>>>>> specific) and generate a serial/MAC from it. So nothing to do with
>>>>>> the
>>>>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be
>>>> then
>>>>>> (much)
>>>>>>> better?
>>>>>>>
>>>>>>> The reason why I went to 'board' with my mind, is because a) the
>>>>>> original
>>>>>>> mac gen code and b) the location was in board/sunxi/board.c. I
>>>> think
>>>>>> it is
>>>>>>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>>>>>> it has
>>>>>>> nothing to do with board(s).
>>>>>>
>>>>>> That sounds good to me - and you should be able to call it directly
>>>>> >from the driver and avoid any weak functions, right?
>>>>> The subclass driver can, the fdt fixup however still needs a weak
>>>> fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr()
>>>> i think.)
>>>>
>>>> OK - I feel that the fdt fixups need a bit of thought. At the moment
>>>> it is all pretty ad-hoc.
>>>
>>> Yeah i think i agree, but i guess thats a separate cleanup step generally, no?
>>
>> OK - are you able to take a look at this?
>
> Any update on this or any of the other patches in your series that I
> did not apply last release? I was expecting a v2 to address some
> things such as this patch.
I have a few after rebasing u-boot-net/master and I just started
yesterday to get back up to speed. I'll double check all review comments
and send a v2 with the remaining patches. Sorry for taking so long here!
Olliver
>
> Thanks!
> -Joe
>
More information about the U-Boot
mailing list