Hi Stefan,
A few more comments here.
On Sun, 27 May 2018 23:54:39 +0200, Stefan Agner <stefan@xxxxxxxx>
wrote:
> Add support for the NAND flash controller found on NVIDIA
> Tegra 2 SoCs. This implementation does not make use of the
> command queue feature. Regular operations/data transfers are
> done in PIO mode. Page read/writes with hardware ECC make
> use of the DMA for data transfer.
>
> Signed-off-by: Lucas Stach <dev@xxxxxxxxxx>
> Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
> ---
> MAINTAINERS | 7 +
> drivers/mtd/nand/raw/Kconfig | 6 +
> drivers/mtd/nand/raw/Makefile | 1 +
> drivers/mtd/nand/raw/tegra_nand.c | 999 ++++++++++++++++++++++++++++++
> 4 files changed, 1013 insertions(+)
> create mode 100644 drivers/mtd/nand/raw/tegra_nand.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58b9861ccf99..8cbbb7111742 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13844,6 +13844,13 @@ M: Laxman Dewangan <ldewangan@xxxxxxxxxx>
> S: Supported
> F: drivers/input/keyboard/tegra-kbc.c
>
> +TEGRA NAND DRIVER
> +M: Stefan Agner <stefan@xxxxxxxx>
> +M: Lucas Stach <dev@xxxxxxxxxx>
> +S: Maintained
> +F: Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
I think most MTD bindings use '-' instead of ','. I don't have a
preference, it's just for coherence.
> +F: drivers/mtd/nand/raw/tegra_nand.c
> +
> TEGRA PWM DRIVER
> M: Thierry Reding <thierry.reding@xxxxxxxxx>
> S: Supported
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 19a2b283fbbe..012c63c6ab47 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -534,4 +534,10 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>
> +config MTD_NAND_TEGRA
> + tristate "Support for NAND on NVIDIA Tegra"
> + depends on ARCH_TEGRA || COMPILE_TEST
> + help
> + Enables support for NAND flash on NVIDIA Tegra SoC based boards.
Please make the term "controller" appear because it's mostly a
controller driver that you're adding.
> +
> endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 165b7ef9e9a1..d5a5f9832b88 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
> obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
> obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
> obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o
> +obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o
>
> nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
> nand-objs += nand_amd.o
[...]
> +static int tegra_nand_setup_data_interface(struct mtd_info *mtd, int csline,
> + const struct nand_data_interface *conf)
> +{
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller);
> + const struct nand_sdr_timings *timings;
> +
> + timings = nand_get_sdr_timings(conf);
> + if (IS_ERR(timings))
> + return PTR_ERR(timings);
> +
> + if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> + return 0;
> +
> + tegra_nand_setup_timing(ctrl, timings);
Is this indirection really needed?
> +
> + return 0;
> +}
> +
> +static int tegra_nand_chips_init(struct device *dev,
> + struct tegra_nand_controller *ctrl)
> +{
> + struct device_node *np = dev->of_node;
> + struct device_node *np_nand;
> + int nchips = of_get_child_count(np);
> + struct tegra_nand_chip *nand;
> + struct mtd_info *mtd;
> + struct nand_chip *chip;
> + unsigned long config, bch_config = 0;
> + int bits_per_step;
> + int err;
> +
> + if (nchips != 1) {
> + dev_err(dev, "currently only one NAND chip supported\n");
> + return -EINVAL;
> + }
> +
> + np_nand = of_get_next_child(np, NULL);
> +
> + nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL);
> + if (!nand) {
> + dev_err(dev, "could not allocate chip structure\n");
> + return -ENOMEM;
> + }
> +
> + nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW);
> +
> + if (IS_ERR(nand->wp_gpio)) {
> + err = PTR_ERR(nand->wp_gpio);
> + dev_err(dev, "failed to request WP GPIO: %d\n", err);
> + return err;
> + }
> +
> + chip = &nand->chip;
> + chip->controller = &ctrl->controller;
> + ctrl->chip = chip;
> +
> + mtd = nand_to_mtd(chip);
> +
> + mtd->dev.parent = dev;
> + mtd->name = "tegra_nand";
> + mtd->owner = THIS_MODULE;
> +
> + nand_set_flash_node(chip, np_nand);
> +
> + chip->options = NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER;
> + chip->exec_op = tegra_nand_exec_op;
> + chip->select_chip = tegra_nand_select_chip;
> + chip->setup_data_interface = tegra_nand_setup_data_interface;
> +
> + err = nand_scan_ident(mtd, 1, NULL);
> + if (err)
> + return err;
> +
> + if (chip->bbt_options & NAND_BBT_USE_FLASH)
> + chip->bbt_options |= NAND_BBT_NO_OOB;
> +
> + chip->ecc.mode = NAND_ECC_HW;
> + if (!chip->ecc.size)
> + chip->ecc.size = 512;
> + if (chip->ecc.size != 512)
> + return -EINVAL;
This is useless. You can force ecc.size to 512 and forget about the DT
property (please update the DT bindings).
> +
> + chip->ecc.read_page = tegra_nand_read_page_hwecc;
> + chip->ecc.write_page = tegra_nand_write_page_hwecc;
> + /* Not functional for unknown reason...
> + chip->ecc.read_page_raw = tegra_nand_read_page;
> + chip->ecc.write_page_raw = tegra_nand_write_page;
Please add the _raw suffix to these helpers.
> + */
> + config = readl(ctrl->regs + CFG);
> + config |= CFG_PIPE_EN | CFG_SKIP_SPARE | CFG_SKIP_SPARE_SIZE_4;
> +
> + if (chip->options & NAND_BUSWIDTH_16)
> + config |= CFG_BUS_WIDTH_16;
> +
> + switch (chip->ecc.algo) {
> + case NAND_ECC_RS:
> + bits_per_step = BITS_PER_STEP_RS * chip->ecc.strength;
> + mtd_set_ooblayout(mtd, &tegra_nand_oob_rs_ops);
> + switch (chip->ecc.strength) {
> + case 4:
> + config |= CFG_ECC_SEL | CFG_TVAL_4;
> + break;
> + case 6:
> + config |= CFG_ECC_SEL | CFG_TVAL_6;
> + break;
> + case 8:
> + config |= CFG_ECC_SEL | CFG_TVAL_8;
> + break;
> + default:
> + dev_err(dev, "ECC strength %d not supported\n",
> + chip->ecc.strength);
> + return -EINVAL;
> + }
> + break;
> + case NAND_ECC_BCH:
> + bits_per_step = BITS_PER_STEP_BCH * chip->ecc.strength;
> + mtd_set_ooblayout(mtd, &tegra_nand_oob_bch_ops);
> + switch (chip->ecc.strength) {
> + case 4:
> + bch_config = BCH_TVAL_4;
> + break;
> + case 8:
> + bch_config = BCH_TVAL_8;
> + break;
> + case 14:
> + bch_config = BCH_TVAL_14;
> + break;
> + case 16:
> + bch_config = BCH_TVAL_16;
> + break;
> + default:
> + dev_err(dev, "ECC strength %d not supported\n",
> + chip->ecc.strength);
> + return -EINVAL;
> + }
> + break;
> + default:
> + dev_err(dev, "ECC algorithm not supported\n");
> + return -EINVAL;
> + }
> +
> + chip->ecc.bytes = DIV_ROUND_UP(bits_per_step, 8);
> +
> + switch (mtd->writesize) {
> + case 256:
> + config |= CFG_PS_256;
> + break;
> + case 512:
> + config |= CFG_PS_512;
> + break;
> + case 1024:
> + config |= CFG_PS_1024;
> + break;
> + case 2048:
> + config |= CFG_PS_2048;
> + break;
> + case 4096:
> + config |= CFG_PS_4096;
> + break;
> + default:
> + dev_err(dev, "unhandled writesize %d\n", mtd->writesize);
> + return -ENODEV;
> + }
> +
> + writel(config, ctrl->regs + CFG);
> + writel(bch_config, ctrl->regs + BCH_CONFIG);
> +
> + err = nand_scan_tail(mtd);
> + if (err)
> + return err;
> +
> + config |= CFG_TAG_BYTE_SIZE(mtd_ooblayout_count_freebytes(mtd) - 1);
> + writel(config, ctrl->regs + CFG);
> +
> + err = mtd_device_register(mtd, NULL, 0);
> + if (err)
Missing nand_cleanup() in the error path.
> + return err;
> +
> + return 0;
> +}
> +
[...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
