2018-08-10 08:12:00

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 00/29] at24: remove at24_platform_data

From: Bartosz Golaszewski <[email protected]>

This is a follow-up to the previously rejected series[1] which partially
removed the at24_platform_data structure. After further development and
taking reviews into account, this series finally removes that struct
completely but not without touching many different parts of the code
base.

Since I took over maintainership of the at24 driver I've been working
towards removing at24_platform_data in favor for device properties.

DaVinci is the only platform that's still using it - all other users
have already been converted.

One of the obstacles in case of DaVinci is removing the setup() callback
from the pdata struct, the only user of which are some davinci boards.

Most boards use the EEPROM to store the MAC address. This series adds
support for cell lookups to the nvmem framework, registers relevant
cells for all users, adds nvmem support to eth_platform_get_mac_address(),
converts davinci_emac driver to using it and replaces at24_platform_data
with device properties.

There's also one board (da850-evm) which uses MTD for reading the MAC
address. I used the patch from Alban Bedel's previous submission[2] to
add support for nvmem to the MTD framework. Since this user doesn't
need device tree, I dropped Alban's patches modifying the DT bindings.
We can add that later once an agreement is reached. For the time being
MTD devices are registered as nvmem devices and we're registering the
mac-address cell using the cell lookup mechanism.

This series adds a blocking notifier chain to the nvmem framework, so
that we can keep the EEPROM reading code in the mityomapl138 board file
with only slight modifications.

I also included some minor fixes to the modified code.

Tested on da850-evm & dm365-evm.

[1] https://lkml.org/lkml/2018/6/29/153
[2] https://lkml.org/lkml/2018/3/24/312

v1 -> v2:

PATCH 4/29:
- change name to nvmem_dev_name()
- return NULL instead of ERR_PTR(-ENOSYS) from nvmem_dev_name() if nvmem is
not configured

PATCH 5/29:
- s/nvmem_device_name/nvmem_dev_name/

PATCH 6/29:
- fix the commit message: remove the part mentioning the OF systems

PATCH 7/29:
- fix ordering of includes

PATCH 8/29:
- fix ordering of includes

PATCH 9/29:
- fix ordering of includes

PATCH 10/29:
- fix ordering of includes

PATCH 11/29:
- fix ordering of includes

PATCH 14/29:
- new patch

PATCH 22/29:
- added missing terminator

PATCH 25/29:
- removed unneeded coma

PATCH 27/29:
- s/nvmem_device_name/nvmem_dev_name/
- fix ordering of includes

PATCH 28/29:
- added missing terminator

Alban Bedel (1):
mtd: Add support for reading MTD devices via the nvmem API

Bartosz Golaszewski (28):
nvmem: add support for cell lookups
Documentation: nvmem: document lookup entries
nvmem: add a notifier chain
nvmem: provide nvmem_dev_name()
nvmem: remove the name field from struct nvmem_device
ARM: davinci: dm365-evm: use nvmem lookup for mac address
ARM: davinci: dm644-evm: use nvmem lookup for mac address
ARM: davinci: dm646x-evm: use nvmem lookup for mac address
ARM: davinci: da830-evm: use nvmem lookup for mac address
ARM: davinci: mityomapl138: add nvmem cells lookup entries
ARM: davinci: da850-evm: use nvmem lookup for mac address
ARM: davinci: da850-evm: remove unnecessary include
net: simplify eth_platform_get_mac_address()
net: split eth_platform_get_mac_address() into subroutines
net: add support for nvmem to eth_platform_get_mac_address()
net: davinci_emac: use eth_platform_get_mac_address()
ARM: davinci: da850-evm: remove dead MTD code
ARM: davinci: mityomapl138: don't read the MAC address from machine
code
ARM: davinci: dm365-evm: use device properties for at24 eeprom
ARM: davinci: da830-evm: use device properties for at24 eeprom
ARM: davinci: dm644x-evm: use device properties for at24 eeprom
ARM: davinci: dm646x-evm: use device properties for at24 eeprom
ARM: davinci: sffsdr: fix the at24 eeprom device name
ARM: davinci: sffsdr: use device properties for at24 eeprom
ARM: davinci: remove dead code related to MAC address reading
ARM: davinci: mityomapl138: use nvmem notifiers
ARM: davinci: mityomapl138: use device properties for at24 eeprom
eeprom: at24: kill at24_platform_data

Documentation/nvmem/nvmem.txt | 28 +++++
MAINTAINERS | 1 -
arch/arm/mach-davinci/board-da830-evm.c | 25 ++--
arch/arm/mach-davinci/board-da850-evm.c | 45 +++-----
arch/arm/mach-davinci/board-dm365-evm.c | 25 ++--
arch/arm/mach-davinci/board-dm644x-evm.c | 25 ++--
arch/arm/mach-davinci/board-dm646x-evm.c | 25 ++--
arch/arm/mach-davinci/board-mityomapl138.c | 60 +++++++---
arch/arm/mach-davinci/board-sffsdr.c | 13 +--
arch/arm/mach-davinci/common.c | 15 ---
drivers/misc/eeprom/at24.c | 127 +++++++++------------
drivers/mtd/Kconfig | 1 +
drivers/mtd/mtdcore.c | 50 ++++++++
drivers/net/ethernet/ti/davinci_emac.c | 12 +-
drivers/nvmem/core.c | 106 ++++++++++++++++-
include/linux/davinci_emac.h | 2 -
include/linux/mtd/mtd.h | 2 +
include/linux/nvmem-consumer.h | 31 +++++
include/linux/nvmem-provider.h | 10 ++
include/linux/platform_data/at24.h | 60 ----------
net/ethernet/eth.c | 84 ++++++++++++--
21 files changed, 492 insertions(+), 255 deletions(-)
delete mode 100644 include/linux/platform_data/at24.h

--
2.18.0



2018-08-10 08:07:12

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 08/29] ARM: davinci: dm644-evm: use nvmem lookup for mac address

From: Bartosz Golaszewski <[email protected]>

We now support nvmem lookups for machine code. Add a lookup for
mac-address.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-dm644x-evm.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index 48436f74fd71..829fa1c8d0b4 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -22,6 +22,7 @@
#include <linux/mtd/rawnand.h>
#include <linux/mtd/partitions.h>
#include <linux/mtd/physmap.h>
+#include <linux/nvmem-provider.h>
#include <linux/phy.h>
#include <linux/clk.h>
#include <linux/videodev2.h>
@@ -476,6 +477,15 @@ static struct pcf857x_platform_data pcf_data_u35 = {
* - ... newer boards may have more
*/

+static struct nvmem_cell_lookup dm6446evm_mac_address_cell = {
+ .info = {
+ .name = "mac-address",
+ .offset = 0x7f00,
+ .bytes = ETH_ALEN,
+ },
+ .nvmem_name = "1-00500",
+};
+
static struct at24_platform_data eeprom_info = {
.byte_len = (256*1024) / 8,
.page_size = 64,
@@ -828,6 +838,8 @@ static __init void davinci_evm_init(void)
phy_register_fixup_for_uid(LXT971_PHY_ID, LXT971_PHY_MASK,
davinci_phy_fixup);
}
+
+ nvmem_add_lookup_table(&dm6446evm_mac_address_cell, 1);
}

MACHINE_START(DAVINCI_EVM, "DaVinci DM644x EVM")
--
2.18.0


2018-08-10 08:07:29

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 17/29] net: davinci_emac: use eth_platform_get_mac_address()

From: Bartosz Golaszewski <[email protected]>

We now support nvmem in eth_platform_get_mac_address() and all boards
have the mac-address cells defined. Stop getting the MAC from pdata
and use the dedicated helper.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/net/ethernet/ti/davinci_emac.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index f270beebb428..5d01bad4baa5 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1700,7 +1700,6 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
const struct of_device_id *match;
const struct emac_platform_data *auxdata;
struct emac_platform_data *pdata = NULL;
- const u8 *mac_addr;

if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
return dev_get_platdata(&pdev->dev);
@@ -1712,12 +1711,6 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
np = pdev->dev.of_node;
pdata->version = EMAC_VERSION_2;

- if (!is_valid_ether_addr(pdata->mac_addr)) {
- mac_addr = of_get_mac_address(np);
- if (mac_addr)
- ether_addr_copy(pdata->mac_addr, mac_addr);
- }
-
of_property_read_u32(np, "ti,davinci-ctrl-reg-offset",
&pdata->ctrl_reg_offset);

@@ -1819,8 +1812,11 @@ static int davinci_emac_probe(struct platform_device *pdev)
goto err_free_netdev;
}

+ rc = eth_platform_get_mac_address(&pdev->dev, priv->mac_addr);
+ if (rc == -EPROBE_DEFER)
+ return -EPROBE_DEFER; /* We'll get the MAC address later. */
+
/* MAC addr and PHY mask , RMII enable info from platform_data */
- memcpy(priv->mac_addr, pdata->mac_addr, ETH_ALEN);
priv->phy_id = pdata->phy_id;
priv->rmii_en = pdata->rmii_en;
priv->version = pdata->version;
--
2.18.0


2018-08-10 08:07:40

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 25/29] ARM: davinci: sffsdr: use device properties for at24 eeprom

From: Bartosz Golaszewski <[email protected]>

We want to work towards phasing out the at24_platform_data structure.
There are few users and its contents can be represented using generic
device properties. Using device properties only will allow us to
significantly simplify the at24 configuration code.

Remove the at24_platform_data structure and replace it with an array
of property entries. Drop the byte_len/size property, as the model name
already implies the EEPROM's size.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
arch/arm/mach-davinci/board-sffsdr.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/board-sffsdr.c b/arch/arm/mach-davinci/board-sffsdr.c
index f6a4d094cbc3..4f3bc78e8154 100644
--- a/arch/arm/mach-davinci/board-sffsdr.c
+++ b/arch/arm/mach-davinci/board-sffsdr.c
@@ -26,7 +26,7 @@
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/i2c.h>
-#include <linux/platform_data/at24.h>
+#include <linux/property.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/rawnand.h>
#include <linux/mtd/partitions.h>
@@ -92,16 +92,15 @@ static struct platform_device davinci_sffsdr_nandflash_device = {
.resource = davinci_sffsdr_nandflash_resource,
};

-static struct at24_platform_data eeprom_info = {
- .byte_len = (64*1024) / 8,
- .page_size = 32,
- .flags = AT24_FLAG_ADDR16,
+static const struct property_entry eeprom_properties[] = {
+ PROPERTY_ENTRY_U32("pagesize", 32),
+ { }
};

static struct i2c_board_info __initdata i2c_info[] = {
{
I2C_BOARD_INFO("24c64", 0x50),
- .platform_data = &eeprom_info,
+ .properties = eeprom_properties,
},
/* Other I2C devices:
* MSP430, addr 0x23 (not used)
--
2.18.0


2018-08-10 08:07:50

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 29/29] eeprom: at24: kill at24_platform_data

From: Bartosz Golaszewski <[email protected]>

There are no more users of at24_platform_data. Remove the relevant
header and modify the driver code to not use it anymore.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
MAINTAINERS | 1 -
drivers/misc/eeprom/at24.c | 127 +++++++++++++----------------
include/linux/platform_data/at24.h | 60 --------------
3 files changed, 57 insertions(+), 131 deletions(-)
delete mode 100644 include/linux/platform_data/at24.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7cebd5bba8a8..8eb87a3548f8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2328,7 +2328,6 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
S: Maintained
F: Documentation/devicetree/bindings/eeprom/at24.txt
F: drivers/misc/eeprom/at24.c
-F: include/linux/platform_data/at24.h

ATA OVER ETHERNET (AOE) DRIVER
M: "Ed L. Cashin" <[email protected]>
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index f5cc517d1131..93642b4b47c5 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -22,10 +22,24 @@
#include <linux/i2c.h>
#include <linux/nvmem-provider.h>
#include <linux/regmap.h>
-#include <linux/platform_data/at24.h>
#include <linux/pm_runtime.h>
#include <linux/gpio/consumer.h>

+/* Address pointer is 16 bit. */
+#define AT24_FLAG_ADDR16 BIT(7)
+/* sysfs-entry will be read-only. */
+#define AT24_FLAG_READONLY BIT(6)
+/* sysfs-entry will be world-readable. */
+#define AT24_FLAG_IRUGO BIT(5)
+/* Take always 8 addresses (24c00). */
+#define AT24_FLAG_TAKE8ADDR BIT(4)
+/* Factory-programmed serial number. */
+#define AT24_FLAG_SERIAL BIT(3)
+/* Factory-programmed mac address. */
+#define AT24_FLAG_MAC BIT(2)
+/* Does not auto-rollover reads to the next slave address. */
+#define AT24_FLAG_NO_RDROL BIT(1)
+
/*
* I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
* Differences between different vendor product lines (like Atmel AT24C or
@@ -124,10 +138,6 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
usleep_range(1000, 1500), op_time = jiffies)

struct at24_chip_data {
- /*
- * these fields mirror their equivalents in
- * struct at24_platform_data
- */
u32 byte_len;
u8 flags;
};
@@ -467,46 +477,11 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
return 0;
}

-static void at24_properties_to_pdata(struct device *dev,
- struct at24_platform_data *chip)
-{
- int err;
- u32 val;
-
- if (device_property_present(dev, "read-only"))
- chip->flags |= AT24_FLAG_READONLY;
- if (device_property_present(dev, "no-read-rollover"))
- chip->flags |= AT24_FLAG_NO_RDROL;
-
- err = device_property_read_u32(dev, "size", &val);
- if (!err)
- chip->byte_len = val;
-
- err = device_property_read_u32(dev, "pagesize", &val);
- if (!err) {
- chip->page_size = val;
- } else {
- /*
- * This is slow, but we can't know all eeproms, so we better
- * play safe. Specifying custom eeprom-types via platform_data
- * is recommended anyhow.
- */
- chip->page_size = 1;
- }
-}
-
-static int at24_get_pdata(struct device *dev, struct at24_platform_data *pdata)
+static const struct at24_chip_data *at24_get_chip_data(struct device *dev)
{
struct device_node *of_node = dev->of_node;
const struct at24_chip_data *cdata;
const struct i2c_device_id *id;
- struct at24_platform_data *pd;
-
- pd = dev_get_platdata(dev);
- if (pd) {
- memcpy(pdata, pd, sizeof(*pdata));
- return 0;
- }

id = i2c_match_id(at24_ids, to_i2c_client(dev));

@@ -523,13 +498,9 @@ static int at24_get_pdata(struct device *dev, struct at24_platform_data *pdata)
cdata = acpi_device_get_match_data(dev);

if (!cdata)
- return -ENODEV;
+ return ERR_PTR(-ENODEV);

- pdata->byte_len = cdata->byte_len;
- pdata->flags = cdata->flags;
- at24_properties_to_pdata(dev, pdata);
-
- return 0;
+ return cdata;
}

static void at24_remove_dummy_clients(struct at24_data *at24)
@@ -598,8 +569,9 @@ static int at24_probe(struct i2c_client *client)
{
struct regmap_config regmap_config = { };
struct nvmem_config nvmem_config = { };
- struct at24_platform_data pdata = { };
+ const struct at24_chip_data *cdata;
struct device *dev = &client->dev;
+ u32 byte_len, page_size, flags;
bool i2c_fn_i2c, i2c_fn_block;
unsigned int i, num_addresses;
struct at24_data *at24;
@@ -613,35 +585,54 @@ static int at24_probe(struct i2c_client *client)
i2c_fn_block = i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_WRITE_I2C_BLOCK);

- err = at24_get_pdata(dev, &pdata);
+ cdata = at24_get_chip_data(dev);
+ if (IS_ERR(cdata))
+ return PTR_ERR(cdata);
+
+ err = device_property_read_u32(dev, "pagesize", &page_size);
+ if (err)
+ /*
+ * This is slow, but we can't know all eeproms, so we better
+ * play safe. Specifying custom eeprom-types via platform_data
+ * is recommended anyhow.
+ */
+ page_size = 1;
+
+ flags = cdata->flags;
+ if (device_property_present(dev, "read-only"))
+ flags |= AT24_FLAG_READONLY;
+ if (device_property_present(dev, "no-read-rollover"))
+ flags |= AT24_FLAG_NO_RDROL;
+
+ err = device_property_read_u32(dev, "size", &byte_len);
if (err)
- return err;
+ byte_len = cdata->byte_len;

if (!i2c_fn_i2c && !i2c_fn_block)
- pdata.page_size = 1;
+ page_size = 1;

