On 10/31/2023 7:53 PM, Mark Brown wrote:
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.
Ok
+++ 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).
Ok
+ * 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.
Ok
+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).
Our controller supports only bam mode , that means for writing/reading even
single register we have to use bam.
submit_descs() is synchronous so I/O is complete when it returns.
Hence freeing the descriptor after it.
+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.
Sorry It was to do item and I missed to mention that in commit log.
Will add in V1.
+ 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.
Will fix in V1
