[PATCH 0/9] BCMA support for brcmnand

[PATCH 0/9] BCMA support for brcmnand

[Florian Fainelli]

Hi all,

This patch series adds support for the BRCMNAND controller revision 3.4
embedded in MIPS-based SoCs such as 5357, typically found in the Netgear
WNR3500L v2 and other kinds of Wi-Fi routers. The upstream platform that
uses this controller is under arch/mips/bcm47xx/ and does not use Device
Tree (and probably never will by now). BCMA (Broadcom AMBA) is a special
kind of discoverable memory mapped interface which requires the use of
special accessors to read from/write to the hardware block.

The integration of brcmnand into that SoC is a bit quirky in that every
register offering byte level data about the flash (OOB, device ID, etc.)
requires byte swapping. The command shift should also have been 24, but
is in fact 0, took me a while to understand why no reads were actually
working because of that.

This has been tested with Linux 5.10.82 and Linus' master with OpenWrt
and confirmed that the squashfs + jffs2 overlay that OpenWrt creates is
entirely functional and that written data is made persistent.

There are a number of open questions:

- the flash that is used is a s34ml01g100tf100 part which is said to be
  ONFI 1.0 compliant however the ONFI signature is not ONFI, but the
  device ID of the flash as if read with address 0, is that a known
issue?

- because no ONFI detection is taking place we fallback to the
  identification table matching, but there is no default ECC
  size/strength filed by the rawnand framework, probably made largely
  worse by the absence of a suitable Device Tree information?


Florian Fainelli (9):
  mtd: rawnand: brcmnand: Allow SoC to provide I/O operations
  mtd: rawnand: brcmnand: Assign soc as early as possible
  mtd: rawnand: brcmnand: Avoid pdev in brcmnand_init_cs()
  mtd: rawnand: brcmnand: Move OF operations out of brcmnand_init_cs()
  mtd: rawnand: brcmnand: Allow working without interrupts
  mtd: rawnand: brcmnand: Add platform data structure for BCMA
  mtd: rawnand: brcmnand: Allow platform data instantation
  mtd: rawnand: brcmnand: BCMA controller uses command shift of 0
  mtd: rawnand: brcmnand: Add BCMA shim

 MAINTAINERS                                 |   1 +
 drivers/bcma/driver_chipcommon_nflash.c     |  17 ++-
 drivers/mtd/nand/raw/Kconfig                |  11 ++
 drivers/mtd/nand/raw/brcmnand/Makefile      |   2 +
 drivers/mtd/nand/raw/brcmnand/bcma_nand.c   | 131 ++++++++++++++++++
 drivers/mtd/nand/raw/brcmnand/brcmnand.c    | 142 +++++++++++++-------
 drivers/mtd/nand/raw/brcmnand/brcmnand.h    |  23 ++++
 include/linux/bcma/bcma_driver_chipcommon.h |   5 +
 include/linux/platform_data/brcmnand.h      |  12 ++
 9 files changed, 291 insertions(+), 53 deletions(-)
 create mode 100644 drivers/mtd/nand/raw/brcmnand/bcma_nand.c
 create mode 100644 include/linux/platform_data/brcmnand.h

-- 
2.25.1

[PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Florian Fainelli]

Allow a brcmnand_soc instance to provide a custom set of I/O operations
which we will require when using this driver on a BCMA bus which is not
directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
to use the SoC operations if provided.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
 drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..7a1673b1b1af 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -594,13 +594,18 @@ enum {
 
 static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
 {
+	if (brcmnand_soc_has_ops(ctrl->soc))
+		return brcmnand_soc_read(ctrl->soc, offs);
 	return brcmnand_readl(ctrl->nand_base + offs);
 }
 
 static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
 				 u32 val)
 {
-	brcmnand_writel(val, ctrl->nand_base + offs);
+	if (brcmnand_soc_has_ops(ctrl->soc))
+		brcmnand_soc_write(ctrl->soc, val, offs);
+	else
+		brcmnand_writel(val, ctrl->nand_base + offs);
 }
 
 static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
@@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,
 
 static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)
 {
+	if (brcmnand_soc_has_ops(ctrl->soc))
+		return brcmnand_soc_read(ctrl->soc, ~0);
 	return __raw_readl(ctrl->nand_fc + word * 4);
 }
 
 static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
 				     int word, u32 val)
 {
-	__raw_writel(val, ctrl->nand_fc + word * 4);
+	if (brcmnand_soc_has_ops(ctrl->soc))
+		brcmnand_soc_write(ctrl->soc, val, ~0);
+	else
+		__raw_writel(val, ctrl->nand_fc + word * 4);
 }
 
 static inline void edu_writel(struct brcmnand_controller *ctrl,
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
index eb498fbe505e..a3f2ad5f6572 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
@@ -11,12 +11,19 @@
 
 struct platform_device;
 struct dev_pm_ops;
+struct brcmnand_io_ops;
 
 struct brcmnand_soc {
 	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
 	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
 	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
 				 bool is_param);
+	const struct brcmnand_io_ops *ops;
+};
+
+struct brcmnand_io_ops {
+	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
+	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
 };
 
 static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,
@@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
 		writel_relaxed(val, addr);
 }
 
+static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)
+{
+	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
+}
+
+static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
+{
+	return soc->ops->read_reg(soc, offset);
+}
+
+static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
+				      u32 offset)
+{
+	soc->ops->write_reg(soc, val, offset);
+}
+
 int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc);
 int brcmnand_remove(struct platform_device *pdev);
 
-- 
2.25.1

[PATCH 2/9] mtd: rawnand: brcmnand: Assign soc as early as possible

[Florian Fainelli]

In order to key off the brcmnand_probe() code in subsequent changes
depending upon ctlr->soc, assign that variable as early as possible,
instead of much later when we have checked that it is non-NULL.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 7a1673b1b1af..fcea5a7443e8 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -3008,6 +3008,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 
 	dev_set_drvdata(dev, ctrl);
 	ctrl->dev = dev;
+	ctrl->soc = soc;
 
 	init_completion(&ctrl->done);
 	init_completion(&ctrl->dma_done);
@@ -3148,8 +3149,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	 * interesting ways
 	 */
 	if (soc) {
-		ctrl->soc = soc;
-
 		ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
 				       DRV_NAME, ctrl);
 
-- 
2.25.1

[PATCH 3/9] mtd: rawnand: brcmnand: Avoid pdev in brcmnand_init_cs()

[Florian Fainelli]

In preparation for encapsulation more of what the loop calling
brcmnand_init_cs() does, avoid using platform_device when it is the
device behind platform_device that we are using for printing errors.