- if (!pdata.page_size) {
+ if (!page_size) {
dev_err(dev, "page_size must not be 0!\n");
return -EINVAL;
}

- if (!is_power_of_2(pdata.page_size))
+ if (!is_power_of_2(page_size))
dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");

- if (pdata.flags & AT24_FLAG_TAKE8ADDR)
+ if (flags & AT24_FLAG_TAKE8ADDR)
num_addresses = 8;
else
- num_addresses = DIV_ROUND_UP(pdata.byte_len,
- (pdata.flags & AT24_FLAG_ADDR16) ? 65536 : 256);
+ num_addresses = DIV_ROUND_UP(byte_len,
+ (flags & AT24_FLAG_ADDR16) ? 65536 : 256);

- if ((pdata.flags & AT24_FLAG_SERIAL) && (pdata.flags & AT24_FLAG_MAC)) {
+ if ((flags & AT24_FLAG_SERIAL) && (flags & AT24_FLAG_MAC)) {
dev_err(dev,
"invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
return -EINVAL;
}

regmap_config.val_bits = 8;
- regmap_config.reg_bits = (pdata.flags & AT24_FLAG_ADDR16) ? 16 : 8;
+ regmap_config.reg_bits = (flags & AT24_FLAG_ADDR16) ? 16 : 8;
regmap_config.disable_locking = true;

regmap = devm_regmap_init_i2c(client, &regmap_config);
@@ -654,11 +645,11 @@ static int at24_probe(struct i2c_client *client)
return -ENOMEM;

mutex_init(&at24->lock);
- at24->byte_len = pdata.byte_len;
- at24->page_size = pdata.page_size;
- at24->flags = pdata.flags;
+ at24->byte_len = byte_len;
+ at24->page_size = page_size;
+ at24->flags = flags;
at24->num_addresses = num_addresses;
- at24->offset_adj = at24_get_offset_adj(pdata.flags, pdata.byte_len);
+ at24->offset_adj = at24_get_offset_adj(flags, byte_len);
at24->client[0].client = client;
at24->client[0].regmap = regmap;

@@ -666,10 +657,10 @@ static int at24_probe(struct i2c_client *client)
if (IS_ERR(at24->wp_gpio))
return PTR_ERR(at24->wp_gpio);

- writable = !(pdata.flags & AT24_FLAG_READONLY);
+ writable = !(flags & AT24_FLAG_READONLY);
if (writable) {
at24->write_max = min_t(unsigned int,
- pdata.page_size, at24_io_limit);
+ page_size, at24_io_limit);
if (!i2c_fn_i2c && at24->write_max > I2C_SMBUS_BLOCK_MAX)
at24->write_max = I2C_SMBUS_BLOCK_MAX;
}
@@ -712,7 +703,7 @@ static int at24_probe(struct i2c_client *client)
nvmem_config.priv = at24;
nvmem_config.stride = 1;
nvmem_config.word_size = 1;
- nvmem_config.size = pdata.byte_len;
+ nvmem_config.size = byte_len;

at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
if (IS_ERR(at24->nvmem)) {
@@ -721,13 +712,9 @@ static int at24_probe(struct i2c_client *client)
}

dev_info(dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
- pdata.byte_len, client->name,
+ byte_len, client->name,
writable ? "writable" : "read-only", at24->write_max);

- /* export data to kernel code */
- if (pdata.setup)
- pdata.setup(at24->nvmem, pdata.context);
-
return 0;

err_clients:
diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h
deleted file mode 100644
index 63507ff464ee..000000000000
--- a/include/linux/platform_data/at24.h
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * at24.h - platform_data for the at24 (generic eeprom) driver
- * (C) Copyright 2008 by Pengutronix
- * (C) Copyright 2012 by Wolfram Sang
- * same license as the driver
- */
-
-#ifndef _LINUX_AT24_H
-#define _LINUX_AT24_H
-
-#include <linux/types.h>
-#include <linux/nvmem-consumer.h>
-#include <linux/bitops.h>
-
-/**
- * struct at24_platform_data - data to set up at24 (generic eeprom) driver
- * @byte_len: size of eeprom in byte
- * @page_size: number of byte which can be written in one go
- * @flags: tunable options, check AT24_FLAG_* defines
- * @setup: an optional callback invoked after eeprom is probed; enables kernel
- code to access eeprom via nvmem, see example
- * @context: optional parameter passed to setup()
- *
- * If you set up a custom eeprom type, please double-check the parameters.
- * Especially page_size needs extra care, as you risk data loss if your value
- * is bigger than what the chip actually supports!
- *
- * An example in pseudo code for a setup() callback:
- *
- * void get_mac_addr(struct nvmem_device *nvmem, void *context)
- * {
- * u8 *mac_addr = ethernet_pdata->mac_addr;
- * off_t offset = context;
- *
- * // Read MAC addr from EEPROM
- * if (nvmem_device_read(nvmem, offset, ETH_ALEN, mac_addr) == ETH_ALEN)
- * pr_info("Read MAC addr from EEPROM: %pM\n", mac_addr);
- * }
- *
- * This function pointer and context can now be set up in at24_platform_data.
- */
-
-struct at24_platform_data {
- u32 byte_len; /* size (sum of all addr) */
- u16 page_size; /* for writes */
- u8 flags;
-#define AT24_FLAG_ADDR16 BIT(7) /* address pointer is 16 bit */
-#define AT24_FLAG_READONLY BIT(6) /* sysfs-entry will be read-only */
-#define AT24_FLAG_IRUGO BIT(5) /* sysfs-entry will be world-readable */
-#define AT24_FLAG_TAKE8ADDR BIT(4) /* take always 8 addresses (24c00) */
-#define AT24_FLAG_SERIAL BIT(3) /* factory-programmed serial number */
-#define AT24_FLAG_MAC BIT(2) /* factory-programmed mac address */
-#define AT24_FLAG_NO_RDROL BIT(1) /* does not auto-rollover reads to */
- /* the next slave address */
-
- void (*setup)(struct nvmem_device *nvmem, void *context);
- void *context;
-};
-
-#endif /* _LINUX_AT24_H */
--
2.18.0


2018-08-10 08:08:06

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 27/29] ARM: davinci: mityomapl138: use nvmem notifiers

From: Bartosz Golaszewski <[email protected]>

Stop using the at24_platform_data setup callback in favor of nvmem
notifiers.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-mityomapl138.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index 35b2b6068a7a..dacc5292f9bd 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -15,6 +15,8 @@
#include <linux/console.h>
#include <linux/platform_device.h>
#include <linux/mtd/partitions.h>
+#include <linux/notifier.h>
+#include <linux/nvmem-consumer.h>
#include <linux/nvmem-provider.h>
#include <linux/regulator/machine.h>
#include <linux/i2c.h>
@@ -116,10 +118,15 @@ static void mityomapl138_cpufreq_init(const char *partnum)
static void mityomapl138_cpufreq_init(const char *partnum) { }
#endif

-static void read_factory_config(struct nvmem_device *nvmem, void *context)
+static int read_factory_config(struct notifier_block *nb,
+ unsigned long event, void *data)
{
int ret;
const char *partnum = NULL;
+ struct nvmem_device *nvmem = data;
+
+ if (strcmp(nvmem_dev_name(nvmem), "1-00500") != 0)
+ return NOTIFY_DONE;

if (!IS_BUILTIN(CONFIG_NVMEM)) {
pr_warn("Factory Config not available without CONFIG_NVMEM\n");
@@ -151,8 +158,14 @@ static void read_factory_config(struct nvmem_device *nvmem, void *context)
bad_config:
/* default maximum speed is valid for all platforms */
mityomapl138_cpufreq_init(partnum);
+
+ return NOTIFY_STOP;
}

+static struct notifier_block mityomapl138_nvmem_notifier = {
+ .notifier_call = read_factory_config,
+};
+
static struct nvmem_cell_lookup mityomapl138_nvmem_cells[] = {
{
.info = {
@@ -176,8 +189,6 @@ static struct at24_platform_data mityomapl138_fd_chip = {
.byte_len = 256,
.page_size = 8,
.flags = AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
- .setup = read_factory_config,
- .context = NULL,
};

static struct davinci_i2c_platform_data mityomap_i2c_0_pdata = {
@@ -546,6 +557,7 @@ static void __init mityomapl138_init(void)
if (ret)
pr_warn("spi 1 registration failed: %d\n", ret);

+ nvmem_register_notifier(&mityomapl138_nvmem_notifier);
nvmem_add_lookup_table(mityomapl138_nvmem_cells,
ARRAY_SIZE(mityomapl138_nvmem_cells));
mityomapl138_config_emac();
--
2.18.0


2018-08-10 08:08:26

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 24/29] ARM: davinci: sffsdr: fix the at24 eeprom device name

From: Bartosz Golaszewski <[email protected]>

The currently used 24lc64 i2c device name doesn't match against any
of the devices supported by the at24 driver. Change it to the closest
compatible chip.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-sffsdr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/board-sffsdr.c b/arch/arm/mach-davinci/board-sffsdr.c
index e7c1728b0833..f6a4d094cbc3 100644
--- a/arch/arm/mach-davinci/board-sffsdr.c
+++ b/arch/arm/mach-davinci/board-sffsdr.c
@@ -100,7 +100,7 @@ static struct at24_platform_data eeprom_info = {

static struct i2c_board_info __initdata i2c_info[] = {
{
- I2C_BOARD_INFO("24lc64", 0x50),
+ I2C_BOARD_INFO("24c64", 0x50),
.platform_data = &eeprom_info,
},
/* Other I2C devices:
--
2.18.0


2018-08-10 08:08:41

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 23/29] ARM: davinci: dm646x-evm: use device properties for at24 eeprom

From: Bartosz Golaszewski <[email protected]>

We want to work towards phasing out the at24_platform_data structure.
There are few users and its contents can be represented using generic
device properties. Using device properties only will allow us to
significantly simplify the at24 configuration code.

Remove the at24_platform_data structure and replace it with an array
of property entries. Drop the byte_len/size property, as the model name
already implies the EEPROM's size.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
arch/arm/mach-davinci/board-dm646x-evm.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index 943ab0ca6db8..04d293096962 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -22,7 +22,7 @@
#include <linux/gpio.h>
#include <linux/platform_device.h>
#include <linux/i2c.h>
-#include <linux/platform_data/at24.h>
+#include <linux/property.h>
#include <linux/platform_data/pcf857x.h>

#include <media/i2c/tvp514x.h>
@@ -320,12 +320,9 @@ static struct nvmem_cell_lookup dm646x_evm_mac_address_cell = {
.nvmem_name = "1-00500",
};

-static struct at24_platform_data eeprom_info = {
- .byte_len = (256*1024) / 8,
- .page_size = 64,
- .flags = AT24_FLAG_ADDR16,
- .setup = davinci_get_mac_addr,
- .context = (void *)0x7f00,
+static const struct property_entry eeprom_properties[] = {
+ PROPERTY_ENTRY_U32("pagesize", 64),
+ { }
};
#endif

@@ -396,7 +393,7 @@ static void evm_init_cpld(void)
static struct i2c_board_info __initdata i2c_info[] = {
{
I2C_BOARD_INFO("24c256", 0x50),
- .platform_data = &eeprom_info,
+ .properties = eeprom_properties,
},
{
I2C_BOARD_INFO("pcf8574a", 0x38),
--
2.18.0


2018-08-10 08:08:43

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 26/29] ARM: davinci: remove dead code related to MAC address reading

From: Bartosz Golaszewski <[email protected]>

There are no more users of davinci_get_mac_addr(). Remove it. Also
remove the mac_addr field from the emac platform data struct.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/common.c | 15 ---------------
include/linux/davinci_emac.h | 2 --
2 files changed, 17 deletions(-)

diff --git a/arch/arm/mach-davinci/common.c b/arch/arm/mach-davinci/common.c
index bcb6a7ba84e9..0c6cc354a4aa 100644
--- a/arch/arm/mach-davinci/common.c
+++ b/arch/arm/mach-davinci/common.c
@@ -28,21 +28,6 @@ EXPORT_SYMBOL(davinci_soc_info);
void __iomem *davinci_intc_base;
int davinci_intc_type;

-void davinci_get_mac_addr(struct nvmem_device *nvmem, void *context)
-{
- char *mac_addr = davinci_soc_info.emac_pdata->mac_addr;
- off_t offset = (off_t)context;
-
- if (!IS_BUILTIN(CONFIG_NVMEM)) {
- pr_warn("Cannot read MAC addr from EEPROM without CONFIG_NVMEM\n");
- return;
- }
-
- /* Read MAC addr from EEPROM */
- if (nvmem_device_read(nvmem, offset, ETH_ALEN, mac_addr) == ETH_ALEN)
- pr_info("Read MAC addr from EEPROM: %pM\n", mac_addr);
-}
-
static int __init davinci_init_id(struct davinci_soc_info *soc_info)
{
int i;
diff --git a/include/linux/davinci_emac.h b/include/linux/davinci_emac.h
index 05b97144d342..19888b27706d 100644
--- a/include/linux/davinci_emac.h
+++ b/include/linux/davinci_emac.h
@@ -19,7 +19,6 @@ struct mdio_platform_data {
};

struct emac_platform_data {
- char mac_addr[ETH_ALEN];
u32 ctrl_reg_offset;
u32 ctrl_mod_reg_offset;
u32 ctrl_ram_offset;
@@ -46,5 +45,4 @@ enum {
EMAC_VERSION_2, /* DM646x */
};

-void davinci_get_mac_addr(struct nvmem_device *nvmem, void *context);
#endif
--
2.18.0


2018-08-10 08:08:56

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 21/29] ARM: davinci: da830-evm: use device properties for at24 eeprom

From: Bartosz Golaszewski <[email protected]>

We want to work towards phasing out the at24_platform_data structure.
There are few users and its contents can be represented using generic
device properties. Using device properties only will allow us to
significantly simplify the at24 configuration code.

Remove the at24_platform_data structure and replace it with an array
of property entries. Drop the byte_len/size property, as the model name
already implies the EEPROM's size.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
arch/arm/mach-davinci/board-da830-evm.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 9a055ebba081..fcfd1565bfdc 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -18,7 +18,7 @@
#include <linux/platform_device.h>
#include <linux/i2c.h>
#include <linux/platform_data/pcf857x.h>
-#include <linux/platform_data/at24.h>
+#include <linux/property.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
#include <linux/nvmem-provider.h>
@@ -419,12 +419,9 @@ static struct nvmem_cell_lookup da830_evm_mac_address_cell = {
.nvmem_name = "1-00500",
};

-static struct at24_platform_data da830_evm_i2c_eeprom_info = {
- .byte_len = SZ_256K / 8,
- .page_size = 64,
- .flags = AT24_FLAG_ADDR16,
- .setup = davinci_get_mac_addr,
- .context = (void *)0x7f00,
+static const struct property_entry da830_evm_i2c_eeprom_properties[] = {
+ PROPERTY_ENTRY_U32("pagesize", 64),
+ { }
};

static int __init da830_evm_ui_expander_setup(struct i2c_client *client,
@@ -458,7 +455,7 @@ static struct pcf857x_platform_data __initdata da830_evm_ui_expander_info = {
static struct i2c_board_info __initdata da830_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("24c256", 0x50),
- .platform_data = &da830_evm_i2c_eeprom_info,
+ .properties = da830_evm_i2c_eeprom_properties,
},
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
--
2.18.0


2018-08-10 08:09:08

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 28/29] ARM: davinci: mityomapl138: use device properties for at24 eeprom

From: Bartosz Golaszewski <[email protected]>

We want to work towards phasing out the at24_platform_data structure.
There are few users and its contents can be represented using generic
device properties. Using device properties only will allow us to
significantly simplify the at24 configuration code.

Remove the at24_platform_data structure and replace it with an array
of property entries. Drop the byte_len/size property, as the model name
already implies the EEPROM's size.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
arch/arm/mach-davinci/board-mityomapl138.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index dacc5292f9bd..db50fcce5976 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -14,13 +14,13 @@
#include <linux/init.h>
#include <linux/console.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/mtd/partitions.h>
#include <linux/notifier.h>
#include <linux/nvmem-consumer.h>
#include <linux/nvmem-provider.h>
#include <linux/regulator/machine.h>
#include <linux/i2c.h>
-#include <linux/platform_data/at24.h>
#include <linux/etherdevice.h>
#include <linux/spi/spi.h>
#include <linux/spi/flash.h>
@@ -185,10 +185,10 @@ static struct nvmem_cell_lookup mityomapl138_nvmem_cells[] = {
}
};

-static struct at24_platform_data mityomapl138_fd_chip = {
- .byte_len = 256,
- .page_size = 8,
- .flags = AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
+static const struct property_entry mityomapl138_fd_chip_properties[] = {
+ PROPERTY_ENTRY_U32("pagesize", 8),
+ PROPERTY_ENTRY_BOOL("read-only"),
+ { }
};

static struct davinci_i2c_platform_data mityomap_i2c_0_pdata = {
@@ -317,7 +317,7 @@ static struct i2c_board_info __initdata mityomap_tps65023_info[] = {
},
{
I2C_BOARD_INFO("24c02", 0x50),
- .platform_data = &mityomapl138_fd_chip,
+ .properties = mityomapl138_fd_chip_properties,
},
};

--
2.18.0


2018-08-10 08:10:33

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 10/29] ARM: davinci: da830-evm: use nvmem lookup for mac address

From: Bartosz Golaszewski <[email protected]>

We now support nvmem lookups for machine code. Add a lookup for
mac-address.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-da830-evm.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 14a6fc061744..9a055ebba081 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -21,6 +21,7 @@
#include <linux/platform_data/at24.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
+#include <linux/nvmem-provider.h>
#include <linux/spi/spi.h>
#include <linux/spi/flash.h>
#include <linux/platform_data/gpio-davinci.h>
@@ -409,6 +410,15 @@ static inline void da830_evm_init_lcdc(int mux_mode)
static inline void da830_evm_init_lcdc(int mux_mode) { }
#endif

+static struct nvmem_cell_lookup da830_evm_mac_address_cell = {
+ .info = {
+ .name = "mac-address",
+ .offset = 0x7f00,
+ .bytes = ETH_ALEN,
+ },
+ .nvmem_name = "1-00500",
+};
+
static struct at24_platform_data da830_evm_i2c_eeprom_info = {
.byte_len = SZ_256K / 8,
.page_size = 64,
@@ -618,6 +628,8 @@ static __init void da830_evm_init(void)
pr_warn("%s: spi 0 registration failed: %d\n", __func__, ret);

regulator_has_full_constraints();
+
+ nvmem_add_lookup_table(&da830_evm_mac_address_cell, 1);
}

#ifdef CONFIG_SERIAL_8250_CONSOLE
--
2.18.0


2018-08-10 08:11:14

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

From: Alban Bedel <[email protected]>

Allow drivers that use the nvmem API to read data stored on MTD devices.
For this the mtd devices are registered as read-only NVMEM providers.

Signed-off-by: Alban Bedel <[email protected]>
[Bartosz:
- use the managed variant of nvmem_register(),
- set the nvmem name]
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/mtd/Kconfig | 1 +
drivers/mtd/mtdcore.c | 50 +++++++++++++++++++++++++++++++++++++++++
include/linux/mtd/mtd.h | 2 ++
3 files changed, 53 insertions(+)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 46ab7feec6b6..f5549482d0df 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -1,5 +1,6 @@
menuconfig MTD
tristate "Memory Technology Device (MTD) support"
+ imply NVMEM
help
Memory Technology Devices are flash, RAM and similar chips, often
used for solid state file systems on embedded devices. This option
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 42395df06be9..a57302eaceb5 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -488,6 +488,49 @@ int mtd_pairing_groups(struct mtd_info *mtd)
}
EXPORT_SYMBOL_GPL(mtd_pairing_groups);

+static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
+ void *val, size_t bytes)
+{
+ struct mtd_info *mtd = priv;
+ size_t retlen;
+ int err;
+
+ err = mtd_read(mtd, offset, bytes, &retlen, val);
+ if (err && err != -EUCLEAN)
+ return err;
+
+ return retlen == bytes ? 0 : -EIO;
+}
+
+static int mtd_nvmem_add(struct mtd_info *mtd)
+{
+ struct nvmem_config config = { };
+
+ config.dev = &mtd->dev;
+ config.owner = THIS_MODULE;
+ config.name = mtd->name;
+ config.reg_read = mtd_nvmem_reg_read;
+ config.size = mtd->size;
+ config.word_size = 1;
+ config.stride = 1;
+ config.read_only = true;
+ config.root_only = true;
+ config.priv = mtd;
+
+ mtd->nvmem = devm_nvmem_register(&mtd->dev, &config);
+ if (IS_ERR(mtd->nvmem)) {
+ /* Just ignore if there is no NVMEM support in the kernel */
+ if (PTR_ERR(mtd->nvmem) == -ENOSYS) {
+ mtd->nvmem = NULL;
+ } else {
+ dev_err(&mtd->dev, "Failed to register NVMEM device\n");
+ return PTR_ERR(mtd->nvmem);
+ }
+ }
+
+ return 0;
+}
+
static struct dentry *dfs_dir_mtd;

/**
@@ -570,6 +613,11 @@ int add_mtd_device(struct mtd_info *mtd)
if (error)
goto fail_added;

+ /* Add the nvmem provider */
+ error = mtd_nvmem_add(mtd);
+ if (error)
+ goto fail_nvmem_add;
+
if (!IS_ERR_OR_NULL(dfs_dir_mtd)) {
mtd->dbg.dfs_dir = debugfs_create_dir(dev_name(&mtd->dev), dfs_dir_mtd);
if (IS_ERR_OR_NULL(mtd->dbg.dfs_dir)) {
@@ -595,6 +643,8 @@ int add_mtd_device(struct mtd_info *mtd)
__module_get(THIS_MODULE);
return 0;

+fail_nvmem_add:
+ device_unregister(&mtd->dev);
fail_added:
of_node_put(mtd_get_of_node(mtd));
idr_remove(&mtd_idr, i);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a86c4fa93115..8121c6582285 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -25,6 +25,7 @@
#include <linux/notifier.h>
#include <linux/device.h>
#include <linux/of.h>
+#include <linux/nvmem-provider.h>

#include <mtd/mtd-abi.h>

@@ -339,6 +340,7 @@ struct mtd_info {
struct device dev;
int usecount;
struct mtd_debug_info dbg;
+ struct nvmem_device *nvmem;
};

int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
--
2.18.0


2018-08-10 08:11:25

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 09/29] ARM: davinci: dm646x-evm: use nvmem lookup for mac address

From: Bartosz Golaszewski <[email protected]>

We now support nvmem lookups for machine code. Add a lookup for
mac-address.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-dm646x-evm.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index 584064fdabf5..943ab0ca6db8 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -31,6 +31,7 @@
#include <linux/mtd/mtd.h>
#include <linux/mtd/rawnand.h>
#include <linux/mtd/partitions.h>
+#include <linux/nvmem-provider.h>
#include <linux/clk.h>
#include <linux/export.h>
#include <linux/platform_data/gpio-davinci.h>
@@ -310,6 +311,15 @@ static struct pcf857x_platform_data pcf_data = {
* - ... newer boards may have more
*/

+static struct nvmem_cell_lookup dm646x_evm_mac_address_cell = {
+ .info = {
+ .name = "mac-address",
+ .offset = 0x7f00,
+ .bytes = ETH_ALEN,
+ },
+ .nvmem_name = "1-00500",
+};
+
static struct at24_platform_data eeprom_info = {
.byte_len = (256*1024) / 8,
.page_size = 64,
@@ -802,6 +812,8 @@ static __init void evm_init(void)
davinci_init_ide();

soc_info->emac_pdata->phy_id = DM646X_EVM_PHY_ID;
+
+ nvmem_add_lookup_table(&dm646x_evm_mac_address_cell, 1);
}

MACHINE_START(DAVINCI_DM6467_EVM, "DaVinci DM646x EVM")
--
2.18.0


2018-08-10 08:11:39

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 03/29] nvmem: add a notifier chain

From: Bartosz Golaszewski <[email protected]>

Add a blocking notifier chain with two events (add and remove) so that
users can get notified about the addition of nvmem devices they're
waiting for.

We'll use this instead of the at24 setup callback in the mityomapl138
board file.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 20 ++++++++++++++++++++
include/linux/nvmem-consumer.h | 18 ++++++++++++++++++
2 files changed, 38 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 329ea5b8f809..128c8e51bff2 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -65,6 +65,8 @@ static DEFINE_MUTEX(nvmem_cells_mutex);
static LIST_HEAD(nvmem_cell_lookups);
static DEFINE_MUTEX(nvmem_lookup_mutex);

+static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key eeprom_lock_key;
#endif
@@ -479,6 +481,18 @@ static int nvmem_setup_compat(struct nvmem_device *nvmem,
return 0;
}

+int nvmem_register_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&nvmem_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(nvmem_register_notifier);
+
+int nvmem_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&nvmem_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(nvmem_unregister_notifier);
+
/**
* nvmem_register() - Register a nvmem device for given nvmem_config.
* Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
@@ -559,6 +573,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
if (config->cells)
nvmem_add_cells(nvmem, config->cells, config->ncells);

+ rval = blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
+ if (rval)
+ goto err_device_del;
+
return nvmem;

err_device_del:
@@ -586,6 +604,8 @@ int nvmem_unregister(struct nvmem_device *nvmem)
}
mutex_unlock(&nvmem_mutex);

+ blocking_notifier_call_chain(&nvmem_notifier, NVMEM_REMOVE, nvmem);
+
if (nvmem->flags & FLAG_COMPAT)
device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);

diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index f4b5d3186e94..ae4d30347602 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -14,6 +14,7 @@

#include <linux/err.h>
#include <linux/errno.h>
+#include <linux/notifier.h>

struct device;
struct device_node;
@@ -35,6 +36,11 @@ struct nvmem_cell_lookup {
const char *nvmem_name;
};

+enum {
+ NVMEM_ADD = 1,
+ NVMEM_REMOVE,
+};
+
#if IS_ENABLED(CONFIG_NVMEM)

/* Cell based interface */
@@ -61,6 +67,8 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
int nvmem_device_cell_write(struct nvmem_device *nvmem,
struct nvmem_cell_info *info, void *buf);

+int nvmem_register_notifier(struct notifier_block *nb);
+int nvmem_unregister_notifier(struct notifier_block *nb);
#else

static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
@@ -149,6 +157,16 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
{
return -ENOSYS;
}
+
+static inline int nvmem_register_notifier(struct notifier_block *nb)
+{
+ return -ENOSYS;
+}
+
+static inline int int nvmem_unregister_notifier(struct notifier_block *nb)
+{
+ return -ENOSYS;
+}
#endif /* CONFIG_NVMEM */

#if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
--
2.18.0


2018-08-10 08:11:45

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 02/29] Documentation: nvmem: document lookup entries

From: Bartosz Golaszewski <[email protected]>

Describe the usage of nvmem cell lookup tables.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
Documentation/nvmem/nvmem.txt | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt
index 8d8d8f58f96f..9d5e3ca2b4f3 100644
--- a/Documentation/nvmem/nvmem.txt
+++ b/Documentation/nvmem/nvmem.txt
@@ -58,6 +58,34 @@ static int qfprom_probe(struct platform_device *pdev)
It is mandatory that the NVMEM provider has a regmap associated with its
struct device. Failure to do would return error code from nvmem_register().

+Additionally it is possible to create nvmem cell lookup entries and register
+them with the nvmem framework from machine code as shown in the example below:
+
+static struct nvmem_cell_lookup foobar_lookup = {
+ .info = {
+ .name = "mac-address",
+ .offset = 0xd000,
+ .bytes = ERH_ALEN,
+ },
+ .nvmem_name = "foobar",
+};
+
+static void foobar_register(void)
+{
+ ...
+ nvmem_add_lookup_table(&foobar_lookup, 1);
+ ...
+}
+
+A lookup entry table can be later removed if needed:
+
+static void foobar_fini(void)
+{
+ ...
+ nvmem_del_lookup_table(&foobar_lookup, 1);
+ ...
+}
+
NVMEM Consumers
+++++++++++++++

--
2.18.0


2018-08-10 08:11:46

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 05/29] nvmem: remove the name field from struct nvmem_device

From: Bartosz Golaszewski <[email protected]>

This field is never set and is only used in a single error message.
Remove the field and use nvmem_dev_name() instead.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 31df2e6d6f72..ab3ced2d9a84 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -26,7 +26,6 @@
#include <linux/slab.h>

struct nvmem_device {
- const char *name;
struct module *owner;
struct device dev;
int stride;
@@ -712,7 +711,7 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np,
if (!try_module_get(nvmem->owner)) {
dev_err(&nvmem->dev,
"could not increase module refcount for cell %s\n",
- nvmem->name);
+ nvmem_dev_name(nvmem));

mutex_lock(&nvmem_mutex);
nvmem->users--;
--
2.18.0


2018-08-10 08:11:55

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 01/29] nvmem: add support for cell lookups

From: Bartosz Golaszewski <[email protected]>

We can currently only register nvmem cells from device tree or by
manually calling nvmem_add_cells(). The latter options however forces
users to make sure that the nvmem provider with which the cells are
associated is registered before the call.

This patch proposes a new solution inspired by other frameworks that
offer resource lookups (GPIO, PWM etc.). It adds functions that allow
machine code to register nvmem lookup which are later lazily used to
add corresponding nvmem cells and remove them if no longer needed.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Acked-by: Srinivas Kandagatla <[email protected]>
---
drivers/nvmem/core.c | 77 +++++++++++++++++++++++++++++++++-
include/linux/nvmem-consumer.h | 6 +++
include/linux/nvmem-provider.h | 10 +++++
3 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 514d1dfc5630..329ea5b8f809 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida);
static LIST_HEAD(nvmem_cells);
static DEFINE_MUTEX(nvmem_cells_mutex);

+static LIST_HEAD(nvmem_cell_lookups);
+static DEFINE_MUTEX(nvmem_lookup_mutex);
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key eeprom_lock_key;
#endif
@@ -247,6 +250,41 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
NULL,
};

+/**
+ * nvmem_add_lookup_table() - register a number of nvmem cell lookup entries
+ *
+ * @lookup: array of nvmem cell lookup entries
+ * @nentries: number of lookup entries in the array
+ */
+void nvmem_add_lookup_table(struct nvmem_cell_lookup *lookup, size_t nentries)
+{
+ int i;
+
+ mutex_lock(&nvmem_lookup_mutex);
+ for (i = 0; i < nentries; i++)
+ list_add_tail(&lookup[i].list, &nvmem_cell_lookups);
+ mutex_unlock(&nvmem_lookup_mutex);
+}
+EXPORT_SYMBOL_GPL(nvmem_add_lookup_table);
+
+/**
+ * nvmem_del_lookup_table() - unregister a set of previously added nvmem cell
+ * lookup entries
+ *
+ * @lookup: array of nvmem cell lookup entries
+ * @nentries: number of lookup entries in the array
+ */
+void nvmem_del_lookup_table(struct nvmem_cell_lookup *lookup, size_t nentries)
+{
+ int i;
+
+ mutex_lock(&nvmem_lookup_mutex);
+ for (i = 0; i < nentries; i++)
+ list_del(&lookup[i].list);
+ mutex_unlock(&nvmem_lookup_mutex);
+}
+EXPORT_SYMBOL_GPL(nvmem_del_lookup_table);
+
static void nvmem_release(struct device *dev)
{
struct nvmem_device *nvmem = to_nvmem_device(dev);
@@ -916,6 +954,39 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
#endif

+static struct nvmem_cell *nvmem_cell_from_lookup(const char *cell_id)
+{
+ struct nvmem_cell *cell = ERR_PTR(-ENOENT);
+ struct nvmem_cell_lookup *lookup;
+ struct nvmem_device *nvmem;
+ int rc;
+
+ mutex_lock(&nvmem_lookup_mutex);
+
+ list_for_each_entry(lookup, &nvmem_cell_lookups, list) {
+ if (strcmp(cell_id, lookup->info.name) == 0) {
+ nvmem = nvmem_find(lookup->nvmem_name);
+ if (!nvmem) {
+ cell = ERR_PTR(-EPROBE_DEFER);
+ goto out;
+ }
+
+ rc = nvmem_add_cells(nvmem, &lookup->info, 1);
+ if (rc) {
+ cell = ERR_PTR(rc);
+ goto out;
+ }
+
+ cell = nvmem_cell_get_from_list(cell_id);
+ break;
+ }
+ }
+
+out:
+ mutex_unlock(&nvmem_lookup_mutex);
+ return cell;
+}
+
/**
* nvmem_cell_get() - Get nvmem cell of device form a given cell name
*
@@ -940,7 +1011,11 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *cell_id)
if (!cell_id)
return ERR_PTR(-EINVAL);

- return nvmem_cell_get_from_list(cell_id);
+ cell = nvmem_cell_get_from_list(cell_id);
+ if (!IS_ERR(cell) || PTR_ERR(cell) == -EPROBE_DEFER)
+ return cell;
+
+ return nvmem_cell_from_lookup(cell_id);
}
EXPORT_SYMBOL_GPL(nvmem_cell_get);

diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 4e85447f7860..f4b5d3186e94 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -29,6 +29,12 @@ struct nvmem_cell_info {
unsigned int nbits;
};

+struct nvmem_cell_lookup {
+ struct nvmem_cell_info info;
+ struct list_head list;
+ const char *nvmem_name;
+};
+
#if IS_ENABLED(CONFIG_NVMEM)

/* Cell based interface */
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 24def6ad09bb..6a17d722062b 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -17,6 +17,7 @@

struct nvmem_device;
struct nvmem_cell_info;
+struct nvmem_cell_lookup;
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,
@@ -72,6 +73,9 @@ struct nvmem_config {
struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
int nvmem_unregister(struct nvmem_device *nvmem);

+void nvmem_add_lookup_table(struct nvmem_cell_lookup *lookup, size_t nentries);
+void nvmem_del_lookup_table(struct nvmem_cell_lookup *lookup, size_t nentries);
+
struct nvmem_device *devm_nvmem_register(struct device *dev,
const struct nvmem_config *cfg);

@@ -92,6 +96,12 @@ static inline int nvmem_unregister(struct nvmem_device *nvmem)
return -ENOSYS;
}

+static inline void
+nvmem_add_lookup_table(struct nvmem_cell_lookup *lookup, size_t nentries) {}
+
+static inline void
+nvmem_del_lookup_table(struct nvmem_cell_lookup *lookup, size_t nentries) {}
+
static inline struct nvmem_device *
devm_nvmem_register(struct device *dev, const struct nvmem_config *c)
{
--
2.18.0


2018-08-10 08:12:27

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 04/29] nvmem: provide nvmem_dev_name()



On 10/08/18 09:05, Bartosz Golaszewski wrote:
>
> +const char *nvmem_dev_name(struct nvmem_device *nvmem)
> +{
> + return dev_name(&nvmem->dev);
> +}
> +EXPORT_SYMBOL_GPL(nvmem_dev_name);
> +
Kernel doc for this is missing!
Other than that it looks good to me!

--srini

2018-08-10 08:12:46

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 04/29] nvmem: provide nvmem_dev_name()

From: Bartosz Golaszewski <[email protected]>

Kernel users don't have any means of checking the names of nvmem
devices. Add a routine that returns the name of the nvmem provider.

This will be useful for nvmem notifier subscribers - otherwise they
can't check what device is being added/removed.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 6 ++++++
include/linux/nvmem-consumer.h | 7 +++++++
2 files changed, 13 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 128c8e51bff2..31df2e6d6f72 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1440,6 +1440,12 @@ int nvmem_device_write(struct nvmem_device *nvmem,
}
EXPORT_SYMBOL_GPL(nvmem_device_write);

+const char *nvmem_dev_name(struct nvmem_device *nvmem)
+{
+ return dev_name(&nvmem->dev);
+}
+EXPORT_SYMBOL_GPL(nvmem_dev_name);
+
static int __init nvmem_init(void)
{
return bus_register(&nvmem_bus_type);
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index ae4d30347602..14bb86a7a78d 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -69,6 +69,8 @@ int nvmem_device_cell_write(struct nvmem_device *nvmem,

int nvmem_register_notifier(struct notifier_block *nb);
int nvmem_unregister_notifier(struct notifier_block *nb);
+
+const char *nvmem_dev_name(struct nvmem_device *nvmem);
#else

static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
@@ -167,6 +169,11 @@ static inline int int nvmem_unregister_notifier(struct notifier_block *nb)
{
return -ENOSYS;
}
+
+static inline const char *nvmem_dev_name(struct nvmem_device *nvmem)
+{
+ return NULL;
+}
#endif /* CONFIG_NVMEM */

#if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
--
2.18.0


2018-08-10 08:34:10

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 19/29] ARM: davinci: mityomapl138: don't read the MAC address from machine code

From: Bartosz Golaszewski <[email protected]>

This is now done by the emac driver using a registered nvmem cell.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-mityomapl138.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index 48a9bae76e4a..35b2b6068a7a 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -120,7 +120,6 @@ static void read_factory_config(struct nvmem_device *nvmem, void *context)
{
int ret;
const char *partnum = NULL;
- struct davinci_soc_info *soc_info = &davinci_soc_info;

if (!IS_BUILTIN(CONFIG_NVMEM)) {
pr_warn("Factory Config not available without CONFIG_NVMEM\n");
@@ -146,13 +145,6 @@ static void read_factory_config(struct nvmem_device *nvmem, void *context)
goto bad_config;
}

- pr_info("Found MAC = %pM\n", factory_config.mac);
- if (is_valid_ether_addr(factory_config.mac))
- memcpy(soc_info->emac_pdata->mac_addr,
- factory_config.mac, ETH_ALEN);
- else
- pr_warn("Invalid MAC found in factory config block\n");
-
partnum = factory_config.partnum;
pr_info("Part Number = %s\n", partnum);

--
2.18.0


2018-08-10 08:34:10

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 20/29] ARM: davinci: dm365-evm: use device properties for at24 eeprom

From: Bartosz Golaszewski <[email protected]>

We want to work towards phasing out the at24_platform_data structure.
There are few users and its contents can be represented using generic
device properties. Using device properties only will allow us to
significantly simplify the at24 configuration code.

Remove the at24_platform_data structure and replace it with an array
of property entries. Drop the byte_len/size property, as the model name
already implies the EEPROM's size.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
arch/arm/mach-davinci/board-dm365-evm.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c
index cf3f2e611228..2447e08dd68d 100644
--- a/arch/arm/mach-davinci/board-dm365-evm.c
+++ b/arch/arm/mach-davinci/board-dm365-evm.c
@@ -18,7 +18,7 @@
#include <linux/i2c.h>
#include <linux/io.h>
#include <linux/clk.h>
-#include <linux/platform_data/at24.h>
+#include <linux/property.h>
#include <linux/leds.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
@@ -179,18 +179,15 @@ static struct nvmem_cell_lookup dm365evm_mac_address_cell = {
.nvmem_name = "1-00500",
};

-static struct at24_platform_data eeprom_info = {
- .byte_len = (256*1024) / 8,
- .page_size = 64,
- .flags = AT24_FLAG_ADDR16,
- .setup = davinci_get_mac_addr,
- .context = (void *)0x7f00,
+static const struct property_entry eeprom_properties[] = {
+ PROPERTY_ENTRY_U32("pagesize", 64),
+ { }
};

static struct i2c_board_info i2c_info[] = {
{
I2C_BOARD_INFO("24c256", 0x50),
- .platform_data = &eeprom_info,
+ .properties = eeprom_properties,
},
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
--
2.18.0


2018-08-10 08:35:47

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 03/29] nvmem: add a notifier chain



On 10/08/18 09:05, Bartosz Golaszewski wrote:
> +int nvmem_register_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(&nvmem_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(nvmem_register_notifier);
> +
> +int nvmem_unregister_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_unregister(&nvmem_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(nvmem_unregister_notifier);
> +
Kerneldoc is missing for these both exported symbols too!

--srini

2018-08-10 08:37:48

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 05/29] nvmem: remove the name field from struct nvmem_device



On 10/08/18 09:05, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> This field is never set and is only used in a single error message.
> Remove the field and use nvmem_dev_name() instead.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
Looks good to me!

> drivers/nvmem/core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 31df2e6d6f72..ab3ced2d9a84 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -26,7 +26,6 @@
> #include <linux/slab.h>
>
> struct nvmem_device {
> - const char *name;
> struct module *owner;
> struct device dev;
> int stride;
> @@ -712,7 +711,7 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np,
> if (!try_module_get(nvmem->owner)) {
> dev_err(&nvmem->dev,
> "could not increase module refcount for cell %s\n",
> - nvmem->name);
> + nvmem_dev_name(nvmem));
>
> mutex_lock(&nvmem_mutex);
> nvmem->users--;
>

2018-08-10 09:00:22

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 07/29] ARM: davinci: dm365-evm: use nvmem lookup for mac address

From: Bartosz Golaszewski <[email protected]>

We now support nvmem lookups for machine code. Add a lookup for
mac-address.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-dm365-evm.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c
index 435f7ec7d9af..cf3f2e611228 100644
--- a/arch/arm/mach-davinci/board-dm365-evm.c
+++ b/arch/arm/mach-davinci/board-dm365-evm.c
@@ -22,6 +22,7 @@
#include <linux/leds.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
+#include <linux/nvmem-provider.h>
#include <linux/slab.h>
#include <linux/mtd/rawnand.h>
#include <linux/input.h>
@@ -169,6 +170,15 @@ static struct platform_device davinci_nand_device = {
},
};

+static struct nvmem_cell_lookup dm365evm_mac_address_cell = {
+ .info = {
+ .name = "mac-address",
+ .offset = 0x7f00,
+ .bytes = ETH_ALEN,
+ },
+ .nvmem_name = "1-00500",
+};
+
static struct at24_platform_data eeprom_info = {
.byte_len = (256*1024) / 8,
.page_size = 64,
@@ -769,6 +779,8 @@ static __init void dm365_evm_init(void)

dm365_init_spi0(BIT(0), dm365_evm_spi_info,
ARRAY_SIZE(dm365_evm_spi_info));
+
+ nvmem_add_lookup_table(&dm365evm_mac_address_cell, 1);
}

MACHINE_START(DAVINCI_DM365_EVM, "DaVinci DM365 EVM")
--
2.18.0


2018-08-10 09:00:33

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 13/29] ARM: davinci: da850-evm: remove unnecessary include

From: Bartosz Golaszewski <[email protected]>

The include file for at24_platform_data is not needed in this file.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-da850-evm.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 5a634a04ec69..521a26b5c20d 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -20,7 +20,6 @@
#include <linux/kernel.h>
#include <linux/leds.h>
#include <linux/i2c.h>
-#include <linux/platform_data/at24.h>
#include <linux/platform_data/pca953x.h>
#include <linux/input.h>
#include <linux/input/tps6507x-ts.h>
--
2.18.0


2018-08-10 09:00:37

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 15/29] net: split eth_platform_get_mac_address() into subroutines

From: Bartosz Golaszewski <[email protected]>

We want do add more sources from which to read the MAC address. In
order to avoid bloating this function too much, start by splitting it
into subroutines, each of which takes care of reading the MAC from
one source.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
net/ethernet/eth.c | 44 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 7f08105402c8..d01dd55de037 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -525,22 +525,50 @@ unsigned char * __weak arch_get_platform_mac_address(void)
return NULL;
}

-int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
+static int mac_address_from_of(struct device *dev, u8 *mac_addr)
{
const unsigned char *addr;
- struct device_node *dp;
+ struct device_node *np;

- dp = dev->of_node;
- addr = NULL;
- if (dp)
- addr = of_get_mac_address(dp);
- if (!addr)
- addr = arch_get_platform_mac_address();
+ np = dev->of_node;
+ if (!np)
+ return -ENODEV;

+ addr = of_get_mac_address(np);
if (!addr)
return -ENODEV;

+ if (!addr || !is_valid_ether_addr(addr))
+ return -ENODEV;
+
ether_addr_copy(mac_addr, addr);
return 0;
}
+
+static int mac_address_from_arch(u8 *mac_addr)
+{
+ const unsigned char *addr;
+
+ addr = arch_get_platform_mac_address();
+ if (!addr || !is_valid_ether_addr(addr))
+ return -ENODEV;
+
+ ether_addr_copy(mac_addr, addr);
+ return 0;
+}
+
+int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
+{
+ int rv;
+
+ rv = mac_address_from_of(dev, mac_addr);
+ if (!rv)
+ return 0;
+
+ rv = mac_address_from_arch(mac_addr);
+ if (!rv)
+ return 0;
+
+ return -ENODEV;
+}
EXPORT_SYMBOL(eth_platform_get_mac_address);
--
2.18.0


2018-08-10 09:00:47

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 16/29] net: add support for nvmem to eth_platform_get_mac_address()

From: Bartosz Golaszewski <[email protected]>

Many non-DT platforms read the MAC address from EEPROM. Usually it's
either done with callbacks defined in board files or from SoC-specific
ethernet drivers.

In order to generalize this, try to read the MAC from nvmem in
eth_platform_get_mac_address() using a standard lookup name:
"mac-address".

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
net/ethernet/eth.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index d01dd55de037..f6f6af05fb58 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -54,6 +54,7 @@
#include <linux/if_ether.h>
#include <linux/of_net.h>
#include <linux/pci.h>
+#include <linux/nvmem-consumer.h>
#include <net/dst.h>
#include <net/arp.h>
#include <net/sock.h>
@@ -557,6 +558,39 @@ static int mac_address_from_arch(u8 *mac_addr)
return 0;
}

