Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

Hi, sorry for the delay, and thanks for the response.

I'm still not sure whether I'll pursue this series further, but I'd like
to at least narrow down what it'd look like.

On Wed, Aug 15, 2018 at 04:16:01PM -0600, Rob Herring wrote:
> On Tue, Aug 14, 2018 at 06:44:36PM -0700, Brian Norris wrote:
> > On Tue, Aug 14, 2018 at 05:52:49PM -0700, Florian Fainelli wrote:
> > > On 08/14/2018 05:22 PM, Brian Norris wrote:
> > > >> Also, aliases in DT are meant to provide some stability.
> > > > 
> > > > How, specifically? I don't see any relevant binding description for
> > > > aliases under Documentation/devicetree/bindings/net/.
> > > 
> > > Indeed they are not, likewise, we should probably update devicetree-spec
> > > to come up with standard names that of_alias_get_id() already consumes.
> > 
> > A quick grep shows we already have divergence: both "eth" and "ethernet"
> > are in use.
> 
> Uggg, it would be nice to clean that up. 

Is it fair to just change them, since they were never documented? Of
course, only after documenting the "right" way.

> There's several aliases I'd like to get rid of (some platforms went a 
> little crazy with them) and I'd like to start requiring alias names to 
> be documented. I created an issue for the spec. Patches welcomeTM. :)

So e.g., new text in Documentation/devicetree/bindings/net/ethernet.txt
and Documentation/devicetree/bindings/net/wireless/ieee80211.txt about
'aliases' entries? I can do that, especially if I give up on a new
binding like in this series.

> > But anyway, would the idea be that you just put 'ethernet{0,1,...}' and
> > 'wifi{0,1,...}' aliases in the /chosen node, then require boot firmware
> > to insert any {ethernet,wifi}_mac{0,1,...} into the paths represented by
> > the corresponding aliases?
> 
> In the /aliases node, but yes.

Oops, yes.

> Seems to me that nvmem needs to be extended to allow providers to 
> retrieve and interpret data. Not everything is at some fixed offset and 
> size. Something like this is valid dts:
> 
> nvmem = <&phandle> "a-string";
> 
> But that's pretty uncommon (I can't think of a binding that actually 
> uses that). Perhaps the provider has an array of keys defined and the 
> consumer just provides the index.

In the case of VPD, all keys are 0-terminated strings (there's also a
length field, but the key is still 0-terminated), so that scheme could
work. (I'm not sure an indexed provider is extremely relevant right now,
although it probably could be supported if I expand the of_nvmem
retrieval to support a generic of_xlate() override anyway.) The
information represented is almost the same as in my proposal, except that:
(a) now I have to give the VPD a phandle -- so far, I've avoided that,
    as it's just an auto-enumerated device underneath the
    /firmware/coreboot device (see drivers/firmware/google/vpd.c)
(b) this is no longer directly useful to ACPI systems -- I'm not
    actually sure how (if at all) nvmem provider/consumer is supposed to
    work there

But maybe this isn't really that useful to ACPI, and it's sufficient to
just have fwnode_get_mac_address() call of_get_nvmem_mac_address() when
we're using DT.

> Or we could do '<key>-nvmem = <&phandle>', but parsing that is a bit 
> more complicated.

That doesn't seem to have much advantage to me.

Brian