| Message ID | 1383656135-8627-23-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote: > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -826,7 +887,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, > prepare_start_command(info, command); > > info->state = STATE_PREPARED; > - exec_cmd = prepare_set_command(info, command, column, page_addr); > + exec_cmd = prepare_set_command(info, command, -1, column, page_addr); Is it safe to use -1 for the third parameter (ext_cmd_type)? AFAICT, this doesn't make for correct input to the NDCB0_EXT_CMD_TYPE() macro. > + > if (exec_cmd) { > init_completion(&info->cmd_complete); > init_completion(&info->dev_ready); > @@ -844,6 +906,91 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, > info->state = STATE_IDLE; > } > > +static void armada370_nand_cmdfunc(struct mtd_info *mtd, > + const unsigned command, > + int column, int page_addr) > +{ > + struct pxa3xx_nand_host *host = mtd->priv; > + struct pxa3xx_nand_info *info = host->info_data; > + int ret, exec_cmd, ext_cmd_type; > + > + /* > + * if this is a x16 device ,then convert the input Misplaced comma/whitespace. > + * "byte" address into a "word" address appropriate > + * for indexing a word-oriented device > + */ > + if (info->reg_ndcr & NDCR_DWIDTH_M) > + column /= 2; > + > + /* > + * There may be different NAND chip hooked to > + * different chip select, so check whether > + * chip select has been changed, if yes, reset the timing > + */ > + if (info->cs != host->cs) { > + info->cs = host->cs; > + nand_writel(info, NDTR0CS0, info->ndtr0cs0); > + nand_writel(info, NDTR1CS0, info->ndtr1cs0); > + } > + > + /* Select the extended command for the first command */ > + switch (command) { > + case NAND_CMD_READ0: > + case NAND_CMD_READOOB: > + ext_cmd_type = EXT_CMD_TYPE_MONO; > + break; > + } You have no default case for this switch statement, leaving ext_cmd_type uninitialized in some cases. You add the other cases in a later patch, but this patch is temporarily broken. > + > + prepare_start_command(info, command); > + > + /* > + * Prepare the "is ready" completion before starting a command > + * transaction sequence. If the command is not executed the > + * completion will be completed, see below. > + * > + * We can do that inside the loop because the command variable > + * is invariant and thus so is the exec_cmd. > + */ > + atomic_set(&info->is_ready, 0); > + init_completion(&info->dev_ready); > + do { > + info->state = STATE_PREPARED; > + exec_cmd = prepare_set_command(info, command, ext_cmd_type, > + column, page_addr); [...] > @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info, > struct nand_ecc_ctrl *ecc, > int strength, int page_size) > { > - /* Unimplemented yet */ > + if (strength == 4 && page_size == 4096) { You compare only to ecc_strength_ds, and not ecc_step_ds. While it is likely that a 4-bit ECC NAND will have a 512-byte ECC step size, you should probably check this. What about strength < 4? Shouldn't you be able to support a 1-bit ECC NAND with your 4-bit ECC? Also, do you plan to support non-ONFI NAND? Remember that nand_base doesn't guarantee giving you a non-zero ECC strength. You might need a DT binding to specify this, if it's not automatically detectable. > + ecc->mode = NAND_ECC_HW; > + ecc->size = 512; > + ecc->layout = &ecc_layout_4KB_bch4bit; > + ecc->strength = 4; > + > + info->ecc_bch = 1; > + info->chunk_size = 2048; > + info->spare_size = 32; > + info->ecc_size = 32; > + return 1; > + > + } else if (strength == 8 && page_size == 4096) { > + ecc->mode = NAND_ECC_HW; > + ecc->size = 512; > + ecc->layout = &ecc_layout_4KB_bch8bit; > + ecc->strength = 8; These ECC parameters (8-bit per 512 and 4-bit per 512) sound reasonable and consistent with other ECC schemes I've seen. But I'm still not clear if we are 100% certain that matches the actual hardware implementation. Did you do any further research since the last time we talked about this? > + > + info->ecc_bch = 1; > + info->chunk_size = 1024; > + info->spare_size = 0; > + info->ecc_size = 32; > + return 1; > + } > return 0; > } > Brian
One more thing on this patch: On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote: > @@ -437,7 +487,7 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask) > > static void handle_data_pio(struct pxa3xx_nand_info *info) > { > - unsigned int do_bytes = min(info->data_size, info->fifo_size); > + unsigned int do_bytes = min(info->data_size, info->chunk_size); After this change, you no longer are using fifo_size globally (you only use it locally in pxa3xx_nand_detect_config() to set the chunk_size). Perhaps you can drop the field? > > switch (info->state) { > case STATE_PIO_WRITING: Brian
On Tue, Nov 05, 2013 at 11:04:32AM -0800, Brian Norris wrote: > On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote: > > --- a/drivers/mtd/nand/pxa3xx_nand.c > > +++ b/drivers/mtd/nand/pxa3xx_nand.c > > @@ -826,7 +887,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, > > prepare_start_command(info, command); > > > > info->state = STATE_PREPARED; > > - exec_cmd = prepare_set_command(info, command, column, page_addr); > > + exec_cmd = prepare_set_command(info, command, -1, column, page_addr); > > Is it safe to use -1 for the third parameter (ext_cmd_type)? AFAICT, > this doesn't make for correct input to the NDCB0_EXT_CMD_TYPE() macro. > Right. Probably '0' is a saner value. > > + > > if (exec_cmd) { > > init_completion(&info->cmd_complete); > > init_completion(&info->dev_ready); > > @@ -844,6 +906,91 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, > > info->state = STATE_IDLE; > > } > > > > +static void armada370_nand_cmdfunc(struct mtd_info *mtd, > > + const unsigned command, > > + int column, int page_addr) > > +{ > > + struct pxa3xx_nand_host *host = mtd->priv; > > + struct pxa3xx_nand_info *info = host->info_data; > > + int ret, exec_cmd, ext_cmd_type; > > + > > + /* > > + * if this is a x16 device ,then convert the input > > Misplaced comma/whitespace. > Ack. > > + * "byte" address into a "word" address appropriate > > + * for indexing a word-oriented device > > + */ > > + if (info->reg_ndcr & NDCR_DWIDTH_M) > > + column /= 2; > > + > > + /* > > + * There may be different NAND chip hooked to > > + * different chip select, so check whether > > + * chip select has been changed, if yes, reset the timing > > + */ > > + if (info->cs != host->cs) { > > + info->cs = host->cs; > > + nand_writel(info, NDTR0CS0, info->ndtr0cs0); > > + nand_writel(info, NDTR1CS0, info->ndtr1cs0); > > + } > > + > > + /* Select the extended command for the first command */ > > + switch (command) { > > + case NAND_CMD_READ0: > > + case NAND_CMD_READOOB: > > + ext_cmd_type = EXT_CMD_TYPE_MONO; > > + break; > > + } > > You have no default case for this switch statement, leaving ext_cmd_type > uninitialized in some cases. You add the other cases in a later patch, > but this patch is temporarily broken. > Right, I'll add a zero-valued default. > > + > > + prepare_start_command(info, command); > > + > > + /* > > + * Prepare the "is ready" completion before starting a command > > + * transaction sequence. If the command is not executed the > > + * completion will be completed, see below. > > + * > > + * We can do that inside the loop because the command variable > > + * is invariant and thus so is the exec_cmd. > > + */ > > + atomic_set(&info->is_ready, 0); > > + init_completion(&info->dev_ready); > > + do { > > + info->state = STATE_PREPARED; > > + exec_cmd = prepare_set_command(info, command, ext_cmd_type, > > + column, page_addr); > > [...] > > > @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info, > > struct nand_ecc_ctrl *ecc, > > int strength, int page_size) > > { > > - /* Unimplemented yet */ > > + if (strength == 4 && page_size == 4096) { > > You compare only to ecc_strength_ds, and not ecc_step_ds. While it is > likely that a 4-bit ECC NAND will have a 512-byte ECC step size, you > should probably check this. > OK. > What about strength < 4? Shouldn't you be able to support a 1-bit ECC > NAND with your 4-bit ECC? > Yes, probably. The rationale behind these xxx_ecc_init() functions is to only validate the devices I've tested or that are known to work. That said, maybe I'm being too paranoid. > Also, do you plan to support non-ONFI NAND? Not for the time being. (Are there any new non-ONFI NAND devices?) > Remember that nand_base > doesn't guarantee giving you a non-zero ECC strength. You might need a > DT binding to specify this, if it's not automatically detectable. > Given this series is already long and complex, I'd like to push as much features as possible to follow-up patches. That way we can support the current boards out there now (Mirabox's a relevant example) and add support for more later, gradually. > > + ecc->mode = NAND_ECC_HW; > > + ecc->size = 512; > > + ecc->layout = &ecc_layout_4KB_bch4bit; > > + ecc->strength = 4; > > + > > + info->ecc_bch = 1; > > + info->chunk_size = 2048; > > + info->spare_size = 32; > > + info->ecc_size = 32; > > + return 1; > > + > > + } else if (strength == 8 && page_size == 4096) { > > + ecc->mode = NAND_ECC_HW; > > + ecc->size = 512; > > + ecc->layout = &ecc_layout_4KB_bch8bit; > > + ecc->strength = 8; > > These ECC parameters (8-bit per 512 and 4-bit per 512) sound reasonable > and consistent with other ECC schemes I've seen. But I'm still not clear > if we are 100% certain that matches the actual hardware implementation. > Did you do any further research since the last time we talked about > this? > Yes, and I tried to answer your questions in detail in the cover letters (which now is in a patch for Documentation/mtd/nand/...) and in past discussion. See for instance: http://permalink.gmane.org/gmane.linux.drivers.mtd/48895. Feel free to ask any further clarification about how the ECC engine works. If you think it'll help, I can write a separate mail describing it (to the best of my knowledge) and we can discuss there. In particular, the above link contains a question that is still unanswered and that would help me understand this better. I'll copy-paste it here: """ Regarding this: I'd really like to understand better this 'step' concept and it applicability on this controller. So any clarification is welcome :) As far as I can see: currently the hardware does ECC corrections in a completely transparent fashion and merely 'reports' the no. of corrected bits through some IRQ plus some registers. Therefore, it implements the ecc.{read,write}_page() functions. So, why should I tell the NAND core any of the ECC details? I see other drivers need to implement some of the functions in the nand_ecc_ctrl struct, such as: hwctl(), calculate() or correct(). However, I can't see how any of that applies on this controller. """ Thanks a lot for reviewing all of this!
On Tue, Nov 5, 2013 at 5:13 PM, Ezequiel Garcia <ezequiel.garcia@free-electrons.com> wrote: > On Tue, Nov 05, 2013 at 11:04:32AM -0800, Brian Norris wrote: >> On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote: >> > --- a/drivers/mtd/nand/pxa3xx_nand.c >> > +++ b/drivers/mtd/nand/pxa3xx_nand.c >> > @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info, >> > struct nand_ecc_ctrl *ecc, >> > int strength, int page_size) >> > { >> > - /* Unimplemented yet */ >> > + if (strength == 4 && page_size == 4096) { >> >> You compare only to ecc_strength_ds, and not ecc_step_ds. While it is >> likely that a 4-bit ECC NAND will have a 512-byte ECC step size, you >> should probably check this. > > OK. > >> What about strength < 4? Shouldn't you be able to support a 1-bit ECC >> NAND with your 4-bit ECC? >> > > Yes, probably. The rationale behind these xxx_ecc_init() functions is > to only validate the devices I've tested or that are known to work. > > That said, maybe I'm being too paranoid. No, maybe paranoid is fine. I was just bringing it up. Being too liberal in what you accept can create compatibility problems later. So if you wait to handle generalities until you see them in practice, you can potentially save yourself some problems. >> Also, do you plan to support non-ONFI NAND? > > Not for the time being. (Are there any new non-ONFI NAND devices?) Sure there are. I've been seeing non-ONFI parts from Toshiba and Macronix, at least (I think Macronix is moving to ONFI soon, though). Samsung is around still, too. >> Remember that nand_base >> doesn't guarantee giving you a non-zero ECC strength. You might need a >> DT binding to specify this, if it's not automatically detectable. >> > > Given this series is already long and complex, I'd like to push as much > features as possible to follow-up patches. That way we can support the > current boards out there now (Mirabox's a relevant example) and add support > for more later, gradually. Right, I agree with a follow-up, and only if necessary. I was just trying to plant the seeds of thought for you, so we don't run into a maintenance problem such as Huang's gpmi-nand has. And as long as you can plan a reasonable forward/backward-compatible solution for any currently-supported configurations, then you don't need to add the DT binding at all, if you don't currently need it. This is where the paranoid approach can help you, too. >> > + ecc->mode = NAND_ECC_HW; >> > + ecc->size = 512; >> > + ecc->layout = &ecc_layout_4KB_bch4bit; >> > + ecc->strength = 4; >> > + >> > + info->ecc_bch = 1; >> > + info->chunk_size = 2048; >> > + info->spare_size = 32; >> > + info->ecc_size = 32; >> > + return 1; >> > + >> > + } else if (strength == 8 && page_size == 4096) { >> > + ecc->mode = NAND_ECC_HW; >> > + ecc->size = 512; >> > + ecc->layout = &ecc_layout_4KB_bch8bit; >> > + ecc->strength = 8; >> >> These ECC parameters (8-bit per 512 and 4-bit per 512) sound reasonable >> and consistent with other ECC schemes I've seen. But I'm still not clear >> if we are 100% certain that matches the actual hardware implementation. >> Did you do any further research since the last time we talked about >> this? >> > > Yes, and I tried to answer your questions in detail in the cover letters > (which now is in a patch for Documentation/mtd/nand/...) and in past > discussion. I don't see a patch for Documentation/mtd/nand/... Am I missing something? > See for instance: http://permalink.gmane.org/gmane.linux.drivers.mtd/48895. My memory fails me, I guess. You did answer some of my questions, but I was still left with some more. I failed to follow up properly... From the linked response: "So, if you set the controller to transfer 2048B you can correct up to 16 bits on this 2048B region, or 4 bits per 512B, hence BCH-4." Well, "16 bits per 2048" is at least as good as BCH-4 over 512B, but it is not exactly the same. A true "16 bits per 2048" would be able to correct 16 bitflips concentrated in a 512B region, whereas BCH-4/512B would not. But unless we can get better documentation/experiments with the controller, it's hard to tell which is the case, and it's probably not a big deal at this point. Your current code looks OK, I think. > Feel free to ask any further clarification about how the ECC engine > works. If you think it'll help, I can write a separate mail describing > it (to the best of my knowledge) and we can discuss there. > > In particular, the above link contains a question that is still > unanswered and that would help me understand this better. > I'll copy-paste it here: > > """ > Regarding this: I'd really like to understand better this 'step' concept > and it applicability on this controller. So any clarification is welcome > :) > > As far as I can see: currently the hardware does ECC corrections in a > completely transparent fashion and merely 'reports' the no. of corrected > bits through some IRQ plus some registers. > Therefore, it implements the ecc.{read,write}_page() functions. > > So, why should I tell the NAND core any of the ECC details? Because MTD/NAND now implements a bitflip threshold based on the reported ECC strength and step size. See commit: commit edbc4540e02c201bdd4f4d498ebb6ed517fd36e2 Author: Mike Dunn <mikedunn@newsguy.com> Date: Wed Apr 25 12:06:11 2012 -0700 mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN and some of the other related code and documentation, such as in this commit: commit d062d4ede877fcd2ecc4c6262abad09a6f32950a Author: Mike Dunn <mikedunn@newsguy.com> Date: Wed Apr 25 12:06:08 2012 -0700 mtd: bitflip_threshold added to mtd_info and sysfs If you don't have the correct strength, then this may not work as intended. (I realized now that the ECC step size doesn't play a part in the bitflip threshold process; only the strength is required to be correct.) ...and now that I bring this up, I see that pxa3xx_nand.c has not been updated to report 'max_bitflips' via return code from pxa3xx_nand_read_page_hwecc(). This needs fixed. I wonder how many other drivers are similarly broken. I just realized that my out-of-tree driver is broken for this feature :( > I see other drivers need to implement some of the functions in the > nand_ecc_ctrl struct, such as: hwctl(), calculate() or correct(). > However, I can't see how any of that applies on this controller. > """ I was not suggesting you need to implement these functions. If I've missed any other important questions, please bear with me and repeat them. Thanks, Brian
On Tue, Nov 5, 2013 at 6:20 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, Nov 5, 2013 at 5:13 PM, Ezequiel Garcia <ezequiel.garcia@free-electrons.com> wrote: >> So, why should I tell the NAND core any of the ECC details? > > Because MTD/NAND now implements a bitflip threshold based on the > reported ECC strength and step size. See commit: > > commit edbc4540e02c201bdd4f4d498ebb6ed517fd36e2 > Author: Mike Dunn <mikedunn@newsguy.com> > Date: Wed Apr 25 12:06:11 2012 -0700 > > mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN > > and some of the other related code and documentation, such as in this commit: > > commit d062d4ede877fcd2ecc4c6262abad09a6f32950a > Author: Mike Dunn <mikedunn@newsguy.com> > Date: Wed Apr 25 12:06:08 2012 -0700 > > mtd: bitflip_threshold added to mtd_info and sysfs > > If you don't have the correct strength, then this may not work as > intended. (I realized now that the ECC step size doesn't play a part > in the bitflip threshold process; only the strength is required to be > correct.) To clarify a bit further: the ECC step size doesn't play a part in nand_base when dealing with max_bitflips, but it is implicit in the specification: the driver should be returning the maximum number of bitflips corrected in a single ECC sector of the page that was read. So if your driver reports ECC strength of 8 over a 512 byte step, then you should report a number in the range of 0 to 8, representing bitflips in a 512B sector. If you don't have this granularity (which it looks like you might not?) then maybe your strength-per-sector should really be 16-per-2048B or 16-per-1024B, etc. Brian
On Tue, Nov 05, 2013 at 06:20:18PM -0800, Brian Norris wrote: [..] > > > > Yes, and I tried to answer your questions in detail in the cover letters > > (which now is in a patch for Documentation/mtd/nand/...) and in past > > discussion. > > I don't see a patch for Documentation/mtd/nand/... Am I missing something? > Just for the record: http://www.spinics.net/lists/arm-kernel/msg284271.html And I'll continue replying the rest of the feedback tomorrow :-) Thanks!
On Tue, Nov 05, 2013 at 06:20:18PM -0800, Brian Norris wrote: [..] > > >> Also, do you plan to support non-ONFI NAND? > > > > Not for the time being. (Are there any new non-ONFI NAND devices?) > > Sure there are. I've been seeing non-ONFI parts from Toshiba and > Macronix, at least (I think Macronix is moving to ONFI soon, though). > Samsung is around still, too. > Right. Well, for now I would just add a pr_warn("Your device is not supported. Please ask Ezequiel to add it") or something along those lines. BTW: So, I guess that for non-ONFI devices we must simply put the ECC information in the DT (being a hardware parameter)? Having followed (part) of the OMAP DT ECC discussion, I'm thinking: wouldn't it be good to have an 'nand-ecc-strength' property in the DT? It would match the ecc_strength_ds field. This way, the ECC mode selection is left to the driver and not to the user. On the other side, this can cause some severe incompatibilities unless such driver *always* make the *same* selection. Anyway, none of this matters in this patchset, for the time being. [..] > > From the linked response: > > "So, if you set the controller to transfer 2048B you can correct up to > 16 bits on this 2048B region, or 4 bits per 512B, hence BCH-4." > > Well, "16 bits per 2048" is at least as good as BCH-4 over 512B, but > it is not exactly the same. A true "16 bits per 2048" would be able to > correct 16 bitflips concentrated in a 512B region, whereas BCH-4/512B > would not. > Indeed, 16-over-2048 is stronger than 4-over-512 :) > But unless we can get better documentation/experiments with the > controller, it's hard to tell which is the case, and it's probably not > a big deal at this point. Your current code looks OK, I think. > Ok, great. > > Feel free to ask any further clarification about how the ECC engine > > works. If you think it'll help, I can write a separate mail describing > > it (to the best of my knowledge) and we can discuss there. > > > > In particular, the above link contains a question that is still > > unanswered and that would help me understand this better. > > I'll copy-paste it here: > > > > """ > > Regarding this: I'd really like to understand better this 'step' concept > > and it applicability on this controller. So any clarification is welcome > > :) > > > > As far as I can see: currently the hardware does ECC corrections in a > > completely transparent fashion and merely 'reports' the no. of corrected > > bits through some IRQ plus some registers. > > Therefore, it implements the ecc.{read,write}_page() functions. > > > > So, why should I tell the NAND core any of the ECC details? > > Because MTD/NAND now implements a bitflip threshold based on the > reported ECC strength and step size. See commit: > > commit edbc4540e02c201bdd4f4d498ebb6ed517fd36e2 > Author: Mike Dunn <mikedunn@newsguy.com> > Date: Wed Apr 25 12:06:11 2012 -0700 > > mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN > > and some of the other related code and documentation, such as in this commit: > > commit d062d4ede877fcd2ecc4c6262abad09a6f32950a > Author: Mike Dunn <mikedunn@newsguy.com> > Date: Wed Apr 25 12:06:08 2012 -0700 > > mtd: bitflip_threshold added to mtd_info and sysfs > > If you don't have the correct strength, then this may not work as > intended. (I realized now that the ECC step size doesn't play a part > in the bitflip threshold process; only the strength is required to be > correct.) > > ...and now that I bring this up, I see that pxa3xx_nand.c has not been > updated to report 'max_bitflips' via return code from > pxa3xx_nand_read_page_hwecc(). This needs fixed. I wonder how many > other drivers are similarly broken. I just realized that my > out-of-tree driver is broken for this feature :( > Hm.. I see. Well, it's just a matter of returning the corrected bits, which are already obtained. Furthermore, one of the last patches in this series, namely: "mtd: nand: pxa3xx: Add ECC BCH correctable errors detection" Takes care of obtaining the proper corrected bits number. (Let me paste here your other reply to this same mail) """ > To clarify a bit further: the ECC step size doesn't play a part in > nand_base when dealing with max_bitflips, but it is implicit in the > specification: the driver should be returning the maximum number of > bitflips corrected in a single ECC sector of the page that was read. > So if your driver reports ECC strength of 8 over a 512 byte step, then > you should report a number in the range of 0 to 8, representing > bitflips in a 512B sector. If you don't have this granularity (which > it looks like you might not?) then maybe your strength-per-sector > should really be 16-per-2048B or 16-per-1024B, etc. """ So, if I understand correctly and assuming the controller works as the specification says (sorry, it's not public yet) I think the 'step' is the chunk size and the strength is 16 bytes: /* In BCH mode */ ecc->size = chunk->size; ecc->strength = 16; And maybe I could try to expand some more the ECC description in the Documentation/mtd/nand/pxa3xx-nand.txt.
Hi Ezequiel, There's one question of yours that I think hasn't been addressed: On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote: > Having followed (part) of the OMAP DT ECC discussion, I'm thinking: > wouldn't it be good to have an 'nand-ecc-strength' property in the DT? > > It would match the ecc_strength_ds field. This way, the ECC mode > selection is left to the driver and not to the user. > On the other side, this can cause some severe incompatibilities > unless such driver *always* make the *same* selection. I'm not quite so sure about the whole question of ECC in device tree. There seems to be 2 subtly different properties we might want to capture: 1) What is the minimum ECC required for the flash? 2) What is the exact ECC layout/strength/type used for this flash? (i.e., what is the bootloader using?) The first is quite generic: the property consists of a stength and step size. (But this is also duplicated in ONFI, and our full-ID device table.) Still, I think a property for #1 could be useful, for those chips that are not discoverable. And if we (you?) add it, it should be done at the nand_base level, I think, so its binding is shared by all drivers. I'm not quite sure how we identify the appropriate struct device_node for nand_base to use, though. Maybe we should force drivers to initialize a new mtd_info.of_node field? And then maybe this could also be integrated into the 'ofpart' parser, which currently requires drivers to pass a device_node via the mtd_part_parser_data struct? Property #2 is very driver/hardware specific, and it may not be easy to capture this information properly using the same set of properties for all NAND drivers. For example, "BCH-8" is not the same on all systems; and even on the same system a software BCH-X could potentially be very different than (and incompatible with) a BCH-X as provided by hardware. And different hardware provides wildly different choices regarding ECC, so I'm not convinced that we could create a good generic binding for describing #2. But I think a property like #2 is necessary for many platforms that need to eliminate the problem that you mention, where drivers must always make the same selection. Essentially, we're assuming bootloader/driver co-design, rather than properly communicating this information via a shared data structure like device tree. Now, it's another question as to whether we need a property for both #1 and #2. The latter would probably just override the former, but that doesn't mean that the former is unnecessary... Anyway, I'll stop having a conversation with myself now. If you think something should be done, I'm willing to listen and help integrate a solution. Brian
On Mon, Nov 18, 2013 at 10:10:09AM -0800, Brian Norris wrote: > Hi Ezequiel, > > There's one question of yours that I think hasn't been addressed: > > On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote: > > Having followed (part) of the OMAP DT ECC discussion, I'm thinking: > > wouldn't it be good to have an 'nand-ecc-strength' property in the DT? > > > > It would match the ecc_strength_ds field. This way, the ECC mode > > selection is left to the driver and not to the user. > > On the other side, this can cause some severe incompatibilities > > unless such driver *always* make the *same* selection. > > I'm not quite so sure about the whole question of ECC in device tree. > There seems to be 2 subtly different properties we might want to > capture: > > 1) What is the minimum ECC required for the flash? > > 2) What is the exact ECC layout/strength/type used for this flash? > (i.e., what is the bootloader using?) > > The first is quite generic: the property consists of a stength and step > size. (But this is also duplicated in ONFI, and our full-ID device > table.) > > Still, I think a property for #1 could be useful, for those chips that > are not discoverable. And if we (you?) add it, it should be done at the > nand_base level, I think, so its binding is shared by all drivers. > > I'm not quite sure how we identify the appropriate struct device_node > for nand_base to use, though. Maybe we should force drivers to > initialize a new mtd_info.of_node field? And then maybe this could also > be integrated into the 'ofpart' parser, which currently requires drivers > to pass a device_node via the mtd_part_parser_data struct? > > Property #2 is very driver/hardware specific, and it may not be easy to > capture this information properly using the same set of properties for > all NAND drivers. For example, "BCH-8" is not the same on all systems; > and even on the same system a software BCH-X could potentially be very > different than (and incompatible with) a BCH-X as provided by hardware. > And different hardware provides wildly different choices regarding ECC, > so I'm not convinced that we could create a good generic binding for > describing #2. > > But I think a property like #2 is necessary for many platforms that need > to eliminate the problem that you mention, where drivers must always > make the same selection. Essentially, we're assuming bootloader/driver > co-design, rather than properly communicating this information via a > shared data structure like device tree. > > Now, it's another question as to whether we need a property for both #1 > and #2. The latter would probably just override the former, but that > doesn't mean that the former is unnecessary... > I completely agree with all of the above, but I'm still a bit uncertain about how useful implementing #1 would be. As you say, encoding the specific (per-driver) ECC information in the devicetree seems the safest way of dealing with that. On the other side, I fail to clearly see a valid use case for reporting the "minimum" ECC required strength in the devicetree. If I'm not missing anything, then I'd say just implement #2, for each driver that needs it. I know that pxa3xx-nand should have it to avoid future issues. This item is on top of my NAND TODO list. Thanks for following the discussion!
Hi Ezequiel, On Mon, Nov 18, 2013 at 03:33:26PM -0300, Ezequiel Garcia wrote: > On Mon, Nov 18, 2013 at 10:10:09AM -0800, Brian Norris wrote: > > There's one question of yours that I think hasn't been addressed: > > > > On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote: > > > Having followed (part) of the OMAP DT ECC discussion, I'm thinking: > > > wouldn't it be good to have an 'nand-ecc-strength' property in the DT? > > > > > > It would match the ecc_strength_ds field. This way, the ECC mode > > > selection is left to the driver and not to the user. > > > On the other side, this can cause some severe incompatibilities > > > unless such driver *always* make the *same* selection. > > > > I'm not quite so sure about the whole question of ECC in device tree. > > There seems to be 2 subtly different properties we might want to > > capture: > > > > 1) What is the minimum ECC required for the flash? > > > > 2) What is the exact ECC layout/strength/type used for this flash? > > (i.e., what is the bootloader using?) > > > > The first is quite generic: the property consists of a stength and step > > size. (But this is also duplicated in ONFI, and our full-ID device > > table.) > > > > Still, I think a property for #1 could be useful, for those chips that > > are not discoverable. And if we (you?) add it, it should be done at the > > nand_base level, I think, so its binding is shared by all drivers. > > > > I'm not quite sure how we identify the appropriate struct device_node > > for nand_base to use, though. Maybe we should force drivers to > > initialize a new mtd_info.of_node field? And then maybe this could also > > be integrated into the 'ofpart' parser, which currently requires drivers > > to pass a device_node via the mtd_part_parser_data struct? > > > > Property #2 is very driver/hardware specific, and it may not be easy to > > capture this information properly using the same set of properties for > > all NAND drivers. For example, "BCH-8" is not the same on all systems; > > and even on the same system a software BCH-X could potentially be very > > different than (and incompatible with) a BCH-X as provided by hardware. > > And different hardware provides wildly different choices regarding ECC, > > so I'm not convinced that we could create a good generic binding for > > describing #2. > > > > But I think a property like #2 is necessary for many platforms that need > > to eliminate the problem that you mention, where drivers must always > > make the same selection. Essentially, we're assuming bootloader/driver > > co-design, rather than properly communicating this information via a > > shared data structure like device tree. > > > > Now, it's another question as to whether we need a property for both #1 > > and #2. The latter would probably just override the former, but that > > doesn't mean that the former is unnecessary... > > > > I completely agree with all of the above, but I'm still a bit uncertain > about how useful implementing #1 would be. > > As you say, encoding the specific (per-driver) ECC information in the > devicetree seems the safest way of dealing with that. > > On the other side, I fail to clearly see a valid use case for reporting > the "minimum" ECC required strength in the devicetree. > > If I'm not missing anything, then I'd say just implement #2, for each driver > that needs it. I know that pxa3xx-nand should have it to avoid future issues. > This item is on top of my NAND TODO list. Well, Huang kinda dodged #2 for now in GPMI NAND, by hacking in a 'fsl,use-minimum-ecc' property. That potentially yields less work for the bootloader; it just defers the "minimum ECC" choice to Linux. This is certainly less specific, and it leaves the door open just a bit for Linux to inadvertently make incompatible changes... But the more I think about it, the more I think that #2 is really the right way. And yes, I think pxa3xx-nand should do #2. Huang, Do you have a good reason why you needed GPMI NAND to choose the ECC configuration (a la "miminum ECC") instead of fully specifying your ECC selection in device tree? I recall most of your arguments were about using an ECC strength that leaves room for user data in OOB (e.g., with JFFS2). But you could have done the same thing by creating a proper DT property to describe the desidered ECC strength, with no real disadvantage, right? Brian
Hi Huang, I'd like to follow up on this question, since you didn't answer it, and it's still relevant, since we haven't yet merged your GPMI DT binding (it's queued for the next merge window): On Mon, Nov 18, 2013 at 10:50:59AM -0800, Brian Norris wrote: > Do you have a good reason why you needed GPMI NAND to choose the ECC > configuration (a la "miminum ECC") instead of fully specifying your ECC > selection in device tree? I recall most of your arguments were about > using an ECC strength that leaves room for user data in OOB (e.g., with > JFFS2). But you could have done the same thing by creating a proper DT > property to describe the desidered ECC strength, with no real > disadvantage, right? I'll rephrase it: why can't/don't you define a GPMI binding for the actual ECC level, like: fsl,nand-ecc-strength and fsl,nand-ecc-sector ? Then, you could still default to the old geometry if these properties aren't present, and you don't have to rely on Linux auto-detecting ECC properties for non-ONFI chips. (Side note: I see you submitted patches for Hynix MLC ECC auto-detection; I'm not sure this approach is really scalable, but it may be OK for some chips. Anyway, I'll address these patches separately once I get to them.) Brian
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 9b42832..b9acd66 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -7,6 +7,20 @@ * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. + * + * Large pages implementation + * -------------------------- + * + * NAND stack expects data in the following format: + * --------------------------- + * | Data (4K) | Spare | ECC | + * --------------------------- + * + * While NAND controller expects data to be in the following format: + * ----------------------------------------------------- + * | Data (2K) | Spare | ECC | Data (2K) | Spare | ECC | + * ----------------------------------------------------- + * */ #include <linux/kernel.h> @@ -101,6 +115,8 @@ #define NDCB0_ST_ROW_EN (0x1 << 26) #define NDCB0_AUTO_RS (0x1 << 25) #define NDCB0_CSEL (0x1 << 24) +#define NDCB0_EXT_CMD_TYPE_MASK (0x7 << 29) +#define NDCB0_EXT_CMD_TYPE(x) (((x) << 29) & NDCB0_EXT_CMD_TYPE_MASK) #define NDCB0_CMD_TYPE_MASK (0x7 << 21) #define NDCB0_CMD_TYPE(x) (((x) << 21) & NDCB0_CMD_TYPE_MASK) #define NDCB0_NC (0x1 << 20) @@ -111,6 +127,14 @@ #define NDCB0_CMD1_MASK (0xff) #define NDCB0_ADDR_CYC_SHIFT (16) +#define EXT_CMD_TYPE_DISPATCH 6 /* Command dispatch */ +#define EXT_CMD_TYPE_NAKED_RW 5 /* Naked read or Naked write */ +#define EXT_CMD_TYPE_READ 4 /* Read */ +#define EXT_CMD_TYPE_DISP_WR 4 /* Command dispatch with write */ +#define EXT_CMD_TYPE_FINAL 3 /* Final command */ +#define EXT_CMD_TYPE_LAST_RW 1 /* Last naked read/write */ +#define EXT_CMD_TYPE_MONO 0 /* Monolithic read/write */ + /* macros for registers read/write */ #define nand_writel(info, off, val) \ __raw_writel((val), (info)->mmio_base + (off)) @@ -212,6 +236,7 @@ struct pxa3xx_nand_info { unsigned int fifo_size; /* max. data size in the FIFO */ unsigned int data_size; /* data to be read from FIFO */ + unsigned int chunk_size; /* split commands chunk size */ unsigned int oob_size; unsigned int spare_size; unsigned int ecc_size; @@ -275,6 +300,31 @@ static struct nand_bbt_descr bbt_mirror_descr = { .pattern = bbt_mirror_pattern }; +static struct nand_ecclayout ecc_layout_4KB_bch4bit = { + .eccbytes = 64, + .eccpos = { + 32, 33, 34, 35, 36, 37, 38, 39, + 40, 41, 42, 43, 44, 45, 46, 47, + 48, 49, 50, 51, 52, 53, 54, 55, + 56, 57, 58, 59, 60, 61, 62, 63, + 96, 97, 98, 99, 100, 101, 102, 103, + 104, 105, 106, 107, 108, 109, 110, 111, + 112, 113, 114, 115, 116, 117, 118, 119, + 120, 121, 122, 123, 124, 125, 126, 127}, + /* Bootrom looks in bytes 0 & 5 for bad blocks */ + .oobfree = { {6, 26}, { 64, 32} } +}; + +static struct nand_ecclayout ecc_layout_4KB_bch8bit = { + .eccbytes = 128, + .eccpos = { + 32, 33, 34, 35, 36, 37, 38, 39, + 40, 41, 42, 43, 44, 45, 46, 47, + 48, 49, 50, 51, 52, 53, 54, 55, + 56, 57, 58, 59, 60, 61, 62, 63}, + .oobfree = { } +}; + /* Define a default flash type setting serve as flash detecting only */ #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0]) @@ -437,7 +487,7 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask) static void handle_data_pio(struct pxa3xx_nand_info *info) { - unsigned int do_bytes = min(info->data_size, info->fifo_size); + unsigned int do_bytes = min(info->data_size, info->chunk_size); switch (info->state) { case STATE_PIO_WRITING: @@ -675,7 +725,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command) } static int prepare_set_command(struct pxa3xx_nand_info *info, int command, - uint16_t column, int page_addr) + int ext_cmd_type, uint16_t column, int page_addr) { int addr_cycle, exec_cmd; struct pxa3xx_nand_host *host; @@ -708,9 +758,20 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command, if (command == NAND_CMD_READOOB) info->buf_start += mtd->writesize; - /* Second command setting for large pages */ - if (mtd->writesize >= PAGE_CHUNK_SIZE) + /* + * Multiple page read needs an 'extended command type' field, + * which is either naked-read or last-read according to the + * state. + */ + if (mtd->writesize == PAGE_CHUNK_SIZE) { info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8); + } else if (mtd->writesize > PAGE_CHUNK_SIZE) { + info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8) + | NDCB0_LEN_OVRD + | NDCB0_EXT_CMD_TYPE(ext_cmd_type); + info->ndcb3 = info->chunk_size + + info->oob_size; + } set_command_address(info, mtd->writesize, column, page_addr); break; @@ -826,7 +887,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, prepare_start_command(info, command); info->state = STATE_PREPARED; - exec_cmd = prepare_set_command(info, command, column, page_addr); + exec_cmd = prepare_set_command(info, command, -1, column, page_addr); + if (exec_cmd) { init_completion(&info->cmd_complete); init_completion(&info->dev_ready); @@ -844,6 +906,91 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, info->state = STATE_IDLE; } +static void armada370_nand_cmdfunc(struct mtd_info *mtd, + const unsigned command, + int column, int page_addr) +{ + struct pxa3xx_nand_host *host = mtd->priv; + struct pxa3xx_nand_info *info = host->info_data; + int ret, exec_cmd, ext_cmd_type; + + /* + * if this is a x16 device ,then convert the input + * "byte" address into a "word" address appropriate + * for indexing a word-oriented device + */ + if (info->reg_ndcr & NDCR_DWIDTH_M) + column /= 2; + + /* + * There may be different NAND chip hooked to + * different chip select, so check whether + * chip select has been changed, if yes, reset the timing + */ + if (info->cs != host->cs) { + info->cs = host->cs; + nand_writel(info, NDTR0CS0, info->ndtr0cs0); + nand_writel(info, NDTR1CS0, info->ndtr1cs0); + } + + /* Select the extended command for the first command */ + switch (command) { + case NAND_CMD_READ0: + case NAND_CMD_READOOB: + ext_cmd_type = EXT_CMD_TYPE_MONO; + break; + } + + prepare_start_command(info, command); + + /* + * Prepare the "is ready" completion before starting a command + * transaction sequence. If the command is not executed the + * completion will be completed, see below. + * + * We can do that inside the loop because the command variable + * is invariant and thus so is the exec_cmd. + */ + atomic_set(&info->is_ready, 0); + init_completion(&info->dev_ready); + do { + info->state = STATE_PREPARED; + exec_cmd = prepare_set_command(info, command, ext_cmd_type, + column, page_addr); + if (!exec_cmd) { + atomic_set(&info->is_ready, 1); + complete(&info->dev_ready); + break; + } + + init_completion(&info->cmd_complete); + pxa3xx_nand_start(info); + + ret = wait_for_completion_timeout(&info->cmd_complete, + CHIP_DELAY_TIMEOUT); + if (!ret) { + dev_err(&info->pdev->dev, "Wait time out!!!\n"); + /* Stop State Machine for next command cycle */ + pxa3xx_nand_stop(info); + break; + } + + /* Check if the sequence is complete */ + if (info->data_size == 0) + break; + + if (command == NAND_CMD_READ0 || command == NAND_CMD_READOOB) { + /* Last read: issue a 'last naked read' */ + if (info->data_size == info->chunk_size) + ext_cmd_type = EXT_CMD_TYPE_LAST_RW; + else + ext_cmd_type = EXT_CMD_TYPE_NAKED_RW; + } + } while (1); + + info->state = STATE_IDLE; +} + static int pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { @@ -1029,6 +1176,8 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info) host->read_id_bytes = 2; } + /* Set an initial chunk size */ + info->chunk_size = info->fifo_size; info->reg_ndcr = ndcr & ~NDCR_INT_MASK; info->ndtr0cs0 = nand_readl(info, NDTR0CS0); info->ndtr1cs0 = nand_readl(info, NDTR1CS0); @@ -1135,6 +1284,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info, ecc->size = 512; ecc->strength = 1; + info->chunk_size = 2048; info->spare_size = 40; info->ecc_size = 24; return 1; @@ -1144,6 +1294,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info, ecc->size = 512; ecc->strength = 1; + info->chunk_size = 512; info->spare_size = 8; info->ecc_size = 8; return 1; @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info, struct nand_ecc_ctrl *ecc, int strength, int page_size) { - /* Unimplemented yet */ + if (strength == 4 && page_size == 4096) { + ecc->mode = NAND_ECC_HW; + ecc->size = 512; + ecc->layout = &ecc_layout_4KB_bch4bit; + ecc->strength = 4; + + info->ecc_bch = 1; + info->chunk_size = 2048; + info->spare_size = 32; + info->ecc_size = 32; + return 1; + + } else if (strength == 8 && page_size == 4096) { + ecc->mode = NAND_ECC_HW; + ecc->size = 512; + ecc->layout = &ecc_layout_4KB_bch8bit; + ecc->strength = 8; + + info->ecc_bch = 1; + info->chunk_size = 1024; + info->spare_size = 0; + info->ecc_size = 32; + return 1; + } return 0; } @@ -1319,12 +1493,16 @@ static int alloc_nand_resource(struct platform_device *pdev) chip->controller = &info->controller; chip->waitfunc = pxa3xx_nand_waitfunc; chip->select_chip = pxa3xx_nand_select_chip; - chip->cmdfunc = pxa3xx_nand_cmdfunc; chip->read_word = pxa3xx_nand_read_word; chip->read_byte = pxa3xx_nand_read_byte; chip->read_buf = pxa3xx_nand_read_buf; chip->write_buf = pxa3xx_nand_write_buf; chip->options |= NAND_NO_SUBPAGE_WRITE; + + if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370) + chip->cmdfunc = armada370_nand_cmdfunc; + else + chip->cmdfunc = pxa3xx_nand_cmdfunc; } spin_lock_init(&chip->controller->lock);
As preparation work to fully support large pages, this commit adds the initial infrastructure to support splitted (aka chunked) I/O operation. This commit adds support for read, and follow-up patches will add write support. When a read (aka READ0) command is issued, the driver loops issuing the same command until all the requested data is transfered, changing the 'extended' command field as needed. For instance, if the driver is required to read a 4 KiB page, using a chunk size of 2 KiB, the transaction is splitted in: 1. Monolithic read, first 2 KiB page chunk is read 2. Last naked read, second and last 2KiB page chunk is read If ECC is enabled it is calculated on each chunk transfered and added at a controller-fixed location after the data chunk that must be spare area. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- drivers/mtd/nand/pxa3xx_nand.c | 192 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 185 insertions(+), 7 deletions(-)