Re: [PATCH RFC linux] dt-bindings: nvmem: Add binding for U-Boot environment NVMEM provider

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 14/10/2021 11:55, Marek Behún wrote:
> On Thu, 14 Oct 2021 11:30:13 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> On 14/10/2021 11:06, Marek Behún wrote:
>>> On Thu, 14 Oct 2021 09:26:27 +0100
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
>>>    
>>>> On 14/10/2021 00:20, Marek Behún wrote:
>>>>> Add device tree bindings for U-Boot environment NVMEM provider.
>>>>>
>>>>> U-Boot environment can be stored at a specific offset of a MTD
>>>>> device, EEPROM, MMC, NAND or SATA device, on an UBI volume, or in
>>>>> a file on a filesystem.
>>>>>
>>>>> The environment can contain information such as device's MAC
>>>>> address, which should be used by the ethernet controller node.
>>>>>       
>>>>
>>>> Have you looked at
>>>> ./Documentation/devicetree/bindings/mtd/partitions/nvmem-cells.yaml
>>>> ?
>>>
>>> Hello srini,
>>>
>>> yes, I have. What about it? :)
>>>
>>> That binding won't work for u-boot-env, because the data are stored
>>> in a different way. A cell does not have a constant predetermined
>>> offset on the MTD.
>>
>> Can't you dynamically update the nodes before nvmem-provider is
>> registered?
> 
> Are you talking about dynamically updating nvmem-cell OF nodes, adding
> reg properties with actual offsets and lengths found after parsing?

Yes, atleast for the nodes that are defined in the dt.

> 
>>> The variables are stored as a sequence of values of format
>>> "name=value", separated by '\0's, for example:
>>>     board=turris_mox\0ethaddr=00:11:22:33:44:55\0bootcmd=run
>>> distro_bootcmd\0.... Chaning lengths of values of variables, or
>>> deleting variables, moves the data around. Integers and MAC
>>> addresses are stored as strings, and so on.
>>
>> Do you already have a provider driver for handing this.
> 
> Not yet, I will send the proposal together with a driver in next
> round.
> 
>> How is pre parsing cell info and post processing data planned to be
>> handled?
> 
> My plan was to read the variables from the u-boot-env partition, create
> a nvmem-cell for each variable, and then pair the ones mentioned in
> device tree with their DT nodes, and post-process according to type
Adding cells using nvmem_cell_info should work. I think pairing the one 
with DT entries is something that is missing. Currently nvmem_cell_info 
does not have device_node pointer may be that is something that could be 
added to help here.


> (post-processing would be done only for those mentioned in device tree,
> others would be left as strings).
> 
>> Currently in nvmem core we do check for "reg" property for each cell,
>> unless the provider driver is adding/updating dt entries dynamically
>> before registering nvmem provider
> 
> I don't think updaring DT entries dynamically is a correct solution at
> all. Is this done in Linux? Updating device properties is something
> different, but changing DT properties seems wrong to me.
> 
>> It will not work as it is. Alteast this is what I suggested in similar
>> case where cell information is in tlv format.
> 
> Hmm. OK, I shall try to implement a driver for this and then will
> return to you.

Sounds good.

> 
>> Secondly mac-address seems to be delimited, we recently introduced
>> post processing callback for provider driver [1], which should help
>> in this case.
> 
> Cool, I shall use that.
yes, please it should show up in 5.16 anyway.

> 
>> If the nvmem-cell names are standard like "mac-address" then you do
>> not need to add a new "type" binding to cell too, you can do
>> post-processing based on name.
> 
> I plan to add functions
>    of_nvmem_get_mac_address()
>    nvmem_get_mac_address()
we already have nvmem_get_mac_address() in  ./net/ethernet/eth.c that 
looks for mac-address.

--srini
> which would look at (in this order):
>    mac-address, address, mac-address-backup
> We have to keep the name "address" for backwards compatibility with one
> driver that uses this (drivers/net/ethernet/ni/nixge.c)
> 
> Thanks.
> 
> Marek
>