On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> are necessary to address the correct page. The driver sets the address for
> more than 16 bits, but it uses 16-bit arguments and variables (these are
> page_id, block_id, row) to do address operations. Obviously, these
> arguments and variables cannot deal with more than 16-bit address.
>
> Signed-off-by: Jheng-Jhong Wu <[email protected]>
This seems reasonable... It would be needed to make commit 6efb21d6d0e7
("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
addressing.") work. It also fixes a static checker warning.
My only concern is that the mtd/nand code seems to use -1 as a magical
page_id. For example:
2069 /**
2070 * nand_exit_status_op - Exit a STATUS operation
2071 * @chip: The NAND chip
2072 *
2073 * This function sends a READ0 command to cancel the effect of the STATUS
2074 * command to avoid reading only the status until a new read command is sent.
2075 *
2076 * This function does not select/unselect the CS line.
2077 *
2078 * Returns 0 on success, a negative error code otherwise.
2079 */
2080 int nand_exit_status_op(struct nand_chip *chip)
2081 {
2082 struct mtd_info *mtd = nand_to_mtd(chip);
2083
2084 if (chip->exec_op) {
2085 struct nand_op_instr instrs[] = {
2086 NAND_OP_CMD(NAND_CMD_READ0, 0),
2087 };
2088 struct nand_operation op = NAND_OPERATION(instrs);
2089
2090 return nand_exec_op(chip, &op);
2091 }
2092
2093 chip->cmdfunc(mtd, NAND_CMD_READ0, -1, -1);
^^^^^^^^^^^^^^ ^^
2094
2095 return 0;
2096 }
2097 EXPORT_SYMBOL_GPL(nand_exit_status_op);
I'm not sure if this affect spinand_read_page() etc.
regards,
dan carpenter
Hi Dan,
Dan Carpenter <[email protected]> wrote on Wed, 1 Aug 2018
15:05:51 +0300:
> On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > are necessary to address the correct page. The driver sets the address for
> > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > page_id, block_id, row) to do address operations. Obviously, these
> > arguments and variables cannot deal with more than 16-bit address.
> >
> > Signed-off-by: Jheng-Jhong Wu <[email protected]>
>
> This seems reasonable... It would be needed to make commit 6efb21d6d0e7
> ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> addressing.") work. It also fixes a static checker warning.
>
> My only concern is that the mtd/nand code seems to use -1 as a magical
> page_id. For example:
I guess you missed Boris' comment: this driver is very likely to be
removed. A SPI-NAND framework has been added. It does not use the raw
NAND framework anymore, which was wrong anyway.
Thanks,
Miquèl
On Wed, Aug 01, 2018 at 03:55:30PM +0200, Miquel Raynal wrote:
> Hi Dan,
>
> Dan Carpenter <[email protected]> wrote on Wed, 1 Aug 2018
> 15:05:51 +0300:
>
> > On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > > are necessary to address the correct page. The driver sets the address for
> > > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > > page_id, block_id, row) to do address operations. Obviously, these
> > > arguments and variables cannot deal with more than 16-bit address.
> > >
> > > Signed-off-by: Jheng-Jhong Wu <[email protected]>
> >
> > This seems reasonable... It would be needed to make commit 6efb21d6d0e7
> > ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> > addressing.") work. It also fixes a static checker warning.
> >
> > My only concern is that the mtd/nand code seems to use -1 as a magical
> > page_id. For example:
>
> I guess you missed Boris' comment: this driver is very likely to be
> removed. A SPI-NAND framework has been added. It does not use the raw
> NAND framework anymore, which was wrong anyway.
>
We should probably revert commit 6efb21d6d0e7 ("staging:mt29f_spinand:
MT29F2G failing as only 16 bits used for addressing.") in that case if
all it does is add static checker warnings...
regards,
dan carpenter
So should I change this into a revert patch, or you will revert commit
6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16 bits
used for addressing.") by yourself?
Thanks.
Best Regards,
Jheng-Jhong Wu (Victor Wu)
On Mon, Aug 06, 2018 at 09:49:01AM +0800, Jheng-Jhong Wu wrote:
> So should I change this into a revert patch, or you will revert commit
> 6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16 bits
> used for addressing.") by yourself?
> Thanks.
>
Are you asking me? I'm not going to revert it.
regards,
dan carpenter
Hi Dan,
On Wed, 1 Aug 2018 15:05:51 +0300
Dan Carpenter <[email protected]> wrote:
> On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > are necessary to address the correct page. The driver sets the address for
> > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > page_id, block_id, row) to do address operations. Obviously, these
> > arguments and variables cannot deal with more than 16-bit address.
> >
> > Signed-off-by: Jheng-Jhong Wu <[email protected]>
>
> This seems reasonable... It would be needed to make commit 6efb21d6d0e7
> ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> addressing.") work. It also fixes a static checker warning.
>
> My only concern is that the mtd/nand code seems to use -1 as a magical
> page_id. For example:
Yes, -1 means "don't issue the row/page address cycles", though I
don't think page can be -1 for NAND_CMD_READ{1,0} commands.
Anyway, if you want this patch merged to fix a static checker warning,
I'm fine with that. In any case, I still plan to send a patch removing
this driver for v4.20, so, anyone using this driver should start
testing the new SPI NAND driver (drivers/mtd/nand/spi) and tweak/fix
the new implementation if needed.
Regards,
Boris
On Mon, Aug 06, 2018 at 01:46:48PM +0200, Boris Brezillon wrote:
> Hi Dan,
>
> On Wed, 1 Aug 2018 15:05:51 +0300
> Dan Carpenter <[email protected]> wrote:
>
> > On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > > are necessary to address the correct page. The driver sets the address for
> > > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > > page_id, block_id, row) to do address operations. Obviously, these
> > > arguments and variables cannot deal with more than 16-bit address.
> > >
> > > Signed-off-by: Jheng-Jhong Wu <[email protected]>
> >
> > This seems reasonable... It would be needed to make commit 6efb21d6d0e7
> > ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> > addressing.") work. It also fixes a static checker warning.
> >
> > My only concern is that the mtd/nand code seems to use -1 as a magical
> > page_id. For example:
>
> Yes, -1 means "don't issue the row/page address cycles", though I
> don't think page can be -1 for NAND_CMD_READ{1,0} commands.
>
It sure looks like it can be in nand_exit_status_op()...
> Anyway, if you want this patch merged to fix a static checker warning,
> I'm fine with that. In any case, I still plan to send a patch removing
> this driver for v4.20, so, anyone using this driver should start
> testing the new SPI NAND driver (drivers/mtd/nand/spi) and tweak/fix
> the new implementation if needed.
I don't think we should make the code more complicated than necessary
just to make static checkers happy. When you say "this driver", you
mean the staging driver? In that case, there is no need to revert
commit 6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16
bits used for addressing.").
regards,
dan carpenter
On Mon, 6 Aug 2018 15:01:37 +0300
Dan Carpenter <[email protected]> wrote:
> On Mon, Aug 06, 2018 at 01:46:48PM +0200, Boris Brezillon wrote:
> > Hi Dan,
> >
> > On Wed, 1 Aug 2018 15:05:51 +0300
> > Dan Carpenter <[email protected]> wrote:
> >
> > > On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > > > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > > > are necessary to address the correct page. The driver sets the address for
> > > > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > > > page_id, block_id, row) to do address operations. Obviously, these
> > > > arguments and variables cannot deal with more than 16-bit address.
> > > >
> > > > Signed-off-by: Jheng-Jhong Wu <[email protected]>
> > >
> > > This seems reasonable... It would be needed to make commit 6efb21d6d0e7
> > > ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> > > addressing.") work. It also fixes a static checker warning.
> > >
> > > My only concern is that the mtd/nand code seems to use -1 as a magical
> > > page_id. For example:
> >
> > Yes, -1 means "don't issue the row/page address cycles", though I
> > don't think page can be -1 for NAND_CMD_READ{1,0} commands.
> >
>
> It sure looks like it can be in nand_exit_status_op()...
True, but nand_exit_status_op() won't be called here. It's only used
when the NAND controller driver is implementing ->exec_op() and needs
to do status polling, and from the micron NAND code that deals with raw
NAND chips with on-die ECC (the mt29f driver is supposed to deal with
SPI NANDs).
>
> > Anyway, if you want this patch merged to fix a static checker warning,
> > I'm fine with that. In any case, I still plan to send a patch removing
> > this driver for v4.20, so, anyone using this driver should start
> > testing the new SPI NAND driver (drivers/mtd/nand/spi) and tweak/fix
> > the new implementation if needed.
>
> I don't think we should make the code more complicated than necessary
> just to make static checkers happy. When you say "this driver", you
> mean the staging driver?
Yes.
> In that case, there is no need to revert
> commit 6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16
> bits used for addressing.").
Okay.