+static int mac_address_from_nvmem(struct device *dev, u8 *mac_addr)
+{
+ const unsigned char *addr;
+ struct nvmem_cell *cell;
+ size_t alen;
+ int rv = 0;
+
+ cell = nvmem_cell_get(dev, "mac-address");
+ if (IS_ERR(cell))
+ return PTR_ERR(cell);
+
+ addr = nvmem_cell_read(cell, &alen);
+ if (IS_ERR(addr)) {
+ rv = PTR_ERR(addr);
+ goto put_nvmem;
+ }
+
+ if (alen != ETH_ALEN || !is_valid_ether_addr(addr)) {
+ rv = -ENODEV;
+ goto free_addr;
+ }
+
+ ether_addr_copy(mac_addr, addr);
+
+free_addr:
+ kfree(addr);
+
+put_nvmem:
+ nvmem_cell_put(cell);
+
+ return rv;
+}
+
int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
{
int rv;
@@ -569,6 +603,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
if (!rv)
return 0;

+ rv = mac_address_from_nvmem(dev, mac_addr);
+ if (!rv)
+ return 0;
+
return -ENODEV;
}
EXPORT_SYMBOL(eth_platform_get_mac_address);
--
2.18.0


2018-08-10 09:00:52

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 18/29] ARM: davinci: da850-evm: remove dead MTD code

From: Bartosz Golaszewski <[email protected]>

We no longer need to register the MTD notifier to read the MAC address
as it's now being done in the emac driver.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-da850-evm.c | 28 -------------------------
1 file changed, 28 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 521a26b5c20d..1df796b67b82 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -137,32 +137,6 @@ static struct spi_board_info da850evm_spi_info[] = {
},
};

-#ifdef CONFIG_MTD
-static void da850_evm_m25p80_notify_add(struct mtd_info *mtd)
-{
- char *mac_addr = davinci_soc_info.emac_pdata->mac_addr;
- size_t retlen;
-
- if (!strcmp(mtd->name, "MAC-Address")) {
- mtd_read(mtd, 0, ETH_ALEN, &retlen, mac_addr);
- if (retlen == ETH_ALEN)
- pr_info("Read MAC addr from SPI Flash: %pM\n",
- mac_addr);
- }
-}
-
-static struct mtd_notifier da850evm_spi_notifier = {
- .add = da850_evm_m25p80_notify_add,
-};
-
-static void da850_evm_setup_mac_addr(void)
-{
- register_mtd_user(&da850evm_spi_notifier);
-}
-#else
-static void da850_evm_setup_mac_addr(void) { }
-#endif
-
static struct mtd_partition da850_evm_norflash_partition[] = {
{
.name = "bootloaders + env",
@@ -1470,8 +1444,6 @@ static __init void da850_evm_init(void)
if (ret)
pr_warn("%s: SATA registration failed: %d\n", __func__, ret);

- da850_evm_setup_mac_addr();
-
ret = da8xx_register_rproc();
if (ret)
pr_warn("%s: dsp/rproc registration failed: %d\n",
--
2.18.0


2018-08-10 09:00:56

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 22/29] ARM: davinci: dm644x-evm: use device properties for at24 eeprom

From: Bartosz Golaszewski <[email protected]>

We want to work towards phasing out the at24_platform_data structure.
There are few users and its contents can be represented using generic
device properties. Using device properties only will allow us to
significantly simplify the at24 configuration code.

Remove the at24_platform_data structure and replace it with an array
of property entries. Drop the byte_len/size property, as the model name
already implies the EEPROM's size.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
arch/arm/mach-davinci/board-dm644x-evm.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index 829fa1c8d0b4..954296d23297 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -16,8 +16,8 @@
#include <linux/gpio/machine.h>
#include <linux/i2c.h>
#include <linux/platform_data/pcf857x.h>
-#include <linux/platform_data/at24.h>
#include <linux/platform_data/gpio-davinci.h>
+#include <linux/property.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/rawnand.h>
#include <linux/mtd/partitions.h>
@@ -486,12 +486,9 @@ static struct nvmem_cell_lookup dm6446evm_mac_address_cell = {
.nvmem_name = "1-00500",
};

-static struct at24_platform_data eeprom_info = {
- .byte_len = (256*1024) / 8,
- .page_size = 64,
- .flags = AT24_FLAG_ADDR16,
- .setup = davinci_get_mac_addr,
- .context = (void *)0x7f00,
+static const struct property_entry eeprom_properties[] = {
+ PROPERTY_ENTRY_U32("pagesize", 64),
+ { }
};

/*
@@ -601,7 +598,7 @@ static struct i2c_board_info __initdata i2c_info[] = {
},
{
I2C_BOARD_INFO("24c256", 0x50),
- .platform_data = &eeprom_info,
+ .properties = eeprom_properties,
},
{
I2C_BOARD_INFO("tlv320aic33", 0x1b),
--
2.18.0


2018-08-10 09:01:21

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 14/29] net: simplify eth_platform_get_mac_address()

From: Bartosz Golaszewski <[email protected]>

We don't need to use pci_device_to_OF_node() - we can retrieve
dev->of_node directly even for pci devices.

Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
net/ethernet/eth.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index ee28440f57c5..7f08105402c8 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -530,11 +530,7 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
const unsigned char *addr;
struct device_node *dp;

- if (dev_is_pci(dev))
- dp = pci_device_to_OF_node(to_pci_dev(dev));
- else
- dp = dev->of_node;
-
+ dp = dev->of_node;
addr = NULL;
if (dp)
addr = of_get_mac_address(dp);
--
2.18.0


2018-08-10 09:01:35

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 11/29] ARM: davinci: mityomapl138: add nvmem cells lookup entries

From: Bartosz Golaszewski <[email protected]>

We now support nvmem lookups for machine code. Add a lookup for
mac-address.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-mityomapl138.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index 37b3e48a21d1..48a9bae76e4a 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -15,6 +15,7 @@
#include <linux/console.h>
#include <linux/platform_device.h>
#include <linux/mtd/partitions.h>
+#include <linux/nvmem-provider.h>
#include <linux/regulator/machine.h>
#include <linux/i2c.h>
#include <linux/platform_data/at24.h>
@@ -160,6 +161,25 @@ static void read_factory_config(struct nvmem_device *nvmem, void *context)
mityomapl138_cpufreq_init(partnum);
}

+static struct nvmem_cell_lookup mityomapl138_nvmem_cells[] = {
+ {
+ .info = {
+ .name = "factory-config",
+ .offset = 0x0,
+ .bytes = sizeof(struct factory_config),
+ },
+ .nvmem_name = "1-00500",
+ },
+ {
+ .info = {
+ .name = "mac-address",
+ .offset = 0x64,
+ .bytes = ETH_ALEN,
+ },
+ .nvmem_name = "1-00500",
+ }
+};
+
static struct at24_platform_data mityomapl138_fd_chip = {
.byte_len = 256,
.page_size = 8,
@@ -534,6 +554,8 @@ static void __init mityomapl138_init(void)
if (ret)
pr_warn("spi 1 registration failed: %d\n", ret);

+ nvmem_add_lookup_table(mityomapl138_nvmem_cells,
+ ARRAY_SIZE(mityomapl138_nvmem_cells));
mityomapl138_config_emac();

ret = da8xx_register_rtc();
--
2.18.0


2018-08-10 09:21:06

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v2 12/29] ARM: davinci: da850-evm: use nvmem lookup for mac address

From: Bartosz Golaszewski <[email protected]>

We now support nvmem lookups for machine code. Add a lookup for
mac-address.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm/mach-davinci/board-da850-evm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 6d5beb11bd96..5a634a04ec69 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -29,6 +29,7 @@
#include <linux/mtd/rawnand.h>
#include <linux/mtd/partitions.h>
#include <linux/mtd/physmap.h>
+#include <linux/nvmem-provider.h>
#include <linux/platform_device.h>
#include <linux/platform_data/gpio-davinci.h>
#include <linux/platform_data/mtd-davinci.h>
@@ -99,6 +100,19 @@ static struct mtd_partition da850evm_spiflash_part[] = {
},
};

+static struct nvmem_cell_lookup da850evm_mac_address_cell = {
+ .info = {
+ .name = "mac-address",
+ .offset = 0x0,
+ .bytes = ETH_ALEN,
+ },
+ /*
+ * The nvmem name differs from the partition name because of the
+ * internal works of the nvmem framework.
+ */
+ .nvmem_name = "MAC-Address0",
+};
+
static struct flash_platform_data da850evm_spiflash_data = {
.name = "m25p80",
.parts = da850evm_spiflash_part,
@@ -1447,6 +1461,8 @@ static __init void da850_evm_init(void)
pr_warn("%s: spi info registration failed: %d\n", __func__,
ret);

+ nvmem_add_lookup_table(&da850evm_mac_address_cell, 1);
+
ret = da8xx_register_spi_bus(1, ARRAY_SIZE(da850evm_spi_info));
if (ret)
pr_warn("%s: SPI 1 registration failed: %d\n", __func__, ret);
--
2.18.0


2018-08-10 09:37:33

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data



On 10/08/18 09:04, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> This is a follow-up to the previously rejected series[1] which partially
> removed the at24_platform_data structure. After further development and
> taking reviews into account, this series finally removes that struct
> completely but not without touching many different parts of the code
> base.
>
> Since I took over maintainership of the at24 driver I've been working
> towards removing at24_platform_data in favor for device properties.
>
> DaVinci is the only platform that's still using it - all other users
> have already been converted.
>
> One of the obstacles in case of DaVinci is removing the setup() callback
> from the pdata struct, the only user of which are some davinci boards.
>
> Most boards use the EEPROM to store the MAC address. This series adds
> support for cell lookups to the nvmem framework, registers relevant
> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> converts davinci_emac driver to using it and replaces at24_platform_data
> with device properties.
>
> There's also one board (da850-evm) which uses MTD for reading the MAC
> address. I used the patch from Alban Bedel's previous submission[2] to
> add support for nvmem to the MTD framework. Since this user doesn't
> need device tree, I dropped Alban's patches modifying the DT bindings.
> We can add that later once an agreement is reached. For the time being
> MTD devices are registered as nvmem devices and we're registering the
> mac-address cell using the cell lookup mechanism.
>
> This series adds a blocking notifier chain to the nvmem framework, so
> that we can keep the EEPROM reading code in the mityomapl138 board file
> with only slight modifications.
>
> I also included some minor fixes to the modified code.
>
> Tested on da850-evm & dm365-evm.
>
> [1] https://lkml.org/lkml/2018/6/29/153
> [2] https://lkml.org/lkml/2018/3/24/312
>
...

> Bartosz Golaszewski (28):
> nvmem: add support for cell lookups
> Documentation: nvmem: document lookup entries
> nvmem: add a notifier chain
> nvmem: provide nvmem_dev_name()
> nvmem: remove the name field from struct nvmem_device
>
> Documentation/nvmem/nvmem.txt | 28 +++++
..
> drivers/nvmem/core.c | 106 ++++++++++++++++-

nvmem parts looks good to me other then few trivial kernel doc comments!
I can either Ack those patches or Send them after rc3-4 to Greg KH as
4.20 material.

Thanks
srini

>

2018-08-10 14:54:17

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 14/29] net: simplify eth_platform_get_mac_address()

On Fri, Aug 10, 2018 at 11:05 AM, Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> We don't need to use pci_device_to_OF_node() - we can retrieve
> dev->of_node directly even for pci devices.
>

> struct device_node *dp;
>
> - if (dev_is_pci(dev))
> - dp = pci_device_to_OF_node(to_pci_dev(dev));
> - else
> - dp = dev->of_node;
> -
> + dp = dev->of_node;
> addr = NULL;
> if (dp)
> addr = of_get_mac_address(dp);

Looking more at this I could even propose to change all above by

addr = device_get_mac_address(dev, mac_addr, ETH_ALEN);

Thoughts?

--
With Best Regards,
Andy Shevchenko

2018-08-10 16:18:55

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 14/29] net: simplify eth_platform_get_mac_address()

2018-08-10 16:39 GMT+02:00 Andy Shevchenko <[email protected]>:
> On Fri, Aug 10, 2018 at 11:05 AM, Bartosz Golaszewski <[email protected]> wrote:
>> From: Bartosz Golaszewski <[email protected]>
>>
>> We don't need to use pci_device_to_OF_node() - we can retrieve
>> dev->of_node directly even for pci devices.
>>
>
>> struct device_node *dp;
>>
>> - if (dev_is_pci(dev))
>> - dp = pci_device_to_OF_node(to_pci_dev(dev));
>> - else
>> - dp = dev->of_node;
>> -
>> + dp = dev->of_node;
>> addr = NULL;
>> if (dp)
>> addr = of_get_mac_address(dp);
>
> Looking more at this I could even propose to change all above by
>
> addr = device_get_mac_address(dev, mac_addr, ETH_ALEN);
>
> Thoughts?
>
> --
> With Best Regards,
> Andy Shevchenko

Indeed seems like it's even more generalized. Thanks for spotting that.

Bart

2018-08-17 16:28:58

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

Hi Bartosz,

On Fri, 10 Aug 2018 10:05:03 +0200
Bartosz Golaszewski <[email protected]> wrote:

> From: Alban Bedel <[email protected]>
>
> Allow drivers that use the nvmem API to read data stored on MTD devices.
> For this the mtd devices are registered as read-only NVMEM providers.
>
> Signed-off-by: Alban Bedel <[email protected]>
> [Bartosz:
> - use the managed variant of nvmem_register(),
> - set the nvmem name]
> Signed-off-by: Bartosz Golaszewski <[email protected]>

What happened to the 2 other patches of Alban's series? I'd really like
the DT case to be handled/agreed on in the same patchset, but IIRC,
Alban and Srinivas disagreed on how this should be represented. I
hope this time we'll come to an agreement, because the MTD <-> NVMEM
glue has been floating around for quite some time...

Regards,

Boris

> ---
> drivers/mtd/Kconfig | 1 +
> drivers/mtd/mtdcore.c | 50 +++++++++++++++++++++++++++++++++++++++++
> include/linux/mtd/mtd.h | 2 ++
> 3 files changed, 53 insertions(+)
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 46ab7feec6b6..f5549482d0df 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -1,5 +1,6 @@
> menuconfig MTD
> tristate "Memory Technology Device (MTD) support"
> + imply NVMEM
> help
> Memory Technology Devices are flash, RAM and similar chips, often
> used for solid state file systems on embedded devices. This option
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 42395df06be9..a57302eaceb5 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -488,6 +488,49 @@ int mtd_pairing_groups(struct mtd_info *mtd)
> }
> EXPORT_SYMBOL_GPL(mtd_pairing_groups);
>
> +static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
> + void *val, size_t bytes)
> +{
> + struct mtd_info *mtd = priv;
> + size_t retlen;
> + int err;
> +
> + err = mtd_read(mtd, offset, bytes, &retlen, val);
> + if (err && err != -EUCLEAN)
> + return err;
> +
> + return retlen == bytes ? 0 : -EIO;
> +}
> +
> +static int mtd_nvmem_add(struct mtd_info *mtd)
> +{
> + struct nvmem_config config = { };
> +
> + config.dev = &mtd->dev;
> + config.owner = THIS_MODULE;
> + config.name = mtd->name;
> + config.reg_read = mtd_nvmem_reg_read;
> + config.size = mtd->size;
> + config.word_size = 1;
> + config.stride = 1;
> + config.read_only = true;
> + config.root_only = true;
> + config.priv = mtd;
> +
> + mtd->nvmem = devm_nvmem_register(&mtd->dev, &config);
> + if (IS_ERR(mtd->nvmem)) {
> + /* Just ignore if there is no NVMEM support in the kernel */
> + if (PTR_ERR(mtd->nvmem) == -ENOSYS) {
> + mtd->nvmem = NULL;
> + } else {
> + dev_err(&mtd->dev, "Failed to register NVMEM device\n");
> + return PTR_ERR(mtd->nvmem);
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct dentry *dfs_dir_mtd;
>
> /**
> @@ -570,6 +613,11 @@ int add_mtd_device(struct mtd_info *mtd)
> if (error)
> goto fail_added;
>
> + /* Add the nvmem provider */
> + error = mtd_nvmem_add(mtd);
> + if (error)
> + goto fail_nvmem_add;
> +
> if (!IS_ERR_OR_NULL(dfs_dir_mtd)) {
> mtd->dbg.dfs_dir = debugfs_create_dir(dev_name(&mtd->dev), dfs_dir_mtd);
> if (IS_ERR_OR_NULL(mtd->dbg.dfs_dir)) {
> @@ -595,6 +643,8 @@ int add_mtd_device(struct mtd_info *mtd)
> __module_get(THIS_MODULE);
> return 0;
>
> +fail_nvmem_add:
> + device_unregister(&mtd->dev);
> fail_added:
> of_node_put(mtd_get_of_node(mtd));
> idr_remove(&mtd_idr, i);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index a86c4fa93115..8121c6582285 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -25,6 +25,7 @@
> #include <linux/notifier.h>
> #include <linux/device.h>
> #include <linux/of.h>
> +#include <linux/nvmem-provider.h>
>
> #include <mtd/mtd-abi.h>
>
> @@ -339,6 +340,7 @@ struct mtd_info {
> struct device dev;
> int usecount;
> struct mtd_debug_info dbg;
> + struct nvmem_device *nvmem;
> };
>
> int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,


2018-08-19 11:34:26

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Fri, 17 Aug 2018 18:27:20 +0200
Boris Brezillon <[email protected]> wrote:

> Hi Bartosz,
>
> On Fri, 10 Aug 2018 10:05:03 +0200
> Bartosz Golaszewski <[email protected]> wrote:
>
> > From: Alban Bedel <[email protected]>
> >
> > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > For this the mtd devices are registered as read-only NVMEM providers.
> >
> > Signed-off-by: Alban Bedel <[email protected]>
> > [Bartosz:
> > - use the managed variant of nvmem_register(),
> > - set the nvmem name]
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> What happened to the 2 other patches of Alban's series? I'd really
> like the DT case to be handled/agreed on in the same patchset, but
> IIRC, Alban and Srinivas disagreed on how this should be represented.
> I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> glue has been floating around for quite some time...

These other patches were to fix what I consider a fundamental flaw in
the generic NVMEM bindings, however we couldn't agree on this point.
Bartosz later contacted me to take over this series and I suggested to
just change the MTD NVMEM binding to use a compatible string on the
NVMEM cells as an alternative solution to fix the clash with the old
style MTD partition.

However all this has no impact on the code needed to add NVMEM support
to MTD, so the above patch didn't change at all.

Alban


Attachments:
(No filename) (836.00 B)
OpenPGP digital signature

2018-08-19 16:47:35

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Sun, 19 Aug 2018 13:31:06 +0200
Alban <[email protected]> wrote:

> On Fri, 17 Aug 2018 18:27:20 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > Hi Bartosz,
> >
> > On Fri, 10 Aug 2018 10:05:03 +0200
> > Bartosz Golaszewski <[email protected]> wrote:
> >
> > > From: Alban Bedel <[email protected]>
> > >
> > > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > > For this the mtd devices are registered as read-only NVMEM providers.
> > >
> > > Signed-off-by: Alban Bedel <[email protected]>
> > > [Bartosz:
> > > - use the managed variant of nvmem_register(),
> > > - set the nvmem name]
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> >
> > What happened to the 2 other patches of Alban's series? I'd really
> > like the DT case to be handled/agreed on in the same patchset, but
> > IIRC, Alban and Srinivas disagreed on how this should be represented.
> > I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> > glue has been floating around for quite some time...
>
> These other patches were to fix what I consider a fundamental flaw in
> the generic NVMEM bindings, however we couldn't agree on this point.
> Bartosz later contacted me to take over this series and I suggested to
> just change the MTD NVMEM binding to use a compatible string on the
> NVMEM cells as an alternative solution to fix the clash with the old
> style MTD partition.
>
> However all this has no impact on the code needed to add NVMEM support
> to MTD, so the above patch didn't change at all.

It does have an impact on the supported binding though.
nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
means people will be able to define their NVMEM cells directly under
the MTD device and reference them from other nodes (even if it's not
documented), and as you said, it conflict with the old MTD partition
bindings. So we'd better agree on this binding before merging this
patch.

I see several options:

1/ provide a way to tell the NVMEM framework not to use parent->of_node
even if it's != NULL. This way we really don't support defining
NVMEM cells in the DT, and also don't support referencing the nvmem
device using a phandle.

2/ define a new binding where all nvmem-cells are placed in an
"nvmem" subnode (just like we have this "partitions" subnode for
partitions), and then add a config->of_node field so that the
nvmem provider can explicitly specify the DT node representing the
nvmem device. We'll also need to set this field to ERR_PTR(-ENOENT)
in case this node does not exist so that the nvmem framework knows
that it should not assign nvmem->dev.of_node to parent->of_node

3/ only declare partitions as nvmem providers. This would solve the
problem we have with partitions defined in the DT since
defining sub-partitions in the DT is not (yet?) supported and
partition nodes are supposed to be leaf nodes. Still, I'm not a big
fan of this solution because it will prevent us from supporting
sub-partitions if we ever want/need to.

4/ Add a ->of_xlate() hook that would be called if present by the
framework instead of using the default parsing we have right now.

5/ Tell the nvmem framework the name of the subnode containing nvmem
cell definitions (if NULL that means cells are directly defined
under the nvmem provider node). We would set it to "nvmem-cells" (or
whatever you like) for the MTD case.

There are probably other options (some were proposed by Alban and
Srinivas already), but I'd like to get this sorted out before we merge
this patch.

Alban, Srinivas, any opinion?

2018-08-20 10:45:41

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

Thanks Boris, for looking into this in more detail.

On 19/08/18 17:46, Boris Brezillon wrote:
> On Sun, 19 Aug 2018 13:31:06 +0200
> Alban <[email protected]> wrote:
>
>> On Fri, 17 Aug 2018 18:27:20 +0200
>> Boris Brezillon <[email protected]> wrote:
>>
>>> Hi Bartosz,
>>>
>>> On Fri, 10 Aug 2018 10:05:03 +0200
>>> Bartosz Golaszewski <[email protected]> wrote:
>>>
>>>> From: Alban Bedel <[email protected]>
>>>>
>>>> Allow drivers that use the nvmem API to read data stored on MTD devices.
>>>> For this the mtd devices are registered as read-only NVMEM providers.
>>>>
>>>> Signed-off-by: Alban Bedel <[email protected]>
>>>> [Bartosz:
>>>> - use the managed variant of nvmem_register(),
>>>> - set the nvmem name]
>>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>>
>>> What happened to the 2 other patches of Alban's series? I'd really
>>> like the DT case to be handled/agreed on in the same patchset, but
>>> IIRC, Alban and Srinivas disagreed on how this should be represented.
>>> I hope this time we'll come to an agreement, because the MTD <-> NVMEM
>>> glue has been floating around for quite some time...
>>
>> These other patches were to fix what I consider a fundamental flaw in
>> the generic NVMEM bindings, however we couldn't agree on this point.
>> Bartosz later contacted me to take over this series and I suggested to
>> just change the MTD NVMEM binding to use a compatible string on the
>> NVMEM cells as an alternative solution to fix the clash with the old
>> style MTD partition.
>>
>> However all this has no impact on the code needed to add NVMEM support
>> to MTD, so the above patch didn't change at all.
>
> It does have an impact on the supported binding though.
> nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
> means people will be able to define their NVMEM cells directly under
> the MTD device and reference them from other nodes (even if it's not
> documented), and as you said, it conflict with the old MTD partition
> bindings. So we'd better agree on this binding before merging this
> patch.
>

Yes, I agree with you!

> I see several options:
>
> 1/ provide a way to tell the NVMEM framework not to use parent->of_node
> even if it's != NULL. This way we really don't support defining
> NVMEM cells in the DT, and also don't support referencing the nvmem
> device using a phandle.
>
Other options look much better than this one!

> 2/ define a new binding where all nvmem-cells are placed in an
> "nvmem" subnode (just like we have this "partitions" subnode for
> partitions), and then add a config->of_node field so that the
> nvmem provider can explicitly specify the DT node representing the
> nvmem device. We'll also need to set this field to ERR_PTR(-ENOENT)
> in case this node does not exist so that the nvmem framework knows
> that it should not assign nvmem->dev.of_node to parent->of_node
>
This one looks promising, One Question though..

Do we expect that there would be nvmem cells in any of the partitions?
or
nvmem cell are only valid for unpartioned area?

Am sure that the nvmem cells would be in multiple partitions, Is it okay
to have some parts of partition to be in a separate subnode?

I would like this case to be considered too.

> 3/ only declare partitions as nvmem providers. This would solve the
> problem we have with partitions defined in the DT since
> defining sub-partitions in the DT is not (yet?) supported and
> partition nodes are supposed to be leaf nodes. Still, I'm not a big
> fan of this solution because it will prevent us from supporting
> sub-partitions if we ever want/need to.
>
This one is going to come back so, its better we

> 4/ Add a ->of_xlate() hook that would be called if present by the
> framework instead of using the default parsing we have right now.
This looks much cleaner! We could hook that up under
__nvmem_device_get() to do that translation.

>
> 5/ Tell the nvmem framework the name of the subnode containing nvmem
> cell definitions (if NULL that means cells are directly defined
> under the nvmem provider node). We would set it to "nvmem-cells" (or
> whatever you like) for the MTD case.
Option 2 looks better than this.

>
> There are probably other options (some were proposed by Alban and
> Srinivas already), but I'd like to get this sorted out before we merge
> this patch.
>
> Alban, Srinivas, any opinion?

Overall am still not able to clear visualize on how MTD bindings with
nvmem cells would look in both partition and un-partition usecases?
An example DT would be nice here!!

Option 4 looks like much generic solution to me, may be we should try
this once bindings on MTD side w.r.t nvmem cells are decided.

Thanks,
Srini

>

2018-08-20 18:22:17

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Mon, 20 Aug 2018 11:43:34 +0100
Srinivas Kandagatla <[email protected]> wrote:

>
> Overall am still not able to clear visualize on how MTD bindings with
> nvmem cells would look in both partition and un-partition usecases?
> An example DT would be nice here!!

Something along those lines:

mtdnode {
nvmem-cells {
#address-cells = <1>;
#size-cells = <1>;

cell@0 {
reg = <0x0 0x14>;
};
};

partitions {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;

partition@0 {
reg = <0x0 0x20000>;

nvmem-cells {
#address-cells = <1>;
#size-cells = <1>;

cell@0 {
reg = <0x0 0x10>;
};
};
};
};
};

2018-08-20 18:52:21

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

2018-08-20 20:20 GMT+02:00 Boris Brezillon <[email protected]>:
> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <[email protected]> wrote:
>
>>
>> Overall am still not able to clear visualize on how MTD bindings with
>> nvmem cells would look in both partition and un-partition usecases?
>> An example DT would be nice here!!
>
> Something along those lines:
>
> mtdnode {
> nvmem-cells {
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x14>;
> };
> };
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
>
> nvmem-cells {
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x10>;
> };
> };
> };
> };
> };

If there'll be an agreement on the bindings: will you be willing to
merge Alban's patch even without support in the code for the above
(with the assumption that it will be added later)? My use-case is on
non-DT systems and creating nvmem devices corresponding to MTD
partitions if fine by me. I also don't have the means to test the
support for these bindings if I were to actually write them myself.

Best regards,
Bartosz Golaszewski

2018-08-20 19:08:29

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Mon, 20 Aug 2018 20:50:55 +0200
Bartosz Golaszewski <[email protected]> wrote:

> 2018-08-20 20:20 GMT+02:00 Boris Brezillon <[email protected]>:
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <[email protected]> wrote:
> >
> >>
> >> Overall am still not able to clear visualize on how MTD bindings with
> >> nvmem cells would look in both partition and un-partition usecases?
> >> An example DT would be nice here!!
> >
> > Something along those lines:
> >
> > mtdnode {
> > nvmem-cells {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x14>;
> > };
> > };
> >
> > partitions {
> > compatible = "fixed-partitions";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > partition@0 {
> > reg = <0x0 0x20000>;
> >
> > nvmem-cells {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x10>;
> > };
> > };
> > };
> > };
> > };
>
> If there'll be an agreement on the bindings: will you be willing to
> merge Alban's patch even without support in the code for the above
> (with the assumption that it will be added later)?

No, because Alban's patch actually allows people to define and
reference nvmem cells in a DT, but without documenting it (see my first
reply).

> My use-case is on
> non-DT systems and creating nvmem devices corresponding to MTD
> partitions if fine by me.

What you propose is option #1 in my list of proposals, and it requires
some changes to avoid automatically assigning nvmem->dev.of_node to
parent->of_node (which will be != NULL when the MTD device has been
instantiated from a DT node).

> I also don't have the means to test the
> support for these bindings if I were to actually write them myself.

And that's the very reason I proposed #1. I don't want to block this
stuff, but in its current state, I'm not willing to accept it either.
Either we agree on the binding and patch the nvmem framework to support
this new binding, or we find a way to hide the fact that the mtd
device (the nvmem parent) has a DT node attached to it.

2018-08-20 21:31:00

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Mon, 20 Aug 2018 20:20:38 +0200
Boris Brezillon <[email protected]> wrote:

> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <[email protected]> wrote:
>
> >
> > Overall am still not able to clear visualize on how MTD bindings with
> > nvmem cells would look in both partition and un-partition usecases?
> > An example DT would be nice here!!
>
> Something along those lines:

We must also have a compatible string on the nvmem-cells node to make
sure we don't clash with the old style MTD partitions, or some other
device specific binding.

>
> mtdnode {
> nvmem-cells {
compatible = "nvmem-cells";
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x14>;
> };
> };
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
>
> nvmem-cells {
compatible = "nvmem-cells";
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x10>;
> };
> };
> };
> };
> };


Alban


Attachments:
(No filename) (836.00 B)
OpenPGP digital signature

2018-08-20 22:56:15

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Sun, 19 Aug 2018 18:46:09 +0200
Boris Brezillon <[email protected]> wrote:

> On Sun, 19 Aug 2018 13:31:06 +0200
> Alban <[email protected]> wrote:
>
> > On Fri, 17 Aug 2018 18:27:20 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > Hi Bartosz,
> > >
> > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > Bartosz Golaszewski <[email protected]> wrote:
> > >
> > > > From: Alban Bedel <[email protected]>
> > > >
> > > > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > > > For this the mtd devices are registered as read-only NVMEM providers.
> > > >
> > > > Signed-off-by: Alban Bedel <[email protected]>
> > > > [Bartosz:
> > > > - use the managed variant of nvmem_register(),
> > > > - set the nvmem name]
> > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > >
> > > What happened to the 2 other patches of Alban's series? I'd really
> > > like the DT case to be handled/agreed on in the same patchset, but
> > > IIRC, Alban and Srinivas disagreed on how this should be represented.
> > > I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> > > glue has been floating around for quite some time...
> >
> > These other patches were to fix what I consider a fundamental flaw in
> > the generic NVMEM bindings, however we couldn't agree on this point.
> > Bartosz later contacted me to take over this series and I suggested to
> > just change the MTD NVMEM binding to use a compatible string on the
> > NVMEM cells as an alternative solution to fix the clash with the old
> > style MTD partition.
> >
> > However all this has no impact on the code needed to add NVMEM support
> > to MTD, so the above patch didn't change at all.
>
> It does have an impact on the supported binding though.
> nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
> means people will be able to define their NVMEM cells directly under
> the MTD device and reference them from other nodes (even if it's not
> documented), and as you said, it conflict with the old MTD partition
> bindings. So we'd better agree on this binding before merging this
> patch.