No functional change.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index fcea5a7443e8..35f8d8e02d4a 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2763,7 +2763,7 @@ static const struct nand_controller_ops brcmnand_controller_ops = {
 static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
 {
 	struct brcmnand_controller *ctrl = host->ctrl;
-	struct platform_device *pdev = host->pdev;
+	struct device *dev = ctrl->dev;
 	struct mtd_info *mtd;
 	struct nand_chip *chip;
 	int ret;
@@ -2771,7 +2771,7 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
 
 	ret = of_property_read_u32(dn, "reg", &host->cs);
 	if (ret) {
-		dev_err(&pdev->dev, "can't get chip-select\n");
+		dev_err(dev, "can't get chip-select\n");
 		return -ENXIO;
 	}
 
@@ -2780,13 +2780,13 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
 
 	nand_set_flash_node(chip, dn);
 	nand_set_controller_data(chip, host);
-	mtd->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "brcmnand.%d",
+	mtd->name = devm_kasprintf(dev, GFP_KERNEL, "brcmnand.%d",
 				   host->cs);
 	if (!mtd->name)
 		return -ENOMEM;
 
 	mtd->owner = THIS_MODULE;
-	mtd->dev.parent = &pdev->dev;
+	mtd->dev.parent = dev;
 
 	chip->legacy.cmd_ctrl = brcmnand_cmd_ctrl;
 	chip->legacy.cmdfunc = brcmnand_cmdfunc;
-- 
2.25.1

[PATCH 4/9] mtd: rawnand: brcmnand: Move OF operations out of brcmnand_init_cs()

[Florian Fainelli]

In order to initialize a given chip select object for use by the
brcmnand driver, move all of the Device Tree specific routines outside
of brcmnand_init_cs() in order to make it usable in a platform data
configuration which will be necessary for supporting BCMA chips.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 35f8d8e02d4a..60a7f375df83 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2760,7 +2760,7 @@ static const struct nand_controller_ops brcmnand_controller_ops = {
 	.attach_chip = brcmnand_attach_chip,
 };
 
-static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
+static int brcmnand_init_cs(struct brcmnand_host *host)
 {
 	struct brcmnand_controller *ctrl = host->ctrl;
 	struct device *dev = ctrl->dev;
@@ -2769,16 +2769,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
 	int ret;
 	u16 cfg_offs;
 
-	ret = of_property_read_u32(dn, "reg", &host->cs);
-	if (ret) {
-		dev_err(dev, "can't get chip-select\n");
-		return -ENXIO;
-	}
-
 	mtd = nand_to_mtd(&host->chip);
 	chip = &host->chip;
 
-	nand_set_flash_node(chip, dn);
 	nand_set_controller_data(chip, host);
 	mtd->name = devm_kasprintf(dev, GFP_KERNEL, "brcmnand.%d",
 				   host->cs);
@@ -3179,7 +3172,16 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 			host->pdev = pdev;
 			host->ctrl = ctrl;
 
-			ret = brcmnand_init_cs(host, child);
+			ret = of_property_read_u32(dn, "reg", &host->cs);
+			if (ret) {
+				dev_err(dev, "can't get chip-select\n");
+				devm_kfree(dev, host);
+				continue;
+			}
+
+			nand_set_flash_node(&host->chip, dn);
+
+			ret = brcmnand_init_cs(host);
 			if (ret) {
 				devm_kfree(dev, host);
 				continue; /* Try all chip-selects */
-- 
2.25.1

[PATCH 5/9] mtd: rawnand: brcmnand: Allow working without interrupts

[Florian Fainelli]

The BCMA devices include the brcmnand controller but they do not wire up
any interrupt line, allow the main interrupt to be optional and update
the completion path to also check for the lack of an interrupt line.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 52 +++++++++++-------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 60a7f375df83..e7947cff4dd1 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -213,7 +213,7 @@ struct brcmnand_controller {
 	void __iomem		*nand_base;
 	void __iomem		*nand_fc; /* flash cache */
 	void __iomem		*flash_dma_base;
-	unsigned int		irq;
+	int			irq;
 	unsigned int		dma_irq;
 	int			nand_version;
 
@@ -1602,7 +1602,7 @@ static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
 	bool err = false;
 	int sts;
 
-	if (mtd->oops_panic_write) {
+	if (mtd->oops_panic_write || ctrl->irq < 0) {
 		/* switch to interrupt polling and PIO mode */
 		disable_ctrl_irqs(ctrl);
 		sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY,
@@ -3130,33 +3130,29 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	}
 
 	/* IRQ */
-	ctrl->irq = platform_get_irq(pdev, 0);
-	if ((int)ctrl->irq < 0) {
-		dev_err(dev, "no IRQ defined\n");
-		ret = -ENODEV;
-		goto err;
-	}
-
-	/*
-	 * Some SoCs integrate this controller (e.g., its interrupt bits) in
-	 * interesting ways
-	 */
-	if (soc) {
-		ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
-				       DRV_NAME, ctrl);
+	ctrl->irq = platform_get_irq_optional(pdev, 0);
+	if (ctrl->irq >= 0) {
+		/*
+		 * Some SoCs integrate this controller (e.g., its interrupt bits) in
+		 * interesting ways
+		 */
+		if (soc) {
+			ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+					       DRV_NAME, ctrl);
 
-		/* Enable interrupt */
-		ctrl->soc->ctlrdy_ack(ctrl->soc);
-		ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
-	} else {
-		/* Use standard interrupt infrastructure */
-		ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
-				       DRV_NAME, ctrl);
-	}
-	if (ret < 0) {
-		dev_err(dev, "can't allocate IRQ %d: error %d\n",
-			ctrl->irq, ret);
-		goto err;
+			/* Enable interrupt */
+			ctrl->soc->ctlrdy_ack(ctrl->soc);
+			ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+		} else {
+			/* Use standard interrupt infrastructure */
+			ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+					       DRV_NAME, ctrl);
+		}
+		if (ret < 0) {
+			dev_err(dev, "can't allocate IRQ %d: error %d\n",
+				ctrl->irq, ret);
+			goto err;
+		}
 	}
 
 	for_each_available_child_of_node(dn, child) {
-- 
2.25.1

[PATCH 6/9] mtd: rawnand: brcmnand: Add platform data structure for BCMA

[Florian Fainelli]

Update the BCMA's chipcommon nand flash driver to detect which
chip-select is used and pass that information via platform data to the
brcmnand driver. Make sure that the brcmnand platform data structure is
always at the beginning of the platform data of the "nflash" device
created by BCMA to allow brcmnand to safely de-reference it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 MAINTAINERS                                 |  1 +
 drivers/bcma/driver_chipcommon_nflash.c     | 17 ++++++++++++++++-
 include/linux/bcma/bcma_driver_chipcommon.h |  5 +++++
 include/linux/platform_data/brcmnand.h      | 12 ++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/platform_data/brcmnand.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8912b2c1260c..37062172083c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3965,6 +3965,7 @@ L:	linux-mtd@lists.infradead.org
 L:	bcm-kernel-feedback-list@broadcom.com
 S:	Maintained
 F:	drivers/mtd/nand/raw/brcmnand/
+F:	include/linux/platform_data/brcmnand.h
 
 BROADCOM STB PCIE DRIVER
 M:	Jim Quinlan <jim2101024@gmail.com>
diff --git a/drivers/bcma/driver_chipcommon_nflash.c b/drivers/bcma/driver_chipcommon_nflash.c
index d4f699aef8c4..9457f4b7ea9d 100644
--- a/drivers/bcma/driver_chipcommon_nflash.c
+++ b/drivers/bcma/driver_chipcommon_nflash.c
@@ -7,18 +7,25 @@
 
 #include "bcma_private.h"
 
+#include <linux/bitops.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/brcmnand.h>
 #include <linux/bcma/bcma.h>
 
+static const char *bcma_nflash_alt_name = "bcma_brcmnand";
+
 struct platform_device bcma_nflash_dev = {
 	.name		= "bcma_nflash",
 	.num_resources	= 0,
 };
 
+static const char *probes[] = { "bcm47xxpart", NULL };
+
 /* Initialize NAND flash access */
 int bcma_nflash_init(struct bcma_drv_cc *cc)
 {
 	struct bcma_bus *bus = cc->core->bus;
+	u32 reg;
 
 	if (bus->chipinfo.id != BCMA_CHIP_ID_BCM4706 &&
 	    cc->core->id.rev != 38) {
@@ -33,8 +40,16 @@ int bcma_nflash_init(struct bcma_drv_cc *cc)
 
 	cc->nflash.present = true;
 	if (cc->core->id.rev == 38 &&
-	    (cc->status & BCMA_CC_CHIPST_5357_NAND_BOOT))
+	    (cc->status & BCMA_CC_CHIPST_5357_NAND_BOOT)) {
 		cc->nflash.boot = true;
+		/* Determine the chip select that is being used */
+		reg = bcma_cc_read32(cc, BCMA_CC_NAND_CS_NAND_SELECT) & 0xff;
+		cc->nflash.brcmnand_info.chip_select = ffs(reg) - 1;
+		cc->nflash.brcmnand_info.part_probe_types = probes;
+		cc->nflash.brcmnand_info.ecc_stepsize = 512;
+		cc->nflash.brcmnand_info.ecc_strength = 1;
+		bcma_nflash_dev.name = bcma_nflash_alt_name;
+	}
 
 	/* Prepare platform device, but don't register it yet. It's too early,
 	 * malloc (required by device_private_init) is not available yet. */
diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h
index d35b9206096d..e3314f746bfa 100644
--- a/include/linux/bcma/bcma_driver_chipcommon.h
+++ b/include/linux/bcma/bcma_driver_chipcommon.h
@@ -3,6 +3,7 @@
 #define LINUX_BCMA_DRIVER_CC_H_
 
 #include <linux/platform_device.h>
+#include <linux/platform_data/brcmnand.h>
 #include <linux/gpio.h>
 
 /** ChipCommon core registers. **/
@@ -599,6 +600,10 @@ struct bcma_sflash {
 
 #ifdef CONFIG_BCMA_NFLASH
 struct bcma_nflash {
+	/* Must be the fist member for the brcmnand driver to
+	 * de-reference that structure.
+	 */
+	struct brcmnand_platform_data brcmnand_info;
 	bool present;
 	bool boot;		/* This is the flash the SoC boots from */
 };
diff --git a/include/linux/platform_data/brcmnand.h b/include/linux/platform_data/brcmnand.h
new file mode 100644
index 000000000000..8b8777985dce
--- /dev/null
+++ b/include/linux/platform_data/brcmnand.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef BRCMNAND_PLAT_DATA_H
+#define BRCMNAND_PLAT_DATA_H
+
+struct brcmnand_platform_data {
+	int	chip_select;
+	const char * const *part_probe_types;
+	unsigned int ecc_stepsize;
+	unsigned int ecc_strength;
+};
+
+#endif /* BRCMNAND_PLAT_DATA_H */
-- 
2.25.1

[PATCH 7/9] mtd: rawnand: brcmnand: Allow platform data instantation

[Florian Fainelli]

Make use of the recently refactored code in brcmnand_init_cs() and
derive the chip-select from the platform data that is supplied. Update
the various code paths to avoid relying on possibly non-existent
resources, too.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 45 ++++++++++++++++++------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e7947cff4dd1..2f2aaa554282 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -9,6 +9,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/brcmnand.h>
 #include <linux/err.h>
 #include <linux/completion.h>
 #include <linux/interrupt.h>
@@ -2760,7 +2761,8 @@ static const struct nand_controller_ops brcmnand_controller_ops = {
 	.attach_chip = brcmnand_attach_chip,
 };
 
-static int brcmnand_init_cs(struct brcmnand_host *host)
+static int brcmnand_init_cs(struct brcmnand_host *host,
+			    const char * const *part_probe_types)
 {
 	struct brcmnand_controller *ctrl = host->ctrl;
 	struct device *dev = ctrl->dev;
@@ -2813,7 +2815,7 @@ static int brcmnand_init_cs(struct brcmnand_host *host)
 	if (ret)
 		return ret;
 
-	ret = mtd_device_register(mtd, NULL, 0);
+	ret = mtd_device_parse_register(mtd, part_probe_types, NULL, NULL, 0);
 	if (ret)
 		nand_cleanup(chip);
 
@@ -2982,17 +2984,15 @@ static int brcmnand_edu_setup(struct platform_device *pdev)
 
 int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 {
+	struct brcmnand_platform_data *pd = dev_get_platdata(&pdev->dev);
 	struct device *dev = &pdev->dev;
 	struct device_node *dn = dev->of_node, *child;
 	struct brcmnand_controller *ctrl;
+	struct brcmnand_host *host;
 	struct resource *res;
 	int ret;
 
-	/* We only support device-tree instantiation */
-	if (!dn)
-		return -ENODEV;
-
-	if (!of_match_node(brcmnand_of_match, dn))
+	if (dn && !of_match_node(brcmnand_of_match, dn))
 		return -ENODEV;
 
 	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
@@ -3013,7 +3013,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	/* NAND register range */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ctrl->nand_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(ctrl->nand_base))
+	if (IS_ERR(ctrl->nand_base) && !brcmnand_soc_has_ops(soc))
 		return PTR_ERR(ctrl->nand_base);
 
 	/* Enable clock before using NAND registers */
@@ -3157,7 +3157,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 
 	for_each_available_child_of_node(dn, child) {
 		if (of_device_is_compatible(child, "brcm,nandcs")) {
-			struct brcmnand_host *host;
 
 			host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
 			if (!host) {
@@ -3177,7 +3176,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 
 			nand_set_flash_node(&host->chip, dn);
 
-			ret = brcmnand_init_cs(host);
+			ret = brcmnand_init_cs(host, NULL);
 			if (ret) {
 				devm_kfree(dev, host);
 				continue; /* Try all chip-selects */
@@ -3187,6 +3186,32 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 		}
 	}
 
+	if (!list_empty(&ctrl->host_list))
+		return 0;
+
+	if (!pd) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	/* If we got there we must have been probing via platform data */
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	host->pdev = pdev;
+	host->ctrl = ctrl;
+	host->cs = pd->chip_select;
+	host->chip.ecc.size = pd->ecc_stepsize;
+	host->chip.ecc.strength = pd->ecc_strength;
+
+	ret = brcmnand_init_cs(host, pd->part_probe_types);
+	if (ret)
+		goto err;
+
+	list_add_tail(&host->node, &ctrl->host_list);
+
 	/* No chip-selects could initialize properly */
 	if (list_empty(&ctrl->host_list)) {
 		ret = -ENODEV;
-- 
2.25.1

[PATCH 8/9] mtd: rawnand: brcmnand: BCMA controller uses command shift of 0

[Florian Fainelli]

For some odd and unexplained reason the BCMA NAND controller, albeit
revision 3.4 uses a command shift of 0 instead of 24 as it should be,
quirk that.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 2f2aaa554282..2dfb94c4cc93 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -908,6 +908,12 @@ static void brcmnand_wr_corr_thresh(struct brcmnand_host *host, u8 val)
 
 static inline int brcmnand_cmd_shift(struct brcmnand_controller *ctrl)
 {
+	/* Kludge for the BCMA-based NAND controller which does not actually
+	 * shift the command
+	 */
+	if (ctrl->nand_version == 0x0304 && brcmnand_soc_has_ops(ctrl->soc))
+		return 0;
+
 	if (ctrl->nand_version < 0x0602)
 		return 24;
 	return 0;
-- 
2.25.1

[PATCH 9/9] mtd: rawnand: brcmnand: Add BCMA shim

[Florian Fainelli]

Add a BCMA shim to allow us to register the brcmnand driver using the
BCMA bus which provides indirect memory mapped access to SoC registers.

There are a number of registers that need to be byte swapped because
they are natively big endian, coming directly from the NAND chip, and
there is no bus interface unlike the iProc or STB platforms that
performs the byte swapping for us.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mtd/nand/raw/Kconfig              |  11 ++
 drivers/mtd/nand/raw/brcmnand/Makefile    |   2 +
 drivers/mtd/nand/raw/brcmnand/bcma_nand.c | 131 ++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/brcmnand/bcma_nand.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 0a45d3c6c15b..f643e02e5559 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -208,6 +208,17 @@ config MTD_NAND_BRCMNAND
 	  originally designed for Set-Top Box but is used on various BCM7xxx,
 	  BCM3xxx, BCM63xxx, iProc/Cygnus and more.
 
+if MTD_NAND_BRCMNAND
+
+config MTD_NAND_BRCMNAND_BCMA
+	tristate "Broadcom BCMA NAND controller"
+	depends on BCMA_NFLASH
+	depends on BCMA
+	help
+	  Enables the BRCMNAND controller over BCMA on BCM47186/BCM5358 SoCs.
+
+endif # MTD_NAND_BRCMNAND
+
 config MTD_NAND_BCM47XXNFLASH
 	tristate "BCM4706 BCMA NAND controller"
 	depends on BCMA_NFLASH
diff --git a/drivers/mtd/nand/raw/brcmnand/Makefile b/drivers/mtd/nand/raw/brcmnand/Makefile
index 195b845e48b8..16dc7254200e 100644
--- a/drivers/mtd/nand/raw/brcmnand/Makefile
+++ b/drivers/mtd/nand/raw/brcmnand/Makefile
@@ -6,3 +6,5 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63138_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm6368_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmstb_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand.o
+
+obj-$(CONFIG_MTD_NAND_BRCMNAND_BCMA)	+= bcma_nand.o
diff --git a/drivers/mtd/nand/raw/brcmnand/bcma_nand.c b/drivers/mtd/nand/raw/brcmnand/bcma_nand.c
new file mode 100644
index 000000000000..e3be9ecf0761
--- /dev/null
+++ b/drivers/mtd/nand/raw/brcmnand/bcma_nand.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright © 2021 Broadcom
+ */
+#include <linux/bcma/bcma.h>
+#include <linux/bcma/bcma_driver_chipcommon.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+
+#include "brcmnand.h"
+
+struct brcmnand_bcma_soc {
+	struct brcmnand_soc soc;
+	struct bcma_drv_cc *cc;
+};
+
+static inline bool brcmnand_bcma_needs_swapping(u32 offset)
+{
+	switch (offset) {
+	case BCMA_CC_NAND_SPARE_RD0:
+	case BCMA_CC_NAND_SPARE_RD4:
+	case BCMA_CC_NAND_SPARE_RD8:
+	case BCMA_CC_NAND_SPARE_RD12:
+	case BCMA_CC_NAND_SPARE_WR0:
+	case BCMA_CC_NAND_SPARE_WR4:
+	case BCMA_CC_NAND_SPARE_WR8:
+	case BCMA_CC_NAND_SPARE_WR12:
+	case BCMA_CC_NAND_DEVID:
+	case BCMA_CC_NAND_DEVID_X:
+	case BCMA_CC_NAND_SPARE_RD16:
+	case BCMA_CC_NAND_SPARE_RD20:
+	case BCMA_CC_NAND_SPARE_RD24:
+	case BCMA_CC_NAND_SPARE_RD28:
+		return true;
+	}
+
+	return false;
+}
+
+static u32 brcmnand_bcma_read_reg(struct brcmnand_soc *soc, u32 offset)
+{
+	struct brcmnand_bcma_soc *sc;
+	u32 val;
+
+	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
+
+	/* Offset into the NAND block and deal with the flash cache separately */
+	if (offset == ~0)
+		offset = BCMA_CC_NAND_CACHE_DATA;
+	else
+		offset += BCMA_CC_NAND_REVISION;
+
+	val = bcma_cc_read32(sc->cc, offset);
+
+	/* Swap if necessary */
+	if (brcmnand_bcma_needs_swapping(offset))
+		val = be32_to_cpu(val);
+	return val;
+}
+
+static void brcmnand_bcma_write_reg(struct brcmnand_soc *soc, u32 val,
+				    u32 offset)
+{
+	struct brcmnand_bcma_soc *sc;
+
+	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
+
+	/* Offset into the NAND block */
+	if (offset == ~0)
+		offset = BCMA_CC_NAND_CACHE_DATA;
+	else
+		offset += BCMA_CC_NAND_REVISION;
+
+	/* Swap if necessary */
+	if (brcmnand_bcma_needs_swapping(offset))
+		val = cpu_to_be32(val);
+
+	bcma_cc_write32(sc->cc, offset, val);
+}
+
+static struct brcmnand_io_ops brcmnand_bcma_io_ops = {
+	.read_reg	= brcmnand_bcma_read_reg,
+	.write_reg	= brcmnand_bcma_write_reg,
+};
+
+static void brcmnand_bcma_prepare_data_bus(struct brcmnand_soc *soc, bool prepare,
+					   bool is_param)
+{
+	struct brcmnand_bcma_soc *sc;
+
+	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
+
+	bcma_cc_write32(sc->cc, BCMA_CC_NAND_CACHE_ADDR, 0);
+}
+
+static int brcmnand_bcma_nand_probe(struct platform_device *pdev)
+{
+	struct bcma_nflash *nflash = dev_get_platdata(&pdev->dev);
+	struct brcmnand_bcma_soc *soc;
+
+	soc = devm_kzalloc(&pdev->dev, sizeof(*soc), GFP_KERNEL);
+	if (!soc)
+		return -ENOMEM;
+
+	soc->cc = container_of(nflash, struct bcma_drv_cc, nflash);
+	soc->soc.prepare_data_bus = brcmnand_bcma_prepare_data_bus;
+	soc->soc.ops = &brcmnand_bcma_io_ops;
+
+	if (soc->cc->core->bus->chipinfo.id == BCMA_CHIP_ID_BCM4706) {
+		dev_err(&pdev->dev, "Use bcm47xxnflash for 4706!\n");
+		return -ENODEV;
+	}
+
+	return brcmnand_probe(pdev, &soc->soc);
+}
+
+static struct platform_driver brcmnand_bcma_nand_driver = {
+	.probe			= brcmnand_bcma_nand_probe,
+	.remove			= brcmnand_remove,
+	.driver = {
+		.name		= "bcma_brcmnand",
+		.pm		= &brcmnand_pm_ops,
+	}
+};
+module_platform_driver(brcmnand_bcma_nand_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Braodcom");
+MODULE_DESCRIPTION("NAND driver for BCMA chips");
-- 
2.25.1

Re: [PATCH 5/9] mtd: rawnand: brcmnand: Allow working without interrupts

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 1:41 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> The BCMA devices include the brcmnand controller but they do not wire up
> any interrupt line, allow the main interrupt to be optional and update
> the completion path to also check for the lack of an interrupt line.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

> -       unsigned int            irq;
> +       int                     irq;

instead.,,

> +       ctrl->irq = platform_get_irq_optional(pdev, 0);
> +       if (ctrl->irq >= 0) {

ret = ...
if (ret > 0) {

And drop 0 from the equation, OF never uses 0 as valid vIRQ.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/9] mtd: rawnand: brcmnand: Allow working without interrupts

[Florian Fainelli]

On 12/25/2021 9:45 AM, Andy Shevchenko wrote:
> On Sat, Dec 25, 2021 at 1:41 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> The BCMA devices include the brcmnand controller but they do not wire up
>> any interrupt line, allow the main interrupt to be optional and update
>> the completion path to also check for the lack of an interrupt line.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
>> -       unsigned int            irq;
>> +       int                     irq;
> 
> instead.,,
> 
>> +       ctrl->irq = platform_get_irq_optional(pdev, 0);
>> +       if (ctrl->irq >= 0) {
> 
> ret = ...
> if (ret > 0) {
> 
> And drop 0 from the equation, OF never uses 0 as valid vIRQ.

OK but the point of this patch series is to allow the use of the 
brcmnand driver in a configuration without OF. I don't really see the 
point in continuing to use unsigned int instead of just letting 
request_irq() play through and tell us if the interrupt descriptor was 
valid later on.
-- 
Florian

Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Miquel Raynal]

Hi Florian,

f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:

> Allow a brcmnand_soc instance to provide a custom set of I/O operations
> which we will require when using this driver on a BCMA bus which is not
> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
> to use the SoC operations if provided.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
>  drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index f75929783b94..7a1673b1b1af 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -594,13 +594,18 @@ enum {
>  
>  static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
>  {
> +	if (brcmnand_soc_has_ops(ctrl->soc))
> +		return brcmnand_soc_read(ctrl->soc, offs);
>  	return brcmnand_readl(ctrl->nand_base + offs);
>  }
>  
>  static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
>  				 u32 val)
>  {
> -	brcmnand_writel(val, ctrl->nand_base + offs);
> +	if (brcmnand_soc_has_ops(ctrl->soc))
> +		brcmnand_soc_write(ctrl->soc, val, offs);
> +	else
> +		brcmnand_writel(val, ctrl->nand_base + offs);
>  }
>  
>  static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,
>  
>  static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)
>  {
> +	if (brcmnand_soc_has_ops(ctrl->soc))
> +		return brcmnand_soc_read(ctrl->soc, ~0);
>  	return __raw_readl(ctrl->nand_fc + word * 4);
>  }
>  
>  static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
>  				     int word, u32 val)
>  {
> -	__raw_writel(val, ctrl->nand_fc + word * 4);
> +	if (brcmnand_soc_has_ops(ctrl->soc))
> +		brcmnand_soc_write(ctrl->soc, val, ~0);
> +	else
> +		__raw_writel(val, ctrl->nand_fc + word * 4);
>  }
>  
>  static inline void edu_writel(struct brcmnand_controller *ctrl,
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> index eb498fbe505e..a3f2ad5f6572 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> @@ -11,12 +11,19 @@
>  
>  struct platform_device;
>  struct dev_pm_ops;
> +struct brcmnand_io_ops;
>  
>  struct brcmnand_soc {
>  	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
>  	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
>  	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
>  				 bool is_param);
> +	const struct brcmnand_io_ops *ops;
> +};
> +
> +struct brcmnand_io_ops {
> +	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
> +	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
>  };
>  
>  static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,
> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
>  		writel_relaxed(val, addr);
>  }
>  
> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)
> +{
> +	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
> +}
> +
> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
> +{
> +	return soc->ops->read_reg(soc, offset);
> +}
> +
> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
> +				      u32 offset)
> +{
> +	soc->ops->write_reg(soc, val, offset);
> +}
> +

It might be worth looking into more optimized ways to do these checks,
in particular the read/write_reg ones because you're checking against
some static data which cannot be optimized out by the compiler but
won't change in the lifetime of the kernel.

Thanks,
Miquèl

Re: [PATCH 4/9] mtd: rawnand: brcmnand: Move OF operations out of brcmnand_init_cs()

[Miquel Raynal]

Hi Florian,

f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:20 -0800:

> In order to initialize a given chip select object for use by the
> brcmnand driver, move all of the Device Tree specific routines outside
> of brcmnand_init_cs() in order to make it usable in a platform data
> configuration which will be necessary for supporting BCMA chips.

TBH I'm note a big fan of the idea. I'm not sure going back to
supporting platform data this way really is a good idea... There are so
much things that are well described with DT that we now rely upon that
I am not entirely convinced by these changes :-/ The move is generally
in the other direction: getting rid of the legacy platform data.

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Cheers,
Miquèl

> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 35f8d8e02d4a..60a7f375df83 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -2760,7 +2760,7 @@ static const struct nand_controller_ops brcmnand_controller_ops = {
>  	.attach_chip = brcmnand_attach_chip,
>  };
>  
> -static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
> +static int brcmnand_init_cs(struct brcmnand_host *host)
>  {
>  	struct brcmnand_controller *ctrl = host->ctrl;
>  	struct device *dev = ctrl->dev;
> @@ -2769,16 +2769,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
>  	int ret;
>  	u16 cfg_offs;
>  
> -	ret = of_property_read_u32(dn, "reg", &host->cs);
> -	if (ret) {
> -		dev_err(dev, "can't get chip-select\n");
> -		return -ENXIO;
> -	}
> -
>  	mtd = nand_to_mtd(&host->chip);
>  	chip = &host->chip;
>  
> -	nand_set_flash_node(chip, dn);
>  	nand_set_controller_data(chip, host);
>  	mtd->name = devm_kasprintf(dev, GFP_KERNEL, "brcmnand.%d",
>  				   host->cs);
> @@ -3179,7 +3172,16 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
>  			host->pdev = pdev;
>  			host->ctrl = ctrl;
>  
> -			ret = brcmnand_init_cs(host, child);
> +			ret = of_property_read_u32(dn, "reg", &host->cs);
> +			if (ret) {
> +				dev_err(dev, "can't get chip-select\n");
> +				devm_kfree(dev, host);
> +				continue;
> +			}
> +
> +			nand_set_flash_node(&host->chip, dn);
> +
> +			ret = brcmnand_init_cs(host);
>  			if (ret) {
>  				devm_kfree(dev, host);
>  				continue; /* Try all chip-selects */

Re: [PATCH 9/9] mtd: rawnand: brcmnand: Add BCMA shim

[Miquel Raynal]

Hi Florian,

f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:25 -0800:

> Add a BCMA shim to allow us to register the brcmnand driver using the
> BCMA bus which provides indirect memory mapped access to SoC registers.
> 
> There are a number of registers that need to be byte swapped because
> they are natively big endian, coming directly from the NAND chip, and
> there is no bus interface unlike the iProc or STB platforms that
> performs the byte swapping for us.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/mtd/nand/raw/Kconfig              |  11 ++
>  drivers/mtd/nand/raw/brcmnand/Makefile    |   2 +
>  drivers/mtd/nand/raw/brcmnand/bcma_nand.c | 131 ++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/brcmnand/bcma_nand.c
> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 0a45d3c6c15b..f643e02e5559 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -208,6 +208,17 @@ config MTD_NAND_BRCMNAND
>  	  originally designed for Set-Top Box but is used on various BCM7xxx,
>  	  BCM3xxx, BCM63xxx, iProc/Cygnus and more.
>  
> +if MTD_NAND_BRCMNAND
> +
> +config MTD_NAND_BRCMNAND_BCMA
> +	tristate "Broadcom BCMA NAND controller"
> +	depends on BCMA_NFLASH
> +	depends on BCMA
> +	help
> +	  Enables the BRCMNAND controller over BCMA on BCM47186/BCM5358 SoCs.
> +
> +endif # MTD_NAND_BRCMNAND
> +
>  config MTD_NAND_BCM47XXNFLASH
>  	tristate "BCM4706 BCMA NAND controller"
>  	depends on BCMA_NFLASH
> diff --git a/drivers/mtd/nand/raw/brcmnand/Makefile b/drivers/mtd/nand/raw/brcmnand/Makefile
> index 195b845e48b8..16dc7254200e 100644
> --- a/drivers/mtd/nand/raw/brcmnand/Makefile
> +++ b/drivers/mtd/nand/raw/brcmnand/Makefile
> @@ -6,3 +6,5 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63138_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm6368_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmstb_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand.o
> +
> +obj-$(CONFIG_MTD_NAND_BRCMNAND_BCMA)	+= bcma_nand.o
> diff --git a/drivers/mtd/nand/raw/brcmnand/bcma_nand.c b/drivers/mtd/nand/raw/brcmnand/bcma_nand.c
> new file mode 100644
> index 000000000000..e3be9ecf0761
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/brcmnand/bcma_nand.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright © 2021 Broadcom
> + */
> +#include <linux/bcma/bcma.h>
> +#include <linux/bcma/bcma_driver_chipcommon.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +
> +#include "brcmnand.h"
> +
> +struct brcmnand_bcma_soc {
> +	struct brcmnand_soc soc;
> +	struct bcma_drv_cc *cc;
> +};
> +
> +static inline bool brcmnand_bcma_needs_swapping(u32 offset)
> +{
> +	switch (offset) {
> +	case BCMA_CC_NAND_SPARE_RD0:
> +	case BCMA_CC_NAND_SPARE_RD4:
> +	case BCMA_CC_NAND_SPARE_RD8:
> +	case BCMA_CC_NAND_SPARE_RD12:
> +	case BCMA_CC_NAND_SPARE_WR0:
> +	case BCMA_CC_NAND_SPARE_WR4:
> +	case BCMA_CC_NAND_SPARE_WR8:
> +	case BCMA_CC_NAND_SPARE_WR12:
> +	case BCMA_CC_NAND_DEVID:
> +	case BCMA_CC_NAND_DEVID_X:
> +	case BCMA_CC_NAND_SPARE_RD16:
> +	case BCMA_CC_NAND_SPARE_RD20:
> +	case BCMA_CC_NAND_SPARE_RD24:
> +	case BCMA_CC_NAND_SPARE_RD28:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static u32 brcmnand_bcma_read_reg(struct brcmnand_soc *soc, u32 offset)
> +{
> +	struct brcmnand_bcma_soc *sc;
> +	u32 val;
> +
> +	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
> +
> +	/* Offset into the NAND block and deal with the flash cache separately */
> +	if (offset == ~0)
> +		offset = BCMA_CC_NAND_CACHE_DATA;
> +	else
> +		offset += BCMA_CC_NAND_REVISION;
> +
> +	val = bcma_cc_read32(sc->cc, offset);
> +
> +	/* Swap if necessary */
> +	if (brcmnand_bcma_needs_swapping(offset))
> +		val = be32_to_cpu(val);
> +	return val;
> +}
> +
> +static void brcmnand_bcma_write_reg(struct brcmnand_soc *soc, u32 val,
> +				    u32 offset)
> +{
> +	struct brcmnand_bcma_soc *sc;
> +
> +	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
> +
> +	/* Offset into the NAND block */
> +	if (offset == ~0)
> +		offset = BCMA_CC_NAND_CACHE_DATA;
> +	else
> +		offset += BCMA_CC_NAND_REVISION;
> +
> +	/* Swap if necessary */
> +	if (brcmnand_bcma_needs_swapping(offset))
> +		val = cpu_to_be32(val);
> +
> +	bcma_cc_write32(sc->cc, offset, val);
> +}
> +
> +static struct brcmnand_io_ops brcmnand_bcma_io_ops = {
> +	.read_reg	= brcmnand_bcma_read_reg,
> +	.write_reg	= brcmnand_bcma_write_reg,
> +};
> +
> +static void brcmnand_bcma_prepare_data_bus(struct brcmnand_soc *soc, bool prepare,
> +					   bool is_param)
> +{
> +	struct brcmnand_bcma_soc *sc;
> +
> +	sc = container_of(soc, struct brcmnand_bcma_soc, soc);

A helper for that would be nice?

Otherwise looks good.

> +
> +	bcma_cc_write32(sc->cc, BCMA_CC_NAND_CACHE_ADDR, 0);
> +}
> +
> +static int brcmnand_bcma_nand_probe(struct platform_device *pdev)
> +{
> +	struct bcma_nflash *nflash = dev_get_platdata(&pdev->dev);
> +	struct brcmnand_bcma_soc *soc;
> +
> +	soc = devm_kzalloc(&pdev->dev, sizeof(*soc), GFP_KERNEL);
> +	if (!soc)
> +		return -ENOMEM;
> +
> +	soc->cc = container_of(nflash, struct bcma_drv_cc, nflash);
> +	soc->soc.prepare_data_bus = brcmnand_bcma_prepare_data_bus;
> +	soc->soc.ops = &brcmnand_bcma_io_ops;
> +
> +	if (soc->cc->core->bus->chipinfo.id == BCMA_CHIP_ID_BCM4706) {
> +		dev_err(&pdev->dev, "Use bcm47xxnflash for 4706!\n");
> +		return -ENODEV;
> +	}
> +
> +	return brcmnand_probe(pdev, &soc->soc);
> +}
> +
> +static struct platform_driver brcmnand_bcma_nand_driver = {
> +	.probe			= brcmnand_bcma_nand_probe,
> +	.remove			= brcmnand_remove,
> +	.driver = {
> +		.name		= "bcma_brcmnand",
> +		.pm		= &brcmnand_pm_ops,
> +	}
> +};
> +module_platform_driver(brcmnand_bcma_nand_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Braodcom");
> +MODULE_DESCRIPTION("NAND driver for BCMA chips");


Thanks,
Miquèl

Re: [PATCH 9/9] mtd: rawnand: brcmnand: Add BCMA shim

[Miquel Raynal]

> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Braodcom");

Small typo here :)

> +MODULE_DESCRIPTION("NAND driver for BCMA chips");

NAND controller driver, I want to enforce this *controller* word
because NAND is way too vague and refers to nothing specific.

Plus, it's not really a controller driver that we have here, more like
a glue interface.

Thanks,
Miquèl

Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Florian Fainelli]

On 1/3/2022 8:49 AM, Miquel Raynal wrote:
> Hi Florian,
> 
> f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:
> 
>> Allow a brcmnand_soc instance to provide a custom set of I/O operations
>> which we will require when using this driver on a BCMA bus which is not
>> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
>> to use the SoC operations if provided.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
>>   2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index f75929783b94..7a1673b1b1af 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -594,13 +594,18 @@ enum {
>>   
>>   static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
>>   {
>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>> +		return brcmnand_soc_read(ctrl->soc, offs);
>>   	return brcmnand_readl(ctrl->nand_base + offs);
>>   }
>>   
>>   static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
>>   				 u32 val)
>>   {
>> -	brcmnand_writel(val, ctrl->nand_base + offs);
>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>> +		brcmnand_soc_write(ctrl->soc, val, offs);
>> +	else
>> +		brcmnand_writel(val, ctrl->nand_base + offs);
>>   }
>>   
>>   static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
>> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,
>>   
>>   static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)
>>   {
>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>> +		return brcmnand_soc_read(ctrl->soc, ~0);
>>   	return __raw_readl(ctrl->nand_fc + word * 4);
>>   }
>>   
>>   static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
>>   				     int word, u32 val)
>>   {
>> -	__raw_writel(val, ctrl->nand_fc + word * 4);
>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>> +		brcmnand_soc_write(ctrl->soc, val, ~0);
>> +	else
>> +		__raw_writel(val, ctrl->nand_fc + word * 4);
>>   }
>>   
>>   static inline void edu_writel(struct brcmnand_controller *ctrl,
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>> index eb498fbe505e..a3f2ad5f6572 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>> @@ -11,12 +11,19 @@
>>   
>>   struct platform_device;
>>   struct dev_pm_ops;
>> +struct brcmnand_io_ops;
>>   
>>   struct brcmnand_soc {
>>   	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
>>   	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
>>   	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
>>   				 bool is_param);
>> +	const struct brcmnand_io_ops *ops;
>> +};
>> +
>> +struct brcmnand_io_ops {
>> +	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
>> +	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
>>   };
>>   
>>   static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,
>> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
>>   		writel_relaxed(val, addr);
>>   }
>>   
>> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)
>> +{
>> +	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
>> +}
>> +
>> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
>> +{
>> +	return soc->ops->read_reg(soc, offset);
>> +}
>> +
>> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
>> +				      u32 offset)
>> +{
>> +	soc->ops->write_reg(soc, val, offset);
>> +}
>> +
> 
> It might be worth looking into more optimized ways to do these checks,
> in particular the read/write_reg ones because you're checking against
> some static data which cannot be optimized out by the compiler but
> won't change in the lifetime of the kernel.

I suppose I could add an addition if 
IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA) at the front of 
brcmnand_soc_has_ops(), would that address your concern or you have 
something else in mind?
--
Florian

Re: [PATCH 4/9] mtd: rawnand: brcmnand: Move OF operations out of brcmnand_init_cs()

[Florian Fainelli]

On 1/3/2022 8:56 AM, Miquel Raynal wrote:
> Hi Florian,
> 
> f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:20 -0800:
> 
>> In order to initialize a given chip select object for use by the
>> brcmnand driver, move all of the Device Tree specific routines outside
>> of brcmnand_init_cs() in order to make it usable in a platform data
>> configuration which will be necessary for supporting BCMA chips.
> 
> TBH I'm note a big fan of the idea. I'm not sure going back to
> supporting platform data this way really is a good idea... There are so
> much things that are well described with DT that we now rely upon that
> I am not entirely convinced by these changes :-/ The move is generally
> in the other direction: getting rid of the legacy platform data.

In the cover letter there is an explanation as to why we need to 
introduce platform data/device support here: the platforms on which this 
NAND controller shim is used do not have Device Tree support, and won't 
have it in the future either. They are old platforms (first SoC 
supported by bcm47xx is maybe 15 years old now) but they are still in 
active and wide use by the OpenWrt, dd-wrt communities.
-- 
Florian

Re: [PATCH 9/9] mtd: rawnand: brcmnand: Add BCMA shim

[Florian Fainelli]

On 1/3/2022 9:12 AM, Miquel Raynal wrote:
> 
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Braodcom");
> 
> Small typo here :)

Whoops

> 
>> +MODULE_DESCRIPTION("NAND driver for BCMA chips");
> 
> NAND controller driver, I want to enforce this *controller* word
> because NAND is way too vague and refers to nothing specific.
> 
> Plus, it's not really a controller driver that we have here, more like
> a glue interface.

Certainly, thanks!
-- 
Florian

Re: [PATCH 9/9] mtd: rawnand: brcmnand: Add BCMA shim

[Florian Fainelli]

On 1/3/2022 9:06 AM, Miquel Raynal wrote:
> Hi Florian,
> 
> f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:25 -0800:
> 
>> Add a BCMA shim to allow us to register the brcmnand driver using the
>> BCMA bus which provides indirect memory mapped access to SoC registers.
>>
>> There are a number of registers that need to be byte swapped because
>> they are natively big endian, coming directly from the NAND chip, and
>> there is no bus interface unlike the iProc or STB platforms that
>> performs the byte swapping for us.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/mtd/nand/raw/Kconfig              |  11 ++
>>   drivers/mtd/nand/raw/brcmnand/Makefile    |   2 +
>>   drivers/mtd/nand/raw/brcmnand/bcma_nand.c | 131 ++++++++++++++++++++++
>>   3 files changed, 144 insertions(+)
>>   create mode 100644 drivers/mtd/nand/raw/brcmnand/bcma_nand.c
>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index 0a45d3c6c15b..f643e02e5559 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -208,6 +208,17 @@ config MTD_NAND_BRCMNAND
>>   	  originally designed for Set-Top Box but is used on various BCM7xxx,
>>   	  BCM3xxx, BCM63xxx, iProc/Cygnus and more.
>>   
>> +if MTD_NAND_BRCMNAND
>> +
>> +config MTD_NAND_BRCMNAND_BCMA
>> +	tristate "Broadcom BCMA NAND controller"
>> +	depends on BCMA_NFLASH
>> +	depends on BCMA
>> +	help
>> +	  Enables the BRCMNAND controller over BCMA on BCM47186/BCM5358 SoCs.
>> +
>> +endif # MTD_NAND_BRCMNAND
>> +
>>   config MTD_NAND_BCM47XXNFLASH
>>   	tristate "BCM4706 BCMA NAND controller"
>>   	depends on BCMA_NFLASH
>> diff --git a/drivers/mtd/nand/raw/brcmnand/Makefile b/drivers/mtd/nand/raw/brcmnand/Makefile
>> index 195b845e48b8..16dc7254200e 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/Makefile
>> +++ b/drivers/mtd/nand/raw/brcmnand/Makefile
>> @@ -6,3 +6,5 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63138_nand.o
>>   obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm6368_nand.o
>>   obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmstb_nand.o
>>   obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand.o
>> +
>> +obj-$(CONFIG_MTD_NAND_BRCMNAND_BCMA)	+= bcma_nand.o
>> diff --git a/drivers/mtd/nand/raw/brcmnand/bcma_nand.c b/drivers/mtd/nand/raw/brcmnand/bcma_nand.c
>> new file mode 100644
>> index 000000000000..e3be9ecf0761
>> --- /dev/null
>> +++ b/drivers/mtd/nand/raw/brcmnand/bcma_nand.c
>> @@ -0,0 +1,131 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright © 2021 Broadcom
>> + */
>> +#include <linux/bcma/bcma.h>
>> +#include <linux/bcma/bcma_driver_chipcommon.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "brcmnand.h"
>> +
>> +struct brcmnand_bcma_soc {
>> +	struct brcmnand_soc soc;
>> +	struct bcma_drv_cc *cc;
>> +};
>> +
>> +static inline bool brcmnand_bcma_needs_swapping(u32 offset)
>> +{
>> +	switch (offset) {
>> +	case BCMA_CC_NAND_SPARE_RD0:
>> +	case BCMA_CC_NAND_SPARE_RD4:
>> +	case BCMA_CC_NAND_SPARE_RD8:
>> +	case BCMA_CC_NAND_SPARE_RD12:
>> +	case BCMA_CC_NAND_SPARE_WR0:
>> +	case BCMA_CC_NAND_SPARE_WR4:
>> +	case BCMA_CC_NAND_SPARE_WR8:
>> +	case BCMA_CC_NAND_SPARE_WR12:
>> +	case BCMA_CC_NAND_DEVID:
>> +	case BCMA_CC_NAND_DEVID_X:
>> +	case BCMA_CC_NAND_SPARE_RD16:
>> +	case BCMA_CC_NAND_SPARE_RD20:
>> +	case BCMA_CC_NAND_SPARE_RD24:
>> +	case BCMA_CC_NAND_SPARE_RD28:
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static u32 brcmnand_bcma_read_reg(struct brcmnand_soc *soc, u32 offset)
>> +{
>> +	struct brcmnand_bcma_soc *sc;
>> +	u32 val;
>> +
>> +	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
>> +
>> +	/* Offset into the NAND block and deal with the flash cache separately */
>> +	if (offset == ~0)
>> +		offset = BCMA_CC_NAND_CACHE_DATA;
>> +	else
>> +		offset += BCMA_CC_NAND_REVISION;
>> +
>> +	val = bcma_cc_read32(sc->cc, offset);
>> +
>> +	/* Swap if necessary */
>> +	if (brcmnand_bcma_needs_swapping(offset))
>> +		val = be32_to_cpu(val);
>> +	return val;
>> +}
>> +
>> +static void brcmnand_bcma_write_reg(struct brcmnand_soc *soc, u32 val,
>> +				    u32 offset)
>> +{
>> +	struct brcmnand_bcma_soc *sc;
>> +
>> +	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
>> +
>> +	/* Offset into the NAND block */
>> +	if (offset == ~0)
>> +		offset = BCMA_CC_NAND_CACHE_DATA;
>> +	else
>> +		offset += BCMA_CC_NAND_REVISION;
>> +
>> +	/* Swap if necessary */
>> +	if (brcmnand_bcma_needs_swapping(offset))
>> +		val = cpu_to_be32(val);
>> +
>> +	bcma_cc_write32(sc->cc, offset, val);
>> +}
>> +
>> +static struct brcmnand_io_ops brcmnand_bcma_io_ops = {
>> +	.read_reg	= brcmnand_bcma_read_reg,
>> +	.write_reg	= brcmnand_bcma_write_reg,
>> +};
>> +
>> +static void brcmnand_bcma_prepare_data_bus(struct brcmnand_soc *soc, bool prepare,
>> +					   bool is_param)
>> +{
>> +	struct brcmnand_bcma_soc *sc;
>> +
>> +	sc = container_of(soc, struct brcmnand_bcma_soc, soc);
> 
> A helper for that would be nice?

Sure, I can add a helper since we use the same construct about 3 times, 
thanks!
-- 
Florian

Re: [PATCH 4/9] mtd: rawnand: brcmnand: Move OF operations out of brcmnand_init_cs()

[Miquel Raynal]

Hi Florian,

f.fainelli@gmail.com wrote on Mon, 3 Jan 2022 09:27:28 -0800:

> On 1/3/2022 8:56 AM, Miquel Raynal wrote:
> > Hi Florian,
> > 
> > f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:20 -0800:
> >   
> >> In order to initialize a given chip select object for use by the
> >> brcmnand driver, move all of the Device Tree specific routines outside
> >> of brcmnand_init_cs() in order to make it usable in a platform data
> >> configuration which will be necessary for supporting BCMA chips.  
> > 
> > TBH I'm note a big fan of the idea. I'm not sure going back to
> > supporting platform data this way really is a good idea... There are so
> > much things that are well described with DT that we now rely upon that
> > I am not entirely convinced by these changes :-/ The move is generally
> > in the other direction: getting rid of the legacy platform data.  
> 
> In the cover letter there is an explanation as to why we need to introduce platform data/device support here: the platforms on which this NAND controller shim is used do not have Device Tree support, and won't have it in the future either. They are old platforms (first SoC supported by bcm47xx is maybe 15 years old now) but they are still in active and wide use by the OpenWrt, dd-wrt communities.

Yeah, I read the cover letter, I understand these platforms won't ever
be updated so you're stuck. I'll close my eyes.

Thanks,
Miquèl

Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Miquel Raynal]

Hi Florian,

f.fainelli@gmail.com wrote on Mon, 3 Jan 2022 09:24:26 -0800:

> On 1/3/2022 8:49 AM, Miquel Raynal wrote:
> > Hi Florian,
> > 
> > f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:
> >   
> >> Allow a brcmnand_soc instance to provide a custom set of I/O operations
> >> which we will require when using this driver on a BCMA bus which is not
> >> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
> >> to use the SoC operations if provided.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
> >>   drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
> >>   2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index f75929783b94..7a1673b1b1af 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -594,13 +594,18 @@ enum {  
> >>   >>   static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)  
> >>   {
> >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >> +		return brcmnand_soc_read(ctrl->soc, offs);
> >>   	return brcmnand_readl(ctrl->nand_base + offs);
> >>   }  
> >>   >>   static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,  
> >>   				 u32 val)
> >>   {
> >> -	brcmnand_writel(val, ctrl->nand_base + offs);
> >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >> +		brcmnand_soc_write(ctrl->soc, val, offs);
> >> +	else
> >> +		brcmnand_writel(val, ctrl->nand_base + offs);
> >>   }  
> >>   >>   static int brcmnand_revision_init(struct brcmnand_controller *ctrl)  
> >> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,  
> >>   >>   static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)  
> >>   {
> >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >> +		return brcmnand_soc_read(ctrl->soc, ~0);
> >>   	return __raw_readl(ctrl->nand_fc + word * 4);
> >>   }  
> >>   >>   static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,  
> >>   				     int word, u32 val)
> >>   {
> >> -	__raw_writel(val, ctrl->nand_fc + word * 4);
> >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >> +		brcmnand_soc_write(ctrl->soc, val, ~0);
> >> +	else
> >> +		__raw_writel(val, ctrl->nand_fc + word * 4);
> >>   }  
> >>   >>   static inline void edu_writel(struct brcmnand_controller *ctrl,  
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >> index eb498fbe505e..a3f2ad5f6572 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >> @@ -11,12 +11,19 @@  
> >>   >>   struct platform_device;  
> >>   struct dev_pm_ops;
> >> +struct brcmnand_io_ops;  
> >>   >>   struct brcmnand_soc {  
> >>   	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
> >>   	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
> >>   	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
> >>   				 bool is_param);
> >> +	const struct brcmnand_io_ops *ops;
> >> +};
> >> +
> >> +struct brcmnand_io_ops {
> >> +	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
> >> +	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
> >>   };  
> >>   >>   static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,  
> >> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
> >>   		writel_relaxed(val, addr);
> >>   }  
> >>   >> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)  
> >> +{
> >> +	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
> >> +}
> >> +
> >> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
> >> +{
> >> +	return soc->ops->read_reg(soc, offset);
> >> +}
> >> +
> >> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
> >> +				      u32 offset)
> >> +{
> >> +	soc->ops->write_reg(soc, val, offset);
> >> +}
> >> +  
> > 
> > It might be worth looking into more optimized ways to do these checks,
> > in particular the read/write_reg ones because you're checking against
> > some static data which cannot be optimized out by the compiler but
> > won't change in the lifetime of the kernel.  
> 
> I suppose I could add an addition if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA) at the front of brcmnand_soc_has_ops(), would that address your concern or you have something else in mind?

