[RFC PATCH 0/3] mtd: nand: provide a clear separation of chip and controller ops

[RFC PATCH 0/3] mtd: nand: provide a clear separation of chip and controller ops

[Boris Brezillon]

Hello,

I'm pretty sure I'm starting a controversial discussion here, but that's
something I am thinking about for quite a while now, and instead of working
on my own to provide a full solution I thought this would be good to share
my vision and hopefully get feedback from other developers (including
maintainers ;-)).

This patch series is an attempt to define a clear separation between NAND
chip methods (those that are really dependent on the discovered NAND chip),
and NAND controller merhods (those generic enough to access any NAND chip
connected to a NAND controller).

This separation is motivated by several things.

The first one being my current attempt to support MLC chips. These chips
need some extra care (like read-retry or paired pages detection to avoid
any data loss, and obviously other things I'm not yet aware of).
MLC vendors each implement these features in a non standard way,
meaning that NAND core (or, as I see it, vendor specific code) has to fill
those function pointers accordingly.
While the kernel documentation tries to specify which function should be
filled by the controller part, and which one should be filled by NAND core,
providing a clear separation would makes things even clearer.

The second aspect, which IHMO is less important, is that NAND controller
drivers that deal with multiple chips won't have to fill the chip
function each time they add one, and it would even save some space in this
case (though I don't know any board embedding several NAND chips yet :-().

There surely is other pros and cons to this approach, and I'm pretty sure
you will point plenty of them.

I'd like to discuss another thing, not directly related to the controller
ops separation: the automatic assignment of default methods done in by
the core.
I've done a few reviews and worked on a few NAND drivers lately, and I
noticed that a lot of them do not implement the raw access functions
(write/read_page_raw in nand_ecc_ctrl).
It's obviously a bad thing, but that's not my point :-).
Actually they all leave the read/write_page_raw unassigned and let NAND
core assign default values which will obviously not do what's expected
at all.
My point is, maybe we should provide default implementations to all drivers
(export GPL symbols) and let them fill their function pointer instead of
guessing what they want to do.
This way we would easily detect offending drivers and complain before they
can even register a NAND chip.


Now, let's talk about the implementation proposed in this RFC. It's not
complete yet, since the methods in nand_chip are still there (and
assigned to the NAND controller ops one), but my goal is to eventually
remove them.
Anyway, this implies modifying all the drivers and I'm not inclined to do
that until every body has agreed on something.

I've only extracted methods I was pretty sure they were related to the NAND
controller, but there may be other methods that could be moved out (or I
might have wrongly assumed some of them were related to NAND controllers).
Please share your opinion on that point.

The sunxi nand driver has been patched to make use of this new approach and
can be considered as a reference implementation.


That's all I got for now.
I'm waiting for your feedback.

Thanks.

Best Regards,

Boris

Boris Brezillon (3):
  mtd: nand: introduce NAND controller ops
  mtd: nand: export nand_wait so that NAND controller driver can use it
  mtd: nand: sunxi: define NAND controller ops instead of assigning chip
    functions

 drivers/mtd/nand/nand_base.c  | 26 +++++++++++++++++++++++++-
 drivers/mtd/nand/sunxi_nand.c | 15 ++++++++++-----
 include/linux/mtd/nand.h      | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 6 deletions(-)

-- 
1.9.1

[RFC PATCH 1/3] mtd: nand: introduce NAND controller ops

[Boris Brezillon]

Currently all NAND operations are attached to the NAND chip itself (
struct nand_chip).
While this makes sense for some of them, like setup_read_retry or cmdfunc
because they depend on the discovered chip, most of them can be shared
across all chips attached to the NAND controller.

This would not only clean the nand_chip struct but also add a clear
separation between what's controller specific and what's chip specific.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 23 +++++++++++++++++++++++
 include/linux/mtd/nand.h     | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 41585df..e36ec5e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2944,9 +2944,32 @@ static void nand_resume(struct mtd_info *mtd)
 			__func__);
 }
 
