[PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[Takahiro Kuwano]

On 11/13/2024 6:11 PM, Miquel Raynal wrote:
> On 13/11/2024 at 11:06:21 +09, Takahiro Kuwano <tkuw584924 at gmail.com> wrote:
> 
>> Hi,
>>
>> On 11/12/2024 3:54 AM, Miquel Raynal wrote:
>>> Hi,
>>>
>>> On 31/10/2024 at 11:21:54 +09, tkuw584924 at gmail.com wrote:
>>>
>>>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>
>>>> SkyHigh spinand device has ECC enable bit in configuration register but
>>>> it must be always enabled. If ECC is disabled, read and write ops
>>>> results in undetermined state. For such devices, a way to avoid raw
>>>> access is needed.
>>>>
>>>> Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
>>>> support raw access. Read and write page in raw mode for the device
>>>> returns error.
>>>>
>>>> Checking and marking BBM need to be performed with ECC enabled to read
>>>> and write the BBM correctly.
>>>
>>> I see your point but I'm a bit puzzled by how it's being done.
>>>
>>> First, you disregard completely the isbad() and markbad()
>>> situations. Please have look into that because these functions are
>>> broken with your devices.
>>>
>> Yes... now I understand how they are broken.
>>
>>> Second, what about adding this detail to the ondie ECC engine? You could
>>> simply return an error from there, so basically a single (or maybe two)
>>> changes overall.
>>>
>> Thank you for the suggestion.
>> How about change like below in prepare_finish_io_req()?
>>
>> static int spinand_ondie_ecc_prepare_io_req(struct nand_device *nand,
>> 					    struct nand_page_io_req *req)
>> {
>> 	struct spinand_device *spinand = nand_to_spinand(nand);
>> 	bool enable = (req->mode != MTD_OPS_RAW);
>>
>> +	/*
>> +	 * For the devices that prohibit raw access (on-die ECC must be always
>> +	 * enabled), return error in case of raw data access. Accessing to OOB
>> +	 * needs to be allowed with on-die ECC enabled to support BBM checking
>> +	 * and marking.
>> +	 */
>> +	if (!enable && spinand->flags & SPINAND_NO_RAW_ACCESS) {
>> +		if (req->datalen)
>> +			return -ENOTSUPP;
> 
> EOPNOTSUPP
> 
>> +
>> +		enable = true;
>> +	}
> 
> No, this is lying, I don't like that.
> 
> What about just returnint an error here if you cannot do what the upper
> layer has asked you to do. And in the upper layer, you can add two
> specific conditions to is_bad/mark_bad() allowing to fallback to a "with
> ECC" operation in the EOPNOTSUPP case (with a short comment).
> 
OK, I will do it in next revision.
Thanks again.

BTW, spinand_write_enable_op() in spinand_markbad() looks redundant as it is
called in spinand_write_page(). Let me remove it before adding fallback.

> Thanks,
> Miquèl