I don't like much the #ifdef solution, instead you might think of
static keys, or even better using a regmap. Regmap implementation is
free, you can use either one way or the other and for almost no
overhead compared to the bunch of functions you have here.

Thanks,
Miquèl

Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Miquel Raynal]

Hi Miquel,

miquel.raynal@bootlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100:

> Hi Florian,
> 
> f.fainelli@gmail.com wrote on Mon, 3 Jan 2022 09:24:26 -0800:
> 
> > On 1/3/2022 8:49 AM, Miquel Raynal wrote:  
> > > Hi Florian,
> > > 
> > > f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:
> > >     
> > >> Allow a brcmnand_soc instance to provide a custom set of I/O operations
> > >> which we will require when using this driver on a BCMA bus which is not
> > >> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
> > >> to use the SoC operations if provided.
> > >>
> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > >> ---
> > >>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
> > >>   drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
> > >>   2 files changed, 35 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> index f75929783b94..7a1673b1b1af 100644
> > >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> @@ -594,13 +594,18 @@ enum {    
> > >>   >>   static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)    
> > >>   {
> > >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> > >> +		return brcmnand_soc_read(ctrl->soc, offs);
> > >>   	return brcmnand_readl(ctrl->nand_base + offs);
> > >>   }    
> > >>   >>   static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,    
> > >>   				 u32 val)
> > >>   {
> > >> -	brcmnand_writel(val, ctrl->nand_base + offs);
> > >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> > >> +		brcmnand_soc_write(ctrl->soc, val, offs);
> > >> +	else
> > >> +		brcmnand_writel(val, ctrl->nand_base + offs);
> > >>   }    
> > >>   >>   static int brcmnand_revision_init(struct brcmnand_controller *ctrl)    
> > >> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,    
> > >>   >>   static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)    
> > >>   {
> > >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> > >> +		return brcmnand_soc_read(ctrl->soc, ~0);
> > >>   	return __raw_readl(ctrl->nand_fc + word * 4);
> > >>   }    
> > >>   >>   static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,    
> > >>   				     int word, u32 val)
> > >>   {
> > >> -	__raw_writel(val, ctrl->nand_fc + word * 4);
> > >> +	if (brcmnand_soc_has_ops(ctrl->soc))
> > >> +		brcmnand_soc_write(ctrl->soc, val, ~0);
> > >> +	else
> > >> +		__raw_writel(val, ctrl->nand_fc + word * 4);
> > >>   }    
> > >>   >>   static inline void edu_writel(struct brcmnand_controller *ctrl,    
> > >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> > >> index eb498fbe505e..a3f2ad5f6572 100644
> > >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> > >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> > >> @@ -11,12 +11,19 @@    
> > >>   >>   struct platform_device;    
> > >>   struct dev_pm_ops;
> > >> +struct brcmnand_io_ops;    
> > >>   >>   struct brcmnand_soc {    
> > >>   	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
> > >>   	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
> > >>   	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
> > >>   				 bool is_param);
> > >> +	const struct brcmnand_io_ops *ops;
> > >> +};
> > >> +
> > >> +struct brcmnand_io_ops {
> > >> +	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
> > >> +	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
> > >>   };    
> > >>   >>   static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,    
> > >> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
> > >>   		writel_relaxed(val, addr);
> > >>   }    
> > >>   >> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)    
> > >> +{
> > >> +	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
> > >> +}
> > >> +
> > >> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
> > >> +{
> > >> +	return soc->ops->read_reg(soc, offset);
> > >> +}
> > >> +
> > >> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
> > >> +				      u32 offset)
> > >> +{
> > >> +	soc->ops->write_reg(soc, val, offset);
> > >> +}
> > >> +    
> > > 
> > > It might be worth looking into more optimized ways to do these checks,
> > > in particular the read/write_reg ones because you're checking against
> > > some static data which cannot be optimized out by the compiler but
> > > won't change in the lifetime of the kernel.    
> > 
> > I suppose I could add an addition if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA) at the front of brcmnand_soc_has_ops(), would that address your concern or you have something else in mind?  
> 
> I don't like much the #ifdef solution, instead you might think of
> static keys, or even better using a regmap. Regmap implementation is
> free, you can use either one way or the other and for almost no
> overhead compared to the bunch of functions you have here.

