Refactored NAND ECC and CMD address configuration code to use inline
functions.
Signed-off-by: Kamal Dasu <[email protected]>
---
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 100 +++++++++++++++++++------------
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index ce0b8ff..77b7850 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -588,6 +588,54 @@ static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
__raw_writel(val, ctrl->nand_fc + word * 4);
}
+static inline void brcmnand_clear_ecc_addr(struct brcmnand_controller *ctrl)
+{
+
+ /* Clear error addresses */
+ brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0);
+ brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0);
+ brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0);
+ brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0);
+}
+
+static inline u64 brcmnand_get_uncorrecc_addr(struct brcmnand_controller *ctrl)
+{
+ u64 err_addr;
+
+ err_addr = brcmnand_read_reg(ctrl, BRCMNAND_UNCORR_ADDR);
+ err_addr |= ((u64)(brcmnand_read_reg(ctrl,
+ BRCMNAND_UNCORR_EXT_ADDR)
+ & 0xffff) << 32);
+
+ return err_addr;
+}
+
+static inline u64 brcmnand_get_correcc_addr(struct brcmnand_controller *ctrl)
+{
+ u64 err_addr;
+
+ err_addr = brcmnand_read_reg(ctrl, BRCMNAND_CORR_ADDR);
+ err_addr |= ((u64)(brcmnand_read_reg(ctrl,
+ BRCMNAND_CORR_EXT_ADDR)
+ & 0xffff) << 32);
+
+ return err_addr;
+}
+
+static inline void brcmnand_set_cmd_addr(struct mtd_info *mtd, u64 addr)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct brcmnand_host *host = nand_get_controller_data(chip);
+ struct brcmnand_controller *ctrl = host->ctrl;
+
+ brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
+ (host->cs << 16) | ((addr >> 32) & 0xffff));
+ (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
+ brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
+ lower_32_bits(addr));
+ (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
+}
+
static inline u16 brcmnand_cs_offset(struct brcmnand_controller *ctrl, int cs,
enum brcmnand_cs_reg reg)
{
@@ -1213,9 +1261,12 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
{
struct brcmnand_controller *ctrl = host->ctrl;
int ret;
+ u64 cmd_addr;
+
+ cmd_addr = brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
+
+ dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
- dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
- brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
BUG_ON(ctrl->cmd_pending != 0);
ctrl->cmd_pending = cmd;
@@ -1374,12 +1425,7 @@ static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
if (!native_cmd)
return;
- brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
- (host->cs << 16) | ((addr >> 32) & 0xffff));
- (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
- brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS, lower_32_bits(addr));
- (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
-
+ brcmnand_set_cmd_addr(mtd, addr);
brcmnand_send_cmd(host, native_cmd);
brcmnand_waitfunc(chip);
@@ -1597,20 +1643,10 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
struct brcmnand_controller *ctrl = host->ctrl;
int i, j, ret = 0;
- /* Clear error addresses */
- brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0);
- brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0);
- brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0);
- brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0);
-
- brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
- (host->cs << 16) | ((addr >> 32) & 0xffff));
- (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
+ brcmnand_clear_ecc_addr(ctrl);
for (i = 0; i < trans; i++, addr += FC_BYTES) {
- brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
- lower_32_bits(addr));
- (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
+ brcmnand_set_cmd_addr(mtd, addr);
/* SPARE_AREA_READ does not use ECC, so just use PAGE_READ */
brcmnand_send_cmd(host, CMD_PAGE_READ);
brcmnand_waitfunc(chip);
@@ -1630,21 +1666,15 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
host->hwcfg.sector_size_1k);
if (!ret) {
- *err_addr = brcmnand_read_reg(ctrl,
- BRCMNAND_UNCORR_ADDR) |
- ((u64)(brcmnand_read_reg(ctrl,
- BRCMNAND_UNCORR_EXT_ADDR)
- & 0xffff) << 32);
+ *err_addr = brcmnand_get_uncorrecc_addr(ctrl);
+
if (*err_addr)
ret = -EBADMSG;
}
if (!ret) {
- *err_addr = brcmnand_read_reg(ctrl,
- BRCMNAND_CORR_ADDR) |
- ((u64)(brcmnand_read_reg(ctrl,
- BRCMNAND_CORR_EXT_ADDR)
- & 0xffff) << 32);
+ *err_addr = brcmnand_get_correcc_addr(ctrl);
+
if (*err_addr)
ret = -EUCLEAN;
}
@@ -1711,7 +1741,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);
try_dmaread:
- brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_COUNT, 0);
+ brcmnand_clear_ecc_addr(ctrl);
if (has_flash_dma(ctrl) && !oob && flash_dma_buf_ok(buf)) {
err = brcmnand_dma_trans(host, addr, buf, trans * FC_BYTES,
@@ -1858,15 +1888,9 @@ static int brcmnand_write(struct mtd_info *mtd, struct nand_chip *chip,
goto out;
}
- brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
- (host->cs << 16) | ((addr >> 32) & 0xffff));
- (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
-
for (i = 0; i < trans; i++, addr += FC_BYTES) {
/* full address MUST be set before populating FC */
- brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
- lower_32_bits(addr));
- (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
+ brcmnand_set_cmd_addr(mtd, addr);
if (buf) {
brcmnand_soc_data_bus_prepare(ctrl->soc, false);
--
1.9.0.138.g2de3478
This change adds support for brcm NAND v7.3 controller. This controller
uses a newer version of flash_dma engine and change mostly implements
these differences.
Signed-off-by: Kamal Dasu <[email protected]>
---
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 102 ++++++++++++++++++++++++-------
1 file changed, 80 insertions(+), 22 deletions(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 77b7850..544088f 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -92,6 +92,12 @@ struct brcm_nand_dma_desc {
#define FLASH_DMA_ECC_ERROR (1 << 8)
#define FLASH_DMA_CORR_ERROR (1 << 9)
+/* Bitfields for DMA_MODE */
+#define FLASH_DMA_MODE_STOP_ON_ERROR BIT(1) /* stop in Uncorr ECC error */
+#define FLASH_DMA_MODE_MODE BIT(0) /* link list */
+#define FLASH_DMA_MODE_MASK (FLASH_DMA_MODE_STOP_ON_ERROR | \
+ FLASH_DMA_MODE_MODE)
+
/* 512B flash cache in the NAND controller HW */
#define FC_SHIFT 9U
#define FC_BYTES 512U
@@ -104,6 +110,51 @@ struct brcm_nand_dma_desc {
#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY)
#define NAND_POLL_STATUS_TIMEOUT_MS 100
+/* flash_dma registers */
+enum flash_dma_reg {
+ FLASH_DMA_REVISION = 0,
+ FLASH_DMA_FIRST_DESC,
+ FLASH_DMA_FIRST_DESC_EXT,
+ FLASH_DMA_CTRL,
+ FLASH_DMA_MODE,
+ FLASH_DMA_STATUS,
+ FLASH_DMA_INTERRUPT_DESC,
+ FLASH_DMA_INTERRUPT_DESC_EXT,
+ FLASH_DMA_ERROR_STATUS,
+ FLASH_DMA_CURRENT_DESC,
+ FLASH_DMA_CURRENT_DESC_EXT,
+};
+
+/* flash_dma registers v1*/
+static const u16 flash_dma_regs_v1[] = {
+ [FLASH_DMA_REVISION] = 0x00,
+ [FLASH_DMA_FIRST_DESC] = 0x04,
+ [FLASH_DMA_FIRST_DESC_EXT] = 0x08,
+ [FLASH_DMA_CTRL] = 0x0c,
+ [FLASH_DMA_MODE] = 0x10,
+ [FLASH_DMA_STATUS] = 0x14,
+ [FLASH_DMA_INTERRUPT_DESC] = 0x18,
+ [FLASH_DMA_INTERRUPT_DESC_EXT] = 0x1c,
+ [FLASH_DMA_ERROR_STATUS] = 0x20,
+ [FLASH_DMA_CURRENT_DESC] = 0x24,
+ [FLASH_DMA_CURRENT_DESC_EXT] = 0x28,
+};
+
+/* flash_dma registers v4 */
+static const u16 flash_dma_regs_v4[] = {
+ [FLASH_DMA_REVISION] = 0x00,
+ [FLASH_DMA_FIRST_DESC] = 0x08,
+ [FLASH_DMA_FIRST_DESC_EXT] = 0x0c,
+ [FLASH_DMA_CTRL] = 0x10,
+ [FLASH_DMA_MODE] = 0x14,
+ [FLASH_DMA_STATUS] = 0x18,
+ [FLASH_DMA_INTERRUPT_DESC] = 0x20,
+ [FLASH_DMA_INTERRUPT_DESC_EXT] = 0x24,
+ [FLASH_DMA_ERROR_STATUS] = 0x28,
+ [FLASH_DMA_CURRENT_DESC] = 0x30,
+ [FLASH_DMA_CURRENT_DESC_EXT] = 0x34,
+};
+
/* Controller feature flags */
enum {
BRCMNAND_HAS_1K_SECTORS = BIT(0),
@@ -136,6 +187,8 @@ struct brcmnand_controller {
/* List of NAND hosts (one for each chip-select) */
struct list_head host_list;
+ /* flash_dma reg */
+ const u16 *flash_dma_offsets;
struct brcm_nand_dma_desc *dma_desc;
dma_addr_t dma_pa;
@@ -470,7 +523,7 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
/* Register offsets */
if (ctrl->nand_version >= 0x0702)
ctrl->reg_offsets = brcmnand_regs_v72;
- else if (ctrl->nand_version >= 0x0701)
+ else if (ctrl->nand_version == 0x0701)
ctrl->reg_offsets = brcmnand_regs_v71;
else if (ctrl->nand_version >= 0x0600)
ctrl->reg_offsets = brcmnand_regs_v60;
@@ -515,7 +568,7 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
}
/* Maximum spare area sector size (per 512B) */
- if (ctrl->nand_version >= 0x0702)
+ if (ctrl->nand_version == 0x0702)
ctrl->max_oob = 128;
else if (ctrl->nand_version >= 0x0600)
ctrl->max_oob = 64;
@@ -546,6 +599,15 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
return 0;
}
+static void brcmnand_flash_dma_revision_init(struct brcmnand_controller *ctrl)
+{
+ /* flash_dma register offsets */
+ if (ctrl->nand_version >= 0x0703)
+ ctrl->flash_dma_offsets = flash_dma_regs_v4;
+ else
+ ctrl->flash_dma_offsets = flash_dma_regs_v1;
+}
+
static inline u32 brcmnand_read_reg(struct brcmnand_controller *ctrl,
enum brcmnand_reg reg)
{
@@ -668,7 +730,7 @@ static void brcmnand_wr_corr_thresh(struct brcmnand_host *host, u8 val)
enum brcmnand_reg reg = BRCMNAND_CORR_THRESHOLD;
int cs = host->cs;
- if (ctrl->nand_version >= 0x0702)
+ if (ctrl->nand_version == 0x0702)
bits = 7;
else if (ctrl->nand_version >= 0x0600)
bits = 6;
@@ -722,7 +784,7 @@ enum {
static inline u32 brcmnand_spare_area_mask(struct brcmnand_controller *ctrl)
{
- if (ctrl->nand_version >= 0x0702)
+ if (ctrl->nand_version == 0x0702)
return GENMASK(7, 0);
else if (ctrl->nand_version >= 0x0600)
return GENMASK(6, 0);
@@ -852,20 +914,6 @@ static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
* Flash DMA
***********************************************************************/
-enum flash_dma_reg {
- FLASH_DMA_REVISION = 0x00,
- FLASH_DMA_FIRST_DESC = 0x04,
- FLASH_DMA_FIRST_DESC_EXT = 0x08,
- FLASH_DMA_CTRL = 0x0c,
- FLASH_DMA_MODE = 0x10,
- FLASH_DMA_STATUS = 0x14,
- FLASH_DMA_INTERRUPT_DESC = 0x18,
- FLASH_DMA_INTERRUPT_DESC_EXT = 0x1c,
- FLASH_DMA_ERROR_STATUS = 0x20,
- FLASH_DMA_CURRENT_DESC = 0x24,
- FLASH_DMA_CURRENT_DESC_EXT = 0x28,
-};
-
static inline bool has_flash_dma(struct brcmnand_controller *ctrl)
{
return ctrl->flash_dma_base;
@@ -877,14 +925,19 @@ static inline bool flash_dma_buf_ok(const void *buf)
likely(IS_ALIGNED((uintptr_t)buf, 4));
}
-static inline void flash_dma_writel(struct brcmnand_controller *ctrl, u8 offs,
- u32 val)
+static inline void flash_dma_writel(struct brcmnand_controller *ctrl,
+ enum flash_dma_reg dma_reg, u32 val)
{
+ u16 offs = ctrl->flash_dma_offsets[dma_reg];
+
brcmnand_writel(val, ctrl->flash_dma_base + offs);
}
-static inline u32 flash_dma_readl(struct brcmnand_controller *ctrl, u8 offs)
+static inline u32 flash_dma_readl(struct brcmnand_controller *ctrl,
+ enum flash_dma_reg dma_reg)
{
+ u16 offs = ctrl->flash_dma_offsets[dma_reg];
+
return brcmnand_readl(ctrl->flash_dma_base + offs);
}
@@ -2427,6 +2480,7 @@ static int brcmnand_resume(struct device *dev)
{ .compatible = "brcm,brcmnand-v7.0" },
{ .compatible = "brcm,brcmnand-v7.1" },
{ .compatible = "brcm,brcmnand-v7.2" },
+ { .compatible = "brcm,brcmnand-v7.3" },
{},
};
MODULE_DEVICE_TABLE(of, brcmnand_of_match);
@@ -2513,7 +2567,11 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
goto err;
}
- flash_dma_writel(ctrl, FLASH_DMA_MODE, 1); /* linked-list */
+ /* initialize the dma version */
+ brcmnand_flash_dma_revision_init(ctrl);
+
+ /* linked-list and stop on error */
+ flash_dma_writel(ctrl, FLASH_DMA_MODE, FLASH_DMA_MODE_MASK);
flash_dma_writel(ctrl, FLASH_DMA_ERROR_STATUS, 0);
/* Allocate descriptor(s) */
--
1.9.0.138.g2de3478
On Thu, 30 May 2019 17:20:35 -0400
Kamal Dasu <[email protected]> wrote:
> Refactored NAND ECC and CMD address configuration code to use inline
> functions.
I'd expect the compiler to be smart enough to decide when inlining is
appropriate. Did you check that adding the inline specifier actually
makes a difference?
>
> Signed-off-by: Kamal Dasu <[email protected]>
> ---
> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 100 +++++++++++++++++++------------
> 1 file changed, 62 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index ce0b8ff..77b7850 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -588,6 +588,54 @@ static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
> __raw_writel(val, ctrl->nand_fc + word * 4);
> }
>
> +static inline void brcmnand_clear_ecc_addr(struct brcmnand_controller *ctrl)
> +{
> +
> + /* Clear error addresses */
> + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0);
> + brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0);
> + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0);
> + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0);
> +}
> +
> +static inline u64 brcmnand_get_uncorrecc_addr(struct brcmnand_controller *ctrl)
> +{
> + u64 err_addr;
> +
> + err_addr = brcmnand_read_reg(ctrl, BRCMNAND_UNCORR_ADDR);
> + err_addr |= ((u64)(brcmnand_read_reg(ctrl,
> + BRCMNAND_UNCORR_EXT_ADDR)
> + & 0xffff) << 32);
> +
> + return err_addr;
> +}
> +
> +static inline u64 brcmnand_get_correcc_addr(struct brcmnand_controller *ctrl)
> +{
> + u64 err_addr;
> +
> + err_addr = brcmnand_read_reg(ctrl, BRCMNAND_CORR_ADDR);
> + err_addr |= ((u64)(brcmnand_read_reg(ctrl,
> + BRCMNAND_CORR_EXT_ADDR)
> + & 0xffff) << 32);
> +
> + return err_addr;
> +}
> +
> +static inline void brcmnand_set_cmd_addr(struct mtd_info *mtd, u64 addr)
> +{
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct brcmnand_host *host = nand_get_controller_data(chip);
> + struct brcmnand_controller *ctrl = host->ctrl;
> +
> + brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> + (host->cs << 16) | ((addr >> 32) & 0xffff));
> + (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> + brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
> + lower_32_bits(addr));
> + (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> +}
> +
> static inline u16 brcmnand_cs_offset(struct brcmnand_controller *ctrl, int cs,
> enum brcmnand_cs_reg reg)
> {
> @@ -1213,9 +1261,12 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> {
> struct brcmnand_controller *ctrl = host->ctrl;
> int ret;
> + u64 cmd_addr;
> +
> + cmd_addr = brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> +
> + dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>
> - dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
> - brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
> BUG_ON(ctrl->cmd_pending != 0);
> ctrl->cmd_pending = cmd;
>
> @@ -1374,12 +1425,7 @@ static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
> if (!native_cmd)
> return;
>
> - brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> - (host->cs << 16) | ((addr >> 32) & 0xffff));
> - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> - brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS, lower_32_bits(addr));
> - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> -
> + brcmnand_set_cmd_addr(mtd, addr);
> brcmnand_send_cmd(host, native_cmd);
> brcmnand_waitfunc(chip);
>
> @@ -1597,20 +1643,10 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
> struct brcmnand_controller *ctrl = host->ctrl;
> int i, j, ret = 0;
>
> - /* Clear error addresses */
> - brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0);
> - brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0);
> - brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0);
> - brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0);
> -
> - brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> - (host->cs << 16) | ((addr >> 32) & 0xffff));
> - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> + brcmnand_clear_ecc_addr(ctrl);
>
> for (i = 0; i < trans; i++, addr += FC_BYTES) {
> - brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
> - lower_32_bits(addr));
> - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> + brcmnand_set_cmd_addr(mtd, addr);
> /* SPARE_AREA_READ does not use ECC, so just use PAGE_READ */
> brcmnand_send_cmd(host, CMD_PAGE_READ);
> brcmnand_waitfunc(chip);
> @@ -1630,21 +1666,15 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
> host->hwcfg.sector_size_1k);
>
> if (!ret) {
> - *err_addr = brcmnand_read_reg(ctrl,
> - BRCMNAND_UNCORR_ADDR) |
> - ((u64)(brcmnand_read_reg(ctrl,
> - BRCMNAND_UNCORR_EXT_ADDR)
> - & 0xffff) << 32);
> + *err_addr = brcmnand_get_uncorrecc_addr(ctrl);
> +
> if (*err_addr)
> ret = -EBADMSG;
> }
>
> if (!ret) {
> - *err_addr = brcmnand_read_reg(ctrl,
> - BRCMNAND_CORR_ADDR) |
> - ((u64)(brcmnand_read_reg(ctrl,
> - BRCMNAND_CORR_EXT_ADDR)
> - & 0xffff) << 32);
> + *err_addr = brcmnand_get_correcc_addr(ctrl);
> +
> if (*err_addr)
> ret = -EUCLEAN;
> }
> @@ -1711,7 +1741,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
> dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);
>
> try_dmaread:
> - brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_COUNT, 0);
> + brcmnand_clear_ecc_addr(ctrl);
>
> if (has_flash_dma(ctrl) && !oob && flash_dma_buf_ok(buf)) {
> err = brcmnand_dma_trans(host, addr, buf, trans * FC_BYTES,
> @@ -1858,15 +1888,9 @@ static int brcmnand_write(struct mtd_info *mtd, struct nand_chip *chip,
> goto out;
> }
>
> - brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> - (host->cs << 16) | ((addr >> 32) & 0xffff));
> - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> -
> for (i = 0; i < trans; i++, addr += FC_BYTES) {
> /* full address MUST be set before populating FC */
> - brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
> - lower_32_bits(addr));
> - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> + brcmnand_set_cmd_addr(mtd, addr);
>
> if (buf) {
> brcmnand_soc_data_bus_prepare(ctrl->soc, false);
Boris,
On Sat, Jun 1, 2019 at 3:57 AM Boris Brezillon
<[email protected]> wrote:
>
> On Thu, 30 May 2019 17:20:35 -0400
> Kamal Dasu <[email protected]> wrote:
>
> > Refactored NAND ECC and CMD address configuration code to use inline
> > functions.
>
> I'd expect the compiler to be smart enough to decide when inlining is
> appropriate. Did you check that adding the inline specifier actually
> makes a difference?
This was done to make the code more readable. It does not make any
difference to performance.
>
> >
> > Signed-off-by: Kamal Dasu <[email protected]>
> > ---
> > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 100 +++++++++++++++++++------------
> > 1 file changed, 62 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > index ce0b8ff..77b7850 100644
> > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > @@ -588,6 +588,54 @@ static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
> > __raw_writel(val, ctrl->nand_fc + word * 4);
> > }
> >
> > +static inline void brcmnand_clear_ecc_addr(struct brcmnand_controller *ctrl)
> > +{
> > +
> > + /* Clear error addresses */
> > + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0);
> > + brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0);
> > + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0);
> > + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0);
> > +}
> > +
> > +static inline u64 brcmnand_get_uncorrecc_addr(struct brcmnand_controller *ctrl)
> > +{
> > + u64 err_addr;
> > +
> > + err_addr = brcmnand_read_reg(ctrl, BRCMNAND_UNCORR_ADDR);
> > + err_addr |= ((u64)(brcmnand_read_reg(ctrl,
> > + BRCMNAND_UNCORR_EXT_ADDR)
> > + & 0xffff) << 32);
> > +
> > + return err_addr;
> > +}
> > +
> > +static inline u64 brcmnand_get_correcc_addr(struct brcmnand_controller *ctrl)
> > +{
> > + u64 err_addr;
> > +
> > + err_addr = brcmnand_read_reg(ctrl, BRCMNAND_CORR_ADDR);
> > + err_addr |= ((u64)(brcmnand_read_reg(ctrl,
> > + BRCMNAND_CORR_EXT_ADDR)
> > + & 0xffff) << 32);
> > +
> > + return err_addr;
> > +}
> > +
> > +static inline void brcmnand_set_cmd_addr(struct mtd_info *mtd, u64 addr)
> > +{
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > + struct brcmnand_host *host = nand_get_controller_data(chip);
> > + struct brcmnand_controller *ctrl = host->ctrl;
> > +
> > + brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> > + (host->cs << 16) | ((addr >> 32) & 0xffff));
> > + (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> > + brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
> > + lower_32_bits(addr));
> > + (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> > +}
> > +
> > static inline u16 brcmnand_cs_offset(struct brcmnand_controller *ctrl, int cs,
> > enum brcmnand_cs_reg reg)
> > {
> > @@ -1213,9 +1261,12 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> > {
> > struct brcmnand_controller *ctrl = host->ctrl;
> > int ret;
> > + u64 cmd_addr;
> > +
> > + cmd_addr = brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> > +
> > + dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
> >
> > - dev_dbg(ctrl->dev, "send native cmd %d addr_lo 0x%x\n", cmd,
> > - brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS));
> > BUG_ON(ctrl->cmd_pending != 0);
> > ctrl->cmd_pending = cmd;
> >
> > @@ -1374,12 +1425,7 @@ static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
> > if (!native_cmd)
> > return;
> >
> > - brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> > - (host->cs << 16) | ((addr >> 32) & 0xffff));
> > - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> > - brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS, lower_32_bits(addr));
> > - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> > -
> > + brcmnand_set_cmd_addr(mtd, addr);
> > brcmnand_send_cmd(host, native_cmd);
> > brcmnand_waitfunc(chip);
> >
> > @@ -1597,20 +1643,10 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
> > struct brcmnand_controller *ctrl = host->ctrl;
> > int i, j, ret = 0;
> >
> > - /* Clear error addresses */
> > - brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0);
> > - brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0);
> > - brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0);
> > - brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0);
> > -
> > - brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> > - (host->cs << 16) | ((addr >> 32) & 0xffff));
> > - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> > + brcmnand_clear_ecc_addr(ctrl);
> >
> > for (i = 0; i < trans; i++, addr += FC_BYTES) {
> > - brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
> > - lower_32_bits(addr));
> > - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> > + brcmnand_set_cmd_addr(mtd, addr);
> > /* SPARE_AREA_READ does not use ECC, so just use PAGE_READ */
> > brcmnand_send_cmd(host, CMD_PAGE_READ);
> > brcmnand_waitfunc(chip);
> > @@ -1630,21 +1666,15 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
> > host->hwcfg.sector_size_1k);
> >
> > if (!ret) {
> > - *err_addr = brcmnand_read_reg(ctrl,
> > - BRCMNAND_UNCORR_ADDR) |
> > - ((u64)(brcmnand_read_reg(ctrl,
> > - BRCMNAND_UNCORR_EXT_ADDR)
> > - & 0xffff) << 32);
> > + *err_addr = brcmnand_get_uncorrecc_addr(ctrl);
> > +
> > if (*err_addr)
> > ret = -EBADMSG;
> > }
> >
> > if (!ret) {
> > - *err_addr = brcmnand_read_reg(ctrl,
> > - BRCMNAND_CORR_ADDR) |
> > - ((u64)(brcmnand_read_reg(ctrl,
> > - BRCMNAND_CORR_EXT_ADDR)
> > - & 0xffff) << 32);
> > + *err_addr = brcmnand_get_correcc_addr(ctrl);
> > +
> > if (*err_addr)
> > ret = -EUCLEAN;
> > }
> > @@ -1711,7 +1741,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
> > dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);
> >
> > try_dmaread:
> > - brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_COUNT, 0);
> > + brcmnand_clear_ecc_addr(ctrl);
> >
> > if (has_flash_dma(ctrl) && !oob && flash_dma_buf_ok(buf)) {
> > err = brcmnand_dma_trans(host, addr, buf, trans * FC_BYTES,
> > @@ -1858,15 +1888,9 @@ static int brcmnand_write(struct mtd_info *mtd, struct nand_chip *chip,
> > goto out;
> > }
> >
> > - brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS,
> > - (host->cs << 16) | ((addr >> 32) & 0xffff));
> > - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS);
> > -
> > for (i = 0; i < trans; i++, addr += FC_BYTES) {
> > /* full address MUST be set before populating FC */
> > - brcmnand_write_reg(ctrl, BRCMNAND_CMD_ADDRESS,
> > - lower_32_bits(addr));
> > - (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS);
> > + brcmnand_set_cmd_addr(mtd, addr);
> >
> > if (buf) {
> > brcmnand_soc_data_bus_prepare(ctrl->soc, false);
>
Thanks
Kamal
On Mon, 3 Jun 2019 10:11:20 -0400
Kamal Dasu <[email protected]> wrote:
> Boris,
>
> On Sat, Jun 1, 2019 at 3:57 AM Boris Brezillon
> <[email protected]> wrote:
> >
> > On Thu, 30 May 2019 17:20:35 -0400
> > Kamal Dasu <[email protected]> wrote:
> >
> > > Refactored NAND ECC and CMD address configuration code to use inline
> > > functions.
> >
> > I'd expect the compiler to be smart enough to decide when inlining is
> > appropriate. Did you check that adding the inline specifier actually
> > makes a difference?
>
> This was done to make the code more readable. It does not make any
> difference to performance.
I meant dropping the inline specifier, not going back to manual
inlining. As a general rule, you don't need to add the 'inline'
specifier unless your function is defined in a header. In all other
cases the compiler is able to inline things on its own when it sees the
number of instructions is small enough or when the function is only
called once.
On Mon, Jun 3, 2019 at 10:18 AM Boris Brezillon
<[email protected]> wrote:
>
> On Mon, 3 Jun 2019 10:11:20 -0400
> Kamal Dasu <[email protected]> wrote:
>
> > Boris,
> >
> > On Sat, Jun 1, 2019 at 3:57 AM Boris Brezillon
> > <[email protected]> wrote:
> > >
> > > On Thu, 30 May 2019 17:20:35 -0400
> > > Kamal Dasu <[email protected]> wrote:
> > >
> > > > Refactored NAND ECC and CMD address configuration code to use inline
> > > > functions.
> > >
> > > I'd expect the compiler to be smart enough to decide when inlining is
> > > appropriate. Did you check that adding the inline specifier actually
> > > makes a difference?
> >
> > This was done to make the code more readable. It does not make any
> > difference to performance.
>
> I meant dropping the inline specifier, not going back to manual
> inlining. As a general rule, you don't need to add the 'inline'
> specifier unless your function is defined in a header. In all other
> cases the compiler is able to inline things on its own when it sees the
> number of instructions is small enough or when the function is only
> called once.
Oh ok got it, will get rid if the inline specifier and send a v2
version of the change.
Thanks
Kamal