| 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: <1b77e02a-2dfe-3a41-6a28-c415f42e5fd7@microchip.com>
Date: Wed, 20 Apr 2022 05:25:36 +0000
From: <Tudor.Ambarus@...rochip.com>
To: <p.yadav@...com>
CC: <michael@...le.cc>, <miquel.raynal@...tlin.com>, <richard@....at>,
<vigneshr@...com>, <linux-mtd@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <Nicolas.Ferre@...rochip.com>,
<Takahiro.Kuwano@...ineon.com>
Subject: Re: [PATCH v3 6/9] mtd: spi-nor: core: Add helpers to read/write any
register
On 4/20/22 07:34, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 19/04/22 12:56PM, Tudor.Ambarus@...rochip.com wrote:
>> On 4/19/22 15:46, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Am 2022-04-19 14:32, schrieb Pratyush Yadav:
>>>> On 19/04/22 12:08PM, Tudor.Ambarus@...rochip.com wrote:
>>>>> On 4/19/22 14:46, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> Am 2022-04-19 13:19, schrieb Michael Walle:
>>>>>>> Am 2022-04-11 11:10, schrieb Tudor Ambarus:
>>>>>>>> There are manufacturers that use registers indexed by address. Some of
>>>>>>>> them support "read/write any register" opcodes. Provide core methods
>>>>>>>> that
>>>>>>>> can be used by all manufacturers. SPI NOR controller ops are
>>>>>>>> intentionally
>>>>>>>> not supported as we intend to move all the SPI NOR controller drivers
>>>>>>>> under the SPI subsystem.
>>>>>>>>
>>>>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@...rochip.com>
>>>>>>>> Tested-by: Takahiro Kuwano <Takahiro.Kuwano@...ineon.com>
>>>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@...com>
>>>>>>>
>>>>>>> I still don't like it because the function doesn't do
>>>>>>> anything what the function name might suggest. The read
>>>>>>> just executes an op, the write executes an op with a
>>>>>>> write enable before. All the behavior is determined by the
>>>>>>> 'op' argument.
>>>>>>>
>>>>>>> Anyway,
>>>>>>> Reviewed-by: Michael Walle <michael@...le.cc>
>>>>>>>
>>>>>>>> ---
>>>>>>>> v3: no changes
>>>>>>>>
>>>>>>>> drivers/mtd/spi-nor/core.c | 41
>>>>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>>>> drivers/mtd/spi-nor/core.h | 4 ++++
>>>>>>>> 2 files changed, 45 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>>>> index 6165dc7bfd17..42794328d3b6 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>>> @@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor,
>>>>>>>> loff_t to, size_t len,
>>>>>>>> return nor->controller_ops->write(nor, to, len, buf);
>>>>>>>> }
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * spi_nor_read_reg() - read register to flash memory
>>>>>>>> + * @nor: pointer to 'struct spi_nor'.
>>>>>>>> + * @op: SPI memory operation. op->data.buf must be DMA-able.
>>>>>>>> + * @proto: SPI protocol to use for the register operation.
>>>>>>>> + *
>>>>>>>> + * Return: zero on success, -errno otherwise
>>>>>>>> + */
>>>>>>>> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
>>>>>>>> + enum spi_nor_protocol proto)
>>>>>>>> +{
>>>>>>>> + if (!nor->spimem)
>>>>>>>> + return -EOPNOTSUPP;
>>>>>>>> +
>>>>>>>> + spi_nor_spimem_setup_op(nor, op, proto);
>>>>>>>> + return spi_nor_spimem_exec_op(nor, op);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * spi_nor_write_reg() - write register to flash memory
>>>>>>>> + * @nor: pointer to 'struct spi_nor'
>>>>>>>> + * @op: SPI memory operation. op->data.buf must be DMA-able.
>>>>>>>> + * @proto: SPI protocol to use for the register operation.
>>>>>>>> + *
>>>>>>>> + * Return: zero on success, -errno otherwise
>>>>>>>> + */
>>>>>>>> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
>>>>>>>> + enum spi_nor_protocol proto)
>>>>>>>> +{
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + if (!nor->spimem)
>>>>>>>> + return -EOPNOTSUPP;
>>>>>>>> +
>>>>>>>> + ret = spi_nor_write_enable(nor);
>>>>>>>> + if (ret)
>>>>>>>> + return ret;
>>>>>>>> + spi_nor_spimem_setup_op(nor, op, proto);
>>>>>>>> + return spi_nor_spimem_exec_op(nor, op);
>>>>>>
>>>>>> After seeing your next two patches. Shouldn't the
>>>>>> spi_nor_wait_until_ready() call be here too?
>>>>>>
>>>>>
>>>>> I thought of this too, but seems that for a reason that I don't
>>>>> remember, we don't call for spi_nor_wait_until_ready after we
>>>>> write the octal DTR bit. Pratyush, do you remember why?
>>>>
>>>> We are not sure the protocol changed correctly so we can't rely on
>>>> spi_nor_wait_until_ready(). We read the ID instead to be sure.
>>>
>>> So besides the fact that the write_reg only works with the 'correct'
>>> op parameter, it is also tailored to the special use case. For real
>>> write_reg(), the user would actually has to poll the status bit
>>> afterwards? :(
>>>
>> Don't be sad :D. Are the octal DTR methods an exception?
>> If yes, let's add the call to spi_nor_wait_until_ready() in the
>> read/write_any_reg() methods, and let the octal methods handle
>> the specific write themselves, without calling for ready()
>
> It has been a while, but IIRC I asked Cypress about this, because I was
> worried about reading ID while the switch to 8D mode was still in
> progress. They said that volatile register writes are instant and do not
> need any status polling. So the switch to 8D-8D-8D mode would be instant
> and there is no need to wait for anything.
>
> The Cypress S28 flash datasheet does not say this explicitly. It does
> say that writing to non-volatile registers takes time and you need to
> wait for ready for those, but makes no mention of volatile registers. We
> don't ever want to write non-volatile registers so we can ignore that
> problem.
>
> I see the Micron MT35 datasheet say this explicitly, that changes to
> volatile registers are instant.
>
> So I would say that based on my limited sample size, volatile register
> writes for Cypress and Micron flashes do _not_ need
> spi_nor_wait_until_ready().
Thanks, Pratyush! I'll add a comment and/or update the commit message,
this is useful info.
Cheers,
ta
Powered by blists - more mailing lists