Maybe regmaps will actually be slower than these regular if's. Perhaps
static keys are the best option?

Cheers,
Miquèl

Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Florian Fainelli]

On 1/4/22 12:57 AM, Miquel Raynal wrote:
> Hi Miquel,
> 
> miquel.raynal@bootlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100:
> 
>> Hi Florian,
>>
>> f.fainelli@gmail.com wrote on Mon, 3 Jan 2022 09:24:26 -0800:
>>
>>> On 1/3/2022 8:49 AM, Miquel Raynal wrote:  
>>>> Hi Florian,
>>>>
>>>> f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:
>>>>     
>>>>> Allow a brcmnand_soc instance to provide a custom set of I/O operations
>>>>> which we will require when using this driver on a BCMA bus which is not
>>>>> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
>>>>> to use the SoC operations if provided.
>>>>>
>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>> ---
>>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
>>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
>>>>>   2 files changed, 35 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>> index f75929783b94..7a1673b1b1af 100644
>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>> @@ -594,13 +594,18 @@ enum {    
>>>>>   >>   static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)    
>>>>>   {
>>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>>>>> +		return brcmnand_soc_read(ctrl->soc, offs);
>>>>>   	return brcmnand_readl(ctrl->nand_base + offs);
>>>>>   }    
>>>>>   >>   static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,    
>>>>>   				 u32 val)
>>>>>   {
>>>>> -	brcmnand_writel(val, ctrl->nand_base + offs);
>>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>>>>> +		brcmnand_soc_write(ctrl->soc, val, offs);
>>>>> +	else
>>>>> +		brcmnand_writel(val, ctrl->nand_base + offs);
>>>>>   }    
>>>>>   >>   static int brcmnand_revision_init(struct brcmnand_controller *ctrl)    
>>>>> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,    
>>>>>   >>   static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)    
>>>>>   {
>>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>>>>> +		return brcmnand_soc_read(ctrl->soc, ~0);
>>>>>   	return __raw_readl(ctrl->nand_fc + word * 4);
>>>>>   }    
>>>>>   >>   static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,    
>>>>>   				     int word, u32 val)
>>>>>   {
>>>>> -	__raw_writel(val, ctrl->nand_fc + word * 4);
>>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
>>>>> +		brcmnand_soc_write(ctrl->soc, val, ~0);
>>>>> +	else
>>>>> +		__raw_writel(val, ctrl->nand_fc + word * 4);
>>>>>   }    
>>>>>   >>   static inline void edu_writel(struct brcmnand_controller *ctrl,    
>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>>>>> index eb498fbe505e..a3f2ad5f6572 100644
>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>>>>> @@ -11,12 +11,19 @@    
>>>>>   >>   struct platform_device;    
>>>>>   struct dev_pm_ops;
>>>>> +struct brcmnand_io_ops;    
>>>>>   >>   struct brcmnand_soc {    
>>>>>   	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
>>>>>   	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
>>>>>   	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
>>>>>   				 bool is_param);
>>>>> +	const struct brcmnand_io_ops *ops;
>>>>> +};
>>>>> +
>>>>> +struct brcmnand_io_ops {
>>>>> +	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
>>>>> +	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
>>>>>   };    
>>>>>   >>   static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,    
>>>>> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
>>>>>   		writel_relaxed(val, addr);
>>>>>   }    
>>>>>   >> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)    
>>>>> +{
>>>>> +	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
>>>>> +}
>>>>> +
>>>>> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
>>>>> +{
>>>>> +	return soc->ops->read_reg(soc, offset);
>>>>> +}
>>>>> +
>>>>> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
>>>>> +				      u32 offset)
>>>>> +{
>>>>> +	soc->ops->write_reg(soc, val, offset);
>>>>> +}
>>>>> +    
>>>>
>>>> It might be worth looking into more optimized ways to do these checks,
>>>> in particular the read/write_reg ones because you're checking against
>>>> some static data which cannot be optimized out by the compiler but
>>>> won't change in the lifetime of the kernel.    
>>>
>>> I suppose I could add an addition if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA) at the front of brcmnand_soc_has_ops(), would that address your concern or you have something else in mind?  
>>
>> I don't like much the #ifdef solution, instead you might think of
>> static keys, or even better using a regmap. Regmap implementation is
>> free, you can use either one way or the other and for almost no
>> overhead compared to the bunch of functions you have here.
> 
> Maybe regmaps will actually be slower than these regular if's. Perhaps
> static keys are the best option?