+static void nand_set_controller_ops(struct nand_chip *chip)
+{
+	const struct nand_controller_ops *ops = chip->controller->ops;
+
+	if (!ops)
+		return;
+
+	chip->read_byte = ops->read_byte;
+	chip->read_word = ops->read_word;
+	chip->write_byte = ops->write_byte;
+	chip->write_buf = ops->write_buf;
+	chip->read_buf = ops->read_buf;
+	chip->select_chip = ops->select_chip;
+	chip->cmd_ctrl = ops->cmd_ctrl;
+	chip->init_size = ops->init_size;
+	chip->waitfunc = ops->waitfunc;
+	chip->erase = ops->erase;
+	chip->write_page = ops->write_page;
+}
+
 /* Set default functions */
 static void nand_set_defaults(struct nand_chip *chip, int busw)
 {
+	/* assing chip callbacks to controller ops functions if available */
+	nand_set_controller_ops(chip);
+
 	/* check for proper chip_delay setup, set 20us if not */
 	if (!chip->chip_delay)
 		chip->chip_delay = 20;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 3d4ea7e..6c0aadc 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -426,17 +426,54 @@ struct nand_jedec_params {
 } __packed;
 
 /**
+ * struct nand_controller_ops - NAND Controller operations
+ * @read_byte:		read one byte from the chip
+ * @read_word:		read one word from the chip
+ * @write_byte:		write a single byte to the chip on the low 8 I/O lines
+ * @write_buf:		write data from the buffer to the chip
+ * @read_buf:		read data from the chip into the buffer
+ * @select_chip:	select chip nr
+ * @cmd_ctrl:		hardwarespecific function for controlling ALE/CLE/nCE.
+ *			Also used to write command and address
+ * @init_size:		hardwarespecific function for setting mtd->oobsize,
+ *			mtd->writesize and so on.
+ *			@id_data contains the 8 bytes values of NAND_CMD_READID.
+ *			Return with the bus width.
+ * @waitfunc:		hardwarespecific function for wait on ready.
+ * @erase:		erase function
+ * @write_page:		High-level page write function
+ */
+struct nand_controller_ops {
+	uint8_t (*read_byte)(struct mtd_info *mtd);
+	u16 (*read_word)(struct mtd_info *mtd);
+	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
+	void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
+	void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
+	void (*select_chip)(struct mtd_info *mtd, int chip);
+	void (*cmd_ctrl)(struct mtd_info *mtd, int dat, unsigned int ctrl);
+	int (*init_size)(struct mtd_info *mtd, struct nand_chip *this,
+			 u8 *id_data);
+	int(*waitfunc)(struct mtd_info *mtd, struct nand_chip *this);
+	int (*erase)(struct mtd_info *mtd, int page);
+	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
+			  uint32_t offset, int data_len, const uint8_t *buf,
+			  int oob_required, int page, int cached, int raw);
+};
+
+/**
  * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
  * @lock:               protection lock
  * @active:		the mtd device which holds the controller currently
  * @wq:			wait queue to sleep on if a NAND operation is in
  *			progress used instead of the per chip wait queue
  *			when a hw controller is available.
+ * @ops:		controller operations
  */
 struct nand_hw_control {
 	spinlock_t lock;
 	struct nand_chip *active;
 	wait_queue_head_t wq;
+	const struct nand_controller_ops *ops;
 };
 
 /**
-- 
1.9.1

[RFC PATCH 2/3] mtd: nand: export nand_wait so that NAND controller driver can use it

[Boris Brezillon]

Export this function for NAND controller that which to use this default
implementation as their waitfunc method.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 3 ++-
 include/linux/mtd/nand.h     | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e36ec5e..63bf2ba 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -872,7 +872,7 @@ static void panic_nand_wait(struct mtd_info *mtd, struct nand_chip *chip,
  * take up to 400ms and program up to 20ms according to general NAND and
  * SmartMedia specs.
  */
