Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH RFC linux] dt-bindings: nvmem: Add binding for U-Boot
 environment NVMEM provider
To:     =?UTF-8?Q?Marek_Beh=c3=ban?= <kabel@kernel.org>
Cc:     robh+dt@kernel.org, devicetree@vger.kernel.org,
        U-Boot Mailing List <u-boot@lists.denx.de>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Luka Kovacic <luka.kovacic@sartura.hr>
References: <20211013232048.16559-1-kabel@kernel.org>
 <629c8ba1-c924-565f-0b3c-8b625f4e5fb0@linaro.org>
 <20211014120601.133e9a84@dellmb>
 <857c27a6-5c4b-e0ed-a830-35762799613f@linaro.org>
 <20211014125526.10d4861b@dellmb>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <d2c0d673-440d-9e58-82b4-a73740a9c180@linaro.org>
Date:   Thu, 14 Oct 2021 12:36:11 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20211014125526.10d4861b@dellmb>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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
>