Re: [PATCH 5/5] mtd: rawnand: qcom: reorganization by removing read/write helpers
On 2018-07-19 03:24, Boris Brezillon wrote: On Fri, 6 Jul 2018 13:21:59 +0530 Abhishek Sahu wrote: Driver does not send the commands to NAND device for page read/write operations in ->cmdfunc(). It just does some minor variable initialization and rest of the things are being done in actual ->read/write_oob[_raw]. The generic helper function calls for invoking commands during page read/write are making this driver complicated. For QCOM NAND driver, ->cmdfunc() does minor part of initialization and rest of the initialization is performed by actual page read/write functions. Also, ->read/write_oob() does not calls helper function and all the initialization is being done in ->read/write_oob() itself. This sounds hazardous in the long run. Some vendor specific commands are re-using the READ0/READSTART semantic to read particular registers/OTP sections programmed at flash production time. For these operations, we don't want to go through the regular chip->ecc.read_page[_raw]() hooks, and instead use ->cmdfunc()/->exec_op(). You probably don't have setups with such NANDs yet, but that might be the case at some point. Thanks Boris and Miquel for pointing this out. Yes. We don't have that setup yet and the current driver code also can't handle that condition since the command function was not doing anything in this regard. Since the current driver code was complicated with lot of code duplication, so this patch was raised to cleanup few things. As already suggested by Miqule, I strongly recommend that you work on supporting ->exec_op() instead of trying to clean things up prematurely. I did some analysis and it seems moving to ->exec_op() will help in long run. I will start working on that but it will take some time for us since It requires major code changes with lot of testing. I will update you when we have RFC patch ready for this. Meanwhile, I will address review comments in other patches of this series and will post v2 after removing this patch from patch series. Regards, Abhishek Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to explicitly send READ/PROG commands")', sending of commands has been moved to driver for page read/write, so this patch does following changes to make code more readable: 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op() helper functions which helps in removing code duplication in each operation. 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND controller waits for BUSY signal to be de asserted and automatically issues the READ STATUS command. Currently, driver is storing this status privately and returns the same when status command comes from helper function after program/erase operation. Now, for write operations, the status can be returned from main function itself, so storing status can be removed for program operations. Signed-off-by: Abhishek Sahu --- drivers/mtd/nand/raw/qcom_nandc.c | 222 -- 1 file changed, 91 insertions(+), 131 deletions(-) diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index 6fb85d3..f73ee0e 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host *host, int command) host->last_command = command; clear_read_regs(nandc); - - if (command == NAND_CMD_RESET || command == NAND_CMD_READID || - command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1) - clear_bam_transaction(nandc); + clear_bam_transaction(nandc); } /* - * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our - * privately maintained status byte, this status byte can be read after - * NAND_CMD_STATUS is called + * QCOM NAND controller by default issues READ STATUS command after PROGRAM + * PAGE/BLOCK ERASE operation and updates the same in its internal status + * register for last codeword. This function parses status for each CW and + * return actual status byte for write/erase operation. */ -static void parse_erase_write_errors(struct qcom_nand_host *host, int command) +static u8 parse_erase_write_errors(struct qcom_nand_host *host, int num_cw) { struct nand_chip *chip = >chip; struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); - struct nand_ecc_ctrl *ecc = >ecc; - int num_cw; int i; + u8 status = 0; - num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1; nandc_read_buffer_sync(nandc, true); for (i = 0; i < num_cw; i++) { u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]); if (flash_status & FS_MPU_ERR) - host->status &= ~NAND_STATUS_WP; + status &= ~NAND_STATUS_WP; if (flash_status & FS_OP_ERR || (i == (num_cw - 1) &&
Re: [PATCH 5/5] mtd: rawnand: qcom: reorganization by removing read/write helpers
On 2018-07-19 03:24, Boris Brezillon wrote: On Fri, 6 Jul 2018 13:21:59 +0530 Abhishek Sahu wrote: Driver does not send the commands to NAND device for page read/write operations in ->cmdfunc(). It just does some minor variable initialization and rest of the things are being done in actual ->read/write_oob[_raw]. The generic helper function calls for invoking commands during page read/write are making this driver complicated. For QCOM NAND driver, ->cmdfunc() does minor part of initialization and rest of the initialization is performed by actual page read/write functions. Also, ->read/write_oob() does not calls helper function and all the initialization is being done in ->read/write_oob() itself. This sounds hazardous in the long run. Some vendor specific commands are re-using the READ0/READSTART semantic to read particular registers/OTP sections programmed at flash production time. For these operations, we don't want to go through the regular chip->ecc.read_page[_raw]() hooks, and instead use ->cmdfunc()/->exec_op(). You probably don't have setups with such NANDs yet, but that might be the case at some point. Thanks Boris and Miquel for pointing this out. Yes. We don't have that setup yet and the current driver code also can't handle that condition since the command function was not doing anything in this regard. Since the current driver code was complicated with lot of code duplication, so this patch was raised to cleanup few things. As already suggested by Miqule, I strongly recommend that you work on supporting ->exec_op() instead of trying to clean things up prematurely. I did some analysis and it seems moving to ->exec_op() will help in long run. I will start working on that but it will take some time for us since It requires major code changes with lot of testing. I will update you when we have RFC patch ready for this. Meanwhile, I will address review comments in other patches of this series and will post v2 after removing this patch from patch series. Regards, Abhishek Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to explicitly send READ/PROG commands")', sending of commands has been moved to driver for page read/write, so this patch does following changes to make code more readable: 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op() helper functions which helps in removing code duplication in each operation. 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND controller waits for BUSY signal to be de asserted and automatically issues the READ STATUS command. Currently, driver is storing this status privately and returns the same when status command comes from helper function after program/erase operation. Now, for write operations, the status can be returned from main function itself, so storing status can be removed for program operations. Signed-off-by: Abhishek Sahu --- drivers/mtd/nand/raw/qcom_nandc.c | 222 -- 1 file changed, 91 insertions(+), 131 deletions(-) diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index 6fb85d3..f73ee0e 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host *host, int command) host->last_command = command; clear_read_regs(nandc); - - if (command == NAND_CMD_RESET || command == NAND_CMD_READID || - command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1) - clear_bam_transaction(nandc); + clear_bam_transaction(nandc); } /* - * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our - * privately maintained status byte, this status byte can be read after - * NAND_CMD_STATUS is called + * QCOM NAND controller by default issues READ STATUS command after PROGRAM + * PAGE/BLOCK ERASE operation and updates the same in its internal status + * register for last codeword. This function parses status for each CW and + * return actual status byte for write/erase operation. */ -static void parse_erase_write_errors(struct qcom_nand_host *host, int command) +static u8 parse_erase_write_errors(struct qcom_nand_host *host, int num_cw) { struct nand_chip *chip = >chip; struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); - struct nand_ecc_ctrl *ecc = >ecc; - int num_cw; int i; + u8 status = 0; - num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1; nandc_read_buffer_sync(nandc, true); for (i = 0; i < num_cw; i++) { u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]); if (flash_status & FS_MPU_ERR) - host->status &= ~NAND_STATUS_WP; + status &= ~NAND_STATUS_WP; if (flash_status & FS_OP_ERR || (i == (num_cw - 1) &&
Re: [PATCH 5/5] mtd: rawnand: qcom: reorganization by removing read/write helpers
On Fri, 6 Jul 2018 13:21:59 +0530 Abhishek Sahu wrote: > Driver does not send the commands to NAND device for page > read/write operations in ->cmdfunc(). It just does some > minor variable initialization and rest of the things > are being done in actual ->read/write_oob[_raw]. > > The generic helper function calls for invoking commands during > page read/write are making this driver complicated. For QCOM NAND > driver, ->cmdfunc() does minor part of initialization and rest of > the initialization is performed by actual page read/write > functions. Also, ->read/write_oob() does not calls helper > function and all the initialization is being done in > ->read/write_oob() itself. This sounds hazardous in the long run. Some vendor specific commands are re-using the READ0/READSTART semantic to read particular registers/OTP sections programmed at flash production time. For these operations, we don't want to go through the regular chip->ecc.read_page[_raw]() hooks, and instead use ->cmdfunc()/->exec_op(). You probably don't have setups with such NANDs yet, but that might be the case at some point. As already suggested by Miqule, I strongly recommend that you work on supporting ->exec_op() instead of trying to clean things up prematurely. > > Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to > explicitly send READ/PROG commands")', sending of commands has > been moved to driver for page read/write, so this patch does > following changes to make code more readable: > > 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op() >helper functions which helps in removing code duplication in each >operation. > > 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND >controller waits for BUSY signal to be de asserted and >automatically issues the READ STATUS command. Currently, driver >is storing this status privately and returns the same when status >command comes from helper function after program/erase operation. >Now, for write operations, the status can be returned from main >function itself, so storing status can be removed for program >operations. > > Signed-off-by: Abhishek Sahu > --- > drivers/mtd/nand/raw/qcom_nandc.c | 222 > -- > 1 file changed, 91 insertions(+), 131 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c > b/drivers/mtd/nand/raw/qcom_nandc.c > index 6fb85d3..f73ee0e 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host *host, > int command) > host->last_command = command; > > clear_read_regs(nandc); > - > - if (command == NAND_CMD_RESET || command == NAND_CMD_READID || > - command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1) > - clear_bam_transaction(nandc); > + clear_bam_transaction(nandc); > } > > /* > - * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our > - * privately maintained status byte, this status byte can be read after > - * NAND_CMD_STATUS is called > + * QCOM NAND controller by default issues READ STATUS command after PROGRAM > + * PAGE/BLOCK ERASE operation and updates the same in its internal status > + * register for last codeword. This function parses status for each CW and > + * return actual status byte for write/erase operation. > */ > -static void parse_erase_write_errors(struct qcom_nand_host *host, int > command) > +static u8 parse_erase_write_errors(struct qcom_nand_host *host, int num_cw) > { > struct nand_chip *chip = >chip; > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct nand_ecc_ctrl *ecc = >ecc; > - int num_cw; > int i; > + u8 status = 0; > > - num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1; > nandc_read_buffer_sync(nandc, true); > > for (i = 0; i < num_cw; i++) { > u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]); > > if (flash_status & FS_MPU_ERR) > - host->status &= ~NAND_STATUS_WP; > + status &= ~NAND_STATUS_WP; > > if (flash_status & FS_OP_ERR || (i == (num_cw - 1) && >(flash_status & > FS_DEVICE_STS_ERR))) > - host->status |= NAND_STATUS_FAIL; > + status |= NAND_STATUS_FAIL; > } > + > + return status; > } > > static void post_command(struct qcom_nand_host *host, int command) > @@ -1428,9 +1426,12 @@ static void post_command(struct qcom_nand_host *host, > int command) > memcpy(nandc->data_buffer, nandc->reg_read_buf, > nandc->buf_count); > break; > - case NAND_CMD_PAGEPROG: > case NAND_CMD_ERASE1: > - parse_erase_write_errors(host, command); > + /*
Re: [PATCH 5/5] mtd: rawnand: qcom: reorganization by removing read/write helpers
On Fri, 6 Jul 2018 13:21:59 +0530 Abhishek Sahu wrote: > Driver does not send the commands to NAND device for page > read/write operations in ->cmdfunc(). It just does some > minor variable initialization and rest of the things > are being done in actual ->read/write_oob[_raw]. > > The generic helper function calls for invoking commands during > page read/write are making this driver complicated. For QCOM NAND > driver, ->cmdfunc() does minor part of initialization and rest of > the initialization is performed by actual page read/write > functions. Also, ->read/write_oob() does not calls helper > function and all the initialization is being done in > ->read/write_oob() itself. This sounds hazardous in the long run. Some vendor specific commands are re-using the READ0/READSTART semantic to read particular registers/OTP sections programmed at flash production time. For these operations, we don't want to go through the regular chip->ecc.read_page[_raw]() hooks, and instead use ->cmdfunc()/->exec_op(). You probably don't have setups with such NANDs yet, but that might be the case at some point. As already suggested by Miqule, I strongly recommend that you work on supporting ->exec_op() instead of trying to clean things up prematurely. > > Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to > explicitly send READ/PROG commands")', sending of commands has > been moved to driver for page read/write, so this patch does > following changes to make code more readable: > > 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op() >helper functions which helps in removing code duplication in each >operation. > > 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND >controller waits for BUSY signal to be de asserted and >automatically issues the READ STATUS command. Currently, driver >is storing this status privately and returns the same when status >command comes from helper function after program/erase operation. >Now, for write operations, the status can be returned from main >function itself, so storing status can be removed for program >operations. > > Signed-off-by: Abhishek Sahu > --- > drivers/mtd/nand/raw/qcom_nandc.c | 222 > -- > 1 file changed, 91 insertions(+), 131 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c > b/drivers/mtd/nand/raw/qcom_nandc.c > index 6fb85d3..f73ee0e 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host *host, > int command) > host->last_command = command; > > clear_read_regs(nandc); > - > - if (command == NAND_CMD_RESET || command == NAND_CMD_READID || > - command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1) > - clear_bam_transaction(nandc); > + clear_bam_transaction(nandc); > } > > /* > - * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our > - * privately maintained status byte, this status byte can be read after > - * NAND_CMD_STATUS is called > + * QCOM NAND controller by default issues READ STATUS command after PROGRAM > + * PAGE/BLOCK ERASE operation and updates the same in its internal status > + * register for last codeword. This function parses status for each CW and > + * return actual status byte for write/erase operation. > */ > -static void parse_erase_write_errors(struct qcom_nand_host *host, int > command) > +static u8 parse_erase_write_errors(struct qcom_nand_host *host, int num_cw) > { > struct nand_chip *chip = >chip; > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct nand_ecc_ctrl *ecc = >ecc; > - int num_cw; > int i; > + u8 status = 0; > > - num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1; > nandc_read_buffer_sync(nandc, true); > > for (i = 0; i < num_cw; i++) { > u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]); > > if (flash_status & FS_MPU_ERR) > - host->status &= ~NAND_STATUS_WP; > + status &= ~NAND_STATUS_WP; > > if (flash_status & FS_OP_ERR || (i == (num_cw - 1) && >(flash_status & > FS_DEVICE_STS_ERR))) > - host->status |= NAND_STATUS_FAIL; > + status |= NAND_STATUS_FAIL; > } > + > + return status; > } > > static void post_command(struct qcom_nand_host *host, int command) > @@ -1428,9 +1426,12 @@ static void post_command(struct qcom_nand_host *host, > int command) > memcpy(nandc->data_buffer, nandc->reg_read_buf, > nandc->buf_count); > break; > - case NAND_CMD_PAGEPROG: > case NAND_CMD_ERASE1: > - parse_erase_write_errors(host, command); > + /*
Re: [PATCH 5/5] mtd: rawnand: qcom: reorganization by removing read/write helpers
Hi Abhishek, Abhishek Sahu wrote on Fri, 6 Jul 2018 13:21:59 +0530: > Driver does not send the commands to NAND device for page > read/write operations in ->cmdfunc(). It just does some > minor variable initialization and rest of the things > are being done in actual ->read/write_oob[_raw]. Thank you for cleaning actively this driver. I think you are comfortable enough now to switch to start the migration to ->exec_op(). I will take this patch as I think it shrinks a bit the driver (which is inordinately huge) but I would like to get rid of any kind of hackish ->cmdfunc() implementation. Boris and I can help you doing that, you can grep for drivers already converted and observe they structure (marvell, fsmc, vf610). > > The generic helper function calls for invoking commands during > page read/write are making this driver complicated. For QCOM NAND > driver, ->cmdfunc() does minor part of initialization and rest of > the initialization is performed by actual page read/write > functions. Also, ->read/write_oob() does not calls helper > function and all the initialization is being done in > ->read/write_oob() itself. > > Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to > explicitly send READ/PROG commands")', sending of commands has > been moved to driver for page read/write, so this patch does > following changes to make code more readable: > > 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op() >helper functions which helps in removing code duplication in each >operation. > > 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND >controller waits for BUSY signal to be de asserted and >automatically issues the READ STATUS command. Currently, driver >is storing this status privately and returns the same when status >command comes from helper function after program/erase operation. >Now, for write operations, the status can be returned from main >function itself, so storing status can be removed for program >operations. > > Signed-off-by: Abhishek Sahu > --- Thanks, Miquèl
Re: [PATCH 5/5] mtd: rawnand: qcom: reorganization by removing read/write helpers
Hi Abhishek, Abhishek Sahu wrote on Fri, 6 Jul 2018 13:21:59 +0530: > Driver does not send the commands to NAND device for page > read/write operations in ->cmdfunc(). It just does some > minor variable initialization and rest of the things > are being done in actual ->read/write_oob[_raw]. Thank you for cleaning actively this driver. I think you are comfortable enough now to switch to start the migration to ->exec_op(). I will take this patch as I think it shrinks a bit the driver (which is inordinately huge) but I would like to get rid of any kind of hackish ->cmdfunc() implementation. Boris and I can help you doing that, you can grep for drivers already converted and observe they structure (marvell, fsmc, vf610). > > The generic helper function calls for invoking commands during > page read/write are making this driver complicated. For QCOM NAND > driver, ->cmdfunc() does minor part of initialization and rest of > the initialization is performed by actual page read/write > functions. Also, ->read/write_oob() does not calls helper > function and all the initialization is being done in > ->read/write_oob() itself. > > Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to > explicitly send READ/PROG commands")', sending of commands has > been moved to driver for page read/write, so this patch does > following changes to make code more readable: > > 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op() >helper functions which helps in removing code duplication in each >operation. > > 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND >controller waits for BUSY signal to be de asserted and >automatically issues the READ STATUS command. Currently, driver >is storing this status privately and returns the same when status >command comes from helper function after program/erase operation. >Now, for write operations, the status can be returned from main >function itself, so storing status can be removed for program >operations. > > Signed-off-by: Abhishek Sahu > --- Thanks, Miquèl