| Message ID | 20161020043127.20027-2-judge.packham@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
On 20 October 2016 at 01:31, Chris Packham <judge.packham@gmail.com> wrote: > The initial buffer is used for the initial commands used to detect > a flash device (STATUS, READID and PARAM). > > ONFI param page is 256 bytes, and there are three redundant copies > to be read. JEDEC param page is 512 bytes, and there are also three > redundant copies to be read. Hence this buffer should be at least > 512 x 3. This commits rounds the buffer size to 2048. > Hey Chris, So you are basically picking the commit and commit log from Linux: http://lists.infradead.org/pipermail/linux-mtd/2015-August/060721.html Shouldn't you mention that somewhere? > Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Signed-off-by: Chris Packham <judge.packham@gmail.com> > --- > > drivers/mtd/nand/pxa3xx_nand.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index dfe8966b56b6..ea0a6f3778bd 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -26,10 +26,13 @@ > > /* > * Define a buffer size for the initial command that detects the flash device: > - * STATUS, READID and PARAM. The largest of these is the PARAM command, > - * needing 256 bytes. > + * STATUS, READID and PARAM. > + * ONFI param page is 256 bytes, and there are three redundant copies > + * to be read. JEDEC param page is 512 bytes, and there are also three > + * redundant copies to be read. > + * Hence this buffer should be at least 512 x 3. Let's pick 2048. > */ > -#define INIT_BUFFER_SIZE 256 > +#define INIT_BUFFER_SIZE 2048 > > /* registers and bit definitions */ > #define NDCR (0x00) /* Control register */ > @@ -838,14 +841,14 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command, > break; > > case NAND_CMD_PARAM: > - info->buf_count = 256; > + info->buf_count = INIT_BUFFER_SIZE; > info->ndcb0 |= NDCB0_CMD_TYPE(0) > | NDCB0_ADDR_CYC(1) > | NDCB0_LEN_OVRD > | command; > info->ndcb1 = (column & 0xFF); > - info->ndcb3 = 256; > - info->data_size = 256; > + info->ndcb3 = INIT_BUFFER_SIZE; > + info->data_size = INIT_BUFFER_SIZE; > break; > > case NAND_CMD_READID: > @@ -1468,6 +1471,7 @@ KEEP_CONFIG: > host->row_addr_cycles = 3; > else > host->row_addr_cycles = 2; > + Spurious change. I suggest to drop it. > return nand_scan_tail(mtd); > } > > -- > 2.10.0.479.g7c56b16 >
On Fri, Oct 21, 2016 at 1:58 AM, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > On 20 October 2016 at 01:31, Chris Packham <judge.packham@gmail.com> wrote: >> The initial buffer is used for the initial commands used to detect >> a flash device (STATUS, READID and PARAM). >> >> ONFI param page is 256 bytes, and there are three redundant copies >> to be read. JEDEC param page is 512 bytes, and there are also three >> redundant copies to be read. Hence this buffer should be at least >> 512 x 3. This commits rounds the buffer size to 2048. >> > > Hey Chris, > > So you are basically picking the commit and commit log from Linux: > > http://lists.infradead.org/pipermail/linux-mtd/2015-August/060721.html > > Shouldn't you mention that somewhere? Indeed. I mentioned it here http://lists.denx.de/pipermail/u-boot/2016-October/270605.html and that was the intent of the Cc line below. I should probably set the From: line to the original author and call out the Linux commit sha1. I wasn't sure of the usual u-boot practice, I could equally squash these all together and say "sync with linux". > >> Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> >> Signed-off-by: Chris Packham <judge.packham@gmail.com> >> --- >> >> drivers/mtd/nand/pxa3xx_nand.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c >> index dfe8966b56b6..ea0a6f3778bd 100644 >> --- a/drivers/mtd/nand/pxa3xx_nand.c >> +++ b/drivers/mtd/nand/pxa3xx_nand.c >> @@ -26,10 +26,13 @@ >> >> /* >> * Define a buffer size for the initial command that detects the flash device: >> - * STATUS, READID and PARAM. The largest of these is the PARAM command, >> - * needing 256 bytes. >> + * STATUS, READID and PARAM. >> + * ONFI param page is 256 bytes, and there are three redundant copies >> + * to be read. JEDEC param page is 512 bytes, and there are also three >> + * redundant copies to be read. >> + * Hence this buffer should be at least 512 x 3. Let's pick 2048. >> */ >> -#define INIT_BUFFER_SIZE 256 >> +#define INIT_BUFFER_SIZE 2048 >> >> /* registers and bit definitions */ >> #define NDCR (0x00) /* Control register */ >> @@ -838,14 +841,14 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command, >> break; >> >> case NAND_CMD_PARAM: >> - info->buf_count = 256; >> + info->buf_count = INIT_BUFFER_SIZE; >> info->ndcb0 |= NDCB0_CMD_TYPE(0) >> | NDCB0_ADDR_CYC(1) >> | NDCB0_LEN_OVRD >> | command; >> info->ndcb1 = (column & 0xFF); >> - info->ndcb3 = 256; >> - info->data_size = 256; >> + info->ndcb3 = INIT_BUFFER_SIZE; >> + info->data_size = INIT_BUFFER_SIZE; >> break; >> >> case NAND_CMD_READID: >> @@ -1468,6 +1471,7 @@ KEEP_CONFIG: >> host->row_addr_cycles = 3; >> else >> host->row_addr_cycles = 2; >> + > > Spurious change. I suggest to drop it. > Will do. >> return nand_scan_tail(mtd); >> } >> >> -- >> 2.10.0.479.g7c56b16 >> > > > > -- > Ezequiel García, VanguardiaSur > www.vanguardiasur.com.ar
On 20 October 2016 at 17:14, Chris Packham <judge.packham@gmail.com> wrote: > On Fri, Oct 21, 2016 at 1:58 AM, Ezequiel Garcia > <ezequiel@vanguardiasur.com.ar> wrote: >> On 20 October 2016 at 01:31, Chris Packham <judge.packham@gmail.com> wrote: >>> The initial buffer is used for the initial commands used to detect >>> a flash device (STATUS, READID and PARAM). >>> >>> ONFI param page is 256 bytes, and there are three redundant copies >>> to be read. JEDEC param page is 512 bytes, and there are also three >>> redundant copies to be read. Hence this buffer should be at least >>> 512 x 3. This commits rounds the buffer size to 2048. >>> >> >> Hey Chris, >> >> So you are basically picking the commit and commit log from Linux: >> >> http://lists.infradead.org/pipermail/linux-mtd/2015-August/060721.html >> >> Shouldn't you mention that somewhere? > > Indeed. I mentioned it here > http://lists.denx.de/pipermail/u-boot/2016-October/270605.html and > that was the intent of the Cc line below. > Oh, I see. IMO, that won't do it. See, the mail is going to get lost in the internet sea, but the commit log is going to be set on git stone and pass to generations to come. I'm not really about taking credit, but rather about creating a persistent reference between code you've copy-pasted. This will be useful, e.g. in case it turns out it's wrong, which is perfectly possible. If someone notices it's wrong in U-Boot code, it might git-blame-it and so from there realise the change is needed in Linux too. Just a silly example, but I'm trying to make a point on why it's useful to link copy-pasted implementations. Mentioning in the cover letter is probably great, but don't forget the actual patch serves a different purpose. > I should probably set the From: line to the original author and call > out the Linux commit sha1. I wasn't sure of the usual u-boot practice, > I could equally squash these all together and say "sync with linux". > I'm not sure that matters, as long as you state where is the code coming from. >> >>> Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> >>> Signed-off-by: Chris Packham <judge.packham@gmail.com> >>> --- >>> >>> drivers/mtd/nand/pxa3xx_nand.c | 16 ++++++++++------ >>> 1 file changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c >>> index dfe8966b56b6..ea0a6f3778bd 100644 >>> --- a/drivers/mtd/nand/pxa3xx_nand.c >>> +++ b/drivers/mtd/nand/pxa3xx_nand.c >>> @@ -26,10 +26,13 @@ >>> >>> /* >>> * Define a buffer size for the initial command that detects the flash device: >>> - * STATUS, READID and PARAM. The largest of these is the PARAM command, >>> - * needing 256 bytes. >>> + * STATUS, READID and PARAM. >>> + * ONFI param page is 256 bytes, and there are three redundant copies >>> + * to be read. JEDEC param page is 512 bytes, and there are also three >>> + * redundant copies to be read. >>> + * Hence this buffer should be at least 512 x 3. Let's pick 2048. >>> */ >>> -#define INIT_BUFFER_SIZE 256 >>> +#define INIT_BUFFER_SIZE 2048 >>> >>> /* registers and bit definitions */ >>> #define NDCR (0x00) /* Control register */ >>> @@ -838,14 +841,14 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command, >>> break; >>> >>> case NAND_CMD_PARAM: >>> - info->buf_count = 256; >>> + info->buf_count = INIT_BUFFER_SIZE; >>> info->ndcb0 |= NDCB0_CMD_TYPE(0) >>> | NDCB0_ADDR_CYC(1) >>> | NDCB0_LEN_OVRD >>> | command; >>> info->ndcb1 = (column & 0xFF); >>> - info->ndcb3 = 256; >>> - info->data_size = 256; >>> + info->ndcb3 = INIT_BUFFER_SIZE; >>> + info->data_size = INIT_BUFFER_SIZE; >>> break; >>> >>> case NAND_CMD_READID: >>> @@ -1468,6 +1471,7 @@ KEEP_CONFIG: >>> host->row_addr_cycles = 3; >>> else >>> host->row_addr_cycles = 2; >>> + >> >> Spurious change. I suggest to drop it. >> > > Will do. > >>> return nand_scan_tail(mtd); >>> } >>> >>> -- >>> 2.10.0.479.g7c56b16 >>> >> >> >> >> -- >> Ezequiel García, VanguardiaSur >> www.vanguardiasur.com.ar
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index dfe8966b56b6..ea0a6f3778bd 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -26,10 +26,13 @@ /* * Define a buffer size for the initial command that detects the flash device: - * STATUS, READID and PARAM. The largest of these is the PARAM command, - * needing 256 bytes. + * STATUS, READID and PARAM. + * ONFI param page is 256 bytes, and there are three redundant copies + * to be read. JEDEC param page is 512 bytes, and there are also three + * redundant copies to be read. + * Hence this buffer should be at least 512 x 3. Let's pick 2048. */ -#define INIT_BUFFER_SIZE 256 +#define INIT_BUFFER_SIZE 2048 /* registers and bit definitions */ #define NDCR (0x00) /* Control register */ @@ -838,14 +841,14 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command, break; case NAND_CMD_PARAM: - info->buf_count = 256; + info->buf_count = INIT_BUFFER_SIZE; info->ndcb0 |= NDCB0_CMD_TYPE(0) | NDCB0_ADDR_CYC(1) | NDCB0_LEN_OVRD | command; info->ndcb1 = (column & 0xFF); - info->ndcb3 = 256; - info->data_size = 256; + info->ndcb3 = INIT_BUFFER_SIZE; + info->data_size = INIT_BUFFER_SIZE; break; case NAND_CMD_READID: @@ -1468,6 +1471,7 @@ KEEP_CONFIG: host->row_addr_cycles = 3; else host->row_addr_cycles = 2; + return nand_scan_tail(mtd); }
The initial buffer is used for the initial commands used to detect a flash device (STATUS, READID and PARAM). ONFI param page is 256 bytes, and there are three redundant copies to be read. JEDEC param page is 512 bytes, and there are also three redundant copies to be read. Hence this buffer should be at least 512 x 3. This commits rounds the buffer size to 2048. Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Signed-off-by: Chris Packham <judge.packham@gmail.com> --- drivers/mtd/nand/pxa3xx_nand.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)