| Message ID | 81c9caf04c94cba1a5ae4ba79dd09e0789583fcd.1730340421.git.Takahiro.Kuwano@infineon.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: spinand: Add support for SkyHigh S35ML-3 family | expand |
Hi, On 31/10/2024 at 11:21:54 +09, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@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. 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. Thanks, Miquèl
Hi, On 11/12/2024 3:54 AM, Miquel Raynal wrote: > Hi, > > On 31/10/2024 at 11:21:54 +09, tkuw584924@gmail.com wrote: > >> From: Takahiro Kuwano <Takahiro.Kuwano@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; + + enable = true; + } ... > Thanks, > Miquèl Thanks! Takahiro
On 13/11/2024 at 11:06:21 +09, Takahiro Kuwano <tkuw584924@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@gmail.com wrote: >> >>> From: Takahiro Kuwano <Takahiro.Kuwano@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). Thanks, Miquèl
On 11/13/2024 6:11 PM, Miquel Raynal wrote: > On 13/11/2024 at 11:06:21 +09, Takahiro Kuwano <tkuw584924@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@gmail.com wrote: >>> >>>> From: Takahiro Kuwano <Takahiro.Kuwano@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
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 4d76f9f71a0e..de04f6e7ce1e 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -608,6 +608,9 @@ static int spinand_read_page(struct spinand_device *spinand, u8 status; int ret; + if (req->mode == MTD_OPS_RAW && spinand->flags & SPINAND_NO_RAW_ACCESS) + return -ENOTSUPP; + ret = nand_ecc_prepare_io_req(nand, (struct nand_page_io_req *)req); if (ret) return ret; @@ -639,6 +642,9 @@ static int spinand_write_page(struct spinand_device *spinand, u8 status; int ret; + if (req->mode == MTD_OPS_RAW && spinand->flags & SPINAND_NO_RAW_ACCESS) + return -ENOTSUPP; + ret = nand_ecc_prepare_io_req(nand, (struct nand_page_io_req *)req); if (ret) return ret; @@ -902,6 +908,9 @@ static bool spinand_isbad(struct nand_device *nand, const struct nand_pos *pos) .mode = MTD_OPS_RAW, }; + if (spinand->flags & SPINAND_NO_RAW_ACCESS) + req.mode = MTD_OPS_PLACE_OOB; + spinand_select_target(spinand, pos->target); spinand_read_page(spinand, &req); if (marker[0] != 0xff || marker[1] != 0xff) @@ -938,6 +947,9 @@ static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) }; int ret; + if (spinand->flags & SPINAND_NO_RAW_ACCESS) + req.mode = MTD_OPS_PLACE_OOB; + ret = spinand_select_target(spinand, pos->target); if (ret) return ret; diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index 702e5fb13dae..5cf11005b41a 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -314,6 +314,7 @@ struct spinand_ecc_info { #define SPINAND_HAS_CR_FEAT_BIT BIT(1) #define SPINAND_HAS_PROG_PLANE_SELECT_BIT BIT(2) #define SPINAND_HAS_READ_PLANE_SELECT_BIT BIT(3) +#define SPINAND_NO_RAW_ACCESS BIT(4) /** * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure