2023-05-11 15:52:54

by Arseniy Krasnov

[permalink] [raw]
Subject: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC

This adds support for OTP area access on MX30LFxG18AC chip series.

Changelog:
v1 -> v2:
* Add slab.h include due to kernel test robot error.
v2 -> v3:
* Use 'uint64_t' as input argument for 'do_div()' instead
of 'unsigned long' due to kernel test robot error.

Signed-off-by: Arseniy Krasnov <[email protected]>
---
drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
1 file changed, 213 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
index 1472f925f386..2301f990678e 100644
--- a/drivers/mtd/nand/raw/nand_macronix.c
+++ b/drivers/mtd/nand/raw/nand_macronix.c
@@ -6,6 +6,7 @@
* Author: Boris Brezillon <[email protected]>
*/

+#include <linux/slab.h>
#include "linux/delay.h"
#include "internals.h"

@@ -31,6 +32,20 @@

#define MXIC_CMD_POWER_DOWN 0xB9

+#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
+#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
+#define MACRONIX_30LFXG18AC_OTP_PAGES 30
+#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
+#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
+ (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
+ MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
+#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
+ (MACRONIX_30LFXG18AC_OTP_PAGES * \
+ MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
+
+#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
+#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
+
struct nand_onfi_vendor_macronix {
u8 reserved;
u8 reliability_func;
@@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
chip->ops.resume = mxic_nand_resume;
}

+static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen,
+ struct otp_info *buf)
+{
+ if (len < sizeof(*buf))
+ return -EINVAL;
+
+ /* Don't know how to check that OTP is locked. */
+ buf->locked = 0;
+ buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
+ buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
+
+ *retlen = sizeof(*buf);
+
+ return 0;
+}
+
+static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
+{
+ uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
+
+ feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
+ return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
+ feature_buf);
+}
+
+static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
+{
+ uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
+
+ return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
+ feature_buf);
+}
+
+static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
+ loff_t offs_in_flash,
+ size_t len, size_t *retlen,
+ u_char *buf, bool write)
+{
+ struct nand_chip *nand;
+ size_t bytes_handled;
+ off_t offs_in_page;
+ uint64_t page;
+ void *dma_buf;
+ int ret;
+
+ /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
+ * so allocate properly aligned memory for it. This is
+ * needed because cross page accesses may lead to unaligned
+ * buffer address for DMA.
+ */
+ dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
+ if (!dma_buf)
+ return -ENOMEM;
+
+ nand = mtd_to_nand(mtd);
+ nand_select_target(nand, 0);
+
+ ret = macronix_30lfxg18ac_otp_enable(nand);
+ if (ret)
+ goto out_otp;
+
+ page = offs_in_flash;
+ /* 'page' will be result of division. */
+ offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
+ bytes_handled = 0;
+
+ while (bytes_handled < len &&
+ page < MACRONIX_30LFXG18AC_OTP_PAGES) {
+ size_t bytes_to_handle;
+
+ bytes_to_handle = min_t(size_t, len - bytes_handled,
+ MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
+ offs_in_page);
+
+ if (write) {
+ memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
+ ret = nand_prog_page_op(nand, page, offs_in_page,
+ dma_buf, bytes_to_handle);
+ } else {
+ ret = nand_read_page_op(nand, page, offs_in_page,
+ dma_buf, bytes_to_handle);
+ if (!ret)
+ memcpy(&buf[bytes_handled], dma_buf,
+ bytes_to_handle);
+ }
+ if (ret)
+ goto out_otp;
+
+ bytes_handled += bytes_to_handle;
+ offs_in_page = 0;
+ page++;
+ }
+
+ *retlen = bytes_handled;
+
+out_otp:
+ if (ret)
+ dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
+
+ ret = macronix_30lfxg18ac_otp_disable(nand);
+ WARN(ret, "failed to leave OTP mode after %s\n",
+ write ? "write" : "read");
+ nand_deselect_target(nand);
+ kfree(dma_buf);
+
+ return ret;
+}
+
+static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
+ size_t len, size_t *rlen,
+ const u_char *buf)
+{
+ return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
+ true);
+}
+
+static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
+ size_t len, size_t *rlen,
+ u_char *buf)
+{
+ return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
+}
+
+static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
+ size_t len)
+{
+ uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
+ struct nand_chip *nand;
+ int ret;
+
+ if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
+ len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
+ return -EINVAL;
+
+ dev_dbg(&mtd->dev, "locking OTP\n");
+
+ nand = mtd_to_nand(mtd);
+ nand_select_target(nand, 0);
+
+ feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
+ MACRONIX_30LFXG18AC_OTP_LOCKED;
+ ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
+ feature_buf);
+ if (ret) {
+ dev_err(&mtd->dev,
+ "failed to lock OTP (set features): %i\n", ret);
+ nand_deselect_target(nand);
+ return ret;
+ }
+
+ /* Do dummy page prog with zero address. */
+ feature_buf[0] = 0;
+ ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
+ if (ret)
+ dev_err(&mtd->dev,
+ "failed to lock OTP (page prog): %i\n", ret);
+
+ ret = macronix_30lfxg18ac_otp_disable(nand);
+ WARN(ret, "failed to leave OTP mode after lock\n");
+
+ nand_deselect_target(nand);
+
+ return ret;
+}
+
+static void macronix_nand_setup_otp(struct nand_chip *chip)
+{
+ static const char * const supported_otp_models[] = {
+ "MX30LF1G18AC",
+ "MX30LF2G18AC",
+ "MX30LF4G18AC",
+ };
+ struct mtd_info *mtd;
+
+ if (!chip->parameters.supports_set_get_features)
+ return;
+
+ if (match_string(supported_otp_models,
+ ARRAY_SIZE(supported_otp_models),
+ chip->parameters.model) < 0)
+ return;
+
+ bitmap_set(chip->parameters.get_feature_list,
+ ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
+ bitmap_set(chip->parameters.set_feature_list,
+ ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
+
+ mtd = nand_to_mtd(chip);
+ mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
+ mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
+ mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
+ mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
+ mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
+ mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
+}
+
static int macronix_nand_init(struct nand_chip *chip)
{
if (nand_is_slc(chip))
@@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
macronix_nand_onfi_init(chip);
macronix_nand_block_protection_support(chip);
macronix_nand_deep_power_down_support(chip);
+ macronix_nand_setup_otp(chip);

return 0;
}
--
2.35.0



2023-05-11 18:29:21

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC

Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>

On 11.05.2023 18:21, Arseniy Krasnov wrote:
> This adds support for OTP area access on MX30LFxG18AC chip series.
>
> Changelog:
> v1 -> v2:
> * Add slab.h include due to kernel test robot error.
> v2 -> v3:
> * Use 'uint64_t' as input argument for 'do_div()' instead
> of 'unsigned long' due to kernel test robot error.
>
> Signed-off-by: Arseniy Krasnov <[email protected]>
> ---
> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
> 1 file changed, 213 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> index 1472f925f386..2301f990678e 100644
> --- a/drivers/mtd/nand/raw/nand_macronix.c
> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> @@ -6,6 +6,7 @@
> * Author: Boris Brezillon <[email protected]>
> */
>
> +#include <linux/slab.h>
> #include "linux/delay.h"
> #include "internals.h"
>
> @@ -31,6 +32,20 @@
>
> #define MXIC_CMD_POWER_DOWN 0xB9
>
> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> +
> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
> +
> struct nand_onfi_vendor_macronix {
> u8 reserved;
> u8 reliability_func;
> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
> chip->ops.resume = mxic_nand_resume;
> }
>
> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
> + size_t *retlen,
> + struct otp_info *buf)
> +{
> + if (len < sizeof(*buf))
> + return -EINVAL;
> +
> + /* Don't know how to check that OTP is locked. */
> + buf->locked = 0;
> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
> +
> + *retlen = sizeof(*buf);
> +
> + return 0;
> +}
> +
> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
> +{
> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> +
> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> + feature_buf);
> +}
> +
> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
> +{
> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> +
> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> + feature_buf);
> +}
> +
> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
> + loff_t offs_in_flash,
> + size_t len, size_t *retlen,
> + u_char *buf, bool write)
> +{
> + struct nand_chip *nand;
> + size_t bytes_handled;
> + off_t offs_in_page;
> + uint64_t page;
> + void *dma_buf;
> + int ret;
> +
> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
> + * so allocate properly aligned memory for it. This is
> + * needed because cross page accesses may lead to unaligned
> + * buffer address for DMA.
> + */
> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
> + if (!dma_buf)
> + return -ENOMEM;
> +
> + nand = mtd_to_nand(mtd);
> + nand_select_target(nand, 0);
> +
> + ret = macronix_30lfxg18ac_otp_enable(nand);
> + if (ret)
> + goto out_otp;
> +
> + page = offs_in_flash;
> + /* 'page' will be result of division. */
> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
> + bytes_handled = 0;
> +
> + while (bytes_handled < len &&
> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
> + size_t bytes_to_handle;
> +
> + bytes_to_handle = min_t(size_t, len - bytes_handled,
> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
> + offs_in_page);
> +
> + if (write) {
> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
> + ret = nand_prog_page_op(nand, page, offs_in_page,
> + dma_buf, bytes_to_handle);
> + } else {
> + ret = nand_read_page_op(nand, page, offs_in_page,
> + dma_buf, bytes_to_handle);
> + if (!ret)
> + memcpy(&buf[bytes_handled], dma_buf,
> + bytes_to_handle);
> + }
> + if (ret)
> + goto out_otp;
> +
> + bytes_handled += bytes_to_handle;
> + offs_in_page = 0;
> + page++;
> + }
> +
> + *retlen = bytes_handled;
> +
> +out_otp:
> + if (ret)
> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
> +
> + ret = macronix_30lfxg18ac_otp_disable(nand);
> + WARN(ret, "failed to leave OTP mode after %s\n",
> + write ? "write" : "read");
> + nand_deselect_target(nand);
> + kfree(dma_buf);
> +
> + return ret;
> +}
> +
> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
> + size_t len, size_t *rlen,
> + const u_char *buf)
> +{
> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
> + true);
> +}
> +
> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
> + size_t len, size_t *rlen,
> + u_char *buf)
> +{
> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
> +}
> +
> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
> + size_t len)
> +{
> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> + struct nand_chip *nand;
> + int ret;
> +
> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
> + return -EINVAL;
> +
> + dev_dbg(&mtd->dev, "locking OTP\n");
> +
> + nand = mtd_to_nand(mtd);
> + nand_select_target(nand, 0);
> +
> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
> + MACRONIX_30LFXG18AC_OTP_LOCKED;
> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> + feature_buf);
> + if (ret) {
> + dev_err(&mtd->dev,
> + "failed to lock OTP (set features): %i\n", ret);
> + nand_deselect_target(nand);
> + return ret;
> + }
> +
> + /* Do dummy page prog with zero address. */
> + feature_buf[0] = 0;
> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
> + if (ret)
> + dev_err(&mtd->dev,
> + "failed to lock OTP (page prog): %i\n", ret);
> +
> + ret = macronix_30lfxg18ac_otp_disable(nand);
> + WARN(ret, "failed to leave OTP mode after lock\n");
> +
> + nand_deselect_target(nand);
> +
> + return ret;
> +}
> +
> +static void macronix_nand_setup_otp(struct nand_chip *chip)
> +{
> + static const char * const supported_otp_models[] = {
> + "MX30LF1G18AC",
> + "MX30LF2G18AC",
> + "MX30LF4G18AC",
> + };
> + struct mtd_info *mtd;
> +
> + if (!chip->parameters.supports_set_get_features)
> + return;
> +
> + if (match_string(supported_otp_models,
> + ARRAY_SIZE(supported_otp_models),
> + chip->parameters.model) < 0)
> + return;
> +
> + bitmap_set(chip->parameters.get_feature_list,
> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> + bitmap_set(chip->parameters.set_feature_list,
> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> +
> + mtd = nand_to_mtd(chip);
> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
> +}
> +
> static int macronix_nand_init(struct nand_chip *chip)
> {
> if (nand_is_slc(chip))
> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
> macronix_nand_onfi_init(chip);
> macronix_nand_block_protection_support(chip);
> macronix_nand_deep_power_down_support(chip);
> + macronix_nand_setup_otp(chip);
>
> return 0;
> }

2023-05-15 10:08:44

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC

Hello @Miquel!

Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
someone who knows it well? I tested this patch on our devices (with already known Meson NAND
controller).

Thanks, Arseniy

On 11.05.2023 21:21, Arseniy Krasnov wrote:
> Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
>
> On 11.05.2023 18:21, Arseniy Krasnov wrote:
>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>
>> Changelog:
>> v1 -> v2:
>> * Add slab.h include due to kernel test robot error.
>> v2 -> v3:
>> * Use 'uint64_t' as input argument for 'do_div()' instead
>> of 'unsigned long' due to kernel test robot error.
>>
>> Signed-off-by: Arseniy Krasnov <[email protected]>
>> ---
>> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
>> 1 file changed, 213 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>> index 1472f925f386..2301f990678e 100644
>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>> @@ -6,6 +6,7 @@
>> * Author: Boris Brezillon <[email protected]>
>> */
>>
>> +#include <linux/slab.h>
>> #include "linux/delay.h"
>> #include "internals.h"
>>
>> @@ -31,6 +32,20 @@
>>
>> #define MXIC_CMD_POWER_DOWN 0xB9
>>
>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>> +
>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>> +
>> struct nand_onfi_vendor_macronix {
>> u8 reserved;
>> u8 reliability_func;
>> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>> chip->ops.resume = mxic_nand_resume;
>> }
>>
>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>> + size_t *retlen,
>> + struct otp_info *buf)
>> +{
>> + if (len < sizeof(*buf))
>> + return -EINVAL;
>> +
>> + /* Don't know how to check that OTP is locked. */
>> + buf->locked = 0;
>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>> +
>> + *retlen = sizeof(*buf);
>> +
>> + return 0;
>> +}
>> +
>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>> +{
>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>> +
>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>> + feature_buf);
>> +}
>> +
>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>> +{
>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>> +
>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>> + feature_buf);
>> +}
>> +
>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>> + loff_t offs_in_flash,
>> + size_t len, size_t *retlen,
>> + u_char *buf, bool write)
>> +{
>> + struct nand_chip *nand;
>> + size_t bytes_handled;
>> + off_t offs_in_page;
>> + uint64_t page;
>> + void *dma_buf;
>> + int ret;
>> +
>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>> + * so allocate properly aligned memory for it. This is
>> + * needed because cross page accesses may lead to unaligned
>> + * buffer address for DMA.
>> + */
>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>> + if (!dma_buf)
>> + return -ENOMEM;
>> +
>> + nand = mtd_to_nand(mtd);
>> + nand_select_target(nand, 0);
>> +
>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>> + if (ret)
>> + goto out_otp;
>> +
>> + page = offs_in_flash;
>> + /* 'page' will be result of division. */
>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>> + bytes_handled = 0;
>> +
>> + while (bytes_handled < len &&
>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>> + size_t bytes_to_handle;
>> +
>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>> + offs_in_page);
>> +
>> + if (write) {
>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>> + dma_buf, bytes_to_handle);
>> + } else {
>> + ret = nand_read_page_op(nand, page, offs_in_page,
>> + dma_buf, bytes_to_handle);
>> + if (!ret)
>> + memcpy(&buf[bytes_handled], dma_buf,
>> + bytes_to_handle);
>> + }
>> + if (ret)
>> + goto out_otp;
>> +
>> + bytes_handled += bytes_to_handle;
>> + offs_in_page = 0;
>> + page++;
>> + }
>> +
>> + *retlen = bytes_handled;
>> +
>> +out_otp:
>> + if (ret)
>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>> +
>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>> + WARN(ret, "failed to leave OTP mode after %s\n",
>> + write ? "write" : "read");
>> + nand_deselect_target(nand);
>> + kfree(dma_buf);
>> +
>> + return ret;
>> +}
>> +
>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>> + size_t len, size_t *rlen,
>> + const u_char *buf)
>> +{
>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>> + true);
>> +}
>> +
>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>> + size_t len, size_t *rlen,
>> + u_char *buf)
>> +{
>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>> +}
>> +
>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>> + size_t len)
>> +{
>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>> + struct nand_chip *nand;
>> + int ret;
>> +
>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>> + return -EINVAL;
>> +
>> + dev_dbg(&mtd->dev, "locking OTP\n");
>> +
>> + nand = mtd_to_nand(mtd);
>> + nand_select_target(nand, 0);
>> +
>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>> + feature_buf);
>> + if (ret) {
>> + dev_err(&mtd->dev,
>> + "failed to lock OTP (set features): %i\n", ret);
>> + nand_deselect_target(nand);
>> + return ret;
>> + }
>> +
>> + /* Do dummy page prog with zero address. */
>> + feature_buf[0] = 0;
>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>> + if (ret)
>> + dev_err(&mtd->dev,
>> + "failed to lock OTP (page prog): %i\n", ret);
>> +
>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>> + WARN(ret, "failed to leave OTP mode after lock\n");
>> +
>> + nand_deselect_target(nand);
>> +
>> + return ret;
>> +}
>> +
>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>> +{
>> + static const char * const supported_otp_models[] = {
>> + "MX30LF1G18AC",
>> + "MX30LF2G18AC",
>> + "MX30LF4G18AC",
>> + };
>> + struct mtd_info *mtd;
>> +
>> + if (!chip->parameters.supports_set_get_features)
>> + return;
>> +
>> + if (match_string(supported_otp_models,
>> + ARRAY_SIZE(supported_otp_models),
>> + chip->parameters.model) < 0)
>> + return;
>> +
>> + bitmap_set(chip->parameters.get_feature_list,
>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>> + bitmap_set(chip->parameters.set_feature_list,
>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>> +
>> + mtd = nand_to_mtd(chip);
>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>> +}
>> +
>> static int macronix_nand_init(struct nand_chip *chip)
>> {
>> if (nand_is_slc(chip))
>> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>> macronix_nand_onfi_init(chip);
>> macronix_nand_block_protection_support(chip);
>> macronix_nand_deep_power_down_support(chip);
>> + macronix_nand_setup_otp(chip);
>>
>> return 0;
>> }

2023-05-22 14:29:35

by Miquel Raynal

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC

Hi Arseniy,

[email protected] wrote on Mon, 15 May 2023 12:49:50 +0300:

> Hello @Miquel!
>
> Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
> someone who knows it well? I tested this patch on our devices (with already known Meson NAND
> controller).

+ Jaime, who might test

>
> Thanks, Arseniy
>
> On 11.05.2023 21:21, Arseniy Krasnov wrote:
> > Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
> >
> > On 11.05.2023 18:21, Arseniy Krasnov wrote:
> >> This adds support for OTP area access on MX30LFxG18AC chip series.
> >>
> >> Changelog:
> >> v1 -> v2:
> >> * Add slab.h include due to kernel test robot error.
> >> v2 -> v3:
> >> * Use 'uint64_t' as input argument for 'do_div()' instead
> >> of 'unsigned long' due to kernel test robot error.
> >>
> >> Signed-off-by: Arseniy Krasnov <[email protected]>
> >> ---
> >> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
> >> 1 file changed, 213 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> >> index 1472f925f386..2301f990678e 100644
> >> --- a/drivers/mtd/nand/raw/nand_macronix.c
> >> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> >> @@ -6,6 +6,7 @@
> >> * Author: Boris Brezillon <[email protected]>
> >> */
> >>
> >> +#include <linux/slab.h>
> >> #include "linux/delay.h"
> >> #include "internals.h"
> >>
> >> @@ -31,6 +32,20 @@
> >>
> >> #define MXIC_CMD_POWER_DOWN 0xB9
> >>
> >> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
> >> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
> >> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
> >> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
> >> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
> >> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
> >> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> >> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
> >> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
> >> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> >> +
> >> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
> >> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
> >> +
> >> struct nand_onfi_vendor_macronix {
> >> u8 reserved;
> >> u8 reliability_func;
> >> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
> >> chip->ops.resume = mxic_nand_resume;
> >> }
> >>
> >> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
> >> + size_t *retlen,
> >> + struct otp_info *buf)
> >> +{
> >> + if (len < sizeof(*buf))
> >> + return -EINVAL;
> >> +
> >> + /* Don't know how to check that OTP is locked. */

Jaime, any idea?

> >> + buf->locked = 0;
> >> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
> >> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
> >> +
> >> + *retlen = sizeof(*buf);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
> >> +{
> >> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> >> +
> >> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
> >> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> >> + feature_buf);
> >> +}
> >> +
> >> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
> >> +{
> >> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> >> +
> >> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> >> + feature_buf);
> >> +}
> >> +
> >> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
> >> + loff_t offs_in_flash,
> >> + size_t len, size_t *retlen,
> >> + u_char *buf, bool write)
> >> +{
> >> + struct nand_chip *nand;
> >> + size_t bytes_handled;
> >> + off_t offs_in_page;
> >> + uint64_t page;
> >> + void *dma_buf;
> >> + int ret;
> >> +
> >> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
> >> + * so allocate properly aligned memory for it. This is
> >> + * needed because cross page accesses may lead to unaligned
> >> + * buffer address for DMA.
> >> + */
> >> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
> >> + if (!dma_buf)
> >> + return -ENOMEM;
> >> +
> >> + nand = mtd_to_nand(mtd);
> >> + nand_select_target(nand, 0);
> >> +
> >> + ret = macronix_30lfxg18ac_otp_enable(nand);
> >> + if (ret)
> >> + goto out_otp;
> >> +
> >> + page = offs_in_flash;
> >> + /* 'page' will be result of division. */
> >> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
> >> + bytes_handled = 0;
> >> +
> >> + while (bytes_handled < len &&
> >> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
> >> + size_t bytes_to_handle;
> >> +
> >> + bytes_to_handle = min_t(size_t, len - bytes_handled,
> >> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
> >> + offs_in_page);
> >> +
> >> + if (write) {
> >> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
> >> + ret = nand_prog_page_op(nand, page, offs_in_page,
> >> + dma_buf, bytes_to_handle);
> >> + } else {
> >> + ret = nand_read_page_op(nand, page, offs_in_page,
> >> + dma_buf, bytes_to_handle);
> >> + if (!ret)
> >> + memcpy(&buf[bytes_handled], dma_buf,
> >> + bytes_to_handle);
> >> + }
> >> + if (ret)
> >> + goto out_otp;
> >> +
> >> + bytes_handled += bytes_to_handle;
> >> + offs_in_page = 0;
> >> + page++;
> >> + }
> >> +
> >> + *retlen = bytes_handled;
> >> +
> >> +out_otp:
> >> + if (ret)
> >> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
> >> +
> >> + ret = macronix_30lfxg18ac_otp_disable(nand);
> >> + WARN(ret, "failed to leave OTP mode after %s\n",
> >> + write ? "write" : "read");

Let's avoid WARN() calls (none in this driver are relevant IMHO). Just a
dev_err() should be enough.

> >> + nand_deselect_target(nand);
> >> + kfree(dma_buf);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
> >> + size_t len, size_t *rlen,
> >> + const u_char *buf)
> >> +{
> >> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
> >> + true);
> >> +}
> >> +
> >> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
> >> + size_t len, size_t *rlen,
> >> + u_char *buf)
> >> +{
> >> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
> >> +}
> >> +
> >> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
> >> + size_t len)
> >> +{
> >> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> >> + struct nand_chip *nand;
> >> + int ret;
> >> +
> >> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
> >> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
> >> + return -EINVAL;
> >> +
> >> + dev_dbg(&mtd->dev, "locking OTP\n");
> >> +
> >> + nand = mtd_to_nand(mtd);
> >> + nand_select_target(nand, 0);
> >> +
> >> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
> >> + MACRONIX_30LFXG18AC_OTP_LOCKED;
> >> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> >> + feature_buf);
> >> + if (ret) {
> >> + dev_err(&mtd->dev,
> >> + "failed to lock OTP (set features): %i\n", ret);
> >> + nand_deselect_target(nand);
> >> + return ret;
> >> + }
> >> +
> >> + /* Do dummy page prog with zero address. */
> >> + feature_buf[0] = 0;
> >> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
> >> + if (ret)
> >> + dev_err(&mtd->dev,
> >> + "failed to lock OTP (page prog): %i\n", ret);
> >> +
> >> + ret = macronix_30lfxg18ac_otp_disable(nand);
> >> + WARN(ret, "failed to leave OTP mode after lock\n");
> >> +
> >> + nand_deselect_target(nand);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void macronix_nand_setup_otp(struct nand_chip *chip)
> >> +{
> >> + static const char * const supported_otp_models[] = {
> >> + "MX30LF1G18AC",
> >> + "MX30LF2G18AC",
> >> + "MX30LF4G18AC",
> >> + };
> >> + struct mtd_info *mtd;
> >> +
> >> + if (!chip->parameters.supports_set_get_features)
> >> + return;
> >> +
> >> + if (match_string(supported_otp_models,
> >> + ARRAY_SIZE(supported_otp_models),
> >> + chip->parameters.model) < 0)
> >> + return;

I would start by checking the model, then it's list of supported ops.

> >> +
> >> + bitmap_set(chip->parameters.get_feature_list,
> >> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> >> + bitmap_set(chip->parameters.set_feature_list,
> >> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> >> +
> >> + mtd = nand_to_mtd(chip);
> >> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
> >> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
> >> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
> >> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
> >> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
> >> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
> >> +}
> >> +
> >> static int macronix_nand_init(struct nand_chip *chip)
> >> {
> >> if (nand_is_slc(chip))
> >> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
> >> macronix_nand_onfi_init(chip);
> >> macronix_nand_block_protection_support(chip);
> >> macronix_nand_deep_power_down_support(chip);
> >> + macronix_nand_setup_otp(chip);
> >>
> >> return 0;
> >> }


Thanks,
Miquèl

2023-05-23 10:41:48

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC



On 22.05.2023 17:14, Miquel Raynal wrote:
> Hi Arseniy,
>
> [email protected] wrote on Mon, 15 May 2023 12:49:50 +0300:
>
>> Hello @Miquel!
>>
>> Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
>> someone who knows it well? I tested this patch on our devices (with already known Meson NAND
>> controller).
>
> + Jaime, who might test
>
>>
>> Thanks, Arseniy
>>
>> On 11.05.2023 21:21, Arseniy Krasnov wrote:
>>> Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
>>>
>>> On 11.05.2023 18:21, Arseniy Krasnov wrote:
>>>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>>>
>>>> Changelog:
>>>> v1 -> v2:
>>>> * Add slab.h include due to kernel test robot error.
>>>> v2 -> v3:
>>>> * Use 'uint64_t' as input argument for 'do_div()' instead
>>>> of 'unsigned long' due to kernel test robot error.
>>>>
>>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>>> ---
>>>> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
>>>> 1 file changed, 213 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>>>> index 1472f925f386..2301f990678e 100644
>>>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>>>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>>>> @@ -6,6 +6,7 @@
>>>> * Author: Boris Brezillon <[email protected]>
>>>> */
>>>>
>>>> +#include <linux/slab.h>
>>>> #include "linux/delay.h"
>>>> #include "internals.h"
>>>>
>>>> @@ -31,6 +32,20 @@
>>>>
>>>> #define MXIC_CMD_POWER_DOWN 0xB9
>>>>
>>>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>>>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>>>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>>>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>>>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>> +
>>>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>>>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>>>> +
>>>> struct nand_onfi_vendor_macronix {
>>>> u8 reserved;
>>>> u8 reliability_func;
>>>> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>>>> chip->ops.resume = mxic_nand_resume;
>>>> }
>>>>
>>>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>>>> + size_t *retlen,
>>>> + struct otp_info *buf)
>>>> +{
>>>> + if (len < sizeof(*buf))
>>>> + return -EINVAL;
>>>> +
>>>> + /* Don't know how to check that OTP is locked. */
>
> Jaime, any idea?
>

That will be great, because i didn't found any doc about that

Thanks

>>>> + buf->locked = 0;
>>>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>>>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>>> +
>>>> + *retlen = sizeof(*buf);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>>>> +{
>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>> +
>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>> + feature_buf);
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>>>> +{
>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>> +
>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>> + feature_buf);
>>>> +}
>>>> +
>>>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>>>> + loff_t offs_in_flash,
>>>> + size_t len, size_t *retlen,
>>>> + u_char *buf, bool write)
>>>> +{
>>>> + struct nand_chip *nand;
>>>> + size_t bytes_handled;
>>>> + off_t offs_in_page;
>>>> + uint64_t page;
>>>> + void *dma_buf;
>>>> + int ret;
>>>> +
>>>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>>>> + * so allocate properly aligned memory for it. This is
>>>> + * needed because cross page accesses may lead to unaligned
>>>> + * buffer address for DMA.
>>>> + */
>>>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>>>> + if (!dma_buf)
>>>> + return -ENOMEM;
>>>> +
>>>> + nand = mtd_to_nand(mtd);
>>>> + nand_select_target(nand, 0);
>>>> +
>>>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>>>> + if (ret)
>>>> + goto out_otp;
>>>> +
>>>> + page = offs_in_flash;
>>>> + /* 'page' will be result of division. */
>>>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>>>> + bytes_handled = 0;
>>>> +
>>>> + while (bytes_handled < len &&
>>>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>>>> + size_t bytes_to_handle;
>>>> +
>>>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>>>> + offs_in_page);
>>>> +
>>>> + if (write) {
>>>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>>>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>>>> + dma_buf, bytes_to_handle);
>>>> + } else {
>>>> + ret = nand_read_page_op(nand, page, offs_in_page,
>>>> + dma_buf, bytes_to_handle);
>>>> + if (!ret)
>>>> + memcpy(&buf[bytes_handled], dma_buf,
>>>> + bytes_to_handle);
>>>> + }
>>>> + if (ret)
>>>> + goto out_otp;
>>>> +
>>>> + bytes_handled += bytes_to_handle;
>>>> + offs_in_page = 0;
>>>> + page++;
>>>> + }
>>>> +
>>>> + *retlen = bytes_handled;
>>>> +
>>>> +out_otp:
>>>> + if (ret)
>>>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>>>> +
>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>> + WARN(ret, "failed to leave OTP mode after %s\n",
>>>> + write ? "write" : "read");
>
> Let's avoid WARN() calls (none in this driver are relevant IMHO). Just a
> dev_err() should be enough.

