From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ECD6BC56201 for ; Wed, 25 Nov 2020 18:53:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2C9C620674 for ; Wed, 25 Nov 2020 18:53:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="nVguUHHR"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=walle.cc header.i=@walle.cc header.b="jvW56JVa" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2C9C620674 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=walle.cc Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=makQmqOTenjaLtbe/4U0YNaOFW3PZtKWWZsQ8ik/lbc=; b=nVguUHHRqexRJ94/JW/9htvDE MdfFtjjJKbY7eSTWzuushaA8ebeKIwdiXpvaXzEiy0Bc4Ngl4dvYBCUfm4NSdbeNdXoTsAMg81EpU Ldh9wPKlPwSquRAqf1I/3CBahou1t86Hox2mx9nr45rR2hw3BYULnJx8xbSgbQLQBXIJE6O0xUDtM 1s97ygwAR7VxFlYzIieyH+V23U0MZciQ+gOKMtVgBmCGYqFXYmBfdEfP3KQuLwyfCm6EIFfzHHwau ypTqOX0dDRJ+i72991GfmmJJNAzOqnE13TrCIoEMbdx9l+xzgV8CpNS4UrbmGE342amwf3BAJL5ll rzR7jRMlQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1khzuA-0005NQ-4h; Wed, 25 Nov 2020 18:52:38 +0000 Received: from ssl.serverraum.org ([176.9.125.105]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khzu7-0005Mw-98 for linux-mtd@lists.infradead.org; Wed, 25 Nov 2020 18:52:36 +0000 Received: from ssl.serverraum.org (web.serverraum.org [172.16.0.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ssl.serverraum.org (Postfix) with ESMTPSA id 92A4A22FEB; Wed, 25 Nov 2020 19:52:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=walle.cc; s=mail2016061301; t=1606330352; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5b56CBLECMvLLKiJplwMaGwXiawOOTRdGRqJJQKr60I=; b=jvW56JVaOaX6AwYedmmrRJoWAYryzNh2J+8S2X67C/ELepCS/uQIn29w0kdulebLeY7Ns0 2CgSVxsaKLaecMat8AipHEJBhQmiWBd/jUQI5rmOZngk9PzFUzvzlHCU5l9TAlepTVLf5L eYALLM6Xxprz7sCwLA60fSQfXL/xw9k= MIME-Version: 1.0 Date: Wed, 25 Nov 2020 19:52:32 +0100 From: Michael Walle To: Tudor.Ambarus@microchip.com Subject: Re: [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile In-Reply-To: References: <20201003153235.29762-1-michael@walle.cc> <20201003153235.29762-4-michael@walle.cc> User-Agent: Roundcube Webmail/1.4.9 Message-ID: X-Sender: michael@walle.cc X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201125_135235_565890_6B95B4E2 X-CRM114-Status: GOOD ( 31.47 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: vigneshr@ti.com, richard@nod.at, linux-kernel@vger.kernel.org, boris.brezillon@collabora.com, linux-mtd@lists.infradead.org, miquel.raynal@bootlin.com Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org Am 2020-11-25 13:21, schrieb Tudor.Ambarus@microchip.com: [..] >> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c >> index 49d392c6c8bc..19665095aeab 100644 >> --- a/drivers/mtd/spi-nor/atmel.c >> +++ b/drivers/mtd/spi-nor/atmel.c >> @@ -8,23 +8,120 @@ >> >> #include "core.h" >> >> +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2) >> + >> +/** >> + * atmel_set_global_protection - Do a Global Protect or Unprotect >> command >> + * @nor: pointer to 'struct spi_nor' >> + * @ofs: offset in bytes >> + * @len: len in bytes >> + * @is_protect: if true do a Global Protect otherwise it is a >> Global Unprotect >> + * >> + * Return: 0 on success, -error otherwise. >> + */ >> +static int atmel_set_global_protection(struct spi_nor *nor, loff_t >> ofs, >> + uint64_t len, bool is_protect) >> +{ >> + int ret; >> + u8 sr; >> + >> + /* We only support locking the whole flash array */ >> + if (ofs || len != nor->params->size) >> + return -EINVAL; >> + >> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >> + if (ret) >> + return ret; >> + sr = nor->bouncebuf[0]; >> + >> + /* SRWD bit needs to be cleared, otherwise the protection >> doesn't change */ >> + if (sr & SR_SRWD) { >> + sr &= ~SR_SRWD; >> + ret = spi_nor_write_sr_and_check(nor, sr); >> + if (ret) { >> + dev_err(nor->dev, "unable to clear SRWD bit, >> WP# asserted?\n"); >> + return ret; >> + } >> + } >> + >> + if (is_protect) >> + sr |= ATMEL_SR_GLOBAL_PROTECT_MASK; >> + else >> + sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK; >> + >> + return spi_nor_write_sr_and_check(nor, sr); >> +} >> + >> +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + return atmel_set_global_protection(nor, ofs, len, true); >> +} >> + >> +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + return atmel_set_global_protection(nor, ofs, len, false); >> +} >> + >> +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + int ret; >> + >> + if (ofs >= nor->params->size || (ofs + len) > >> nor->params->size) >> + return -EINVAL; >> + >> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >> + if (ret) >> + return ret; >> + >> + return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == >> ATMEL_SR_GLOBAL_PROTECT_MASK); >> +} >> + >> +static const struct spi_nor_locking_ops atmel_global_protection_ops = >> { >> + .lock = atmel_global_protect, >> + .unlock = atmel_global_unprotect, >> + .is_locked = atmel_is_global_protected, >> +}; > > Skimming through my notes in 1/3, I'm guessing this will not work for > any > of the atmel flashes that we currently have. > > Can we instead return -EOPNOTSUPP for .is_locked and .lock and just > write a 0x00 to SR > for the .unlock method? As mentioned in my previous reply to 1/3 I don't think it is enough to just write 0 because you first have to disable the SPRL. For the other ops, for what atmel flash do you think this won't work? All the flashes which uses these ops were marked as "Global Protect/Unprotect same as in at25df041a." by you. So I think it should work on all of these. Or am I missing something here? [..] >> diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c >> index c93170008118..c2ebf29d95f2 100644 >> --- a/drivers/mtd/spi-nor/esmt.c >> +++ b/drivers/mtd/spi-nor/esmt.c >> @@ -11,9 +11,13 @@ >> static const struct flash_info esmt_parts[] = { >> /* ESMT */ >> { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, >> - SECT_4K | SPI_NOR_HAS_LOCK) }, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, > > https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32PA.pdf > BP GENMASK(4,2), volatile, ok > >> { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64, >> - SECT_4K | SPI_NOR_HAS_LOCK) }, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, > > https://datasheetspdf.com/pdf-file/796196/ESMT/F25L32QA/1 > Datasheet states that "BP0~3, QE and BPL bits are non-volatile." > At the same time, it says: "After power-up, BP3, BP2, BP1 and BP0 bits > are set to 0." Mhh I had this datasheet: https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32QA.pdf In that one they are volatile.. but yours is a newer version. So I guess the flashes with the PA suffix have volatile BP and the QA ones have the non-volatile version. > Maybe factory default setting for BPn is 0? Let's treat them as NV, as > in > f25l64qa. Yes will fix it. > Do we need BP3? Rather the top bottom bit. But that is outside of the scope of this patch. And as per your rule, as I don't have this particular flash I cannot test and thus couldn't add the TB bit (technically). But if you like I can do another patch (outside of this series and after it is applied) which will add the TB bit flag. > >> + /* >> + * According to the datasheet the BPn bits are non-volatile, >> whereas >> + * they are volatile for the smaller f25l32qa. >> + */ >> { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128, >> SECT_4K | SPI_NOR_HAS_LOCK) }, > > https://datasheetspdf.com/pdf-file/967488/EliteSemiconductor/F25L64QA/1 > BP GENMASK(5, 2), non-volatile. > > BP3? Same as F25L32QA. > > >> }; >> diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c >> index d8196f101368..c45ea0ad01f0 100644 >> --- a/drivers/mtd/spi-nor/intel.c >> +++ b/drivers/mtd/spi-nor/intel.c >> @@ -10,9 +10,9 @@ >> >> static const struct flash_info intel_parts[] = { >> /* Intel/Numonyx -- xxxs33b */ >> - { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, 0) }, >> - { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, 0) }, >> - { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, 0) }, >> + { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, >> SPI_NOR_WP_IS_VOLATILE) }, >> + { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, >> SPI_NOR_WP_IS_VOLATILE) }, >> + { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, >> SPI_NOR_WP_IS_VOLATILE) }, > > http://www.datasheet.hk/view_download.php?id=1561787&file=0282\qh25f016s33b8_649107.pdf > BP GENMASK(4,2) all volatile, all set to 1 at power-up. > > You can add SPI_NOR_HAS_LOCK to each flash, and get rid of the > manufacturer > generalization. ok > >> }; >> >> static void intel_default_init(struct spi_nor *nor) >> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c >> index 8b169fa4102a..5e4450877d66 100644 >> --- a/drivers/mtd/spi-nor/sst.c >> +++ b/drivers/mtd/spi-nor/sst.c >> @@ -11,26 +11,27 @@ >> static const struct flash_info sst_parts[] = { >> /* SST -- large erase sizes are "overlays", "sectors" are 4K >> */ >> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> - { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | >> SPI_NOR_HAS_LOCK) }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> + { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, > > Looks like BP3 is needed here. https://ww1.microchip.com/downloads/en/DeviceDoc/20005036C.pdf agreed. But again cannot test it. Would add it as a seperate patch to this series. (or leave it like it is) > >> { "sst25wf512", INFO(0xbf2501, 0, 64 * 1024, 1, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> { "sst25wf010", INFO(0xbf2502, 0, 64 * 1024, 2, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> { "sst25wf020", INFO(0xbf2503, 0, 64 * 1024, 4, >> - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) >> }, >> + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) }, >> { "sst25wf020a", INFO(0x621612, 0, 64 * 1024, 4, SECT_4K | >> SPI_NOR_HAS_LOCK) }, >> { "sst25wf040b", INFO(0x621613, 0, 64 * 1024, 8, SECT_4K | >> SPI_NOR_HAS_LOCK) }, > > These two flashes have just two BP bits located at bit 2 and 3. > Probably will work. Mhh? What datasheet were you looking at? There are three BPs: https://ww1.microchip.com/downloads/en/DeviceDoc/SST25WF040B-4-Mbit-1.8V-SPI-Serial-Flash-Data-Sheet-DS20005193E.pdf Ahh here are the tables which only inidicate two. But there are three. https://ww1.microchip.com/downloads/en/DeviceDoc/20005016C.pdf And yes since the rework of the BP bits algorithm this should work as expected. Its just because the flash is too small to actually fill up all the BP bits. -michael ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/