-static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
+int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 {
 
 	int status, state = chip->state;
@@ -910,6 +910,7 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 	WARN_ON(!(status & NAND_STATUS_READY));
 	return status;
 }
+EXPORT_SYMBOL_GPL(nand_wait);
 
 /**
  * __nand_unlock - [REPLACEABLE] unlocks specified locked blocks
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6c0aadc..30f8aa1 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -26,6 +26,8 @@
 
 struct mtd_info;
 struct nand_flash_dev;
+struct nand_chip;
+
 /* Scan and identify a NAND device */
 extern int nand_scan(struct mtd_info *mtd, int max_chips);
 /*
@@ -42,6 +44,9 @@ extern void nand_release(struct mtd_info *mtd);
 /* Internal helper for board drivers which need to override command function */
 extern void nand_wait_ready(struct mtd_info *mtd);
 
+/* Internal helper for drivers which rely on the default nand_wait func */
+int nand_wait(struct mtd_info *mtd, struct nand_chip *chip);
+
 /* locks all blocks present in the device */
 extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 
-- 
1.9.1

[RFC PATCH 3/3] mtd: nand: sunxi: define NAND controller ops instead of assigning chip functions

[Boris Brezillon]

Now that we have NAND controller operations we can assign them once in
the NAND controller probe function and rely on these ops for all NAND
chips attached to this controller.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index ccaa8e2..43fb2ab 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -1235,11 +1235,6 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 	/* Default tR value specified in the ONFI spec (chapter 4.15.1) */
 	nand->chip_delay = 200;
 	nand->controller = &nfc->controller;
-	nand->select_chip = sunxi_nfc_select_chip;
-	nand->cmd_ctrl = sunxi_nfc_cmd_ctrl;
-	nand->read_buf = sunxi_nfc_read_buf;
-	nand->write_buf = sunxi_nfc_write_buf;
-	nand->read_byte = sunxi_nfc_read_byte;
 
 	if (of_get_nand_on_flash_bbt(np))
 		nand->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
@@ -1317,6 +1312,15 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
 	}
 }
 
+static const struct nand_controller_ops sunxi_nfc_ops = {
+	.select_chip = sunxi_nfc_select_chip,
+	.cmd_ctrl = sunxi_nfc_cmd_ctrl,
+	.read_buf = sunxi_nfc_read_buf,
+	.write_buf = sunxi_nfc_write_buf,
+	.read_byte = sunxi_nfc_read_byte,
+	.waitfunc = nand_wait,
+};
+
 static int sunxi_nfc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1330,6 +1334,7 @@ static int sunxi_nfc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	nfc->dev = dev;
+	nfc->controller.ops = &sunxi_nfc_ops;
 	spin_lock_init(&nfc->controller.lock);
 	init_waitqueue_head(&nfc->controller.wq);
 	INIT_LIST_HEAD(&nfc->chips);
-- 
1.9.1

Re: [RFC PATCH 0/3] mtd: nand: provide a clear separation of chip and controller ops

[Brian Norris]

Hi Boris,

Sorry for the delay here. I've owed you a response for a while.

On Sat, Feb 14, 2015 at 02:32:35PM +0100, Boris Brezillon wrote:
> Hello,
> 
> I'm pretty sure I'm starting a controversial discussion here, but that's
> something I am thinking about for quite a while now, and instead of working
> on my own to provide a full solution I thought this would be good to share
> my vision and hopefully get feedback from other developers (including
> maintainers ;-)).
> 
> This patch series is an attempt to define a clear separation between NAND
> chip methods (those that are really dependent on the discovered NAND chip),
> and NAND controller merhods (those generic enough to access any NAND chip
> connected to a NAND controller).

