[PATCH v2 0/2] Changes in the internal raw NAND API

[PATCH v2 0/2] Changes in the internal raw NAND API

[Miquel Raynal]

Hello,

After having sent two versions of a series removing the
nand_scan_ident/nand_scan_tail limitation that prevents us to allocate
memory at critical moments, we are still discussing the internal API.

To avoid sending again more than 20 patches, this is an RFC of the
internal changes prior to the above modifications in each driver:

1/ Rename struct nand_hw_control -> struct nand_controller which is
   much more meaningful.
2/ Rename the function initializing the above structure
   nand_hw_control_init() -> nand_controller_init().
3/ Rename the dummy controller implementation in the nand_chip structure
   hwcontrol -> dummy_controller.
3/ Create a nand_controller_ops structure which will be embedded in the
   nand_controller structure. These operations are:
   int (*attach_chip)(struct nand_chip *) and
   void (*detach_chip)(struct nand_chip *).

If we agree on this, I could merge them first and then send the bunch of
patches making use of these API changes.

Thanks,
Miquèl

Changes since v1 (RFC):
=======================
* Added Acked-by tags from Boris.
* Updated the commit logs as commented.
* Squashed the 2 patches about nand_hw_control and
  nand_init_hw_control().
* Also added the rename of hwcontrol in patch 1.
* Added helpers to 1/ check the controller operations are present and
  2/ ->attach_chip() or ->detach_chip() is available, as suggested by
  Boris.


Miquel Raynal (2):
  mtd: rawnand: better name for the controller structure
  mtd: rawnand: add hooks that may be called during nand_scan()

 drivers/mtd/nand/raw/atmel/nand-controller.c | 10 ++++----
 drivers/mtd/nand/raw/brcmnand/brcmnand.c     |  4 ++--
 drivers/mtd/nand/raw/docg4.c                 |  4 ++--
 drivers/mtd/nand/raw/fsl_elbc_nand.c         |  4 ++--
 drivers/mtd/nand/raw/fsl_ifc_nand.c          |  4 ++--
 drivers/mtd/nand/raw/jz4780_nand.c           |  7 +++---
 drivers/mtd/nand/raw/marvell_nand.c          |  6 ++---
 drivers/mtd/nand/raw/mtk_nand.c              |  2 +-
 drivers/mtd/nand/raw/nand_base.c             | 36 ++++++++++++++++++++++++----
 drivers/mtd/nand/raw/ndfc.c                  |  4 ++--
 drivers/mtd/nand/raw/omap2.c                 |  2 +-
 drivers/mtd/nand/raw/oxnas_nand.c            |  4 ++--
 drivers/mtd/nand/raw/qcom_nandc.c            |  4 ++--
 drivers/mtd/nand/raw/s3c2410.c               |  4 ++--
 drivers/mtd/nand/raw/sunxi_nand.c            |  6 ++---
 drivers/mtd/nand/raw/tango_nand.c            |  4 ++--
 drivers/mtd/nand/raw/tegra_nand.c            |  6 ++---
 drivers/mtd/nand/raw/txx9ndfmc.c             |  4 ++--
 include/linux/mtd/rawnand.h                  | 30 ++++++++++++++++++-----
 19 files changed, 96 insertions(+), 49 deletions(-)

-- 
2.14.1

[PATCH v2 1/2] mtd: rawnand: better name for the controller structure

[Miquel Raynal]

In the raw NAND core, a NAND chip is described by a nand_chip structure,
while a NAND controller is described with a nand_hw_control structure
which is not very meaningful.

Rename this structure nand_controller.

As the structure gets renamed, it is logical to also rename the core
function initializing it from nand_hw_control_init() to
nand_controller_init().

Lastly, the 'hwcontrol' entry of the nand_chip structure is not
meaningful neither while it has the role of fallback when no controller
structure is provided by the driver (the controller driver is dumb and
can only control a single chip). Thus, it is renamed dummy_controller.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/nand/raw/atmel/nand-controller.c | 10 +++++-----
 drivers/mtd/nand/raw/brcmnand/brcmnand.c     |  4 ++--
 drivers/mtd/nand/raw/docg4.c                 |  4 ++--
 drivers/mtd/nand/raw/fsl_elbc_nand.c         |  4 ++--
 drivers/mtd/nand/raw/fsl_ifc_nand.c          |  4 ++--
 drivers/mtd/nand/raw/jz4780_nand.c           |  7 ++++---
 drivers/mtd/nand/raw/marvell_nand.c          |  6 +++---
 drivers/mtd/nand/raw/mtk_nand.c              |  2 +-
 drivers/mtd/nand/raw/nand_base.c             |  4 ++--
 drivers/mtd/nand/raw/ndfc.c                  |  4 ++--
 drivers/mtd/nand/raw/omap2.c                 |  2 +-
 drivers/mtd/nand/raw/oxnas_nand.c            |  4 ++--
 drivers/mtd/nand/raw/qcom_nandc.c            |  4 ++--
 drivers/mtd/nand/raw/s3c2410.c               |  4 ++--
 drivers/mtd/nand/raw/sunxi_nand.c            |  6 +++---
 drivers/mtd/nand/raw/tango_nand.c            |  4 ++--
 drivers/mtd/nand/raw/tegra_nand.c            |  6 +++---
 drivers/mtd/nand/raw/txx9ndfmc.c             |  4 ++--
 include/linux/mtd/rawnand.h                  | 14 ++++++++------
 19 files changed, 50 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
