| Message ID | 20240508083637.3744003-1-quic_mdalam@quicinc.com |
|---|---|
| Headers | show |
| Series | Add QPIC SPI NAND driver | expand |
Hello, > --- a/drivers/mtd/nand/Kconfig > +++ b/drivers/mtd/nand/Kconfig > @@ -61,6 +61,14 @@ config MTD_NAND_ECC_MEDIATEK > help > This enables support for the hardware ECC engine from Mediatek. > > +config QPIC_COMMON > + tristate "QPIC common api file" > + depends on ARCH_QCOM || COMPILE_TEST > + help > + This enables support for common api for qpic nand controller. > + common apis will be used by both raw nand driver and serial nand > + driver. This should probably not be a standalone selectable item. Make it automatically selected by the users including the common header. > + > endmenu > > endmenu > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile > index 19e1291ac4d5..c0c1f8bd0220 100644 > --- a/drivers/mtd/nand/Makefile > +++ b/drivers/mtd/nand/Makefile > @@ -3,6 +3,7 @@ > nandcore-objs := core.o bbt.o > obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o > obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o > +obj-$(CONFIG_QPIC_COMMON) += qpic_common.o > > obj-y += onenand/ > obj-y += raw/ > diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c > new file mode 100644 > index 000000000000..d48892141365 > --- /dev/null > +++ b/drivers/mtd/nand/qpic_common.c > @@ -0,0 +1,741 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2016, The Linux Foundation. All rights reserved. > + */ > +#include <linux/bitops.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-mapping.h> > +#include <linux/dma/qcom_adm.h> > +#include <linux/dma/qcom_bam_dma.h> > +#include <linux/module.h> > +#include <linux/mtd/partitions.h> > +#include <linux/mtd/rawnand.h> No raw NAND include in the common file. If there is something raw NAND specific, it should not be here. > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/mtd/nand-qpic-common.h> Thanks, Miquèl
Hi, > --- /dev/null > +++ b/drivers/spi/spi-qpic-snand.c > @@ -0,0 +1,1423 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. > + * > + * Authors: > + * Md Sadre Alam <quic_mdalam@quicinc.com> > + * Sricharan R <quic_srichara@quicinc.com> > + * Varadarajan Narayanan <quic_varada@quicinc.com> > + */ > +#include <linux/bitops.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-mapping.h> > +#include <linux/dma/qcom_adm.h> > +#include <linux/dma/qcom_bam_dma.h> > +#include <linux/module.h> > +#include <linux/mtd/partitions.h> Not sure why partitions.h is needed here? > +#include <linux/mtd/rawnand.h> No raw NAND include. > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/mtd/nand-qpic-common.h> > +#include <linux/mtd/spinand.h> ... > +static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand) > +{ > + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); > + struct nand_ecc_props *conf = &nand->ecc.ctx.conf; > + struct nand_ecc_props *reqs = &nand->ecc.requirements; > + struct nand_ecc_props *user = &nand->ecc.user_conf; > + struct mtd_info *mtd = nanddev_to_mtd(nand); > + int step_size = 0, strength = 0, steps; > + int cwperpage, bad_block_byte; > + struct qpic_ecc *ecc_cfg; > + > + cwperpage = mtd->writesize / NANDC_STEP_SIZE; > + snandc->num_cw = cwperpage; > + > + ecc_cfg = kzalloc(sizeof(*ecc_cfg), GFP_KERNEL); > + if (!ecc_cfg) > + return -ENOMEM; > + > + nand->ecc.ctx.priv = ecc_cfg; > + > + if (user->step_size && user->strength) { > + step_size = user->step_size; > + strength = user->strength; > + } else if (reqs->step_size && reqs->strength) { > + step_size = reqs->step_size; > + strength = reqs->strength; > + } > + > + if (step_size && strength) > + steps = mtd->writesize / step_size; > + > + ecc_cfg->ecc_bytes_hw = 7; > + ecc_cfg->spare_bytes = 4; > + ecc_cfg->bbm_size = 1; > + ecc_cfg->bch_enabled = true; > + ecc_cfg->bytes = ecc_cfg->ecc_bytes_hw + ecc_cfg->spare_bytes + ecc_cfg->bbm_size; > + > + ecc_cfg->steps = 4; > + ecc_cfg->strength = 4; > + ecc_cfg->step_size = 512; > + > + mtd_set_ooblayout(mtd, &qcom_spi_ooblayout); > + > + ecc_cfg->cw_data = 516; > + ecc_cfg->cw_size = ecc_cfg->cw_data + ecc_cfg->bytes; > + bad_block_byte = mtd->writesize - ecc_cfg->cw_size * (cwperpage - 1) + 1; > + > + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE > + | ecc_cfg->cw_data << UD_SIZE_BYTES > + | 1 << DISABLE_STATUS_AFTER_WRITE > + | 3 << NUM_ADDR_CYCLES > + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS > + | 0 << STATUS_BFR_READ > + | 1 << SET_RD_MODE_AFTER_STATUS > + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; Can we please use FIELD_GET/FIELD_SET? Also I think the coding style proposes: val = foo | bar; instead of val = foo | bar; ... > +static int qcom_spi_write_page_cache(struct qcom_nand_controller *snandc, > + const struct spi_mem_op *op) > +{ > + struct qpic_snand_op s_op = {}; > + u32 cmd; > + > + cmd = qcom_spi_cmd_mapping(snandc, op->cmd.opcode); > + s_op.cmd_reg = cmd; > + > + if (op->cmd.opcode == SPINAND_PROGRAM_LOAD) { > + if (snandc->page_rw) > + snandc->data_buf = (u8 *)op->data.buf.out; > + if (snandc->oob_rw) > + snandc->oob_buf = (u8 *)op->data.buf.out; Quick comment: looks wrong, no? > + } > + > + return 0; > +} > + > +static int qcom_spi_send_cmdaddr(struct qcom_nand_controller *snandc, > + const struct spi_mem_op *op) > +{ > + struct qpic_snand_op s_op = {}; > + u32 cmd; > + int ret, opcode; > + > + cmd = qcom_spi_cmd_mapping(snandc, op->cmd.opcode); > + > + s_op.cmd_reg = cmd; > + s_op.addr1_reg = op->addr.val; > + s_op.addr2_reg = 0; > + > + opcode = op->cmd.opcode; > + > + switch (opcode) { > + case SPINAND_WRITE_EN: > + return 0; > + case SPINAND_PROGRAM_EXECUTE: > + s_op.addr1_reg = op->addr.val << 16; > + s_op.addr2_reg = op->addr.val >> 16 & 0xff; > + snandc->addr1 = s_op.addr1_reg; > + snandc->addr2 = s_op.addr2_reg; > + snandc->cmd = cmd; > + return qcom_spi_program_execute(snandc, op); > + case SPINAND_READ: > + s_op.addr1_reg = (op->addr.val << 16); > + s_op.addr2_reg = op->addr.val >> 16 & 0xff; > + snandc->addr1 = s_op.addr1_reg; > + snandc->addr2 = s_op.addr2_reg; > + snandc->cmd = cmd; > + return 0; > + case SPINAND_ERASE: > + s_op.addr2_reg = (op->addr.val >> 16) & 0xffff; > + s_op.addr1_reg = op->addr.val; > + snandc->addr1 = s_op.addr1_reg; > + snandc->addr1 <<= 16; Feels wrong as well > + snandc->addr2 = s_op.addr2_reg; > + snandc->cmd = cmd; > + qcom_spi_block_erase(snandc); > + return 0; > + default: > + break; > + } > + > + snandc->buf_count = 0; > + snandc->buf_start = 0; > + qcom_clear_read_regs(snandc); > + qcom_clear_bam_transaction(snandc); > + > + snandc->regs->cmd = s_op.cmd_reg; > + snandc->regs->exec = 1; > + snandc->regs->addr0 = s_op.addr1_reg; > + snandc->regs->addr1 = s_op.addr2_reg; > + > + qcom_write_reg_dma(snandc, &snandc->regs->cmd, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); > + qcom_write_reg_dma(snandc, &snandc->regs->exec, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); > + > + ret = qcom_submit_descs(snandc); > + if (ret) > + dev_err(snandc->dev, "failure in sbumitting cmd descriptor\n"); > + > + return ret; > +} > + > +static int qcom_spi_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op) > +{ > + int ret, val, opcode; > + bool copy = false, copy_ftr = false; > + > + ret = qcom_spi_send_cmdaddr(snandc, op); > + if (ret) > + return ret; > + > + snandc->buf_count = 0; > + snandc->buf_start = 0; > + qcom_clear_read_regs(snandc); > + qcom_clear_bam_transaction(snandc); > + opcode = op->cmd.opcode; > + > + switch (opcode) { > + case SPINAND_READID: > + snandc->buf_count = 4; > + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); > + copy = true; > + break; > + case SPINAND_GET_FEATURE: > + snandc->buf_count = 4; > + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); > + copy_ftr = true; > + break; > + case SPINAND_SET_FEATURE: > + snandc->regs->flash_feature = *(u32 *)op->data.buf.out; > + qcom_write_reg_dma(snandc, &snandc->regs->flash_feature, > + NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); > + break; > + default: > + return 0; No error state? > + } > + > + ret = qcom_submit_descs(snandc); > + if (ret) > + dev_err(snandc->dev, "failure in submitting descriptor for:%d\n", opcode); > + > + if (copy) { > + qcom_nandc_dev_to_mem(snandc, true); > + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count); > + } > + > + if (copy_ftr) { > + qcom_nandc_dev_to_mem(snandc, true); > + val = le32_to_cpu(*(__le32 *)snandc->reg_read_buf); > + val >>= 8; > + memcpy(op->data.buf.in, &val, snandc->buf_count); > + } > + > + return ret; > +} > + > +static bool qcom_spi_is_page_op(const struct spi_mem_op *op) > +{ > + if (op->addr.buswidth != 1 && op->addr.buswidth != 2 && op->addr.buswidth != 4) > + return false; > + > + if (op->data.dir == SPI_MEM_DATA_IN) { > + if (op->addr.buswidth == 4 && op->data.buswidth == 4) > + return true; > + > + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) > + return true; > + > + } else if (op->data.dir == SPI_MEM_DATA_OUT) { > + if (op->data.buswidth == 4) > + return true; > + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) > + return true; > + } > + > + return false; > +} > + > +static bool qcom_spi_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) > +{ > + if (!spi_mem_default_supports_op(mem, op)) > + return false; > + > + if (op->cmd.nbytes != 1 || op->cmd.buswidth != 1) > + return false; > + > + if (qcom_spi_is_page_op(op)) > + return true; > + > + return ((!op->addr.nbytes || op->addr.buswidth == 1) && > + (!op->dummy.nbytes || op->dummy.buswidth == 1) && > + (!op->data.nbytes || op->data.buswidth == 1)); > +} > + > +static int qcom_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > +{ > + struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->controller); > + > + dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode, > + op->addr.val, op->addr.buswidth, op->addr.nbytes, > + op->data.buswidth, op->data.nbytes); > + > + if (qcom_spi_is_page_op(op)) { > + if (op->data.dir == SPI_MEM_DATA_IN) > + return qcom_spi_read_page_cache(snandc, op); > + if (op->data.dir == SPI_MEM_DATA_OUT) > + return qcom_spi_write_page_cache(snandc, op); > + } else { > + return qcom_spi_io_op(snandc, op); > + } > + > + return 0; > +} > + > +static const struct spi_controller_mem_ops qcom_spi_mem_ops = { > + .supports_op = qcom_spi_supports_op, > + .exec_op = qcom_spi_exec_op, > +}; > + > +static const struct spi_controller_mem_caps qcom_spi_mem_caps = { > + .ecc = true, > +}; > + > +static int qcom_spi_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct spi_controller *ctlr; > + struct qcom_nand_controller *snandc; > + struct resource *res; > + const void *dev_data; > + struct qpic_ecc *ecc; > + int ret; > + > + ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL); > + if (!ecc) > + return -ENOMEM; > + > + ctlr = __devm_spi_alloc_controller(dev, sizeof(*snandc), false); > + if (!ctlr) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, ctlr); > + > + snandc = spi_controller_get_devdata(ctlr); > + > + snandc->ctlr = ctlr; > + snandc->dev = dev; > + snandc->ecc = ecc; > + > + snandc->oob_buf = devm_kzalloc(dev, OOB_BUF_SIZE, GFP_KERNEL); > + if (!snandc->oob_buf) > + return -ENOMEM; > + > + dev_data = of_device_get_match_data(dev); > + if (!dev_data) { > + dev_err(&pdev->dev, "failed to get device data\n"); > + return -ENODEV; > + } > + > + snandc->props = dev_data; > + snandc->dev = &pdev->dev; > + > + snandc->core_clk = devm_clk_get(dev, "core"); > + if (IS_ERR(snandc->core_clk)) > + return PTR_ERR(snandc->core_clk); > + > + snandc->aon_clk = devm_clk_get(dev, "aon"); > + if (IS_ERR(snandc->aon_clk)) > + return PTR_ERR(snandc->aon_clk); > + > + snandc->iomacro_clk = devm_clk_get(dev, "iom"); > + if (IS_ERR(snandc->iomacro_clk)) > + return PTR_ERR(snandc->iomacro_clk); > + > + snandc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > + if (IS_ERR(snandc->base)) > + return PTR_ERR(snandc->base); > + > + snandc->base_phys = res->start; > + snandc->base_dma = dma_map_resource(dev, res->start, resource_size(res), > + DMA_BIDIRECTIONAL, 0); > + if (dma_mapping_error(dev, snandc->base_dma)) > + return -ENXIO; > + > + ret = clk_prepare_enable(snandc->core_clk); > + if (ret) > + goto err_core_clk; error labels should point at the next freeing instruction, they should not be named against where they are called. Same below. > + > + ret = clk_prepare_enable(snandc->aon_clk); > + if (ret) > + goto err_aon_clk; > + > + ret = clk_prepare_enable(snandc->iomacro_clk); > + if (ret) > + goto err_snandc_alloc; This label is really badly named > + > + ret = qcom_nandc_alloc(snandc); > + if (ret) > + goto err_iom_clk; goto err_dis_iom_clk; ? > + > + ret = qcom_spi_init(snandc); > + if (ret) > + goto err_init; > + > + /* setup ECC engine */ > + snandc->ecc_eng.dev = &pdev->dev; > + snandc->ecc_eng.integration = NAND_ECC_ENGINE_INTEGRATION_PIPELINED; > + snandc->ecc_eng.ops = &qcom_spi_ecc_engine_ops_pipelined; > + snandc->ecc_eng.priv = snandc; > + > + ret = nand_ecc_register_on_host_hw_engine(&snandc->ecc_eng); > + if (ret) { > + dev_err(&pdev->dev, "failed to register ecc engine.\n"); Maybe you can print the error if we want the dev_err() call to be useful? > + goto err_init; > + } > + > + ctlr->num_chipselect = QPIC_QSPI_NUM_CS; > + ctlr->mem_ops = &qcom_spi_mem_ops; > + ctlr->mem_caps = &qcom_spi_mem_caps; > + ctlr->dev.of_node = pdev->dev.of_node; > + ctlr->mode_bits = SPI_TX_DUAL | SPI_RX_DUAL | > + SPI_TX_QUAD | SPI_RX_QUAD; > + > + ret = spi_register_controller(ctlr); > + if (ret) { > + dev_err(&pdev->dev, "spi_register_controller failed.\n"); > + goto err_init; > + } > + > + return 0; > + > +err_init: > + qcom_nandc_unalloc(snandc); > +err_iom_clk: > + clk_disable_unprepare(snandc->iomacro_clk); > +err_snandc_alloc: > + clk_disable_unprepare(snandc->aon_clk); > +err_aon_clk: > + clk_disable_unprepare(snandc->core_clk); > +err_core_clk: > + dma_unmap_resource(dev, res->start, resource_size(res), > + DMA_BIDIRECTIONAL, 0); > + return ret; > +} > + > +static int qcom_spi_remove(struct platform_device *pdev) > +{ > + struct spi_controller *ctlr = platform_get_drvdata(pdev); > + struct qcom_nand_controller *snandc = spi_controller_get_devdata(ctlr); > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + spi_unregister_controller(ctlr); > + > + clk_disable_unprepare(snandc->aon_clk); > + clk_disable_unprepare(snandc->core_clk); > + clk_disable_unprepare(snandc->iomacro_clk); > + > + dma_unmap_resource(&pdev->dev, snandc->base_dma, resource_size(res), > + DMA_BIDIRECTIONAL, 0); No unalloc? BTW unalloc is a bit weird. "free" or "cleanup" might be better. > + return 0; > +} > + > +static const struct qcom_nandc_props ipq9574_snandc_props = { > + .dev_cmd_reg_start = 0x7000, > + .supports_bam = true, > +}; > + > +static const struct of_device_id qcom_snandc_of_match[] = { > + { > + .compatible = "qcom,spi-qpic-snand", > + .data = &ipq9574_snandc_props, > + }, > + {} > +} > +MODULE_DEVICE_TABLE(of, qcom_snandc_of_match); > + > +static struct platform_driver qcom_spi_driver = { > + .driver = { > + .name = "qcom_snand", > + .of_match_table = qcom_snandc_of_match, > + }, > + .probe = qcom_spi_probe, > + .remove = qcom_spi_remove, > +}; > +module_platform_driver(qcom_spi_driver); > + > +MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores"); > +MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@quicinc.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h > index 096c15d3be76..dd2b56125bc1 100644 > --- a/include/linux/mtd/nand-qpic-common.h > +++ b/include/linux/mtd/nand-qpic-common.h > @@ -315,11 +315,56 @@ struct nandc_regs { > __le32 read_location_last1; > __le32 read_location_last2; > __le32 read_location_last3; > + __le32 spi_cfg; > + __le32 num_addr_cycle; > + __le32 busy_wait_cnt; > + __le32 flash_feature; > > __le32 erased_cw_detect_cfg_clr; > __le32 erased_cw_detect_cfg_set; > }; > > +/* > + * ECC state struct > + * @corrected: ECC corrected > + * @bitflips: Max bit flip > + * @failed: ECC failed > + */ > +struct qcom_ecc_stats { > + u32 corrected; > + u32 bitflips; > + u32 failed; > +}; > + > +struct qpic_ecc { > + struct device *dev; > + const struct qpic_ecc_caps *caps; > + struct completion done; > + u32 sectors; > + u8 *eccdata; > + bool use_ecc; > + u32 ecc_modes; > + int ecc_bytes_hw; > + int spare_bytes; > + int bbm_size; > + int ecc_mode; > + int bytes; > + int steps; > + int step_size; > + int strength; > + int cw_size; > + int cw_data; > + u32 cfg0, cfg1; > + u32 cfg0_raw, cfg1_raw; > + u32 ecc_buf_cfg; > + u32 ecc_bch_cfg; > + u32 clrflashstatus; > + u32 clrreadstatus; > + bool bch_enabled; > +}; > + > +struct qpic_ecc; > + > /* > * NAND controller data struct > * > @@ -329,6 +374,7 @@ struct nandc_regs { > * > * @core_clk: controller clock > * @aon_clk: another controller clock > + * @iomacro_clk: io macro clock > * > * @regs: a contiguous chunk of memory for DMA register > * writes. contains the register values to be > @@ -338,6 +384,7 @@ struct nandc_regs { > * initialized via DT match data > * > * @controller: base controller structure > + * @ctlr: spi controller structure > * @host_list: list containing all the chips attached to the > * controller > * > @@ -375,6 +422,7 @@ struct qcom_nand_controller { > > struct clk *core_clk; > struct clk *aon_clk; > + struct clk *iomacro_clk; > > struct nandc_regs *regs; > struct bam_transaction *bam_txn; > @@ -382,6 +430,7 @@ struct qcom_nand_controller { > const struct qcom_nandc_props *props; > > struct nand_controller controller; > + struct spi_controller *ctlr; > struct list_head host_list; > > union { > @@ -418,6 +467,21 @@ struct qcom_nand_controller { > > u32 cmd1, vld; > bool exec_opwrite; > + struct qpic_ecc *ecc; > + struct qcom_ecc_stats ecc_stats; > + struct nand_ecc_engine ecc_eng; > + u8 *data_buf; > + u8 *oob_buf; > + u32 wlen; > + u32 addr1; > + u32 addr2; > + u32 cmd; > + u32 num_cw; > + u32 pagesize; > + bool oob_rw; > + bool page_rw; > + bool raw_rw; > + bool read_last_cw; > }; If all these definitions are only used by the spi controller, I don't see why you want to put them in the common file. Thanks, Miquèl
Hi,
quic_mdalam@quicinc.com wrote on Wed, 8 May 2024 14:06:37 +0530:
> Disable eMMC node
Please explain why or remove this patch from this series, because at a
first glance there is no reason to link this patch with the others.
Thanks,
Miquèl
On 5/16/2024 6:07 PM, Miquel Raynal wrote: > Hello, > >> --- a/drivers/mtd/nand/Kconfig >> +++ b/drivers/mtd/nand/Kconfig >> @@ -61,6 +61,14 @@ config MTD_NAND_ECC_MEDIATEK >> help >> This enables support for the hardware ECC engine from Mediatek. >> >> +config QPIC_COMMON >> + tristate "QPIC common api file" >> + depends on ARCH_QCOM || COMPILE_TEST >> + help >> + This enables support for common api for qpic nand controller. >> + common apis will be used by both raw nand driver and serial nand >> + driver. > > This should probably not be a standalone selectable item. Make it > automatically selected by the users including the common header. Ok. > >> + >> endmenu >> >> endmenu >> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile >> index 19e1291ac4d5..c0c1f8bd0220 100644 >> --- a/drivers/mtd/nand/Makefile >> +++ b/drivers/mtd/nand/Makefile >> @@ -3,6 +3,7 @@ >> nandcore-objs := core.o bbt.o >> obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o >> obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o >> +obj-$(CONFIG_QPIC_COMMON) += qpic_common.o >> >> obj-y += onenand/ >> obj-y += raw/ >> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c >> new file mode 100644 >> index 000000000000..d48892141365 >> --- /dev/null >> +++ b/drivers/mtd/nand/qpic_common.c >> @@ -0,0 +1,741 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2016, The Linux Foundation. All rights reserved. >> + */ >> +#include <linux/bitops.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/dmaengine.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/dma/qcom_adm.h> >> +#include <linux/dma/qcom_bam_dma.h> >> +#include <linux/module.h> >> +#include <linux/mtd/partitions.h> >> +#include <linux/mtd/rawnand.h> > > No raw NAND include in the common file. If there is something raw NAND > specific, it should not be here. Ok , will fix in next patch > >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/mtd/nand-qpic-common.h> > > Thanks, > Miquèl
On 5/16/2024 6:26 PM, Miquel Raynal wrote: > Hi, > >> --- /dev/null >> +++ b/drivers/spi/spi-qpic-snand.c >> @@ -0,0 +1,1423 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0 >> + * >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. >> + * >> + * Authors: >> + * Md Sadre Alam <quic_mdalam@quicinc.com> >> + * Sricharan R <quic_srichara@quicinc.com> >> + * Varadarajan Narayanan <quic_varada@quicinc.com> >> + */ >> +#include <linux/bitops.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/dmaengine.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/dma/qcom_adm.h> >> +#include <linux/dma/qcom_bam_dma.h> >> +#include <linux/module.h> >> +#include <linux/mtd/partitions.h> > > Not sure why partitions.h is needed here? Will remove in next patch > >> +#include <linux/mtd/rawnand.h> > > No raw NAND include. Ok > >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/mtd/nand-qpic-common.h> >> +#include <linux/mtd/spinand.h> > > ... > >> +static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand) >> +{ >> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); >> + struct nand_ecc_props *conf = &nand->ecc.ctx.conf; >> + struct nand_ecc_props *reqs = &nand->ecc.requirements; >> + struct nand_ecc_props *user = &nand->ecc.user_conf; >> + struct mtd_info *mtd = nanddev_to_mtd(nand); >> + int step_size = 0, strength = 0, steps; >> + int cwperpage, bad_block_byte; >> + struct qpic_ecc *ecc_cfg; >> + >> + cwperpage = mtd->writesize / NANDC_STEP_SIZE; >> + snandc->num_cw = cwperpage; >> + >> + ecc_cfg = kzalloc(sizeof(*ecc_cfg), GFP_KERNEL); >> + if (!ecc_cfg) >> + return -ENOMEM; >> + >> + nand->ecc.ctx.priv = ecc_cfg; >> + >> + if (user->step_size && user->strength) { >> + step_size = user->step_size; >> + strength = user->strength; >> + } else if (reqs->step_size && reqs->strength) { >> + step_size = reqs->step_size; >> + strength = reqs->strength; >> + } >> + >> + if (step_size && strength) >> + steps = mtd->writesize / step_size; >> + >> + ecc_cfg->ecc_bytes_hw = 7; >> + ecc_cfg->spare_bytes = 4; >> + ecc_cfg->bbm_size = 1; >> + ecc_cfg->bch_enabled = true; >> + ecc_cfg->bytes = ecc_cfg->ecc_bytes_hw + ecc_cfg->spare_bytes + ecc_cfg->bbm_size; >> + >> + ecc_cfg->steps = 4; >> + ecc_cfg->strength = 4; >> + ecc_cfg->step_size = 512; >> + >> + mtd_set_ooblayout(mtd, &qcom_spi_ooblayout); >> + >> + ecc_cfg->cw_data = 516; >> + ecc_cfg->cw_size = ecc_cfg->cw_data + ecc_cfg->bytes; >> + bad_block_byte = mtd->writesize - ecc_cfg->cw_size * (cwperpage - 1) + 1; >> + >> + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE >> + | ecc_cfg->cw_data << UD_SIZE_BYTES >> + | 1 << DISABLE_STATUS_AFTER_WRITE >> + | 3 << NUM_ADDR_CYCLES >> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS >> + | 0 << STATUS_BFR_READ >> + | 1 << SET_RD_MODE_AFTER_STATUS >> + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; > > Can we please use FIELD_GET/FIELD_SET? > > Also I think the coding style proposes: > > val = foo | > bar; > > instead of > > val = foo > | bar; > Ok, Will fix in next patch. > > ... > >> +static int qcom_spi_write_page_cache(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_snand_op s_op = {}; >> + u32 cmd; >> + >> + cmd = qcom_spi_cmd_mapping(snandc, op->cmd.opcode); >> + s_op.cmd_reg = cmd; >> + >> + if (op->cmd.opcode == SPINAND_PROGRAM_LOAD) { >> + if (snandc->page_rw) >> + snandc->data_buf = (u8 *)op->data.buf.out; >> + if (snandc->oob_rw) >> + snandc->oob_buf = (u8 *)op->data.buf.out; > > Quick comment: looks wrong, no? Yes , oob_buf not needed here. will fix in next patch. > >> + } >> + >> + return 0; >> +} >> + >> +static int qcom_spi_send_cmdaddr(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_snand_op s_op = {}; >> + u32 cmd; >> + int ret, opcode; >> + >> + cmd = qcom_spi_cmd_mapping(snandc, op->cmd.opcode); >> + >> + s_op.cmd_reg = cmd; >> + s_op.addr1_reg = op->addr.val; >> + s_op.addr2_reg = 0; >> + >> + opcode = op->cmd.opcode; >> + >> + switch (opcode) { >> + case SPINAND_WRITE_EN: >> + return 0; >> + case SPINAND_PROGRAM_EXECUTE: >> + s_op.addr1_reg = op->addr.val << 16; >> + s_op.addr2_reg = op->addr.val >> 16 & 0xff; >> + snandc->addr1 = s_op.addr1_reg; >> + snandc->addr2 = s_op.addr2_reg; >> + snandc->cmd = cmd; >> + return qcom_spi_program_execute(snandc, op); >> + case SPINAND_READ: >> + s_op.addr1_reg = (op->addr.val << 16); >> + s_op.addr2_reg = op->addr.val >> 16 & 0xff; >> + snandc->addr1 = s_op.addr1_reg; >> + snandc->addr2 = s_op.addr2_reg; >> + snandc->cmd = cmd; >> + return 0; >> + case SPINAND_ERASE: >> + s_op.addr2_reg = (op->addr.val >> 16) & 0xffff; >> + s_op.addr1_reg = op->addr.val; >> + snandc->addr1 = s_op.addr1_reg; >> + snandc->addr1 <<= 16; > > Feels wrong as well Ok , will fix in next patch. > >> + snandc->addr2 = s_op.addr2_reg; >> + snandc->cmd = cmd; >> + qcom_spi_block_erase(snandc); >> + return 0; >> + default: >> + break; >> + } >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + qcom_clear_bam_transaction(snandc); >> + >> + snandc->regs->cmd = s_op.cmd_reg; >> + snandc->regs->exec = 1; >> + snandc->regs->addr0 = s_op.addr1_reg; >> + snandc->regs->addr1 = s_op.addr2_reg; >> + >> + qcom_write_reg_dma(snandc, &snandc->regs->cmd, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, &snandc->regs->exec, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failure in sbumitting cmd descriptor\n"); >> + >> + return ret; >> +} >> + >> +static int qcom_spi_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op) >> +{ >> + int ret, val, opcode; >> + bool copy = false, copy_ftr = false; >> + >> + ret = qcom_spi_send_cmdaddr(snandc, op); >> + if (ret) >> + return ret; >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + qcom_clear_bam_transaction(snandc); >> + opcode = op->cmd.opcode; >> + >> + switch (opcode) { >> + case SPINAND_READID: >> + snandc->buf_count = 4; >> + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); >> + copy = true; >> + break; >> + case SPINAND_GET_FEATURE: >> + snandc->buf_count = 4; >> + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); >> + copy_ftr = true; >> + break; >> + case SPINAND_SET_FEATURE: >> + snandc->regs->flash_feature = *(u32 *)op->data.buf.out; >> + qcom_write_reg_dma(snandc, &snandc->regs->flash_feature, >> + NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); >> + break; >> + default: >> + return 0; > > No error state? We can't return return error here , since this API is not for checking supported command. We can return error only if we submitted the descriptor. That already we are handling. > >> + } >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failure in submitting descriptor for:%d\n", opcode); >> + >> + if (copy) { >> + qcom_nandc_dev_to_mem(snandc, true); >> + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count); >> + } >> + >> + if (copy_ftr) { >> + qcom_nandc_dev_to_mem(snandc, true); >> + val = le32_to_cpu(*(__le32 *)snandc->reg_read_buf); >> + val >>= 8; >> + memcpy(op->data.buf.in, &val, snandc->buf_count); >> + } >> + >> + return ret; >> +} >> + >> +static bool qcom_spi_is_page_op(const struct spi_mem_op *op) >> +{ >> + if (op->addr.buswidth != 1 && op->addr.buswidth != 2 && op->addr.buswidth != 4) >> + return false; >> + >> + if (op->data.dir == SPI_MEM_DATA_IN) { >> + if (op->addr.buswidth == 4 && op->data.buswidth == 4) >> + return true; >> + >> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) >> + return true; >> + >> + } else if (op->data.dir == SPI_MEM_DATA_OUT) { >> + if (op->data.buswidth == 4) >> + return true; >> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +static bool qcom_spi_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) >> +{ >> + if (!spi_mem_default_supports_op(mem, op)) >> + return false; >> + >> + if (op->cmd.nbytes != 1 || op->cmd.buswidth != 1) >> + return false; >> + >> + if (qcom_spi_is_page_op(op)) >> + return true; >> + >> + return ((!op->addr.nbytes || op->addr.buswidth == 1) && >> + (!op->dummy.nbytes || op->dummy.buswidth == 1) && >> + (!op->data.nbytes || op->data.buswidth == 1)); >> +} >> + >> +static int qcom_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) >> +{ >> + struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->controller); >> + >> + dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode, >> + op->addr.val, op->addr.buswidth, op->addr.nbytes, >> + op->data.buswidth, op->data.nbytes); >> + >> + if (qcom_spi_is_page_op(op)) { >> + if (op->data.dir == SPI_MEM_DATA_IN) >> + return qcom_spi_read_page_cache(snandc, op); >> + if (op->data.dir == SPI_MEM_DATA_OUT) >> + return qcom_spi_write_page_cache(snandc, op); >> + } else { >> + return qcom_spi_io_op(snandc, op); >> + } >> + >> + return 0; >> +} >> + >> +static const struct spi_controller_mem_ops qcom_spi_mem_ops = { >> + .supports_op = qcom_spi_supports_op, >> + .exec_op = qcom_spi_exec_op, >> +}; >> + >> +static const struct spi_controller_mem_caps qcom_spi_mem_caps = { >> + .ecc = true, >> +}; >> + >> +static int qcom_spi_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct spi_controller *ctlr; >> + struct qcom_nand_controller *snandc; >> + struct resource *res; >> + const void *dev_data; >> + struct qpic_ecc *ecc; >> + int ret; >> + >> + ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL); >> + if (!ecc) >> + return -ENOMEM; >> + >> + ctlr = __devm_spi_alloc_controller(dev, sizeof(*snandc), false); >> + if (!ctlr) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, ctlr); >> + >> + snandc = spi_controller_get_devdata(ctlr); >> + >> + snandc->ctlr = ctlr; >> + snandc->dev = dev; >> + snandc->ecc = ecc; >> + >> + snandc->oob_buf = devm_kzalloc(dev, OOB_BUF_SIZE, GFP_KERNEL); >> + if (!snandc->oob_buf) >> + return -ENOMEM; >> + >> + dev_data = of_device_get_match_data(dev); >> + if (!dev_data) { >> + dev_err(&pdev->dev, "failed to get device data\n"); >> + return -ENODEV; >> + } >> + >> + snandc->props = dev_data; >> + snandc->dev = &pdev->dev; >> + >> + snandc->core_clk = devm_clk_get(dev, "core"); >> + if (IS_ERR(snandc->core_clk)) >> + return PTR_ERR(snandc->core_clk); >> + >> + snandc->aon_clk = devm_clk_get(dev, "aon"); >> + if (IS_ERR(snandc->aon_clk)) >> + return PTR_ERR(snandc->aon_clk); >> + >> + snandc->iomacro_clk = devm_clk_get(dev, "iom"); >> + if (IS_ERR(snandc->iomacro_clk)) >> + return PTR_ERR(snandc->iomacro_clk); >> + >> + snandc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >> + if (IS_ERR(snandc->base)) >> + return PTR_ERR(snandc->base); >> + >> + snandc->base_phys = res->start; >> + snandc->base_dma = dma_map_resource(dev, res->start, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); >> + if (dma_mapping_error(dev, snandc->base_dma)) >> + return -ENXIO; >> + >> + ret = clk_prepare_enable(snandc->core_clk); >> + if (ret) >> + goto err_core_clk; > > error labels should point at the next freeing instruction, they should > not be named against where they are called. Ok > > Same below. Ok > >> + >> + ret = clk_prepare_enable(snandc->aon_clk); >> + if (ret) >> + goto err_aon_clk; >> + >> + ret = clk_prepare_enable(snandc->iomacro_clk); >> + if (ret) >> + goto err_snandc_alloc; > > This label is really badly named Ok > >> + >> + ret = qcom_nandc_alloc(snandc); >> + if (ret) >> + goto err_iom_clk; > > goto err_dis_iom_clk; ? OK > >> + >> + ret = qcom_spi_init(snandc); >> + if (ret) >> + goto err_init; >> + >> + /* setup ECC engine */ >> + snandc->ecc_eng.dev = &pdev->dev; >> + snandc->ecc_eng.integration = NAND_ECC_ENGINE_INTEGRATION_PIPELINED; >> + snandc->ecc_eng.ops = &qcom_spi_ecc_engine_ops_pipelined; >> + snandc->ecc_eng.priv = snandc; >> + >> + ret = nand_ecc_register_on_host_hw_engine(&snandc->ecc_eng); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register ecc engine.\n"); > > Maybe you can print the error if we want the dev_err() call to be > useful? Ok > >> + goto err_init; >> + } >> + >> + ctlr->num_chipselect = QPIC_QSPI_NUM_CS; >> + ctlr->mem_ops = &qcom_spi_mem_ops; >> + ctlr->mem_caps = &qcom_spi_mem_caps; >> + ctlr->dev.of_node = pdev->dev.of_node; >> + ctlr->mode_bits = SPI_TX_DUAL | SPI_RX_DUAL | >> + SPI_TX_QUAD | SPI_RX_QUAD; >> + >> + ret = spi_register_controller(ctlr); >> + if (ret) { >> + dev_err(&pdev->dev, "spi_register_controller failed.\n"); >> + goto err_init; >> + } >> + >> + return 0; >> + >> +err_init: >> + qcom_nandc_unalloc(snandc); >> +err_iom_clk: >> + clk_disable_unprepare(snandc->iomacro_clk); >> +err_snandc_alloc: >> + clk_disable_unprepare(snandc->aon_clk); >> +err_aon_clk: >> + clk_disable_unprepare(snandc->core_clk); >> +err_core_clk: >> + dma_unmap_resource(dev, res->start, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); >> + return ret; >> +} >> + >> +static int qcom_spi_remove(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctlr = platform_get_drvdata(pdev); >> + struct qcom_nand_controller *snandc = spi_controller_get_devdata(ctlr); >> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + >> + spi_unregister_controller(ctlr); >> + >> + clk_disable_unprepare(snandc->aon_clk); >> + clk_disable_unprepare(snandc->core_clk); >> + clk_disable_unprepare(snandc->iomacro_clk); >> + >> + dma_unmap_resource(&pdev->dev, snandc->base_dma, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); > > No unalloc? > > BTW unalloc is a bit weird. "free" or "cleanup" might be better. We are having a existing api called qcom_nandc_unalloc() , will use the same in next patch. > >> + return 0; >> +} >> + >> +static const struct qcom_nandc_props ipq9574_snandc_props = { >> + .dev_cmd_reg_start = 0x7000, >> + .supports_bam = true, >> +}; >> + >> +static const struct of_device_id qcom_snandc_of_match[] = { >> + { >> + .compatible = "qcom,spi-qpic-snand", >> + .data = &ipq9574_snandc_props, >> + }, >> + {} >> +} >> +MODULE_DEVICE_TABLE(of, qcom_snandc_of_match); >> + >> +static struct platform_driver qcom_spi_driver = { >> + .driver = { >> + .name = "qcom_snand", >> + .of_match_table = qcom_snandc_of_match, >> + }, >> + .probe = qcom_spi_probe, >> + .remove = qcom_spi_remove, >> +}; >> +module_platform_driver(qcom_spi_driver); >> + >> +MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores"); >> +MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@quicinc.com>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h >> index 096c15d3be76..dd2b56125bc1 100644 >> --- a/include/linux/mtd/nand-qpic-common.h >> +++ b/include/linux/mtd/nand-qpic-common.h >> @@ -315,11 +315,56 @@ struct nandc_regs { >> __le32 read_location_last1; >> __le32 read_location_last2; >> __le32 read_location_last3; >> + __le32 spi_cfg; >> + __le32 num_addr_cycle; >> + __le32 busy_wait_cnt; >> + __le32 flash_feature; >> >> __le32 erased_cw_detect_cfg_clr; >> __le32 erased_cw_detect_cfg_set; >> }; >> >> +/* >> + * ECC state struct >> + * @corrected: ECC corrected >> + * @bitflips: Max bit flip >> + * @failed: ECC failed >> + */ >> +struct qcom_ecc_stats { >> + u32 corrected; >> + u32 bitflips; >> + u32 failed; >> +}; >> + >> +struct qpic_ecc { >> + struct device *dev; >> + const struct qpic_ecc_caps *caps; >> + struct completion done; >> + u32 sectors; >> + u8 *eccdata; >> + bool use_ecc; >> + u32 ecc_modes; >> + int ecc_bytes_hw; >> + int spare_bytes; >> + int bbm_size; >> + int ecc_mode; >> + int bytes; >> + int steps; >> + int step_size; >> + int strength; >> + int cw_size; >> + int cw_data; >> + u32 cfg0, cfg1; >> + u32 cfg0_raw, cfg1_raw; >> + u32 ecc_buf_cfg; >> + u32 ecc_bch_cfg; >> + u32 clrflashstatus; >> + u32 clrreadstatus; >> + bool bch_enabled; >> +}; >> + >> +struct qpic_ecc; >> + >> /* >> * NAND controller data struct >> * >> @@ -329,6 +374,7 @@ struct nandc_regs { >> * >> * @core_clk: controller clock >> * @aon_clk: another controller clock >> + * @iomacro_clk: io macro clock >> * >> * @regs: a contiguous chunk of memory for DMA register >> * writes. contains the register values to be >> @@ -338,6 +384,7 @@ struct nandc_regs { >> * initialized via DT match data >> * >> * @controller: base controller structure >> + * @ctlr: spi controller structure >> * @host_list: list containing all the chips attached to the >> * controller >> * >> @@ -375,6 +422,7 @@ struct qcom_nand_controller { >> >> struct clk *core_clk; >> struct clk *aon_clk; >> + struct clk *iomacro_clk; >> >> struct nandc_regs *regs; >> struct bam_transaction *bam_txn; >> @@ -382,6 +430,7 @@ struct qcom_nand_controller { >> const struct qcom_nandc_props *props; >> >> struct nand_controller controller; >> + struct spi_controller *ctlr; >> struct list_head host_list; >> >> union { >> @@ -418,6 +467,21 @@ struct qcom_nand_controller { >> >> u32 cmd1, vld; >> bool exec_opwrite; >> + struct qpic_ecc *ecc; >> + struct qcom_ecc_stats ecc_stats; >> + struct nand_ecc_engine ecc_eng; >> + u8 *data_buf; >> + u8 *oob_buf; >> + u32 wlen; >> + u32 addr1; >> + u32 addr2; >> + u32 cmd; >> + u32 num_cw; >> + u32 pagesize; >> + bool oob_rw; >> + bool page_rw; >> + bool raw_rw; >> + bool read_last_cw; >> }; > > If all these definitions are only used by the spi controller, I don't > see why you want to put them in the common file. We are using qcom_nand_controller{..} structure as common b/w raw nand and spi nand. These all variables will be used by spi nand only , but qcom_nand_controller structure is passed across all the SPI API, thats why define these all variables inside qcom_nand_controller structure. so that i can access directlty. > > Thanks, > Miquèl
On 5/16/2024 6:29 PM, Miquel Raynal wrote: > Hi, > > quic_mdalam@quicinc.com wrote on Wed, 8 May 2024 14:06:37 +0530: > >> Disable eMMC node > > Please explain why or remove this patch from this series, because at a > first glance there is no reason to link this patch with the others. rdp433 default boot mode is norplusnand and GPIOs are shared b/w eMMC and NAND so we can't keep both.So we are disabling the eMMC for rdp433. > > Thanks, > Miquèl
Hi, > >> +static int qcom_spi_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op) > >> +{ > >> + int ret, val, opcode; > >> + bool copy = false, copy_ftr = false; > >> + > >> + ret = qcom_spi_send_cmdaddr(snandc, op); > >> + if (ret) > >> + return ret; > >> + > >> + snandc->buf_count = 0; > >> + snandc->buf_start = 0; > >> + qcom_clear_read_regs(snandc); > >> + qcom_clear_bam_transaction(snandc); > >> + opcode = op->cmd.opcode; > >> + > >> + switch (opcode) { > >> + case SPINAND_READID: > >> + snandc->buf_count = 4; > >> + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); > >> + copy = true; > >> + break; > >> + case SPINAND_GET_FEATURE: > >> + snandc->buf_count = 4; > >> + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); > >> + copy_ftr = true; > >> + break; > >> + case SPINAND_SET_FEATURE: > >> + snandc->regs->flash_feature = *(u32 *)op->data.buf.out; > >> + qcom_write_reg_dma(snandc, &snandc->regs->flash_feature, > >> + NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); > >> + break; > >> + default: > >> + return 0; > > > > No error state? > We can't return return error here , since this API is not for checking supported command. I no longer remember exactly where this is called, but if there are possible unhandled cases, I want an error to be returned. > We can return error only if we submitted the descriptor. That already we are handling. ... > >> --- a/include/linux/mtd/nand-qpic-common.h > >> +++ b/include/linux/mtd/nand-qpic-common.h > >> @@ -315,11 +315,56 @@ struct nandc_regs { > >> __le32 read_location_last1; > >> __le32 read_location_last2; > >> __le32 read_location_last3; > >> + __le32 spi_cfg; > >> + __le32 num_addr_cycle; > >> + __le32 busy_wait_cnt; > >> + __le32 flash_feature; > >> >> __le32 erased_cw_detect_cfg_clr; > >> __le32 erased_cw_detect_cfg_set; > >> }; > >> >> +/* > >> + * ECC state struct > >> + * @corrected: ECC corrected > >> + * @bitflips: Max bit flip > >> + * @failed: ECC failed > >> + */ > >> +struct qcom_ecc_stats { > >> + u32 corrected; > >> + u32 bitflips; > >> + u32 failed; > >> +}; > >> + > >> +struct qpic_ecc { > >> + struct device *dev; > >> + const struct qpic_ecc_caps *caps; > >> + struct completion done; > >> + u32 sectors; > >> + u8 *eccdata; > >> + bool use_ecc; > >> + u32 ecc_modes; > >> + int ecc_bytes_hw; > >> + int spare_bytes; > >> + int bbm_size; > >> + int ecc_mode; > >> + int bytes; > >> + int steps; > >> + int step_size; > >> + int strength; > >> + int cw_size; > >> + int cw_data; > >> + u32 cfg0, cfg1; > >> + u32 cfg0_raw, cfg1_raw; > >> + u32 ecc_buf_cfg; > >> + u32 ecc_bch_cfg; > >> + u32 clrflashstatus; > >> + u32 clrreadstatus; > >> + bool bch_enabled; > >> +}; > >> + > >> +struct qpic_ecc; > >> + > >> /* > >> * NAND controller data struct > >> * > >> @@ -329,6 +374,7 @@ struct nandc_regs { > >> * > >> * @core_clk: controller clock > >> * @aon_clk: another controller clock > >> + * @iomacro_clk: io macro clock > >> * > >> * @regs: a contiguous chunk of memory for DMA register > >> * writes. contains the register values to be > >> @@ -338,6 +384,7 @@ struct nandc_regs { > >> * initialized via DT match data > >> * > >> * @controller: base controller structure > >> + * @ctlr: spi controller structure > >> * @host_list: list containing all the chips attached to the > >> * controller > >> * > >> @@ -375,6 +422,7 @@ struct qcom_nand_controller { > >> >> struct clk *core_clk; > >> struct clk *aon_clk; > >> + struct clk *iomacro_clk; > >> >> struct nandc_regs *regs; > >> struct bam_transaction *bam_txn; > >> @@ -382,6 +430,7 @@ struct qcom_nand_controller { > >> const struct qcom_nandc_props *props; > >> >> struct nand_controller controller; > >> + struct spi_controller *ctlr; > >> struct list_head host_list; > >> >> union { > >> @@ -418,6 +467,21 @@ struct qcom_nand_controller { > >> >> u32 cmd1, vld; > >> bool exec_opwrite; > >> + struct qpic_ecc *ecc; > >> + struct qcom_ecc_stats ecc_stats; > >> + struct nand_ecc_engine ecc_eng; > >> + u8 *data_buf; > >> + u8 *oob_buf; > >> + u32 wlen; > >> + u32 addr1; > >> + u32 addr2; > >> + u32 cmd; > >> + u32 num_cw; > >> + u32 pagesize; > >> + bool oob_rw; > >> + bool page_rw; > >> + bool raw_rw; > >> + bool read_last_cw; > >> }; > > > > If all these definitions are only used by the spi controller, I don't > > see why you want to put them in the common file. > We are using qcom_nand_controller{..} structure as common b/w raw nand > and spi nand. These all variables will be used by spi nand only , but > qcom_nand_controller structure is passed across all the SPI API, thats > why define these all variables inside qcom_nand_controller structure. > so that i can access directlty. Maybe you can move the spi-nand specific variables in a struct, and the raw NAND specific variables in another, and then use an enum in this structure. This way only the useful fields are available. Or maybe you can have two pointers and only populate the relevant one from the relevant driver with the fields that are missing. But this is a generic include, so don't put specific fields there just because it is convenient. Thanks, Miquèl
On 5/20/2024 6:13 PM, Miquel Raynal wrote: > Hi, > >>>> +static int qcom_spi_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op) >>>> +{ >>>> + int ret, val, opcode; >>>> + bool copy = false, copy_ftr = false; >>>> + >>>> + ret = qcom_spi_send_cmdaddr(snandc, op); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + snandc->buf_count = 0; >>>> + snandc->buf_start = 0; >>>> + qcom_clear_read_regs(snandc); >>>> + qcom_clear_bam_transaction(snandc); >>>> + opcode = op->cmd.opcode; >>>> + >>>> + switch (opcode) { >>>> + case SPINAND_READID: >>>> + snandc->buf_count = 4; >>>> + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); >>>> + copy = true; >>>> + break; >>>> + case SPINAND_GET_FEATURE: >>>> + snandc->buf_count = 4; >>>> + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); >>>> + copy_ftr = true; >>>> + break; >>>> + case SPINAND_SET_FEATURE: >>>> + snandc->regs->flash_feature = *(u32 *)op->data.buf.out; >>>> + qcom_write_reg_dma(snandc, &snandc->regs->flash_feature, >>>> + NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); >>>> + break; >>>> + default: >>>> + return 0; >>> >>> No error state? >> We can't return return error here , since this API is not for checking supported command. > > I no longer remember exactly where this is called, but if there are > possible unhandled cases, I want an error to be returned. Ok > >> We can return error only if we submitted the descriptor. That already we are handling. > > ... > >>>> --- a/include/linux/mtd/nand-qpic-common.h >>>> +++ b/include/linux/mtd/nand-qpic-common.h >>>> @@ -315,11 +315,56 @@ struct nandc_regs { >>>> __le32 read_location_last1; >>>> __le32 read_location_last2; >>>> __le32 read_location_last3; >>>> + __le32 spi_cfg; >>>> + __le32 num_addr_cycle; >>>> + __le32 busy_wait_cnt; >>>> + __le32 flash_feature; >>>> >> __le32 erased_cw_detect_cfg_clr; >>>> __le32 erased_cw_detect_cfg_set; >>>> }; >>>> >> +/* >>>> + * ECC state struct >>>> + * @corrected: ECC corrected >>>> + * @bitflips: Max bit flip >>>> + * @failed: ECC failed >>>> + */ >>>> +struct qcom_ecc_stats { >>>> + u32 corrected; >>>> + u32 bitflips; >>>> + u32 failed; >>>> +}; >>>> + >>>> +struct qpic_ecc { >>>> + struct device *dev; >>>> + const struct qpic_ecc_caps *caps; >>>> + struct completion done; >>>> + u32 sectors; >>>> + u8 *eccdata; >>>> + bool use_ecc; >>>> + u32 ecc_modes; >>>> + int ecc_bytes_hw; >>>> + int spare_bytes; >>>> + int bbm_size; >>>> + int ecc_mode; >>>> + int bytes; >>>> + int steps; >>>> + int step_size; >>>> + int strength; >>>> + int cw_size; >>>> + int cw_data; >>>> + u32 cfg0, cfg1; >>>> + u32 cfg0_raw, cfg1_raw; >>>> + u32 ecc_buf_cfg; >>>> + u32 ecc_bch_cfg; >>>> + u32 clrflashstatus; >>>> + u32 clrreadstatus; >>>> + bool bch_enabled; >>>> +}; >>>> + >>>> +struct qpic_ecc; >>>> + >>>> /* >>>> * NAND controller data struct >>>> * >>>> @@ -329,6 +374,7 @@ struct nandc_regs { >>>> * >>>> * @core_clk: controller clock >>>> * @aon_clk: another controller clock >>>> + * @iomacro_clk: io macro clock >>>> * >>>> * @regs: a contiguous chunk of memory for DMA register >>>> * writes. contains the register values to be >>>> @@ -338,6 +384,7 @@ struct nandc_regs { >>>> * initialized via DT match data >>>> * >>>> * @controller: base controller structure >>>> + * @ctlr: spi controller structure >>>> * @host_list: list containing all the chips attached to the >>>> * controller >>>> * >>>> @@ -375,6 +422,7 @@ struct qcom_nand_controller { >>>> >> struct clk *core_clk; >>>> struct clk *aon_clk; >>>> + struct clk *iomacro_clk; >>>> >> struct nandc_regs *regs; >>>> struct bam_transaction *bam_txn; >>>> @@ -382,6 +430,7 @@ struct qcom_nand_controller { >>>> const struct qcom_nandc_props *props; >>>> >> struct nand_controller controller; >>>> + struct spi_controller *ctlr; >>>> struct list_head host_list; >>>> >> union { >>>> @@ -418,6 +467,21 @@ struct qcom_nand_controller { >>>> >> u32 cmd1, vld; >>>> bool exec_opwrite; >>>> + struct qpic_ecc *ecc; >>>> + struct qcom_ecc_stats ecc_stats; >>>> + struct nand_ecc_engine ecc_eng; >>>> + u8 *data_buf; >>>> + u8 *oob_buf; >>>> + u32 wlen; >>>> + u32 addr1; >>>> + u32 addr2; >>>> + u32 cmd; >>>> + u32 num_cw; >>>> + u32 pagesize; >>>> + bool oob_rw; >>>> + bool page_rw; >>>> + bool raw_rw; >>>> + bool read_last_cw; >>>> }; >>> >>> If all these definitions are only used by the spi controller, I don't >>> see why you want to put them in the common file. >> We are using qcom_nand_controller{..} structure as common b/w raw nand >> and spi nand. These all variables will be used by spi nand only , but >> qcom_nand_controller structure is passed across all the SPI API, thats >> why define these all variables inside qcom_nand_controller structure. >> so that i can access directlty. > > Maybe you can move the spi-nand specific variables in a struct, and the > raw NAND specific variables in another, and then use an enum in this > structure. This way only the useful fields are available. Or maybe you > can have two pointers and only populate the relevant one from the > relevant driver with the fields that are missing. But this is a generic > include, so don't put specific fields there just because it is > convenient. Ok , will do in next patch. > > Thanks, > Miquèl