Unless the nvmem cell node has a compatible string, then it won't be
considered as a partition by the MTD code. That is were the clash is,
both bindings allow free named child nodes without a compatible string.

> I see several options:
>
> 1/ provide a way to tell the NVMEM framework not to use parent->of_node
> even if it's != NULL. This way we really don't support defining
> NVMEM cells in the DT, and also don't support referencing the nvmem
> device using a phandle.

I really don't get what the point of this would be. Make the whole API
useless?

> 2/ define a new binding where all nvmem-cells are placed in an
> "nvmem" subnode (just like we have this "partitions" subnode for
> partitions), and then add a config->of_node field so that the
> nvmem provider can explicitly specify the DT node representing the
> nvmem device. We'll also need to set this field to ERR_PTR(-ENOENT)
> in case this node does not exist so that the nvmem framework knows
> that it should not assign nvmem->dev.of_node to parent->of_node

This is not good. First the NVMEM device is only a virtual concept of
the Linux kernel, it has no place in the DT. Secondly the NVMEM
provider (here the MTD device) then has to manually parse its DT node to
find this subnode, pass it to the NVMEM framework to later again
resolve it back to the MTD device. Not very complex but still a lot of
useless code, just registering the MTD device is a lot simpler and much
more inline with most other kernel API that register a "service"
available from a device.

> 3/ only declare partitions as nvmem providers. This would solve the
> problem we have with partitions defined in the DT since
> defining sub-partitions in the DT is not (yet?) supported and
> partition nodes are supposed to be leaf nodes. Still, I'm not a big
> fan of this solution because it will prevent us from supporting
> sub-partitions if we ever want/need to.

That sound like a poor workaround. Remember that this problem could
appear with any device that has a binding that use child nodes.

> 4/ Add a ->of_xlate() hook that would be called if present by the
> framework instead of using the default parsing we have right now.

That is a bit cleaner, but I don't think it would be worse the
complexity. Furthermore xlate functions are more about converting
from hardware parameters to internal kernel representation than to hide
extra DT parsing.

> 5/ Tell the nvmem framework the name of the subnode containing nvmem
> cell definitions (if NULL that means cells are directly defined
> under the nvmem provider node). We would set it to "nvmem-cells" (or
> whatever you like) for the MTD case.

If so please match on compatible and not on the node name.

6/ Extend the current NVMEM cell lookup to check if the parent node of
the cell has a compatible string set to "nvmem-cells". If it doesn't it
mean we have the current binding and this node is the NVMEM device. If
it does the device node is just the next parent. This is trivial to
implement (literally 2 lines of code) and cover all the cases currently
known.

7/ Just add a compatible string to the nvmem cell. No code change is
needed, however as the nvmem cells have an address space (the offset in
byte in the storage) it might still clash with another address space
used by the main device biding (for example a number of child
functions).

> There are probably other options (some were proposed by Alban and
> Srinivas already), but I'd like to get this sorted out before we merge
> this patch.
>
> Alban, Srinivas, any opinion?

My preference goes to 6/ as it is trivial to implement, solves all
known shortcomings and is backward compatible with the current binding.
All other solutions have limitations and/or require too complex
implementations compared to what they try to solve.

Alban


Attachments:
(No filename) (836.00 B)
OpenPGP digital signature

2018-08-21 05:09:07

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Mon, 20 Aug 2018 23:27:48 +0200
Alban <[email protected]> wrote:

> On Mon, 20 Aug 2018 20:20:38 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <[email protected]> wrote:
> >
> > >
> > > Overall am still not able to clear visualize on how MTD bindings with
> > > nvmem cells would look in both partition and un-partition usecases?
> > > An example DT would be nice here!!
> >
> > Something along those lines:
>
> We must also have a compatible string on the nvmem-cells node to make
> sure we don't clash with the old style MTD partitions,

That's not possible, because we don't have a reg prop in the
nvmem-cells node.

> or some other
> device specific binding.

This one might happen.

Was Rob okay with this compatible? If he was, I guess we can go for
this binding. Srinivas, any objection?

>
> >
> > mtdnode {
> > nvmem-cells {
> compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x14>;
> > };
> > };
> >
> > partitions {
> > compatible = "fixed-partitions";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > partition@0 {
> > reg = <0x0 0x20000>;
> >
> > nvmem-cells {
> compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x10>;
> > };
> > };
> > };
> > };
> > };
>
>
> Alban


2018-08-21 05:45:33

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 00:53:27 +0200
Alban <[email protected]> wrote:

> On Sun, 19 Aug 2018 18:46:09 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Sun, 19 Aug 2018 13:31:06 +0200
> > Alban <[email protected]> wrote:
> >
> > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > Boris Brezillon <[email protected]> wrote:
> > >
> > > > Hi Bartosz,
> > > >
> > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > Bartosz Golaszewski <[email protected]> wrote:
> > > >
> > > > > From: Alban Bedel <[email protected]>
> > > > >
> > > > > Allow drivers that use the nvmem API to read data stored on MTD devices.
> > > > > For this the mtd devices are registered as read-only NVMEM providers.
> > > > >
> > > > > Signed-off-by: Alban Bedel <[email protected]>
> > > > > [Bartosz:
> > > > > - use the managed variant of nvmem_register(),
> > > > > - set the nvmem name]
> > > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > >
> > > > What happened to the 2 other patches of Alban's series? I'd really
> > > > like the DT case to be handled/agreed on in the same patchset, but
> > > > IIRC, Alban and Srinivas disagreed on how this should be represented.
> > > > I hope this time we'll come to an agreement, because the MTD <-> NVMEM
> > > > glue has been floating around for quite some time...
> > >
> > > These other patches were to fix what I consider a fundamental flaw in
> > > the generic NVMEM bindings, however we couldn't agree on this point.
> > > Bartosz later contacted me to take over this series and I suggested to
> > > just change the MTD NVMEM binding to use a compatible string on the
> > > NVMEM cells as an alternative solution to fix the clash with the old
> > > style MTD partition.
> > >
> > > However all this has no impact on the code needed to add NVMEM support
> > > to MTD, so the above patch didn't change at all.
> >
> > It does have an impact on the supported binding though.
> > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, which
> > means people will be able to define their NVMEM cells directly under
> > the MTD device and reference them from other nodes (even if it's not
> > documented), and as you said, it conflict with the old MTD partition
> > bindings. So we'd better agree on this binding before merging this
> > patch.
>
> Unless the nvmem cell node has a compatible string, then it won't be
> considered as a partition by the MTD code. That is were the clash is,
> both bindings allow free named child nodes without a compatible string.

Except the current nvmem cells parsing code does not enforce that, and
existing DTs rely on this behavior, so we're screwed. Or are you
suggesting to add a new "bool check_cells_compat;" field to
nvmem_config?

>
> > I see several options:
> >
> > 1/ provide a way to tell the NVMEM framework not to use parent->of_node
> > even if it's != NULL. This way we really don't support defining
> > NVMEM cells in the DT, and also don't support referencing the nvmem
> > device using a phandle.
>
> I really don't get what the point of this would be. Make the whole API
> useless?

No, just allow Bartosz to get his changes merged without waiting for you
and Srinivas to agree on how to handle the new binding. As I said
earlier, this mtd <-> nvmem stuff has been around for quite some time,
and instead of trying to find an approach that makes everyone happy, you
decided to let the patchset die.

>
> > 2/ define a new binding where all nvmem-cells are placed in an
> > "nvmem" subnode (just like we have this "partitions" subnode for
> > partitions), and then add a config->of_node field so that the
> > nvmem provider can explicitly specify the DT node representing the
> > nvmem device. We'll also need to set this field to ERR_PTR(-ENOENT)
> > in case this node does not exist so that the nvmem framework knows
> > that it should not assign nvmem->dev.of_node to parent->of_node
>
> This is not good. First the NVMEM device is only a virtual concept of
> the Linux kernel, it has no place in the DT.

nvmem-cells is a virtual concept too, still, you define them in the DT.

> Secondly the NVMEM
> provider (here the MTD device) then has to manually parse its DT node to
> find this subnode, pass it to the NVMEM framework to later again
> resolve it back to the MTD device.

We don't resolve it back to the MTD device, because the MTD device is
just the parent of the nvmem device.

> Not very complex but still a lot of
> useless code, just registering the MTD device is a lot simpler and much
> more inline with most other kernel API that register a "service"
> available from a device.

I'm not a big fan of this option either, but I thought I had to propose
it.

>
> > 3/ only declare partitions as nvmem providers. This would solve the
> > problem we have with partitions defined in the DT since
> > defining sub-partitions in the DT is not (yet?) supported and
> > partition nodes are supposed to be leaf nodes. Still, I'm not a big
> > fan of this solution because it will prevent us from supporting
> > sub-partitions if we ever want/need to.
>
> That sound like a poor workaround.

Yes, that's a workaround. And the reason I propose it, is, again,
because I don't want to block Bartosz.

> Remember that this problem could
> appear with any device that has a binding that use child nodes.

I'm talking about partitions, and you're talking about mtd devices.
Right now partitions don't have subnodes, and if we define that
partition subnodes should describe nvmem-cells, then it becomes part of
the official binding. So, no, the problem you mention does not (yet)
exist.

>
> > 4/ Add a ->of_xlate() hook that would be called if present by the
> > framework instead of using the default parsing we have right now.
>
> That is a bit cleaner, but I don't think it would be worse the
> complexity.

But it's way more flexible than putting everything in the nvmem
framework. BTW, did you notice that nvmem-cells parsing does not work
with flashes bigger than 4GB, because the framework assumes
#address-cells and #size-cells are always 1. That's probably something
we'll have to fix for the MTD case.

> Furthermore xlate functions are more about converting
> from hardware parameters to internal kernel representation than to hide
> extra DT parsing.

Hm, how is that different? ->of_xlate() is just a way for drivers to
have their own DT representation, which is exactly what we want here.

>
> > 5/ Tell the nvmem framework the name of the subnode containing nvmem
> > cell definitions (if NULL that means cells are directly defined
> > under the nvmem provider node). We would set it to "nvmem-cells" (or
> > whatever you like) for the MTD case.
>
> If so please match on compatible and not on the node name.

If you like.

>
> 6/ Extend the current NVMEM cell lookup to check if the parent node of
> the cell has a compatible string set to "nvmem-cells". If it doesn't it
> mean we have the current binding and this node is the NVMEM device. If
> it does the device node is just the next parent. This is trivial to
> implement (literally 2 lines of code) and cover all the cases currently
> known.

Except Srinivas was not happy with this solution, and this stalled the
discussion. I'm trying to find other options and you keep rejecting all
of them to come back to this one.

>
> 7/ Just add a compatible string to the nvmem cell. No code change is
> needed,

That's not true!!! What forces people to add this compatible in their
DT? Nothing. I'll tell you what will happen: people will start defining
their nvmem cells directly under the MTD node because that *works*, and
even if the binding is not documented and we consider it invalid, we'll
be stuck supporting it forever. As said above, the very reason for
option #1 to exist is to give you and Srinivas some more time to sort
this out, while unblocking Bartosz in the meantime.

> however as the nvmem cells have an address space (the offset in
> byte in the storage) it might still clash with another address space
> used by the main device biding (for example a number of child
> functions).
>
> > There are probably other options (some were proposed by Alban and
> > Srinivas already), but I'd like to get this sorted out before we merge
> > this patch.
> >
> > Alban, Srinivas, any opinion?
>
> My preference goes to 6/ as it is trivial to implement, solves all
> known shortcomings and is backward compatible with the current binding.
> All other solutions have limitations and/or require too complex
> implementations compared to what they try to solve.

So we're back to square 1, and you're again blocking everything because
you refuse to consider other options.

There's obviously nothing more I can do to help, and that's unfortunate
because other people are waiting for this feature.

Regards,

Boris

2018-08-21 09:41:39

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

Hi Boris/Bartosz,

On 21/08/18 06:44, Boris Brezillon wrote:
>>> 4/ Add a ->of_xlate() hook that would be called if present by the
>>> framework instead of using the default parsing we have right now.
>> That is a bit cleaner, but I don't think it would be worse the
>> complexity.
> But it's way more flexible than putting everything in the nvmem
> framework. BTW, did you notice that nvmem-cells parsing does not work
> with flashes bigger than 4GB, because the framework assumes
> #address-cells and #size-cells are always 1. That's probably something
> we'll have to fix for the MTD case.
>

I have hacked up some thing on these lines to add a custom match
function for nvmem provider and it looks like it can work for mtd case.

This addresses concern #1 "to ignore of_node from dev pointer passed to
nvmem_config" also provides way to do some sanity checks on nvmem cell node.
In this patch I have just added a simple mtd_nvmem_match() example which
will be always true, however we can add checks here to see if the np is
actually a nvmem-cells node or something on those lines to enforce the
bindings. Please fix and remove this from nvmem-core patch incase you
plan to use/test this.

We still have one open issue of supporting #address-cells and
#size-cells in nvmem, which I can look at if you are happy with this
approach!

----------------------------------->cut<---------------------------------
Author: Srinivas Kandagatla <[email protected]>
Date: Tue Aug 21 10:07:24 2018 +0100

nvmem: core: add custom match function support

Some nvmem providers might not have a simple DT layout, nvmem cells
could be part of the unpartioned space or with-in partition or
even in sub partition of the provider.

Current matching function is expecting that the provider should be
immediate parent of the cell, which might not be true for the above
cases. So allow a custom match function for such devices which can
validate and match the cell as per the provider specific bindings.

Signed-off-by: Srinivas Kandagatla <[email protected]>

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index a57302eaceb5..33541b18ac30 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -502,6 +502,19 @@ static int mtd_nvmem_reg_read(void *priv, unsigned
int offset,
return retlen == bytes ? 0 : -EIO;
}

