[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Simon Glass]

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?

Regards,
Simon