I think you might be on to something here. I'm feeling more and more
like a lot of the NAND subsystem is layered kinda badly [1]. This is
probably much due to history, where nand_base was first designed for
relatively simple NAND controllers, where much of the heavy lifting was
done in software. Nowadays, controllers do a lot more automagically.

Anyway, I agree in principle that a clearer separation between flash and
controller is a good goal.

> This separation is motivated by several things.
> 
> The first one being my current attempt to support MLC chips. These chips
> need some extra care (like read-retry or paired pages detection to avoid
> any data loss, and obviously other things I'm not yet aware of).
> MLC vendors each implement these features in a non standard way,
> meaning that NAND core (or, as I see it, vendor specific code) has to fill
> those function pointers accordingly.
> While the kernel documentation tries to specify which function should be
> filled by the controller part, and which one should be filled by NAND core,
> providing a clear separation would makes things even clearer.

Sure, I think these should be kept separate in some way. Whether that's
with your current proposal or with something else remains to be seen.

> The second aspect, which IHMO is less important,

I agree.

> is that NAND controller
> drivers that deal with multiple chips won't have to fill the chip
> function each time they add one, and it would even save some space in this
> case (though I don't know any board embedding several NAND chips yet :-().

I've seen a few that used a boot SLC NAND and a large MLC NAND for
secondary storage. It's been a while since I've seen anything like that
though.

I think the more important aspect of this point is that it puts the
driver writer in the right mindset. If we have a clearer "controller"
struct (it exists already, but it's used for very little), we can hope
to eventually encourage drivers that will not have to modify struct
nand_chip at all, but leave that to the common code.

> There surely is other pros and cons to this approach, and I'm pretty sure
> you will point plenty of them.

At the moment (and, though I have taken I while to respond, I have
thought about this issue occasionally over the last two months) I don't
actually see a lot of problems with your current proposal. The code is
pretty trivial at the moment.

The problem might only be that this does not yet go far enough. A lot of
the controller-specific stuff ends up being related to the nand_ecc_ctrl
struct. Those function pointers essentially end up being the bulk of
controller-specific function pointers, in some cases, but those may even
vary from nand_chip to nand_chip. I haven't figured out what best to do
with these yet.

(BTW, this source of problem makes it hard to deal with on-die/built-in
ECC too; we might need to make use of the controller-specific *raw* read
functions while still handling the on-die ECC status info. *hint hint
Richard* *I'll comment on your on-die ECC patches soon, I hope*)

> I'd like to discuss another thing, not directly related to the controller
> ops separation: the automatic assignment of default methods done in by
> the core.
> I've done a few reviews and worked on a few NAND drivers lately, and I
> noticed that a lot of them do not implement the raw access functions
> (write/read_page_raw in nand_ecc_ctrl).
> It's obviously a bad thing, but that's not my point :-).

I agree that's bad :)

> Actually they all leave the read/write_page_raw unassigned and let NAND
> core assign default values which will obviously not do what's expected
> at all.

Actually, that might be expected. e.g., on some NAND_ECC_HW
implementations, the driver may be relying entirely on the
ecc.{hwctl,correct,calculate} functions, and in the absence of those
calls (e.g., when read_page_raw() just performs calls to read_buf()),
then no HW ECC is initiated.

> My point is, maybe we should provide default implementations to all drivers
> (export GPL symbols) and let them fill their function pointer instead of
> guessing what they want to do.
> This way we would easily detect offending drivers and complain before they
> can even register a NAND chip.

I don't think we can do exactly that -- that would leave a lot of
implementations where we have to duplicate too much stuff -- though a
compromise might work. Perhaps we can at least have nand_base complain /
error out [2] if ecc.{read,write}_page() are provided by the driver but
ecc.{read,write}_page_raw() are not. That would cover the likely problem
cases, while avoiding work on some cases that are done properly.

> Now, let's talk about the implementation proposed in this RFC. It's not
> complete yet, since the methods in nand_chip are still there (and
> assigned to the NAND controller ops one), but my goal is to eventually
> remove them.

Maybe some extra comments are in order there, then? We are always in
need of big blocks of explanatory text :)

> Anyway, this implies modifying all the drivers and I'm not inclined to do
> that until every body has agreed on something.

Right, good idea. But it seems that only you and I have opinions here,
so "agreement" may just be between us. Or you just weren't loud enough.

> I've only extracted methods I was pretty sure they were related to the NAND
> controller, but there may be other methods that could be moved out (or I
> might have wrongly assumed some of them were related to NAND controllers).
> Please share your opinion on that point.

I think your initial set looks OK, though the 'write_page' function
sticks out a bit. It's not symmetric (there's no similar 'read_page'),
and I actually think the nand_ecc_ctrl pointers should cover everyone's
uses (atmel_nand is the only user of nand_chip::write_page).

> The sunxi nand driver has been patched to make use of this new approach and
> can be considered as a reference implementation.
> 
> 
> That's all I got for now.
> I'm waiting for your feedback.

Thanks for the thoughts and patience. I've been pretty swamped (yeah,
yeah, so is everyone; not the greatest excuse). And now I have a nice
MTD backlog!

