| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <571484c7-0cf4-6a7d-6d7f-375cfb13ce8b@gmail.com>
Date: Tue, 25 Jun 2019 01:46:13 +0900
From: Tokunori Ikegami <ikegami.t@...il.com>
To: Vignesh Raghavendra <vigneshr@...com>,
Boris Brezillon <bbrezillon@...nel.org>,
Marek Vasut <marek.vasut@...il.com>,
Richard Weinberger <richard@....at>,
Rob Herring <robh+dt@...nel.org>
Cc: devicetree@...r.kernel.org,
Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
Miquel Raynal <miquel.raynal@...tlin.com>,
Mason Yang <masonccyang@...c.com.tw>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v7 1/5] mtd: cfi_cmdset_0002: Add support for polling
status register
On 2019/06/21 2:22, Vignesh Raghavendra wrote:
> HyperFlash devices are compliant with CFI AMD/Fujitsu Extended Command
> Set (0x0002) for flash operations, therefore
> drivers/mtd/chips/cfi_cmdset_0002.c can be used as is. But these devices
> do not support DQ polling method of determining chip ready/good status.
> These flashes provide Status Register whose bits can be polled to know
> status of flash operation.
>
> Cypress HyperFlash datasheet here[1], talks about CFI Amd/Fujitsu
> Extended Query version 1.5. Bit 0 of "Software Features supported" field
> of CFI Primary Vendor-Specific Extended Query table indicates
> presence/absence of status register and Bit 1 indicates whether or not
> DQ polling is supported. Using these bits, its possible to determine
> whether flash supports DQ polling or need to use Status Register.
>
> Add support for polling Status Register to know device ready/status of
> erase/write operations when DQ polling is not supported.
> Print error messages on erase/program failure by looking at related
> Status Register bits.
>
> [1] https://www.cypress.com/file/213346/download
>
> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
> ---
> v7: No change
>
> drivers/mtd/chips/cfi_cmdset_0002.c | 90 +++++++++++++++++++++++++++++
> include/linux/mtd/cfi.h | 5 ++
> 2 files changed, 95 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index c8fa5906bdf9..0f571f162e3b 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -49,6 +49,16 @@
> #define SST49LF008A 0x005a
> #define AT49BV6416 0x00d6
>
> +/*
> + * Status Register bit description. Used by flash devices that don't
> + * support DQ polling (e.g. HyperFlash)
> + */
> +#define CFI_SR_DRB BIT(7)
> +#define CFI_SR_ESB BIT(5)
> +#define CFI_SR_PSB BIT(4)
> +#define CFI_SR_WBASB BIT(3)
> +#define CFI_SR_SLSB BIT(1)
> +
> static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
> static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
> static int cfi_amdstd_write_buffers(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
> @@ -97,6 +107,48 @@ static struct mtd_chip_driver cfi_amdstd_chipdrv = {
> .module = THIS_MODULE
> };
>
> +/*
> + * Use status register to poll for Erase/write completion when DQ is not
> + * supported. This is indicated by Bit[1:0] of SoftwareFeatures field in
> + * CFI Primary Vendor-Specific Extended Query table 1.5
> + */
> +static int cfi_use_status_reg(struct cfi_private *cfi)
> +{
> + struct cfi_pri_amdstd *extp = cfi->cmdset_priv;
> +
> + return extp->MinorVersion >= '5' &&
> + (extp->SoftwareFeatures & 0x3) == 0x1;
Seems to be better to use defined values instead of 0x3 and 0x1 hard
coded values.
> +}
> +
> +static void cfi_check_err_status(struct map_info *map, unsigned long adr)
> +{
> + struct cfi_private *cfi = map->fldrv_priv;
> + map_word status;
> +
> + if (!cfi_use_status_reg(cfi))
> + return;
> +
> + cfi_send_gen_cmd(0x70, cfi->addr_unlock1, 0, map, cfi,
Is it not necessary to set chip->start as the base parameter for
cfi_send_gen_cmd()?
> + cfi->device_type, NULL);
> + status = map_read(map, adr);
> +
> + if (map_word_bitsset(map, status, CMD(0x3a))) {
> + unsigned long chipstatus = MERGESTATUS(status);
> +
> + if (chipstatus & CFI_SR_ESB)
> + pr_err("%s erase operation failed, status %lx\n",
> + map->name, chipstatus);
> + if (chipstatus & CFI_SR_PSB)
> + pr_err("%s program operation failed, status %lx\n",
> + map->name, chipstatus);
> + if (chipstatus & CFI_SR_WBASB)
> + pr_err("%s buffer program command aborted, status %lx\n",
> + map->name, chipstatus);
> + if (chipstatus & CFI_SR_SLSB)
> + pr_err("%s sector write protected, status %lx\n",
> + map->name, chipstatus);
> + }
> +}
>
> /* #define DEBUG_CFI_FEATURES */
>
> @@ -744,8 +796,22 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
> */
> static int __xipram chip_ready(struct map_info *map, unsigned long addr)
> {
> + struct cfi_private *cfi = map->fldrv_priv;
> map_word d, t;
>
> + if (cfi_use_status_reg(cfi)) {
> + map_word ready = CMD(CFI_SR_DRB);
> + /*
> + * For chips that support status register, check device
> + * ready bit
> + */
> + cfi_send_gen_cmd(0x70, cfi->addr_unlock1, 0, map, cfi,
Same comment as cfi_check_err_status() about the base address.
> + cfi->device_type, NULL);
> + d = map_read(map, addr);
> +
> + return map_word_andequal(map, d, ready, ready);
> + }
> +
> d = map_read(map, addr);
> t = map_read(map, addr);
>
> @@ -769,8 +835,27 @@ static int __xipram chip_ready(struct map_info *map, unsigned long addr)
> */
> static int __xipram chip_good(struct map_info *map, unsigned long addr, map_word expected)
> {
> + struct cfi_private *cfi = map->fldrv_priv;
> map_word oldd, curd;
>
> + if (cfi_use_status_reg(cfi)) {
> + map_word ready = CMD(CFI_SR_DRB);
> + map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB);
Is it not necessary to check CFI_SR_WBASB and CFI_SR_SLSB that are
checked by cfi_check_err_status()?
> + /*
> + * For chips that support status register, check device
> + * ready bit and Erase/Program status bit to know if
> + * operation succeeded.
> + */
> + cfi_send_gen_cmd(0x70, cfi->addr_unlock1, 0, map, cfi,
Same as cfi_check_err_status() and chip_ready() about the base address.
> + cfi->device_type, NULL);
> + curd = map_read(map, addr);
> +
> + if (map_word_andequal(map, curd, ready, ready))
> + return !map_word_bitsset(map, curd, err);
> +
> + return 0;
> + }
> +
> oldd = map_read(map, addr);
> curd = map_read(map, addr);
>
> @@ -1644,6 +1729,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
> /* Did we succeed? */
> if (!chip_good(map, adr, datum)) {
> /* reset on all failures. */
> + cfi_check_err_status(map, adr);
> map_write(map, CMD(0xF0), chip->start);
> /* FIXME - should have reset delay before continuing */
>
> @@ -1901,6 +1987,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
> * See e.g.
> * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
> */
> + cfi_check_err_status(map, adr);
> cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
> cfi->device_type, NULL);
> cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
> @@ -2107,6 +2194,7 @@ static int do_panic_write_oneword(struct map_info *map, struct flchip *chip,
>
> if (!chip_good(map, adr, datum)) {
> /* reset on all failures. */
> + cfi_check_err_status(map, adr);
> map_write(map, CMD(0xF0), chip->start);
> /* FIXME - should have reset delay before continuing */
>
> @@ -2316,6 +2404,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
> /* Did we succeed? */
> if (ret) {
> /* reset on all failures. */
> + cfi_check_err_status(map, adr);
> map_write(map, CMD(0xF0), chip->start);
> /* FIXME - should have reset delay before continuing */
>
> @@ -2412,6 +2501,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
> /* Did we succeed? */
> if (ret) {
> /* reset on all failures. */
> + cfi_check_err_status(map, adr);
> map_write(map, CMD(0xF0), chip->start);
> /* FIXME - should have reset delay before continuing */
>
> diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
> index 208c87cf2e3e..b50416169049 100644
> --- a/include/linux/mtd/cfi.h
> +++ b/include/linux/mtd/cfi.h
> @@ -219,6 +219,11 @@ struct cfi_pri_amdstd {
> uint8_t VppMin;
> uint8_t VppMax;
> uint8_t TopBottom;
> + /* Below field are added from version 1.5 */
> + uint8_t ProgramSuspend;
> + uint8_t UnlockBypass;
> + uint8_t SecureSiliconSector;
> + uint8_t SoftwareFeatures;
> } __packed;
>
> /* Vendor-Specific PRI for Atmel chips (command set 0x0002) */
Powered by blists - more mailing lists