lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v1 06/14] nvmem: core: introduce NVMEM layouts
    From


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

    If we relax add_cells + add nvmem_unregister_layout() and update struct
    nvmem_layout to include post_process callback like

    struct nvmem_layout {
    const char *name;
    const struct of_device_id *of_match_table;
    int (*add_cells)(struct nvmem_device *nvmem, struct nvmem_layout *layout);
    struct list_head node;
    /* default callback for every cell */
    nvmem_cell_post_process_t post_process;
    };

    then we can move imx-ocotp to this layout style without add_cell
    callback, and finally get rid of cell_process_callback from both
    nvmem_config and nvmem_device.

    If layout specific post_process callback is available and cell does not
    have a callback set then we can can be either updated cell post_process
    callback with this one or invoke layout specific callback directly.

    does that make sense?


    --srini


    >
    > -michael

    \
     
     \ /
      Last update: 2022-08-30 16:43    [W:4.070 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site