> Thanks.
> 
> Best Regards,
> 
> Boris
> 
> Boris Brezillon (3):
>   mtd: nand: introduce NAND controller ops
>   mtd: nand: export nand_wait so that NAND controller driver can use it
>   mtd: nand: sunxi: define NAND controller ops instead of assigning chip
>     functions
> 
>  drivers/mtd/nand/nand_base.c  | 26 +++++++++++++++++++++++++-
>  drivers/mtd/nand/sunxi_nand.c | 15 ++++++++++-----
>  include/linux/mtd/nand.h      | 42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 6 deletions(-)
> 
> -- 
> 1.9.1
> 

Brian

[1] Tangent on layering: can you think of any good reason why we don't
embed a struct mtd_info in struct nand_chip? That seems like it would be
the better layering. That would help unclutter some of the APIs, where
we have both 'mtd' and 'chip' parameters passed all over the place,
where we should only need 'chip'.

[2] That's another thing; there are too many BUG()'s in
nand_scan_tail()!

Re: [RFC PATCH 0/3] mtd: nand: provide a clear separation of chip and controller ops

[Boris Brezillon]

Hi Brian,

On Fri, 24 Apr 2015 19:22:54 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> Sorry for the delay here. I've owed you a response for a while.

Hey, I thought this 

> > This patch series is an attempt to define a clear separation between NAND
> > chip methods (those that are really dependent on the discovered NAND chip),
> > and NAND controller merhods (those generic enough to access any NAND chip
> > connected to a NAND controller).
> 
> I think you might be on to something here. I'm feeling more and more
> like a lot of the NAND subsystem is layered kinda badly [1]. This is
> probably much due to history, where nand_base was first designed for
> relatively simple NAND controllers, where much of the heavy lifting was
> done in software. Nowadays, controllers do a lot more automagically.
> 
> Anyway, I agree in principle that a clearer separation between flash and
> controller is a good goal.

Great!

