Re: [PATCH v3 6/9] mtd: spi-nor: core: Add helpers to read/write any register

[<Tudor.Ambarus@microchip.com>]

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@microchip.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@microchip.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@microchip.com>
>>>>>>>> Tested-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.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@walle.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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/