OK static keys would probably work. I am not sure that the additional
branches for each register access would actually be causing a noticeable
performance impact. Pretty much any chip where this controller is used
has a DMA interface that you program and kick, the PIO is already
assumed to be slow, and each register access is about 200ns on STB chips
at least.
-- 
Florian

Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

[Miquel Raynal]

Hi Florian,

f.fainelli@gmail.com wrote on Tue, 4 Jan 2022 10:34:35 -0800:

> On 1/4/22 12:57 AM, Miquel Raynal wrote:
> > Hi Miquel,
> > 
> > miquel.raynal@bootlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100:
> >   
> >> Hi Florian,
> >>
> >> f.fainelli@gmail.com wrote on Mon, 3 Jan 2022 09:24:26 -0800:
> >>  
> >>> On 1/3/2022 8:49 AM, Miquel Raynal wrote:    
> >>>> Hi Florian,
> >>>>
> >>>> f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:
> >>>>       
> >>>>> Allow a brcmnand_soc instance to provide a custom set of I/O operations
> >>>>> which we will require when using this driver on a BCMA bus which is not
> >>>>> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
> >>>>> to use the SoC operations if provided.
> >>>>>
> >>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >>>>> ---
> >>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
> >>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
> >>>>>   2 files changed, 35 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >>>>> index f75929783b94..7a1673b1b1af 100644
> >>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >>>>> @@ -594,13 +594,18 @@ enum {      
> >>>>>   >>   static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)      
> >>>>>   {
> >>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >>>>> +		return brcmnand_soc_read(ctrl->soc, offs);
> >>>>>   	return brcmnand_readl(ctrl->nand_base + offs);
> >>>>>   }      
> >>>>>   >>   static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,      
> >>>>>   				 u32 val)
> >>>>>   {
> >>>>> -	brcmnand_writel(val, ctrl->nand_base + offs);
> >>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >>>>> +		brcmnand_soc_write(ctrl->soc, val, offs);
> >>>>> +	else
> >>>>> +		brcmnand_writel(val, ctrl->nand_base + offs);
> >>>>>   }      
> >>>>>   >>   static int brcmnand_revision_init(struct brcmnand_controller *ctrl)      
> >>>>> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,      
> >>>>>   >>   static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)      
> >>>>>   {
> >>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >>>>> +		return brcmnand_soc_read(ctrl->soc, ~0);
> >>>>>   	return __raw_readl(ctrl->nand_fc + word * 4);
> >>>>>   }      
> >>>>>   >>   static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,      
> >>>>>   				     int word, u32 val)
> >>>>>   {
> >>>>> -	__raw_writel(val, ctrl->nand_fc + word * 4);
> >>>>> +	if (brcmnand_soc_has_ops(ctrl->soc))
> >>>>> +		brcmnand_soc_write(ctrl->soc, val, ~0);
> >>>>> +	else
> >>>>> +		__raw_writel(val, ctrl->nand_fc + word * 4);
> >>>>>   }      
> >>>>>   >>   static inline void edu_writel(struct brcmnand_controller *ctrl,      
> >>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >>>>> index eb498fbe505e..a3f2ad5f6572 100644
> >>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >>>>> @@ -11,12 +11,19 @@      
> >>>>>   >>   struct platform_device;      
> >>>>>   struct dev_pm_ops;
> >>>>> +struct brcmnand_io_ops;      
> >>>>>   >>   struct brcmnand_soc {      
> >>>>>   	bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
> >>>>>   	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
> >>>>>   	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
> >>>>>   				 bool is_param);
> >>>>> +	const struct brcmnand_io_ops *ops;
> >>>>> +};
> >>>>> +
> >>>>> +struct brcmnand_io_ops {
> >>>>> +	u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
> >>>>> +	void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
> >>>>>   };      
> >>>>>   >>   static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,      
> >>>>> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
> >>>>>   		writel_relaxed(val, addr);
> >>>>>   }      
> >>>>>   >> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)      
> >>>>> +{
> >>>>> +	return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
> >>>>> +}
> >>>>> +
> >>>>> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
> >>>>> +{
> >>>>> +	return soc->ops->read_reg(soc, offset);
> >>>>> +}
> >>>>> +
> >>>>> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
> >>>>> +				      u32 offset)
> >>>>> +{
> >>>>> +	soc->ops->write_reg(soc, val, offset);
> >>>>> +}
> >>>>> +      
> >>>>
> >>>> It might be worth looking into more optimized ways to do these checks,
> >>>> in particular the read/write_reg ones because you're checking against
> >>>> some static data which cannot be optimized out by the compiler but
> >>>> won't change in the lifetime of the kernel.      
> >>>
> >>> I suppose I could add an addition if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA) at the front of brcmnand_soc_has_ops(), would that address your concern or you have something else in mind?    
> >>
> >> I don't like much the #ifdef solution, instead you might think of
> >> static keys, or even better using a regmap. Regmap implementation is
> >> free, you can use either one way or the other and for almost no
> >> overhead compared to the bunch of functions you have here.  
> > 
> > Maybe regmaps will actually be slower than these regular if's. Perhaps
> > static keys are the best option?  
> 
> OK static keys would probably work. I am not sure that the additional
> branches for each register access would actually be causing a noticeable
> performance impact. Pretty much any chip where this controller is used
> has a DMA interface that you program and kick, the PIO is already
> assumed to be slow, and each register access is about 200ns on STB chips
> at least.

I see. I'll let you decide what you prefer, I won't block if you stick
to the regular if/else implementation.

Thanks,
Miquèl