Messages in this thread | | | Date | Tue, 30 Aug 2022 15:43:00 +0100 | Subject | Re: [PATCH v1 06/14] nvmem: core: introduce NVMEM layouts | From | Srinivas Kandagatla <> |
| |
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@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
| |