| Message ID | 20220825214423.903672-7-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 25.08.2022 23:44, Michael Walle wrote: > 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. Not really true (see Broadcom's NVRAM and U-Boot's env data).
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. > > 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. > + > +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. > + > + 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. --srini > + int (*add_cells)(struct nvmem_device *nvmem, struct nvmem_layout *layout); > + struct list_head node; > +}; > + > #if IS_ENABLED(CONFIG_NVMEM) > > struct nvmem_device *nvmem_register(const struct nvmem_config *cfg); > @@ -141,6 +163,10 @@ void nvmem_del_cell_table(struct nvmem_cell_table *table); > int nvmem_add_one_cell(struct nvmem_device *nvmem, > const struct nvmem_cell_info *info); > > +int nvmem_register_layout(struct nvmem_layout *layout); > +const void *nvmem_layout_get_match_data(struct nvmem_device *nvmem, > + struct nvmem_layout *layout); > + > #else > > static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c) > @@ -161,5 +187,17 @@ static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {} > static inline int nvmem_add_one_cell(struct nvmem_device *nvmem, > const struct nvmem_cell_info *info) {} > > +static inline int nvmem_register_layout(struct nvmem_layout *layout) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline const void * > +nvmem_layout_get_match_data(struct nvmem_device *nvmem, > + struct nvmem_layout *layout) > +{ > + return NULL; > +} > + > #endif /* CONFIG_NVMEM */ > #endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
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? -michael
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
Am 2022-08-30 16:43, schrieb Srinivas Kandagatla: >>>> 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? Yes I get what you mean. BUT I'm not so sure; it mixes different things together. Layouts will add cells, analogue to nvmem_add_cells_from_of() or nvmem_add_cells_from_table(). With the hook above, the layout mechanism is abused to add post processing to cells added by other means. What is then the difference to the driver having that "global" post process hook? The correct place to add the per-cell hook in this case would be nvmem_add_cells_from_of(). -michael
On 30/08/2022 16:02, Michael Walle wrote: > Am 2022-08-30 16:43, schrieb Srinivas Kandagatla: > >>>>> 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? > > Yes I get what you mean. BUT I'm not so sure; it mixes different > things together. Layouts will add cells, analogue to > nvmem_add_cells_from_of() or nvmem_add_cells_from_table(). With > the hook above, the layout mechanism is abused to add post > processing to cells added by other means. We are still defining what layout exactly mean w.r.t to nvmem :-) > There are two aspects to this as nvmem core is concerned 1> parse and add cells based on some provider specific algo/stucture. 2> post process cell data before user can see it. In some cases we need 1 and 2 while in other cases we just need 1 or 2. Having an unified interface would help with maintenance and removing duplication. > What is then the difference to the driver having that "global" > post process hook? w.r.t post processing there should be no difference. cell can have no post-processing or a default post processing or a specific one depending on its configuration. > > The correct place to add the per-cell hook in this case would be > nvmem_add_cells_from_of(). yes, that is the place where it should go. we have to work on the details but if provider is associated with a layout then this should be doable. --srini > > -michael
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); + +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); + + 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; + int (*add_cells)(struct nvmem_device *nvmem, struct nvmem_layout *layout); + struct list_head node; +}; + #if IS_ENABLED(CONFIG_NVMEM) struct nvmem_device *nvmem_register(const struct nvmem_config *cfg); @@ -141,6 +163,10 @@ void nvmem_del_cell_table(struct nvmem_cell_table *table); int nvmem_add_one_cell(struct nvmem_device *nvmem, const struct nvmem_cell_info *info); +int nvmem_register_layout(struct nvmem_layout *layout); +const void *nvmem_layout_get_match_data(struct nvmem_device *nvmem, + struct nvmem_layout *layout); + #else static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c) @@ -161,5 +187,17 @@ static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {} static inline int nvmem_add_one_cell(struct nvmem_device *nvmem, const struct nvmem_cell_info *info) {} +static inline int nvmem_register_layout(struct nvmem_layout *layout) +{ + return -EOPNOTSUPP; +} + +static inline const void * +nvmem_layout_get_match_data(struct nvmem_device *nvmem, + struct nvmem_layout *layout) +{ + return NULL; +} + #endif /* CONFIG_NVMEM */ #endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
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