[PATCH v1 06/14] nvmem: core: introduce NVMEM layouts
Srinivas Kandagatla
srinivas.kandagatla at linaro.org
Tue Aug 30 07:43:00 PDT 2022
On 30/08/2022 15:24, Michael Walle wrote:
> Am 2022-08-30 15:36, schrieb Srinivas Kandagatla:
>> On 25/08/2022 22:44, Michael Walle wrote:
>>> NVMEM layouts are used to generate NVMEM cells during runtime. Think of
>>> an EEPROM with a well-defined conent. For now, the content can be
>>> described by a device tree or a board file. But this only works if the
>>> offsets and lengths are static and don't change. One could also argue
>>> that putting the layout of the EEPROM in the device tree is the wrong
>>> place. Instead, the device tree should just have a specific compatible
>>> string.
>>>
>>> Right now there are two use cases:
>>> (1) The NVMEM cell needs special processing. E.g. if it only specifies
>>> a base MAC address offset and you need to add an offset, or it
>>> needs to parse a MAC from ASCII format or some proprietary format.
>>> (Post processing of cells is added in a later commit).
>>> (2) u-boot environment parsing. The cells don't have a particular
>>> offset but it needs parsing the content to determine the offsets
>>> and length.
>>>
>>> Signed-off-by: Michael Walle <michael at walle.cc>
>>> ---
>>> drivers/nvmem/Kconfig | 2 ++
>>> drivers/nvmem/Makefile | 1 +
>>> drivers/nvmem/core.c | 57 ++++++++++++++++++++++++++++++++++
>>> drivers/nvmem/layouts/Kconfig | 5 +++
>>> drivers/nvmem/layouts/Makefile | 4 +++
>>> include/linux/nvmem-provider.h | 38 +++++++++++++++++++++++
>>> 6 files changed, 107 insertions(+)
>>> create mode 100644 drivers/nvmem/layouts/Kconfig
>>> create mode 100644 drivers/nvmem/layouts/Makefile
>>
>> update to ./Documentation/driver-api/nvmem.rst would help others.
>
> Sure. Didn't know about that one.
>
>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>> index bab8a29c9861..1416837b107b 100644
>>> --- a/drivers/nvmem/Kconfig
>>> +++ b/drivers/nvmem/Kconfig
>>> @@ -357,4 +357,6 @@ config NVMEM_U_BOOT_ENV
>>> If compiled as module it will be called nvmem_u-boot-env.
>>> +source "drivers/nvmem/layouts/Kconfig"
>>> +
>>> endif
>>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>>> index 399f9972d45b..cd5a5baa2f3a 100644
>>> --- a/drivers/nvmem/Makefile
>>> +++ b/drivers/nvmem/Makefile
>>> @@ -5,6 +5,7 @@
>>> obj-$(CONFIG_NVMEM) += nvmem_core.o
>>> nvmem_core-y := core.o
>>> +obj-y += layouts/
>>> # Devices
>>> obj-$(CONFIG_NVMEM_BCM_OCOTP) += nvmem-bcm-ocotp.o
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index 3dfd149374a8..5357fc378700 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -74,6 +74,9 @@ static LIST_HEAD(nvmem_lookup_list);
>>> static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>>> +static DEFINE_SPINLOCK(nvmem_layout_lock);
>>> +static LIST_HEAD(nvmem_layouts);
>>> +
>>> static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned
>>> int offset,
>>> void *val, size_t bytes)
>>> {
>>> @@ -744,6 +747,56 @@ static int nvmem_add_cells_from_of(struct
>>> nvmem_device *nvmem)
>>> return 0;
>>> }
>>> +int nvmem_register_layout(struct nvmem_layout *layout)
>>> +{
>>> + spin_lock(&nvmem_layout_lock);
>>> + list_add(&layout->node, &nvmem_layouts);
>>> + spin_unlock(&nvmem_layout_lock);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(nvmem_register_layout);
>>
>> we should provide nvmem_unregister_layout too, so that providers can
>> add them if they can in there respective drivers.
>
> Actually, that was the idea; that you can have layouts outside of layouts/.
> I also had a nvmem_unregister_layout() but removed it because it was dead
> code. Will re-add it again.
>
>>> +
>>> +static struct nvmem_layout *nvmem_get_compatible_layout(struct
>>> device_node *np)
>>> +{
>>> + struct nvmem_layout *p, *ret = NULL;
>>> +
>>> + spin_lock(&nvmem_layout_lock);
>>> +
>>> + list_for_each_entry(p, &nvmem_layouts, node) {
>>> + if (of_match_node(p->of_match_table, np)) {
>>> + ret = p;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + spin_unlock(&nvmem_layout_lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int nvmem_add_cells_from_layout(struct nvmem_device *nvmem)
>>> +{
>>> + struct nvmem_layout *layout;
>>> +
>>> + layout = nvmem_get_compatible_layout(nvmem->dev.of_node);
>>> + if (layout)
>>> + layout->add_cells(&nvmem->dev, nvmem, layout);
>>
>> access to add_cells can crash hear as we did not check it before
>> adding in to list.
>> Or
>> we could relax add_cells callback for usecases like imx-octop.
>
> good catch, will use layout && layout->add_cells.
>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +const void *nvmem_layout_get_match_data(struct nvmem_device *nvmem,
>>> + struct nvmem_layout *layout)
>>> +{
>>> + const struct of_device_id *match;
>>> +
>>> + match = of_match_node(layout->of_match_table, nvmem->dev.of_node);
>>> +
>>> + return match ? match->data : NULL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(nvmem_layout_get_match_data);
>>> +
>>> /**
>>> * nvmem_register() - Register a nvmem device for given nvmem_config.
>>> * Also creates a binary entry in
>>> /sys/bus/nvmem/devices/dev-name/nvmem
>>> @@ -872,6 +925,10 @@ struct nvmem_device *nvmem_register(const struct
>>> nvmem_config *config)
>>> if (rval)
>>> goto err_remove_cells;
>>> + rval = nvmem_add_cells_from_layout(nvmem);
>>> + if (rval)
>>> + goto err_remove_cells;
>>> +
>>> blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>>> return nvmem;
>>> diff --git a/drivers/nvmem/layouts/Kconfig
>>> b/drivers/nvmem/layouts/Kconfig
>>> new file mode 100644
>>> index 000000000000..9ad3911d1605
>>> --- /dev/null
>>> +++ b/drivers/nvmem/layouts/Kconfig
>>> @@ -0,0 +1,5 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +menu "Layout Types"
>>> +
>>> +endmenu
>>> diff --git a/drivers/nvmem/layouts/Makefile
>>> b/drivers/nvmem/layouts/Makefile
>>> new file mode 100644
>>> index 000000000000..6fdb3c60a4fa
>>> --- /dev/null
>>> +++ b/drivers/nvmem/layouts/Makefile
>>> @@ -0,0 +1,4 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +#
>>> +# Makefile for nvmem layouts.
>>> +#
>>> diff --git a/include/linux/nvmem-provider.h
>>> b/include/linux/nvmem-provider.h
>>> index e710404959e7..323685841e9f 100644
>>> --- a/include/linux/nvmem-provider.h
>>> +++ b/include/linux/nvmem-provider.h
>>> @@ -127,6 +127,28 @@ struct nvmem_cell_table {
>>> struct list_head node;
>>> };
>>> +/**
>>> + * struct nvmem_layout - NVMEM layout definitions
>>> + *
>>> + * @name: Layout name.
>>> + * @of_match_table: Open firmware match table.
>>> + * @add_cells: Will be called if a nvmem device is found which
>>> + * has this layout. The function will add layout
>>> + * specific cells with nvmem_add_one_cell().
>>> + * @node: List node.
>>> + *
>>> + * A nvmem device can hold a well defined structure which can just be
>>> + * evaluated during runtime. For example a TLV list, or a list of
>>> "name=val"
>>> + * pairs. A nvmem layout can parse the nvmem device and add appropriate
>>> + * cells.
>>> + */
>>> +struct nvmem_layout {
>>> + const char *name;
>>> + const struct of_device_id *of_match_table;
>>
>> looking at this, I think its doable to convert the existing
>> cell_post_process callback to layouts by adding a layout specific
>> callback here.
>
> can you elaborate on that?
If we relax add_cells + add nvmem_unregister_layout() and update struct
nvmem_layout to include post_process callback like
struct nvmem_layout {
const char *name;
const struct of_device_id *of_match_table;
int (*add_cells)(struct nvmem_device *nvmem, struct nvmem_layout *layout);
struct list_head node;
/* default callback for every cell */
nvmem_cell_post_process_t post_process;
};
then we can move imx-ocotp to this layout style without add_cell
callback, and finally get rid of cell_process_callback from both
nvmem_config and nvmem_device.
If layout specific post_process callback is available and cell does not
have a callback set then we can can be either updated cell post_process
callback with this one or invoke layout specific callback directly.
does that make sense?
--srini
>
> -michael
More information about the linux-mtd
mailing list