+static int mtd_nvmem_match(void *priv, struct device *dev,
+ struct device_node *np)
+{
+ struct mtd_info *mtd = priv;
+
+ /*
+ * Add more checks to make sure device node is inline with
+ * binding if required
+ */
+
+ return &mtd->dev == dev->parent;
+}
+
static int mtd_nvmem_add(struct mtd_info *mtd)
{
struct nvmem_config config = { };
@@ -516,6 +529,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
config.read_only = true;
config.root_only = true;
config.priv = mtd;
+ config.match = mtd_nvmem_match;

mtd->nvmem = devm_nvmem_register(&mtd->dev, &config);
if (IS_ERR(mtd->nvmem)) {
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 3a8bf832243d..32bc4e70522c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -41,6 +41,7 @@ struct nvmem_device {
struct device *base_dev;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
+ nvmem_match_t match;
void *priv;
};

@@ -265,6 +266,11 @@ static struct bus_type nvmem_bus_type = {

static int of_nvmem_match(struct device *dev, void *nvmem_np)
{
+ struct nvmem_device *nvmem = to_nvmem_device(dev);
+
+ if (nvmem->match)
+ return nvmem->match(nvmem->priv, dev, nvmem_np);
+
return dev->of_node == nvmem_np;
}

@@ -482,7 +488,9 @@ struct nvmem_device *nvmem_register(const struct
nvmem_config *config)
nvmem->priv = config->priv;
nvmem->reg_read = config->reg_read;
nvmem->reg_write = config->reg_write;
- nvmem->dev.of_node = config->dev->of_node;
+
+ if (!config->match)
+ nvmem->dev.of_node = config->dev->of_node;

if (config->id == -1 && config->name) {
dev_set_name(&nvmem->dev, "%s", config->name);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 24def6ad09bb..b29059bb406e 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -14,6 +14,7 @@

#include <linux/err.h>
#include <linux/errno.h>
+#include <linux/of.h>

struct nvmem_device;
struct nvmem_cell_info;
@@ -21,6 +22,9 @@ 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_match_t)(void *priv, struct device *dev,
+ struct device_node *np);
+

/**
* struct nvmem_config - NVMEM device configuration
@@ -58,6 +62,7 @@ struct nvmem_config {
bool root_only;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
+ nvmem_match_t match;
int size;
int word_size;
int stride;

----------------------------------->cut<---------------------------------

thanks,
srini

2018-08-21 09:51:42

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 20/08/18 19:20, Boris Brezillon wrote:
> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <[email protected]> wrote:
>
>>
>> Overall am still not able to clear visualize on how MTD bindings with
>> nvmem cells would look in both partition and un-partition usecases?
>> An example DT would be nice here!!
>
> Something along those lines:
>
This looks good to me.
> mtdnode {
> nvmem-cells {
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x14>;
> };
> };
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
>
> nvmem-cells {
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x10>;
> };
> };
> };
> };
> }; >

Just curious...Is there a reason why we can't do it like this?:
Is this because of issue of #address-cells and #size-cells Or mtd
bindings always prefer subnodes?

mtdnode {
reg = <0x0123000 0x40000>;
#address-cells = <1>;
#size-cells = <1>;
cell@0 {
compatible = "nvmem-cell";
reg = <0x0 0x14>;
};

partitions {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;

partition@0 {
reg = <0x0 0x20000>;
cell@0 {
compatible = "nvmem-cell";
reg = <0x0 0x10>;
};
};
};
};

Am okay either way!

thanks,
srini

2018-08-21 09:58:43

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 10:50:07 +0100
Srinivas Kandagatla <[email protected]> wrote:

> On 20/08/18 19:20, Boris Brezillon wrote:
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <[email protected]> wrote:
> >
> >>
> >> Overall am still not able to clear visualize on how MTD bindings with
> >> nvmem cells would look in both partition and un-partition usecases?
> >> An example DT would be nice here!!
> >
> > Something along those lines:
> >
> This looks good to me.
> > mtdnode {
> > nvmem-cells {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x14>;
> > };
> > };
> >
> > partitions {
> > compatible = "fixed-partitions";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > partition@0 {
> > reg = <0x0 0x20000>;
> >
> > nvmem-cells {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x10>;
> > };
> > };
> > };
> > };
> > }; >
>
> Just curious...Is there a reason why we can't do it like this?:
> Is this because of issue of #address-cells and #size-cells Or mtd
> bindings always prefer subnodes?
>
> mtdnode {
> reg = <0x0123000 0x40000>;
> #address-cells = <1>;
> #size-cells = <1>;
> cell@0 {
> compatible = "nvmem-cell";
> reg = <0x0 0x14>;
> };
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
> cell@0 {
> compatible = "nvmem-cell";
> reg = <0x0 0x10>;
> };
> };
> };
> };

It's because partitions were initially directly defined under the mtd
node, so, if you have an old DT you might have something like:

mtdnode {
reg = <0x0123000 0x40000>;
#address-cells = <1>;
#size-cells = <1>;

partition@0 {
reg = <0x0 0x20000>;
...
};
...
};

If we use such a DT with this patch applied, the NVMEM framework will
consider MTD partitions as nvmem cells, which is not what we want.

2018-08-21 10:47:19

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 11:11:58 +0100
Srinivas Kandagatla <[email protected]> wrote:

> On 21/08/18 10:56, Boris Brezillon wrote:
> > On Tue, 21 Aug 2018 10:50:07 +0100
> > Srinivas Kandagatla<[email protected]> wrote:
> >
> >> On 20/08/18 19:20, Boris Brezillon wrote:
> >>> On Mon, 20 Aug 2018 11:43:34 +0100
> >>> Srinivas Kandagatla<[email protected]> wrote:
> >>>
> >>>> Overall am still not able to clear visualize on how MTD bindings with
> >>>> nvmem cells would look in both partition and un-partition usecases?
> >>>> An example DT would be nice here!!
> >>> Something along those lines:
> >>>
> >> This looks good to me.
> >>> mtdnode {
> >>> nvmem-cells {
> >>> #address-cells = <1>;
> >>> #size-cells = <1>;
> >>>
> >>> cell@0 {
> >>> reg = <0x0 0x14>;
> >>> };
> >>> };
> >>>
> >>> partitions {
> >>> compatible = "fixed-partitions";
> >>> #address-cells = <1>;
> >>> #size-cells = <1>;
> >>>
> >>> partition@0 {
> >>> reg = <0x0 0x20000>;
> >>>
> >>> nvmem-cells {
> >>> #address-cells = <1>;
> >>> #size-cells = <1>;
> >>>
> >>> cell@0 {
> >>> reg = <0x0 0x10>;
> >>> };
> >>> };
> >>> };
> >>> };
> >>> }; >
> >> Just curious...Is there a reason why we can't do it like this?:
> >> Is this because of issue of #address-cells and #size-cells Or mtd
> >> bindings always prefer subnodes?
> >>
> >> mtdnode {
> >> reg = <0x0123000 0x40000>;
> >> #address-cells = <1>;
> >> #size-cells = <1>;
> >> cell@0 {
> >> compatible = "nvmem-cell";
> >> reg = <0x0 0x14>;
> >> };
> >>
> >> partitions {
> >> compatible = "fixed-partitions";
> >> #address-cells = <1>;
> >> #size-cells = <1>;
> >>
> >> partition@0 {
> >> reg = <0x0 0x20000>;
> >> cell@0 {
> >> compatible = "nvmem-cell";
> >> reg = <0x0 0x10>;
> >> };
> >> };
> >> };
> >> };
> > It's because partitions were initially directly defined under the mtd
> > node, so, if you have an old DT you might have something like:
> >
> > mtdnode {
> > reg = <0x0123000 0x40000>;
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > partition@0 {
> > reg = <0x0 0x20000>;
> > ...
> > };
> > ...
> > };
> >
> > If we use such a DT with this patch applied, the NVMEM framework will
> > consider MTD partitions as nvmem cells, which is not what we want.
> Yep, I agree.
> TBH, I wanted to add compatible string to nvmem-cell at some point in
> time and it seems more natural update too. One of the reason we
> discussed this in the past was parsers. Looks like mtd can make use of this.
>
> We should be able to add this as an optional flag in nvmem_config to
> enforce this check in case providers wanted to.
>
> Do you think that would help mtd nvmem case?

Yes, it should work if nvmem cells are defined directly under the mtd
node (or the partition they belong to).

> Also I felt like nvmem-cells subnode seems to be a bit heavy!

I still think grouping nvmem cells in a subnode is cleaner (just like
we do for partitions), but I won't object if all parties (you, Alban
and Rob) agree on this solution.

2018-08-21 10:47:19

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 21/08/18 10:56, Boris Brezillon wrote:
> On Tue, 21 Aug 2018 10:50:07 +0100
> Srinivas Kandagatla<[email protected]> wrote:
>
>> On 20/08/18 19:20, Boris Brezillon wrote:
>>> On Mon, 20 Aug 2018 11:43:34 +0100
>>> Srinivas Kandagatla<[email protected]> wrote:
>>>
>>>> Overall am still not able to clear visualize on how MTD bindings with
>>>> nvmem cells would look in both partition and un-partition usecases?
>>>> An example DT would be nice here!!
>>> Something along those lines:
>>>
>> This looks good to me.
>>> mtdnode {
>>> nvmem-cells {
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>>> cell@0 {
>>> reg = <0x0 0x14>;
>>> };
>>> };
>>>
>>> partitions {
>>> compatible = "fixed-partitions";
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>>> partition@0 {
>>> reg = <0x0 0x20000>;
>>>
>>> nvmem-cells {
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>>> cell@0 {
>>> reg = <0x0 0x10>;
>>> };
>>> };
>>> };
>>> };
>>> }; >
>> Just curious...Is there a reason why we can't do it like this?:
>> Is this because of issue of #address-cells and #size-cells Or mtd
>> bindings always prefer subnodes?
>>
>> mtdnode {
>> reg = <0x0123000 0x40000>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> cell@0 {
>> compatible = "nvmem-cell";
>> reg = <0x0 0x14>;
>> };
>>
>> partitions {
>> compatible = "fixed-partitions";
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> partition@0 {
>> reg = <0x0 0x20000>;
>> cell@0 {
>> compatible = "nvmem-cell";
>> reg = <0x0 0x10>;
>> };
>> };
>> };
>> };
> It's because partitions were initially directly defined under the mtd
> node, so, if you have an old DT you might have something like:
>
> mtdnode {
> reg = <0x0123000 0x40000>;
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
> ...
> };
> ...
> };
>
> If we use such a DT with this patch applied, the NVMEM framework will
> consider MTD partitions as nvmem cells, which is not what we want.
Yep, I agree.
TBH, I wanted to add compatible string to nvmem-cell at some point in
time and it seems more natural update too. One of the reason we
discussed this in the past was parsers. Looks like mtd can make use of this.

We should be able to add this as an optional flag in nvmem_config to
enforce this check in case providers wanted to.

Do you think that would help mtd nvmem case?
Also I felt like nvmem-cells subnode seems to be a bit heavy!

thanks,
srini

2018-08-21 11:34:14

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 10:38:13 +0100
Srinivas Kandagatla <[email protected]> wrote:

> Hi Boris/Bartosz,
>
> On 21/08/18 06:44, Boris Brezillon wrote:
> >>> 4/ Add a ->of_xlate() hook that would be called if present by the
> >>> framework instead of using the default parsing we have right now.
> >> That is a bit cleaner, but I don't think it would be worse the
> >> complexity.
> > But it's way more flexible than putting everything in the nvmem
> > framework. BTW, did you notice that nvmem-cells parsing does not work
> > with flashes bigger than 4GB, because the framework assumes
> > #address-cells and #size-cells are always 1. That's probably something
> > we'll have to fix for the MTD case.
> >
>
> I have hacked up some thing on these lines to add a custom match
> function for nvmem provider and it looks like it can work for mtd case.
>
> This addresses concern #1 "to ignore of_node from dev pointer passed to
> nvmem_config" also provides way to do some sanity checks on nvmem cell node.
> In this patch I have just added a simple mtd_nvmem_match() example which
> will be always true, however we can add checks here to see if the np is
> actually a nvmem-cells node or something on those lines to enforce the
> bindings. Please fix and remove this from nvmem-core patch incase you
> plan to use/test this.
>
> We still have one open issue of supporting #address-cells and
> #size-cells in nvmem, which I can look at if you are happy with this
> approach!
>
> ----------------------------------->cut<---------------------------------
> Author: Srinivas Kandagatla <[email protected]>
> Date: Tue Aug 21 10:07:24 2018 +0100
>
> nvmem: core: add custom match function support
>
> Some nvmem providers might not have a simple DT layout, nvmem cells
> could be part of the unpartioned space or with-in partition or
> even in sub partition of the provider.
>
> Current matching function is expecting that the provider should be
> immediate parent of the cell, which might not be true for the above
> cases. So allow a custom match function for such devices which can
> validate and match the cell as per the provider specific bindings.
>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index a57302eaceb5..33541b18ac30 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -502,6 +502,19 @@ static int mtd_nvmem_reg_read(void *priv, unsigned
> int offset,
> return retlen == bytes ? 0 : -EIO;
> }
>
> +static int mtd_nvmem_match(void *priv, struct device *dev,
> + struct device_node *np)
> +{
> + struct mtd_info *mtd = priv;
> +
> + /*
> + * Add more checks to make sure device node is inline with
> + * binding if required
> + */
> +
> + return &mtd->dev == dev->parent;
> +}
> +
> static int mtd_nvmem_add(struct mtd_info *mtd)
> {
> struct nvmem_config config = { };
> @@ -516,6 +529,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> config.read_only = true;
> config.root_only = true;
> config.priv = mtd;
> + config.match = mtd_nvmem_match;
>
> mtd->nvmem = devm_nvmem_register(&mtd->dev, &config);
> if (IS_ERR(mtd->nvmem)) {
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 3a8bf832243d..32bc4e70522c 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -41,6 +41,7 @@ struct nvmem_device {
> struct device *base_dev;
> nvmem_reg_read_t reg_read;
> nvmem_reg_write_t reg_write;
> + nvmem_match_t match;
> void *priv;
> };
>
> @@ -265,6 +266,11 @@ static struct bus_type nvmem_bus_type = {
>
> static int of_nvmem_match(struct device *dev, void *nvmem_np)
> {
> + struct nvmem_device *nvmem = to_nvmem_device(dev);
> +
> + if (nvmem->match)
> + return nvmem->match(nvmem->priv, dev, nvmem_np);
> +
> return dev->of_node == nvmem_np;
> }
>
> @@ -482,7 +488,9 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> nvmem->priv = config->priv;
> nvmem->reg_read = config->reg_read;
> nvmem->reg_write = config->reg_write;
> - nvmem->dev.of_node = config->dev->of_node;
> +
> + if (!config->match)
> + nvmem->dev.of_node = config->dev->of_node;
>
> if (config->id == -1 && config->name) {
> dev_set_name(&nvmem->dev, "%s", config->name);
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 24def6ad09bb..b29059bb406e 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -14,6 +14,7 @@
>
> #include <linux/err.h>
> #include <linux/errno.h>
> +#include <linux/of.h>
>
> struct nvmem_device;
> struct nvmem_cell_info;
> @@ -21,6 +22,9 @@ 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_match_t)(void *priv, struct device *dev,
> + struct device_node *np);
> +
>
> /**
> * struct nvmem_config - NVMEM device configuration
> @@ -58,6 +62,7 @@ struct nvmem_config {
> bool root_only;
> nvmem_reg_read_t reg_read;
> nvmem_reg_write_t reg_write;
> + nvmem_match_t match;
> int size;
> int word_size;
> int stride;
>

That might work if nvmem cells are defined directly under the mtdnode.
If we go for this approach, I'd recommend replacing this ->match() hook
by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
node, because what we're really after here is knowing which subnode is
an nvmem cell and which subnode is not.

2018-08-21 11:45:41

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 10:50:07 +0100
Srinivas Kandagatla <[email protected]> wrote:

> Just curious...Is there a reason why we can't do it like this?:
> Is this because of issue of #address-cells and #size-cells Or mtd
> bindings always prefer subnodes?
>
> mtdnode {
> reg = <0x0123000 0x40000>;
> #address-cells = <1>;
> #size-cells = <1>;
> cell@0 {
> compatible = "nvmem-cell";
> reg = <0x0 0x14>;
> };
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
> cell@0 {
> compatible = "nvmem-cell";
> reg = <0x0 0x10>;
> };
> };
> };
> };

That would work, the MTD partitions parser ignore child nodes with a
compatible string when looking for "old style" partitions, see [1].
However we still have the a potential address space clash between the
nvmem cells and the main device binding.

Alban

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/mtd/ofpart.c#L28


Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2018-08-21 12:01:34

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 21/08/18 12:39, Alban wrote:
> However we still have the a potential address space clash between the
> nvmem cells and the main device binding.
Can you elaborate?

--srini

2018-08-21 12:31:28

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 07:44:04 +0200
Boris Brezillon <[email protected]> wrote:

> On Tue, 21 Aug 2018 00:53:27 +0200
> Alban <[email protected]> wrote:
>
> > On Sun, 19 Aug 2018 18:46:09 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > On Sun, 19 Aug 2018 13:31:06 +0200
> > > Alban <[email protected]> wrote:
> > >
> > > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > > Boris Brezillon <[email protected]> wrote:
> > > >
> > > > > Hi Bartosz,
> > > > >
> > > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > > Bartosz Golaszewski <[email protected]> wrote:
> > > > >
> > > > > > From: Alban Bedel <[email protected]>
> > > > > >
> > > > > > Allow drivers that use the nvmem API to read data stored on
> > > > > > MTD devices. For this the mtd devices are registered as
> > > > > > read-only NVMEM providers.
> > > > > >
> > > > > > Signed-off-by: Alban Bedel <[email protected]>
> > > > > > [Bartosz:
> > > > > > - use the managed variant of nvmem_register(),
> > > > > > - set the nvmem name]
> > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > <[email protected]>
> > > > >
> > > > > What happened to the 2 other patches of Alban's series? I'd
> > > > > really like the DT case to be handled/agreed on in the same
> > > > > patchset, but IIRC, Alban and Srinivas disagreed on how this
> > > > > should be represented. I hope this time we'll come to an
> > > > > agreement, because the MTD <-> NVMEM glue has been floating
> > > > > around for quite some time...
> > > >
> > > > These other patches were to fix what I consider a fundamental
> > > > flaw in the generic NVMEM bindings, however we couldn't agree
> > > > on this point. Bartosz later contacted me to take over this
> > > > series and I suggested to just change the MTD NVMEM binding to
> > > > use a compatible string on the NVMEM cells as an alternative
> > > > solution to fix the clash with the old style MTD partition.
> > > >
> > > > However all this has no impact on the code needed to add NVMEM
> > > > support to MTD, so the above patch didn't change at all.
> > >
> > > It does have an impact on the supported binding though.
> > > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node,
> > > which means people will be able to define their NVMEM cells
> > > directly under the MTD device and reference them from other nodes
> > > (even if it's not documented), and as you said, it conflict with
> > > the old MTD partition bindings. So we'd better agree on this
> > > binding before merging this patch.
> >
> > Unless the nvmem cell node has a compatible string, then it won't be
> > considered as a partition by the MTD code. That is were the clash
> > is, both bindings allow free named child nodes without a compatible
> > string.
>
> Except the current nvmem cells parsing code does not enforce that, and
> existing DTs rely on this behavior, so we're screwed. Or are you
> suggesting to add a new "bool check_cells_compat;" field to
> nvmem_config?

There is no nvmem cell parsing at the moment. The DT lookup just
resolve the phandle to the cell node, take the parent node and search
for the nvmem provider that has this OF node. So extending it in case
the node has a *new* compatible string would not break users of the old
binding, none of them has a compatible string.

> >
> > > I see several options:
> > >
> > > 1/ provide a way to tell the NVMEM framework not to use
> > > parent->of_node even if it's != NULL. This way we really don't
> > > support defining NVMEM cells in the DT, and also don't support
> > > referencing the nvmem device using a phandle.
> >
> > I really don't get what the point of this would be. Make the whole
> > API useless?
>
> No, just allow Bartosz to get his changes merged without waiting for
> you and Srinivas to agree on how to handle the new binding. As I said
> earlier, this mtd <-> nvmem stuff has been around for quite some time,
> and instead of trying to find an approach that makes everyone happy,
> you decided to let the patchset die.

As long as that wouldn't prevent using DT in the future I'm fine with
it.

> >
> > > 2/ define a new binding where all nvmem-cells are placed in an
> > > "nvmem" subnode (just like we have this "partitions" subnode
> > > for partitions), and then add a config->of_node field so that the
> > > nvmem provider can explicitly specify the DT node representing
> > > the nvmem device. We'll also need to set this field to
> > > ERR_PTR(-ENOENT) in case this node does not exist so that the
> > > nvmem framework knows that it should not assign
> > > nvmem->dev.of_node to parent->of_node
> >
> > This is not good. First the NVMEM device is only a virtual concept
> > of the Linux kernel, it has no place in the DT.
>
> nvmem-cells is a virtual concept too, still, you define them in the
> DT.

To be honest I also think that naming this concept "nvmem" in the DT was
a bad idea. Perhaps something like "driver-data" or "data-cell" would
have been better as that would make it clear what this is about, nvmem
is just the Linux implementation of this concept.

> > Secondly the NVMEM
> > provider (here the MTD device) then has to manually parse its DT
> > node to find this subnode, pass it to the NVMEM framework to later
> > again resolve it back to the MTD device.
>
> We don't resolve it back to the MTD device, because the MTD device is
> just the parent of the nvmem device.
>
> > Not very complex but still a lot of
> > useless code, just registering the MTD device is a lot simpler and
> > much more inline with most other kernel API that register a
> > "service" available from a device.
>
> I'm not a big fan of this option either, but I thought I had to
> propose it.
>
> >
> > > 3/ only declare partitions as nvmem providers. This would solve
> > > the problem we have with partitions defined in the DT since
> > > defining sub-partitions in the DT is not (yet?) supported and
> > > partition nodes are supposed to be leaf nodes. Still, I'm not
> > > a big fan of this solution because it will prevent us from
> > > supporting sub-partitions if we ever want/need to.
> >
> > That sound like a poor workaround.
>
> Yes, that's a workaround. And the reason I propose it, is, again,
> because I don't want to block Bartosz.
>
> > Remember that this problem could
> > appear with any device that has a binding that use child nodes.
>
> I'm talking about partitions, and you're talking about mtd devices.
> Right now partitions don't have subnodes, and if we define that
> partition subnodes should describe nvmem-cells, then it becomes part
> of the official binding. So, no, the problem you mention does not
> (yet) exist.

That would add another binding that allow free named child nodes
without compatible string although experience has repeatedly shown that
this was a bad idea.

> >
> > > 4/ Add a ->of_xlate() hook that would be called if present by the
> > > framework instead of using the default parsing we have right
> > > now.
> >
> > That is a bit cleaner, but I don't think it would be worse the
> > complexity.
>
> But it's way more flexible than putting everything in the nvmem
> framework. BTW, did you notice that nvmem-cells parsing does not work
> with flashes bigger than 4GB, because the framework assumes
> #address-cells and #size-cells are always 1. That's probably something
> we'll have to fix for the MTD case.

Yes, however that's just an implementation limitation which is trivial
to solve.

> > Furthermore xlate functions are more about converting
> > from hardware parameters to internal kernel representation than to
> > hide extra DT parsing.
>
> Hm, how is that different? ->of_xlate() is just a way for drivers to
> have their own DT representation, which is exactly what we want here.

There is a big difference. DT represent the hardware and the
relationship between the devices in an OS independent format. We don't
add extra stuff in there just to map back internal Linux API details.

> >
> > > 5/ Tell the nvmem framework the name of the subnode containing
> > > nvmem cell definitions (if NULL that means cells are directly
> > > defined under the nvmem provider node). We would set it to
> > > "nvmem-cells" (or whatever you like) for the MTD case.
> >
> > If so please match on compatible and not on the node name.
>
> If you like.
>
> >
> > 6/ Extend the current NVMEM cell lookup to check if the parent node
> > of the cell has a compatible string set to "nvmem-cells". If it
> > doesn't it mean we have the current binding and this node is the
> > NVMEM device. If it does the device node is just the next parent.
> > This is trivial to implement (literally 2 lines of code) and cover
> > all the cases currently known.
>
> Except Srinivas was not happy with this solution, and this stalled the
> discussion. I'm trying to find other options and you keep rejecting
> all of them to come back to this one.

Well, I think this is the best solution :/

> >
> > 7/ Just add a compatible string to the nvmem cell. No code change is
> > needed,
>
> That's not true!!!

What is not true in this statement? The current nvmem lookup don't care
about compatible strings, so the cell lookup would just works fine. The
MTD partition parser won't consider them as a partition because of the
compatible string. Problem solved!

> What forces people to add this compatible in their
> DT? Nothing. I'll tell you what will happen: people will start
> defining their nvmem cells directly under the MTD node because that
> *works*, and even if the binding is not documented and we consider it
> invalid, we'll be stuck supporting it forever.

Do note that undocumented bindings are not allowed. DTS that use
undocumented bindings (normally) just get rejected.

> As said above, the
> very reason for option #1 to exist is to give you and Srinivas some
> more time to sort this out, while unblocking Bartosz in the meantime.

I'm fine with #1, I just didn't understood what it was useful for.

> > however as the nvmem cells have an address space (the offset in
> > byte in the storage) it might still clash with another address space
> > used by the main device biding (for example a number of child
> > functions).
> >
> > > There are probably other options (some were proposed by Alban and
> > > Srinivas already), but I'd like to get this sorted out before we
> > > merge this patch.
> > >
> > > Alban, Srinivas, any opinion?
> >
> > My preference goes to 6/ as it is trivial to implement, solves all
> > known shortcomings and is backward compatible with the current
> > binding. All other solutions have limitations and/or require too
> > complex implementations compared to what they try to solve.
>
> So we're back to square 1, and you're again blocking everything
> because you refuse to consider other options.

As I'm not a maintainer so I just can't block anything. But I won't lie
and pretend that I support a solution with known shortcomings.

Alban


Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2018-08-21 13:00:46

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 14:27:16 +0200
Alban <[email protected]> wrote:

> On Tue, 21 Aug 2018 07:44:04 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Tue, 21 Aug 2018 00:53:27 +0200
> > Alban <[email protected]> wrote:
> >
> > > On Sun, 19 Aug 2018 18:46:09 +0200
> > > Boris Brezillon <[email protected]> wrote:
> > >
> > > > On Sun, 19 Aug 2018 13:31:06 +0200
> > > > Alban <[email protected]> wrote:
> > > >
> > > > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > > > Boris Brezillon <[email protected]> wrote:
> > > > >
> > > > > > Hi Bartosz,
> > > > > >
> > > > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > > > Bartosz Golaszewski <[email protected]> wrote:
> > > > > >
> > > > > > > From: Alban Bedel <[email protected]>
> > > > > > >
> > > > > > > Allow drivers that use the nvmem API to read data stored on
> > > > > > > MTD devices. For this the mtd devices are registered as
> > > > > > > read-only NVMEM providers.
> > > > > > >
> > > > > > > Signed-off-by: Alban Bedel <[email protected]>
> > > > > > > [Bartosz:
> > > > > > > - use the managed variant of nvmem_register(),
> > > > > > > - set the nvmem name]
> > > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > > <[email protected]>
> > > > > >
> > > > > > What happened to the 2 other patches of Alban's series? I'd
> > > > > > really like the DT case to be handled/agreed on in the same
> > > > > > patchset, but IIRC, Alban and Srinivas disagreed on how this
> > > > > > should be represented. I hope this time we'll come to an
> > > > > > agreement, because the MTD <-> NVMEM glue has been floating
> > > > > > around for quite some time...
> > > > >
> > > > > These other patches were to fix what I consider a fundamental
> > > > > flaw in the generic NVMEM bindings, however we couldn't agree
> > > > > on this point. Bartosz later contacted me to take over this
> > > > > series and I suggested to just change the MTD NVMEM binding to
> > > > > use a compatible string on the NVMEM cells as an alternative
> > > > > solution to fix the clash with the old style MTD partition.
> > > > >
> > > > > However all this has no impact on the code needed to add NVMEM
> > > > > support to MTD, so the above patch didn't change at all.
> > > >
> > > > It does have an impact on the supported binding though.
> > > > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node,
> > > > which means people will be able to define their NVMEM cells
> > > > directly under the MTD device and reference them from other nodes
> > > > (even if it's not documented), and as you said, it conflict with
> > > > the old MTD partition bindings. So we'd better agree on this
> > > > binding before merging this patch.
> > >
> > > Unless the nvmem cell node has a compatible string, then it won't be
> > > considered as a partition by the MTD code. That is were the clash
> > > is, both bindings allow free named child nodes without a compatible
> > > string.
> >
> > Except the current nvmem cells parsing code does not enforce that, and
> > existing DTs rely on this behavior, so we're screwed. Or are you
> > suggesting to add a new "bool check_cells_compat;" field to
> > nvmem_config?
>
> There is no nvmem cell parsing at the moment. The DT lookup just
> resolve the phandle to the cell node, take the parent node and search
> for the nvmem provider that has this OF node. So extending it in case
> the node has a *new* compatible string would not break users of the old
> binding, none of them has a compatible string.

But we want to enforce the compat check on MTD devices, otherwise old
MTD partitions (those defined under the MTD node) will be considered as
NVMEM cells by the NVMEM framework. Hence the bool check_cells_compat
field.

>
> > >
> > > > I see several options:
> > > >
> > > > 1/ provide a way to tell the NVMEM framework not to use
> > > > parent->of_node even if it's != NULL. This way we really don't
> > > > support defining NVMEM cells in the DT, and also don't support
> > > > referencing the nvmem device using a phandle.
> > >
> > > I really don't get what the point of this would be. Make the whole
> > > API useless?
> >
> > No, just allow Bartosz to get his changes merged without waiting for
> > you and Srinivas to agree on how to handle the new binding. As I said
> > earlier, this mtd <-> nvmem stuff has been around for quite some time,
> > and instead of trying to find an approach that makes everyone happy,
> > you decided to let the patchset die.
>
> As long as that wouldn't prevent using DT in the future I'm fine with
> it.
>
> > >
> > > > 2/ define a new binding where all nvmem-cells are placed in an
> > > > "nvmem" subnode (just like we have this "partitions" subnode
> > > > for partitions), and then add a config->of_node field so that the
> > > > nvmem provider can explicitly specify the DT node representing
> > > > the nvmem device. We'll also need to set this field to
> > > > ERR_PTR(-ENOENT) in case this node does not exist so that the
> > > > nvmem framework knows that it should not assign
> > > > nvmem->dev.of_node to parent->of_node
> > >
> > > This is not good. First the NVMEM device is only a virtual concept
> > > of the Linux kernel, it has no place in the DT.
> >
> > nvmem-cells is a virtual concept too, still, you define them in the
> > DT.
>
> To be honest I also think that naming this concept "nvmem" in the DT was
> a bad idea. Perhaps something like "driver-data" or "data-cell" would
> have been better as that would make it clear what this is about, nvmem
> is just the Linux implementation of this concept.

I'm fine using a different name.

>
> > > Secondly the NVMEM
> > > provider (here the MTD device) then has to manually parse its DT
> > > node to find this subnode, pass it to the NVMEM framework to later
> > > again resolve it back to the MTD device.
> >
> > We don't resolve it back to the MTD device, because the MTD device is
> > just the parent of the nvmem device.
> >
> > > Not very complex but still a lot of
> > > useless code, just registering the MTD device is a lot simpler and
> > > much more inline with most other kernel API that register a
> > > "service" available from a device.
> >
> > I'm not a big fan of this option either, but I thought I had to
> > propose it.
> >
> > >
> > > > 3/ only declare partitions as nvmem providers. This would solve
> > > > the problem we have with partitions defined in the DT since
> > > > defining sub-partitions in the DT is not (yet?) supported and
> > > > partition nodes are supposed to be leaf nodes. Still, I'm not
> > > > a big fan of this solution because it will prevent us from
> > > > supporting sub-partitions if we ever want/need to.
> > >
> > > That sound like a poor workaround.
> >
> > Yes, that's a workaround. And the reason I propose it, is, again,
> > because I don't want to block Bartosz.
> >
> > > Remember that this problem could
> > > appear with any device that has a binding that use child nodes.
> >
> > I'm talking about partitions, and you're talking about mtd devices.
> > Right now partitions don't have subnodes, and if we define that
> > partition subnodes should describe nvmem-cells, then it becomes part
> > of the official binding. So, no, the problem you mention does not
> > (yet) exist.
>
> That would add another binding that allow free named child nodes
> without compatible string although experience has repeatedly shown that
> this was a bad idea.

Yes, I agree. Just thought it was important to have this solution in
the list, even if it's just to reject it.

>
> > >
> > > > 4/ Add a ->of_xlate() hook that would be called if present by the
> > > > framework instead of using the default parsing we have right
> > > > now.
> > >
> > > That is a bit cleaner, but I don't think it would be worse the
> > > complexity.
> >
> > But it's way more flexible than putting everything in the nvmem
> > framework. BTW, did you notice that nvmem-cells parsing does not work
> > with flashes bigger than 4GB, because the framework assumes
> > #address-cells and #size-cells are always 1. That's probably something
> > we'll have to fix for the MTD case.
>
> Yes, however that's just an implementation limitation which is trivial
> to solve.

Agree. I was just pointing it in case you hadn't noticed.

>
> > > Furthermore xlate functions are more about converting
> > > from hardware parameters to internal kernel representation than to
> > > hide extra DT parsing.
> >
> > Hm, how is that different? ->of_xlate() is just a way for drivers to
> > have their own DT representation, which is exactly what we want here.
>
> There is a big difference. DT represent the hardware and the
> relationship between the devices in an OS independent format. We don't
> add extra stuff in there just to map back internal Linux API details.

And I'm not talking about adding SW information in the DT, I'm talking
about HW specific description. We have the same solution for pinctrl
configs (it's HW/driver specific).

>
> > >
> > > > 5/ Tell the nvmem framework the name of the subnode containing
> > > > nvmem cell definitions (if NULL that means cells are directly
> > > > defined under the nvmem provider node). We would set it to
> > > > "nvmem-cells" (or whatever you like) for the MTD case.
> > >
> > > If so please match on compatible and not on the node name.
> >
> > If you like.
> >
> > >
> > > 6/ Extend the current NVMEM cell lookup to check if the parent node
> > > of the cell has a compatible string set to "nvmem-cells". If it
> > > doesn't it mean we have the current binding and this node is the
> > > NVMEM device. If it does the device node is just the next parent.
> > > This is trivial to implement (literally 2 lines of code) and cover
> > > all the cases currently known.
> >
> > Except Srinivas was not happy with this solution, and this stalled the
> > discussion. I'm trying to find other options and you keep rejecting
> > all of them to come back to this one.
>
> Well, I think this is the best solution :/
>
> > >
> > > 7/ Just add a compatible string to the nvmem cell. No code change is
> > > needed,
> >
> > That's not true!!!
>
> What is not true in this statement? The current nvmem lookup don't care
> about compatible strings, so the cell lookup would just works fine. The
> MTD partition parser won't consider them as a partition because of the
> compatible string. Problem solved!

No because partitions defined the old way (as direct subnodes of the MTD
node) will be considered as NVMEM cells by the NVMEM framework, and I
don't want that. Plus, I don't want people to start defining their
NVMEM cells and forget the compat string (which would work just fine
because the NVMEM framework doesn't care).

>
> > What forces people to add this compatible in their
> > DT? Nothing. I'll tell you what will happen: people will start
> > defining their nvmem cells directly under the MTD node because that
> > *works*, and even if the binding is not documented and we consider it
> > invalid, we'll be stuck supporting it forever.
>
> Do note that undocumented bindings are not allowed. DTS that use
> undocumented bindings (normally) just get rejected.

Except that's just in theory. In practice, if people can do something
wrong, they'll complain if you later fix the bug and break their setup.
So no, if we go for the "nvmem cells have an 'nvmem-cell' compat", then
I'd like the NVMEM framework to enforce that somehow.

2018-08-21 13:04:11

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 13:00:04 +0100
Srinivas Kandagatla <[email protected]> wrote:

> On 21/08/18 12:39, Alban wrote:
> > However we still have the a potential address space clash between the
> > nvmem cells and the main device binding.
> Can you elaborate?
>

Yes, I'd be interested in having a real example too, cause I don't see
what this address space clash is.

2018-08-21 13:40:15

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 21/08/18 14:34, Srinivas Kandagatla wrote:
>
>
> On 21/08/18 12:31, Boris Brezillon wrote:
>>> * struct nvmem_config - NVMEM device configuration
>>> @@ -58,6 +62,7 @@ struct nvmem_config {
>>> bool root_only;
>>> nvmem_reg_read_t reg_read;
>>> nvmem_reg_write_t reg_write;
>>> + nvmem_match_t match;
>>> int size;
>>> int word_size;
>>> int stride;
>>>
>> That might work if nvmem cells are defined directly under the mtdnode.
> Layout should not matter! which is the purpose of this callback.
>
> The only purpose of this callback is to tell nvmem core that the
> node(nvmem cell) belongs to that provider or not, if it is then we
> successfully found the provider. Its up to the provider on which layout
> it describes nvmem cells. Additionally the provider can add additional
> sanity checks in this match function to ensure that cell is correctly
> represented.
>
>
>> If we go for this approach, I'd recommend replacing this ->match() hook
>> by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
>> node, because what we're really after here is knowing which subnode is
>> an nvmem cell and which subnode is not.
>
> I agree on passing cell node instead of its parent. Regarding basic
> validating if its nvmem cell or not, we can check compatible string in
> nvmem core if we decide to use "nvmem-cell" compatible.
>
> Also just in case if you missed this, nvmem would not iterate the
Sorry !! i hit send button too quickly I guess.

What I meant to say here, is that nvmem core would not iterate the
provider node in any case.

Only time it looks at the cell node is when a consumer requests for the
cell.

--srini

2018-08-21 14:00:05

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 14:57:25 +0200
Boris Brezillon <[email protected]> wrote:

> On Tue, 21 Aug 2018 14:27:16 +0200
> Alban <[email protected]> wrote:
>
> > On Tue, 21 Aug 2018 07:44:04 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > On Tue, 21 Aug 2018 00:53:27 +0200
> > > Alban <[email protected]> wrote:
> > >
> > > > On Sun, 19 Aug 2018 18:46:09 +0200
> > > > Boris Brezillon <[email protected]> wrote:
> > > >
> > > > > On Sun, 19 Aug 2018 13:31:06 +0200
> > > > > Alban <[email protected]> wrote:
> > > > >
> > > > > > On Fri, 17 Aug 2018 18:27:20 +0200
> > > > > > Boris Brezillon <[email protected]> wrote:
> > > > > >
> > > > > > > Hi Bartosz,
> > > > > > >
> > > > > > > On Fri, 10 Aug 2018 10:05:03 +0200
> > > > > > > Bartosz Golaszewski <[email protected]> wrote:
> > > > > > >
> > > > > > > > From: Alban Bedel <[email protected]>
> > > > > > > >
> > > > > > > > Allow drivers that use the nvmem API to read data
> > > > > > > > stored on MTD devices. For this the mtd devices are
> > > > > > > > registered as read-only NVMEM providers.
> > > > > > > >
> > > > > > > > Signed-off-by: Alban Bedel <[email protected]>
> > > > > > > > [Bartosz:
> > > > > > > > - use the managed variant of nvmem_register(),
> > > > > > > > - set the nvmem name]
> > > > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > > > <[email protected]>
> > > > > > >
> > > > > > > What happened to the 2 other patches of Alban's series?
> > > > > > > I'd really like the DT case to be handled/agreed on in
> > > > > > > the same patchset, but IIRC, Alban and Srinivas disagreed
> > > > > > > on how this should be represented. I hope this time we'll
> > > > > > > come to an agreement, because the MTD <-> NVMEM glue has
> > > > > > > been floating around for quite some time...
> > > > > >
> > > > > > These other patches were to fix what I consider a
> > > > > > fundamental flaw in the generic NVMEM bindings, however we
> > > > > > couldn't agree on this point. Bartosz later contacted me to
> > > > > > take over this series and I suggested to just change the
> > > > > > MTD NVMEM binding to use a compatible string on the NVMEM
> > > > > > cells as an alternative solution to fix the clash with the
> > > > > > old style MTD partition.
> > > > > >
> > > > > > However all this has no impact on the code needed to add
> > > > > > NVMEM support to MTD, so the above patch didn't change at
> > > > > > all.
> > > > >
> > > > > It does have an impact on the supported binding though.
> > > > > nvmem->dev.of_node is automatically assigned to
> > > > > mtd->dev.of_node, which means people will be able to define
> > > > > their NVMEM cells directly under the MTD device and reference
> > > > > them from other nodes (even if it's not documented), and as
> > > > > you said, it conflict with the old MTD partition bindings. So
> > > > > we'd better agree on this binding before merging this
> > > > > patch.
> > > >
> > > > Unless the nvmem cell node has a compatible string, then it
> > > > won't be considered as a partition by the MTD code. That is
> > > > were the clash is, both bindings allow free named child nodes
> > > > without a compatible string.
> > >
> > > Except the current nvmem cells parsing code does not enforce
> > > that, and existing DTs rely on this behavior, so we're screwed.
> > > Or are you suggesting to add a new "bool check_cells_compat;"
> > > field to nvmem_config?
> >
> > There is no nvmem cell parsing at the moment. The DT lookup just
> > resolve the phandle to the cell node, take the parent node and
> > search for the nvmem provider that has this OF node. So extending
> > it in case the node has a *new* compatible string would not break
> > users of the old binding, none of them has a compatible string.
>
> But we want to enforce the compat check on MTD devices, otherwise old
> MTD partitions (those defined under the MTD node) will be considered
> as NVMEM cells by the NVMEM framework. Hence the bool
> check_cells_compat field.

That would only be needed if the NVMEM framework would do "forward"
parsing, creating data structure for each NVMEM cell found under an
NVMEM provider. However currently it doesn't do that and only goes
"backward", starting by resolving a phandle pointing to a cell, then
finding the provider that the cell belongs to.

This also has the side effect that nvmem cells defined in DT don't
appear in sysfs, unlike those defined from board code.

> >
> > > >
> > > > > I see several options:
> > > > >
> > > > > 1/ provide a way to tell the NVMEM framework not to use
> > > > > parent->of_node even if it's != NULL. This way we really don't
> > > > > support defining NVMEM cells in the DT, and also don't support
> > > > > referencing the nvmem device using a phandle.
> > > >
> > > > I really don't get what the point of this would be. Make the
> > > > whole API useless?
> > >
> > > No, just allow Bartosz to get his changes merged without waiting
> > > for you and Srinivas to agree on how to handle the new binding.
> > > As I said earlier, this mtd <-> nvmem stuff has been around for
> > > quite some time, and instead of trying to find an approach that
> > > makes everyone happy, you decided to let the patchset die.
> >
> > As long as that wouldn't prevent using DT in the future I'm fine
> > with it.
> >
> > > >
> > > > > 2/ define a new binding where all nvmem-cells are placed in an
> > > > > "nvmem" subnode (just like we have this "partitions"
> > > > > subnode for partitions), and then add a config->of_node field
> > > > > so that the nvmem provider can explicitly specify the DT node
> > > > > representing the nvmem device. We'll also need to set this
> > > > > field to ERR_PTR(-ENOENT) in case this node does not exist so
> > > > > that the nvmem framework knows that it should not assign
> > > > > nvmem->dev.of_node to parent->of_node
> > > >
> > > > This is not good. First the NVMEM device is only a virtual
> > > > concept of the Linux kernel, it has no place in the DT.
> > >
> > > nvmem-cells is a virtual concept too, still, you define them in
> > > the DT.
> >
> > To be honest I also think that naming this concept "nvmem" in the
> > DT was a bad idea. Perhaps something like "driver-data" or
> > "data-cell" would have been better as that would make it clear what
> > this is about, nvmem is just the Linux implementation of this
> > concept.
>
> I'm fine using a different name.
>
> >
> > > > Secondly the NVMEM
> > > > provider (here the MTD device) then has to manually parse its DT
> > > > node to find this subnode, pass it to the NVMEM framework to
> > > > later again resolve it back to the MTD device.
> > >
> > > We don't resolve it back to the MTD device, because the MTD
> > > device is just the parent of the nvmem device.
> > >
> > > > Not very complex but still a lot of
> > > > useless code, just registering the MTD device is a lot simpler
> > > > and much more inline with most other kernel API that register a
> > > > "service" available from a device.
> > >
> > > I'm not a big fan of this option either, but I thought I had to
> > > propose it.
> > >
> > > >
> > > > > 3/ only declare partitions as nvmem providers. This would
> > > > > solve the problem we have with partitions defined in the DT
> > > > > since defining sub-partitions in the DT is not (yet?)
> > > > > supported and partition nodes are supposed to be leaf nodes.
> > > > > Still, I'm not a big fan of this solution because it will
> > > > > prevent us from supporting sub-partitions if we ever
> > > > > want/need to.
> > > >
> > > > That sound like a poor workaround.
> > >
> > > Yes, that's a workaround. And the reason I propose it, is, again,
> > > because I don't want to block Bartosz.
> > >
> > > > Remember that this problem could
> > > > appear with any device that has a binding that use child
> > > > nodes.
> > >
> > > I'm talking about partitions, and you're talking about mtd
> > > devices. Right now partitions don't have subnodes, and if we
> > > define that partition subnodes should describe nvmem-cells, then
> > > it becomes part of the official binding. So, no, the problem you
> > > mention does not (yet) exist.
> >
> > That would add another binding that allow free named child nodes
> > without compatible string although experience has repeatedly shown
> > that this was a bad idea.
>
> Yes, I agree. Just thought it was important to have this solution in
> the list, even if it's just to reject it.
>
> >
> > > >
> > > > > 4/ Add a ->of_xlate() hook that would be called if present by
> > > > > the framework instead of using the default parsing we have
> > > > > right now.
> > > >
> > > > That is a bit cleaner, but I don't think it would be worse the
> > > > complexity.
> > >
> > > But it's way more flexible than putting everything in the nvmem
> > > framework. BTW, did you notice that nvmem-cells parsing does not
> > > work with flashes bigger than 4GB, because the framework assumes
> > > #address-cells and #size-cells are always 1. That's probably
> > > something we'll have to fix for the MTD case.
> >
> > Yes, however that's just an implementation limitation which is
> > trivial to solve.
>
> Agree. I was just pointing it in case you hadn't noticed.
>
> >
> > > > Furthermore xlate functions are more about converting
> > > > from hardware parameters to internal kernel representation than
> > > > to hide extra DT parsing.
> > >
> > > Hm, how is that different? ->of_xlate() is just a way for drivers
> > > to have their own DT representation, which is exactly what we
> > > want here.
> >
> > There is a big difference. DT represent the hardware and the
> > relationship between the devices in an OS independent format. We
> > don't add extra stuff in there just to map back internal Linux API
> > details.
>
> And I'm not talking about adding SW information in the DT, I'm talking
> about HW specific description. We have the same solution for pinctrl
> configs (it's HW/driver specific).

For pinctrl I do understand, these beast can be very different from SoC
to SoC, having a single biding for all doesn't make much sense.

However here we are talking about a simple linear storage, nothing
special at all. I could see the need for an xlate to for example
support a device with several partitions, but not to just allow each
driver to have slightly incompatible bindings.

> >
> > > >
> > > > > 5/ Tell the nvmem framework the name of the subnode containing
> > > > > nvmem cell definitions (if NULL that means cells are directly
> > > > > defined under the nvmem provider node). We would set it to
> > > > > "nvmem-cells" (or whatever you like) for the MTD case.
> > > >
> > > > If so please match on compatible and not on the node name.
> > >
> > > If you like.
> > >
> > > >
> > > > 6/ Extend the current NVMEM cell lookup to check if the parent
> > > > node of the cell has a compatible string set to "nvmem-cells".
> > > > If it doesn't it mean we have the current binding and this node
> > > > is the NVMEM device. If it does the device node is just the
> > > > next parent. This is trivial to implement (literally 2 lines of
> > > > code) and cover all the cases currently known.
> > >
> > > Except Srinivas was not happy with this solution, and this
> > > stalled the discussion. I'm trying to find other options and you
> > > keep rejecting all of them to come back to this one.
> >
> > Well, I think this is the best solution :/
> >
> > > >
> > > > 7/ Just add a compatible string to the nvmem cell. No code
> > > > change is needed,
> > >
> > > That's not true!!!
> >
> > What is not true in this statement? The current nvmem lookup don't
> > care about compatible strings, so the cell lookup would just works
> > fine. The MTD partition parser won't consider them as a partition
> > because of the compatible string. Problem solved!
>
> No because partitions defined the old way (as direct subnodes of the
> MTD node) will be considered as NVMEM cells by the NVMEM framework,
> and I don't want that.

As I explained above that is not currently the case. If the NVMEM,
framework is ever changed to explicitly parse NVMEM cells in advance
we can first update the few existing users to add the compatible string.

> Plus, I don't want people to start defining their NVMEM cells and
> forget the compat string (which would work just fine because the
> NVMEM framework doesn't care).

A review of a new DTS should check that it use each binding correctly,
AFAIK the DT people do that. We could also add a warning when there is
no compatible string, that would also help pushing people to update
their DTS.

> >
> > > What forces people to add this compatible in their
> > > DT? Nothing. I'll tell you what will happen: people will start
> > > defining their nvmem cells directly under the MTD node because
> > > that *works*, and even if the binding is not documented and we
> > > consider it invalid, we'll be stuck supporting it forever.
> >
> > Do note that undocumented bindings are not allowed. DTS that use
> > undocumented bindings (normally) just get rejected.
>
> Except that's just in theory. In practice, if people can do something
> wrong, they'll complain if you later fix the bug and break their
> setup. So no, if we go for the "nvmem cells have an 'nvmem-cell'
> compat", then I'd like the NVMEM framework to enforce that somehow.

That should be trivial to implement.

Alban


Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2018-08-21 14:00:46

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 14:37:37 +0100
Srinivas Kandagatla <[email protected]> wrote:

> On 21/08/18 14:34, Srinivas Kandagatla wrote:
> >
> >
> > On 21/08/18 12:31, Boris Brezillon wrote:
> >>> * struct nvmem_config - NVMEM device configuration
> >>> @@ -58,6 +62,7 @@ struct nvmem_config {
> >>> bool root_only;
> >>> nvmem_reg_read_t reg_read;
> >>> nvmem_reg_write_t reg_write;
> >>> + nvmem_match_t match;
> >>> int size;
> >>> int word_size;
> >>> int stride;
> >>>
> >> That might work if nvmem cells are defined directly under the mtdnode.
> > Layout should not matter! which is the purpose of this callback.
> >
> > The only purpose of this callback is to tell nvmem core that the
> > node(nvmem cell) belongs to that provider or not, if it is then we
> > successfully found the provider. Its up to the provider on which layout
> > it describes nvmem cells. Additionally the provider can add additional
> > sanity checks in this match function to ensure that cell is correctly
> > represented.
> >
> >
> >> If we go for this approach, I'd recommend replacing this ->match() hook
> >> by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
> >> node, because what we're really after here is knowing which subnode is
> >> an nvmem cell and which subnode is not.
> >
> > I agree on passing cell node instead of its parent. Regarding basic
> > validating if its nvmem cell or not, we can check compatible string in
> > nvmem core if we decide to use "nvmem-cell" compatible.
> >
> > Also just in case if you missed this, nvmem would not iterate the
> Sorry !! i hit send button too quickly I guess.
>
> What I meant to say here, is that nvmem core would not iterate the
> provider node in any case.
>
> Only time it looks at the cell node is when a consumer requests for the
> cell.

I did miss that, indeed. Thanks for the heads up.

So, the "old partitions being considered as nvmem cells" is not really
a problem, because those parts shouldn't be referenced.
This leaves us with the config->force_compat_check topic, which I'd
like to have to ensure that nvmem cells under MTD nodes actually have
compatible = "nvmem-cell" and prevent people from inadvertently
omitting this prop.

And of course, we need Rob's approval on this new binding :-).

2018-08-21 14:22:30

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 21/08/18 12:31, Boris Brezillon wrote:
>> * struct nvmem_config - NVMEM device configuration
>> @@ -58,6 +62,7 @@ struct nvmem_config {
>> bool root_only;
>> nvmem_reg_read_t reg_read;
>> nvmem_reg_write_t reg_write;
>> + nvmem_match_t match;
>> int size;
>> int word_size;
>> int stride;
>>
> That might work if nvmem cells are defined directly under the mtdnode.
Layout should not matter! which is the purpose of this callback.

The only purpose of this callback is to tell nvmem core that the
node(nvmem cell) belongs to that provider or not, if it is then we
successfully found the provider. Its up to the provider on which layout
it describes nvmem cells. Additionally the provider can add additional
sanity checks in this match function to ensure that cell is correctly
represented.


> If we go for this approach, I'd recommend replacing this ->match() hook
> by ->is_nvmem_cell() and pass it the cell node instead of the nvmem
> node, because what we're really after here is knowing which subnode is
> an nvmem cell and which subnode is not.

I agree on passing cell node instead of its parent. Regarding basic
validating if its nvmem cell or not, we can check compatible string in
nvmem core if we decide to use "nvmem-cell" compatible.

Also just in case if you missed this, nvmem would not iterate the

--srini

2018-08-21 14:29:13

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 15:57:06 +0200
Alban <[email protected]> wrote:

>
> That would only be needed if the NVMEM framework would do "forward"
> parsing, creating data structure for each NVMEM cell found under an
> NVMEM provider. However currently it doesn't do that and only goes
> "backward", starting by resolving a phandle pointing to a cell, then
> finding the provider that the cell belongs to.

Yes, I missed that when briefly looking at the code.

>
> This also has the side effect that nvmem cells defined in DT don't
> appear in sysfs, unlike those defined from board code.

Wow, that's not good. I guess we'll want to make that consistent at
some point.


> > > > > Furthermore xlate functions are more about converting
> > > > > from hardware parameters to internal kernel representation than
> > > > > to hide extra DT parsing.
> > > >
> > > > Hm, how is that different? ->of_xlate() is just a way for drivers
> > > > to have their own DT representation, which is exactly what we
> > > > want here.
> > >
> > > There is a big difference. DT represent the hardware and the
> > > relationship between the devices in an OS independent format. We
> > > don't add extra stuff in there just to map back internal Linux API
> > > details.
> >
> > And I'm not talking about adding SW information in the DT, I'm talking
> > about HW specific description. We have the same solution for pinctrl
> > configs (it's HW/driver specific).
>
> For pinctrl I do understand, these beast can be very different from SoC
> to SoC, having a single biding for all doesn't make much sense.
>
> However here we are talking about a simple linear storage, nothing
> special at all. I could see the need for an xlate to for example
> support a device with several partitions, but not to just allow each
> driver to have slightly incompatible bindings.

Maybe, but I guess that's up to the subsystem maintainer to decide what
he prefers.
> >
> > No because partitions defined the old way (as direct subnodes of the
> > MTD node) will be considered as NVMEM cells by the NVMEM framework,
> > and I don't want that.
>
> As I explained above that is not currently the case. If the NVMEM,
> framework is ever changed to explicitly parse NVMEM cells in advance
> we can first update the few existing users to add the compatible string.

We're supposed to be backward compatible (compatible with old DTs), so
that's not an option, though we could add a way to check the compat
string afterwards.

>
> > Plus, I don't want people to start defining their NVMEM cells and
> > forget the compat string (which would work just fine because the
> > NVMEM framework doesn't care).
>
> A review of a new DTS should check that it use each binding correctly,
> AFAIK the DT people do that. We could also add a warning when there is
> no compatible string, that would also help pushing people to update
> their DTS.

Yes, but I'd still prefer if we were preventing people from referencing
mtd-nvmem cells if the node does not have an "nvmem-cell" compat.

>
> > >
> > > > What forces people to add this compatible in their
> > > > DT? Nothing. I'll tell you what will happen: people will start
> > > > defining their nvmem cells directly under the MTD node because
> > > > that *works*, and even if the binding is not documented and we
> > > > consider it invalid, we'll be stuck supporting it forever.
> > >
> > > Do note that undocumented bindings are not allowed. DTS that use
> > > undocumented bindings (normally) just get rejected.
> >
> > Except that's just in theory. In practice, if people can do something
> > wrong, they'll complain if you later fix the bug and break their
> > setup. So no, if we go for the "nvmem cells have an 'nvmem-cell'
> > compat", then I'd like the NVMEM framework to enforce that somehow.
>
> That should be trivial to implement.

Exactly, and that's why I'm insisting on this point.

2018-08-21 14:35:43

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 21/08/18 15:26, Boris Brezillon wrote:
>> This also has the side effect that nvmem cells defined in DT don't
>> appear in sysfs, unlike those defined from board code.
> Wow, that's not good. I guess we'll want to make that consistent at
> some point.
>
support for sysfs entries per cell is not available in nvmem. However
there is nvmem sysfs entry per provider which can be read by the user
space if required.


--srini

2018-08-23 16:03:48

by Alban

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Tue, 21 Aug 2018 15:01:21 +0200
Boris Brezillon <[email protected]> wrote:

> On Tue, 21 Aug 2018 13:00:04 +0100
> Srinivas Kandagatla <[email protected]> wrote:
>
> > On 21/08/18 12:39, Alban wrote:
> > > However we still have the a potential address space clash between
> > > the nvmem cells and the main device binding.
> > Can you elaborate?
> >
>
> Yes, I'd be interested in having a real example too, cause I don't see
> what this address space clash is.

Let say I have a device that use the following binding:

device {
compatible = "example-device";
#address-cells = <2>;
#size-cells = <1>;

child@0,0 {
reg = <0x0 0x0>;
...
};

child@1,2 {
reg = <0x1 0x2>;
...
};

};

Now this binding already use the node address space for something,
so putting a nvmem node as direct child would not work. Here it is
quiet clear as we have 2 address cells, however even if the number of
cells and the cells size would match it would still be conceptually
wrong as both bindings then use the same address space for something
different.

Alban


Attachments:
(No filename) (836.00 B)
OpenPGP digital signature

2018-08-24 14:40:47

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API

On Thu, 23 Aug 2018 12:29:21 +0200
Alban <[email protected]> wrote:

> On Tue, 21 Aug 2018 15:01:21 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Tue, 21 Aug 2018 13:00:04 +0100
> > Srinivas Kandagatla <[email protected]> wrote:
> >
> > > On 21/08/18 12:39, Alban wrote:
> > > > However we still have the a potential address space clash between
> > > > the nvmem cells and the main device binding.
> > > Can you elaborate?
> > >
> >
> > Yes, I'd be interested in having a real example too, cause I don't see
> > what this address space clash is.
>
> Let say I have a device that use the following binding:
>
> device {
> compatible = "example-device";
> #address-cells = <2>;
> #size-cells = <1>;
>
> child@0,0 {
> reg = <0x0 0x0>;
> ...
> };
>
> child@1,2 {
> reg = <0x1 0x2>;
> ...
> };
>
> };
>
> Now this binding already use the node address space for something,
> so putting a nvmem node as direct child would not work. Here it is
> quiet clear as we have 2 address cells, however even if the number of
> cells and the cells size would match it would still be conceptually
> wrong as both bindings then use the same address space for something
> different.

When would we end up in this situation? MTD/flash nodes are supposed to
be leaf nodes (if you omit the partitions underneath). What would be
the case for defining subnodes there?

Do you know of any devices that uses such a representation?

2018-08-24 15:10:42

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

Hi Bartosz,

On Fri, 10 Aug 2018 10:04:58 +0200
Bartosz Golaszewski <[email protected]> wrote:

> +struct nvmem_cell_lookup {
> + struct nvmem_cell_info info;
> + struct list_head list;
> + const char *nvmem_name;
> +};

Hm, maybe I don't get it right, but this looks suspicious. Usually the
consumer lookup table is here to attach device specific names to
external resources.

So what I'd expect here is:

struct nvmem_cell_lookup {
/* The nvmem device name. */
const char *nvmem_name;

/* The nvmem cell name */
const char *nvmem_cell_name;

/*
* The local resource name. Basically what you have in the
* nvmem-cell-names prop.
*/
const char *conid;
};

struct nvmem_cell_lookup_table {
struct list_head list;

/* ID of the consumer device. */
const char *devid;

/* Array of cell lookup entries. */
unsigned int ncells;
const struct nvmem_cell_lookup *cells;
};

Looks like your nvmem_cell_lookup is more something used to attach cells
to an nvmem device, which is NVMEM provider's responsibility not the
consumer one.

Regards,

Boris

2018-08-24 15:31:07

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

On Fri, Aug 24, 2018 at 05:08:48PM +0200, Boris Brezillon wrote:
> Hi Bartosz,
>
> On Fri, 10 Aug 2018 10:04:58 +0200
> Bartosz Golaszewski <[email protected]> wrote:
>
> > +struct nvmem_cell_lookup {
> > + struct nvmem_cell_info info;
> > + struct list_head list;
> > + const char *nvmem_name;
> > +};
>
> Hm, maybe I don't get it right, but this looks suspicious. Usually the
> consumer lookup table is here to attach device specific names to
> external resources.
>
> So what I'd expect here is:
>
> struct nvmem_cell_lookup {
> /* The nvmem device name. */
> const char *nvmem_name;
>
> /* The nvmem cell name */
> const char *nvmem_cell_name;
>
> /*
> * The local resource name. Basically what you have in the
> * nvmem-cell-names prop.
> */
> const char *conid;
> };
>
> struct nvmem_cell_lookup_table {
> struct list_head list;
>
> /* ID of the consumer device. */
> const char *devid;
>
> /* Array of cell lookup entries. */
> unsigned int ncells;
> const struct nvmem_cell_lookup *cells;
> };
>
> Looks like your nvmem_cell_lookup is more something used to attach cells
> to an nvmem device, which is NVMEM provider's responsibility not the
> consumer one.

Hi Boris

There are cases where there is not a clear providier/consumer split. I
have an x86 platform, with a few at24 EEPROMs on it. It uses an off
the shelf Komtron module, placed on a custom carrier board. One of the
EEPROMs contains the hardware variant information. Once i know the
variant, i need to instantiate other I2C, SPI, MDIO devices, all using
platform devices, since this is x86, no DT available.

So the first thing my x86 platform device does is instantiate the
first i2c device for the AT24. Once the EEPROM pops into existence, i
need to add nvmem cells onto it. So at that point, the x86 platform
driver is playing the provider role. Once the cells are added, i can
then use nvmem consumer interfaces to get the contents of the cell,
run a checksum, and instantiate the other devices.

I wish the embedded world was all DT, but the reality is that it is
not :-(

Andrew

2018-08-25 06:30:30

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

On Fri, 24 Aug 2018 17:27:40 +0200
Andrew Lunn <[email protected]> wrote:

> On Fri, Aug 24, 2018 at 05:08:48PM +0200, Boris Brezillon wrote:
> > Hi Bartosz,
> >
> > On Fri, 10 Aug 2018 10:04:58 +0200
> > Bartosz Golaszewski <[email protected]> wrote:
> >
> > > +struct nvmem_cell_lookup {
> > > + struct nvmem_cell_info info;
> > > + struct list_head list;
> > > + const char *nvmem_name;
> > > +};
> >
> > Hm, maybe I don't get it right, but this looks suspicious. Usually the
> > consumer lookup table is here to attach device specific names to
> > external resources.
> >
> > So what I'd expect here is:
> >
> > struct nvmem_cell_lookup {
> > /* The nvmem device name. */
> > const char *nvmem_name;
> >
> > /* The nvmem cell name */
> > const char *nvmem_cell_name;
> >
> > /*
> > * The local resource name. Basically what you have in the
> > * nvmem-cell-names prop.
> > */
> > const char *conid;
> > };
> >
> > struct nvmem_cell_lookup_table {
> > struct list_head list;
> >
> > /* ID of the consumer device. */
> > const char *devid;
> >
> > /* Array of cell lookup entries. */
> > unsigned int ncells;
> > const struct nvmem_cell_lookup *cells;
> > };
> >
> > Looks like your nvmem_cell_lookup is more something used to attach cells
> > to an nvmem device, which is NVMEM provider's responsibility not the
> > consumer one.
>
> Hi Boris
>
> There are cases where there is not a clear providier/consumer split. I
> have an x86 platform, with a few at24 EEPROMs on it. It uses an off
> the shelf Komtron module, placed on a custom carrier board. One of the
> EEPROMs contains the hardware variant information. Once i know the
> variant, i need to instantiate other I2C, SPI, MDIO devices, all using
> platform devices, since this is x86, no DT available.
>
> So the first thing my x86 platform device does is instantiate the
> first i2c device for the AT24. Once the EEPROM pops into existence, i
> need to add nvmem cells onto it. So at that point, the x86 platform
> driver is playing the provider role. Once the cells are added, i can
> then use nvmem consumer interfaces to get the contents of the cell,
> run a checksum, and instantiate the other devices.
>
> I wish the embedded world was all DT, but the reality is that it is
> not :-(

Actually, I'm not questioning the need for this feature (being able to
attach NVMEM cells to an NVMEM device on a platform that does not use
DT). What I'm saying is that this functionality is provider related,
not consumer related. Also, I wonder if defining such NVMEM cells
shouldn't go through the provider driver instead of being passed
directly to the NVMEM layer, because nvmem_config already have a fields
to pass cells at registration time, plus, the name of the NVMEM cell
device is sometimes created dynamically and can be hard to guess at
platform_device registration time.

I also think non-DT consumers will need a way to reference exiting
NVMEM cells, but this consumer-oriented nvmem cell lookup table should
look like the gpio or pwm lookup table (basically what I proposed in my
previous email).

2018-08-27 08:57:51

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

2018-08-25 8:27 GMT+02:00 Boris Brezillon <[email protected]>:
> On Fri, 24 Aug 2018 17:27:40 +0200
> Andrew Lunn <[email protected]> wrote:
>
>> On Fri, Aug 24, 2018 at 05:08:48PM +0200, Boris Brezillon wrote:
>> > Hi Bartosz,
>> >
>> > On Fri, 10 Aug 2018 10:04:58 +0200
>> > Bartosz Golaszewski <[email protected]> wrote:
>> >
>> > > +struct nvmem_cell_lookup {
>> > > + struct nvmem_cell_info info;
>> > > + struct list_head list;
>> > > + const char *nvmem_name;
>> > > +};
>> >
>> > Hm, maybe I don't get it right, but this looks suspicious. Usually the
>> > consumer lookup table is here to attach device specific names to
>> > external resources.
>> >
>> > So what I'd expect here is:
>> >
>> > struct nvmem_cell_lookup {
>> > /* The nvmem device name. */
>> > const char *nvmem_name;
>> >
>> > /* The nvmem cell name */
>> > const char *nvmem_cell_name;
>> >
>> > /*
>> > * The local resource name. Basically what you have in the
>> > * nvmem-cell-names prop.
>> > */
>> > const char *conid;
>> > };
>> >
>> > struct nvmem_cell_lookup_table {
>> > struct list_head list;
>> >
>> > /* ID of the consumer device. */
>> > const char *devid;
>> >
>> > /* Array of cell lookup entries. */
>> > unsigned int ncells;
>> > const struct nvmem_cell_lookup *cells;
>> > };
>> >
>> > Looks like your nvmem_cell_lookup is more something used to attach cells
>> > to an nvmem device, which is NVMEM provider's responsibility not the
>> > consumer one.
>>
>> Hi Boris
>>
>> There are cases where there is not a clear providier/consumer split. I
>> have an x86 platform, with a few at24 EEPROMs on it. It uses an off
>> the shelf Komtron module, placed on a custom carrier board. One of the
>> EEPROMs contains the hardware variant information. Once i know the
>> variant, i need to instantiate other I2C, SPI, MDIO devices, all using
>> platform devices, since this is x86, no DT available.
>>
>> So the first thing my x86 platform device does is instantiate the
>> first i2c device for the AT24. Once the EEPROM pops into existence, i
>> need to add nvmem cells onto it. So at that point, the x86 platform
>> driver is playing the provider role. Once the cells are added, i can
>> then use nvmem consumer interfaces to get the contents of the cell,
>> run a checksum, and instantiate the other devices.
>>
>> I wish the embedded world was all DT, but the reality is that it is
>> not :-(
>
> Actually, I'm not questioning the need for this feature (being able to
> attach NVMEM cells to an NVMEM device on a platform that does not use
> DT). What I'm saying is that this functionality is provider related,
> not consumer related. Also, I wonder if defining such NVMEM cells
> shouldn't go through the provider driver instead of being passed
> directly to the NVMEM layer, because nvmem_config already have a fields
> to pass cells at registration time, plus, the name of the NVMEM cell
> device is sometimes created dynamically and can be hard to guess at
> platform_device registration time.
>

In my use case the provider is at24 EEPROM driver. This is where the
nvmem_config lives but I can't image a correct and clean way of
passing this cell config to the driver from board files without using
new ugly fields in platform_data which this very series is trying to
remove. This is why this cell config should live in machine code.

> I also think non-DT consumers will need a way to reference exiting
> NVMEM cells, but this consumer-oriented nvmem cell lookup table should
> look like the gpio or pwm lookup table (basically what I proposed in my
> previous email).

How about introducing two new interfaces to nvmem: one for defining
nvmem cells from machine code and the second for connecting these
cells with devices?

Best regards,
Bart

2018-08-27 09:02:24

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

On Mon, 27 Aug 2018 10:56:29 +0200
Bartosz Golaszewski <[email protected]> wrote:

> 2018-08-25 8:27 GMT+02:00 Boris Brezillon <[email protected]>:
> > On Fri, 24 Aug 2018 17:27:40 +0200
> > Andrew Lunn <[email protected]> wrote:
> >
> >> On Fri, Aug 24, 2018 at 05:08:48PM +0200, Boris Brezillon wrote:
> >> > Hi Bartosz,
> >> >
> >> > On Fri, 10 Aug 2018 10:04:58 +0200
> >> > Bartosz Golaszewski <[email protected]> wrote:
> >> >
> >> > > +struct nvmem_cell_lookup {
> >> > > + struct nvmem_cell_info info;
> >> > > + struct list_head list;
> >> > > + const char *nvmem_name;
> >> > > +};
> >> >
> >> > Hm, maybe I don't get it right, but this looks suspicious. Usually the
> >> > consumer lookup table is here to attach device specific names to
> >> > external resources.
> >> >
> >> > So what I'd expect here is:
> >> >
> >> > struct nvmem_cell_lookup {
> >> > /* The nvmem device name. */
> >> > const char *nvmem_name;
> >> >
> >> > /* The nvmem cell name */
> >> > const char *nvmem_cell_name;
> >> >
> >> > /*
> >> > * The local resource name. Basically what you have in the
> >> > * nvmem-cell-names prop.
> >> > */
> >> > const char *conid;
> >> > };
> >> >
> >> > struct nvmem_cell_lookup_table {
> >> > struct list_head list;
> >> >
> >> > /* ID of the consumer device. */
> >> > const char *devid;
> >> >
> >> > /* Array of cell lookup entries. */
> >> > unsigned int ncells;
> >> > const struct nvmem_cell_lookup *cells;
> >> > };
> >> >
> >> > Looks like your nvmem_cell_lookup is more something used to attach cells
> >> > to an nvmem device, which is NVMEM provider's responsibility not the
> >> > consumer one.
> >>
> >> Hi Boris
> >>
> >> There are cases where there is not a clear providier/consumer split. I
> >> have an x86 platform, with a few at24 EEPROMs on it. It uses an off
> >> the shelf Komtron module, placed on a custom carrier board. One of the
> >> EEPROMs contains the hardware variant information. Once i know the
> >> variant, i need to instantiate other I2C, SPI, MDIO devices, all using
> >> platform devices, since this is x86, no DT available.
> >>
> >> So the first thing my x86 platform device does is instantiate the
> >> first i2c device for the AT24. Once the EEPROM pops into existence, i
> >> need to add nvmem cells onto it. So at that point, the x86 platform
> >> driver is playing the provider role. Once the cells are added, i can
> >> then use nvmem consumer interfaces to get the contents of the cell,
> >> run a checksum, and instantiate the other devices.
> >>
> >> I wish the embedded world was all DT, but the reality is that it is
> >> not :-(
> >
> > Actually, I'm not questioning the need for this feature (being able to
> > attach NVMEM cells to an NVMEM device on a platform that does not use
> > DT). What I'm saying is that this functionality is provider related,
> > not consumer related. Also, I wonder if defining such NVMEM cells
> > shouldn't go through the provider driver instead of being passed
> > directly to the NVMEM layer, because nvmem_config already have a fields
> > to pass cells at registration time, plus, the name of the NVMEM cell
> > device is sometimes created dynamically and can be hard to guess at
> > platform_device registration time.
> >
>
> In my use case the provider is at24 EEPROM driver. This is where the
> nvmem_config lives but I can't image a correct and clean way of
> passing this cell config to the driver from board files without using
> new ugly fields in platform_data which this very series is trying to
> remove. This is why this cell config should live in machine code.

Okay.

>
> > I also think non-DT consumers will need a way to reference exiting
> > NVMEM cells, but this consumer-oriented nvmem cell lookup table should
> > look like the gpio or pwm lookup table (basically what I proposed in my
> > previous email).
>
> How about introducing two new interfaces to nvmem: one for defining
> nvmem cells from machine code and the second for connecting these
> cells with devices?

Yes, that's basically what I was suggesting: move what you've done in
nvmem-provider.h (maybe rename some of the structs to make it clear
that this is about defining cells not referencing existing ones), and
add a new consumer interface (based on what other subsystems do) in
nvmem-consumer.h.

This way you have both things clearly separated, and if a driver is
both a consumer and a provider you'll just have to include both headers.

Regards,

Boris

2018-08-27 13:38:51

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

2018-08-27 11:00 GMT+02:00 Boris Brezillon <[email protected]>:
> On Mon, 27 Aug 2018 10:56:29 +0200
> Bartosz Golaszewski <[email protected]> wrote:
>
>> 2018-08-25 8:27 GMT+02:00 Boris Brezillon <[email protected]>:
>> > On Fri, 24 Aug 2018 17:27:40 +0200
>> > Andrew Lunn <[email protected]> wrote:
>> >
>> >> On Fri, Aug 24, 2018 at 05:08:48PM +0200, Boris Brezillon wrote:
>> >> > Hi Bartosz,
>> >> >
>> >> > On Fri, 10 Aug 2018 10:04:58 +0200
>> >> > Bartosz Golaszewski <[email protected]> wrote:
>> >> >
>> >> > > +struct nvmem_cell_lookup {
>> >> > > + struct nvmem_cell_info info;
>> >> > > + struct list_head list;
>> >> > > + const char *nvmem_name;
>> >> > > +};
>> >> >
>> >> > Hm, maybe I don't get it right, but this looks suspicious. Usually the
>> >> > consumer lookup table is here to attach device specific names to
>> >> > external resources.
>> >> >
>> >> > So what I'd expect here is:
>> >> >
>> >> > struct nvmem_cell_lookup {
>> >> > /* The nvmem device name. */
>> >> > const char *nvmem_name;
>> >> >
>> >> > /* The nvmem cell name */
>> >> > const char *nvmem_cell_name;
>> >> >
>> >> > /*
>> >> > * The local resource name. Basically what you have in the
>> >> > * nvmem-cell-names prop.
>> >> > */
>> >> > const char *conid;
>> >> > };
>> >> >
>> >> > struct nvmem_cell_lookup_table {
>> >> > struct list_head list;
>> >> >
>> >> > /* ID of the consumer device. */
>> >> > const char *devid;
>> >> >
>> >> > /* Array of cell lookup entries. */
>> >> > unsigned int ncells;
>> >> > const struct nvmem_cell_lookup *cells;
>> >> > };
>> >> >
>> >> > Looks like your nvmem_cell_lookup is more something used to attach cells
>> >> > to an nvmem device, which is NVMEM provider's responsibility not the
>> >> > consumer one.
>> >>
>> >> Hi Boris
>> >>
>> >> There are cases where there is not a clear providier/consumer split. I
>> >> have an x86 platform, with a few at24 EEPROMs on it. It uses an off
>> >> the shelf Komtron module, placed on a custom carrier board. One of the
>> >> EEPROMs contains the hardware variant information. Once i know the
>> >> variant, i need to instantiate other I2C, SPI, MDIO devices, all using
>> >> platform devices, since this is x86, no DT available.
>> >>
>> >> So the first thing my x86 platform device does is instantiate the
>> >> first i2c device for the AT24. Once the EEPROM pops into existence, i
>> >> need to add nvmem cells onto it. So at that point, the x86 platform
>> >> driver is playing the provider role. Once the cells are added, i can
>> >> then use nvmem consumer interfaces to get the contents of the cell,
>> >> run a checksum, and instantiate the other devices.
>> >>
>> >> I wish the embedded world was all DT, but the reality is that it is
>> >> not :-(
>> >
>> > Actually, I'm not questioning the need for this feature (being able to
>> > attach NVMEM cells to an NVMEM device on a platform that does not use
>> > DT). What I'm saying is that this functionality is provider related,
>> > not consumer related. Also, I wonder if defining such NVMEM cells
>> > shouldn't go through the provider driver instead of being passed
>> > directly to the NVMEM layer, because nvmem_config already have a fields
>> > to pass cells at registration time, plus, the name of the NVMEM cell
>> > device is sometimes created dynamically and can be hard to guess at
>> > platform_device registration time.
>> >
>>
>> In my use case the provider is at24 EEPROM driver. This is where the
>> nvmem_config lives but I can't image a correct and clean way of
>> passing this cell config to the driver from board files without using
>> new ugly fields in platform_data which this very series is trying to
>> remove. This is why this cell config should live in machine code.
>
> Okay.
>
>>
>> > I also think non-DT consumers will need a way to reference exiting
>> > NVMEM cells, but this consumer-oriented nvmem cell lookup table should
>> > look like the gpio or pwm lookup table (basically what I proposed in my
>> > previous email).
>>
>> How about introducing two new interfaces to nvmem: one for defining
>> nvmem cells from machine code and the second for connecting these
>> cells with devices?
>
> Yes, that's basically what I was suggesting: move what you've done in
> nvmem-provider.h (maybe rename some of the structs to make it clear
> that this is about defining cells not referencing existing ones), and
> add a new consumer interface (based on what other subsystems do) in
> nvmem-consumer.h.
>
> This way you have both things clearly separated, and if a driver is
> both a consumer and a provider you'll just have to include both headers.
>
> Regards,
>
> Boris

I didn't notice it before but there's a global list of nvmem cells
with each cell referencing its owner nvmem device. I'm wondering if
this isn't some kind of inversion of ownership. Shouldn't each nvmem
device have a separate list of nvmem cells owned by it? What happens
if we have two nvmem providers with the same names for cells? I'm
asking because dev_id based lookup doesn't make sense if internally
nvmem_cell_get_from_list() doesn't care about any device names (takes
only the cell_id as argument).

This doesn't cause any trouble now since there are no users defining
cells in nvmem_config - there are only DT users - but this must be
clarified before I can advance with correctly implementing nvmem
lookups.

BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
instance even if the cell for this node was already added to the nvmem
device.

Bart

2018-08-27 14:03:18

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

On Mon, 27 Aug 2018 15:37:23 +0200
Bartosz Golaszewski <[email protected]> wrote:

> 2018-08-27 11:00 GMT+02:00 Boris Brezillon <[email protected]>:
> > On Mon, 27 Aug 2018 10:56:29 +0200
> > Bartosz Golaszewski <[email protected]> wrote:
> >
> >> 2018-08-25 8:27 GMT+02:00 Boris Brezillon <[email protected]>:
> >> > On Fri, 24 Aug 2018 17:27:40 +0200
> >> > Andrew Lunn <[email protected]> wrote:
> >> >
> >> >> On Fri, Aug 24, 2018 at 05:08:48PM +0200, Boris Brezillon wrote:
> >> >> > Hi Bartosz,
> >> >> >
> >> >> > On Fri, 10 Aug 2018 10:04:58 +0200
> >> >> > Bartosz Golaszewski <[email protected]> wrote:
> >> >> >
> >> >> > > +struct nvmem_cell_lookup {
> >> >> > > + struct nvmem_cell_info info;
> >> >> > > + struct list_head list;
> >> >> > > + const char *nvmem_name;
> >> >> > > +};
> >> >> >
> >> >> > Hm, maybe I don't get it right, but this looks suspicious. Usually the
> >> >> > consumer lookup table is here to attach device specific names to
> >> >> > external resources.
> >> >> >
> >> >> > So what I'd expect here is:
> >> >> >
> >> >> > struct nvmem_cell_lookup {
> >> >> > /* The nvmem device name. */
> >> >> > const char *nvmem_name;
> >> >> >
> >> >> > /* The nvmem cell name */
> >> >> > const char *nvmem_cell_name;
> >> >> >
> >> >> > /*
> >> >> > * The local resource name. Basically what you have in the
> >> >> > * nvmem-cell-names prop.
> >> >> > */
> >> >> > const char *conid;
> >> >> > };
> >> >> >
> >> >> > struct nvmem_cell_lookup_table {
> >> >> > struct list_head list;
> >> >> >
> >> >> > /* ID of the consumer device. */
> >> >> > const char *devid;
> >> >> >
> >> >> > /* Array of cell lookup entries. */
> >> >> > unsigned int ncells;
> >> >> > const struct nvmem_cell_lookup *cells;
> >> >> > };
> >> >> >
> >> >> > Looks like your nvmem_cell_lookup is more something used to attach cells
> >> >> > to an nvmem device, which is NVMEM provider's responsibility not the
> >> >> > consumer one.
> >> >>
> >> >> Hi Boris
> >> >>
> >> >> There are cases where there is not a clear providier/consumer split. I
> >> >> have an x86 platform, with a few at24 EEPROMs on it. It uses an off
> >> >> the shelf Komtron module, placed on a custom carrier board. One of the
> >> >> EEPROMs contains the hardware variant information. Once i know the
> >> >> variant, i need to instantiate other I2C, SPI, MDIO devices, all using
> >> >> platform devices, since this is x86, no DT available.
> >> >>
> >> >> So the first thing my x86 platform device does is instantiate the
> >> >> first i2c device for the AT24. Once the EEPROM pops into existence, i
> >> >> need to add nvmem cells onto it. So at that point, the x86 platform
> >> >> driver is playing the provider role. Once the cells are added, i can
> >> >> then use nvmem consumer interfaces to get the contents of the cell,
> >> >> run a checksum, and instantiate the other devices.
> >> >>
> >> >> I wish the embedded world was all DT, but the reality is that it is
> >> >> not :-(
> >> >
> >> > Actually, I'm not questioning the need for this feature (being able to
> >> > attach NVMEM cells to an NVMEM device on a platform that does not use
> >> > DT). What I'm saying is that this functionality is provider related,
> >> > not consumer related. Also, I wonder if defining such NVMEM cells
> >> > shouldn't go through the provider driver instead of being passed
> >> > directly to the NVMEM layer, because nvmem_config already have a fields
> >> > to pass cells at registration time, plus, the name of the NVMEM cell
> >> > device is sometimes created dynamically and can be hard to guess at
> >> > platform_device registration time.
> >> >
> >>
> >> In my use case the provider is at24 EEPROM driver. This is where the
> >> nvmem_config lives but I can't image a correct and clean way of
> >> passing this cell config to the driver from board files without using
> >> new ugly fields in platform_data which this very series is trying to
> >> remove. This is why this cell config should live in machine code.
> >
> > Okay.
> >
> >>
> >> > I also think non-DT consumers will need a way to reference exiting
> >> > NVMEM cells, but this consumer-oriented nvmem cell lookup table should
> >> > look like the gpio or pwm lookup table (basically what I proposed in my
> >> > previous email).
> >>
> >> How about introducing two new interfaces to nvmem: one for defining
> >> nvmem cells from machine code and the second for connecting these
> >> cells with devices?
> >
> > Yes, that's basically what I was suggesting: move what you've done in
> > nvmem-provider.h (maybe rename some of the structs to make it clear
> > that this is about defining cells not referencing existing ones), and
> > add a new consumer interface (based on what other subsystems do) in
> > nvmem-consumer.h.
> >
> > This way you have both things clearly separated, and if a driver is
> > both a consumer and a provider you'll just have to include both headers.
> >
> > Regards,
> >
> > Boris
>
> I didn't notice it before but there's a global list of nvmem cells
> with each cell referencing its owner nvmem device. I'm wondering if
> this isn't some kind of inversion of ownership. Shouldn't each nvmem
> device have a separate list of nvmem cells owned by it? What happens
> if we have two nvmem providers with the same names for cells? I'm
> asking because dev_id based lookup doesn't make sense if internally
> nvmem_cell_get_from_list() doesn't care about any device names (takes
> only the cell_id as argument).
>
> This doesn't cause any trouble now since there are no users defining
> cells in nvmem_config - there are only DT users - but this must be
> clarified before I can advance with correctly implementing nvmem
> lookups.
>
> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
> instance even if the cell for this node was already added to the nvmem
> device.

Yep, don't know if it's done on purpose or not, but it's weird. I'd
expect cells to be instantiated at NVMEM registration time (and stored
in a list attached to the device) and then, anytime someone calls
nvmem_cell_get(), you would search in this list for a match.


2018-08-28 10:16:42

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups


On 27/08/18 14:37, Bartosz Golaszewski wrote:
> I didn't notice it before but there's a global list of nvmem cells

Bit of history here.

The global list of nvmem_cell is to assist non device tree based cell
lookups. These cell entries come as part of the non-dt providers
nvmem_config.

All the device tree based cell lookup happen dynamically on
request/demand, and all the cell definition comes from DT.

As of today NVMEM supports both DT and non DT usecase, this is much simpler.

Non dt cases have various consumer usecases.

1> Consumer is aware of provider name and cell details.
This is probably simple usecase where it can just use device based apis.

2> Consumer is not aware of provider name, its just aware of cell name.
This is the case where global list of cells are used.

> with each cell referencing its owner nvmem device. I'm wondering if
> this isn't some kind of inversion of ownership. Shouldn't each nvmem
> device have a separate list of nvmem cells owned by it? What happens
This is mainly done for use case where consumer does not have idea of
provider name or any details.

First thing non dt user should do is use "NVMEM device based consumer APIs"

ex: First get handle to nvmem device using its nvmem provider name by
calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.

Also am not 100% sure how would maintaining cells list per nvmem
provider would help for the intended purpose of global list?

> if we have two nvmem providers with the same names for cells? I'm
Yes, it would return the first instance.. which is a known issue.
Am not really sure this is a big problem as of today! but am open for
any better suggestions!


> asking because dev_id based lookup doesn't make sense if internally
> nvmem_cell_get_from_list() doesn't care about any device names (takes
> only the cell_id as argument).

As I said this is for non DT usecase where consumers are not aware of
provider details.

>
> This doesn't cause any trouble now since there are no users defining
> cells in nvmem_config - there are only DT users - but this must be
> clarified before I can advance with correctly implementing nvmem
> lookups.
DT users should not be defining this to start with! It's redundant and
does not make sense!

>
> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
> instance even if the cell for this node was already added to the nvmem
> device.
I hope you got the reason why of_nvmem_cell_get() always allocates new
instance for every get!!

thanks,
srini

2018-08-28 10:23:30

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API



On 23/08/18 11:29, Alban wrote:
> Let say I have a device that use the following binding:
>
> device {
> compatible = "example-device";
> #address-cells = <2>;
> #size-cells = <1>;
>
> child@0,0 {
> reg = <0x0 0x0>;
> ...
> };
>
> child@1,2 {
> reg = <0x1 0x2>;
> ...
> };
>
> };
>
> Now this binding already use the node address space for something,
> so putting a nvmem node as direct child would not work.

AFAIK, It should work but the we would get a DT warning this, as nvmem
does not use of_address based apis to parse this. Which should be
totally fixed!!

As discussed before once we add support to #address-cells and
#size-cells in nvmem core this should not be a problem.


--srini
Here it is
> quiet clear as we have 2 address cells, however even if the number of
> cells and the cells size would match it would still be conceptually
> wrong as both bindings then use the same address space for something
> different.

2018-08-28 11:57:58

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
> On 27/08/18 14:37, Bartosz Golaszewski wrote:
>>
>> I didn't notice it before but there's a global list of nvmem cells
>
>
> Bit of history here.
>
> The global list of nvmem_cell is to assist non device tree based cell
> lookups. These cell entries come as part of the non-dt providers
> nvmem_config.
>
> All the device tree based cell lookup happen dynamically on request/demand,
> and all the cell definition comes from DT.
>

Makes perfect sense.

> As of today NVMEM supports both DT and non DT usecase, this is much simpler.
>
> Non dt cases have various consumer usecases.
>
> 1> Consumer is aware of provider name and cell details.
> This is probably simple usecase where it can just use device based
> apis.
>
> 2> Consumer is not aware of provider name, its just aware of cell name.
> This is the case where global list of cells are used.
>

I would like to support an additional use case here: the provider is
generic and is not aware of its cells at all. Since the only way of
defining nvmem cells is through DT or nvmem_config, we lack a way to
allow machine code to define cells without the provider code being
aware.

>> with each cell referencing its owner nvmem device. I'm wondering if
>> this isn't some kind of inversion of ownership. Shouldn't each nvmem
>> device have a separate list of nvmem cells owned by it? What happens
>
> This is mainly done for use case where consumer does not have idea of
> provider name or any details.
>

It doesn't need to know the provider details, but in most subsystems
the core code associates such resources by dev_id and optional con_id
as Boris already said.

> First thing non dt user should do is use "NVMEM device based consumer APIs"
>
> ex: First get handle to nvmem device using its nvmem provider name by
> calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.
>
> Also am not 100% sure how would maintaining cells list per nvmem provider
> would help for the intended purpose of global list?
>

It would fix the use case where the consumer wants to use
nvmem_cell_get(dev, name) and two nvmem providers would have a cell
with the same name.

Next we could add a way to associate dev_ids with nvmem cells.

>> if we have two nvmem providers with the same names for cells? I'm
>
> Yes, it would return the first instance.. which is a known issue.
> Am not really sure this is a big problem as of today! but am open for any
> better suggestions!
>

Yes, I would like to rework nvmem a bit. I don't see any non-DT users
defining nvmem-cells using nvmem_config. I think that what we need is
a way of specifying cell config outside of nvmem providers in some
kind of structures. These tables would reference the provider by name
and define the cells. Then we would have an additional lookup
structure which would associate the consumer (by dev_id and con_id,
where dev_id could optionally be NULL and where we would fall back to
using con_id only) and the nvmem provider + cell together. Similarly
to how GPIO consumers are associated with the gpiochip and hwnum. How
does it sound?

>
>> asking because dev_id based lookup doesn't make sense if internally
>> nvmem_cell_get_from_list() doesn't care about any device names (takes
>> only the cell_id as argument).
>
>
> As I said this is for non DT usecase where consumers are not aware of
> provider details.
>
>>
>> This doesn't cause any trouble now since there are no users defining
>> cells in nvmem_config - there are only DT users - but this must be
>> clarified before I can advance with correctly implementing nvmem
>> lookups.
>
> DT users should not be defining this to start with! It's redundant and does
> not make sense!
>

Yes, this is what I said: we only seem to have DT users, so this API
is not used at the moment.

>>
>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>> instance even if the cell for this node was already added to the nvmem
>> device.
>
> I hope you got the reason why of_nvmem_cell_get() always allocates new
> instance for every get!!


I admit I didn't test it, but just from reading the code it seems like
in nvmem_cell_get() for DT-users we'll always get to
of_nvmem_cell_get() and in there we always end up calling line 873:
cell = kzalloc(sizeof(*cell), GFP_KERNEL);

There may be something I'm missing though.

>
> thanks,
> srini

BR
Bart

2018-08-28 13:47:26

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups



On 28/08/18 12:56, Bartosz Golaszewski wrote:
> 2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>>
>> On 27/08/18 14:37, Bartosz Golaszewski wrote:
>>>
>>> I didn't notice it before but there's a global list of nvmem cells
>>
>>
>> Bit of history here.
>>
>> The global list of nvmem_cell is to assist non device tree based cell
>> lookups. These cell entries come as part of the non-dt providers
>> nvmem_config.
>>
>> All the device tree based cell lookup happen dynamically on request/demand,
>> and all the cell definition comes from DT.
>>
>
> Makes perfect sense.
>
>> As of today NVMEM supports both DT and non DT usecase, this is much simpler.
>>
>> Non dt cases have various consumer usecases.
>>
>> 1> Consumer is aware of provider name and cell details.
>> This is probably simple usecase where it can just use device based
>> apis.
>>
>> 2> Consumer is not aware of provider name, its just aware of cell name.
>> This is the case where global list of cells are used.
>>
>
> I would like to support an additional use case here: the provider is
> generic and is not aware of its cells at all. Since the only way of
> defining nvmem cells is through DT or nvmem_config, we lack a way to
> allow machine code to define cells without the provider code being
> aware.

machine driver should be able to do
nvmem_device_get()
nvmem_add_cells()

currently this adds to the global cell list which is exactly like doing
it via nvmem_config.
>
>>> with each cell referencing its owner nvmem device. I'm wondering if
>>> this isn't some kind of inversion of ownership. Shouldn't each nvmem
>>> device have a separate list of nvmem cells owned by it? What happens
>>
>> This is mainly done for use case where consumer does not have idea of
>> provider name or any details.
>>
>
> It doesn't need to know the provider details, but in most subsystems
> the core code associates such resources by dev_id and optional con_id
> as Boris already said.
>

If dev_id here is referring to provider dev_id, then we already do that
using nvmem device apis, except in global cell list which makes dev_id
optional.


>> First thing non dt user should do is use "NVMEM device based consumer APIs"
>>
>> ex: First get handle to nvmem device using its nvmem provider name by
>> calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.
>>
>> Also am not 100% sure how would maintaining cells list per nvmem provider
>> would help for the intended purpose of global list?
>>
>
> It would fix the use case where the consumer wants to use
> nvmem_cell_get(dev, name) and two nvmem providers would have a cell
> with the same name.

There is no code to enforce duplicate checks, so this would just
decrease the chances rather than fixing the problem totally.
I guess this is same problem

Finding cell by name without dev_id would still be an issue, am not too
concerned about this ATM.

However, the idea of having cells per provider does sound good to me.
We should also maintain list of providers in core as a lookup in cases
where dev_id is null.

I did hack up a patch, incase you might want to try:
I did only compile test.
---------------------------------->cut<-------------------------------
Author: Srinivas Kandagatla <[email protected]>
Date: Tue Aug 28 13:46:21 2018 +0100

nvmem: core: maintain per provider cell list

Having a global cell list could be a issue in cases where the
cell-id is same across multiple providers. Making the cell list specific
to provider could avoid such issue by adding additional checks while
addding cells.

Signed-off-by: Srinivas Kandagatla <[email protected]>

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index aa1657831b70..29da603f2fa4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -40,6 +40,8 @@ struct nvmem_device {
struct device *base_dev;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
+ struct list_head node;
+ struct list_head cells;
void *priv;
};

@@ -57,9 +59,7 @@ struct nvmem_cell {

static DEFINE_MUTEX(nvmem_mutex);
static DEFINE_IDA(nvmem_ida);
-
-static LIST_HEAD(nvmem_cells);
-static DEFINE_MUTEX(nvmem_cells_mutex);
+static LIST_HEAD(nvmem_devices);

#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key eeprom_lock_key;
@@ -284,26 +284,28 @@ static struct nvmem_device *of_nvmem_find(struct
device_node *nvmem_np)

static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
{
- struct nvmem_cell *p;
+ struct nvmem_device *d;

- mutex_lock(&nvmem_cells_mutex);
-
- list_for_each_entry(p, &nvmem_cells, node)
- if (!strcmp(p->name, cell_id)) {
- mutex_unlock(&nvmem_cells_mutex);
- return p;
- }
+ mutex_lock(&nvmem_mutex);
+ list_for_each_entry(d, &nvmem_devices, node) {
+ struct nvmem_cell *p;
+ list_for_each_entry(p, &d->cells, node)
+ if (!strcmp(p->name, cell_id)) {
+ mutex_unlock(&nvmem_mutex);
+ return p;
+ }
+ }

- mutex_unlock(&nvmem_cells_mutex);
+ mutex_unlock(&nvmem_mutex);

return NULL;
}

static void nvmem_cell_drop(struct nvmem_cell *cell)
{
- mutex_lock(&nvmem_cells_mutex);
+ mutex_lock(&nvmem_mutex);
list_del(&cell->node);
- mutex_unlock(&nvmem_cells_mutex);
+ mutex_unlock(&nvmem_mutex);
kfree(cell);
}

@@ -312,18 +314,18 @@ static void nvmem_device_remove_all_cells(const
struct nvmem_device *nvmem)
struct nvmem_cell *cell;
struct list_head *p, *n;

- list_for_each_safe(p, n, &nvmem_cells) {
+ list_for_each_safe(p, n, &nvmem->cells) {
cell = list_entry(p, struct nvmem_cell, node);
if (cell->nvmem == nvmem)
nvmem_cell_drop(cell);
}
}

-static void nvmem_cell_add(struct nvmem_cell *cell)
+static void nvmem_cell_add(struct nvmem_device *nvmem, struct
nvmem_cell *cell)
{
- mutex_lock(&nvmem_cells_mutex);
- list_add_tail(&cell->node, &nvmem_cells);
- mutex_unlock(&nvmem_cells_mutex);
+ mutex_lock(&nvmem_mutex);
+ list_add_tail(&cell->node, &nvmem->cells);
+ mutex_unlock(&nvmem_mutex);
}

static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
@@ -385,7 +387,7 @@ int nvmem_add_cells(struct nvmem_device *nvmem,
goto err;
}

- nvmem_cell_add(cells[i]);
+ nvmem_cell_add(nvmem, cells[i]);
}

/* remove tmp array */
@@ -519,6 +521,10 @@ struct nvmem_device *nvmem_register(const struct
nvmem_config *config)
if (config->cells)
nvmem_add_cells(nvmem, config->cells, config->ncells);

+ mutex_lock(&nvmem_mutex);
+ list_add_tail(&nvmem->node, &nvmem_devices);
+ mutex_unlock(&nvmem_mutex);
+
return nvmem;

err_device_del:
@@ -544,6 +550,8 @@ int nvmem_unregister(struct nvmem_device *nvmem)
mutex_unlock(&nvmem_mutex);
return -EBUSY;
}
+
+ list_del(&nvmem->node);
mutex_unlock(&nvmem_mutex);

