Hello Marco,
On 08.08.23 11:46, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-08-08, Ahmad Fatoum wrote:
>> Hello Marco,
>>
>> On 07.08.23 19:07, Marco Felsch wrote:
>>> Some vendors like Polyhex store the MAC address ASCII encoded instead of
>>> using the plain 6-byte MAC address. This commit adds the support to
>>> decode the 12-byte ASCII encoded MAC addresses.
>>>
>>> Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
>>
>> FYI, the upstream device tree binding for this is NVMEM layout, which was only
>> recently added to Linux and for which barebox has no support yet.
>
> I know that, thanks for the info :) I thought that this is no "layout"
> it's just the mac-address stored in ASCII instead of plain 6-byte
> storage.
Sequential big-endian 6 bytes is the normal format. Anything else (ASCII
with nothing between it, ASCII with :, ASCII with -) is a different layout IMO.
>> I can understand that porting NVMEM layouts, just to get a MAC address assigned
>> might not be an attractive proposition, but I don't think that adding a new
>> barebox-specific binding is the right way here.
>
> Me neither therefore I dropped the barebox specific binding and did just
> do some heuristic.
It's a binding, whether you use a boolean property, the size in the reg field
or add a nvmem-layout subnode.
>> I'd suggest, you get the nvmem cell in board code and assign it there.
>> There's readily available API for that. If you are interested in a
>> generic solution, NVMEM layouts are the way to go IMO.
>
> Thought about that too but went this way because it's much less code
> than doing it in the board code. Also it allows to share the code with
> others.
How widespread is it to store MAC address that way? If it's just Debix doing
it this way, you are effectively adding a binding that's only useful to Debix
into common code.
> As said, I don't think that this is a layout. Of course there are more
> ASCII strings to store the production test result but this is not
> relevant. I really need to check which is more effort
> board-code vs. layout-support if you think that this is layout.
I'd be more amenable to this patch if there exists no way in upstream bindings
to represent this, which was for a long time the case. That's not the case any
more, so we should not add any new barebox-specific bindings for MAC addresses
that duplicate what's achievable by the upstream binding.
For an example of how to do this in board code, see rdu_eth_register_ethaddr().
Cheers,
Ahmad
>
>>> ---
>>> drivers/of/of_net.c | 19 +++++++++++++++++++
>>> 1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
>>> index 75a24073da51..4e74986cdda8 100644
>>> --- a/drivers/of/of_net.c
>>> +++ b/drivers/of/of_net.c
>>> @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
>>> return -ENODEV;
>>> }
>>>
>>> +#define ETH_ALEN_ASCII 12
>>> +
>>> int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>>> {
>>> struct nvmem_cell *cell;
>>> @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>>> if (IS_ERR(mac))
>>> return PTR_ERR(mac);
>>>
>>> + if (len == ETH_ALEN_ASCII) {
>>> + u8 *mac_new;
>>> + int ret;
>>> +
>>> + mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
>>> + ret = hex2bin(mac_new, mac, ETH_ALEN);
>>
>> Why not parse into a fixed size local buffer and then copy it? Would save
>> you the extra allocation.
>
> I went this way to keep the below free logic the same.
>
> Regards,
> Marco
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
