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

[Olliver Schinagl]

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?

>
>Regards,
>Simon

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.