| Message ID | 20131130060428.GA29397@norris.computersforpeace.net |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > Hi Ezequiel, > > On Fri, Nov 29, 2013 at 09:20:19AM -0300, Ezequiel Garcia wrote: > > On Tue, Nov 26, 2013 at 11:35:12PM -0800, Brian Norris wrote: > > > + Pekon, Ezequiel > > > > > > Can one of you see how this patch works with your BeagleBones w/ x16 > > > NAND? > > I see that you are pushing to straighten out the auto-buswidth part of > nand_base, and I think there may be good reasons to do so. But I think > that part of your problem can be resolved by a patch like Uwe's, where > rather than forcing the entire driver to be configured for x8 just to > use ONFI, we can fix the ONFI operations to use the lower 8 bits. > > IOW, I expect that a patch like Uwe's can shed some better light on the > auto-buswidh situation. (This is why I CC'd you and Pekon.) > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > don't think. It looks like it needs something like the following diff > (only compile-tested). > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index bd39f7b67906..1ab264457d94 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > int *busw) > { > struct nand_onfi_params *p = &chip->onfi_params; > - int i; > + int i, j; > int val; > > /* Try ONFI for unknown chip or LP */ > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > return 0; > > - /* > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > - * with NAND_BUSWIDTH_16 > - */ > - if (chip->options & NAND_BUSWIDTH_16) { > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > - return 0; > - } > - > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > for (i = 0; i < 3; i++) { > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > + for (j = 0; j < sizeof(*p); j++) > + *(uint8_t *)p = chip->read_byte(mtd); > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > le16_to_cpu(p->crc)) { > break; > > What do you think? (And more importantly, how does this test out for > you?) > Let me try this out and let you know.
On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > don't think. It looks like it needs something like the following diff > (only compile-tested). > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index bd39f7b67906..1ab264457d94 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > int *busw) > { > struct nand_onfi_params *p = &chip->onfi_params; > - int i; > + int i, j; > int val; > > /* Try ONFI for unknown chip or LP */ > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > return 0; > > - /* > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > - * with NAND_BUSWIDTH_16 > - */ > - if (chip->options & NAND_BUSWIDTH_16) { > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > - return 0; > - } > - > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > for (i = 0; i < 3; i++) { > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > + for (j = 0; j < sizeof(*p); j++) > + *(uint8_t *)p = chip->read_byte(mtd); > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > le16_to_cpu(p->crc)) { > break; > > What do you think? (And more importantly, how does this test out for > you?) > Your proposal would work (fixing a minor typo for incrementing 'p'), except the nand_command() implementation messes with the buswith. Therefore, after a long debugging session, I could make it work using this hack: @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, /* Serially input address */ if (column != -1) { /* Adjust columns for 16 bit buswidth */ - if (chip->options & NAND_BUSWIDTH_16) - column >>= 1; +// if (chip->options & NAND_BUSWIDTH_16) +// column >>= 1; chip->cmd_ctrl(mtd, column, ctrl); ctrl &= ~NAND_CTRL_CHANGE; } Now, IMHO, the solution of setting the defaults to x8 during the device detection phase, is far simpler. BTW: this is not really related to Uwe's patch, right? It's just a complement to his work, as far as I can see.
On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote: > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > > don't think. It looks like it needs something like the following diff > > (only compile-tested). > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index bd39f7b67906..1ab264457d94 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > int *busw) > > { > > struct nand_onfi_params *p = &chip->onfi_params; > > - int i; > > + int i, j; > > int val; > > > > /* Try ONFI for unknown chip or LP */ > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > > return 0; > > > > - /* > > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > > - * with NAND_BUSWIDTH_16 > > - */ > > - if (chip->options & NAND_BUSWIDTH_16) { > > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > > - return 0; > > - } > > - > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > > for (i = 0; i < 3; i++) { > > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > > + for (j = 0; j < sizeof(*p); j++) > > + *(uint8_t *)p = chip->read_byte(mtd); > > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > > le16_to_cpu(p->crc)) { > > break; > > > > What do you think? (And more importantly, how does this test out for > > you?) > > > > Your proposal would work (fixing a minor typo for incrementing 'p'), > except the nand_command() implementation messes with the buswith. > Therefore, after a long debugging session, I could make it work using > this hack: > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, > /* Serially input address */ > if (column != -1) { > /* Adjust columns for 16 bit buswidth */ > - if (chip->options & NAND_BUSWIDTH_16) > - column >>= 1; > +// if (chip->options & NAND_BUSWIDTH_16) > +// column >>= 1; > chip->cmd_ctrl(mtd, column, ctrl); > ctrl &= ~NAND_CTRL_CHANGE; > } > > Now, IMHO, the solution of setting the defaults to x8 during the device > detection phase, is far simpler. > And after some more debugging, I now realise the above problem is also present at my previous proposal. So it seems we would need to actually temporary "deactivate" NAND_BUSWIDTH_16 from chip->options. I wonder how ugly that could be. Comments? In any case, it seems our proposals are equivalent: * we can change the defaults to x8 (is this at all needed?) * we can use read_byte But, again, we'll need to unset NAND_BUSWIDTH_16 from chip->option. Now, it would be a bit confusing to "trick" NAND_BUSWIDTH_16 from options without also setting the defaults to x8 for the detection phase. Comments?
On Sat, Nov 30, 2013 at 01:35:36PM -0300, Ezequiel Garcia wrote: > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > > don't think. It looks like it needs something like the following diff > > (only compile-tested). > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index bd39f7b67906..1ab264457d94 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > int *busw) > > { > > struct nand_onfi_params *p = &chip->onfi_params; > > - int i; > > + int i, j; > > int val; > > > > /* Try ONFI for unknown chip or LP */ > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > > return 0; > > > > - /* > > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > > - * with NAND_BUSWIDTH_16 > > - */ > > - if (chip->options & NAND_BUSWIDTH_16) { > > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > > - return 0; > > - } > > - > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > > for (i = 0; i < 3; i++) { > > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > > + for (j = 0; j < sizeof(*p); j++) > > + *(uint8_t *)p = chip->read_byte(mtd); > > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > > le16_to_cpu(p->crc)) { > > break; > > > > What do you think? (And more importantly, how does this test out for > > you?) > > > > Your proposal would work (fixing a minor typo for incrementing 'p'), > except the nand_command() implementation messes with the buswith. > Therefore, after a long debugging session, I could make it work using > this hack: > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, > /* Serially input address */ > if (column != -1) { > /* Adjust columns for 16 bit buswidth */ > - if (chip->options & NAND_BUSWIDTH_16) > - column >>= 1; > +// if (chip->options & NAND_BUSWIDTH_16) > +// column >>= 1; > chip->cmd_ctrl(mtd, column, ctrl); > ctrl &= ~NAND_CTRL_CHANGE; > } Hmm, that does seem like a hack. What command is giving you problems, by the way? And is the NAND operational after this hack? It looks like those two lines are there so that we can always specify 'column' in bytes, and nand_command() will do the byte/word translation automatically. But we don't want this for certain commands, I think, so maybe a "hack" is necessary... > Now, IMHO, the solution of setting the defaults to x8 during the device > detection phase, is far simpler. > > BTW: this is not really related to Uwe's patch, right? It's just a complement > to his work, as far as I can see. Yeah, it's a complement. At first, I though Uwe's patch included this, but I was wrong. Brian
On Sat, Nov 30, 2013 at 01:51:23PM -0300, Ezequiel Garcia wrote: > On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote: > > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > > > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > > > don't think. It looks like it needs something like the following diff > > > (only compile-tested). > > > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > index bd39f7b67906..1ab264457d94 100644 > > > --- a/drivers/mtd/nand/nand_base.c > > > +++ b/drivers/mtd/nand/nand_base.c > > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > > int *busw) > > > { > > > struct nand_onfi_params *p = &chip->onfi_params; > > > - int i; > > > + int i, j; > > > int val; > > > > > > /* Try ONFI for unknown chip or LP */ > > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > > > return 0; > > > > > > - /* > > > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > > > - * with NAND_BUSWIDTH_16 > > > - */ > > > - if (chip->options & NAND_BUSWIDTH_16) { > > > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > > > - return 0; > > > - } > > > - > > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > > > for (i = 0; i < 3; i++) { > > > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > > > + for (j = 0; j < sizeof(*p); j++) > > > + *(uint8_t *)p = chip->read_byte(mtd); > > > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > > > le16_to_cpu(p->crc)) { > > > break; > > > > > > What do you think? (And more importantly, how does this test out for > > > you?) > > > > > > > Your proposal would work (fixing a minor typo for incrementing 'p'), > > except the nand_command() implementation messes with the buswith. > > Therefore, after a long debugging session, I could make it work using > > this hack: > > > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, > > /* Serially input address */ > > if (column != -1) { > > /* Adjust columns for 16 bit buswidth */ > > - if (chip->options & NAND_BUSWIDTH_16) > > - column >>= 1; > > +// if (chip->options & NAND_BUSWIDTH_16) > > +// column >>= 1; > > chip->cmd_ctrl(mtd, column, ctrl); > > ctrl &= ~NAND_CTRL_CHANGE; > > } > > > > Now, IMHO, the solution of setting the defaults to x8 during the device > > detection phase, is far simpler. > > > > And after some more debugging, I now realise the above problem is also > present at my previous proposal. So it seems we would need to actually > temporary "deactivate" NAND_BUSWIDTH_16 from chip->options. > > I wonder how ugly that could be. Comments? I'm not sure yet. I'd like to better understand what command is failing, and why. > In any case, it seems our proposals are equivalent: > * we can change the defaults to x8 (is this at all needed?) > * we can use read_byte No, our proposals are not equivalent. Your patches are only solving these ONFI bus-width problems during initialization. I believe we will want to use some ONFI routines (SET FEATURES, especially) after initialization. This is where the rest of Uwe's patch comes into play. So I don't think we can always switch between call-backs and play games with NAND_BUSWIDTH_16; we should get the bus width right as soon as possible, and then make sure that the callbacks always work as expected. You are also placing more burden on drivers. You require the drivers to add failure logic if the NAND core auto-configures a buswidth that the host doesn't support. I prefer that for cases where the bus width is known a-priori, the driver only needs to call nand_scan(), and the NAND core can error out appropriately. > But, again, we'll need to unset NAND_BUSWIDTH_16 from chip->option. > > Now, it would be a bit confusing to "trick" NAND_BUSWIDTH_16 from > options without also setting the defaults to x8 for the detection phase. > > Comments? Brian
On Sat, Nov 30, 2013 at 10:53:08AM -0800, Brian Norris wrote: > On Sat, Nov 30, 2013 at 01:35:36PM -0300, Ezequiel Garcia wrote: > > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > > > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > > > don't think. It looks like it needs something like the following diff > > > (only compile-tested). > > > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > index bd39f7b67906..1ab264457d94 100644 > > > --- a/drivers/mtd/nand/nand_base.c > > > +++ b/drivers/mtd/nand/nand_base.c > > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > > int *busw) > > > { > > > struct nand_onfi_params *p = &chip->onfi_params; > > > - int i; > > > + int i, j; > > > int val; > > > > > > /* Try ONFI for unknown chip or LP */ > > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > > > return 0; > > > > > > - /* > > > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > > > - * with NAND_BUSWIDTH_16 > > > - */ > > > - if (chip->options & NAND_BUSWIDTH_16) { > > > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > > > - return 0; > > > - } > > > - > > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > > > for (i = 0; i < 3; i++) { > > > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > > > + for (j = 0; j < sizeof(*p); j++) > > > + *(uint8_t *)p = chip->read_byte(mtd); > > > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > > > le16_to_cpu(p->crc)) { > > > break; > > > > > > What do you think? (And more importantly, how does this test out for > > > you?) > > > > > > > Your proposal would work (fixing a minor typo for incrementing 'p'), > > except the nand_command() implementation messes with the buswith. > > Therefore, after a long debugging session, I could make it work using > > this hack: > > > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, > > /* Serially input address */ > > if (column != -1) { > > /* Adjust columns for 16 bit buswidth */ > > - if (chip->options & NAND_BUSWIDTH_16) > > - column >>= 1; > > +// if (chip->options & NAND_BUSWIDTH_16) > > +// column >>= 1; > > chip->cmd_ctrl(mtd, column, ctrl); > > ctrl &= ~NAND_CTRL_CHANGE; > > } > > Hmm, that does seem like a hack. What command is giving you problems, by > the way? READ_ID > And is the NAND operational after this hack? > Yup, I guess so. It takes some time to prepare the hardware to test that, so I'll be to re-test it properly to provide some more complete answer.
On Sat, Nov 30, 2013 at 11:01:49AM -0800, Brian Norris wrote: > On Sat, Nov 30, 2013 at 01:51:23PM -0300, Ezequiel Garcia wrote: > > On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote: > > > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > > > > > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > > > > don't think. It looks like it needs something like the following diff > > > > (only compile-tested). > > > > > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > > index bd39f7b67906..1ab264457d94 100644 > > > > --- a/drivers/mtd/nand/nand_base.c > > > > +++ b/drivers/mtd/nand/nand_base.c > > > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > > > int *busw) > > > > { > > > > struct nand_onfi_params *p = &chip->onfi_params; > > > > - int i; > > > > + int i, j; > > > > int val; > > > > > > > > /* Try ONFI for unknown chip or LP */ > > > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > > > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > > > > return 0; > > > > > > > > - /* > > > > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > > > > - * with NAND_BUSWIDTH_16 > > > > - */ > > > > - if (chip->options & NAND_BUSWIDTH_16) { > > > > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > > > > - return 0; > > > > - } > > > > - > > > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > > > > for (i = 0; i < 3; i++) { > > > > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > > > > + for (j = 0; j < sizeof(*p); j++) > > > > + *(uint8_t *)p = chip->read_byte(mtd); > > > > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > > > > le16_to_cpu(p->crc)) { > > > > break; > > > > > > > > What do you think? (And more importantly, how does this test out for > > > > you?) > > > > > > > > > > Your proposal would work (fixing a minor typo for incrementing 'p'), > > > except the nand_command() implementation messes with the buswith. > > > Therefore, after a long debugging session, I could make it work using > > > this hack: > > > > > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, > > > /* Serially input address */ > > > if (column != -1) { > > > /* Adjust columns for 16 bit buswidth */ > > > - if (chip->options & NAND_BUSWIDTH_16) > > > - column >>= 1; > > > +// if (chip->options & NAND_BUSWIDTH_16) > > > +// column >>= 1; > > > chip->cmd_ctrl(mtd, column, ctrl); > > > ctrl &= ~NAND_CTRL_CHANGE; > > > } > > > > > > Now, IMHO, the solution of setting the defaults to x8 during the device > > > detection phase, is far simpler. > > > > > > > And after some more debugging, I now realise the above problem is also > > present at my previous proposal. So it seems we would need to actually > > temporary "deactivate" NAND_BUSWIDTH_16 from chip->options. > > > > I wonder how ugly that could be. Comments? > > I'm not sure yet. I'd like to better understand what command is failing, > and why. > > > In any case, it seems our proposals are equivalent: > > * we can change the defaults to x8 (is this at all needed?) > > * we can use read_byte > > No, our proposals are not equivalent. > > Your patches are only solving these ONFI bus-width problems during > initialization. Agreed. > I believe we will want to use some ONFI routines (SET > FEATURES, especially) after initialization. This is where the rest of > Uwe's patch comes into play. So I don't think we can always switch > between call-backs and play games with NAND_BUSWIDTH_16; we should get > the bus width right as soon as possible, and then make sure that the > callbacks always work as expected. > Sure, I completely understand the above. The patches I've been pushing are meant *only* to solve the initial device detection, and in particular to fix the currently broken ONFI detection. I realise that Uwe's patches (and from what I've been seeing some more) are needed in other to solve other ONFI width-related problems. Just to clarify, the v2 I just sent was motivated by this rationale: if we need to temporarily switch off NAND_BUSWIDTH_16, then it makes sense to also switch the entire default functions. However, if you can find a cleaner (i.e. ot too hacky) solution, so we can prevent this width switching, I'd be happy to test it! > You are also placing more burden on drivers. You require the drivers to > add failure logic if the NAND core auto-configures a buswidth that the > host doesn't support. I prefer that for cases where the bus width is > known a-priori, the driver only needs to call nand_scan(), and the NAND > core can error out appropriately. > Yup, that's correct. Under my solution driver's _must_ handle a two phase initialization: nand_scan_ident() + nand_scan_tail(); but only if they need some special tweaking after the bus width discovering. I admit my proposal might be narrow-minded, and biased by the only few NAND drivers I'm familiar to :(
>From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com] >> > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, >> > > /* Serially input address */ >> > > if (column != -1) { >> > > /* Adjust columns for 16 bit buswidth */ >> > > - if (chip->options & NAND_BUSWIDTH_16) >> > > - column >>= 1; >> > > +// if (chip->options & NAND_BUSWIDTH_16) >> > > +// column >>= 1; >> > > chip->cmd_ctrl(mtd, column, ctrl); >> > > ctrl &= ~NAND_CTRL_CHANGE; >> > > } >> > > This is a very good finding .. The ONFI spec says that data would be present only on lower 8-bits of NAND data-bus. This mean we need to always issue 1-byte aligned address. The above fix enforces that rule. In absence of this fix, if (chip->options & NAND_BUSWIDTH_16) is set, then nand_command() would issue a word-aligned address even when we use chip->read_byte() to read a single data. To take do this cleanly , we might need a larger fix, that is change in interface function chip->cmdfunc().. diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h - void (*cmdfunc)(struct mtd_info *mtd, unsigned command, int column, - int page_addr); + void (*cmdfunc)(struct mtd_info *mtd, unsigned command, int column, + int page_addr, int busw); And replace above hack with.. @@nand_command() - if (chip->options & NAND_BUSWIDTH_16) - column >>= 1; + if (busw) + column >>= 1; [...] >> I believe we will want to use some ONFI routines (SET >> FEATURES, especially) after initialization. This is where the rest of >> Uwe's patch comes into play. So I don't think we can always switch >> between call-backs and play games with NAND_BUSWIDTH_16; we should get >> the bus width right as soon as possible, and then make sure that the >> callbacks always work as expected. >> > >Sure, I completely understand the above. The patches I've been pushing >are meant *only* to solve the initial device detection, and in >particular to fix the currently broken ONFI detection. > So as per my understanding.. *Uwe's patch* - handles byte-wise access for onfi_set_feature()/onfi_get_feature() *Brian's patch* - handles byte-wise access for nand_flash_detect_onfi() *Ezequiel's patch* - modifies nand_command() to issue byte-aligned accesses. (as above) - re-configuring of chip->options in nand_scan_ident before returning back to callee (controller driver here). If correct, then we should group these pieces together in patch-series (with all contributors signed-off). And then test this on multiple devices. I can do that, if all agree that above pieces are correct and required for completion of functionality ?? with regards, pekon
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index bd39f7b67906..1ab264457d94 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, int *busw) { struct nand_onfi_params *p = &chip->onfi_params; - int i; + int i, j; int val; /* Try ONFI for unknown chip or LP */ @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') return 0; - /* - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not - * with NAND_BUSWIDTH_16 - */ - if (chip->options & NAND_BUSWIDTH_16) { - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); - return 0; - } - chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); for (i = 0; i < 3; i++) { - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); + for (j = 0; j < sizeof(*p); j++) + *(uint8_t *)p = chip->read_byte(mtd); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { break;