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

[Joe Hershberger]

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.

Thanks!
-Joe