Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

[Vadym Kochan <vadym.kochan@plvision.eu>]

Hi Srini,

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> Hi Vadym,
>
>
> On 08/09/2021 10:38, Vadym Kochan wrote:
>> 
>> Hi Srini,
>> 
>> Sorry for such delay in replies, I am still confused how to
>> implement it properly, let me please explain the issues
>> which I faced with:
>
>> 
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>> 
>>> On 16/06/2021 13:33, Vadym Kochan wrote:
>>>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>>>> index bca671ff4e54..648373ced6d4 100644
>>>>>> --- a/drivers/nvmem/core.c
>>>>>> +++ b/drivers/nvmem/core.c
>>>>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>>>>>     	nvmem_reg_read_t	reg_read;
>>>>>>     	nvmem_reg_write_t	reg_write;
>>>>>>     	struct gpio_desc	*wp_gpio;
>>>>>> +	struct nvmem_parser_data *parser_data;
>>>>> This should be renamed to nvmem_cell_info_parser or something on those lines
>>>>> to avoid any misunderstanding on what exactly this parser is about.
>>>>>
>>>>> May be can totally avoid this by using parser name only during register.
>>>>>
>>>> I added this to keep parsed cells particulary for this nvmem in case
>>>> same parser is used for several nvmem's and mostly because of using also
>>>> cell lookup info. I will try to also answer your below question why do I need
>>>> lookups ?
>>>>
>>>> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
>>>> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
>>>> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
>>>> info of platform_device. I use the 2nd option with the following sample
>>>> solution:
>>>>
>>>> 	## DT ##
>>>> 	eeprom_at24: at24@56 {
>>>> 		compatible = "atmel,24c32";
>>>> 		nvmem-cell-parser-name = "onie-tlv-cells";
>>>> 		reg = <0x56>;
>>>> 	};
>>>>
>>>> 	onie_tlv_parser: onie-tlv-cells {
>>>> 		compatible = "nvmem-cell-parser";
>>>> 		status = "okay";
>>>>
>>>> ---> add ability here to map cell con_id to cell_name ?
>>>>
>>>> 	};
>>>>
>>>> 	some_dev_node {
>>>> 		compatible = "xxx";
>>>> 		base-mac-provider = <&onie_tlv_parser>;
>>>
>>> Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as
>>> your mac provider?
>>> If you use eeprom_at24 then of_get_mac_address() should get mac-address
>>> directly from cell info.
>> 
>> 1) This DT node is a trick to register it as a platform_device because of:
>> 
>> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>> {
>>          struct platform_device *pdev = of_find_device_by_node(np);
>>          struct nvmem_cell *cell;
>>          const void *mac;
>>          size_t len;
>>          int ret;
>>   
>>          /* Try lookup by device first, there might be a nvmem_cell_lookup
>>           * associated with a given device.
>>           */
>>          if (pdev) {
>>                  ret = nvmem_get_mac_address(&pdev->dev, addr);
>>                  put_device(&pdev->dev);
>>                  return ret;
>>          }
>>   
>>          cell = of_nvmem_cell_get(np, "mac-address");
>>          if (IS_ERR(cell))
>>                  return PTR_ERR(cell);
>>   
>>          ...
>> }
>> 
>> I tried to use at24_eeprom as ref in DTS file, but this device is not a
>> platform device but a nvmem bus device, so it fails on:
>> 
>>          ...
>> 
>>          struct platform_device *pdev = of_find_device_by_node(np);
>> 
>>          ...
>> 
>>          /* Try lookup by device first, there might be a nvmem_cell_lookup
>>           * associated with a given device.
>>           */
>>          if (pdev) {
>>                  ret = nvmem_get_mac_address(&pdev->dev, addr);
>>                  put_device(&pdev->dev);
>>                  return ret;
>>          }
>> 
>>          ...
>> 
>> Probably this might be fixed by lookup nvmem device too ?
>
> Honestly, this approach is a total hack to get it working.
>
> This is what Am thinking which should look like:
>
> ## DT ##
> eeprom_at24: at24@56 {
> 	compatible = "atmel,24c32";
> 	/* Some way to identify that this is a TLV data */
> 	nvmem-cell-parser-name = "onie-tlv-cells";
> 	reg = <0x56>;
>
> 	mac_address: mac-address {
> 		/* THIS REG is updated once TLV is parsed */
> 		reg = <0 0x6>
> 	};

I assume these cell nodes should be marked with some property that they
should be evaluated later, so the cell will be not read
in case it was not parsed because it may
exist in nvmem device optionally.

Or, treat cells with length "0" in a special way and allow to update
cell info later.

> };
>
> some_dev_node {
> 	compatible = "xxx";
> 	nvmem-cells = <&mac_address>;
> 	nvmem-cell-names = "mac-address";
> };
>
> == CODE ==
> ret = of_get_mac_address(dev->of_node, base_mac);
> ==========
>
>
> If you notice the mac_address node reg is 0.
> This node "reg" property should be updated ( using of_update_property()) 
> by nvmem-provider driver while tlv parsing and by matching the node name 
> with tlv name.
>

I assume parser driver can just invoke add_cell_table (with may be
by adding 'bool update' field to the cell_info struct) and core.c will just
update existing cells parsed from OF.

> That way rest of the code will work as usual.
>
> If this work for you then we can ask Rob if he foresee any issues in 
> this approach. I already see similar usage in reserved-memory case.
>
>
> --srini
>
>> 
>> 2) Regarding cell lookups registration, I had to use it because
>> of_nvmem_cell_get() will not find parser cells via OF.
>> 
>>>
>>>
>>>> 		status = "okay";
>>>> 	};
>>>> 	########
>>>>
>>>> 	== CODE ==
>>>> 	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
>>>> 	ret = of_get_mac_address(base_mac_np, base_mac);
>>>> 	==========
>>>>
>>>>
>>>> And it works with this implementation because onie-tlv-cells is
>>>> registered as platform_device which name is the same as parser's name.
>>>> So the really tricky part for me is to make this cells lookup work.
>>>
>>> cell lookups are more of intended for board files, adding them in this
>>> case is really not correct.  The whole purpose of this driver is to
>>> parse the tlv cell infos into nvmem cell info.
>>>
>>>
>>> --srini
>>>
>>>
>>>>
>>>> Of course would be great if you can point a way/idea to get rid the need of
>>>> lookups.
>>>>
>> 


Regards,
Vadym Kochan