if (nvmem->flags & FLAG_COMPAT)
@@ -899,7 +907,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct
device_node *np,
goto err_sanity;
}

- nvmem_cell_add(cell);
+ nvmem_cell_add(nvmem, cell);

return cell;

---------------------------------->cut<-------------------------------

>
> Next we could add a way to associate dev_ids with nvmem cells.
>
>>> if we have two nvmem providers with the same names for cells? I'm
>>
>> Yes, it would return the first instance.. which is a known issue.
>> Am not really sure this is a big problem as of today! but am open for any
>> better suggestions!
>>
>
> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
> defining nvmem-cells using nvmem_config. I think that what we need is
> a way of specifying cell config outside of nvmem providers in some
> kind of structures. These tables would reference the provider by name
> and define the cells. Then we would have an additional lookup
> structure which would associate the consumer (by dev_id and con_id,
> where dev_id could optionally be NULL and where we would fall back to
> using con_id only) and the nvmem provider + cell together. Similarly
> to how GPIO consumers are associated with the gpiochip and hwnum. How
> does it sound?
Yes, sounds good.

Correct me if am wrong!
You should be able to add the new cells using struct nvmem_cell_info and
add them to particular provider using nvmem_add_cells().

Sounds like thats exactly what nvmem_add_lookup_table() would look like.

