On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:
> +config SPI_QPIC_SNAND
> + tristate "QPIC SNAND controller"
> + default y
> + depends on ARCH_QCOM
> + help
> + QPIC_SNAND(Quad SPI) driver for Qualcomm QPIC_SNAND controller.
> +
I don't see any build dependencies on anything QC specific so please add
an || COMPILE_TEST here, this makes it much easier to do generic changes
without having to build some specific config.
> +++ b/drivers/spi/Makefile
> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA) += spi-xtensa-xtfpga.o
> obj-$(CONFIG_SPI_ZYNQ_QSPI) += spi-zynq-qspi.o
> obj-$(CONFIG_SPI_ZYNQMP_GQSPI) += spi-zynqmp-gqspi.o
> obj-$(CONFIG_SPI_AMD) += spi-amd.o
> +obj-$(CONFIG_SPI_QPIC_SNAND) += spi-qpic-snand.o
Please keep this alphabetically sorted (there are some mistakes there
but no need to add to them).
> + * Sricharan R <quic_srichara@xxxxxxxxxxx>
> + */
> +
> +#include <linux/mtd/spinand.h>
> +#include <linux/mtd/nand-qpic-common.h>
> +
This should be including the SPI API, and other API headers that are
used directly like the platform and clock APIs.
> +static int qcom_snand_init(struct qcom_nand_controller *snandc)
> +{
> + u32 snand_cfg_val = 0x0;
> + int ret;
...
> + ret = submit_descs(snandc);
> + if (ret)
> + dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
> +
> + free_descs(snandc);
This seems to be doing a bit more than I would expect an init function
to, and it's very surprising to see the descriptors freed immediately
after something called a submit (which suggests that the descriptors are
still in flight).
> +static int qpic_snand_read_page(struct qcom_nand_controller *snandc,
> + const struct spi_mem_op *op)
> +{
> + return 0;
> +}
> +
> +static int qpic_snand_write_page(struct qcom_nand_controller *snandc,
> + const struct spi_mem_op *op)
> +{
> + return 0;
> +}
...
> +static int qpic_snand_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> + struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->master);
> + 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);
> +
> + /*
> + * Check for page ops or normal ops
> + */
> + if (qpic_snand_is_page_op(op)) {
> + if (op->data.dir == SPI_MEM_DATA_IN)
> + return qpic_snand_read_page(snandc, op);
> + else
> + return qpic_snand_write_page(snandc, op);
So does the device actually support page operations? The above looks
like the driver will silently noop them.
> + 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;
The DMA mapping and clock enables only get undone in error handling,
they're not undone in the normal device release path.
> +
> + 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;
> +
> + ret = qcom_nandc_alloc(snandc);
> + if (ret)
> + goto err_snandc_alloc;
> +
> + ret = qcom_snand_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_snand_ecc_engine_ops;
> + 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");
> + goto err_init;
> + }
> +
> + ctlr->num_chipselect = QPIC_QSPI_NUM_CS;
> + ctlr->mem_ops = &qcom_spi_mem_ops;
> + ctlr->mem_caps = &qcom_snand_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_master(ctlr);
> + if (ret) {
> + dev_err(&pdev->dev, "spi_register_controller failed.\n");
> + goto err_init;
> + }
> +
> + return 0;
> +err_init:
> + qcom_nandc_unalloc(snandc);
> +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_snand_remove(struct platform_device *pdev)
> +{
> + struct spi_controller *ctlr = platform_get_drvdata(pdev);
> + spi_unregister_master(ctlr);
> +
> + return 0;
> +}
> +
> +static const struct qcom_nandc_props ipq9574_snandc_props = {
> + .dev_cmd_reg_start = 0x7000,
> + .is_bam = true,
> + .qpic_v2 = true,
> +};
> +
> +static const struct of_device_id qcom_snandc_of_match[] = {
> + {
> + .compatible = "qcom,ipq9574-nand",
> + .data = &ipq9574_snandc_props,
> + },
> + {}
> +}
> +MODULE_DEVICE_TABLE(of, qcom_snandc_of_match);
> +
> +static struct platform_driver qcom_snand_driver = {
> + .driver = {
> + .name = "qcom_snand",
> + .of_match_table = qcom_snandc_of_match,
> + },
> + .probe = qcom_snand_probe,
> + .remove = qcom_snand_remove,
> +};
> +module_platform_driver(qcom_snand_driver);
> +
> +MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores");
> +MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
Attachment:
signature.asc
Description: PGP signature
