Messages in this thread | | | Subject | Re: [PATCH v2 4/6] nvmem: add support for the Khadas MCU Programmable User Memory | From | Srinivas Kandagatla <> | Date | Fri, 15 May 2020 11:55:10 +0100 |
| |
On 13/05/2020 13:33, Neil Armstrong wrote: > On 13/05/2020 12:34, Srinivas Kandagatla wrote: >> >> >> On 12/05/2020 14:26, Neil Armstrong wrote: >>> The new Khadas VIM2, VIM3 and Edge boards embeds an on-board microcontroller >>> offering a 56bytes User Programmable NVMEM array. >>> >>> This array needs a password to be writable, thus a password sysfs file >>> has been added on the device node to unlock the NVMEM. >> >> Is this one time programmable? Or does it need to be unlocked for every read/write? > > It can be rewritten, and needs the password to read & write. > >> >> How can you make sure that the memory is unlocked before making attempt to read/write? > > The only way to know it's unlock is reading back the password when unlocked.
Current code has no such checks for every read/write? and looks very disconnected w.r.t to password and read/writes.
If user attempts to read/write he will not be aware that he should program the password first!
Also if the password is static or un-changed then why not just statically program this from the driver rather than providing sysfs file?
I dont see how kernel nvmem read/write interface can make sure that memory is unlocked?
Who is the real consumer for this provider?
--srini
> >> >>> >>> The default 6bytes password id: "Khadas" >>> >>> This implements the user NVMEM devices as cell of the Khadas MCU MFD driver. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/nvmem/Kconfig | 8 ++ >>> drivers/nvmem/Makefile | 2 + >>> drivers/nvmem/khadas-mcu-user-mem.c | 128 ++++++++++++++++++++++++++++ >>> 3 files changed, 138 insertions(+) >>> create mode 100644 drivers/nvmem/khadas-mcu-user-mem.c >>> >>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig >>> index d7b7f6d688e7..92cd4f6aa931 100644 >>> --- a/drivers/nvmem/Kconfig >>> +++ b/drivers/nvmem/Kconfig >>> @@ -67,6 +67,14 @@ config JZ4780_EFUSE >>> To compile this driver as a module, choose M here: the module >>> will be called nvmem_jz4780_efuse. >>> +config NVMEM_KHADAS_MCU_USER_MEM >>> + tristate "Khadas MCU User programmable memory support" >>> + depends on MFD_KHADAS_MCU >>> + depends on REGMAP >>> + help >>> + This is a driver for the MCU User programmable memory >>> + available on the Khadas VIM and Edge boards. >>> + >>> config NVMEM_LPC18XX_EEPROM >>> tristate "NXP LPC18XX EEPROM Memory Support" >>> depends on ARCH_LPC18XX || COMPILE_TEST >>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile >>> index a7c377218341..0516a309542d 100644 >>> --- a/drivers/nvmem/Makefile >>> +++ b/drivers/nvmem/Makefile >>> @@ -17,6 +17,8 @@ obj-$(CONFIG_NVMEM_IMX_OCOTP_SCU) += nvmem-imx-ocotp-scu.o >>> nvmem-imx-ocotp-scu-y := imx-ocotp-scu.o >>> obj-$(CONFIG_JZ4780_EFUSE) += nvmem_jz4780_efuse.o >>> nvmem_jz4780_efuse-y := jz4780-efuse.o >>> +obj-$(CONFIG_NVMEM_KHADAS_MCU_USER_MEM) += nvmem-khadas-mcu-user-mem.o >>> +nvmem-khadas-mcu-user-mem-y := khadas-mcu-user-mem.o >>> obj-$(CONFIG_NVMEM_LPC18XX_EEPROM) += nvmem_lpc18xx_eeprom.o >>> nvmem_lpc18xx_eeprom-y := lpc18xx_eeprom.o >>> obj-$(CONFIG_NVMEM_LPC18XX_OTP) += nvmem_lpc18xx_otp.o >>> diff --git a/drivers/nvmem/khadas-mcu-user-mem.c b/drivers/nvmem/khadas-mcu-user-mem.c >>> new file mode 100644 >>> index 000000000000..a1d5ae9a030c >>> --- /dev/null >>> +++ b/drivers/nvmem/khadas-mcu-user-mem.c >>> @@ -0,0 +1,128 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Driver for Khadas MCU User programmable Memory >>> + * >>> + * Copyright (C) 2020 BayLibre SAS >>> + * Author(s): Neil Armstrong <narmstrong@baylibre.com> >>> + */ >>> + >>> +#include <linux/clk.h> >> >> Why do we need this header? > > Will drop > >> >>> +#include <linux/module.h> >>> +#include <linux/nvmem-provider.h> >>> +#include <linux/mfd/khadas-mcu.h> >>> +#include <linux/regmap.h> >>> +#include <linux/of.h> >>> +#include <linux/platform_device.h> >>> + >>> +static int khadas_mcu_user_mem_read(void *context, unsigned int offset, >>> + void *val, size_t bytes) >>> +{ >>> + struct khadas_mcu *khadas_mcu = context; >>> + >>> + return regmap_bulk_read(khadas_mcu->map, >>> + KHADAS_MCU_USER_DATA_0_REG + offset, >>> + val, bytes); >>> +} >>> + >>> +static int khadas_mcu_user_mem_write(void *context, unsigned int offset, >>> + void *val, size_t bytes) >>> +{ >>> + struct khadas_mcu *khadas_mcu = context; >>> + >>> + return regmap_bulk_write(khadas_mcu->map, >>> + KHADAS_MCU_USER_DATA_0_REG + offset, >>> + val, bytes); >>> +} >>> + >>> +static ssize_t password_store(struct device *dev, struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct khadas_mcu *khadas_mcu = dev_get_drvdata(dev); >>> + int i, ret; >>> + >>> + if (count < 6) >>> + return -EINVAL; >> >> Possibly this magic 6 value needs proper define. An additional check here for > 6 would be better as well. > > Ok > >> >>> + >>> + ret = regmap_write(khadas_mcu->map, KHADAS_MCU_PASSWD_START_REG, 1); >>> + if (ret) >>> + return ret; >>> + >>> + for (i = 0 ; i < 6 ; ++i) { >>> + ret = regmap_write(khadas_mcu->map, >>> + KHADAS_MCU_CHECK_USER_PASSWD_REG, >>> + buf[i]); >>> + if (ret) >>> + goto out; >>> + } >>> + >>> + ret = regmap_write(khadas_mcu->map, KHADAS_MCU_PASSWD_START_REG, 0); >>> + if (ret) >>> + return ret; >>> + >>> + return count; >>> +out: >>> + regmap_write(khadas_mcu->map, KHADAS_MCU_PASSWD_START_REG, 0); >>> + >>> + return ret; >>> +} >>> + >>> +static DEVICE_ATTR_WO(password); >>> + >>> +static struct attribute *khadas_mcu_user_mem_sysfs_attributes[] = { >>> + &dev_attr_password.attr, >>> + NULL, >>> +}; >>> + >>> +static const struct attribute_group khadas_mcu_user_mem_sysfs_attr_group = { >>> + .attrs = khadas_mcu_user_mem_sysfs_attributes, >>> +}; >>> + >>> +static int khadas_mcu_user_mem_probe(struct platform_device *pdev) >>> +{ >>> + struct khadas_mcu *khadas_mcu = dev_get_drvdata(pdev->dev.parent); >> >> You could probably get away with dependency of this structure and the header itself by directly getting hold of regmap from parent device. > > Ok > >> >> >>> + struct device *dev = &pdev->dev; >>> + struct nvmem_device *nvmem; >>> + struct nvmem_config *econfig; >>> + >>> + econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL); >>> + if (!econfig) >>> + return -ENOMEM; >>> + >>> + econfig->dev = pdev->dev.parent; >>> + econfig->name = dev_name(pdev->dev.parent); >>> + econfig->stride = 1; >>> + econfig->word_size = 1; >>> + econfig->reg_read = khadas_mcu_user_mem_read; >>> + econfig->reg_write = khadas_mcu_user_mem_write; >>> + econfig->size = 56; >> define 56 to make it more readable! > > Ok > >> >>> + econfig->priv = khadas_mcu; >>> + >>> + platform_set_drvdata(pdev, khadas_mcu); >>> + >>> + nvmem = devm_nvmem_register(&pdev->dev, econfig); >>> + if (IS_ERR(nvmem)) >>> + return PTR_ERR(nvmem); >>> + >>> + return sysfs_create_group(&pdev->dev.kobj, >>> + &khadas_mcu_user_mem_sysfs_attr_group); >>> +} >>> + >>> +static const struct platform_device_id khadas_mcu_user_mem_id_table[] = { >>> + { .name = "khadas-mcu-user-mem", }, >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(platform, khadas_mcu_user_mem_id_table); >>> + >>> +static struct platform_driver khadas_mcu_user_mem_driver = { >>> + .probe = khadas_mcu_user_mem_probe, >>> + .driver = { >>> + .name = "khadas-mcu-user-mem", >>> + }, >>> + .id_table = khadas_mcu_user_mem_id_table, >>> +}; >>> + >>> +module_platform_driver(khadas_mcu_user_mem_driver); >>> + >>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>"); >>> +MODULE_DESCRIPTION("Khadas MCU User MEM driver"); >>> +MODULE_LICENSE("GPL v2"); >>> > > Thanks for the review. > > Neil >
| |