We should add new nvmem_device_cell_get(nvmem, conn_id) which would
return nvmem cell which is specific to the provider. This cell can be
used by the machine driver to read/write.

>>>
>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>>> instance even if the cell for this node was already added to the nvmem
>>> device.
>>
>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>> instance for every get!!
>
>
> I admit I didn't test it, but just from reading the code it seems like
> in nvmem_cell_get() for DT-users we'll always get to
> of_nvmem_cell_get() and in there we always end up calling line 873:
> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>
That is correct, this cell is created when we do a get and release when
we do a put().

thanks,
srini

2018-08-28 14:42:26

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 28/08/18 12:56, Bartosz Golaszewski wrote:
>>
>> 2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla
>> <[email protected]>:
>>>
>>>
>>> On 27/08/18 14:37, Bartosz Golaszewski wrote:
>>>>
>>>>
>>>> I didn't notice it before but there's a global list of nvmem cells
>>>
>>>
>>>
>>> Bit of history here.
>>>
>>> The global list of nvmem_cell is to assist non device tree based cell
>>> lookups. These cell entries come as part of the non-dt providers
>>> nvmem_config.
>>>
>>> All the device tree based cell lookup happen dynamically on
>>> request/demand,
>>> and all the cell definition comes from DT.
>>>
>>
>> Makes perfect sense.
>>
>>> As of today NVMEM supports both DT and non DT usecase, this is much
>>> simpler.
>>>
>>> Non dt cases have various consumer usecases.
>>>
>>> 1> Consumer is aware of provider name and cell details.
>>> This is probably simple usecase where it can just use device
>>> based
>>> apis.
>>>
>>> 2> Consumer is not aware of provider name, its just aware of cell name.
>>> This is the case where global list of cells are used.
>>>
>>
>> I would like to support an additional use case here: the provider is
>> generic and is not aware of its cells at all. Since the only way of
>> defining nvmem cells is through DT or nvmem_config, we lack a way to
>> allow machine code to define cells without the provider code being
>> aware.
>
>
> machine driver should be able to do
> nvmem_device_get()
> nvmem_add_cells()
>

Indeed, I missed the fact that you can retrieve the nvmem device by
name. Except that we cannot know that the nvmem provider has been
registered yet when calling nvmem_device_get(). This could potentially
be solved by my other patch that adds notifiers to nvmem, but it would
require much more boilerplate code in every board file. I think that
removing nvmem_cell_info from nvmem_config and having external cell
definitions would be cleaner.

> currently this adds to the global cell list which is exactly like doing it
> via nvmem_config.
>>
>>
>>>> with each cell referencing its owner nvmem device. I'm wondering if
>>>> this isn't some kind of inversion of ownership. Shouldn't each nvmem
>>>> device have a separate list of nvmem cells owned by it? What happens
>>>
>>>
>>> This is mainly done for use case where consumer does not have idea of
>>> provider name or any details.
>>>
>>
>> It doesn't need to know the provider details, but in most subsystems
>> the core code associates such resources by dev_id and optional con_id
>> as Boris already said.
>>
>
> If dev_id here is referring to provider dev_id, then we already do that
> using nvmem device apis, except in global cell list which makes dev_id
> optional.
>
>
>>> First thing non dt user should do is use "NVMEM device based consumer
>>> APIs"
>>>
>>> ex: First get handle to nvmem device using its nvmem provider name by
>>> calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.
>>>
>>> Also am not 100% sure how would maintaining cells list per nvmem provider
>>> would help for the intended purpose of global list?
>>>
>>
>> It would fix the use case where the consumer wants to use
>> nvmem_cell_get(dev, name) and two nvmem providers would have a cell
>> with the same name.
>
>
> There is no code to enforce duplicate checks, so this would just decrease
> the chances rather than fixing the problem totally.
> I guess this is same problem
>
> Finding cell by name without dev_id would still be an issue, am not too
> concerned about this ATM.
>
> However, the idea of having cells per provider does sound good to me.
> We should also maintain list of providers in core as a lookup in cases where
> dev_id is null.
>
> I did hack up a patch, incase you might want to try:
> I did only compile test.
> ---------------------------------->cut<-------------------------------
> Author: Srinivas Kandagatla <[email protected]>
> Date: Tue Aug 28 13:46:21 2018 +0100
>
> nvmem: core: maintain per provider cell list
>
> Having a global cell list could be a issue in cases where the cell-id is
> same across multiple providers. Making the cell list specific to provider
> could avoid such issue by adding additional checks while addding cells.
>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index aa1657831b70..29da603f2fa4 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -40,6 +40,8 @@ struct nvmem_device {
> struct device *base_dev;
> nvmem_reg_read_t reg_read;
> nvmem_reg_write_t reg_write;
> + struct list_head node;
> + struct list_head cells;
> void *priv;
> };
>
> @@ -57,9 +59,7 @@ struct nvmem_cell {
>
> static DEFINE_MUTEX(nvmem_mutex);
> static DEFINE_IDA(nvmem_ida);
> -
> -static LIST_HEAD(nvmem_cells);
> -static DEFINE_MUTEX(nvmem_cells_mutex);
> +static LIST_HEAD(nvmem_devices);
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> static struct lock_class_key eeprom_lock_key;
> @@ -284,26 +284,28 @@ static struct nvmem_device *of_nvmem_find(struct
> device_node *nvmem_np)
>
> static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
> {
> - struct nvmem_cell *p;
> + struct nvmem_device *d;
>
> - mutex_lock(&nvmem_cells_mutex);
> -
> - list_for_each_entry(p, &nvmem_cells, node)
> - if (!strcmp(p->name, cell_id)) {
> - mutex_unlock(&nvmem_cells_mutex);
> - return p;
> - }
> + mutex_lock(&nvmem_mutex);
> + list_for_each_entry(d, &nvmem_devices, node) {
> + struct nvmem_cell *p;
> + list_for_each_entry(p, &d->cells, node)
> + if (!strcmp(p->name, cell_id)) {
> + mutex_unlock(&nvmem_mutex);
> + return p;
> + }
> + }
>
> - mutex_unlock(&nvmem_cells_mutex);
> + mutex_unlock(&nvmem_mutex);
>
> return NULL;
> }
>
> static void nvmem_cell_drop(struct nvmem_cell *cell)
> {
> - mutex_lock(&nvmem_cells_mutex);
> + mutex_lock(&nvmem_mutex);
> list_del(&cell->node);
> - mutex_unlock(&nvmem_cells_mutex);
> + mutex_unlock(&nvmem_mutex);
> kfree(cell);
> }
>
> @@ -312,18 +314,18 @@ static void nvmem_device_remove_all_cells(const struct
> nvmem_device *nvmem)
> struct nvmem_cell *cell;
> struct list_head *p, *n;
>
> - list_for_each_safe(p, n, &nvmem_cells) {
> + list_for_each_safe(p, n, &nvmem->cells) {
> cell = list_entry(p, struct nvmem_cell, node);
> if (cell->nvmem == nvmem)
> nvmem_cell_drop(cell);
> }
> }
>
> -static void nvmem_cell_add(struct nvmem_cell *cell)
> +static void nvmem_cell_add(struct nvmem_device *nvmem, struct nvmem_cell
> *cell)
> {
> - mutex_lock(&nvmem_cells_mutex);
> - list_add_tail(&cell->node, &nvmem_cells);
> - mutex_unlock(&nvmem_cells_mutex);
> + mutex_lock(&nvmem_mutex);
> + list_add_tail(&cell->node, &nvmem->cells);
> + mutex_unlock(&nvmem_mutex);
> }
>
> static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
> @@ -385,7 +387,7 @@ int nvmem_add_cells(struct nvmem_device *nvmem,
> goto err;
> }
>
> - nvmem_cell_add(cells[i]);
> + nvmem_cell_add(nvmem, cells[i]);
> }
>
> /* remove tmp array */
> @@ -519,6 +521,10 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> if (config->cells)
> nvmem_add_cells(nvmem, config->cells, config->ncells);
>
> + mutex_lock(&nvmem_mutex);
> + list_add_tail(&nvmem->node, &nvmem_devices);
> + mutex_unlock(&nvmem_mutex);
> +
> return nvmem;
>
> err_device_del:
> @@ -544,6 +550,8 @@ int nvmem_unregister(struct nvmem_device *nvmem)
> mutex_unlock(&nvmem_mutex);
> return -EBUSY;
> }
> +
> + list_del(&nvmem->node);
> mutex_unlock(&nvmem_mutex);
>
> if (nvmem->flags & FLAG_COMPAT)
> @@ -899,7 +907,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node
> *np,
> goto err_sanity;
> }
>
> - nvmem_cell_add(cell);
> + nvmem_cell_add(nvmem, cell);
>
> return cell;
>
> ---------------------------------->cut<-------------------------------
>
>>
>> Next we could add a way to associate dev_ids with nvmem cells.
>>
>>>> if we have two nvmem providers with the same names for cells? I'm
>>>
>>>
>>> Yes, it would return the first instance.. which is a known issue.
>>> Am not really sure this is a big problem as of today! but am open for any
>>> better suggestions!
>>>
>>
>> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
>> defining nvmem-cells using nvmem_config. I think that what we need is
>> a way of specifying cell config outside of nvmem providers in some
>> kind of structures. These tables would reference the provider by name
>> and define the cells. Then we would have an additional lookup
>> structure which would associate the consumer (by dev_id and con_id,
>> where dev_id could optionally be NULL and where we would fall back to
>> using con_id only) and the nvmem provider + cell together. Similarly
>> to how GPIO consumers are associated with the gpiochip and hwnum. How
>> does it sound?
>
> Yes, sounds good.
>
> Correct me if am wrong!
> You should be able to add the new cells using struct nvmem_cell_info and add
> them to particular provider using nvmem_add_cells().
>
> Sounds like thats exactly what nvmem_add_lookup_table() would look like.
>
> We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
> nvmem cell which is specific to the provider. This cell can be used by the
> machine driver to read/write.

Except that we could do it lazily - when the nvmem provider actually
gets registered instead of doing it right away and risking that the
device isn't even there yet.

>
>>>>
>>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>>>> instance even if the cell for this node was already added to the nvmem
>>>> device.
>>>
>>>
>>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>>> instance for every get!!
>>
>>
>>
>> I admit I didn't test it, but just from reading the code it seems like
>> in nvmem_cell_get() for DT-users we'll always get to
>> of_nvmem_cell_get() and in there we always end up calling line 873:
>> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>>
> That is correct, this cell is created when we do a get and release when we
> do a put().
>

Shouldn't we add the cell to the list, and check first if it's there
and only create it if not?

Bart

