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

[Simon Glass]

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.

Regards,
Simon