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

[Simon Glass]

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?

Regards,
Simon