2018-08-28 14:50:34

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups



On 28/08/18 15:41, Bartosz Golaszewski wrote:
> 2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>>
>>
...
>>> I would like to support an additional use case here: the provider is
>>> generic and is not aware of its cells at all. Since the only way of
>>> defining nvmem cells is through DT or nvmem_config, we lack a way to
>>> allow machine code to define cells without the provider code being
>>> aware.
>>
>>
>> machine driver should be able to do
>> nvmem_device_get()
>> nvmem_add_cells()
>>
>
> Indeed, I missed the fact that you can retrieve the nvmem device by
> name. Except that we cannot know that the nvmem provider has been
> registered yet when calling nvmem_device_get(). This could potentially
> be solved by my other patch that adds notifiers to nvmem, but it would
> require much more boilerplate code in every board file. I think that
> removing nvmem_cell_info from nvmem_config and having external cell
> definitions would be cleaner.

Yes, notifiers would work!

...
>>>
>>> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
>>> defining nvmem-cells using nvmem_config. I think that what we need is
>>> a way of specifying cell config outside of nvmem providers in some
>>> kind of structures. These tables would reference the provider by name
>>> and define the cells. Then we would have an additional lookup
>>> structure which would associate the consumer (by dev_id and con_id,
>>> where dev_id could optionally be NULL and where we would fall back to
>>> using con_id only) and the nvmem provider + cell together. Similarly
>>> to how GPIO consumers are associated with the gpiochip and hwnum. How
>>> does it sound?
>>
>> Yes, sounds good.
>>
>> Correct me if am wrong!
>> You should be able to add the new cells using struct nvmem_cell_info and add
>> them to particular provider using nvmem_add_cells().
>>
>> Sounds like thats exactly what nvmem_add_lookup_table() would look like.
>>
>> We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
>> nvmem cell which is specific to the provider. This cell can be used by the
>> machine driver to read/write.
>
> Except that we could do it lazily - when the nvmem provider actually
> gets registered instead of doing it right away and risking that the
> device isn't even there yet.
>
Yes, it makes more sense to do it once the provider is actually present!

>>
>>>>>
>>>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>>>>> instance even if the cell for this node was already added to the nvmem
>>>>> device.
>>>>
>>>>
>>>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>>>> instance for every get!!
>>>
>>>
>>>
>>> I admit I didn't test it, but just from reading the code it seems like
>>> in nvmem_cell_get() for DT-users we'll always get to
>>> of_nvmem_cell_get() and in there we always end up calling line 873:
>>> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>>>
>> That is correct, this cell is created when we do a get and release when we
>> do a put().
>>
>
> Shouldn't we add the cell to the list, and check first if it's there
> and only create it if not?
Yes I agree, duplicate entry checks are missing!

--srini
>
> Bart
>

2018-08-28 14:54:47

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups

On Tue, 28 Aug 2018 16:41:04 +0200
Bartosz Golaszewski <[email protected]> wrote:

> 2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <[email protected]>:
> >
> >
> > On 28/08/18 12:56, Bartosz Golaszewski wrote:
> >>
> >> 2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla
> >> <[email protected]>:
> >>>
> >>>
> >>> On 27/08/18 14:37, Bartosz Golaszewski wrote:
> >>>>
> >>>>
> >>>> I didn't notice it before but there's a global list of nvmem cells
> >>>
> >>>
> >>>
> >>> Bit of history here.
> >>>
> >>> The global list of nvmem_cell is to assist non device tree based cell
> >>> lookups. These cell entries come as part of the non-dt providers
> >>> nvmem_config.
> >>>
> >>> All the device tree based cell lookup happen dynamically on
> >>> request/demand,
> >>> and all the cell definition comes from DT.
> >>>
> >>
> >> Makes perfect sense.
> >>
> >>> As of today NVMEM supports both DT and non DT usecase, this is much
> >>> simpler.
> >>>
> >>> Non dt cases have various consumer usecases.
> >>>
> >>> 1> Consumer is aware of provider name and cell details.
> >>> This is probably simple usecase where it can just use device
> >>> based
> >>> apis.
> >>>
> >>> 2> Consumer is not aware of provider name, its just aware of cell name.
> >>> This is the case where global list of cells are used.
> >>>
> >>
> >> I would like to support an additional use case here: the provider is
> >> generic and is not aware of its cells at all. Since the only way of
> >> defining nvmem cells is through DT or nvmem_config, we lack a way to
> >> allow machine code to define cells without the provider code being
> >> aware.
> >
> >
> > machine driver should be able to do
> > nvmem_device_get()
> > nvmem_add_cells()
> >
>
> Indeed, I missed the fact that you can retrieve the nvmem device by
> name. Except that we cannot know that the nvmem provider has been
> registered yet when calling nvmem_device_get(). This could potentially
> be solved by my other patch that adds notifiers to nvmem, but it would
> require much more boilerplate code in every board file. I think that
> removing nvmem_cell_info from nvmem_config and having external cell
> definitions would be cleaner.

I also vote for this option.

> >
> > static struct nvmem_cell *nvmem_find_cell(const char *cell_id)

Can we get rid of this function and just have the the version that
takes an nvmem_name and a cell_id.

> >> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
> >> defining nvmem-cells using nvmem_config. I think that what we need is
> >> a way of specifying cell config outside of nvmem providers in some
> >> kind of structures. These tables would reference the provider by name
> >> and define the cells. Then we would have an additional lookup
> >> structure which would associate the consumer (by dev_id and con_id,
> >> where dev_id could optionally be NULL and where we would fall back to
> >> using con_id only) and the nvmem provider + cell together. Similarly
> >> to how GPIO consumers are associated with the gpiochip and hwnum. How
> >> does it sound?
> >
> > Yes, sounds good.
> >
> > Correct me if am wrong!
> > You should be able to add the new cells using struct nvmem_cell_info and add
> > them to particular provider using nvmem_add_cells().
> >
> > Sounds like thats exactly what nvmem_add_lookup_table() would look like.
> >
> > We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
> > nvmem cell which is specific to the provider. This cell can be used by the
> > machine driver to read/write.
>
> Except that we could do it lazily - when the nvmem provider actually
> gets registered instead of doing it right away and risking that the
> device isn't even there yet.

And again, I agree with you. That's basically what lookup tables are
meant for: defining resources that are supposed to be attached to a
device when it's registered to a subsystem.

>
> >
> >>>>
> >>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
> >>>> instance even if the cell for this node was already added to the nvmem
> >>>> device.
> >>>
> >>>
> >>> I hope you got the reason why of_nvmem_cell_get() always allocates new
> >>> instance for every get!!
> >>
> >>
> >>
> >> I admit I didn't test it, but just from reading the code it seems like
> >> in nvmem_cell_get() for DT-users we'll always get to
> >> of_nvmem_cell_get() and in there we always end up calling line 873:
> >> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
> >>
> > That is correct, this cell is created when we do a get and release when we
> > do a put().
> >
>
> Shouldn't we add the cell to the list, and check first if it's there
> and only create it if not?

Or even better: create the cells at registration time so that the
search code is the same for both DT and non-DT cases. Only the
registration would differ (with one path parsing the DT, and the other
one searching for nvmem cells defined with a nvmem-provider-lookup
table).

2018-08-28 15:10:49

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 01/29] nvmem: add support for cell lookups



On 28/08/18 15:53, Boris Brezillon wrote:
> On Tue, 28 Aug 2018 16:41:04 +0200
> Bartosz Golaszewski <[email protected]> wrote:
>

...
>
>>>
>>> static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
>
> Can we get rid of this function and just have the the version that
> takes an nvmem_name and a cell_id.

That should be feasible!


>>>>>
>>>>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>>>>> instance for every get!!
>>>>
>>>>
>>>>
>>>> I admit I didn't test it, but just from reading the code it seems like
>>>> in nvmem_cell_get() for DT-users we'll always get to
>>>> of_nvmem_cell_get() and in there we always end up calling line 873:
>>>> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>>>>
>>> That is correct, this cell is created when we do a get and release when we
>>> do a put().
>>>
>>
>> Shouldn't we add the cell to the list, and check first if it's there
>> and only create it if not?
>
> Or even better: create the cells at registration time so that the
> search code is the same for both DT and non-DT cases. Only the
> registration would differ (with one path parsing the DT, and the other
> one searching for nvmem cells defined with a nvmem-provider-lookup
> table).
Makes sense! and that would go very well with the plan of "nvmem-cell"
compatible for cells!.
>

2018-08-31 19:48:10

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

Hi,

On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> Most boards use the EEPROM to store the MAC address. This series adds
> support for cell lookups to the nvmem framework, registers relevant
> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> converts davinci_emac driver to using it and replaces at24_platform_data
> with device properties.

We already have:

of_get_nvmem_mac_address() (which does exactly what you're adding,
except it's DT specific)
of_get_mac_address()
fwnode_get_mac_address()
device_get_mac_address()

and now you've taught me that this exists too:

eth_platform_get_mac_address()

These mostly don't share code, and with your series, they'll start to
diverge even more as to what they support. Can you please help rectify
that, instead of widening the gap?

For instance, you can delete most of eth_platform_get_mac_address() and
replace it with device_get_mac_address() [1]. And you could add your new
stuff to fwnode_get_mac_address().

And important part to note here is that you code isn't just useful for
ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
only in an "eth" function is the wrong move.

Brian

[1] arch_get_platform_mac_address() is the only part I wouldn't want to
replicate into a truly generic helper. The following should be a no-op
refactor, AIUI:

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index ee28440f57c5..b738651f71e1 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -47,12 +47,12 @@
#include <linux/inet.h>
#include <linux/ip.h>
#include <linux/netdevice.h>
+#include <linux/property.h>
#include <linux/etherdevice.h>
#include <linux/skbuff.h>
#include <linux/errno.h>
#include <linux/init.h>
#include <linux/if_ether.h>
-#include <linux/of_net.h>
#include <linux/pci.h>
#include <net/dst.h>
#include <net/arp.h>
@@ -528,19 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
{
const unsigned char *addr;
- struct device_node *dp;

- if (dev_is_pci(dev))
- dp = pci_device_to_OF_node(to_pci_dev(dev));
- else
- dp = dev->of_node;
-
- addr = NULL;
- if (dp)
- addr = of_get_mac_address(dp);
- if (!addr)
- addr = arch_get_platform_mac_address();
+ if (device_get_mac_address(dev, mac_addr, ETH_ALEN))
+ return 0;

+ addr = arch_get_platform_mac_address();
if (!addr)
return -ENODEV;


2018-08-31 19:56:10

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH v2 15/29] net: split eth_platform_get_mac_address() into subroutines

Hi,

I responded to the cover letter, but I'll put some notes here too.

On Fri, Aug 10, 2018 at 10:05:12AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> We want do add more sources from which to read the MAC address. In
> order to avoid bloating this function too much, start by splitting it
> into subroutines, each of which takes care of reading the MAC from
> one source.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> net/ethernet/eth.c | 44 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 7f08105402c8..d01dd55de037 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -525,22 +525,50 @@ unsigned char * __weak arch_get_platform_mac_address(void)
> return NULL;
> }
>
> -int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> +static int mac_address_from_of(struct device *dev, u8 *mac_addr)
> {
> const unsigned char *addr;
> - struct device_node *dp;
> + struct device_node *np;
>
> - dp = dev->of_node;
> - addr = NULL;
> - if (dp)
> - addr = of_get_mac_address(dp);
> - if (!addr)
> - addr = arch_get_platform_mac_address();
> + np = dev->of_node;
> + if (!np)
> + return -ENODEV;
>
> + addr = of_get_mac_address(np);
> if (!addr)
> return -ENODEV;
>
> + if (!addr || !is_valid_ether_addr(addr))
> + return -ENODEV;
> +
> ether_addr_copy(mac_addr, addr);
> return 0;
> }
> +
> +static int mac_address_from_arch(u8 *mac_addr)
> +{
> + const unsigned char *addr;
> +
> + addr = arch_get_platform_mac_address();
> + if (!addr || !is_valid_ether_addr(addr))
> + return -ENODEV;
> +
> + ether_addr_copy(mac_addr, addr);
> + return 0;
> +}
> +
> +int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> +{
> + int rv;
> +
> + rv = mac_address_from_of(dev, mac_addr);

This could just be replaced with device_get_mac_address(). Then you
should be extending either fwnode_get_device_mac_address() or
device_get_mac_addres() in the next patch, not
eth_platform_get_mac_address().

Regards,
Brian

> + if (!rv)
> + return 0;
> +
> + rv = mac_address_from_arch(mac_addr);
> + if (!rv)
> + return 0;
> +
> + return -ENODEV;
> +}
> EXPORT_SYMBOL(eth_platform_get_mac_address);
> --
> 2.18.0
>

2018-08-31 20:32:11

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH v2 02/29] Documentation: nvmem: document lookup entries

On Fri, Aug 10, 2018 at 10:04:59AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Describe the usage of nvmem cell lookup tables.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> Documentation/nvmem/nvmem.txt | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt
> index 8d8d8f58f96f..9d5e3ca2b4f3 100644
> --- a/Documentation/nvmem/nvmem.txt
> +++ b/Documentation/nvmem/nvmem.txt
> @@ -58,6 +58,34 @@ static int qfprom_probe(struct platform_device *pdev)
> It is mandatory that the NVMEM provider has a regmap associated with its
> struct device. Failure to do would return error code from nvmem_register().
>
> +Additionally it is possible to create nvmem cell lookup entries and register
> +them with the nvmem framework from machine code as shown in the example below:

Maybe it's partially a lacking in the existing documentation, but what
does the "name" and the "nvmem_name" mean here? AFAICT, "nvmem_name" is
akin to a provider identifier; and "name" is a key to match with the
consumer. It feels like this should be in either the header / kerneldoc
or this file. Or maybe both.

Does this mean there can only be a single "mac-address" cell in the
system? I have systems where there are multiple MACs provided in flash
storage, and we need to map them to ethernet0 and ethernet1. Is that
supported here?

Brian

> +static struct nvmem_cell_lookup foobar_lookup = {
> + .info = {
> + .name = "mac-address",
> + .offset = 0xd000,
> + .bytes = ERH_ALEN,
> + },
> + .nvmem_name = "foobar",
> +};
> +
> +static void foobar_register(void)
> +{
> + ...
> + nvmem_add_lookup_table(&foobar_lookup, 1);
> + ...
> +}
> +
> +A lookup entry table can be later removed if needed:
> +
> +static void foobar_fini(void)
> +{
> + ...
> + nvmem_del_lookup_table(&foobar_lookup, 1);
> + ...
> +}
> +
> NVMEM Consumers
> +++++++++++++++
>
> --
> 2.18.0
>

2018-09-01 13:12:38

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 02/29] Documentation: nvmem: document lookup entries

2018-08-31 22:30 GMT+02:00 Brian Norris <[email protected]>:
> On Fri, Aug 10, 2018 at 10:04:59AM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <[email protected]>
>>
>> Describe the usage of nvmem cell lookup tables.
>>
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>> ---
>> Documentation/nvmem/nvmem.txt | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt
>> index 8d8d8f58f96f..9d5e3ca2b4f3 100644
>> --- a/Documentation/nvmem/nvmem.txt
>> +++ b/Documentation/nvmem/nvmem.txt
>> @@ -58,6 +58,34 @@ static int qfprom_probe(struct platform_device *pdev)
>> It is mandatory that the NVMEM provider has a regmap associated with its
>> struct device. Failure to do would return error code from nvmem_register().
>>
>> +Additionally it is possible to create nvmem cell lookup entries and register
>> +them with the nvmem framework from machine code as shown in the example below:
>
> Maybe it's partially a lacking in the existing documentation, but what
> does the "name" and the "nvmem_name" mean here? AFAICT, "nvmem_name" is
> akin to a provider identifier; and "name" is a key to match with the
> consumer. It feels like this should be in either the header / kerneldoc
> or this file. Or maybe both.
>
> Does this mean there can only be a single "mac-address" cell in the
> system? I have systems where there are multiple MACs provided in flash
> storage, and we need to map them to ethernet0 and ethernet1. Is that
> supported here?
>

This is a shortcoming of the nvmem subsystem we discussed under
another patch in this series with Boris and Srinivas. I will try to
post a series addressing this next week. Basically there's a global
list of nvmem cells referenced only by name and unless you're using DT
you can only have a single cell with given name. I will address it by
introducing the standard dev_id/con_id resource association.

Bart

> Brian
>
>> +static struct nvmem_cell_lookup foobar_lookup = {
>> + .info = {
>> + .name = "mac-address",
>> + .offset = 0xd000,
>> + .bytes = ERH_ALEN,
>> + },
>> + .nvmem_name = "foobar",
>> +};
>> +
>> +static void foobar_register(void)
>> +{
>> + ...
>> + nvmem_add_lookup_table(&foobar_lookup, 1);
>> + ...
>> +}
>> +
>> +A lookup entry table can be later removed if needed:
>> +
>> +static void foobar_fini(void)
>> +{
>> + ...
>> + nvmem_del_lookup_table(&foobar_lookup, 1);
>> + ...
>> +}
>> +
>> NVMEM Consumers
>> +++++++++++++++
>>
>> --
>> 2.18.0
>>

2018-10-03 20:15:59

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

pt., 31 sie 2018 o 21:46 Brian Norris <[email protected]> napisał(a):
>
> Hi,
>
> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> > Most boards use the EEPROM to store the MAC address. This series adds
> > support for cell lookups to the nvmem framework, registers relevant
> > cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> > converts davinci_emac driver to using it and replaces at24_platform_data
> > with device properties.
>
> We already have:
>
> of_get_nvmem_mac_address() (which does exactly what you're adding,
> except it's DT specific)
> of_get_mac_address()
> fwnode_get_mac_address()
> device_get_mac_address()
>
> and now you've taught me that this exists too:
>
> eth_platform_get_mac_address()
>
> These mostly don't share code, and with your series, they'll start to
> diverge even more as to what they support. Can you please help rectify
> that, instead of widening the gap?
>
> For instance, you can delete most of eth_platform_get_mac_address() and
> replace it with device_get_mac_address() [1]. And you could add your new
> stuff to fwnode_get_mac_address().
>
> And important part to note here is that you code isn't just useful for
> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
> only in an "eth" function is the wrong move.
>
> Brian
>
> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
> replicate into a truly generic helper. The following should be a no-op
> refactor, AIUI:
>

The only user of arch_get_platform_mac_address() is sparc. It returns
an address that seems to be read from some kind of EEPROM. I'm not
familiar with this arch though. I'm wondering if we could somehow
seamlessly remove this call and then convert all users of
eth_platform_get_mac_address() to using device_get_mac_address()?

David: I couldn't find a place in sparc code where any ethernet device
would be registered, so is there a chance that nobody is using it?

Best regards,
Bartosz Golaszewski

> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index ee28440f57c5..b738651f71e1 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -47,12 +47,12 @@
> #include <linux/inet.h>
> #include <linux/ip.h>
> #include <linux/netdevice.h>
> +#include <linux/property.h>
> #include <linux/etherdevice.h>
> #include <linux/skbuff.h>
> #include <linux/errno.h>
> #include <linux/init.h>
> #include <linux/if_ether.h>
> -#include <linux/of_net.h>
> #include <linux/pci.h>
> #include <net/dst.h>
> #include <net/arp.h>
> @@ -528,19 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
> int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> {
> const unsigned char *addr;
> - struct device_node *dp;
>
> - if (dev_is_pci(dev))
> - dp = pci_device_to_OF_node(to_pci_dev(dev));
> - else
> - dp = dev->of_node;
> -
> - addr = NULL;
> - if (dp)
> - addr = of_get_mac_address(dp);
> - if (!addr)
> - addr = arch_get_platform_mac_address();
> + if (device_get_mac_address(dev, mac_addr, ETH_ALEN))
> + return 0;
>
> + addr = arch_get_platform_mac_address();
> if (!addr)
> return -ENODEV;
>

2018-10-03 20:31:39

by Lukas Wunner

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

On Wed, Oct 03, 2018 at 10:15:23PM +0200, Bartosz Golaszewski wrote:
> The only user of arch_get_platform_mac_address() is sparc. It returns
> an address that seems to be read from some kind of EEPROM. I'm not
> familiar with this arch though. I'm wondering if we could somehow
> seamlessly remove this call and then convert all users of
> eth_platform_get_mac_address() to using device_get_mac_address()?

My recollection is that '90s SparcStations had a battery-backed memory
containing the MAC address. When the battery wore out, the machine
would fail to boot because the address was either all ones or all zeros.
I recall setting the MAC address in that memory using Forth from the
boot prompt. Ah, the good old days!

2018-10-03 21:04:39

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data



On 10/3/2018 1:15 PM, Bartosz Golaszewski wrote:
> pt., 31 sie 2018 o 21:46 Brian Norris <[email protected]> napisał(a):
>>
>> Hi,
>>
>> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
>>> Most boards use the EEPROM to store the MAC address. This series adds
>>> support for cell lookups to the nvmem framework, registers relevant
>>> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
>>> converts davinci_emac driver to using it and replaces at24_platform_data
>>> with device properties.
>>
>> We already have:
>>
>> of_get_nvmem_mac_address() (which does exactly what you're adding,
>> except it's DT specific)
>> of_get_mac_address()
>> fwnode_get_mac_address()
>> device_get_mac_address()
>>
>> and now you've taught me that this exists too:
>>
>> eth_platform_get_mac_address()
>>
>> These mostly don't share code, and with your series, they'll start to
>> diverge even more as to what they support. Can you please help rectify
>> that, instead of widening the gap?
>>
>> For instance, you can delete most of eth_platform_get_mac_address() and
>> replace it with device_get_mac_address() [1]. And you could add your new
>> stuff to fwnode_get_mac_address().
>>
>> And important part to note here is that you code isn't just useful for
>> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
>> only in an "eth" function is the wrong move.
>>
>> Brian
>>
>> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
>> replicate into a truly generic helper. The following should be a no-op
>> refactor, AIUI:
>>
>
> The only user of arch_get_platform_mac_address() is sparc. It returns
> an address that seems to be read from some kind of EEPROM. I'm not
> familiar with this arch though. I'm wondering if we could somehow
> seamlessly remove this call and then convert all users of
> eth_platform_get_mac_address() to using device_get_mac_address()?
>
> David: I couldn't find a place in sparc code where any ethernet device
> would be registered, so is there a chance that nobody is using it?

SPARC uses a true Open Firmware implementation, so it would register
drivers through the CONFIG_OF infrastructure.
--
Florian

2018-10-04 11:06:58

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

śr., 3 paź 2018 o 23:04 Florian Fainelli <[email protected]> napisał(a):
>
>
>
> On 10/3/2018 1:15 PM, Bartosz Golaszewski wrote:
> > pt., 31 sie 2018 o 21:46 Brian Norris <[email protected]> napisał(a):
> >>
> >> Hi,
> >>
> >> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> >>> Most boards use the EEPROM to store the MAC address. This series adds
> >>> support for cell lookups to the nvmem framework, registers relevant
> >>> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> >>> converts davinci_emac driver to using it and replaces at24_platform_data
> >>> with device properties.
> >>
> >> We already have:
> >>
> >> of_get_nvmem_mac_address() (which does exactly what you're adding,
> >> except it's DT specific)
> >> of_get_mac_address()
> >> fwnode_get_mac_address()
> >> device_get_mac_address()
> >>
> >> and now you've taught me that this exists too:
> >>
> >> eth_platform_get_mac_address()
> >>
> >> These mostly don't share code, and with your series, they'll start to
> >> diverge even more as to what they support. Can you please help rectify
> >> that, instead of widening the gap?
> >>
> >> For instance, you can delete most of eth_platform_get_mac_address() and
> >> replace it with device_get_mac_address() [1]. And you could add your new
> >> stuff to fwnode_get_mac_address().
> >>
> >> And important part to note here is that you code isn't just useful for
> >> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
> >> only in an "eth" function is the wrong move.
> >>
> >> Brian
> >>
> >> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
> >> replicate into a truly generic helper. The following should be a no-op
> >> refactor, AIUI:
> >>
> >
> > The only user of arch_get_platform_mac_address() is sparc. It returns
> > an address that seems to be read from some kind of EEPROM. I'm not
> > familiar with this arch though. I'm wondering if we could somehow
> > seamlessly remove this call and then convert all users of
> > eth_platform_get_mac_address() to using device_get_mac_address()?
> >
> > David: I couldn't find a place in sparc code where any ethernet device
> > would be registered, so is there a chance that nobody is using it?
>
> SPARC uses a true Open Firmware implementation, so it would register
> drivers through the CONFIG_OF infrastructure.
> --

I'm seeing that there are only six callers of
eth_platform_get_mac_address() (the only function which calls
arch_get_platform_mac_address()).

Of these six callers four are intel ethernet drivers and two are usb
ethernet adapter drivers.

Is it even possible that sparc wants to get the mac address for a usb
adapter from some memory chip? Maybe we *can* safely remove that
function completely? That would allow us to simplify a lot of code.

Bart

2018-10-04 13:59:28

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

On Thu, Oct 4, 2018 at 1:06 PM Bartosz Golaszewski <[email protected]> wrote:
> śr., 3 paź 2018 o 23:04 Florian Fainelli <[email protected]> napisał(a):
> > On 10/3/2018 1:15 PM, Bartosz Golaszewski wrote:
> > > pt., 31 sie 2018 o 21:46 Brian Norris <[email protected]> napisał(a):
> > >>
> > >> Hi,
> > >>
> > >> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> > >>> Most boards use the EEPROM to store the MAC address. This series adds
> > >>> support for cell lookups to the nvmem framework, registers relevant
> > >>> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> > >>> converts davinci_emac driver to using it and replaces at24_platform_data
> > >>> with device properties.
> > >>
> > >> We already have:
> > >>
> > >> of_get_nvmem_mac_address() (which does exactly what you're adding,
> > >> except it's DT specific)
> > >> of_get_mac_address()
> > >> fwnode_get_mac_address()
> > >> device_get_mac_address()
> > >>
> > >> and now you've taught me that this exists too:
> > >>
> > >> eth_platform_get_mac_address()
> > >>
> > >> These mostly don't share code, and with your series, they'll start to
> > >> diverge even more as to what they support. Can you please help rectify
> > >> that, instead of widening the gap?
> > >>
> > >> For instance, you can delete most of eth_platform_get_mac_address() and
> > >> replace it with device_get_mac_address() [1]. And you could add your new
> > >> stuff to fwnode_get_mac_address().
> > >>
> > >> And important part to note here is that you code isn't just useful for
> > >> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
> > >> only in an "eth" function is the wrong move.
> > >>
> > >> Brian
> > >>
> > >> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
> > >> replicate into a truly generic helper. The following should be a no-op
> > >> refactor, AIUI:
> > >>
> > >
> > > The only user of arch_get_platform_mac_address() is sparc. It returns
> > > an address that seems to be read from some kind of EEPROM. I'm not
> > > familiar with this arch though. I'm wondering if we could somehow
> > > seamlessly remove this call and then convert all users of
> > > eth_platform_get_mac_address() to using device_get_mac_address()?
> > >
> > > David: I couldn't find a place in sparc code where any ethernet device
> > > would be registered, so is there a chance that nobody is using it?
> >
> > SPARC uses a true Open Firmware implementation, so it would register
> > drivers through the CONFIG_OF infrastructure.
> > --
>
> I'm seeing that there are only six callers of
> eth_platform_get_mac_address() (the only function which calls
> arch_get_platform_mac_address()).
>
> Of these six callers four are intel ethernet drivers and two are usb
> ethernet adapter drivers.
>
> Is it even possible that sparc wants to get the mac address for a usb
> adapter from some memory chip? Maybe we *can* safely remove that
> function completely? That would allow us to simplify a lot of code.

The calls are not even that old, and clearly added intentionally for sparc,
see commit ba94272d08a7 ("i40e: use eth_platform_get_mac_address()")
which added the first one.

Before that commit, the driver did the same as a couple of sun
specific ones that access the idprom directly:

drivers/net/ethernet/aeroflex/greth.c:
macaddr[i] = (unsigned int) idprom->id_ethaddr[i];
drivers/net/ethernet/amd/sun3lance.c: dev->dev_addr[i] =
idprom->id_ethaddr[i];
drivers/net/ethernet/amd/sunlance.c: dev->dev_addr[i] =
idprom->id_ethaddr[i];
drivers/net/ethernet/broadcom/tg3.c: memcpy(dev->dev_addr,
idprom->id_ethaddr, ETH_ALEN);
drivers/net/ethernet/i825xx/sun3_82586.c: dev->dev_addr[i]
= idprom->id_ethaddr[i];
drivers/net/ethernet/sun/sunbmac.c: dev->dev_addr[i] =
idprom->id_ethaddr[i];
drivers/net/ethernet/sun/sungem.c: addr = idprom->id_ethaddr;
drivers/net/ethernet/sun/sunhme.c:
memcpy(dev->dev_addr, idprom->id_ethaddr, ETH_ALEN);
drivers/net/ethernet/sun/sunhme.c:
memcpy(dev->dev_addr, idprom->id_ethaddr, ETH_ALEN);
drivers/net/ethernet/sun/sunqe.c: memcpy(dev->dev_addr,
idprom->id_ethaddr, ETH_ALEN);

Arnd

2018-10-04 14:38:31

by Sowmini Varadhan

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

Just catching up on this thread, so please excuse any unintentional
misquotes here.

> > > > David: I couldn't find a place in sparc code where any ethernet device
> > > > would be registered, so is there a chance that nobody is using it?
> > >
> > > SPARC uses a true Open Firmware implementation, so it would register
> > > drivers through the CONFIG_OF infrastructure.

correct

> The calls are not even that old, and clearly added intentionally for sparc,
> see commit ba94272d08a7 ("i40e: use eth_platform_get_mac_address()")
> which added the first one.

Yes, correct again. Wouldn't PPC also end up doing the same thing?

See also commit c762dff24c06 (for ixgbe) - without this fix sparc systems
will come up with a bogus mac address and then you end up having to
manually fix this in ugly ways.

--Sowmini



2018-10-04 14:41:10

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 00/29] at24: remove at24_platform_data

On Thu, Oct 4, 2018 at 4:36 PM Sowmini Varadhan
<[email protected]> wrote:
>
> Just catching up on this thread, so please excuse any unintentional
> misquotes here.
>
> > > > > David: I couldn't find a place in sparc code where any ethernet device
> > > > > would be registered, so is there a chance that nobody is using it?
> > > >
> > > > SPARC uses a true Open Firmware implementation, so it would register
> > > > drivers through the CONFIG_OF infrastructure.
>
> correct
>
> > The calls are not even that old, and clearly added intentionally for sparc,
> > see commit ba94272d08a7 ("i40e: use eth_platform_get_mac_address()")
> > which added the first one.
>
> Yes, correct again. Wouldn't PPC also end up doing the same thing?
>
> See also commit c762dff24c06 (for ixgbe) - without this fix sparc systems
> will come up with a bogus mac address and then you end up having to
> manually fix this in ugly ways.

The of_get_mac_address() portion is not controversial at all I think,
the question was whether we need the fallback to
arch_get_platform_mac_address() in any of the drivers that
call eth_platform_get_mac_address().

Arnd