> 
> > This separation is motivated by several things.
> > 
> > The first one being my current attempt to support MLC chips. These chips
> > need some extra care (like read-retry or paired pages detection to avoid
> > any data loss, and obviously other things I'm not yet aware of).
> > MLC vendors each implement these features in a non standard way,
> > meaning that NAND core (or, as I see it, vendor specific code) has to fill
> > those function pointers accordingly.
> > While the kernel documentation tries to specify which function should be
> > filled by the controller part, and which one should be filled by NAND core,
> > providing a clear separation would makes things even clearer.
> 
> Sure, I think these should be kept separate in some way. Whether that's
> with your current proposal or with something else remains to be seen.

Feel free to suggest other approaches.

> 
> > There surely is other pros and cons to this approach, and I'm pretty sure
> > you will point plenty of them.
> 
> At the moment (and, though I have taken I while to respond, I have
> thought about this issue occasionally over the last two months) I don't
> actually see a lot of problems with your current proposal. The code is
> pretty trivial at the moment.
> 
> The problem might only be that this does not yet go far enough. A lot of
> the controller-specific stuff ends up being related to the nand_ecc_ctrl
> struct. Those function pointers essentially end up being the bulk of
> controller-specific function pointers, in some cases, but those may even
> vary from nand_chip to nand_chip. I haven't figured out what best to do
> with these yet.

Actually I was planning to make the same separation for nand_ecc_ctrl.
The ops should be part of another structure (nand_ecc_ctrl seems like
a good container), while instantiation information (like the selected
ECC strength/step size and the oob layout) should be assigned to the
NAND chip itself.

> 
> (BTW, this source of problem makes it hard to deal with on-die/built-in
> ECC too; we might need to make use of the controller-specific *raw* read
> functions while still handling the on-die ECC status info. *hint hint
> Richard* *I'll comment on your on-die ECC patches soon, I hope*)

Hum, I'm not sure the raw functions are in cause here, but maybe we
should provide read/write page functions (those ones should access the
NAND in raw mode of course) in the nand_controller_ops.

This brings us to another aspect I'd like to rework: the way we're
attaching an ECC implementation to a NAND chip.
Currently, the nand_chip struct embeds a nand_ecc_ctrl struct which is
then filled by NAND controller drivers.
I'd like to make this field a pointer, so that NAND controllers can
provide an ECC implementation which can then be overloaded by nand core
if on-die ECC is available and explicitly requested.

I might be wrong, but all the attempt to support on-die ECC I've seen
so far are involving ECC implementation selection in each NAND
controller driver.

> > My point is, maybe we should provide default implementations to all drivers
> > (export GPL symbols) and let them fill their function pointer instead of
> > guessing what they want to do.
> > This way we would easily detect offending drivers and complain before they
> > can even register a NAND chip.
> 
> I don't think we can do exactly that -- that would leave a lot of
> implementations where we have to duplicate too much stuff -- though a
> compromise might work. Perhaps we can at least have nand_base complain /
> error out [2] if ecc.{read,write}_page() are provided by the driver but
> ecc.{read,write}_page_raw() are not. That would cover the likely problem
> cases, while avoiding work on some cases that are done properly.

Yes, that's a good idea.

> 
> > Now, let's talk about the implementation proposed in this RFC. It's not
> > complete yet, since the methods in nand_chip are still there (and
> > assigned to the NAND controller ops one), but my goal is to eventually
> > remove them.
> 
> Maybe some extra comments are in order there, then? We are always in
> need of big blocks of explanatory text :)

Sure, I'll add some comments :P.

> 
> > Anyway, this implies modifying all the drivers and I'm not inclined to do
> > that until every body has agreed on something.
> 
> Right, good idea. But it seems that only you and I have opinions here,
> so "agreement" may just be between us. Or you just weren't loud enough.

I'll add active NAND driver maintainers/developers in the loop for the
next version.

> 
> > I've only extracted methods I was pretty sure they were related to the NAND
> > controller, but there may be other methods that could be moved out (or I
> > might have wrongly assumed some of them were related to NAND controllers).
> > Please share your opinion on that point.
> 
> I think your initial set looks OK, though the 'write_page' function
> sticks out a bit. It's not symmetric (there's no similar 'read_page'),
> and I actually think the nand_ecc_ctrl pointers should cover everyone's
> uses (atmel_nand is the only user of nand_chip::write_page).

Yep, I'll look at it (either remove it and modify the atmel driver or
add a read_page for raw read/write page implementation at the NAND
controller level).

Thanks for your feedback.

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC PATCH 0/3] mtd: nand: provide a clear separation of chip and controller ops

[Brian Norris]