index e686fe73159e..1a9b871d01c9 100644
--- a/drivers/mtd/nand/raw/atmel/nand-controller.c
+++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
@@ -216,7 +216,7 @@ struct atmel_nand_controller_caps {
 };
 
 struct atmel_nand_controller {
-	struct nand_hw_control base;
+	struct nand_controller base;
 	const struct atmel_nand_controller_caps *caps;
 	struct device *dev;
 	struct regmap *smc;
@@ -227,7 +227,7 @@ struct atmel_nand_controller {
 };
 
 static inline struct atmel_nand_controller *
-to_nand_controller(struct nand_hw_control *ctl)
+to_nand_controller(struct nand_controller *ctl)
 {
 	return container_of(ctl, struct atmel_nand_controller, base);
 }
@@ -239,7 +239,7 @@ struct atmel_smc_nand_controller {
 };
 
 static inline struct atmel_smc_nand_controller *
-to_smc_nand_controller(struct nand_hw_control *ctl)
+to_smc_nand_controller(struct nand_controller *ctl)
 {
 	return container_of(to_nand_controller(ctl),
 			    struct atmel_smc_nand_controller, base);
@@ -263,7 +263,7 @@ struct atmel_hsmc_nand_controller {
 };
 
 static inline struct atmel_hsmc_nand_controller *
-to_hsmc_nand_controller(struct nand_hw_control *ctl)
+to_hsmc_nand_controller(struct nand_controller *ctl)
 {
 	return container_of(to_nand_controller(ctl),
 			    struct atmel_hsmc_nand_controller, base);
@@ -1966,7 +1966,7 @@ static int atmel_nand_controller_init(struct atmel_nand_controller *nc,
 	struct device_node *np = dev->of_node;
 	int ret;
 
-	nand_hw_control_init(&nc->base);
+	nand_controller_init(&nc->base);
 	INIT_LIST_HEAD(&nc->chips);
 	nc->dev = dev;
 	nc->caps = caps;
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 1306aaa7a8bf..2e5efa0f9ea2 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -114,7 +114,7 @@ enum {
 
 struct brcmnand_controller {
 	struct device		*dev;
-	struct nand_hw_control	controller;
+	struct nand_controller	controller;
 	void __iomem		*nand_base;
 	void __iomem		*nand_fc; /* flash cache */
 	void __iomem		*flash_dma_base;
@@ -2433,7 +2433,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 
 	init_completion(&ctrl->done);
 	init_completion(&ctrl->dma_done);
-	nand_hw_control_init(&ctrl->controller);
+	nand_controller_init(&ctrl->controller);
 	INIT_LIST_HEAD(&ctrl->host_list);
 
 	/* NAND register range */
diff --git a/drivers/mtd/nand/raw/docg4.c b/drivers/mtd/nand/raw/docg4.c
index bb96cb33cd6b..4dccdfba6140 100644
--- a/drivers/mtd/nand/raw/docg4.c
+++ b/drivers/mtd/nand/raw/docg4.c
@@ -1257,8 +1257,8 @@ static void __init init_mtd_structs(struct mtd_info *mtd)
 	nand->ecc.strength = DOCG4_T;
 	nand->options = NAND_BUSWIDTH_16 | NAND_NO_SUBPAGE_WRITE;
 	nand->IO_ADDR_R = nand->IO_ADDR_W = doc->virtadr + DOC_IOSPACE_DATA;
-	nand->controller = &nand->hwcontrol;
-	nand_hw_control_init(nand->controller);
+	nand->controller = &nand->dummy_controller;
+	nand_controller_init(nand->controller);
 
 	/* methods */
 	nand->cmdfunc = docg4_command;
diff --git a/drivers/mtd/nand/raw/fsl_elbc_nand.c b/drivers/mtd/nand/raw/fsl_elbc_nand.c
index 51f0b340bc0d..0aa54a949653 100644
--- a/drivers/mtd/nand/raw/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_elbc_nand.c
@@ -61,7 +61,7 @@ struct fsl_elbc_mtd {
 /* Freescale eLBC FCM controller information */
 
 struct fsl_elbc_fcm_ctrl {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct fsl_elbc_mtd *chips[MAX_BANKS];
 
 	u8 __iomem *addr;        /* Address of assigned FCM buffer        */
@@ -879,7 +879,7 @@ static int fsl_elbc_nand_probe(struct platform_device *pdev)
 		}
 		elbc_fcm_ctrl->counter++;
 
-		nand_hw_control_init(&elbc_fcm_ctrl->controller);
+		nand_controller_init(&elbc_fcm_ctrl->controller);
 		fsl_lbc_ctrl_dev->nand = elbc_fcm_ctrl;
 	} else {
 		elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 382b67e97174..73a5245bddd1 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -51,7 +51,7 @@ struct fsl_ifc_mtd {
 
 /* overview of the fsl ifc controller */
 struct fsl_ifc_nand_ctrl {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT];
 
 	void __iomem *addr;	/* Address of assigned IFC buffer	*/
@@ -1004,7 +1004,7 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 		ifc_nand_ctrl->addr = NULL;
 		fsl_ifc_ctrl_dev->nand = ifc_nand_ctrl;
 
-		nand_hw_control_init(&ifc_nand_ctrl->controller);
+		nand_controller_init(&ifc_nand_ctrl->controller);
 	} else {
 		ifc_nand_ctrl = fsl_ifc_ctrl_dev->nand;
 	}
diff --git a/drivers/mtd/nand/raw/jz4780_nand.c b/drivers/mtd/nand/raw/jz4780_nand.c
index e69f6ae4c539..49841dad347c 100644
--- a/drivers/mtd/nand/raw/jz4780_nand.c
+++ b/drivers/mtd/nand/raw/jz4780_nand.c
@@ -44,7 +44,7 @@ struct jz4780_nand_cs {
 struct jz4780_nand_controller {
 	struct device *dev;
 	struct jz4780_bch *bch;
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	unsigned int num_banks;
 	struct list_head chips;
 	int selected;
@@ -65,7 +65,8 @@ static inline struct jz4780_nand_chip *to_jz4780_nand_chip(struct mtd_info *mtd)
 	return container_of(mtd_to_nand(mtd), struct jz4780_nand_chip, chip);
 }
 
-static inline struct jz4780_nand_controller *to_jz4780_nand_controller(struct nand_hw_control *ctrl)
+static inline struct jz4780_nand_controller
+*to_jz4780_nand_controller(struct nand_controller *ctrl)
 {
 	return container_of(ctrl, struct jz4780_nand_controller, controller);
 }
@@ -368,7 +369,7 @@ static int jz4780_nand_probe(struct platform_device *pdev)
 	nfc->dev = dev;
 	nfc->num_banks = num_banks;
 
-	nand_hw_control_init(&nfc->controller);
+	nand_controller_init(&nfc->controller);
 	INIT_LIST_HEAD(&nfc->chips);
 
 	ret = jz4780_nand_init_chips(nfc, pdev);
diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 80a074cccb82..bd5f9a4b7b16 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -318,7 +318,7 @@ struct marvell_nfc_caps {
  * @dma_buf:		32-bit aligned buffer for DMA transfers (NFCv1 only)
  */
 struct marvell_nfc {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *core_clk;
@@ -335,7 +335,7 @@ struct marvell_nfc {
 	u8 *dma_buf;
 };
 
-static inline struct marvell_nfc *to_marvell_nfc(struct nand_hw_control *ctrl)
+static inline struct marvell_nfc *to_marvell_nfc(struct nand_controller *ctrl)
 {
 	return container_of(ctrl, struct marvell_nfc, controller);
 }
@@ -2745,7 +2745,7 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	nfc->dev = dev;
-	nand_hw_control_init(&nfc->controller);
+	nand_controller_init(&nfc->controller);
 	INIT_LIST_HEAD(&nfc->chips);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/drivers/mtd/nand/raw/mtk_nand.c b/drivers/mtd/nand/raw/mtk_nand.c
index 75c845adb050..254ee76d354d 100644
--- a/drivers/mtd/nand/raw/mtk_nand.c
+++ b/drivers/mtd/nand/raw/mtk_nand.c
@@ -145,7 +145,7 @@ struct mtk_nfc_clk {
 };
 
 struct mtk_nfc {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct mtk_ecc_config ecc_cfg;
 	struct mtk_nfc_clk clk;
 	struct mtk_ecc *ecc;
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 4fa5e20d9690..0aef0299f4f2 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4967,8 +4967,8 @@ static void nand_set_defaults(struct nand_chip *chip)
 		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
 
 	if (!chip->controller) {
-		chip->controller = &chip->hwcontrol;
-		nand_hw_control_init(chip->controller);
+		chip->controller = &chip->dummy_controller;
+		nand_controller_init(chip->controller);
 	}
 
 	if (!chip->buf_align)
diff --git a/drivers/mtd/nand/raw/ndfc.c b/drivers/mtd/nand/raw/ndfc.c
index d8a806894937..540fa1a0ea24 100644
--- a/drivers/mtd/nand/raw/ndfc.c
+++ b/drivers/mtd/nand/raw/ndfc.c
@@ -39,7 +39,7 @@ struct ndfc_controller {
 	void __iomem *ndfcbase;
 	struct nand_chip chip;
 	int chip_select;
-	struct nand_hw_control ndfc_control;
+	struct nand_controller ndfc_control;
 };
 
 static struct ndfc_controller ndfc_ctrl[NDFC_MAX_CS];
@@ -218,7 +218,7 @@ static int ndfc_probe(struct platform_device *ofdev)
 	ndfc = &ndfc_ctrl[cs];
 	ndfc->chip_select = cs;
 
-	nand_hw_control_init(&ndfc->ndfc_control);
+	nand_controller_init(&ndfc->ndfc_control);
 	ndfc->ofdev = ofdev;
 	dev_set_drvdata(&ofdev->dev, ndfc);
 
diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
index e50c64adc3c8..e943b2e5a5e2 100644
--- a/drivers/mtd/nand/raw/omap2.c
+++ b/drivers/mtd/nand/raw/omap2.c
@@ -145,7 +145,7 @@ static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
 static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10};
 
 /* Shared among all NAND instances to synchronize access to the ECC Engine */
-static struct nand_hw_control omap_gpmc_controller = {
+static struct nand_controller omap_gpmc_controller = {
 	.lock = __SPIN_LOCK_UNLOCKED(omap_gpmc_controller.lock),
 	.wq = __WAIT_QUEUE_HEAD_INITIALIZER(omap_gpmc_controller.wq),
 };
diff --git a/drivers/mtd/nand/raw/oxnas_nand.c b/drivers/mtd/nand/raw/oxnas_nand.c
index d649d5944826..01b00bb69c1e 100644
--- a/drivers/mtd/nand/raw/oxnas_nand.c
+++ b/drivers/mtd/nand/raw/oxnas_nand.c
@@ -32,7 +32,7 @@
 #define OXNAS_NAND_MAX_CHIPS	1
 
 struct oxnas_nand_ctrl {
-	struct nand_hw_control base;
+	struct nand_controller base;
 	void __iomem *io_base;
 	struct clk *clk;
 	struct nand_chip *chips[OXNAS_NAND_MAX_CHIPS];
@@ -96,7 +96,7 @@ static int oxnas_nand_probe(struct platform_device *pdev)
 	if (!oxnas)
 		return -ENOMEM;
 
-	nand_hw_control_init(&oxnas->base);
+	nand_controller_init(&oxnas->base);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	oxnas->io_base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 994f980c6d86..cbad0980d890 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -365,7 +365,7 @@ struct nandc_regs {
  *				from all connected NAND devices pagesize
  */
 struct qcom_nand_controller {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct list_head host_list;
 
 	struct device *dev;
@@ -2728,7 +2728,7 @@ static int qcom_nandc_alloc(struct qcom_nand_controller *nandc)
 	INIT_LIST_HEAD(&nandc->desc_list);
 	INIT_LIST_HEAD(&nandc->host_list);
 
-	nand_hw_control_init(&nandc->controller);
+	nand_controller_init(&nandc->controller);
 
 	return 0;
 }
diff --git a/drivers/mtd/nand/raw/s3c2410.c b/drivers/mtd/nand/raw/s3c2410.c
index 19661c5d3220..f7a5fc7daa57 100644
--- a/drivers/mtd/nand/raw/s3c2410.c
+++ b/drivers/mtd/nand/raw/s3c2410.c
@@ -162,7 +162,7 @@ enum s3c_nand_clk_state {
  */
 struct s3c2410_nand_info {
 	/* mtd info */
-	struct nand_hw_control		controller;
+	struct nand_controller		controller;
 	struct s3c2410_nand_mtd		*mtds;
 	struct s3c2410_platform_nand	*platform;
 
@@ -1094,7 +1094,7 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
-	nand_hw_control_init(&info->controller);
+	nand_controller_init(&info->controller);
 
 	/* get the clock source and enable it */
 
diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index d831a141a196..41bd66a9166a 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -234,7 +234,7 @@ static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand)
  *			controller events
  */
 struct sunxi_nfc {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *ahb_clk;
@@ -247,7 +247,7 @@ struct sunxi_nfc {
 	struct dma_chan *dmac;
 };
 
-static inline struct sunxi_nfc *to_sunxi_nfc(struct nand_hw_control *ctrl)
+static inline struct sunxi_nfc *to_sunxi_nfc(struct nand_controller *ctrl)
 {
 	return container_of(ctrl, struct sunxi_nfc, controller);
 }
@@ -2012,7 +2012,7 @@ static int sunxi_nfc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	nfc->dev = dev;
-	nand_hw_control_init(&nfc->controller);
+	nand_controller_init(&nfc->controller);
 	INIT_LIST_HEAD(&nfc->chips);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/drivers/mtd/nand/raw/tango_nand.c b/drivers/mtd/nand/raw/tango_nand.c
index f2052fae21c7..dd7a26efdf4f 100644
--- a/drivers/mtd/nand/raw/tango_nand.c
+++ b/drivers/mtd/nand/raw/tango_nand.c
@@ -83,7 +83,7 @@
 #define MAX_CS		4
 
 struct tango_nfc {
-	struct nand_hw_control hw;
+	struct nand_controller hw;
 	void __iomem *reg_base;
 	void __iomem *mem_base;
 	void __iomem *pbus_base;
@@ -654,7 +654,7 @@ static int tango_nand_probe(struct platform_device *pdev)
 		return PTR_ERR(nfc->chan);
 
 	platform_set_drvdata(pdev, nfc);
-	nand_hw_control_init(&nfc->hw);
+	nand_controller_init(&nfc->hw);
 	nfc->freq_kHz = clk_get_rate(clk) / 1000;
 
 	for_each_child_of_node(pdev->dev.of_node, np) {
diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
index 22d6a7f9ff80..06402c0cbd32 100644
--- a/drivers/mtd/nand/raw/tegra_nand.c
+++ b/drivers/mtd/nand/raw/tegra_nand.c
@@ -164,7 +164,7 @@
 				HWSTATUS_RBSY_VALUE(NAND_STATUS_READY))
 
 struct tegra_nand_controller {
-	struct nand_hw_control controller;
+	struct nand_controller controller;
 	struct device *dev;
 	void __iomem *regs;
 	int irq;
@@ -187,7 +187,7 @@ struct tegra_nand_chip {
 };
 
 static inline struct tegra_nand_controller *
-			to_tegra_ctrl(struct nand_hw_control *hw_ctrl)
+			to_tegra_ctrl(struct nand_controller *hw_ctrl)
 {
 	return container_of(hw_ctrl, struct tegra_nand_controller, controller);
 }
@@ -1136,7 +1136,7 @@ static int tegra_nand_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctrl->dev = &pdev->dev;
-	nand_hw_control_init(&ctrl->controller);
+	nand_controller_init(&ctrl->controller);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ctrl->regs = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/mtd/nand/raw/txx9ndfmc.c b/drivers/mtd/nand/raw/txx9ndfmc.c
index b567d212fe7d..dd232d8740a2 100644
--- a/drivers/mtd/nand/raw/txx9ndfmc.c
+++ b/drivers/mtd/nand/raw/txx9ndfmc.c
@@ -73,7 +73,7 @@ struct txx9ndfmc_drvdata {
 	void __iomem *base;
 	unsigned char hold;	/* in gbusclock */
 	unsigned char spw;	/* in gbusclock */
-	struct nand_hw_control hw_control;
+	struct nand_controller hw_control;
 };
 
 static struct platform_device *mtd_to_platdev(struct mtd_info *mtd)
@@ -303,7 +303,7 @@ static int __init txx9ndfmc_probe(struct platform_device *dev)
 	dev_info(&dev->dev, "CLK:%ldMHz HOLD:%d SPW:%d\n",
 		 (gbusclk + 500000) / 1000000, hold, spw);
 
-	nand_hw_control_init(&drvdata->hw_control);
+	nand_controller_init(&drvdata->hw_control);
 
 	platform_set_drvdata(dev, drvdata);
 	txx9ndfmc_initialize(dev);
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index e383c7f32574..eb5a053fd06b 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -510,20 +510,21 @@ struct nand_id {
 };
 
 /**
- * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
+ * struct nand_controller - Control structure for hardware controller
+ *                          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.
  */
-struct nand_hw_control {
+struct nand_controller {
 	spinlock_t lock;
 	struct nand_chip *active;
 	wait_queue_head_t wq;
 };
 
-static inline void nand_hw_control_init(struct nand_hw_control *nfc)
+static inline void nand_controller_init(struct nand_controller *nfc)
 {
 	nfc->active = NULL;
 	spin_lock_init(&nfc->lock);
@@ -1197,7 +1198,8 @@ int nand_op_parser_exec_op(struct nand_chip *chip,
  *			setting the read-retry mode. Mostly needed for MLC NAND.
  * @ecc:		[BOARDSPECIFIC] ECC control structure
  * @buf_align:		minimum buffer alignment required by a platform
- * @hwcontrol:		platform-specific hardware control structure
+ * @dummy_controller:	dummy controller implementation for drivers that can
+ *			only control a single chip
  * @erase:		[REPLACEABLE] erase function
  * @scan_bbt:		[REPLACEABLE] function to scan bad block table
  * @chip_delay:		[BOARDSPECIFIC] chip dependent delay for transferring
@@ -1334,11 +1336,11 @@ struct nand_chip {
 	flstate_t state;
 
 	uint8_t *oob_poi;
-	struct nand_hw_control *controller;
+	struct nand_controller *controller;
 
 	struct nand_ecc_ctrl ecc;
 	unsigned long buf_align;
-	struct nand_hw_control hwcontrol;
+	struct nand_controller dummy_controller;
 
 	uint8_t *bbt;
 	struct nand_bbt_descr *bbt_td;
-- 
2.14.1

[PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[Miquel Raynal]

In order to remove the limitation that forbids dynamic allocation in
nand_scan_ident(), we must create a path that will be the same for all
controller drivers. The idea is to use nand_scan() instead of the widely
used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
this, controller drivers will need to adjust some parameters between
these two functions depending on the NAND chip wired on them.

For that, a hook called ->attach_chip() is created as part of the
nand_controller_ops structure, itself embedded in the nand_controller
structure. The later may be referenced by two ways:
1/ if the driver does not implement its own controller, the
   chip->controller hook is not populated before nand_scan() so it
   cannot be dereferenced: use chip->dummy_controller instead (which is
   statically allocated and will be referenced later by chip->controller
   anyway).
2/ through chip->controller if the driver implements its own controller.

Another hook, ->detach_chip() is also introduced in order to clean the
controller driver's potential allocations in case of failure of
nand_scan_tail(). There is no need for the controller driver to call the
->detach_chip() hook directly upon error after a successful nand_scan().
In this situation, calling nand_release() as before is enough.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 0aef0299f4f2..a64a344bf7a9 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
 	is_module_text_address((unsigned long)__builtin_return_address(0))
 #endif
 
+static int nand_attach(struct nand_chip *chip)
+{
+	if (chip->controller->ops && chip->controller->ops->attach_chip)
+		return chip->controller->ops->attach_chip(chip);
+
+	return 0;
+}
+
+static void nand_detach(struct nand_chip *chip)
+{
+	if (chip->controller->ops && chip->controller->ops->detach_chip)
+		chip->controller->ops->detach_chip(chip);
+}
+
 /**
  * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
  * @mtd: MTD device structure
@@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
 int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
 		       struct nand_flash_dev *ids)
 {
+	struct nand_chip *chip = mtd_to_nand(mtd);
 	int ret;
 
 	ret = nand_scan_ident(mtd, maxchips, ids);
-	if (!ret)
-		ret = nand_scan_tail(mtd);
+	if (ret)
+		return ret;
+
+	ret = nand_attach(chip);
+	if (ret)
+		return ret;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		nand_detach(chip);
+
 	return ret;
 }
 EXPORT_SYMBOL(nand_scan_with_ids);
@@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
 
 	/* Free manufacturer priv data. */
 	nand_manufacturer_cleanup(chip);
+
+	/* Free controller specific allocations after chip identification */
+	nand_detach(chip);
 }
+
 EXPORT_SYMBOL_GPL(nand_cleanup);
 
 /**
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index eb5a053fd06b..d046b3a61205 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -509,6 +509,19 @@ struct nand_id {
 	int len;
 };
 
+/**
+ * struct nand_controller_ops - Controller operations
+ *
+ * @attach_chip:	Callback that may be called between nand_detect() and
+ *			nand_scan_tail() during nand_scan() (optional).
+ * @detach_chip:	Callback that may be called if nand_scan_tail() fails
+ *			(optional).
+ */
+struct nand_controller_ops {
+	int (*attach_chip)(struct nand_chip *chip);
+	void (*detach_chip)(struct nand_chip *chip);
+};
+
 /**
  * struct nand_controller - Control structure for hardware controller
  *                          shared among independent devices
@@ -517,11 +530,13 @@ struct nand_id {
  * @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:		NAND controller operations.
  */
 struct nand_controller {
 	spinlock_t lock;
 	struct nand_chip *active;
 	wait_queue_head_t wq;
+	const struct nand_controller_ops *ops;
 };
 
 static inline void nand_controller_init(struct nand_controller *nfc)
@@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)
 	nfc->active = NULL;
 	spin_lock_init(&nfc->lock);
 	init_waitqueue_head(&nfc->wq);
+	nfc->ops = NULL;
 }
 
 /**
-- 
2.14.1

Re: [PATCH v2 1/2] mtd: rawnand: better name for the controller structure

[Boris Brezillon]

On Wed, 18 Jul 2018 23:50:55 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>  
>  /**
> - * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
> + * struct nand_controller - Control structure for hardware controller

Hm, the description is weird. How about "NAND controller object" or
"Structure used to describe a NAND controller"

Other than this detail, it looks all good to me.

> + *                          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.
>   */
> -struct nand_hw_control {
> +struct nand_controller {
>  	spinlock_t lock;
>  	struct nand_chip *active;
>  	wait_queue_head_t wq;
>  };

Re: [PATCH v2 1/2] mtd: rawnand: better name for the controller structure

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Wed, 18 Jul 2018
23:58:45 +0200:

> On Wed, 18 Jul 2018 23:50:55 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >  
> >  /**
> > - * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
> > + * struct nand_controller - Control structure for hardware controller  
> 
> Hm, the description is weird. How about "NAND controller object" or
> "Structure used to describe a NAND controller"
> 
> Other than this detail, it looks all good to me.

That's right I don't like it neither. I just removed the "e.g ECC
generator" which was wrong but it deserves a better definition. I'll
stick to "Structure used to describe a NAND controller" which is clear
enough for me.

I'll fix when applying.

Thanks,
Miquèl

Re: [PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[Boris Brezillon]

On Wed, 18 Jul 2018 23:50:56 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> In order to remove the limitation that forbids dynamic allocation in
> nand_scan_ident(), we must create a path that will be the same for all
> controller drivers. The idea is to use nand_scan() instead of the widely
> used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
> this, controller drivers will need to adjust some parameters between
> these two functions depending on the NAND chip wired on them.
> 

I'd reword a bit the following paragraphs.

"
This takes the form of two new hooks (->{attach,detach}_chip()) that are
placed in a new nand_controller_ops structure, which is then attached
to the nand_controller object at driver initialization time.
->attach_chip() is called between nand_scan_ident() and
nand_scan_tail(), and ->detach_chip() is called in the error path of
nand_scan() and in nand_cleanup().

Note that some NAND controller drivers don't have a dedicated
nand_controller object and instead rely on the default/dummy one
embedded in nand_chip. If you're in this case and still want to
initialize the controller ops, you'll have to manipulate
chip->dummy_controller directly.

Last but not least, it's worth mentioning that we plan to move some of
the controller related hooks placed in nand_chip into
nand_controller_ops to make the separation between NAND chip and NAND
controller methods clearer.
"

> For that, a hook called ->attach_chip() is created as part of the
> nand_controller_ops structure, itself embedded in the nand_controller
> structure. The later may be referenced by two ways:
> 1/ if the driver does not implement its own controller, the
>    chip->controller hook is not populated before nand_scan() so it
>    cannot be dereferenced: use chip->dummy_controller instead (which is
>    statically allocated and will be referenced later by chip->controller
>    anyway).
> 2/ through chip->controller if the driver implements its own controller.
> 
> Another hook, ->detach_chip() is also introduced in order to clean the
> controller driver's potential allocations in case of failure of
> nand_scan_tail(). There is no need for the controller driver to call the
> ->detach_chip() hook directly upon error after a successful nand_scan().  
> In this situation, calling nand_release() as before is enough.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
>  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
>  2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 0aef0299f4f2..a64a344bf7a9 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
>  	is_module_text_address((unsigned long)__builtin_return_address(0))
>  #endif
>  
> +static int nand_attach(struct nand_chip *chip)
> +{
> +	if (chip->controller->ops && chip->controller->ops->attach_chip)
> +		return chip->controller->ops->attach_chip(chip);
> +
> +	return 0;
> +}
> +
> +static void nand_detach(struct nand_chip *chip)
> +{
> +	if (chip->controller->ops && chip->controller->ops->detach_chip)
> +		chip->controller->ops->detach_chip(chip);
> +}
> +
>  /**
>   * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
>   * @mtd: MTD device structure
> @@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
>  int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
>  		       struct nand_flash_dev *ids)
>  {
> +	struct nand_chip *chip = mtd_to_nand(mtd);
>  	int ret;
>  
>  	ret = nand_scan_ident(mtd, maxchips, ids);
> -	if (!ret)
> -		ret = nand_scan_tail(mtd);
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_attach(chip);
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_scan_tail(mtd);
> +	if (ret)
> +		nand_detach(chip);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_with_ids);
> @@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
>  
>  	/* Free manufacturer priv data. */
>  	nand_manufacturer_cleanup(chip);
> +
> +	/* Free controller specific allocations after chip identification */
> +	nand_detach(chip);
>  }
> +
>  EXPORT_SYMBOL_GPL(nand_cleanup);
>  
>  /**
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index eb5a053fd06b..d046b3a61205 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -509,6 +509,19 @@ struct nand_id {
>  	int len;
>  };
>  
> +/**
> + * struct nand_controller_ops - Controller operations

				   NAND controller operations

> + *
> + * @attach_chip:	Callback that may be called between nand_detect() and

				      ^ will

Just nitpicking, but I'm not a big fan of tab-based alignment, because
every time you add a new field whose name is longer than what you
initially planned, either you screw up the alignment, or you have to
re-align all other fields. If you just use a single space, at least
there's no alignment expectations to start with :-).

> + *			nand_scan_tail() during nand_scan() (optional).
> + * @detach_chip:	Callback that may be called if nand_scan_tail() fails

				      ^ will

And it's also called from nand_cleanup().

> + *			(optional).
> + */
> +struct nand_controller_ops {
> +	int (*attach_chip)(struct nand_chip *chip);
> +	void (*detach_chip)(struct nand_chip *chip);
> +};
> +
>  /**
>   * struct nand_controller - Control structure for hardware controller
>   *                          shared among independent devices
> @@ -517,11 +530,13 @@ struct nand_id {
>   * @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:		NAND controller operations.
>   */
>  struct nand_controller {
>  	spinlock_t lock;
>  	struct nand_chip *active;
>  	wait_queue_head_t wq;
> +	const struct nand_controller_ops *ops;
>  };
>  
>  static inline void nand_controller_init(struct nand_controller *nfc)
> @@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)

It's probably better to pass ops as a second argument here, or, if you
don't want to patch all drivers calling nand_controller_init() again,
create a nand_controller_init_with_ops() function and then turn
nand_controller_init() into a wrapper that calls
nand_controller_init_with_ops(nfc, NULL). 

>  	nfc->active = NULL;
>  	spin_lock_init(&nfc->lock);
>  	init_waitqueue_head(&nfc->wq);
> +	nfc->ops = NULL;
>  }
>  
>  /**

Re: [PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[Boris Brezillon]

On Wed, 18 Jul 2018 23:50:56 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> In order to remove the limitation that forbids dynamic allocation in
> nand_scan_ident(), we must create a path that will be the same for all
> controller drivers. The idea is to use nand_scan() instead of the widely
> used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
> this, controller drivers will need to adjust some parameters between
> these two functions depending on the NAND chip wired on them.
> 
> For that, a hook called ->attach_chip() is created as part of the
> nand_controller_ops structure, itself embedded in the nand_controller
> structure. The later may be referenced by two ways:
> 1/ if the driver does not implement its own controller, the
>    chip->controller hook is not populated before nand_scan() so it
>    cannot be dereferenced: use chip->dummy_controller instead (which is
>    statically allocated and will be referenced later by chip->controller
>    anyway).
> 2/ through chip->controller if the driver implements its own controller.
> 
> Another hook, ->detach_chip() is also introduced in order to clean the
> controller driver's potential allocations in case of failure of
> nand_scan_tail(). There is no need for the controller driver to call the
> ->detach_chip() hook directly upon error after a successful nand_scan().  
> In this situation, calling nand_release() as before is enough.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
>  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
>  2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 0aef0299f4f2..a64a344bf7a9 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
>  	is_module_text_address((unsigned long)__builtin_return_address(0))
>  #endif
>  
> +static int nand_attach(struct nand_chip *chip)
> +{
> +	if (chip->controller->ops && chip->controller->ops->attach_chip)
> +		return chip->controller->ops->attach_chip(chip);
> +
> +	return 0;
> +}
> +
> +static void nand_detach(struct nand_chip *chip)
> +{
> +	if (chip->controller->ops && chip->controller->ops->detach_chip)
> +		chip->controller->ops->detach_chip(chip);
> +}
> +
>  /**
>   * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
>   * @mtd: MTD device structure
> @@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
>  int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
>  		       struct nand_flash_dev *ids)
>  {
> +	struct nand_chip *chip = mtd_to_nand(mtd);
>  	int ret;
>  
>  	ret = nand_scan_ident(mtd, maxchips, ids);
> -	if (!ret)
> -		ret = nand_scan_tail(mtd);
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_attach(chip);
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_scan_tail(mtd);
> +	if (ret)
> +		nand_detach(chip);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_with_ids);
> @@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
>  
>  	/* Free manufacturer priv data. */
>  	nand_manufacturer_cleanup(chip);
> +
> +	/* Free controller specific allocations after chip identification */
> +	nand_detach(chip);
>  }
> +
>  EXPORT_SYMBOL_GPL(nand_cleanup);
>  
>  /**
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index eb5a053fd06b..d046b3a61205 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -509,6 +509,19 @@ struct nand_id {
>  	int len;
>  };
>  
> +/**
> + * struct nand_controller_ops - Controller operations
> + *
> + * @attach_chip:	Callback that may be called between nand_detect() and
> + *			nand_scan_tail() during nand_scan() (optional).
> + * @detach_chip:	Callback that may be called if nand_scan_tail() fails
> + *			(optional).
> + */
> +struct nand_controller_ops {
> +	int (*attach_chip)(struct nand_chip *chip);
> +	void (*detach_chip)(struct nand_chip *chip);
> +};
> +
>  /**
>   * struct nand_controller - Control structure for hardware controller
>   *                          shared among independent devices
> @@ -517,11 +530,13 @@ struct nand_id {
>   * @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:		NAND controller operations.
>   */
>  struct nand_controller {
>  	spinlock_t lock;
>  	struct nand_chip *active;
>  	wait_queue_head_t wq;
> +	const struct nand_controller_ops *ops;
>  };
>  
>  static inline void nand_controller_init(struct nand_controller *nfc)
> @@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)
>  	nfc->active = NULL;
>  	spin_lock_init(&nfc->lock);
>  	init_waitqueue_head(&nfc->wq);
> +	nfc->ops = NULL;

Hm, actually it does not work for all dummy controllers since this
function is called from nand_set_defaults() which is called from
nand_scan_ident(). That means the previous chip->dummy_controller.ops
assignment you might have done before calling nand_scan() is gone.

Looks like we'll have to patch all drivers to call
nand_controller_init(&chip->dummy_controller), unless you see a better
option.

>  }
>  
>  /**

Re: [PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[Boris Brezillon]

On Thu, 19 Jul 2018 00:42:25 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Wed, 18 Jul 2018 23:50:56 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > In order to remove the limitation that forbids dynamic allocation in
> > nand_scan_ident(), we must create a path that will be the same for all
> > controller drivers. The idea is to use nand_scan() instead of the widely
> > used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
> > this, controller drivers will need to adjust some parameters between
> > these two functions depending on the NAND chip wired on them.
> > 
> > For that, a hook called ->attach_chip() is created as part of the
> > nand_controller_ops structure, itself embedded in the nand_controller
> > structure. The later may be referenced by two ways:
> > 1/ if the driver does not implement its own controller, the
> >    chip->controller hook is not populated before nand_scan() so it
> >    cannot be dereferenced: use chip->dummy_controller instead (which is
> >    statically allocated and will be referenced later by chip->controller
> >    anyway).
> > 2/ through chip->controller if the driver implements its own controller.
> > 
> > Another hook, ->detach_chip() is also introduced in order to clean the
> > controller driver's potential allocations in case of failure of
> > nand_scan_tail(). There is no need for the controller driver to call the  
> > ->detach_chip() hook directly upon error after a successful nand_scan().    
> > In this situation, calling nand_release() as before is enough.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
> >  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
> >  2 files changed, 46 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 0aef0299f4f2..a64a344bf7a9 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
> >  	is_module_text_address((unsigned long)__builtin_return_address(0))
> >  #endif
> >  
> > +static int nand_attach(struct nand_chip *chip)
> > +{
> > +	if (chip->controller->ops && chip->controller->ops->attach_chip)
> > +		return chip->controller->ops->attach_chip(chip);
> > +
> > +	return 0;
> > +}
> > +
> > +static void nand_detach(struct nand_chip *chip)
> > +{
> > +	if (chip->controller->ops && chip->controller->ops->detach_chip)
> > +		chip->controller->ops->detach_chip(chip);
> > +}
> > +
> >  /**
> >   * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
> >   * @mtd: MTD device structure
> > @@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
> >  int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
> >  		       struct nand_flash_dev *ids)
> >  {
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> >  	int ret;
> >  
> >  	ret = nand_scan_ident(mtd, maxchips, ids);
> > -	if (!ret)
> > -		ret = nand_scan_tail(mtd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = nand_attach(chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = nand_scan_tail(mtd);
> > +	if (ret)
> > +		nand_detach(chip);
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(nand_scan_with_ids);
> > @@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
> >  
> >  	/* Free manufacturer priv data. */
> >  	nand_manufacturer_cleanup(chip);
> > +
> > +	/* Free controller specific allocations after chip identification */
> > +	nand_detach(chip);
> >  }
> > +
> >  EXPORT_SYMBOL_GPL(nand_cleanup);
> >  
> >  /**
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index eb5a053fd06b..d046b3a61205 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -509,6 +509,19 @@ struct nand_id {
> >  	int len;
> >  };
> >  
> > +/**
> > + * struct nand_controller_ops - Controller operations
> > + *
> > + * @attach_chip:	Callback that may be called between nand_detect() and
> > + *			nand_scan_tail() during nand_scan() (optional).
> > + * @detach_chip:	Callback that may be called if nand_scan_tail() fails
> > + *			(optional).
> > + */
> > +struct nand_controller_ops {
> > +	int (*attach_chip)(struct nand_chip *chip);
> > +	void (*detach_chip)(struct nand_chip *chip);
> > +};
> > +
> >  /**
> >   * struct nand_controller - Control structure for hardware controller
> >   *                          shared among independent devices
> > @@ -517,11 +530,13 @@ struct nand_id {
> >   * @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:		NAND controller operations.
> >   */
> >  struct nand_controller {
> >  	spinlock_t lock;
> >  	struct nand_chip *active;
> >  	wait_queue_head_t wq;
> > +	const struct nand_controller_ops *ops;
> >  };
> >  
> >  static inline void nand_controller_init(struct nand_controller *nfc)
> > @@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)
> >  	nfc->active = NULL;
> >  	spin_lock_init(&nfc->lock);
> >  	init_waitqueue_head(&nfc->wq);
> > +	nfc->ops = NULL;  
> 
> Hm, actually it does not work for all dummy controllers since this
> function is called from nand_set_defaults() which is called from
> nand_scan_ident(). That means the previous chip->dummy_controller.ops
> assignment you might have done before calling nand_scan() is gone.
> 
> Looks like we'll have to patch all drivers to call
> nand_controller_init(&chip->dummy_controller), unless you see a better
> option.

Alternatively, we could assume that the nand_controller object is
always zeroed at allocation time, and simply not assign nfc->ops from
nand_controller_init() so that dummy controller drivers that need to
pass non-NULL ops can do so before calling nand_scan().

Re: [PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 19 Jul 2018
00:42:25 +0200:

> On Wed, 18 Jul 2018 23:50:56 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > In order to remove the limitation that forbids dynamic allocation in
> > nand_scan_ident(), we must create a path that will be the same for all
> > controller drivers. The idea is to use nand_scan() instead of the widely
> > used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
> > this, controller drivers will need to adjust some parameters between
> > these two functions depending on the NAND chip wired on them.
> > 
> > For that, a hook called ->attach_chip() is created as part of the
> > nand_controller_ops structure, itself embedded in the nand_controller
> > structure. The later may be referenced by two ways:
> > 1/ if the driver does not implement its own controller, the
> >    chip->controller hook is not populated before nand_scan() so it
> >    cannot be dereferenced: use chip->dummy_controller instead (which is
> >    statically allocated and will be referenced later by chip->controller
> >    anyway).
> > 2/ through chip->controller if the driver implements its own controller.
> > 
> > Another hook, ->detach_chip() is also introduced in order to clean the
> > controller driver's potential allocations in case of failure of
> > nand_scan_tail(). There is no need for the controller driver to call the  
> > ->detach_chip() hook directly upon error after a successful nand_scan().    
> > In this situation, calling nand_release() as before is enough.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
> >  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
> >  2 files changed, 46 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 0aef0299f4f2..a64a344bf7a9 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
> >  	is_module_text_address((unsigned long)__builtin_return_address(0))
> >  #endif
> >  
> > +static int nand_attach(struct nand_chip *chip)
> > +{
> > +	if (chip->controller->ops && chip->controller->ops->attach_chip)
> > +		return chip->controller->ops->attach_chip(chip);
> > +
> > +	return 0;
> > +}
> > +
> > +static void nand_detach(struct nand_chip *chip)
> > +{
> > +	if (chip->controller->ops && chip->controller->ops->detach_chip)
> > +		chip->controller->ops->detach_chip(chip);
> > +}
> > +
> >  /**
> >   * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
> >   * @mtd: MTD device structure
> > @@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
> >  int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
> >  		       struct nand_flash_dev *ids)
> >  {
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> >  	int ret;
> >  
> >  	ret = nand_scan_ident(mtd, maxchips, ids);
> > -	if (!ret)
> > -		ret = nand_scan_tail(mtd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = nand_attach(chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = nand_scan_tail(mtd);
> > +	if (ret)
> > +		nand_detach(chip);
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(nand_scan_with_ids);
> > @@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
> >  
> >  	/* Free manufacturer priv data. */
> >  	nand_manufacturer_cleanup(chip);
> > +
> > +	/* Free controller specific allocations after chip identification */
> > +	nand_detach(chip);
> >  }
> > +
> >  EXPORT_SYMBOL_GPL(nand_cleanup);
> >  
> >  /**
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index eb5a053fd06b..d046b3a61205 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -509,6 +509,19 @@ struct nand_id {
> >  	int len;
> >  };
> >  
> > +/**
> > + * struct nand_controller_ops - Controller operations
> > + *
> > + * @attach_chip:	Callback that may be called between nand_detect() and
> > + *			nand_scan_tail() during nand_scan() (optional).
> > + * @detach_chip:	Callback that may be called if nand_scan_tail() fails
> > + *			(optional).
> > + */
> > +struct nand_controller_ops {
> > +	int (*attach_chip)(struct nand_chip *chip);
> > +	void (*detach_chip)(struct nand_chip *chip);
> > +};
> > +
> >  /**
> >   * struct nand_controller - Control structure for hardware controller
> >   *                          shared among independent devices
> > @@ -517,11 +530,13 @@ struct nand_id {
> >   * @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:		NAND controller operations.
> >   */
> >  struct nand_controller {
> >  	spinlock_t lock;
> >  	struct nand_chip *active;
> >  	wait_queue_head_t wq;
> > +	const struct nand_controller_ops *ops;
> >  };
> >  
> >  static inline void nand_controller_init(struct nand_controller *nfc)
> > @@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)
> >  	nfc->active = NULL;
> >  	spin_lock_init(&nfc->lock);
> >  	init_waitqueue_head(&nfc->wq);
> > +	nfc->ops = NULL;  
> 
> Hm, actually it does not work for all dummy controllers since this
> function is called from nand_set_defaults() which is called from
> nand_scan_ident(). That means the previous chip->dummy_controller.ops
> assignment you might have done before calling nand_scan() is gone.
> 
> Looks like we'll have to patch all drivers to call
> nand_controller_init(&chip->dummy_controller), unless you see a better
> option.

Well, if all drivers that allocate the nand_chip structure
initialize it correctly (or use kzalloc for this purpose), then this
extra initialization is not needed.

Miquèl

Re: [PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 19 Jul 2018
00:46:36 +0200:

> On Thu, 19 Jul 2018 00:42:25 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
> > On Wed, 18 Jul 2018 23:50:56 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > In order to remove the limitation that forbids dynamic allocation in
> > > nand_scan_ident(), we must create a path that will be the same for all
> > > controller drivers. The idea is to use nand_scan() instead of the widely
> > > used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
> > > this, controller drivers will need to adjust some parameters between
> > > these two functions depending on the NAND chip wired on them.
> > > 
> > > For that, a hook called ->attach_chip() is created as part of the
> > > nand_controller_ops structure, itself embedded in the nand_controller
> > > structure. The later may be referenced by two ways:
> > > 1/ if the driver does not implement its own controller, the
> > >    chip->controller hook is not populated before nand_scan() so it
> > >    cannot be dereferenced: use chip->dummy_controller instead (which is
> > >    statically allocated and will be referenced later by chip->controller
> > >    anyway).
> > > 2/ through chip->controller if the driver implements its own controller.
> > > 
> > > Another hook, ->detach_chip() is also introduced in order to clean the
> > > controller driver's potential allocations in case of failure of
> > > nand_scan_tail(). There is no need for the controller driver to call the    
> > > ->detach_chip() hook directly upon error after a successful nand_scan().      
> > > In this situation, calling nand_release() as before is enough.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
> > >  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
> > >  2 files changed, 46 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > > index 0aef0299f4f2..a64a344bf7a9 100644
> > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > @@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
> > >  	is_module_text_address((unsigned long)__builtin_return_address(0))
> > >  #endif
> > >  
> > > +static int nand_attach(struct nand_chip *chip)
> > > +{
> > > +	if (chip->controller->ops && chip->controller->ops->attach_chip)
> > > +		return chip->controller->ops->attach_chip(chip);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void nand_detach(struct nand_chip *chip)
> > > +{
> > > +	if (chip->controller->ops && chip->controller->ops->detach_chip)
> > > +		chip->controller->ops->detach_chip(chip);
> > > +}
> > > +
> > >  /**
> > >   * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
> > >   * @mtd: MTD device structure
> > > @@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
> > >  int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
> > >  		       struct nand_flash_dev *ids)
> > >  {
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > >  	int ret;
> > >  
> > >  	ret = nand_scan_ident(mtd, maxchips, ids);
> > > -	if (!ret)
> > > -		ret = nand_scan_tail(mtd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = nand_attach(chip);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = nand_scan_tail(mtd);
> > > +	if (ret)
> > > +		nand_detach(chip);
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL(nand_scan_with_ids);
> > > @@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
> > >  
> > >  	/* Free manufacturer priv data. */
> > >  	nand_manufacturer_cleanup(chip);
> > > +
> > > +	/* Free controller specific allocations after chip identification */
> > > +	nand_detach(chip);
> > >  }
> > > +
> > >  EXPORT_SYMBOL_GPL(nand_cleanup);
> > >  
> > >  /**
> > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > > index eb5a053fd06b..d046b3a61205 100644
> > > --- a/include/linux/mtd/rawnand.h
> > > +++ b/include/linux/mtd/rawnand.h
> > > @@ -509,6 +509,19 @@ struct nand_id {
> > >  	int len;
> > >  };
> > >  
> > > +/**
> > > + * struct nand_controller_ops - Controller operations
> > > + *
> > > + * @attach_chip:	Callback that may be called between nand_detect() and
> > > + *			nand_scan_tail() during nand_scan() (optional).
> > > + * @detach_chip:	Callback that may be called if nand_scan_tail() fails
> > > + *			(optional).
> > > + */
> > > +struct nand_controller_ops {
> > > +	int (*attach_chip)(struct nand_chip *chip);
> > > +	void (*detach_chip)(struct nand_chip *chip);
> > > +};
> > > +
> > >  /**
> > >   * struct nand_controller - Control structure for hardware controller
> > >   *                          shared among independent devices
> > > @@ -517,11 +530,13 @@ struct nand_id {
> > >   * @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:		NAND controller operations.
> > >   */
> > >  struct nand_controller {
> > >  	spinlock_t lock;
> > >  	struct nand_chip *active;
> > >  	wait_queue_head_t wq;
> > > +	const struct nand_controller_ops *ops;
> > >  };
> > >  
> > >  static inline void nand_controller_init(struct nand_controller *nfc)
> > > @@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)
> > >  	nfc->active = NULL;
> > >  	spin_lock_init(&nfc->lock);
> > >  	init_waitqueue_head(&nfc->wq);
> > > +	nfc->ops = NULL;    
> > 
> > Hm, actually it does not work for all dummy controllers since this
> > function is called from nand_set_defaults() which is called from
> > nand_scan_ident(). That means the previous chip->dummy_controller.ops
> > assignment you might have done before calling nand_scan() is gone.
> > 
> > Looks like we'll have to patch all drivers to call
> > nand_controller_init(&chip->dummy_controller), unless you see a better
> > option.  
> 
> Alternatively, we could assume that the nand_controller object is
> always zeroed at allocation time, and simply not assign nfc->ops from
> nand_controller_init() so that dummy controller drivers that need to
> pass non-NULL ops can do so before calling nand_scan().

Exactly what I just answered, then I'll just remove this line and we
should be good.

Thanks,
Miquèl