Hello all guys!
Sorry, ping, FYI there is v4: https://lore.kernel.org/linux-mtd/20230523101637.3009746-1-AVKrasnov@sberdev...
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,
avkrasnov@sberdevices.ru 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 masonccyang@mxic.com.tw and Boris Brezillon boris.brezillon@collabora.com
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 AVKrasnov@sberdevices.ru > --- > 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 boris.brezillon@free-electrons.com > */ > > +#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
- 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).
- 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