* [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser
@ 2021-06-08 19:03 Vadym Kochan
2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
` (2 more replies)
0 siblings, 3 replies; 36+ messages in thread
From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw)
To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
Cc: Robert Marko, Vadym Kochan
This series adds cells parser for the ONIE TLV attributes which are
stored on NVMEM device. It adds possibility to read the mac address (and
other info) by other drivers.
Because ONIE stores info in TLV format it should be parsed first and
then register the cells. Current NVMEM API allows to register cell
table with known cell's offset which is not guaranteed in case of TLV.
To make it properly handled the NVMEM parser object is introduced. The
parser needs to be registered before target NVMEM device is registered.
During the registration of NVMEM device the parser is called to parse
the device's cells and reister the cell table.
v2:
1) Series title changed, "provider" is replaced by "parser".
2) onie-cells.c renamed to onie-tlv-cells.c
3) Do not register onie cells parser via platform driver,
but via module_init().
4) Removed cells_remove callback from nvmem_parser_config because
now cells table and lookups are registered by core.c
5) Introduced nvmem_parser_data which is passed to parser
handler/driver to fill cells table & lookups.
6) core.c registers cells table & lookups to simplify the parser
handler logic.
7) parser's module refcount is incremented/decremented on each parser
bind/unbind to nvmem device.
8) core.c relies on parser name which is obtained via
device_property API, instead of of_node which was used
in previous version.
9) nvmem-cell-parser-name DT nvmem property was added to bind
cells parser to nvmem device during the nvmem registration.
P.S.
I marked it with v2 as relative to series which was sent as:
"nvmem: add ONIE NVMEM cells provider"
Vadym Kochan (3):
nvmem: core: introduce cells parser
dt-bindings: nvmem: document nvmem-cells-parser-name property
nvmem: add ONIE nvmem cells parser
.../devicetree/bindings/nvmem/nvmem.yaml | 5 +
drivers/nvmem/Kconfig | 9 +
drivers/nvmem/Makefile | 2 +
drivers/nvmem/core.c | 178 +++++++++++
drivers/nvmem/onie-tlv-cells.c | 302 ++++++++++++++++++
include/linux/nvmem-provider.h | 31 ++
6 files changed, 527 insertions(+)
create mode 100644 drivers/nvmem/onie-tlv-cells.c
--
2.17.1
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan @ 2021-06-08 19:03 ` Vadym Kochan 2021-06-08 22:49 ` kernel test robot ` (2 more replies) 2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan 2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan 2 siblings, 3 replies; 36+ messages in thread From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree Cc: Robert Marko, Vadym Kochan Current implementation does not allow to register cells for already registered nvmem device and requires that this should be done before. But there might a driver which needs to parse the nvmem device and after register a cells table. Introduce nvmem_parser API which allows to register cells parser which is called during nvmem device registration. During this stage the parser can read the nvmem device and register the cells table. Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> --- v2: 1) Added nvmem_parser_data so parser implementation should fill it with cells table and lookups which will be registered by core.c after cells_parse was succeed. 2) Removed cells_remove callback from nvmem_parser_config which is not needed because cells table & lookups are registered/unregistered automatically by core. 3) Use new device property to read cells parser name during nvmem registration. Removed of_node usage. 4) parser's module refcount is incremented/decremented on each parser bind/unbind to nvmem device. drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++ include/linux/nvmem-provider.h | 31 ++++++ 2 files changed, 209 insertions(+) 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; void *priv; }; @@ -57,6 +58,13 @@ struct nvmem_cell { struct list_head node; }; +struct nvmem_parser { + struct list_head head; + nvmem_parse_t cells_parse; + const char *name; + struct module *owner; +}; + static DEFINE_MUTEX(nvmem_mutex); static DEFINE_IDA(nvmem_ida); @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables); static DEFINE_MUTEX(nvmem_lookup_mutex); static LIST_HEAD(nvmem_lookup_list); +static DEFINE_MUTEX(nvmem_parser_mutex); +static LIST_HEAD(nvmem_parser_list); + static BLOCKING_NOTIFIER_HEAD(nvmem_notifier); static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = { .name = "nvmem", }; +static struct nvmem_parser *__nvmem_parser_get(const char *name) +{ + struct nvmem_parser *tmp, *parser = NULL; + + list_for_each_entry(tmp, &nvmem_parser_list, head) { + if (strcmp(name, tmp->name) == 0) { + parser = tmp; + break; + } + } + + if (!parser) + return ERR_PTR(-EPROBE_DEFER); + + if (!try_module_get(parser->owner)) { + pr_err("could not increase module refcount for parser %s\n", + parser->name); + return ERR_PTR(-EINVAL); + + } + + return parser; +} + +static void nvmem_parser_put(const struct nvmem_parser *parser) +{ + module_put(parser->owner); +} + +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name) +{ + struct nvmem_parser_data *data; + struct nvmem_parser *parser; + int err; + + mutex_lock(&nvmem_parser_mutex); + + parser = __nvmem_parser_get(name); + err = PTR_ERR_OR_ZERO(parser); + if (!err) { + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (data) { + data->parser = parser; + nvmem->parser_data = data; + } else { + nvmem_parser_put(parser); + err = -ENOMEM; + } + } + + mutex_unlock(&nvmem_parser_mutex); + + return err; +} + +static void nvmem_parser_unbind(const struct nvmem_device *nvmem) +{ + struct nvmem_parser_data *data = nvmem->parser_data; + + if (data->table.cells) { + nvmem_del_cell_table(&data->table); + kfree(data->table.cells); + } + if (data->lookup) { + nvmem_del_cell_lookups(data->lookup, data->nlookups); + kfree(data->lookup); + } + + nvmem_parser_put(data->parser); +} + +static void nvmem_parser_data_register(struct nvmem_device *nvmem, + struct nvmem_parser_data *data) +{ + if (data->table.cells) { + if (!data->table.nvmem_name) + data->table.nvmem_name = nvmem_dev_name(nvmem); + + nvmem_add_cell_table(&data->table); + } + + if (data->lookup) { + struct nvmem_cell_lookup *lookup = &data->lookup[0]; + int i = 0; + + for (i = 0; i < data->nlookups; i++, lookup++) { + if (!lookup->nvmem_name) + lookup->nvmem_name = nvmem_dev_name(nvmem); + + if (!lookup->dev_id) + lookup->dev_id = data->parser->name; + } + + nvmem_add_cell_lookups(data->lookup, data->nlookups); + } +} + +static void nvmem_cells_parse(struct nvmem_device *nvmem) +{ + struct nvmem_parser_data *data = nvmem->parser_data; + struct nvmem_parser *parser = data->parser; + int err; + + mutex_lock(&nvmem_parser_mutex); + + err = parser->cells_parse(nvmem, data); + if (!err) + nvmem_parser_data_register(nvmem, data); + + mutex_unlock(&nvmem_parser_mutex); +} + static void nvmem_cell_drop(struct nvmem_cell *cell) { blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell); @@ -435,6 +558,9 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem) list_for_each_entry_safe(cell, p, &nvmem->cells, node) nvmem_cell_drop(cell); + + if (nvmem->parser_data) + nvmem_parser_unbind(nvmem); } static void nvmem_cell_add(struct nvmem_cell *cell) @@ -739,6 +865,7 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem) struct nvmem_device *nvmem_register(const struct nvmem_config *config) { struct nvmem_device *nvmem; + const char *parser_name; int rval; if (!config->dev) @@ -809,6 +936,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) nvmem->read_only = device_property_present(config->dev, "read-only") || config->read_only || !nvmem->reg_write; + if (!device_property_read_string(config->dev, "nvmem-cell-parser-name", + &parser_name)) { + rval = nvmem_parser_bind(nvmem, parser_name); + if (rval) { + ida_free(&nvmem_ida, nvmem->id); + kfree(nvmem); + return ERR_PTR(rval); + } + } + #ifdef CONFIG_NVMEM_SYSFS nvmem->dev.groups = nvmem_dev_groups; #endif @@ -837,6 +974,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) goto err_teardown_compat; } + if (nvmem->parser_data) + nvmem_cells_parse(nvmem); + rval = nvmem_add_cells_from_table(nvmem); if (rval) goto err_remove_cells; @@ -857,6 +997,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) err_device_del: device_del(&nvmem->dev); err_put_device: + if (nvmem->parser_data) + nvmem_parser_unbind(nvmem); put_device(&nvmem->dev); return ERR_PTR(rval); @@ -1891,6 +2033,42 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem) } EXPORT_SYMBOL_GPL(nvmem_dev_name); +struct nvmem_parser * +nvmem_parser_register(const struct nvmem_parser_config *config) +{ + struct nvmem_parser *parser; + + if (!config->cells_parse) + return ERR_PTR(-EINVAL); + + if (!config->name) + return ERR_PTR(-EINVAL); + + parser = kzalloc(sizeof(*parser), GFP_KERNEL); + if (!parser) + return ERR_PTR(-ENOMEM); + + parser->cells_parse = config->cells_parse; + parser->owner = config->owner; + parser->name = config->name; + + mutex_lock(&nvmem_parser_mutex); + list_add(&parser->head, &nvmem_parser_list); + mutex_unlock(&nvmem_parser_mutex); + + return parser; +} +EXPORT_SYMBOL(nvmem_parser_register); + +void nvmem_parser_unregister(struct nvmem_parser *parser) +{ + mutex_lock(&nvmem_parser_mutex); + list_del(&parser->head); + kfree(parser); + mutex_unlock(&nvmem_parser_mutex); +} +EXPORT_SYMBOL(nvmem_parser_unregister); + static int __init nvmem_init(void) { return bus_register(&nvmem_bus_type); diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h index e162b757b6d5..447241706554 100644 --- a/include/linux/nvmem-provider.h +++ b/include/linux/nvmem-provider.h @@ -15,10 +15,15 @@ struct nvmem_device; struct nvmem_cell_info; +struct nvmem_cell_table; +struct nvmem_parser_data; + typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset, void *val, size_t bytes); typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, void *val, size_t bytes); +typedef int (*nvmem_parse_t)(struct nvmem_device *nvmem, + struct nvmem_parser_data *data); enum nvmem_type { NVMEM_TYPE_UNKNOWN = 0, @@ -117,6 +122,19 @@ struct nvmem_cell_table { struct list_head node; }; +struct nvmem_parser_config { + const char *name; + nvmem_parse_t cells_parse; + struct module *owner; +}; + +struct nvmem_parser_data { + struct nvmem_cell_table table; + struct nvmem_cell_lookup *lookup; + int nlookups; + struct nvmem_parser *parser; +}; + #if IS_ENABLED(CONFIG_NVMEM) struct nvmem_device *nvmem_register(const struct nvmem_config *cfg); @@ -130,6 +148,11 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem); void nvmem_add_cell_table(struct nvmem_cell_table *table); void nvmem_del_cell_table(struct nvmem_cell_table *table); +struct nvmem_parser * +nvmem_parser_register(const struct nvmem_parser_config *config); + +void nvmem_parser_unregister(struct nvmem_parser *parser); + #else static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c) @@ -154,5 +177,13 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem) static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {} static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {} +static inline struct nvmem_parser * +nvmem_parser_register(const struct nvmem_parser_config *config) +{ + return -EOPNOTSUPP; +} + +static inline void nvmem_parser_unregister(struct nvmem_parser *parser) {} + #endif /* CONFIG_NVMEM */ #endif /* ifndef _LINUX_NVMEM_PROVIDER_H */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan @ 2021-06-08 22:49 ` kernel test robot 2021-06-09 3:05 ` kernel test robot 2021-06-14 10:44 ` Srinivas Kandagatla 2 siblings, 0 replies; 36+ messages in thread From: kernel test robot @ 2021-06-08 22:49 UTC (permalink / raw) To: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree Cc: kbuild-all, Robert Marko, Vadym Kochan [-- Attachment #1: Type: text/plain, Size: 2230 bytes --] Hi Vadym, I love your patch! Yet something to improve: [auto build test ERROR on robh/for-next] [also build test ERROR on linux/master linus/master v5.13-rc5 next-20210608] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: powerpc-allnoconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/rtc.h:18, from arch/powerpc/kernel/time.c:47: include/linux/nvmem-provider.h: In function 'nvmem_parser_register': >> include/linux/nvmem-provider.h:183:9: error: returning 'int' from a function with return type 'struct nvmem_parser *' makes pointer from integer without a cast [-Werror=int-conversion] 183 | return -EOPNOTSUPP; | ^ cc1: all warnings being treated as errors vim +183 include/linux/nvmem-provider.h 179 180 static inline struct nvmem_parser * 181 nvmem_parser_register(const struct nvmem_parser_config *config) 182 { > 183 return -EOPNOTSUPP; 184 } 185 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 7138 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser @ 2021-06-08 22:49 ` kernel test robot 0 siblings, 0 replies; 36+ messages in thread From: kernel test robot @ 2021-06-08 22:49 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2283 bytes --] Hi Vadym, I love your patch! Yet something to improve: [auto build test ERROR on robh/for-next] [also build test ERROR on linux/master linus/master v5.13-rc5 next-20210608] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: powerpc-allnoconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/rtc.h:18, from arch/powerpc/kernel/time.c:47: include/linux/nvmem-provider.h: In function 'nvmem_parser_register': >> include/linux/nvmem-provider.h:183:9: error: returning 'int' from a function with return type 'struct nvmem_parser *' makes pointer from integer without a cast [-Werror=int-conversion] 183 | return -EOPNOTSUPP; | ^ cc1: all warnings being treated as errors vim +183 include/linux/nvmem-provider.h 179 180 static inline struct nvmem_parser * 181 nvmem_parser_register(const struct nvmem_parser_config *config) 182 { > 183 return -EOPNOTSUPP; 184 } 185 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 7138 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan @ 2021-06-09 3:05 ` kernel test robot 2021-06-09 3:05 ` kernel test robot 2021-06-14 10:44 ` Srinivas Kandagatla 2 siblings, 0 replies; 36+ messages in thread From: kernel test robot @ 2021-06-09 3:05 UTC (permalink / raw) To: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree Cc: kbuild-all, clang-built-linux, Robert Marko, Vadym Kochan [-- Attachment #1: Type: text/plain, Size: 24230 bytes --] Hi Vadym, I love your patch! Perhaps something to improve: [auto build test WARNING on robh/for-next] [also build test WARNING on linux/master linus/master v5.13-rc5 next-20210608] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: s390-randconfig-r012-20210608 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d2012d965d60c3258b3a69d024491698f8aec386) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from lib/vsprintf.c:36: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ In file included from lib/vsprintf.c:40: In file included from include/net/addrconf.h:50: In file included from include/linux/ipv6.h:88: In file included from include/linux/tcp.h:17: In file included from include/linux/skbuff.h:31: In file included from include/linux/dma-mapping.h:10: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from lib/vsprintf.c:40: In file included from include/net/addrconf.h:50: In file included from include/linux/ipv6.h:88: In file included from include/linux/tcp.h:17: In file included from include/linux/skbuff.h:31: In file included from include/linux/dma-mapping.h:10: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from lib/vsprintf.c:40: In file included from include/net/addrconf.h:50: In file included from include/linux/ipv6.h:88: In file included from include/linux/tcp.h:17: In file included from include/linux/skbuff.h:31: In file included from include/linux/dma-mapping.h:10: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ 13 warnings generated. -- In file included from lib/test_printf.c:13: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ lib/test_printf.c:157:52: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:55: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:58: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:63: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:68: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:52: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:55: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:58: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:63: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:68: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:159:41: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); ~~~ ^~~~ %o lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:159:47: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); ~~~ ^~~~ %o lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:159:53: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); ~~~~ ^~~~~~ %#o lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ 14 warnings generated. -- In file included from block/partitions/msdos.c:31: In file included from block/partitions/check.h:3: In file included from include/linux/blkdev.h:25: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from block/partitions/msdos.c:31: In file included from block/partitions/check.h:3: In file included from include/linux/blkdev.h:25: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from block/partitions/msdos.c:31: In file included from block/partitions/check.h:3: In file included from include/linux/blkdev.h:25: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ In file included from block/partitions/msdos.c:32: In file included from block/partitions/efi.h:20: In file included from include/linux/efi.h:20: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ 13 warnings generated. -- In file included from kernel/time/ntp.c:10: In file included from include/linux/clocksource.h:22: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from kernel/time/ntp.c:10: In file included from include/linux/clocksource.h:22: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from kernel/time/ntp.c:10: In file included from include/linux/clocksource.h:22: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ In file included from kernel/time/ntp.c:19: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ kernel/time/ntp.c:245:19: warning: unused function 'ntp_synced' [-Wunused-function] static inline int ntp_synced(void) ^ 14 warnings generated. .. vim +183 include/linux/nvmem-provider.h 179 180 static inline struct nvmem_parser * 181 nvmem_parser_register(const struct nvmem_parser_config *config) 182 { > 183 return -EOPNOTSUPP; 184 } 185 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 15185 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser @ 2021-06-09 3:05 ` kernel test robot 0 siblings, 0 replies; 36+ messages in thread From: kernel test robot @ 2021-06-09 3:05 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 24594 bytes --] Hi Vadym, I love your patch! Perhaps something to improve: [auto build test WARNING on robh/for-next] [also build test WARNING on linux/master linus/master v5.13-rc5 next-20210608] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: s390-randconfig-r012-20210608 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d2012d965d60c3258b3a69d024491698f8aec386) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008 git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from lib/vsprintf.c:36: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ In file included from lib/vsprintf.c:40: In file included from include/net/addrconf.h:50: In file included from include/linux/ipv6.h:88: In file included from include/linux/tcp.h:17: In file included from include/linux/skbuff.h:31: In file included from include/linux/dma-mapping.h:10: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from lib/vsprintf.c:40: In file included from include/net/addrconf.h:50: In file included from include/linux/ipv6.h:88: In file included from include/linux/tcp.h:17: In file included from include/linux/skbuff.h:31: In file included from include/linux/dma-mapping.h:10: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from lib/vsprintf.c:40: In file included from include/net/addrconf.h:50: In file included from include/linux/ipv6.h:88: In file included from include/linux/tcp.h:17: In file included from include/linux/skbuff.h:31: In file included from include/linux/dma-mapping.h:10: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ 13 warnings generated. -- In file included from lib/test_printf.c:13: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ lib/test_printf.c:157:52: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:55: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:58: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:63: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:157:68: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); ~~~~ ^~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:52: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:55: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:58: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:63: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^~~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:158:68: warning: format specifies type 'char' but the argument has type 'int' [-Wformat] test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); ~~~~ ^~ %d lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:159:41: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); ~~~ ^~~~ %o lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:159:47: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); ~~~ ^~~~ %o lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ lib/test_printf.c:159:53: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); ~~~~ ^~~~~~ %#o lib/test_printf.c:137:40: note: expanded from macro 'test' __test(expect, strlen(expect), fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ 14 warnings generated. -- In file included from block/partitions/msdos.c:31: In file included from block/partitions/check.h:3: In file included from include/linux/blkdev.h:25: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from block/partitions/msdos.c:31: In file included from block/partitions/check.h:3: In file included from include/linux/blkdev.h:25: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from block/partitions/msdos.c:31: In file included from block/partitions/check.h:3: In file included from include/linux/blkdev.h:25: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ In file included from block/partitions/msdos.c:32: In file included from block/partitions/efi.h:20: In file included from include/linux/efi.h:20: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ 13 warnings generated. -- In file included from kernel/time/ntp.c:10: In file included from include/linux/clocksource.h:22: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from kernel/time/ntp.c:10: In file included from include/linux/clocksource.h:22: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from kernel/time/ntp.c:10: In file included from include/linux/clocksource.h:22: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ In file included from kernel/time/ntp.c:19: In file included from include/linux/rtc.h:18: >> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion] return -EOPNOTSUPP; ^~~~~~~~~~~ kernel/time/ntp.c:245:19: warning: unused function 'ntp_synced' [-Wunused-function] static inline int ntp_synced(void) ^ 14 warnings generated. .. vim +183 include/linux/nvmem-provider.h 179 180 static inline struct nvmem_parser * 181 nvmem_parser_register(const struct nvmem_parser_config *config) 182 { > 183 return -EOPNOTSUPP; 184 } 185 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 15185 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan 2021-06-08 22:49 ` kernel test robot 2021-06-09 3:05 ` kernel test robot @ 2021-06-14 10:44 ` Srinivas Kandagatla 2021-06-16 12:33 ` Vadym Kochan 2021-09-08 9:44 ` Vadym Kochan 2 siblings, 2 replies; 36+ messages in thread From: Srinivas Kandagatla @ 2021-06-14 10:44 UTC (permalink / raw) To: Vadym Kochan, Rob Herring, linux-kernel, devicetree; +Cc: Robert Marko On 08/06/2021 20:03, Vadym Kochan wrote: > Current implementation does not allow to register cells for already > registered nvmem device and requires that this should be done before. But > there might a driver which needs to parse the nvmem device and after > register a cells table. > > Introduce nvmem_parser API which allows to register cells parser which > is called during nvmem device registration. During this stage the parser > can read the nvmem device and register the cells table. > > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> > --- > v2: > 1) Added nvmem_parser_data so parser implementation > should fill it with cells table and lookups which > will be registered by core.c after cells_parse was > succeed. > > 2) Removed cells_remove callback from nvmem_parser_config which > is not needed because cells table & lookups are > registered/unregistered automatically by core. > > 3) Use new device property to read cells parser name during nvmem > registration. Removed of_node usage. > > 4) parser's module refcount is incremented/decremented on each parser > bind/unbind to nvmem device. > > drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++ > include/linux/nvmem-provider.h | 31 ++++++ > 2 files changed, 209 insertions(+) > > 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. > void *priv; > }; > > @@ -57,6 +58,13 @@ struct nvmem_cell { > struct list_head node; > }; > > +struct nvmem_parser { > + struct list_head head; > + nvmem_parse_t cells_parse; > + const char *name; > + struct module *owner; > +}; > + > static DEFINE_MUTEX(nvmem_mutex); > static DEFINE_IDA(nvmem_ida); > > @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables); > static DEFINE_MUTEX(nvmem_lookup_mutex); > static LIST_HEAD(nvmem_lookup_list); > > +static DEFINE_MUTEX(nvmem_parser_mutex); > +static LIST_HEAD(nvmem_parser_list); > + > static BLOCKING_NOTIFIER_HEAD(nvmem_notifier); > > static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, > @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = { > .name = "nvmem", > }; > > +static struct nvmem_parser *__nvmem_parser_get(const char *name) > +{ > + struct nvmem_parser *tmp, *parser = NULL; > + > + list_for_each_entry(tmp, &nvmem_parser_list, head) { > + if (strcmp(name, tmp->name) == 0) { > + parser = tmp; > + break; > + } > + } > + > + if (!parser) > + return ERR_PTR(-EPROBE_DEFER); > + > + if (!try_module_get(parser->owner)) { > + pr_err("could not increase module refcount for parser %s\n", > + parser->name); > + return ERR_PTR(-EINVAL); > + > + } > + > + return parser; > +} > + > +static void nvmem_parser_put(const struct nvmem_parser *parser) > +{ > + module_put(parser->owner); > +} > + > +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name) > +{ Do we really need parser bind/unbind mechanisms for what we are trying to do here. It's just simply parsing cell info during nvmem register, do we really care if parser is there or not after that? code can be probably made much simpler by just doing this in nvmem_register parser_get() parse_get_cell_info() parser_put() AFAIU, that is all we need. > + struct nvmem_parser_data *data; > + struct nvmem_parser *parser; > + int err; > + > + mutex_lock(&nvmem_parser_mutex); > + > + parser = __nvmem_parser_get(name); > + err = PTR_ERR_OR_ZERO(parser); > + if (!err) { > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (data) { > + data->parser = parser; > + nvmem->parser_data = data; > + } else { > + nvmem_parser_put(parser); > + err = -ENOMEM; > + } > + } > + > + mutex_unlock(&nvmem_parser_mutex); > + > + return err; > +} > + > +static void nvmem_parser_unbind(const struct nvmem_device *nvmem) > +{ > + struct nvmem_parser_data *data = nvmem->parser_data; > + > + if (data->table.cells) { > + nvmem_del_cell_table(&data->table); > + kfree(data->table.cells); who has allocated memory for this, its confusing for this to be freed in core. > + } > + if (data->lookup) { > + nvmem_del_cell_lookups(data->lookup, data->nlookups); > + kfree(data->lookup); > + } > + > + nvmem_parser_put(data->parser); > +} > + > +static void nvmem_parser_data_register(struct nvmem_device *nvmem, > + struct nvmem_parser_data *data) > +{ > + if (data->table.cells) { > + if (!data->table.nvmem_name) > + data->table.nvmem_name = nvmem_dev_name(nvmem); > + > + nvmem_add_cell_table(&data->table); > + } > + > + if (data->lookup) { Why do we need lookups? the cells are already associated with provider. lookups are normally used for associating devices with nvmemcell more for old non-dt machine code. you can remove this. > + struct nvmem_cell_lookup *lookup = &data->lookup[0]; > + int i = 0; > + > + for (i = 0; i < data->nlookups; i++, lookup++) { > + if (!lookup->nvmem_name) > + lookup->nvmem_name = nvmem_dev_name(nvmem); > + > + if (!lookup->dev_id) > + lookup->dev_id = data->parser->name; > + } > + > + nvmem_add_cell_lookups(data->lookup, data->nlookups); > + } > +} > + --srini ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-14 10:44 ` Srinivas Kandagatla @ 2021-06-16 12:33 ` Vadym Kochan 2021-06-21 11:00 ` Srinivas Kandagatla 2021-09-08 9:44 ` Vadym Kochan 1 sibling, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-06-16 12:33 UTC (permalink / raw) To: Srinivas Kandagatla; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko Hi Srinivas, On Mon, Jun 14, 2021 at 11:44:59AM +0100, Srinivas Kandagatla wrote: > > > On 08/06/2021 20:03, Vadym Kochan wrote: > > Current implementation does not allow to register cells for already > > registered nvmem device and requires that this should be done before. But > > there might a driver which needs to parse the nvmem device and after > > register a cells table. > > > > Introduce nvmem_parser API which allows to register cells parser which > > is called during nvmem device registration. During this stage the parser > > can read the nvmem device and register the cells table. > > > > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> > > --- > > v2: > > 1) Added nvmem_parser_data so parser implementation > > should fill it with cells table and lookups which > > will be registered by core.c after cells_parse was > > succeed. > > > > 2) Removed cells_remove callback from nvmem_parser_config which > > is not needed because cells table & lookups are > > registered/unregistered automatically by core. > > > > 3) Use new device property to read cells parser name during nvmem > > registration. Removed of_node usage. > > > > 4) parser's module refcount is incremented/decremented on each parser > > bind/unbind to nvmem device. > > > > drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++ > > include/linux/nvmem-provider.h | 31 ++++++ > > 2 files changed, 209 insertions(+) > > > > 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>; 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. Of course would be great if you can point a way/idea to get rid the need of lookups. > > void *priv; > > }; > > @@ -57,6 +58,13 @@ struct nvmem_cell { > > struct list_head node; > > }; > > +struct nvmem_parser { > > + struct list_head head; > > + nvmem_parse_t cells_parse; > > + const char *name; > > + struct module *owner; > > +}; > > + > > static DEFINE_MUTEX(nvmem_mutex); > > static DEFINE_IDA(nvmem_ida); > > @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables); > > static DEFINE_MUTEX(nvmem_lookup_mutex); > > static LIST_HEAD(nvmem_lookup_list); > > +static DEFINE_MUTEX(nvmem_parser_mutex); > > +static LIST_HEAD(nvmem_parser_list); > > + > > static BLOCKING_NOTIFIER_HEAD(nvmem_notifier); > > static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, > > @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = { > > .name = "nvmem", > > }; > > +static struct nvmem_parser *__nvmem_parser_get(const char *name) > > +{ > > + struct nvmem_parser *tmp, *parser = NULL; > > + > > + list_for_each_entry(tmp, &nvmem_parser_list, head) { > > + if (strcmp(name, tmp->name) == 0) { > > + parser = tmp; > > + break; > > + } > > + } > > + > > + if (!parser) > > + return ERR_PTR(-EPROBE_DEFER); > > + > > + if (!try_module_get(parser->owner)) { > > + pr_err("could not increase module refcount for parser %s\n", > > + parser->name); > > + return ERR_PTR(-EINVAL); > > + > > + } > > + > > + return parser; > > +} > > + > > +static void nvmem_parser_put(const struct nvmem_parser *parser) > > +{ > > + module_put(parser->owner); > > +} > > + > > +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name) > > +{ > Do we really need parser bind/unbind mechanisms for what we are trying to do > here. > > It's just simply parsing cell info during nvmem register, do we really care > if parser is there or not after that? > > code can be probably made much simpler by just doing this in nvmem_register > > parser_get() > parse_get_cell_info() > parser_put() > > AFAIU, that is all we need. > because of need of lookup info (just in case if it is needed) the unbind stage should free the lookups during the nvmem cells removal, but cells table can be freed even during nvmem_register after cells were added. > > + struct nvmem_parser_data *data; > > + struct nvmem_parser *parser; > > + int err; > > + > > + mutex_lock(&nvmem_parser_mutex); > > + > > + parser = __nvmem_parser_get(name); > > + err = PTR_ERR_OR_ZERO(parser); > > + if (!err) { > + data = kzalloc(sizeof(*data), GFP_KERNEL); > > + if (data) { > > + data->parser = parser; > > + nvmem->parser_data = data; > > + } else { > > + nvmem_parser_put(parser); > > + err = -ENOMEM; > > + } > > + } > > + > > + mutex_unlock(&nvmem_parser_mutex); > > + > > + return err; > > +} > > + > > +static void nvmem_parser_unbind(const struct nvmem_device *nvmem) > > +{ > > + struct nvmem_parser_data *data = nvmem->parser_data; > > + > > + if (data->table.cells) { > > + nvmem_del_cell_table(&data->table); > > + kfree(data->table.cells); > who has allocated memory for this, its confusing for this to be freed in > core. Well, looks like I need to call parser->cells_clear() so the parser driver will clear things by itself. > > + } > > + if (data->lookup) { > + nvmem_del_cell_lookups(data->lookup, data->nlookups); > > + kfree(data->lookup); > > + } > > + > > + nvmem_parser_put(data->parser); > > +} > > + > > +static void nvmem_parser_data_register(struct nvmem_device *nvmem, > > + struct nvmem_parser_data *data) > > +{ > > + if (data->table.cells) { > > + if (!data->table.nvmem_name) > > + data->table.nvmem_name = nvmem_dev_name(nvmem); > > + > > + nvmem_add_cell_table(&data->table); > > + } > > + > > + if (data->lookup) { > Why do we need lookups? > the cells are already associated with provider. lookups are normally used > for associating devices with nvmemcell more for old non-dt machine code. > > you can remove this. I hope I replied this in 1st reply. > > + struct nvmem_cell_lookup *lookup = &data->lookup[0]; > > + int i = 0; > > + > > + for (i = 0; i < data->nlookups; i++, lookup++) { > > + if (!lookup->nvmem_name) > > + lookup->nvmem_name = nvmem_dev_name(nvmem); > > + > > + if (!lookup->dev_id) > > + lookup->dev_id = data->parser->name; > > + } > > + > > + nvmem_add_cell_lookups(data->lookup, data->nlookups); > > + } > > +} > > + > > --srini ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-16 12:33 ` Vadym Kochan @ 2021-06-21 11:00 ` Srinivas Kandagatla 2021-09-08 9:38 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-06-21 11:00 UTC (permalink / raw) To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko 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. > 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. > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-21 11:00 ` Srinivas Kandagatla @ 2021-09-08 9:38 ` Vadym Kochan 2021-09-13 14:19 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-08 9:38 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko 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 ? 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. >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-08 9:38 ` Vadym Kochan @ 2021-09-13 14:19 ` Srinivas Kandagatla 2021-09-20 10:24 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-13 14:19 UTC (permalink / raw) To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko 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> }; }; 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. 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. >>> > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-13 14:19 ` Srinivas Kandagatla @ 2021-09-20 10:24 ` Vadym Kochan 2021-09-20 10:36 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-20 10:24 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 10:24 ` Vadym Kochan @ 2021-09-20 10:36 ` Srinivas Kandagatla 2021-09-20 11:25 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-20 10:36 UTC (permalink / raw) To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 20/09/2021 11:24, Vadym Kochan wrote: >>> >>> 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. No, we should hit that use case to start with. nvmem cells are parsed in the core during register path. Parser should parse the cells before this to keep it simple. > > Or, treat cells with length "0" in a special way and allow to update > cell info later.you can update irrespective of the length, as long as this is done before register. > >> }; >> >> 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. > Lets keep the core code clean for now, I would expect the provider driver along with parser function to do update the nodes. --srini >> 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 10:36 ` Srinivas Kandagatla @ 2021-09-20 11:25 ` Vadym Kochan 2021-09-20 11:32 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-20 11:25 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko Just to clarify regarding the interfaces: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > On 20/09/2021 11:24, Vadym Kochan wrote: >>>> >>>> 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. > No, we should hit that use case to start with. > > nvmem cells are parsed in the core during register path. > Parser should parse the cells before this to keep it simple. > >> >> Or, treat cells with length "0" in a special way and allow to update >> cell info later.you can update irrespective of the length, as long as this is done > before register. > > >> >>> }; >>> >>> 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. >> > > Lets keep the core code clean for now, I would expect the provider > driver along with parser function to do update the nodes. > > --srini > core.c sequence: 1) after cells parsed from OF: 2) lookup the parser 3) parser->cells_parse(ctx, table) 3.a) update existing cells matched by name from table 4) parser->cells_clean(ctx, table) /* to free cell's_info allocated by the parser driver */ as alternative way, I think it would be more generic to provide nvmem-provider.h API to update the existing cell info, however it makes sense only when cells were parsed by the cell parser, in the other situations the cell should be well defined. with this approach the parser driver will be just called via parser->cells_parse(ctx) and will call nvmem_cell_info_update() for each parsed cells. >>> 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 11:25 ` Vadym Kochan @ 2021-09-20 11:32 ` Srinivas Kandagatla 2021-09-20 12:29 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-20 11:32 UTC (permalink / raw) To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 20/09/2021 12:25, Vadym Kochan wrote: >>> Or, treat cells with length "0" in a special way and allow to update >>> cell info later.you can update irrespective of the length, as long as this is done >> before register. >> >> >>>> }; >>>> >>>> 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. >>> >> Lets keep the core code clean for now, I would expect the provider >> driver along with parser function to do update the nodes. >> >> --srini >> > core.c sequence: > > 1) after cells parsed from OF: > > 2) lookup the parser > > 3) parser->cells_parse(ctx, table) > > 3.a) update existing cells matched by name from table > > 4) parser->cells_clean(ctx, table) > /* to free cell's_info allocated by the parser driver */ > > as alternative way, I think it would be more generic to > provide nvmem-provider.h API to update the existing cell info, > however it makes sense only when cells were parsed > by the cell parser, in the other situations the > cell should be well defined. > > with this approach the parser driver will be just called > via parser->cells_parse(ctx) and will call nvmem_cell_info_update() > for each parsed cells. TBH, This is an over kill. In nvmem provider driver probe you can parse the tlv data and update the dt nodes before nvmem register. rest of the code should just work as it is. --srini > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 11:32 ` Srinivas Kandagatla @ 2021-09-20 12:29 ` Vadym Kochan 2021-09-20 12:34 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-20 12:29 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > On 20/09/2021 12:25, Vadym Kochan wrote: >>>> Or, treat cells with length "0" in a special way and allow to update >>>> cell info later.you can update irrespective of the length, as long as this is done >>> before register. >>> >>> >>>>> }; >>>>> >>>>> 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. >>>> >>> Lets keep the core code clean for now, I would expect the provider >>> driver along with parser function to do update the nodes. >>> >>> --srini >>> >> core.c sequence: >> >> 1) after cells parsed from OF: >> >> 2) lookup the parser >> >> 3) parser->cells_parse(ctx, table) >> >> 3.a) update existing cells matched by name from table >> >> 4) parser->cells_clean(ctx, table) >> /* to free cell's_info allocated by the parser driver */ >> >> as alternative way, I think it would be more generic to >> provide nvmem-provider.h API to update the existing cell info, >> however it makes sense only when cells were parsed >> by the cell parser, in the other situations the >> cell should be well defined. >> >> with this approach the parser driver will be just called >> via parser->cells_parse(ctx) and will call nvmem_cell_info_update() >> for each parsed cells. > > TBH, This is an over kill. > > In nvmem provider driver probe you can parse the tlv data and update the > dt nodes before nvmem register. > > rest of the code should just work as it is. > > --srini You mean to parse TLV in the particular nvmem provider driver (for example in at24 driver) ? If so, then it will not allow to parse this TLV format from the other kinds of nvmem. > > >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 12:29 ` Vadym Kochan @ 2021-09-20 12:34 ` Srinivas Kandagatla 2021-09-20 13:29 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-20 12:34 UTC (permalink / raw) To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 20/09/2021 13:29, Vadym Kochan wrote: > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > >> On 20/09/2021 12:25, Vadym Kochan wrote: >>>>> Or, treat cells with length "0" in a special way and allow to update >>>>> cell info later.you can update irrespective of the length, as long as this is done >>>> before register. >>>> >>>> >>>>>> }; >>>>>> >>>>>> 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. >>>>> >>>> Lets keep the core code clean for now, I would expect the provider >>>> driver along with parser function to do update the nodes. >>>> >>>> --srini >>>> >>> core.c sequence: >>> >>> 1) after cells parsed from OF: >>> >>> 2) lookup the parser >>> >>> 3) parser->cells_parse(ctx, table) >>> >>> 3.a) update existing cells matched by name from table >>> >>> 4) parser->cells_clean(ctx, table) >>> /* to free cell's_info allocated by the parser driver */ >>> >>> as alternative way, I think it would be more generic to >>> provide nvmem-provider.h API to update the existing cell info, >>> however it makes sense only when cells were parsed >>> by the cell parser, in the other situations the >>> cell should be well defined. >>> >>> with this approach the parser driver will be just called >>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update() >>> for each parsed cells. >> >> TBH, This is an over kill. >> >> In nvmem provider driver probe you can parse the tlv data and update the >> dt nodes before nvmem register. >> >> rest of the code should just work as it is. >> >> --srini > > You mean to parse TLV in the particular nvmem provider driver (for > example in at24 driver) ? If so, then it will not allow to parse > this TLV format from the other kinds of nvmem. Why not? Can you also tell us which other nvmem providers are you going to test this on? As long as you represent this parsing function as some library function, I do not see any issue. If any exported symbol is available for this then any nvmem provider could use it. --srini > >> >> >>> > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 12:34 ` Srinivas Kandagatla @ 2021-09-20 13:29 ` Vadym Kochan 2021-09-20 13:40 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-20 13:29 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > On 20/09/2021 13:29, Vadym Kochan wrote: >> >> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: >> >>> On 20/09/2021 12:25, Vadym Kochan wrote: >>>>>> Or, treat cells with length "0" in a special way and allow to update >>>>>> cell info later.you can update irrespective of the length, as long as this is done >>>>> before register. >>>>> >>>>> >>>>>>> }; >>>>>>> >>>>>>> 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. >>>>>> >>>>> Lets keep the core code clean for now, I would expect the provider >>>>> driver along with parser function to do update the nodes. >>>>> >>>>> --srini >>>>> >>>> core.c sequence: >>>> >>>> 1) after cells parsed from OF: >>>> >>>> 2) lookup the parser >>>> >>>> 3) parser->cells_parse(ctx, table) >>>> >>>> 3.a) update existing cells matched by name from table >>>> >>>> 4) parser->cells_clean(ctx, table) >>>> /* to free cell's_info allocated by the parser driver */ >>>> >>>> as alternative way, I think it would be more generic to >>>> provide nvmem-provider.h API to update the existing cell info, >>>> however it makes sense only when cells were parsed >>>> by the cell parser, in the other situations the >>>> cell should be well defined. >>>> >>>> with this approach the parser driver will be just called >>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update() >>>> for each parsed cells. >>> >>> TBH, This is an over kill. >>> >>> In nvmem provider driver probe you can parse the tlv data and update the >>> dt nodes before nvmem register. >>> >>> rest of the code should just work as it is. >>> >>> --srini >> >> You mean to parse TLV in the particular nvmem provider driver (for >> example in at24 driver) ? If so, then it will not allow to parse >> this TLV format from the other kinds of nvmem. > > Why not? > Well, I think that nvmem driver and TLV parsing should somehow be de-coupled from each other like block devices and FS does. BUT, in case this TLV data will be used only on at24 devices then may be it is OK. > Can you also tell us which other nvmem providers are you going to test > this on? > Currently I can test only on at24 devices. From the: https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html " Each ONIE system must include non-volatile storage which contains vital product data assigned by the manufacturer. The non-volatile storage could take the form of an EEPROM, a NOR-flash sector, a NAND-flash sector or any other non-volatile storage medium. " I am not aware about other nvmem devices which are used for existing ONIE supported boards. > As long as you represent this parsing function as some library function, > I do not see any issue. > If any exported symbol is available for this then any nvmem provider > could use it. > > --srini > > >> >>> >>> >>>> >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 13:29 ` Vadym Kochan @ 2021-09-20 13:40 ` Srinivas Kandagatla 2021-09-21 5:50 ` John Thomson 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-20 13:40 UTC (permalink / raw) To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 20/09/2021 14:29, Vadym Kochan wrote: > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > >> On 20/09/2021 13:29, Vadym Kochan wrote: >>> >>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: >>> >>>> On 20/09/2021 12:25, Vadym Kochan wrote: >>>>>>> Or, treat cells with length "0" in a special way and allow to update >>>>>>> cell info later.you can update irrespective of the length, as long as this is done >>>>>> before register. >>>>>> >>>>>> >>>>>>>> }; >>>>>>>> >>>>>>>> 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. >>>>>>> >>>>>> Lets keep the core code clean for now, I would expect the provider >>>>>> driver along with parser function to do update the nodes. >>>>>> >>>>>> --srini >>>>>> >>>>> core.c sequence: >>>>> >>>>> 1) after cells parsed from OF: >>>>> >>>>> 2) lookup the parser >>>>> >>>>> 3) parser->cells_parse(ctx, table) >>>>> >>>>> 3.a) update existing cells matched by name from table >>>>> >>>>> 4) parser->cells_clean(ctx, table) >>>>> /* to free cell's_info allocated by the parser driver */ >>>>> >>>>> as alternative way, I think it would be more generic to >>>>> provide nvmem-provider.h API to update the existing cell info, >>>>> however it makes sense only when cells were parsed >>>>> by the cell parser, in the other situations the >>>>> cell should be well defined. >>>>> >>>>> with this approach the parser driver will be just called >>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update() >>>>> for each parsed cells. >>>> >>>> TBH, This is an over kill. >>>> >>>> In nvmem provider driver probe you can parse the tlv data and update the >>>> dt nodes before nvmem register. >>>> >>>> rest of the code should just work as it is. >>>> >>>> --srini >>> >>> You mean to parse TLV in the particular nvmem provider driver (for >>> example in at24 driver) ? If so, then it will not allow to parse >>> this TLV format from the other kinds of nvmem. >> >> Why not? >> > > Well, I think that nvmem driver and TLV parsing should somehow be > de-coupled from each other like block devices and FS does. BUT, > in case this TLV data will be used only on at24 devices then > may be it is OK. > It has to be decoupled yes, which could be at any level with simple library function to a infrastructure level.. In this case with few or single users, doing with an additional infrastructure is a bit of over kill IMO. --srini >> Can you also tell us which other nvmem providers are you going to test >> this on? >> > > Currently I can test only on at24 devices. From the: > > https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html > > " > Each ONIE system must include non-volatile storage which contains vital > product data assigned by the manufacturer. The non-volatile storage > could take the form of an EEPROM, a NOR-flash sector, a NAND-flash > sector or any other non-volatile storage medium. > " > > I am not aware about other nvmem devices which are used for existing > ONIE supported boards. > >> As long as you represent this parsing function as some library function, >> I do not see any issue. >> If any exported symbol is available for this then any nvmem provider >> could use it. >> >> --srini >> >> >>> >>>> >>>> >>>>> >>> > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-20 13:40 ` Srinivas Kandagatla @ 2021-09-21 5:50 ` John Thomson 2021-09-27 7:50 ` Vadym Kochan 2021-09-27 10:12 ` Srinivas Kandagatla 0 siblings, 2 replies; 36+ messages in thread From: John Thomson @ 2021-09-21 5:50 UTC (permalink / raw) To: Srinivas Kandagatla, Vadym Kochan Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On Mon, 20 Sep 2021, at 13:40, Srinivas Kandagatla wrote: > On 20/09/2021 14:29, Vadym Kochan wrote: >> >> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: >> >>> On 20/09/2021 13:29, Vadym Kochan wrote: >>>> >>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: >>>> >>>>> On 20/09/2021 12:25, Vadym Kochan wrote: >>>>>>>> Or, treat cells with length "0" in a special way and allow to update >>>>>>>> cell info later.you can update irrespective of the length, as long as this is done >>>>>>> before register. >>>>>>> >>>>>>> >>>>>>>>> }; >>>>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>> Lets keep the core code clean for now, I would expect the provider >>>>>>> driver along with parser function to do update the nodes. >>>>>>> >>>>>>> --srini >>>>>>> >>>>>> core.c sequence: >>>>>> >>>>>> 1) after cells parsed from OF: >>>>>> >>>>>> 2) lookup the parser >>>>>> >>>>>> 3) parser->cells_parse(ctx, table) >>>>>> >>>>>> 3.a) update existing cells matched by name from table >>>>>> >>>>>> 4) parser->cells_clean(ctx, table) >>>>>> /* to free cell's_info allocated by the parser driver */ >>>>>> >>>>>> as alternative way, I think it would be more generic to >>>>>> provide nvmem-provider.h API to update the existing cell info, >>>>>> however it makes sense only when cells were parsed >>>>>> by the cell parser, in the other situations the >>>>>> cell should be well defined. >>>>>> >>>>>> with this approach the parser driver will be just called >>>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update() >>>>>> for each parsed cells. >>>>> >>>>> TBH, This is an over kill. >>>>> >>>>> In nvmem provider driver probe you can parse the tlv data and update the >>>>> dt nodes before nvmem register. >>>>> >>>>> rest of the code should just work as it is. >>>>> >>>>> --srini >>>> >>>> You mean to parse TLV in the particular nvmem provider driver (for >>>> example in at24 driver) ? If so, then it will not allow to parse >>>> this TLV format from the other kinds of nvmem. >>> >>> Why not? >>> >> >> Well, I think that nvmem driver and TLV parsing should somehow be >> de-coupled from each other like block devices and FS does. BUT, >> in case this TLV data will be used only on at24 devices then >> may be it is OK. >> > > It has to be decoupled yes, which could be at any level with simple > library function to a infrastructure level.. > > In this case with few or single users, doing with an additional > infrastructure is a bit of over kill IMO. > > > --srini >>> Can you also tell us which other nvmem providers are you going to test >>> this on? >>> >> >> Currently I can test only on at24 devices. From the: >> >> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html >> >> " >> Each ONIE system must include non-volatile storage which contains vital >> product data assigned by the manufacturer. The non-volatile storage >> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash >> sector or any other non-volatile storage medium. >> " >> >> I am not aware about other nvmem devices which are used for existing >> ONIE supported boards. >> >>> As long as you represent this parsing function as some library function, >>> I do not see any issue. >>> If any exported symbol is available for this then any nvmem provider >>> could use it. >>> >>> --srini >>> Hi Srinivas, Can I note here that I would like to parse TLV data from an SPI-NOR device to NVMEM cells. The same general use case (getting mac-address from OEM data). Was planning to base my work on this series, as well as https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ (thanks for pointing that out Srinivas) Cheers, -- John Thomson ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-21 5:50 ` John Thomson @ 2021-09-27 7:50 ` Vadym Kochan 2021-09-27 10:12 ` Srinivas Kandagatla 2021-09-27 10:12 ` Srinivas Kandagatla 1 sibling, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-27 7:50 UTC (permalink / raw) To: John Thomson Cc: Srinivas Kandagatla, Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko Hi John, Srini, John Thomson <john@johnthomson.fastmail.com.au> writes: > On Mon, 20 Sep 2021, at 13:40, Srinivas Kandagatla wrote: >> On 20/09/2021 14:29, Vadym Kochan wrote: >>> >>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: >>> >>>> On 20/09/2021 13:29, Vadym Kochan wrote: >>>>> >>>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: >>>>> >>>>>> On 20/09/2021 12:25, Vadym Kochan wrote: >>>>>>>>> Or, treat cells with length "0" in a special way and allow to update >>>>>>>>> cell info later.you can update irrespective of the length, as long as this is done >>>>>>>> before register. >>>>>>>> >>>>>>>> >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>> >>>>>>>> Lets keep the core code clean for now, I would expect the provider >>>>>>>> driver along with parser function to do update the nodes. >>>>>>>> >>>>>>>> --srini >>>>>>>> >>>>>>> core.c sequence: >>>>>>> >>>>>>> 1) after cells parsed from OF: >>>>>>> >>>>>>> 2) lookup the parser >>>>>>> >>>>>>> 3) parser->cells_parse(ctx, table) >>>>>>> >>>>>>> 3.a) update existing cells matched by name from table >>>>>>> >>>>>>> 4) parser->cells_clean(ctx, table) >>>>>>> /* to free cell's_info allocated by the parser driver */ >>>>>>> >>>>>>> as alternative way, I think it would be more generic to >>>>>>> provide nvmem-provider.h API to update the existing cell info, >>>>>>> however it makes sense only when cells were parsed >>>>>>> by the cell parser, in the other situations the >>>>>>> cell should be well defined. >>>>>>> >>>>>>> with this approach the parser driver will be just called >>>>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update() >>>>>>> for each parsed cells. >>>>>> >>>>>> TBH, This is an over kill. >>>>>> >>>>>> In nvmem provider driver probe you can parse the tlv data and update the >>>>>> dt nodes before nvmem register. >>>>>> >>>>>> rest of the code should just work as it is. >>>>>> >>>>>> --srini >>>>> >>>>> You mean to parse TLV in the particular nvmem provider driver (for >>>>> example in at24 driver) ? If so, then it will not allow to parse >>>>> this TLV format from the other kinds of nvmem. >>>> >>>> Why not? >>>> >>> >>> Well, I think that nvmem driver and TLV parsing should somehow be >>> de-coupled from each other like block devices and FS does. BUT, >>> in case this TLV data will be used only on at24 devices then >>> may be it is OK. >>> >> >> It has to be decoupled yes, which could be at any level with simple >> library function to a infrastructure level.. >> >> In this case with few or single users, doing with an additional >> infrastructure is a bit of over kill IMO. >> >> >> --srini >>>> Can you also tell us which other nvmem providers are you going to test >>>> this on? >>>> >>> >>> Currently I can test only on at24 devices. From the: >>> >>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html >>> >>> " >>> Each ONIE system must include non-volatile storage which contains vital >>> product data assigned by the manufacturer. The non-volatile storage >>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash >>> sector or any other non-volatile storage medium. >>> " >>> >>> I am not aware about other nvmem devices which are used for existing >>> ONIE supported boards. >>> >>>> As long as you represent this parsing function as some library function, >>>> I do not see any issue. >>>> If any exported symbol is available for this then any nvmem provider >>>> could use it. >>>> >>>> --srini >>>> > > Hi Srinivas, > > Can I note here that I would like to parse > TLV data from an SPI-NOR device to NVMEM cells. > The same general use case (getting mac-address from OEM data). > > Was planning to base my work on this series, as well as > https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ > (thanks for pointing that out Srinivas) > > Cheers, What about at least to have just one call in core.c to make it a bit de-coupled, like: core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) { ... rval = nvmem_add_cells_from_table(nvmem); if (rval) goto err_remove_cells; + rval = nvmem_parse_cells(nvmem, of); + if (rval) { + /* err handling */ + } + rval = nvmem_add_cells_from_of(nvmem); if (rval) goto err_remove_cells; blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem); return nvmem; ... } somewhere in nvmem-parser.c: /* retreive parser name from of_node and call appropriate function to parse non-fixed cells and update via of_update */ int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node) { ... } ? Regards, Vadym Kochan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-27 7:50 ` Vadym Kochan @ 2021-09-27 10:12 ` Srinivas Kandagatla 2021-09-28 13:31 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-27 10:12 UTC (permalink / raw) To: Vadym Kochan, John Thomson Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 27/09/2021 08:50, Vadym Kochan wrote: >>>> Currently I can test only on at24 devices. From the: >>>> >>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html >>>> >>>> " >>>> Each ONIE system must include non-volatile storage which contains vital >>>> product data assigned by the manufacturer. The non-volatile storage >>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash >>>> sector or any other non-volatile storage medium. >>>> " >>>> >>>> I am not aware about other nvmem devices which are used for existing >>>> ONIE supported boards. >>>> >>>>> As long as you represent this parsing function as some library function, >>>>> I do not see any issue. >>>>> If any exported symbol is available for this then any nvmem provider >>>>> could use it. >>>>> >>>>> --srini >>>>> >> Hi Srinivas, >> >> Can I note here that I would like to parse >> TLV data from an SPI-NOR device to NVMEM cells. >> The same general use case (getting mac-address from OEM data). >> >> Was planning to base my work on this series, as well as >> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ >> (thanks for pointing that out Srinivas) >> >> Cheers, > What about at least to have just one call in core.c to make it a bit > de-coupled, like: Why do you want to decouple this? the provider driver should be very well aware of the format the data layout. Its fine to an extent to adding parse_cells() callback in nvmem_config. > > core.c > > struct nvmem_device *nvmem_register(const struct nvmem_config *config) > { > ... > rval = nvmem_add_cells_from_table(nvmem); > if (rval) > goto err_remove_cells; > > + rval = nvmem_parse_cells(nvmem, of); > + if (rval) { > + /* err handling */ > + } > + > rval = nvmem_add_cells_from_of(nvmem); > if (rval) > goto err_remove_cells; > > blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem); > > return nvmem; > > ... > > } > > somewhere in nvmem-parser.c: However this is totally over kill. > > /* retreive parser name from of_node and call appropriate function to parse > non-fixed cells and update via of_update */ This is completely provider drivers job, nothing nvmem core should worry about. If you have concern of having code duplicated then we could make some of the common functions as library functions, But it still is within the scope of provider drivers. --srini > int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node) > { > ... > } > > ? > > Regards, ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-27 10:12 ` Srinivas Kandagatla @ 2021-09-28 13:31 ` Vadym Kochan 2021-09-28 13:51 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-28 13:31 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, John Thomson, Rob Herring, linux-kernel, devicetree, Robert Marko Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > On 27/09/2021 08:50, Vadym Kochan wrote: >>>>> Currently I can test only on at24 devices. From the: >>>>> >>>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html >>>>> >>>>> " >>>>> Each ONIE system must include non-volatile storage which contains vital >>>>> product data assigned by the manufacturer. The non-volatile storage >>>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash >>>>> sector or any other non-volatile storage medium. >>>>> " >>>>> >>>>> I am not aware about other nvmem devices which are used for existing >>>>> ONIE supported boards. >>>>> >>>>>> As long as you represent this parsing function as some library function, >>>>>> I do not see any issue. >>>>>> If any exported symbol is available for this then any nvmem provider >>>>>> could use it. >>>>>> >>>>>> --srini >>>>>> >>> Hi Srinivas, >>> >>> Can I note here that I would like to parse >>> TLV data from an SPI-NOR device to NVMEM cells. >>> The same general use case (getting mac-address from OEM data). >>> >>> Was planning to base my work on this series, as well as >>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ >>> (thanks for pointing that out Srinivas) >>> >>> Cheers, >> What about at least to have just one call in core.c to make it a bit >> de-coupled, like: > > Why do you want to decouple this? the provider driver should be very > well aware of the format the data layout. > In my understanding nvmem device should not aware about the data layout (in case it does not rely on device's specific characteristics). Same cells layout (TLV, etc) might exist on other nvmem devices. > Its fine to an extent to adding parse_cells() callback in nvmem_config. > OK, in that case it will require small change in the core. >> >> core.c >> >> struct nvmem_device *nvmem_register(const struct nvmem_config *config) >> { >> ... >> rval = nvmem_add_cells_from_table(nvmem); >> if (rval) >> goto err_remove_cells; >> >> + rval = nvmem_parse_cells(nvmem, of); >> + if (rval) { >> + /* err handling */ >> + } >> + >> rval = nvmem_add_cells_from_of(nvmem); >> if (rval) >> goto err_remove_cells; >> >> blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem); >> >> return nvmem; >> >> ... >> >> } >> > >> somewhere in nvmem-parser.c: > > However this is totally over kill. > >> >> /* retreive parser name from of_node and call appropriate function to parse >> non-fixed cells and update via of_update */ > > This is completely provider drivers job, nothing nvmem core should worry > about. > > If you have concern of having code duplicated then we could make some of > the common functions as library functions, But it still is within the > scope of provider drivers. > Do I understand correctly that this parser function should be exported from at24.c (in case of ONIE) and not from a separate C module ? Or it just means that if there will be more users of this parsing function then it might be moved to separate C module ? > --srini > BTW, what if such change will be declined by particular nvmem driver maintainer ? >> int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node) >> { >> ... >> } >> >> ? > > >> >> Regards, ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-28 13:31 ` Vadym Kochan @ 2021-09-28 13:51 ` Srinivas Kandagatla 2021-09-28 14:11 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-28 13:51 UTC (permalink / raw) To: Vadym Kochan Cc: John Thomson, Rob Herring, linux-kernel, devicetree, Robert Marko On 28/09/2021 14:31, Vadym Kochan wrote: >>>> Can I note here that I would like to parse >>>> TLV data from an SPI-NOR device to NVMEM cells. >>>> The same general use case (getting mac-address from OEM data). >>>> >>>> Was planning to base my work on this series, as well as >>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ >>>> (thanks for pointing that out Srinivas) >>>> >>>> Cheers, >>> What about at least to have just one call in core.c to make it a bit >>> de-coupled, like: >> Why do you want to decouple this? the provider driver should be very >> well aware of the format the data layout. >> > In my understanding nvmem device should not aware about the data layout > (in case it does not rely on device's specific characteristics). Same > cells layout (TLV, etc) might exist on other nvmem devices. > How would provider driver parse this without even knowing data layout? >> Its fine to an extent to adding parse_cells() callback in nvmem_config. >> > OK, in that case it will require small change in the core. > >>> core.c >>> >>> struct nvmem_device *nvmem_register(const struct nvmem_config *config) >>> { >>> ... >>> rval = nvmem_add_cells_from_table(nvmem); >>> if (rval) >>> goto err_remove_cells; >>> >>> + rval = nvmem_parse_cells(nvmem, of); >>> + if (rval) { >>> + /* err handling */ >>> + } >>> + >>> rval = nvmem_add_cells_from_of(nvmem); >>> if (rval) >>> goto err_remove_cells; >>> >>> blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem); >>> >>> return nvmem; >>> >>> ... >>> >>> } >>> >>> somewhere in nvmem-parser.c: >> However this is totally over kill. >> >>> /* retreive parser name from of_node and call appropriate function to parse >>> non-fixed cells and update via of_update */ >> This is completely provider drivers job, nothing nvmem core should worry >> about. >> >> If you have concern of having code duplicated then we could make some of >> the common functions as library functions, But it still is within the >> scope of provider drivers. >> > Do I understand correctly that this parser function should be exported > from at24.c (in case of ONIE) and not from a separate C module ? Or > it just means that if there will be more users of this parsing function > then it might be moved to separate C module ? yes. For now am not really sure how many users are for such parsing function. > >> --srini >> > BTW, what if such change will be declined by particular nvmem driver > maintainer ? You would need some changes to provider driver to be able to flag that there is some kind of parsing required anyway. --srini > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-28 13:51 ` Srinivas Kandagatla @ 2021-09-28 14:11 ` Vadym Kochan 2021-09-28 14:39 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-28 14:11 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, John Thomson, Rob Herring, linux-kernel, devicetree, Robert Marko Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > On 28/09/2021 14:31, Vadym Kochan wrote: >>>>> Can I note here that I would like to parse >>>>> TLV data from an SPI-NOR device to NVMEM cells. >>>>> The same general use case (getting mac-address from OEM data). >>>>> >>>>> Was planning to base my work on this series, as well as >>>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ >>>>> (thanks for pointing that out Srinivas) >>>>> >>>>> Cheers, >>>> What about at least to have just one call in core.c to make it a bit >>>> de-coupled, like: >>> Why do you want to decouple this? the provider driver should be very >>> well aware of the format the data layout. >>> >> In my understanding nvmem device should not aware about the data layout >> (in case it does not rely on device's specific characteristics). Same >> cells layout (TLV, etc) might exist on other nvmem devices. >> > How would provider driver parse this without even knowing data layout? > > >>> Its fine to an extent to adding parse_cells() callback in nvmem_config. >>> >> OK, in that case it will require small change in the core. >> >>>> core.c >>>> >>>> struct nvmem_device *nvmem_register(const struct nvmem_config *config) >>>> { >>>> ... >>>> rval = nvmem_add_cells_from_table(nvmem); >>>> if (rval) >>>> goto err_remove_cells; >>>> >>>> + rval = nvmem_parse_cells(nvmem, of); >>>> + if (rval) { >>>> + /* err handling */ >>>> + } >>>> + >>>> rval = nvmem_add_cells_from_of(nvmem); >>>> if (rval) >>>> goto err_remove_cells; >>>> >>>> blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem); >>>> >>>> return nvmem; >>>> >>>> ... >>>> >>>> } >>>> >>>> somewhere in nvmem-parser.c: >>> However this is totally over kill. >>> >>>> /* retreive parser name from of_node and call appropriate function to parse >>>> non-fixed cells and update via of_update */ >>> This is completely provider drivers job, nothing nvmem core should worry >>> about. >>> >>> If you have concern of having code duplicated then we could make some of >>> the common functions as library functions, But it still is within the >>> scope of provider drivers. >>> >> Do I understand correctly that this parser function should be exported >> from at24.c (in case of ONIE) and not from a separate C module ? Or >> it just means that if there will be more users of this parsing function >> then it might be moved to separate C module ? > yes. > For now am not really sure how many users are for such parsing function. > >> >>> --srini >>> >> BTW, what if such change will be declined by particular nvmem driver >> maintainer ? > > You would need some changes to provider driver to be able to flag that > there is some kind of parsing required anyway. > It might be some new property in device-tree which can be used also for the other providers. > --srini >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-28 14:11 ` Vadym Kochan @ 2021-09-28 14:39 ` Srinivas Kandagatla 0 siblings, 0 replies; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-28 14:39 UTC (permalink / raw) To: Vadym Kochan Cc: John Thomson, Rob Herring, linux-kernel, devicetree, Robert Marko On 28/09/2021 15:11, Vadym Kochan wrote: > Srinivas Kandagatla<srinivas.kandagatla@linaro.org> writes: > >> On 28/09/2021 14:31, Vadym Kochan wrote: >>>>>> Can I note here that I would like to parse >>>>>> TLV data from an SPI-NOR device to NVMEM cells. >>>>>> The same general use case (getting mac-address from OEM data). >>>>>> >>>>>> Was planning to base my work on this series, as well as >>>>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ >>>>>> (thanks for pointing that out Srinivas) >>>>>> >>>>>> Cheers, >>>>> What about at least to have just one call in core.c to make it a bit >>>>> de-coupled, like: >>>> Why do you want to decouple this? the provider driver should be very >>>> well aware of the format the data layout. >>>> >>> In my understanding nvmem device should not aware about the data layout >>> (in case it does not rely on device's specific characteristics). Same >>> cells layout (TLV, etc) might exist on other nvmem devices. >>> >> How would provider driver parse this without even knowing data layout? >> >> >>>> Its fine to an extent to adding parse_cells() callback in nvmem_config. >>>> >>> OK, in that case it will require small change in the core. >>> >>>>> core.c >>>>> >>>>> struct nvmem_device *nvmem_register(const struct nvmem_config *config) >>>>> { >>>>> ... >>>>> rval = nvmem_add_cells_from_table(nvmem); >>>>> if (rval) >>>>> goto err_remove_cells; >>>>> >>>>> + rval = nvmem_parse_cells(nvmem, of); >>>>> + if (rval) { >>>>> + /* err handling */ >>>>> + } >>>>> + >>>>> rval = nvmem_add_cells_from_of(nvmem); >>>>> if (rval) >>>>> goto err_remove_cells; >>>>> >>>>> blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem); >>>>> >>>>> return nvmem; >>>>> >>>>> ... >>>>> >>>>> } >>>>> >>>>> somewhere in nvmem-parser.c: >>>> However this is totally over kill. >>>> >>>>> /* retreive parser name from of_node and call appropriate function to parse >>>>> non-fixed cells and update via of_update */ >>>> This is completely provider drivers job, nothing nvmem core should worry >>>> about. >>>> >>>> If you have concern of having code duplicated then we could make some of >>>> the common functions as library functions, But it still is within the >>>> scope of provider drivers. >>>> >>> Do I understand correctly that this parser function should be exported >>> from at24.c (in case of ONIE) and not from a separate C module ? Or >>> it just means that if there will be more users of this parsing function >>> then it might be moved to separate C module ? >> yes. >> For now am not really sure how many users are for such parsing function. >> >>>> --srini >>>> >>> BTW, what if such change will be declined by particular nvmem driver >>> maintainer ? >> You would need some changes to provider driver to be able to flag that >> there is some kind of parsing required anyway. >> > It might be some new property in device-tree which can be used also > for the other providers. But this new binding will still belong to the provider driver that you want to support parsing. --srini > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-21 5:50 ` John Thomson 2021-09-27 7:50 ` Vadym Kochan @ 2021-09-27 10:12 ` Srinivas Kandagatla 2021-09-27 12:38 ` John Thomson 1 sibling, 1 reply; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-27 10:12 UTC (permalink / raw) To: John Thomson, Vadym Kochan Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 21/09/2021 06:50, John Thomson wrote: >>> Currently I can test only on at24 devices. From the: >>> >>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html >>> >>> " >>> Each ONIE system must include non-volatile storage which contains vital >>> product data assigned by the manufacturer. The non-volatile storage >>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash >>> sector or any other non-volatile storage medium. >>> " >>> >>> I am not aware about other nvmem devices which are used for existing >>> ONIE supported boards. >>> >>>> As long as you represent this parsing function as some library function, >>>> I do not see any issue. >>>> If any exported symbol is available for this then any nvmem provider >>>> could use it. >>>> >>>> --srini >>>> > Hi Srinivas, > > Can I note here that I would like to parse > TLV data from an SPI-NOR device to NVMEM cells. > The same general use case (getting mac-address from OEM data). > > Was planning to base my work on this series, as well as > https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ This series is for post-processing nvmem cell data before it gets to consumers. Are you referring to parsing nvmem cell information (offset, name) in your usecase like: https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html Or Are you referring to post-processing nvmem cell data ? --srini > (thanks for pointing that out Srinivas) > > Cheers, > -- John Thomson ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-09-27 10:12 ` Srinivas Kandagatla @ 2021-09-27 12:38 ` John Thomson 0 siblings, 0 replies; 36+ messages in thread From: John Thomson @ 2021-09-27 12:38 UTC (permalink / raw) To: Srinivas Kandagatla, Vadym Kochan Cc: Rob Herring, linux-kernel, devicetree, Robert Marko Hi Srinivas, On Mon, 27 Sep 2021, at 10:12, Srinivas Kandagatla wrote: > On 21/09/2021 06:50, John Thomson wrote: >> Hi Srinivas, >> >> Can I note here that I would like to parse >> TLV data from an SPI-NOR device to NVMEM cells. >> The same general use case (getting mac-address from OEM data). >> >> Was planning to base my work on this series, as well as >> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/ > > This series is for post-processing nvmem cell data before it gets to > consumers. > Are you referring to parsing nvmem cell information (offset, name) in > your usecase like: > https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html > > Or > Are you referring to post-processing nvmem cell data ? Both. I have TLV data where I want to parse the tag lengths and tag IDs to map them into offsets and names like a NVMEM cells lookups table. Then, some of these cell data would need to be post processed to use directly. I have only a base MAC address available, and would like to specify increments for different network ports. As an additional example, another TLV tag:value has compressed wifi calibration data. If I could post process this cell data I could then feed it into ath9k with: https://lore.kernel.org/all/f9b732b50a3453fadf3923cc75d365bae3505fe7.1630157099.git.chunkeey@gmail.com/ In my case, this is all currently done in userspace. I saw this (ONIE TLV NVMEM parser) series and thought it offered something very similar to (the first part of) what I would like to do. Cheers, -- John Thomson ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser 2021-06-14 10:44 ` Srinivas Kandagatla 2021-06-16 12:33 ` Vadym Kochan @ 2021-09-08 9:44 ` Vadym Kochan 1 sibling, 0 replies; 36+ messages in thread From: Vadym Kochan @ 2021-09-08 9:44 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes: > On 08/06/2021 20:03, Vadym Kochan wrote: >> Current implementation does not allow to register cells for already >> registered nvmem device and requires that this should be done before. But >> there might a driver which needs to parse the nvmem device and after >> register a cells table. >> >> Introduce nvmem_parser API which allows to register cells parser which >> is called during nvmem device registration. During this stage the parser >> can read the nvmem device and register the cells table. >> >> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> >> --- >> v2: >> 1) Added nvmem_parser_data so parser implementation >> should fill it with cells table and lookups which >> will be registered by core.c after cells_parse was >> succeed. >> >> 2) Removed cells_remove callback from nvmem_parser_config which >> is not needed because cells table & lookups are >> registered/unregistered automatically by core. >> >> 3) Use new device property to read cells parser name during nvmem >> registration. Removed of_node usage. >> >> 4) parser's module refcount is incremented/decremented on each parser >> bind/unbind to nvmem device. >> >> drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++ >> include/linux/nvmem-provider.h | 31 ++++++ >> 2 files changed, 209 insertions(+) >> >> 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. > >> void *priv; >> }; >> >> @@ -57,6 +58,13 @@ struct nvmem_cell { >> struct list_head node; >> }; >> >> +struct nvmem_parser { >> + struct list_head head; >> + nvmem_parse_t cells_parse; >> + const char *name; >> + struct module *owner; >> +}; >> + >> static DEFINE_MUTEX(nvmem_mutex); >> static DEFINE_IDA(nvmem_ida); >> >> @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables); >> static DEFINE_MUTEX(nvmem_lookup_mutex); >> static LIST_HEAD(nvmem_lookup_list); >> >> +static DEFINE_MUTEX(nvmem_parser_mutex); >> +static LIST_HEAD(nvmem_parser_list); >> + >> static BLOCKING_NOTIFIER_HEAD(nvmem_notifier); >> >> static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, >> @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = { >> .name = "nvmem", >> }; >> >> +static struct nvmem_parser *__nvmem_parser_get(const char *name) >> +{ >> + struct nvmem_parser *tmp, *parser = NULL; >> + >> + list_for_each_entry(tmp, &nvmem_parser_list, head) { >> + if (strcmp(name, tmp->name) == 0) { >> + parser = tmp; >> + break; >> + } >> + } >> + >> + if (!parser) >> + return ERR_PTR(-EPROBE_DEFER); >> + >> + if (!try_module_get(parser->owner)) { >> + pr_err("could not increase module refcount for parser %s\n", >> + parser->name); >> + return ERR_PTR(-EINVAL); >> + >> + } >> + >> + return parser; >> +} >> + >> +static void nvmem_parser_put(const struct nvmem_parser *parser) >> +{ >> + module_put(parser->owner); >> +} >> + >> +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name) >> +{ > Do we really need parser bind/unbind mechanisms for what we are trying > to do here. > > It's just simply parsing cell info during nvmem register, do we really > care if parser is there or not after that? > > code can be probably made much simpler by just doing this in nvmem_register > > parser_get() > parse_get_cell_info() > parser_put() > > AFAIU, that is all we need. > bind/unbind is just for connecting parser instance with a nvmem device, but the real reason is because of need of these cell lookups info, and then freeing them. If it would be possible to lookup parser's cell without these lookups (I tried to explain the issue with this in other reply in this series) then yes, just adding cell info would be enough. >> + struct nvmem_parser_data *data; >> + struct nvmem_parser *parser; >> + int err; >> + >> + mutex_lock(&nvmem_parser_mutex); >> + >> + parser = __nvmem_parser_get(name); >> + err = PTR_ERR_OR_ZERO(parser); >> + if (!err) { > + data = kzalloc(sizeof(*data), GFP_KERNEL); >> + if (data) { >> + data->parser = parser; >> + nvmem->parser_data = data; >> + } else { >> + nvmem_parser_put(parser); >> + err = -ENOMEM; >> + } >> + } >> + >> + mutex_unlock(&nvmem_parser_mutex); >> + >> + return err; >> +} >> + >> +static void nvmem_parser_unbind(const struct nvmem_device *nvmem) >> +{ >> + struct nvmem_parser_data *data = nvmem->parser_data; >> + >> + if (data->table.cells) { >> + nvmem_del_cell_table(&data->table); >> + kfree(data->table.cells); > who has allocated memory for this, its confusing for this to be freed in > core. >> + } >> + if (data->lookup) { > + nvmem_del_cell_lookups(data->lookup, data->nlookups); >> + kfree(data->lookup); >> + } >> + >> + nvmem_parser_put(data->parser); >> +} >> + >> +static void nvmem_parser_data_register(struct nvmem_device *nvmem, >> + struct nvmem_parser_data *data) >> +{ >> + if (data->table.cells) { >> + if (!data->table.nvmem_name) >> + data->table.nvmem_name = nvmem_dev_name(nvmem); >> + >> + nvmem_add_cell_table(&data->table); >> + } >> + >> + if (data->lookup) { > Why do we need lookups? > the cells are already associated with provider. lookups are normally > used for associating devices with nvmemcell more for old non-dt machine > code. > > you can remove this. >> + struct nvmem_cell_lookup *lookup = &data->lookup[0]; >> + int i = 0; >> + >> + for (i = 0; i < data->nlookups; i++, lookup++) { >> + if (!lookup->nvmem_name) >> + lookup->nvmem_name = nvmem_dev_name(nvmem); >> + >> + if (!lookup->dev_id) >> + lookup->dev_id = data->parser->name; >> + } >> + >> + nvmem_add_cell_lookups(data->lookup, data->nlookups); >> + } >> +} >> + > > --srini ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property 2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan 2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan @ 2021-06-08 19:03 ` Vadym Kochan 2021-06-18 20:59 ` Rob Herring 2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan 2 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree Cc: Robert Marko, Vadym Kochan Describe new "nvmem-cells-parser-name" which is used to bind registered parser with nvmem device. Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> --- Documentation/devicetree/bindings/nvmem/nvmem.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml index b8dc3d2b6e92..36c6361d0605 100644 --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml @@ -39,6 +39,11 @@ properties: when it's driven low (logical '0') to allow writing. maxItems: 1 + nvmem-cells-parser-name: + description: + Name of the registered parser to parse and register the cells. + maxItems: 1 + patternProperties: "^.*@[0-9a-f]+$": type: object -- 2.17.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property 2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan @ 2021-06-18 20:59 ` Rob Herring 0 siblings, 0 replies; 36+ messages in thread From: Rob Herring @ 2021-06-18 20:59 UTC (permalink / raw) To: Vadym Kochan; +Cc: Srinivas Kandagatla, linux-kernel, devicetree, Robert Marko On Tue, Jun 08, 2021 at 10:03:26PM +0300, Vadym Kochan wrote: > Describe new "nvmem-cells-parser-name" which is used to bind registered > parser with nvmem device. > > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> > --- > Documentation/devicetree/bindings/nvmem/nvmem.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml > index b8dc3d2b6e92..36c6361d0605 100644 > --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml > @@ -39,6 +39,11 @@ properties: > when it's driven low (logical '0') to allow writing. > maxItems: 1 > > + nvmem-cells-parser-name: > + description: > + Name of the registered parser to parse and register the cells. > + maxItems: 1 I think this should be a 'compatible' string. We already have something along that line with: Documentation/devicetree/bindings/mtd/partitions/nvmem-cells.yaml Rob ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser 2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan 2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan 2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan @ 2021-06-08 19:03 ` Vadym Kochan 2021-08-06 15:39 ` Jan Lübbe 2 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree Cc: Robert Marko, Vadym Kochan ONIE is a small operating system, pre-installed on bare metal network switches, that provides an environment for automated provisioning. This system requires that NVMEM (EEPROM) device holds various system information (mac address, platform name, etc) in a special TLV layout. The driver registers ONIE TLV attributes as NVMEM cells which can be accessed by other platform driver. Also it allows to use of_get_mac_address() to retrieve mac address for the netdev. Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> --- v2: 1) onie-cells.c renamed to onie-tlv-cells.c 2) Do not register onie cells parser via platform driver, but via module_init(). 3) Simplified cells table & lookups registration by just filling all this in nvmem_parser_data. drivers/nvmem/Kconfig | 9 + drivers/nvmem/Makefile | 2 + drivers/nvmem/onie-tlv-cells.c | 302 +++++++++++++++++++++++++++++++++ 3 files changed, 313 insertions(+) create mode 100644 drivers/nvmem/onie-tlv-cells.c diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig index dd2019006838..a08ff087361b 100644 --- a/drivers/nvmem/Kconfig +++ b/drivers/nvmem/Kconfig @@ -288,4 +288,13 @@ config NVMEM_BRCM_NVRAM This driver provides support for Broadcom's NVRAM that can be accessed using I/O mapping. +config NVMEM_ONIE_TLV_CELLS + tristate "ONIE TLV cells support" + help + This is a driver to provide cells from ONIE TLV structure stored + on NVME device. + + This driver can also be built as a module. If so, the module + will be called nvmem-onie-tlv-cells. + endif diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile index bbea1410240a..f70d7b817377 100644 --- a/drivers/nvmem/Makefile +++ b/drivers/nvmem/Makefile @@ -59,3 +59,5 @@ obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o nvmem-rmem-y := rmem.o obj-$(CONFIG_NVMEM_BRCM_NVRAM) += nvmem_brcm_nvram.o nvmem_brcm_nvram-y := brcm_nvram.o +obj-$(CONFIG_NVMEM_ONIE_TLV_CELLS) += nvmem-onie-tlv-cells.o +nvmem-onie-tlv-cells-y := onie-tlv-cells.o diff --git a/drivers/nvmem/onie-tlv-cells.c b/drivers/nvmem/onie-tlv-cells.c new file mode 100644 index 000000000000..85b1c92da0c5 --- /dev/null +++ b/drivers/nvmem/onie-tlv-cells.c @@ -0,0 +1,302 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * ONIE NVMEM cells provider + * + * Author: Vadym Kochan <vadym.kochan@plvision.eu> + */ + +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/kref.h> +#include <linux/module.h> +#include <linux/nvmem-consumer.h> +#include <linux/nvmem-provider.h> +#include <linux/slab.h> + +#define ONIE_NVMEM_TLV_MAX_LEN 2048 + +#define ONIE_NVMEM_HDR_ID "TlvInfo" + +struct onie_tlv_hdr { + u8 id[8]; + u8 version; + __be16 data_len; +} __packed; + +struct onie_tlv { + u8 type; + u8 len; + u8 val[0]; +} __packed; + +struct onie_nvmem_attr { + struct list_head head; + const char *name; + unsigned int offset; + unsigned int len; +}; + +struct onie_tlv_parser { + unsigned int attr_count; + struct list_head attrs; + struct device *dev; + + struct nvmem_cell_lookup *lookup; + int nlookups; +}; + +static struct nvmem_parser *nvmem_parser; + +static bool onie_nvmem_hdr_is_valid(struct onie_tlv_hdr *hdr) +{ + if (memcmp(hdr->id, ONIE_NVMEM_HDR_ID, sizeof(hdr->id)) != 0) + return false; + if (hdr->version != 0x1) + return false; + + return true; +} + +static void onie_nvmem_attrs_free(struct onie_tlv_parser *parser) +{ + struct onie_nvmem_attr *attr, *tmp; + + list_for_each_entry_safe(attr, tmp, &parser->attrs, head) { + list_del(&attr->head); + kfree(attr); + } +} + +static const char *onie_nvmem_attr_name(u8 type) +{ + switch (type) { + case 0x21: return "product-name"; + case 0x22: return "part-number"; + case 0x23: return "serial-number"; + case 0x24: return "mac-address"; + case 0x25: return "manufacture-date"; + case 0x26: return "device-version"; + case 0x27: return "label-revision"; + case 0x28: return "platforn-name"; + case 0x29: return "onie-version"; + case 0x2A: return "num-macs"; + case 0x2B: return "manufacturer"; + case 0x2C: return "country-code"; + case 0x2D: return "vendor"; + case 0x2E: return "diag-version"; + case 0x2F: return "service-tag"; + case 0xFD: return "vendor-extension"; + case 0xFE: return "crc32"; + + default: return "unknown"; + } +} + +static int onie_nvmem_tlv_parse(struct onie_tlv_parser *parser, u8 *data, u16 len) +{ + unsigned int hlen = sizeof(struct onie_tlv_hdr); + unsigned int offset = 0; + int err; + + parser->attr_count = 0; + + while (offset < len) { + struct onie_nvmem_attr *attr; + struct onie_tlv *tlv; + + tlv = (struct onie_tlv *)(data + offset); + + if (offset + tlv->len >= len) { + pr_err("TLV len is too big(0x%x) at 0x%x\n", + tlv->len, hlen + offset); + + /* return success in case something was parsed */ + return 0; + } + + attr = kmalloc(sizeof(*attr), GFP_KERNEL); + if (!attr) { + err = -ENOMEM; + goto err_attr_alloc; + } + + attr->name = onie_nvmem_attr_name(tlv->type); + /* skip 'type' and 'len' */ + attr->offset = hlen + offset + 2; + attr->len = tlv->len; + + list_add(&attr->head, &parser->attrs); + parser->attr_count++; + + offset += sizeof(*tlv) + tlv->len; + } + + if (!parser->attr_count) + return -EINVAL; + + return 0; + +err_attr_alloc: + onie_nvmem_attrs_free(parser); + return err; +} + +static int onie_nvmem_decode(struct onie_tlv_parser *parser, struct nvmem_device *nvmem) +{ + struct onie_tlv_hdr hdr; + u8 *data; + u16 len; + int ret; + + ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr); + if (ret < 0) + return ret; + + if (!onie_nvmem_hdr_is_valid(&hdr)) { + pr_err("invalid ONIE TLV header\n"); + return -EINVAL; + } + + len = be16_to_cpu(hdr.data_len); + + if (len > ONIE_NVMEM_TLV_MAX_LEN) + len = ONIE_NVMEM_TLV_MAX_LEN; + + data = kmalloc(len, GFP_KERNEL); + if (!data) + return -ENOMEM; + + ret = nvmem_device_read(nvmem, sizeof(hdr), len, data); + if (ret < 0) + goto err_data_read; + + ret = onie_nvmem_tlv_parse(parser, data, len); + if (ret) + goto err_info_parse; + + kfree(data); + + return 0; + +err_info_parse: +err_data_read: + kfree(data); + return ret; +} + +static int onie_nvmem_cells_parse(struct onie_tlv_parser *parser, + struct nvmem_device *nvmem, + struct nvmem_cell_table *table) +{ + struct nvmem_cell_info *cells; + struct onie_nvmem_attr *attr; + unsigned int ncells = 0; + int err; + + INIT_LIST_HEAD(&parser->attrs); + parser->attr_count = 0; + + err = onie_nvmem_decode(parser, nvmem); + if (err) + return err; + + cells = kmalloc_array(parser->attr_count, sizeof(*cells), GFP_KERNEL); + if (!cells) { + err = -ENOMEM; + goto err_cells_alloc; + } + + parser->lookup = kmalloc_array(parser->attr_count, + sizeof(struct nvmem_cell_lookup), + GFP_KERNEL); + if (!parser->lookup) { + err = -ENOMEM; + goto err_lookup_alloc; + } + + list_for_each_entry(attr, &parser->attrs, head) { + struct nvmem_cell_lookup *lookup; + struct nvmem_cell_info *cell; + + cell = &cells[ncells]; + + lookup = &parser->lookup[ncells]; + lookup->con_id = NULL; + + cell->offset = attr->offset; + cell->name = attr->name; + cell->bytes = attr->len; + cell->bit_offset = 0; + cell->nbits = 0; + + lookup->cell_name = cell->name; + lookup->con_id = cell->name; + + ncells++; + } + + table->ncells = ncells; + table->cells = cells; + + parser->nlookups = ncells; + + onie_nvmem_attrs_free(parser); + + return 0; + +err_lookup_alloc: + kfree(cells); +err_cells_alloc: + onie_nvmem_attrs_free(parser); + + return err; +} + +static int onie_cells_parse(struct nvmem_device *nvmem, + struct nvmem_parser_data *data) +{ + struct onie_tlv_parser parser; + int err; + + err = onie_nvmem_cells_parse(&parser, nvmem, &data->table); + if (err) { + pr_err("failed to parse ONIE attributes\n"); + return err; + } + + data->nlookups = parser.nlookups; + data->lookup = parser.lookup; + + return 0; +} + +static int __init onie_tlv_init(void) +{ + struct nvmem_parser_config parser_config = { }; + + parser_config.cells_parse = onie_cells_parse; + parser_config.owner = THIS_MODULE; + parser_config.name = "onie-tlv-cells"; + + nvmem_parser = nvmem_parser_register(&parser_config); + if (IS_ERR(nvmem_parser)) { + pr_err("failed to register %s parser\n", parser_config.name); + return PTR_ERR(nvmem_parser); + } + + pr_info("registered %s parser\n", parser_config.name); + + return 0; +} + +static void __exit onie_tlv_exit(void) +{ + nvmem_parser_unregister(nvmem_parser); +} + +module_init(onie_tlv_init); +module_exit(onie_tlv_exit); + +MODULE_AUTHOR("Vadym Kochan <vadym.kochan@plvision.eu>"); +MODULE_DESCRIPTION("ONIE TLV NVMEM cells parser"); +MODULE_LICENSE("GPL"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser 2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan @ 2021-08-06 15:39 ` Jan Lübbe 2021-09-08 9:56 ` Vadym Kochan 0 siblings, 1 reply; 36+ messages in thread From: Jan Lübbe @ 2021-08-06 15:39 UTC (permalink / raw) To: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree Cc: Robert Marko Hi, On Tue, 2021-06-08 at 22:03 +0300, Vadym Kochan wrote: ... > + case 0x24: return "mac-address"; ... > + case 0x2A: return "num-macs"; Is suspect these properties define which range of MACs is assigned to the device. How would you use them to assign MAC addresses to multiple interfaces? The nvmem-cells property in the network device's node can only refer to one cell, and not to i.e the cells value + 1. I think it would be useful to have a way to express this setup for systems with many interfaces, but am unsure of where this should be described. Maybe a "mac- address-offset" property in the generic ethernet controller binding? Regards, Jan -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser 2021-08-06 15:39 ` Jan Lübbe @ 2021-09-08 9:56 ` Vadym Kochan 2021-09-12 21:06 ` John Thomson 0 siblings, 1 reply; 36+ messages in thread From: Vadym Kochan @ 2021-09-08 9:56 UTC (permalink / raw) To: Jan Lübbe Cc: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree, Robert Marko Hi Jan, Jan Lübbe <jlu@pengutronix.de> writes: > Hi, > > On Tue, 2021-06-08 at 22:03 +0300, Vadym Kochan wrote: > > ... >> + case 0x24: return "mac-address"; > ... This is a base mac, >> + case 0x2A: return "num-macs"; > > Is suspect these properties define which range of MACs is assigned to the Yes > device. How would you use them to assign MAC addresses to multiple interfaces? > The nvmem-cells property in the network device's node can only refer to one > cell, and not to i.e the cells value + 1. > Currently in net/ethernet/marvell/prestera/prestera_main.c it is incremented and hard-coded by the driver. > I think it would be useful to have a way to express this setup for systems with > many interfaces, but am unsure of where this should be described. Maybe a "mac- > address-offset" property in the generic ethernet controller binding? > > Regards, > Jan May be something like eth_address_provider should be introduced in net/ethernet/ ? This provider can provide something like eth_provider_address_next() which will consider "mac-address-num" (or other specific fields). ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser 2021-09-08 9:56 ` Vadym Kochan @ 2021-09-12 21:06 ` John Thomson 2021-09-13 14:20 ` Srinivas Kandagatla 0 siblings, 1 reply; 36+ messages in thread From: John Thomson @ 2021-09-12 21:06 UTC (permalink / raw) To: Vadym Kochan, Jan Lübbe Cc: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree, Robert Marko Hi Vadym, On Wed, 8 Sep 2021, at 09:56, Vadym Kochan wrote: > > Hi Jan, > > Jan Lübbe <jlu@pengutronix.de> writes: … > > > I think it would be useful to have a way to express this setup for systems with > > many interfaces, but am unsure of where this should be described. Maybe a "mac- > > address-offset" property in the generic ethernet controller binding? > > > > Regards, > > Jan > > May be something like eth_address_provider should be introduced in > net/ethernet/ ? > > This provider can provide something like eth_provider_address_next() which > will consider "mac-address-num" (or other specific fields). > A patch series proposed the devicetree property mac-address-increment, but it did not get support at the time: of_net: add mac-address-increment support https://lore.kernel.org/all/20200920095724.8251-4-ansuelsmth@gmail.com/ dt-bindings: net: Document use of mac-address-increment https://lore.kernel.org/all/20200920095724.8251-5-ansuelsmth@gmail.com/ Cheers, -- John Thomson ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser 2021-09-12 21:06 ` John Thomson @ 2021-09-13 14:20 ` Srinivas Kandagatla 0 siblings, 0 replies; 36+ messages in thread From: Srinivas Kandagatla @ 2021-09-13 14:20 UTC (permalink / raw) To: John Thomson, Vadym Kochan, Jan Lübbe Cc: Rob Herring, linux-kernel, devicetree, Robert Marko On 12/09/2021 22:06, John Thomson wrote: > Hi Vadym, > > On Wed, 8 Sep 2021, at 09:56, Vadym Kochan wrote: >> >> Hi Jan, >> >> Jan Lübbe <jlu@pengutronix.de> writes: > … >> >>> I think it would be useful to have a way to express this setup for systems with >>> many interfaces, but am unsure of where this should be described. Maybe a "mac- >>> address-offset" property in the generic ethernet controller binding? >>> >>> Regards, >>> Jan >> >> May be something like eth_address_provider should be introduced in >> net/ethernet/ ? >> >> This provider can provide something like eth_provider_address_next() which >> will consider "mac-address-num" (or other specific fields). >> > > A patch series proposed the devicetree property > mac-address-increment, but it did not get support at the time: Please have a look at some recent nvmem patches https://lkml.org/lkml/2021/9/8/270 that adds support for vendor specific post-processing of nvmem-cells. Am hoping that increment usecase (along with other variants) should also be dealt in similar way. --srini > of_net: add mac-address-increment support > https://lore.kernel.org/all/20200920095724.8251-4-ansuelsmth@gmail.com/ > dt-bindings: net: Document use of mac-address-increment > https://lore.kernel.org/all/20200920095724.8251-5-ansuelsmth@gmail.com/ > > Cheers, > ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2021-09-28 14:40 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan 2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan 2021-06-08 22:49 ` kernel test robot 2021-06-08 22:49 ` kernel test robot 2021-06-09 3:05 ` kernel test robot 2021-06-09 3:05 ` kernel test robot 2021-06-14 10:44 ` Srinivas Kandagatla 2021-06-16 12:33 ` Vadym Kochan 2021-06-21 11:00 ` Srinivas Kandagatla 2021-09-08 9:38 ` Vadym Kochan 2021-09-13 14:19 ` Srinivas Kandagatla 2021-09-20 10:24 ` Vadym Kochan 2021-09-20 10:36 ` Srinivas Kandagatla 2021-09-20 11:25 ` Vadym Kochan 2021-09-20 11:32 ` Srinivas Kandagatla 2021-09-20 12:29 ` Vadym Kochan 2021-09-20 12:34 ` Srinivas Kandagatla 2021-09-20 13:29 ` Vadym Kochan 2021-09-20 13:40 ` Srinivas Kandagatla 2021-09-21 5:50 ` John Thomson 2021-09-27 7:50 ` Vadym Kochan 2021-09-27 10:12 ` Srinivas Kandagatla 2021-09-28 13:31 ` Vadym Kochan 2021-09-28 13:51 ` Srinivas Kandagatla 2021-09-28 14:11 ` Vadym Kochan 2021-09-28 14:39 ` Srinivas Kandagatla 2021-09-27 10:12 ` Srinivas Kandagatla 2021-09-27 12:38 ` John Thomson 2021-09-08 9:44 ` Vadym Kochan 2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan 2021-06-18 20:59 ` Rob Herring 2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan 2021-08-06 15:39 ` Jan Lübbe 2021-09-08 9:56 ` Vadym Kochan 2021-09-12 21:06 ` John Thomson 2021-09-13 14:20 ` Srinivas Kandagatla
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.