| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6a2760c0066226c67e9323e1ad733fe4@agner.ch>
Date: Wed, 13 Jul 2016 22:28:32 -0700
From: Stefan Agner <stefan@...er.ch>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
robh+dt@...nel.org
Cc: maitysanchayan@...il.com, arnd@...db.de, shawnguo@...nel.org,
robh+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/5] nvmem: core: Add consumer API to get nvmem cell
from node
On 2016-07-08 10:23, Srinivas Kandagatla wrote:
> On 08/07/16 17:42, Stefan Agner wrote:
>> On 2016-07-08 08:41, Srinivas Kandagatla wrote:
>>> On 07/07/16 14:48, maitysanchayan@...il.com wrote:
>>>> Hello Srinivas,
>>>>
>>>> On 16-07-07 1
>
> ...
>
>>>>>>
>>>>>> Our requirement is to be able to pass the soc node pointer and then
>>>>>> be able to get a nvmem cell by specifying it's name. So for our case
>>>>>
>>>>> Why?
>>>>
>>>> Sorry for not providing the background directly. The patches before this
>>>> series used that approach. In the previous discussions it has been pointed
>>>> out that it is not acceptable to have additional device tree bindings for
>>>> providing data that the driver wants at the SoC node level or to have bindings
>>>> just for the SoC bus driver alone since we aren't really describing the
>>>> hardware.
>>>>
>>> SOC driver seems to search for an arbitrary node by its name, which is
>>> not a binding and can break anytime in cases If the scope of nvmem
>>> provider is out of soc node or if the nvmem cells are not named as
>>> expected. That looks very fragile.
>>
>> In that case, that just "won't happen" because the soc driver is a very
>> soc specific driver only used for this device tree. We it will always
>> bind to that high level soc node.
>>
>>>
>>> If the soc node is actual consumer of nvmem cells, I see no reason why
>>> we should not use proper nvmem bindings?
>>
>> There is a reason: We don't describe the hardware with it...
>>
>> The cfg0/cfg1 register which Sanchayan needs to read in the soc bus
>> driver are just two register with a unique ID of the SoC. In whatever
>
> "Unique ID of the SOC" doesn't this mean that its a part of soc hw
> description/configuration/setup?
>
> Am still not clear why this setup any different to other use cases
> like mac address/calibration data?
>
> I still feel that this should be described in the DT.
>
> Rob,
> What do you think?
[added Rob to "To"]
Rob, can you comment?
--
Stefan
>
>
>> driver throughout the system we use that ID (e.g. in a random generator
>> for initialization) we never describe an actual hardware relation... Its
>> just software and how we use that unique ID. The device tree is ment to
>> describe hardware. Hence the NVMEM consumer binding is not suited for
>> such NVMEM cells...
>>
>> By describing the NVMEM cells location in device tree (producer API, the
>> NVMEM cells are in hardware at that location, so using the device tree
>> for that part is fine) and hard coding the NVMEM cell we need in the
>> driver code we don't violate the device tree matra "describe the
>> hardware"...
>
> IMHO, We should indeed describe the SOC hardware and its relationship
> w.r.t to nvmem provider in device tree. Reasoning being these both are
> some form of IP blocks/hw which depend on each other.
>
>>
>> Looking-up the nodes direcly is what Rob suggested here:
>> https://lkml.org/lkml/2016/5/23/573
>
> I did read this, I was not convinced that we should do a direct lookup
> for nvmem cells.
>
> thanks,
> srini
>>
>>>
>>> Given the fact that the patch is potentially bypassing the nvmem
>>> bindings, am not happy to take it!
>>
>> If you can provide a solution acceptable by the device tree folks and
>> works without this patch, I am happy to do it...
>
>
>>
>> Btw, I am not entirely happy with the API name, but did not had a better
>> idea... And we we should probably add a note that the device tree
>> consumer binding is the preferred way to do it.
>>
>> --
>> Stefan
>>
>>
>>>
>>> thanks,
>>> srini
>>>
>>>> For the discussion,
>>>> https://lkml.org/lkml/2016/5/23/573
>>>> https://lkml.org/lkml/2016/5/2/71
>>>>
>>>> Regards,
>>>> Sanchayan.
>>>>
>>>>
>>>>>
>>>>>> ocotp node has cfg0 and cfg1 which we want but we cannot use existing
>>>>>> nvmem consumer API since that requires having the nvmem consumer properties
>>>>>> in the node we are binding to viz. is a direct nvmem consumer.
>>>>>>
>>>>>> Regards,
>>>>>> Sanchayan.
>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>> srini
>>>>>>>>
>>>>>>>> Parent node can also be the of_node of the main SoC device
>>>>>>>> node.
>>>>>>>>
>>>>>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@...il.com>
>>>>>>>> ---
>>>>>>>> drivers/nvmem/core.c | 44 +++++++++++++++++++++++++++++-------------
>>>>>>>> include/linux/nvmem-consumer.h | 1 +
>>>>>>>> 2 files changed, 32 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>>>>>> index 965911d..470abee 100644
>>>>>>>> --- a/drivers/nvmem/core.c
>>>>>>>> +++ b/drivers/nvmem/core.c
>>>>>>>> @@ -743,29 +743,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
>>>>>>>>
>>>>>>>> #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
>>>>>>>> /**
>>>>>>>> - * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>>>>>>>> + * of_nvmem_cell_get_direct() - Get a nvmem cell from given device node
>>>>>>>> *
>>>>>>>> - * @dev node: Device tree node that uses the nvmem cell
>>>>>>>> - * @id: nvmem cell name from nvmem-cell-names property.
>>>>>>>> + * @dev node: Device tree node that uses nvmem cell
>>>>>>>> *
>>>>>>>> * Return: Will be an ERR_PTR() on error or a valid pointer
>>>>>>>> * to a struct nvmem_cell. The nvmem_cell will be freed by the
>>>>>>>> * nvmem_cell_put().
>>>>>>>> */
>>>>>>>> -struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>>>>>>> - const char *name)
>>>>>>>> +struct nvmem_cell *of_nvmem_cell_get_direct(struct device_node *cell_np)
>>>>>>>> {
>>>>>>>> - struct device_node *cell_np, *nvmem_np;
>>>>>>>> + struct device_node *nvmem_np;
>>>>>>>> struct nvmem_cell *cell;
>>>>>>>> struct nvmem_device *nvmem;
>>>>>>>> const __be32 *addr;
>>>>>>>> - int rval, len, index;
>>>>>>>> -
>>>>>>>> - index = of_property_match_string(np, "nvmem-cell-names", name);
>>>>>>>> -
>>>>>>>> - cell_np = of_parse_phandle(np, "nvmem-cells", index);
>>>>>>>> - if (!cell_np)
>>>>>>>> - return ERR_PTR(-EINVAL);
>>>>>>>> + int rval, len;
>>>>>>>>
>>>>>>>> nvmem_np = of_get_next_parent(cell_np);
>>>>>>>> if (!nvmem_np)
>>>>>>>> @@ -824,6 +816,32 @@ err_mem:
>>>>>>>>
>>>>>>>> return ERR_PTR(rval);
>>>>>>>> }
>>>>>>>> +EXPORT_SYMBOL_GPL(of_nvmem_cell_get_direct);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>>>>>>>> + *
>>>>>>>> + * @dev node: Device tree node that uses the nvmem cell
>>>>>>>> + * @id: nvmem cell name from nvmem-cell-names property.
>>>>>>>> + *
>>>>>>>> + * Return: Will be an ERR_PTR() on error or a valid pointer
>>>>>>>> + * to a struct nvmem_cell. The nvmem_cell will be freed by the
>>>>>>>> + * nvmem_cell_put().
>>>>>>>> + */
>>>>>>>> +struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>>>>>>> + const char *name)
>>>>>>>> +{
>>>>>>>> + struct device_node *cell_np;
>>>>>>>> + int index;
>>>>>>>> +
>>>>>>>> + index = of_property_match_string(np, "nvmem-cell-names", name);
>>>>>>>> +
>>>>>>>> + cell_np = of_parse_phandle(np, "nvmem-cells", index);
>>>>>>>> + if (!cell_np)
>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>> +
>>>>>>>> + return of_nvmem_cell_get_direct(cell_np);
>>>>>>>> +}
>>>>>>>> EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
>>>>>>>> index 9bb77d3..bf879fc 100644
>>>>>>>> --- a/include/linux/nvmem-consumer.h
>>>>>>>> +++ b/include/linux/nvmem-consumer.h
>>>>>>>> @@ -136,6 +136,7 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
>>>>>>>> #endif /* CONFIG_NVMEM */
>>>>>>>>
>>>>>>>> #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
>>>>>>>> +struct nvmem_cell *of_nvmem_cell_get_direct(struct device_node *cell_np);
>>>>>>>> struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>>>>>>> const char *name);
>>>>>>>> struct nvmem_device *of_nvmem_device_get(struct device_node *np,
>>>>>>>>
Powered by blists - more mailing lists