| 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: <1dd3b793-f159-2454-aafa-a0f5706cc581@microchip.com>
Date: Thu, 1 Oct 2020 08:50:33 +0000
From: <Tudor.Ambarus@...rochip.com>
To: <p.yadav@...com>
CC: <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<nsekhar@...com>, <boris.brezillon@...labora.com>
Subject: Re: [PATCH v14 14/15] mtd: spi-nor: spansion: add support for Cypress
Semper flash
On 10/1/20 11:40 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 01/10/20 08:35AM, Tudor.Ambarus@...rochip.com wrote:
>> On 9/30/20 9:57 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> The Cypress Semper flash is an xSPI compliant octal DTR flash. Add
>>> support for using it in octal DTR mode.
>>>
>>> The flash by default boots in a hybrid sector mode. But the sector map
>>> table on the part I had was programmed incorrectly and the SMPT values
>>> on the flash don't match the public datasheet. Specifically, in some
>>> places erase type 3 was used instead of 4. In addition, the region sizes
>>> were incorrect in some places. So, for testing I set CFR3N[3] to enable
>>> uniform sector sizes. Since the uniform sector mode bit is a
>>> non-volatile bit, this series does not change it to avoid making any
>>> permanent changes to the flash configuration. The correct data to
>>> implement a fixup is not available right now and will be done in a
>>> follow-up patch if needed.
>>>
>>> Signed-off-by: Pratyush Yadav <p.yadav@...com>
>>> ---
>>> drivers/mtd/spi-nor/spansion.c | 160 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 160 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>> index 8429b4af999a..dc6b14aba405 100644
>>> --- a/drivers/mtd/spi-nor/spansion.c
>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>> @@ -8,6 +8,161 @@
>>>
>>> #include "core.h"
>>>
>>> +#define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */
>>> +#define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
>>> +#define SPINOR_REG_CYPRESS_CFR2V 0x00800003
>>> +#define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24 0xb
>>> +#define SPINOR_REG_CYPRESS_CFR3V 0x00800004
>>> +#define SPINOR_REG_CYPRESS_CFR3V_PGSZ BIT(4) /* Page size. */
>>> +#define SPINOR_REG_CYPRESS_CFR5V 0x00800006
>>> +#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN 0x3
>>> +#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS 0
>>> +#define SPINOR_OP_CYPRESS_RD_FAST 0xee
>>> +
>>> +/**
>>> + * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>> + * @nor: pointer to a 'struct spi_nor'
>>> + * @enable: whether to enable or disable Octal DTR
>>> + *
>>> + * This also sets the memory access latency cycles to 24 to allow the flash to
>>> + * run at up to 200MHz.
>>> + *
>>> + * Return: 0 on success, -errno otherwise.
>>> + */
>>> +static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
>>> +{
>>> + struct spi_mem_op op;
>>> + u8 *buf = nor->bouncebuf;
>>> + int ret;
>>> +
>>> + if (enable) {
>>> + /* Use 24 dummy cycles for memory array reads. */
>>> + ret = spi_nor_write_enable(nor);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + *buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
>>> + op = (struct spi_mem_op)
>>> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
>>> + SPI_MEM_OP_ADDR(3, SPINOR_REG_CYPRESS_CFR2V,
>>> + 1),
>>> + SPI_MEM_OP_NO_DUMMY,
>>> + SPI_MEM_OP_DATA_OUT(1, buf, 1));
>>> +
>>> + ret = spi_mem_exec_op(nor->spimem, &op);
>>> + if (ret) {
>>> + dev_dbg(nor->dev,
>>> + "failed to set default memory latency value: %d\n",
>>> + ret);
>>
>> Why do you consider the message important? I'm asking for consistency
>> reasons, below on the next exec_op call there's no dev_dbg message.
>> I would get rid of the dev_dbg entirely, the error code should be enough,
>> but I agree that this might be just a personal preference. So, do as you
>> prefer, but try to be consistent across the code.
>
> Yes it is mostly personal preference, but I don't mind either way. I'll
> drop the dev_dbg().
>
>>> + return ret;
>>> + }
>>> +
>>> + ret = spi_nor_wait_till_ready(nor);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + nor->read_dummy = 24;
>>> + }
>>> +
>>> + /* Set/unset the octal and DTR enable bits. */
>>> + ret = spi_nor_write_enable(nor);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (enable)
>>> + *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
>>> + else
>>> + *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
>>> +
>>> + op = (struct spi_mem_op)
>>> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
>>> + SPI_MEM_OP_ADDR(enable ? 3 : 4,
>>> + SPINOR_REG_CYPRESS_CFR5V,
>>> + 1),
>>> + SPI_MEM_OP_NO_DUMMY,
>>> + SPI_MEM_OP_DATA_OUT(1, buf, 1));
>>> +
>>> + if (!enable)
>>> + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
>>> +
>>> + ret = spi_mem_exec_op(nor->spimem, &op);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Give some time for the mode change to take place. */
>>> + usleep_range(400, 600);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void s28hs512t_default_init(struct spi_nor *nor)
>>> +{
>>> + nor->params->octal_dtr_enable = spi_nor_cypress_octal_dtr_enable;
>>> +}
>>> +
>>> +static void s28hs512t_post_sfdp_fixup(struct spi_nor *nor)
>>> +{
>>> + /*
>>> + * On older versions of the flash the xSPI Profile 1.0 table has the
>>> + * 8D-8D-8D Fast Read opcode as 0x00. But it actually should be 0xEE.
>>> + */
>>> + if (nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].opcode == 0)
>>> + nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].opcode =
>>> + SPINOR_OP_CYPRESS_RD_FAST;
>>> +
>>> + /* This flash is also missing the 4-byte Page Program opcode bit. */
>>> + spi_nor_set_pp_settings(&nor->params->page_programs[SNOR_CMD_PP],
>>> + SPINOR_OP_PP_4B, SNOR_PROTO_1_1_1);
>>> + /*
>>> + * Since xSPI Page Program opcode is backward compatible with
>>> + * Legacy SPI, use Legacy SPI opcode there as well.
>>> + */
>>> + spi_nor_set_pp_settings(&nor->params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
>>> + SPINOR_OP_PP_4B, SNOR_PROTO_8_8_8_DTR);
>>> +
>>> + /*
>>> + * The xSPI Profile 1.0 table advertises the number of additional
>>> + * address bytes needed for Read Status Register command as 0 but the
>>> + * actual value for that is 4.
>>> + */
>>> + nor->params->rdsr_addr_nbytes = 4;
>>> +}
>>> +
>>> +static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
>>> + const struct sfdp_parameter_header *bfpt_header,
>>> + const struct sfdp_bfpt *bfpt,
>>> + struct spi_nor_flash_parameter *params)
>>> +{
>>> + /*
>>> + * The BFPT table advertises a 512B page size but the page size is
>>> + * actually configurable (with the default being 256B). Read from
>>> + * CFR3V[4] and set the correct size.
>>> + */
>>> + struct spi_mem_op op =
>>> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 1),
>>> + SPI_MEM_OP_ADDR(3, SPINOR_REG_CYPRESS_CFR3V, 1),
>>> + SPI_MEM_OP_NO_DUMMY,
>>> + SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
>>> + int ret;
>>> +
>>> + ret = spi_mem_exec_op(nor->spimem, &op);
>>> + if (ret)
>>> + return ret;
>>
>> no need to wait after CFR3V?
>
> It is a read operation. We don't need to wait after reads.
>
oh, yes.
Powered by blists - more mailing lists