Done in v4

>
>>>> + nand_deselect_target(nand);
>>>> + kfree(dma_buf);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>>>> + size_t len, size_t *rlen,
>>>> + const u_char *buf)
>>>> +{
>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>>>> + true);
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>>>> + size_t len, size_t *rlen,
>>>> + u_char *buf)
>>>> +{
>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>>>> +}
>>>> +
>>>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>>>> + size_t len)
>>>> +{
>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>> + struct nand_chip *nand;
>>>> + int ret;
>>>> +
>>>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>>>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>>>> + return -EINVAL;
>>>> +
>>>> + dev_dbg(&mtd->dev, "locking OTP\n");
>>>> +
>>>> + nand = mtd_to_nand(mtd);
>>>> + nand_select_target(nand, 0);
>>>> +
>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>>>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>>>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>> + feature_buf);
>>>> + if (ret) {
>>>> + dev_err(&mtd->dev,
>>>> + "failed to lock OTP (set features): %i\n", ret);
>>>> + nand_deselect_target(nand);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /* Do dummy page prog with zero address. */
>>>> + feature_buf[0] = 0;
>>>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>>>> + if (ret)
>>>> + dev_err(&mtd->dev,
>>>> + "failed to lock OTP (page prog): %i\n", ret);
>>>> +
>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>> + WARN(ret, "failed to leave OTP mode after lock\n");
>>>> +
>>>> + nand_deselect_target(nand);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>>>> +{
>>>> + static const char * const supported_otp_models[] = {
>>>> + "MX30LF1G18AC",
>>>> + "MX30LF2G18AC",
>>>> + "MX30LF4G18AC",
>>>> + };
>>>> + struct mtd_info *mtd;
>>>> +
>>>> + if (!chip->parameters.supports_set_get_features)
>>>> + return;
>>>> +
>>>> + if (match_string(supported_otp_models,
>>>> + ARRAY_SIZE(supported_otp_models),
>>>> + chip->parameters.model) < 0)
>>>> + return;
>
> I would start by checking the model, then it's list of supported ops.
>

Done in v4

>>>> +
>>>> + bitmap_set(chip->parameters.get_feature_list,
>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>> + bitmap_set(chip->parameters.set_feature_list,
>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>> +
>>>> + mtd = nand_to_mtd(chip);
>>>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>>>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>>>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>>>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>>>> +}
>>>> +
>>>> static int macronix_nand_init(struct nand_chip *chip)
>>>> {
>>>> if (nand_is_slc(chip))
>>>> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>>>> macronix_nand_onfi_init(chip);
>>>> macronix_nand_block_protection_support(chip);
>>>> macronix_nand_deep_power_down_support(chip);
>>>> + macronix_nand_setup_otp(chip);
>>>>
>>>> return 0;
>>>> }
>
>
> Thanks,
> Miquèl

Thanks, Arseniy

2023-05-24 06:40:47

by liao jaime

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC

Hi Miquel

>
> Hi Arseniy,
>
> [email protected] wrote on Mon, 15 May 2023 12:49:50 +0300:
>
> > Hello @Miquel!
> >
> > Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
> > someone who knows it well? I tested this patch on our devices (with already known Meson NAND
> > controller).
>
> + Jaime, who might test
>
> >
> > Thanks, Arseniy
> >
> > On 11.05.2023 21:21, Arseniy Krasnov wrote:
> > > Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
> > >
> > > On 11.05.2023 18:21, Arseniy Krasnov wrote:
> > >> This adds support for OTP area access on MX30LFxG18AC chip series.
> > >>
> > >> Changelog:
> > >> v1 -> v2:
> > >> * Add slab.h include due to kernel test robot error.
> > >> v2 -> v3:
> > >> * Use 'uint64_t' as input argument for 'do_div()' instead
> > >> of 'unsigned long' due to kernel test robot error.
> > >>
> > >> Signed-off-by: Arseniy Krasnov <[email protected]>
> > >> ---
> > >> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
> > >> 1 file changed, 213 insertions(+)
> > >>
> > >> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> > >> index 1472f925f386..2301f990678e 100644
> > >> --- a/drivers/mtd/nand/raw/nand_macronix.c
> > >> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > >> @@ -6,6 +6,7 @@
> > >> * Author: Boris Brezillon <[email protected]>
> > >> */
> > >>
> > >> +#include <linux/slab.h>
> > >> #include "linux/delay.h"
> > >> #include "internals.h"
> > >>
> > >> @@ -31,6 +32,20 @@
> > >>
> > >> #define MXIC_CMD_POWER_DOWN 0xB9
> > >>
> > >> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
> > >> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
> > >> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
> > >> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
> > >> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
> > >> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
> > >> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> > >> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
> > >> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
> > >> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
> > >> +
> > >> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
> > >> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
> > >> +
> > >> struct nand_onfi_vendor_macronix {
> > >> u8 reserved;
> > >> u8 reliability_func;
> > >> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
> > >> chip->ops.resume = mxic_nand_resume;
> > >> }
> > >>
> > >> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
> > >> + size_t *retlen,
> > >> + struct otp_info *buf)
> > >> +{
> > >> + if (len < sizeof(*buf))
> > >> + return -EINVAL;
> > >> +
> > >> + /* Don't know how to check that OTP is locked. */
>
> Jaime, any idea?

OTP info could be check by GET_FEATURE command.
GET_FEATURE command with address 90h could read sub-feature
parameter table, P1 is "3" means OTP protection.

Thanks
Jaime

>
> > >> + buf->locked = 0;
> > >> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
> > >> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
> > >> +
> > >> + *retlen = sizeof(*buf);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
> > >> +{
> > >> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > >> +
> > >> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
> > >> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > >> + feature_buf);
> > >> +}
> > >> +
> > >> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
> > >> +{
> > >> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > >> +
> > >> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > >> + feature_buf);
> > >> +}
> > >> +
> > >> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
> > >> + loff_t offs_in_flash,
> > >> + size_t len, size_t *retlen,
> > >> + u_char *buf, bool write)
> > >> +{
> > >> + struct nand_chip *nand;
> > >> + size_t bytes_handled;
> > >> + off_t offs_in_page;
> > >> + uint64_t page;
> > >> + void *dma_buf;
> > >> + int ret;
> > >> +
> > >> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
> > >> + * so allocate properly aligned memory for it. This is
> > >> + * needed because cross page accesses may lead to unaligned
> > >> + * buffer address for DMA.
> > >> + */
> > >> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
> > >> + if (!dma_buf)
> > >> + return -ENOMEM;
> > >> +
> > >> + nand = mtd_to_nand(mtd);
> > >> + nand_select_target(nand, 0);
> > >> +
> > >> + ret = macronix_30lfxg18ac_otp_enable(nand);
> > >> + if (ret)
> > >> + goto out_otp;
> > >> +
> > >> + page = offs_in_flash;
> > >> + /* 'page' will be result of division. */
> > >> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
> > >> + bytes_handled = 0;
> > >> +
> > >> + while (bytes_handled < len &&
> > >> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
> > >> + size_t bytes_to_handle;
> > >> +
> > >> + bytes_to_handle = min_t(size_t, len - bytes_handled,
> > >> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
> > >> + offs_in_page);
> > >> +
> > >> + if (write) {
> > >> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
> > >> + ret = nand_prog_page_op(nand, page, offs_in_page,
> > >> + dma_buf, bytes_to_handle);
> > >> + } else {
> > >> + ret = nand_read_page_op(nand, page, offs_in_page,
> > >> + dma_buf, bytes_to_handle);
> > >> + if (!ret)
> > >> + memcpy(&buf[bytes_handled], dma_buf,
> > >> + bytes_to_handle);
> > >> + }
> > >> + if (ret)
> > >> + goto out_otp;
> > >> +
> > >> + bytes_handled += bytes_to_handle;
> > >> + offs_in_page = 0;
> > >> + page++;
> > >> + }
> > >> +
> > >> + *retlen = bytes_handled;
> > >> +
> > >> +out_otp:
> > >> + if (ret)
> > >> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
> > >> +
> > >> + ret = macronix_30lfxg18ac_otp_disable(nand);
> > >> + WARN(ret, "failed to leave OTP mode after %s\n",
> > >> + write ? "write" : "read");
>
> Let's avoid WARN() calls (none in this driver are relevant IMHO). Just a
> dev_err() should be enough.
>
> > >> + nand_deselect_target(nand);
> > >> + kfree(dma_buf);
> > >> +
> > >> + return ret;
> > >> +}
> > >> +
> > >> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
> > >> + size_t len, size_t *rlen,
> > >> + const u_char *buf)
> > >> +{
> > >> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
> > >> + true);
> > >> +}
> > >> +
> > >> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
> > >> + size_t len, size_t *rlen,
> > >> + u_char *buf)
> > >> +{
> > >> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
> > >> +}
> > >> +
> > >> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
> > >> + size_t len)
> > >> +{
> > >> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> > >> + struct nand_chip *nand;
> > >> + int ret;
> > >> +
> > >> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
> > >> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
> > >> + return -EINVAL;
> > >> +
> > >> + dev_dbg(&mtd->dev, "locking OTP\n");
> > >> +
> > >> + nand = mtd_to_nand(mtd);
> > >> + nand_select_target(nand, 0);
> > >> +
> > >> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
> > >> + MACRONIX_30LFXG18AC_OTP_LOCKED;
> > >> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
> > >> + feature_buf);
> > >> + if (ret) {
> > >> + dev_err(&mtd->dev,
> > >> + "failed to lock OTP (set features): %i\n", ret);
> > >> + nand_deselect_target(nand);
> > >> + return ret;
> > >> + }
> > >> +
> > >> + /* Do dummy page prog with zero address. */
> > >> + feature_buf[0] = 0;
> > >> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
> > >> + if (ret)
> > >> + dev_err(&mtd->dev,
> > >> + "failed to lock OTP (page prog): %i\n", ret);
> > >> +
> > >> + ret = macronix_30lfxg18ac_otp_disable(nand);
> > >> + WARN(ret, "failed to leave OTP mode after lock\n");
> > >> +
> > >> + nand_deselect_target(nand);
> > >> +
> > >> + return ret;
> > >> +}
> > >> +
> > >> +static void macronix_nand_setup_otp(struct nand_chip *chip)
> > >> +{
> > >> + static const char * const supported_otp_models[] = {
> > >> + "MX30LF1G18AC",
> > >> + "MX30LF2G18AC",
> > >> + "MX30LF4G18AC",
> > >> + };
> > >> + struct mtd_info *mtd;
> > >> +
> > >> + if (!chip->parameters.supports_set_get_features)
> > >> + return;
> > >> +
> > >> + if (match_string(supported_otp_models,
> > >> + ARRAY_SIZE(supported_otp_models),
> > >> + chip->parameters.model) < 0)
> > >> + return;
>
> I would start by checking the model, then it's list of supported ops.
>
> > >> +
> > >> + bitmap_set(chip->parameters.get_feature_list,
> > >> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> > >> + bitmap_set(chip->parameters.set_feature_list,
> > >> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
> > >> +
> > >> + mtd = nand_to_mtd(chip);
> > >> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
> > >> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
> > >> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
> > >> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
> > >> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
> > >> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
> > >> +}
> > >> +
> > >> static int macronix_nand_init(struct nand_chip *chip)
> > >> {
> > >> if (nand_is_slc(chip))
> > >> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
> > >> macronix_nand_onfi_init(chip);
> > >> macronix_nand_block_protection_support(chip);
> > >> macronix_nand_deep_power_down_support(chip);
> > >> + macronix_nand_setup_otp(chip);
> > >>
> > >> return 0;
> > >> }
>
>
> Thanks,
> Miquèl

2023-05-24 07:28:23

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC



On 24.05.2023 09:33, liao jaime wrote:
> Hi Miquel
>
>>
>> Hi Arseniy,
>>
>> [email protected] wrote on Mon, 15 May 2023 12:49:50 +0300:
>>
>>> Hello @Miquel!
>>>
>>> Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
>>> someone who knows it well? I tested this patch on our devices (with already known Meson NAND
>>> controller).
>>
>> + Jaime, who might test
>>
>>>
>>> Thanks, Arseniy
>>>
>>> On 11.05.2023 21:21, Arseniy Krasnov wrote:
>>>> Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
>>>>
>>>> On 11.05.2023 18:21, Arseniy Krasnov wrote:
>>>>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>>>>
>>>>> Changelog:
>>>>> v1 -> v2:
>>>>> * Add slab.h include due to kernel test robot error.
>>>>> v2 -> v3:
>>>>> * Use 'uint64_t' as input argument for 'do_div()' instead
>>>>> of 'unsigned long' due to kernel test robot error.
>>>>>
>>>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>>>> ---
>>>>> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
>>>>> 1 file changed, 213 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>>>>> index 1472f925f386..2301f990678e 100644
>>>>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>>>>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>>>>> @@ -6,6 +6,7 @@
>>>>> * Author: Boris Brezillon <[email protected]>
>>>>> */
>>>>>
>>>>> +#include <linux/slab.h>
>>>>> #include "linux/delay.h"
>>>>> #include "internals.h"
>>>>>
>>>>> @@ -31,6 +32,20 @@
>>>>>
>>>>> #define MXIC_CMD_POWER_DOWN 0xB9
>>>>>
>>>>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>>>>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>>>>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>> +
>>>>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>>>>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>>>>> +
>>>>> struct nand_onfi_vendor_macronix {
>>>>> u8 reserved;
>>>>> u8 reliability_func;
>>>>> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>>>>> chip->ops.resume = mxic_nand_resume;
>>>>> }
>>>>>
>>>>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>>>>> + size_t *retlen,
>>>>> + struct otp_info *buf)
>>>>> +{
>>>>> + if (len < sizeof(*buf))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* Don't know how to check that OTP is locked. */
>>
>> Jaime, any idea?
>
> OTP info could be check by GET_FEATURE command.
> GET_FEATURE command with address 90h could read sub-feature
> parameter table, P1 is "3" means OTP protection.
>
> Thanks
> Jaime
>

Hello Jaime, thanks for quick reply!

I have two devices - with locked and unlocked OTP. I call the following thing for each of them:

@@ -339,6 +341,26 @@ static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
return -EINVAL;

/* Don't know how to check that OTP is locked. */
+ {
+ u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
+ struct nand_chip *nand;
+ int res;
+
+ nand = mtd_to_nand(mtd);
+
+ nand_select_target(nand, 0);
+
+ res = nand_get_features(nand, 0x90, feature_buf);
+
+ nand_deselect_target(nand);
+
+ printk(KERN_EMERG "\n\nRES %i OTP BUF %hhx %hhx %hhx %hhx\n\n\n",
+ res,
+ feature_buf[0],
+ feature_buf[1],
+ feature_buf[2],
+ feature_buf[3]);
+ }
buf->locked = 0;
buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;

And get for both devices:

[ 4.228721] RES 0 OTP BUF 0 0 0 0

May be i'm doing something wrong?

Thanks, Arseniy

>>
>>>>> + buf->locked = 0;
>>>>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>>>>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>>>> +
>>>>> + *retlen = sizeof(*buf);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>>>>> +{
>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>> +
>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>> + feature_buf);
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>>>>> +{
>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>> +
>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>> + feature_buf);
>>>>> +}
>>>>> +
>>>>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>>>>> + loff_t offs_in_flash,
>>>>> + size_t len, size_t *retlen,
>>>>> + u_char *buf, bool write)
>>>>> +{
>>>>> + struct nand_chip *nand;
>>>>> + size_t bytes_handled;
>>>>> + off_t offs_in_page;
>>>>> + uint64_t page;
>>>>> + void *dma_buf;
>>>>> + int ret;
>>>>> +
>>>>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>>>>> + * so allocate properly aligned memory for it. This is
>>>>> + * needed because cross page accesses may lead to unaligned
>>>>> + * buffer address for DMA.
>>>>> + */
>>>>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>>>>> + if (!dma_buf)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + nand = mtd_to_nand(mtd);
>>>>> + nand_select_target(nand, 0);
>>>>> +
>>>>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>>>>> + if (ret)
>>>>> + goto out_otp;
>>>>> +
>>>>> + page = offs_in_flash;
>>>>> + /* 'page' will be result of division. */
>>>>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>>>>> + bytes_handled = 0;
>>>>> +
>>>>> + while (bytes_handled < len &&
>>>>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>>>>> + size_t bytes_to_handle;
>>>>> +
>>>>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>>>>> + offs_in_page);
>>>>> +
>>>>> + if (write) {
>>>>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>>>>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>>>>> + dma_buf, bytes_to_handle);
>>>>> + } else {
>>>>> + ret = nand_read_page_op(nand, page, offs_in_page,
>>>>> + dma_buf, bytes_to_handle);
>>>>> + if (!ret)
>>>>> + memcpy(&buf[bytes_handled], dma_buf,
>>>>> + bytes_to_handle);
>>>>> + }
>>>>> + if (ret)
>>>>> + goto out_otp;
>>>>> +
>>>>> + bytes_handled += bytes_to_handle;
>>>>> + offs_in_page = 0;
>>>>> + page++;
>>>>> + }
>>>>> +
>>>>> + *retlen = bytes_handled;
>>>>> +
>>>>> +out_otp:
>>>>> + if (ret)
>>>>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>>>>> +
>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>> + WARN(ret, "failed to leave OTP mode after %s\n",
>>>>> + write ? "write" : "read");
>>
>> Let's avoid WARN() calls (none in this driver are relevant IMHO). Just a
>> dev_err() should be enough.
>>
>>>>> + nand_deselect_target(nand);
>>>>> + kfree(dma_buf);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>>>>> + size_t len, size_t *rlen,
>>>>> + const u_char *buf)
>>>>> +{
>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>>>>> + true);
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>>>>> + size_t len, size_t *rlen,
>>>>> + u_char *buf)
>>>>> +{
>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>>>>> +}
>>>>> +
>>>>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>>>>> + size_t len)
>>>>> +{
>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>> + struct nand_chip *nand;
>>>>> + int ret;
>>>>> +
>>>>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>>>>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + dev_dbg(&mtd->dev, "locking OTP\n");
>>>>> +
>>>>> + nand = mtd_to_nand(mtd);
>>>>> + nand_select_target(nand, 0);
>>>>> +
>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>>>>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>>>>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>> + feature_buf);
>>>>> + if (ret) {
>>>>> + dev_err(&mtd->dev,
>>>>> + "failed to lock OTP (set features): %i\n", ret);
>>>>> + nand_deselect_target(nand);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + /* Do dummy page prog with zero address. */
>>>>> + feature_buf[0] = 0;
>>>>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>>>>> + if (ret)
>>>>> + dev_err(&mtd->dev,
>>>>> + "failed to lock OTP (page prog): %i\n", ret);
>>>>> +
>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>> + WARN(ret, "failed to leave OTP mode after lock\n");
>>>>> +
>>>>> + nand_deselect_target(nand);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>>>>> +{
>>>>> + static const char * const supported_otp_models[] = {
>>>>> + "MX30LF1G18AC",
>>>>> + "MX30LF2G18AC",
>>>>> + "MX30LF4G18AC",
>>>>> + };
>>>>> + struct mtd_info *mtd;
>>>>> +
>>>>> + if (!chip->parameters.supports_set_get_features)
>>>>> + return;
>>>>> +
>>>>> + if (match_string(supported_otp_models,
>>>>> + ARRAY_SIZE(supported_otp_models),
>>>>> + chip->parameters.model) < 0)
>>>>> + return;
>>
>> I would start by checking the model, then it's list of supported ops.
>>
>>>>> +
>>>>> + bitmap_set(chip->parameters.get_feature_list,
>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>> + bitmap_set(chip->parameters.set_feature_list,
>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>> +
>>>>> + mtd = nand_to_mtd(chip);
>>>>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>>>>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>>>>> +}
>>>>> +
>>>>> static int macronix_nand_init(struct nand_chip *chip)
>>>>> {
>>>>> if (nand_is_slc(chip))
>>>>> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>>>>> macronix_nand_onfi_init(chip);
>>>>> macronix_nand_block_protection_support(chip);
>>>>> macronix_nand_deep_power_down_support(chip);
>>>>> + macronix_nand_setup_otp(chip);
>>>>>
>>>>> return 0;
>>>>> }
>>
>>
>> Thanks,
>> Miquèl

2023-05-25 07:23:30

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC



On 24.05.2023 10:13, Arseniy Krasnov wrote:
>
>
> On 24.05.2023 09:33, liao jaime wrote:
>> Hi Miquel
>>
>>>
>>> Hi Arseniy,
>>>
>>> [email protected] wrote on Mon, 15 May 2023 12:49:50 +0300:
>>>
>>>> Hello @Miquel!
>>>>
>>>> Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
>>>> someone who knows it well? I tested this patch on our devices (with already known Meson NAND
>>>> controller).
>>>
>>> + Jaime, who might test
>>>
>>>>
>>>> Thanks, Arseniy
>>>>
>>>> On 11.05.2023 21:21, Arseniy Krasnov wrote:
>>>>> Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
>>>>>
>>>>> On 11.05.2023 18:21, Arseniy Krasnov wrote:
>>>>>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>>>>>
>>>>>> Changelog:
>>>>>> v1 -> v2:
>>>>>> * Add slab.h include due to kernel test robot error.
>>>>>> v2 -> v3:
>>>>>> * Use 'uint64_t' as input argument for 'do_div()' instead
>>>>>> of 'unsigned long' due to kernel test robot error.
>>>>>>
>>>>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>>>>> ---
>>>>>> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
>>>>>> 1 file changed, 213 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>>>>>> index 1472f925f386..2301f990678e 100644
>>>>>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>>>>>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>>>>>> @@ -6,6 +6,7 @@
>>>>>> * Author: Boris Brezillon <[email protected]>
>>>>>> */
>>>>>>
>>>>>> +#include <linux/slab.h>
>>>>>> #include "linux/delay.h"
>>>>>> #include "internals.h"
>>>>>>
>>>>>> @@ -31,6 +32,20 @@
>>>>>>
>>>>>> #define MXIC_CMD_POWER_DOWN 0xB9
>>>>>>
>>>>>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>>>>>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>>>>>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>>> +
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>>>>>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>>>>>> +
>>>>>> struct nand_onfi_vendor_macronix {
>>>>>> u8 reserved;
>>>>>> u8 reliability_func;
>>>>>> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>>>>>> chip->ops.resume = mxic_nand_resume;
>>>>>> }
>>>>>>
>>>>>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>>>>>> + size_t *retlen,
>>>>>> + struct otp_info *buf)
>>>>>> +{
>>>>>> + if (len < sizeof(*buf))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + /* Don't know how to check that OTP is locked. */
>>>
>>> Jaime, any idea?
>>
>> OTP info could be check by GET_FEATURE command.
>> GET_FEATURE command with address 90h could read sub-feature
>> parameter table, P1 is "3" means OTP protection.
>>
>> Thanks
>> Jaime
>>
>
> Hello Jaime, thanks for quick reply!
>
> I have two devices - with locked and unlocked OTP. I call the following thing for each of them:
>
> @@ -339,6 +341,26 @@ static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
> return -EINVAL;
>
> /* Don't know how to check that OTP is locked. */
> + {
> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
> + struct nand_chip *nand;
> + int res;
> +
> + nand = mtd_to_nand(mtd);
> +
> + nand_select_target(nand, 0);
> +
> + res = nand_get_features(nand, 0x90, feature_buf);
> +
> + nand_deselect_target(nand);
> +
> + printk(KERN_EMERG "\n\nRES %i OTP BUF %hhx %hhx %hhx %hhx\n\n\n",
> + res,
> + feature_buf[0],
> + feature_buf[1],
> + feature_buf[2],
> + feature_buf[3]);
> + }
> buf->locked = 0;
> buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
> buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>
> And get for both devices:
>
> [ 4.228721] RES 0 OTP BUF 0 0 0 0
>
> May be i'm doing something wrong?
>
> Thanks, Arseniy

Small upd: I tried to

1) Call 'nand_get_features()' when OTP mode is enabled, it just returns 1 0 0 0 (e.g. OTP mode enabled, on both locked and unlocked devs).
2) Increase timings and delays in NAND controller driver - it didn't help.

Anyway, I see, that OTP status (enabled or disabled) is returned successfully, but locked status is always missed.

Thanks, Arseniy

>
>>>
>>>>>> + buf->locked = 0;
>>>>>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>>>>>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>>>>> +
>>>>>> + *retlen = sizeof(*buf);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>>>>>> +{
>>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>>> +
>>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>>> + feature_buf);
>>>>>> +}
>>>>>> +
>>>>>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>>>>>> +{
>>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>>> +
>>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>>> + feature_buf);
>>>>>> +}
>>>>>> +
>>>>>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>>>>>> + loff_t offs_in_flash,
>>>>>> + size_t len, size_t *retlen,
>>>>>> + u_char *buf, bool write)
>>>>>> +{
>>>>>> + struct nand_chip *nand;
>>>>>> + size_t bytes_handled;
>>>>>> + off_t offs_in_page;
>>>>>> + uint64_t page;
>>>>>> + void *dma_buf;
>>>>>> + int ret;
>>>>>> +
>>>>>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>>>>>> + * so allocate properly aligned memory for it. This is
>>>>>> + * needed because cross page accesses may lead to unaligned
>>>>>> + * buffer address for DMA.
>>>>>> + */
>>>>>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>>>>>> + if (!dma_buf)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + nand = mtd_to_nand(mtd);
>>>>>> + nand_select_target(nand, 0);
>>>>>> +
>>>>>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>>>>>> + if (ret)
>>>>>> + goto out_otp;
>>>>>> +
>>>>>> + page = offs_in_flash;
>>>>>> + /* 'page' will be result of division. */
>>>>>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>>>>>> + bytes_handled = 0;
>>>>>> +
>>>>>> + while (bytes_handled < len &&
>>>>>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>>>>>> + size_t bytes_to_handle;
>>>>>> +
>>>>>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>>>>>> + offs_in_page);
>>>>>> +
>>>>>> + if (write) {
>>>>>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>>>>>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>>>>>> + dma_buf, bytes_to_handle);
>>>>>> + } else {
>>>>>> + ret = nand_read_page_op(nand, page, offs_in_page,
>>>>>> + dma_buf, bytes_to_handle);
>>>>>> + if (!ret)
>>>>>> + memcpy(&buf[bytes_handled], dma_buf,
>>>>>> + bytes_to_handle);
>>>>>> + }
>>>>>> + if (ret)
>>>>>> + goto out_otp;
>>>>>> +
>>>>>> + bytes_handled += bytes_to_handle;
>>>>>> + offs_in_page = 0;
>>>>>> + page++;
>>>>>> + }
>>>>>> +
>>>>>> + *retlen = bytes_handled;
>>>>>> +
>>>>>> +out_otp:
>>>>>> + if (ret)
>>>>>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>>>>>> +
>>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>>> + WARN(ret, "failed to leave OTP mode after %s\n",
>>>>>> + write ? "write" : "read");
>>>
>>> Let's avoid WARN() calls (none in this driver are relevant IMHO). Just a
>>> dev_err() should be enough.
>>>
>>>>>> + nand_deselect_target(nand);
>>>>>> + kfree(dma_buf);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>>>>>> + size_t len, size_t *rlen,
>>>>>> + const u_char *buf)
>>>>>> +{
>>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>>>>>> + true);
>>>>>> +}
>>>>>> +
>>>>>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>>>>>> + size_t len, size_t *rlen,
>>>>>> + u_char *buf)
>>>>>> +{
>>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>>>>>> +}
>>>>>> +
>>>>>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>>>>>> + size_t len)
>>>>>> +{
>>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>>> + struct nand_chip *nand;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>>>>>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + dev_dbg(&mtd->dev, "locking OTP\n");
>>>>>> +
>>>>>> + nand = mtd_to_nand(mtd);
>>>>>> + nand_select_target(nand, 0);
>>>>>> +
>>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>>>>>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>>>>>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>>> + feature_buf);
>>>>>> + if (ret) {
>>>>>> + dev_err(&mtd->dev,
>>>>>> + "failed to lock OTP (set features): %i\n", ret);
>>>>>> + nand_deselect_target(nand);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + /* Do dummy page prog with zero address. */
>>>>>> + feature_buf[0] = 0;
>>>>>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>>>>>> + if (ret)
>>>>>> + dev_err(&mtd->dev,
>>>>>> + "failed to lock OTP (page prog): %i\n", ret);
>>>>>> +
>>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>>> + WARN(ret, "failed to leave OTP mode after lock\n");
>>>>>> +
>>>>>> + nand_deselect_target(nand);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>>>>>> +{
>>>>>> + static const char * const supported_otp_models[] = {
>>>>>> + "MX30LF1G18AC",
>>>>>> + "MX30LF2G18AC",
>>>>>> + "MX30LF4G18AC",
>>>>>> + };
>>>>>> + struct mtd_info *mtd;
>>>>>> +
>>>>>> + if (!chip->parameters.supports_set_get_features)
>>>>>> + return;
>>>>>> +
>>>>>> + if (match_string(supported_otp_models,
>>>>>> + ARRAY_SIZE(supported_otp_models),
>>>>>> + chip->parameters.model) < 0)
>>>>>> + return;
>>>
>>> I would start by checking the model, then it's list of supported ops.
>>>
>>>>>> +
>>>>>> + bitmap_set(chip->parameters.get_feature_list,
>>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>>> + bitmap_set(chip->parameters.set_feature_list,
>>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>>> +
>>>>>> + mtd = nand_to_mtd(chip);
>>>>>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>>>>>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>>>>>> +}
>>>>>> +
>>>>>> static int macronix_nand_init(struct nand_chip *chip)
>>>>>> {
>>>>>> if (nand_is_slc(chip))
>>>>>> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>>>>>> macronix_nand_onfi_init(chip);
>>>>>> macronix_nand_block_protection_support(chip);
>>>>>> macronix_nand_deep_power_down_support(chip);
>>>>>> + macronix_nand_setup_otp(chip);
>>>>>>
>>>>>> return 0;
>>>>>> }
>>>
>>>
>>> Thanks,
>>> Miquèl

2023-06-05 06:36:48

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RESEND PATCH v3] mtd: rawnand: macronix: OTP access for MX30LFxG18AC

Hello all guys!

Sorry, ping, FYI there is v4:
https://lore.kernel.org/linux-mtd/[email protected]/

Thanks, Arseniy

On 25.05.2023 10:02, Arseniy Krasnov wrote:
>
>
> On 24.05.2023 10:13, Arseniy Krasnov wrote:
>>
>>
>> On 24.05.2023 09:33, liao jaime wrote:
>>> Hi Miquel
>>>
>>>>
>>>> Hi Arseniy,
>>>>
>>>> [email protected] wrote on Mon, 15 May 2023 12:49:50 +0300:
>>>>
>>>>> Hello @Miquel!
>>>>>
>>>>> Sorry, but who could review this patch? :) IIUC this logic is very hw specific and we need
>>>>> someone who knows it well? I tested this patch on our devices (with already known Meson NAND
>>>>> controller).
>>>>
>>>> + Jaime, who might test
>>>>
>>>>>
>>>>> Thanks, Arseniy
>>>>>
>>>>> On 11.05.2023 21:21, Arseniy Krasnov wrote:
>>>>>> Cc: Mason Yang <[email protected]> and Boris Brezillon <[email protected]>
>>>>>>
>>>>>> On 11.05.2023 18:21, Arseniy Krasnov wrote:
>>>>>>> This adds support for OTP area access on MX30LFxG18AC chip series.
>>>>>>>
>>>>>>> Changelog:
>>>>>>> v1 -> v2:
>>>>>>> * Add slab.h include due to kernel test robot error.
>>>>>>> v2 -> v3:
>>>>>>> * Use 'uint64_t' as input argument for 'do_div()' instead
>>>>>>> of 'unsigned long' due to kernel test robot error.
>>>>>>>
>>>>>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>>>>>> ---
>>>>>>> drivers/mtd/nand/raw/nand_macronix.c | 213 +++++++++++++++++++++++++++
>>>>>>> 1 file changed, 213 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
>>>>>>> index 1472f925f386..2301f990678e 100644
>>>>>>> --- a/drivers/mtd/nand/raw/nand_macronix.c
>>>>>>> +++ b/drivers/mtd/nand/raw/nand_macronix.c
>>>>>>> @@ -6,6 +6,7 @@
>>>>>>> * Author: Boris Brezillon <[email protected]>
>>>>>>> */
>>>>>>>
>>>>>>> +#include <linux/slab.h>
>>>>>>> #include "linux/delay.h"
>>>>>>> #include "internals.h"
>>>>>>>
>>>>>>> @@ -31,6 +32,20 @@
>>>>>>>
>>>>>>> #define MXIC_CMD_POWER_DOWN 0xB9
>>>>>>>
>>>>>>> +#define ONFI_FEATURE_ADDR_30LFXG18AC_OTP 0x90
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_PAGE 0
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGES 30
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_PAGE_SIZE 2112
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_START_BYTE \
>>>>>>> + (MACRONIX_30LFXG18AC_OTP_START_PAGE * \
>>>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_SIZE_BYTES \
>>>>>>> + (MACRONIX_30LFXG18AC_OTP_PAGES * \
>>>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE)
>>>>>>> +
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_EN BIT(0)
>>>>>>> +#define MACRONIX_30LFXG18AC_OTP_LOCKED BIT(1)
>>>>>>> +
>>>>>>> struct nand_onfi_vendor_macronix {
>>>>>>> u8 reserved;
>>>>>>> u8 reliability_func;
>>>>>>> @@ -316,6 +331,203 @@ static void macronix_nand_deep_power_down_support(struct nand_chip *chip)
>>>>>>> chip->ops.resume = mxic_nand_resume;
>>>>>>> }
>>>>>>>
>>>>>>> +static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>>>>>>> + size_t *retlen,
>>>>>>> + struct otp_info *buf)
>>>>>>> +{
>>>>>>> + if (len < sizeof(*buf))
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + /* Don't know how to check that OTP is locked. */
>>>>
>>>> Jaime, any idea?
>>>
>>> OTP info could be check by GET_FEATURE command.
>>> GET_FEATURE command with address 90h could read sub-feature
>>> parameter table, P1 is "3" means OTP protection.
>>>
>>> Thanks
>>> Jaime
>>>
>>
>> Hello Jaime, thanks for quick reply!
>>
>> I have two devices - with locked and unlocked OTP. I call the following thing for each of them:
>>
>> @@ -339,6 +341,26 @@ static int macronix_30lfxg18ac_get_otp_info(struct mtd_info *mtd, size_t len,
>> return -EINVAL;
>>
>> /* Don't know how to check that OTP is locked. */
>> + {
>> + u8 feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>> + struct nand_chip *nand;
>> + int res;
>> +
>> + nand = mtd_to_nand(mtd);
>> +
>> + nand_select_target(nand, 0);
>> +
>> + res = nand_get_features(nand, 0x90, feature_buf);
>> +
>> + nand_deselect_target(nand);
>> +
>> + printk(KERN_EMERG "\n\nRES %i OTP BUF %hhx %hhx %hhx %hhx\n\n\n",
>> + res,
>> + feature_buf[0],
>> + feature_buf[1],
>> + feature_buf[2],
>> + feature_buf[3]);
>> + }
>> buf->locked = 0;
>> buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>> buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>
>> And get for both devices:
>>
>> [ 4.228721] RES 0 OTP BUF 0 0 0 0
>>
>> May be i'm doing something wrong?
>>
>> Thanks, Arseniy
>
> Small upd: I tried to
>
> 1) Call 'nand_get_features()' when OTP mode is enabled, it just returns 1 0 0 0 (e.g. OTP mode enabled, on both locked and unlocked devs).
> 2) Increase timings and delays in NAND controller driver - it didn't help.
>
> Anyway, I see, that OTP status (enabled or disabled) is returned successfully, but locked status is always missed.
>
> Thanks, Arseniy
>
>>
>>>>
>>>>>>> + buf->locked = 0;
>>>>>>> + buf->start = MACRONIX_30LFXG18AC_OTP_START_BYTE;
>>>>>>> + buf->length = MACRONIX_30LFXG18AC_OTP_SIZE_BYTES;
>>>>>>> +
>>>>>>> + *retlen = sizeof(*buf);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int macronix_30lfxg18ac_otp_enable(struct nand_chip *nand)
>>>>>>> +{
>>>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>>>> +
>>>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN;
>>>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>>>> + feature_buf);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int macronix_30lfxg18ac_otp_disable(struct nand_chip *nand)
>>>>>>> +{
>>>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>>>> +
>>>>>>> + return nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>>>> + feature_buf);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int __macronix_30lfxg18ac_rw_otp(struct mtd_info *mtd,
>>>>>>> + loff_t offs_in_flash,
>>>>>>> + size_t len, size_t *retlen,
>>>>>>> + u_char *buf, bool write)
>>>>>>> +{
>>>>>>> + struct nand_chip *nand;
>>>>>>> + size_t bytes_handled;
>>>>>>> + off_t offs_in_page;
>>>>>>> + uint64_t page;
>>>>>>> + void *dma_buf;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + /* 'nand_prog/read_page_op()' may use 'buf' as DMA buffer,
>>>>>>> + * so allocate properly aligned memory for it. This is
>>>>>>> + * needed because cross page accesses may lead to unaligned
>>>>>>> + * buffer address for DMA.
>>>>>>> + */
>>>>>>> + dma_buf = kmalloc(MACRONIX_30LFXG18AC_OTP_PAGE_SIZE, GFP_KERNEL);
>>>>>>> + if (!dma_buf)
>>>>>>> + return -ENOMEM;
>>>>>>> +
>>>>>>> + nand = mtd_to_nand(mtd);
>>>>>>> + nand_select_target(nand, 0);
>>>>>>> +
>>>>>>> + ret = macronix_30lfxg18ac_otp_enable(nand);
>>>>>>> + if (ret)
>>>>>>> + goto out_otp;
>>>>>>> +
>>>>>>> + page = offs_in_flash;
>>>>>>> + /* 'page' will be result of division. */
>>>>>>> + offs_in_page = do_div(page, MACRONIX_30LFXG18AC_OTP_PAGE_SIZE);
>>>>>>> + bytes_handled = 0;
>>>>>>> +
>>>>>>> + while (bytes_handled < len &&
>>>>>>> + page < MACRONIX_30LFXG18AC_OTP_PAGES) {
>>>>>>> + size_t bytes_to_handle;
>>>>>>> +
>>>>>>> + bytes_to_handle = min_t(size_t, len - bytes_handled,
>>>>>>> + MACRONIX_30LFXG18AC_OTP_PAGE_SIZE -
>>>>>>> + offs_in_page);
>>>>>>> +
>>>>>>> + if (write) {
>>>>>>> + memcpy(dma_buf, &buf[bytes_handled], bytes_to_handle);
>>>>>>> + ret = nand_prog_page_op(nand, page, offs_in_page,
>>>>>>> + dma_buf, bytes_to_handle);
>>>>>>> + } else {
>>>>>>> + ret = nand_read_page_op(nand, page, offs_in_page,
>>>>>>> + dma_buf, bytes_to_handle);
>>>>>>> + if (!ret)
>>>>>>> + memcpy(&buf[bytes_handled], dma_buf,
>>>>>>> + bytes_to_handle);
>>>>>>> + }
>>>>>>> + if (ret)
>>>>>>> + goto out_otp;
>>>>>>> +
>>>>>>> + bytes_handled += bytes_to_handle;
>>>>>>> + offs_in_page = 0;
>>>>>>> + page++;
>>>>>>> + }
>>>>>>> +
>>>>>>> + *retlen = bytes_handled;
>>>>>>> +
>>>>>>> +out_otp:
>>>>>>> + if (ret)
>>>>>>> + dev_err(&mtd->dev, "failed to perform OTP IO: %i\n", ret);
>>>>>>> +
>>>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>>>> + WARN(ret, "failed to leave OTP mode after %s\n",
>>>>>>> + write ? "write" : "read");
>>>>
>>>> Let's avoid WARN() calls (none in this driver are relevant IMHO). Just a
>>>> dev_err() should be enough.
>>>>
>>>>>>> + nand_deselect_target(nand);
>>>>>>> + kfree(dma_buf);
>>>>>>> +
>>>>>>> + return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int macronix_30lfxg18ac_write_otp(struct mtd_info *mtd, loff_t to,
>>>>>>> + size_t len, size_t *rlen,
>>>>>>> + const u_char *buf)
>>>>>>> +{
>>>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, to, len, rlen, (u_char *)buf,
>>>>>>> + true);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int macronix_30lfxg18ac_read_otp(struct mtd_info *mtd, loff_t from,
>>>>>>> + size_t len, size_t *rlen,
>>>>>>> + u_char *buf)
>>>>>>> +{
>>>>>>> + return __macronix_30lfxg18ac_rw_otp(mtd, from, len, rlen, buf, false);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int macronix_30lfxg18ac_lock_otp(struct mtd_info *mtd, loff_t from,
>>>>>>> + size_t len)
>>>>>>> +{
>>>>>>> + uint8_t feature_buf[ONFI_SUBFEATURE_PARAM_LEN] = { 0 };
>>>>>>> + struct nand_chip *nand;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + if (from != MACRONIX_30LFXG18AC_OTP_START_BYTE ||
>>>>>>> + len != MACRONIX_30LFXG18AC_OTP_SIZE_BYTES)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + dev_dbg(&mtd->dev, "locking OTP\n");
>>>>>>> +
>>>>>>> + nand = mtd_to_nand(mtd);
>>>>>>> + nand_select_target(nand, 0);
>>>>>>> +
>>>>>>> + feature_buf[0] = MACRONIX_30LFXG18AC_OTP_EN |
>>>>>>> + MACRONIX_30LFXG18AC_OTP_LOCKED;
>>>>>>> + ret = nand_set_features(nand, ONFI_FEATURE_ADDR_30LFXG18AC_OTP,
>>>>>>> + feature_buf);
>>>>>>> + if (ret) {
>>>>>>> + dev_err(&mtd->dev,
>>>>>>> + "failed to lock OTP (set features): %i\n", ret);
>>>>>>> + nand_deselect_target(nand);
>>>>>>> + return ret;
>>>>>>> + }
>>>>>>> +
>>>>>>> + /* Do dummy page prog with zero address. */
>>>>>>> + feature_buf[0] = 0;
>>>>>>> + ret = nand_prog_page_op(nand, 0, 0, feature_buf, 1);
>>>>>>> + if (ret)
>>>>>>> + dev_err(&mtd->dev,
>>>>>>> + "failed to lock OTP (page prog): %i\n", ret);
>>>>>>> +
>>>>>>> + ret = macronix_30lfxg18ac_otp_disable(nand);
>>>>>>> + WARN(ret, "failed to leave OTP mode after lock\n");
>>>>>>> +
>>>>>>> + nand_deselect_target(nand);
>>>>>>> +
>>>>>>> + return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void macronix_nand_setup_otp(struct nand_chip *chip)
>>>>>>> +{
>>>>>>> + static const char * const supported_otp_models[] = {
>>>>>>> + "MX30LF1G18AC",
>>>>>>> + "MX30LF2G18AC",
>>>>>>> + "MX30LF4G18AC",
>>>>>>> + };
>>>>>>> + struct mtd_info *mtd;
>>>>>>> +
>>>>>>> + if (!chip->parameters.supports_set_get_features)
>>>>>>> + return;
>>>>>>> +
>>>>>>> + if (match_string(supported_otp_models,
>>>>>>> + ARRAY_SIZE(supported_otp_models),
>>>>>>> + chip->parameters.model) < 0)
>>>>>>> + return;
>>>>
>>>> I would start by checking the model, then it's list of supported ops.
>>>>
>>>>>>> +
>>>>>>> + bitmap_set(chip->parameters.get_feature_list,
>>>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>>>> + bitmap_set(chip->parameters.set_feature_list,
>>>>>>> + ONFI_FEATURE_ADDR_30LFXG18AC_OTP, 1);
>>>>>>> +
>>>>>>> + mtd = nand_to_mtd(chip);
>>>>>>> + mtd->_get_fact_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>>>> + mtd->_read_fact_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>>>> + mtd->_get_user_prot_info = macronix_30lfxg18ac_get_otp_info;
>>>>>>> + mtd->_read_user_prot_reg = macronix_30lfxg18ac_read_otp;
>>>>>>> + mtd->_write_user_prot_reg = macronix_30lfxg18ac_write_otp;
>>>>>>> + mtd->_lock_user_prot_reg = macronix_30lfxg18ac_lock_otp;
>>>>>>> +}
>>>>>>> +
>>>>>>> static int macronix_nand_init(struct nand_chip *chip)
>>>>>>> {
>>>>>>> if (nand_is_slc(chip))
>>>>>>> @@ -325,6 +537,7 @@ static int macronix_nand_init(struct nand_chip *chip)
>>>>>>> macronix_nand_onfi_init(chip);
>>>>>>> macronix_nand_block_protection_support(chip);
>>>>>>> macronix_nand_deep_power_down_support(chip);
>>>>>>> + macronix_nand_setup_otp(chip);
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>
>>>>
>>>> Thanks,
>>>> Miquèl