On Tue, Apr 28, 2015 at 06:02:41PM +0200, Boris Brezillon wrote:
> On Fri, 24 Apr 2015 19:22:54 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> > > There surely is other pros and cons to this approach, and I'm pretty sure
> > > you will point plenty of them.
> > 
> > At the moment (and, though I have taken I while to respond, I have
> > thought about this issue occasionally over the last two months) I don't
> > actually see a lot of problems with your current proposal. The code is
> > pretty trivial at the moment.
> > 
> > The problem might only be that this does not yet go far enough. A lot of
> > the controller-specific stuff ends up being related to the nand_ecc_ctrl
> > struct. Those function pointers essentially end up being the bulk of
> > controller-specific function pointers, in some cases, but those may even
> > vary from nand_chip to nand_chip. I haven't figured out what best to do
> > with these yet.
> 
> Actually I was planning to make the same separation for nand_ecc_ctrl.
> The ops should be part of another structure (nand_ecc_ctrl seems like
> a good container), while instantiation information (like the selected
> ECC strength/step size and the oob layout) should be assigned to the
> NAND chip itself.

Sounds about right.

> > (BTW, this source of problem makes it hard to deal with on-die/built-in
> > ECC too; we might need to make use of the controller-specific *raw* read
> > functions while still handling the on-die ECC status info. *hint hint
> > Richard* *I'll comment on your on-die ECC patches soon, I hope*)
> 
> Hum, I'm not sure the raw functions are in cause here, but maybe we
> should provide read/write page functions (those ones should access the
> NAND in raw mode of course) in the nand_controller_ops.
> 
> This brings us to another aspect I'd like to rework: the way we're
> attaching an ECC implementation to a NAND chip.
> Currently, the nand_chip struct embeds a nand_ecc_ctrl struct which is
> then filled by NAND controller drivers.
> I'd like to make this field a pointer, so that NAND controllers can
> provide an ECC implementation which can then be overloaded by nand core
> if on-die ECC is available and explicitly requested.

Could work. Though, the "overloading" would probably involve wrapping
the controller callbacks, not completely replacing. It would look
loosely like:

 mtd_read()
 |_ ... chip->read_something()
        |_ if (on-die)
	      hwctrl->read_something_raw()
	   else
	      hwctrl->read_something()

> I might be wrong, but all the attempt to support on-die ECC I've seen
> so far are involving ECC implementation selection in each NAND
> controller driver.

Yeah, and that's one reason I haven't merged any of them. They are very
much tied to specific drivers (or classes of drivers). They haven't
provisioned for any way to seamlessly add on-die ECC support to drivers
by, e.g., disabling existing hardware ECC, or using existing "raw"
support to build support for on-die ECC.

In considering options here, I think we have a few things to plan for:
1. one controller may support many types of ECC (SW, HW)
2. one controller may support many strengths for a particular type
(1-bit, 8-bit, 24-bit, etc.)
3. we might want to override all of the above in order to use on-die ECC
4. we might want to opt out of on-die ECC, even on those that support it
(on Micron I think you can disable it? but on Toshiba you can't)
5. different ECC schemes for different partitions? Some have requested
being able to do a HW (?) 1-bit ECC on the bootloader partition, and a
stronger SW or HW ECC on the rest.

#1 and #2 have been handled OK with a variety of driver-specific coding
in between nand_scan_ident() and nand_scan_tail(). But it's a bit too
much work to make every driver add more logic there for #3 and #4. So if
we're going to do that kind of work, we should plan something that
allows a little more modularity / replaceability.

#5 is a lot less important, but I thought I'd throw it in there, in case
it could help guide the thought process for reworking this now.

Maybe some kind of ecc_ctrl() or ecc_select() callback provided (at
least partially) by the controller? It could handle multiplexing the
various ECC modes, so that nand_base can configure them directly. (Or
maybe I'm off-base a bit. I'll admit I haven't 100% though this last
suggestion completely, though I've imagined something like that for a
while.)

Brian