Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-08-08, Ahmad Fatoum wrote:
> 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@pengutronix.de>
> >>
> >> 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.

You're right.

> >> 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.

To be clear, the binding under
"Documentation/devicetree/bindings/net/ethernet-controller.yaml" just
says that the nvmem-cells is <1> and the nvmem-cells-names must be set to
"mac-address". The binding don't say how much space the mac-address
storage occupy.

> >> 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.

Debix is the first I'm aware of. I know that this is common code. As
said above the binding don't stop us from having a larger nvmem-cell
size.

> > 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().

I will move it to the board code, thanks for the link.

Regards,
  Marco