[PATCH v1 0/2] spi: loongson: add bus driver for the loongson spi

[PATCH v1 0/2] spi: loongson: add bus driver for the loongson spi

[Yinbo Zhu]

Loongson platform support spi hardware controller and this series patch
was to add spi driver and binding support.

Yinbo Zhu (2):
  dt-bindings: spi: add loongson spi
  spi: loongson: add bus driver for the loongson spi controller

 .../bindings/spi/loongson,ls-spi.yaml         |  47 ++
 MAINTAINERS                                   |   7 +
 drivers/spi/Kconfig                           |  10 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-loongson.c                    | 502 ++++++++++++++++++
 5 files changed, 567 insertions(+)

-- 
2.20.1

[PATCH v1 1/2] dt-bindings: spi: add loongson spi

[Yinbo Zhu]

Add the Loongson platform spi binding with DT schema format using
json-schema.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml

diff --git a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
new file mode 100644
index 000000000000..8a13a96b3818
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Loongson SPI controller
+
+maintainers:
+  - Yinbo Zhu <zhuyinbo@loongson.cn>
+
+allOf:
+  - $ref: /schemas/spi/spi-controller.yaml#
+
+properties:
+  compatible:
+    const: loongson,ls2k-spi
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: boot
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/loongson,ls2k-clk.h>
+    spi0: spi@1fff0220{
+        compatible = "loongson,ls2k-spi";
+        reg = <0x1fff0220 0x10>;
+        clocks = <&clk LOONGSON2_BOOT_CLK>;
+        clock-names = "boot";
+        #address-cells = <1>;
+        #size-cells = <0>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 631fac6ab555..0efb739e793f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12156,6 +12156,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/clock/loongson,ls2k-clk.yaml
 F:	include/dt-bindings/clock/loongson,ls2k-clk.h
 
+LOONGSON SPI DRIVER
+M:	Yinbo Zhu <zhuyinbo@loongson.cn>
+L:	linux-spi@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
+
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
 M:	Sathya Prakash <sathya.prakash@broadcom.com>
 M:	Sreekanth Reddy <sreekanth.reddy@broadcom.com>
-- 
2.20.1

[PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller

[Yinbo Zhu]

This bus driver supports the Loongson spi hardware controller in the
Loongson platforms and supports to use DTS and PCI framework to
register spi device resources.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 MAINTAINERS                |   1 +
 drivers/spi/Kconfig        |  10 +
 drivers/spi/Makefile       |   1 +
 drivers/spi/spi-loongson.c | 502 +++++++++++++++++++++++++++++++++++++
 4 files changed, 514 insertions(+)
 create mode 100644 drivers/spi/spi-loongson.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0efb739e793f..2fd7a00ab120 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12161,6 +12161,7 @@ M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-spi@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
+F:	drivers/spi/spi-loongson.c
 
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
 M:	Sathya Prakash <sathya.prakash@broadcom.com>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 36a18c215163..ad90b531ff96 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -509,6 +509,16 @@ config SPI_LM70_LLP
 	  which interfaces to an LM70 temperature sensor using
 	  a parallel port.
 
+config SPI_LOONGSON
+	tristate "Loongson SPI Controller Support"
+	depends on LOONGARCH && OF && PCI
+	help
+	  This bus driver supports the Loongson spi hardware controller in
+	  the Loongson platforms and supports to use DTS and PCI framework
+	  to register spi device resources.
+	  Say Y or M here if you want to use the SPI controller on
+	  Loongson platform.
+
 config SPI_LP8841_RTC
 	tristate "ICP DAS LP-8841 SPI Controller for RTC"
 	depends on MACH_PXA27X_DT || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d87cf75bee6a..cab3ff6d1814 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
 obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
 obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
+obj-$(CONFIG_SPI_LOONGSON)		+= spi-loongson.o
 obj-$(CONFIG_SPI_LP8841_RTC)		+= spi-lp8841-rtc.o
 obj-$(CONFIG_SPI_MESON_SPICC)		+= spi-meson-spicc.o
 obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
diff --git a/drivers/spi/spi-loongson.c b/drivers/spi/spi-loongson.c
new file mode 100644
index 000000000000..26316e873965
--- /dev/null
+++ b/drivers/spi/spi-loongson.c
@@ -0,0 +1,502 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Loongson SPI Support
+ *
+ * Copyright (C) 2023 Loongson Technology Corporation Limited
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/pci.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+
+#define	LOONGSON_SPI_SPCR_REG	0x00
+#define	LOONGSON_SPI_SPSR_REG	0x01
+#define	LOONGSON_SPI_FIFO_REG	0x02
+#define	LOONGSON_SPI_SPER_REG	0x03
+#define	LOONGSON_SPI_PARA_REG	0x04
+#define	LOONGSON_SPI_SFCS_REG	0x05
+#define	LOONGSON_SPI_TIMI_REG	0x06
+
+/* Bits definition for Loongson SPI register */
+#define	LOONGSON_SPI_PARA_MEM_EN	BIT(0)
+#define	LOONGSON_SPI_SPSR_SPIF	BIT(7)
+#define	LOONGSON_SPI_SPSR_WCOL	BIT(6)
+#define	LOONGSON_SPI_SPCR_SPE	BIT(6)
+
+struct loongson_spi {
+	struct work_struct	work;
+	spinlock_t		lock;
+	struct	list_head	msg_queue;
+	struct	spi_master	*master;
+	void __iomem		*base;
+	int			cs_active;
+	unsigned int		hz;
+	unsigned char		spcr;
+	unsigned char		sper;
+	unsigned char		spsr;
+	unsigned char		para;
+	unsigned char		sfcs;
+	unsigned char		timi;
+	struct workqueue_struct	*wq;
+	unsigned int		mode;
+	u64			clk_rate;
+};
+
+static inline void loongson_spi_write_reg(struct loongson_spi *spi, unsigned char reg,
+					  unsigned char data)
+{
+	writeb(data, spi->base + reg);
+}
+
+static inline char loongson_spi_read_reg(struct loongson_spi *spi, unsigned char reg)
+{
+	return readb(spi->base + reg);
+}
+
+static inline int loongson_spi_set_cs(struct loongson_spi *loongson_spi,
+				      struct spi_device *spi, int val)
+{
+	int cs;
+
+	if (spi->mode & SPI_CS_HIGH)
+		val = !val;
+
+	if (loongson_spi->mode & SPI_NO_CS)
+		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, 0);
+	else {
+		cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
+					   & ~(0x11 << spi->chip_select);
+		loongson_spi_write_reg(loongson_spi,
+				       LOONGSON_SPI_SFCS_REG,
+				       (val ? (0x11 << spi->chip_select) :
+				       (0x1 << spi->chip_select)) | cs);
+	}
+
+	return 0;
+}
+
+static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
+				     struct spi_device *spi, struct spi_transfer *t)
+{
+	unsigned int hz;
+	unsigned int div, div_tmp;
+	unsigned int bit;
+	unsigned char val;
+	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
+
+	hz  = t ? t->speed_hz : spi->max_speed_hz;
+
+	if (!hz)
+		hz = spi->max_speed_hz;
+
+	if ((hz && loongson_spi->hz != hz) ||
+	    ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {
+		div = DIV_ROUND_UP(loongson_spi->clk_rate, hz);
+		if (div < 2)
+			div = 2;
+		if (div > 4096)
+			div = 4096;
+
+		bit = fls(div) - 1;
+		if ((1<<bit) == div)
+			bit--;
+		div_tmp = rdiv[bit];
+		dev_dbg(&spi->dev, "clk_rate = %llu hz = %d div_tmp = %d bit = %d\n",
+			loongson_spi->clk_rate, hz, div_tmp, bit);
+
+		loongson_spi->hz = hz;
+		loongson_spi->spcr = div_tmp & 3;
+		loongson_spi->sper = (div_tmp >> 2) & 3;
+		val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
+		val &= ~0xc;
+		if (spi->mode & SPI_CPOL)
+			val |= 8;
+		if (spi->mode & SPI_CPHA)
+			val |= 4;
+
+		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
+				       loongson_spi->spcr);
+		val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
+		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
+				       loongson_spi->sper);
+		loongson_spi->mode &= SPI_NO_CS;
+		loongson_spi->mode |= spi->mode;
+	}
+
+	return 0;
+}
+
+static int loongson_spi_setup(struct spi_device *spi)
+{
+	struct loongson_spi *loongson_spi;
+
+	loongson_spi = spi_master_get_devdata(spi->master);
+	if (spi->bits_per_word % 8)
+		return -EINVAL;
+
+	if (spi->chip_select >= spi->master->num_chipselect)
+		return -EINVAL;
+
+	loongson_spi_update_state(loongson_spi, spi, NULL);
+	loongson_spi_set_cs(loongson_spi, spi, 1);
+
+	return 0;
+}
+
+static int loongson_spi_write_read_8bit(struct spi_device *spi, const u8 **tx_buf,
+					u8 **rx_buf, unsigned int num)
+{
+	struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
+
+	if (tx_buf && *tx_buf) {
+		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
+		while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1)
+			;
+	} else {
+		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, 0);
+		while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1)
+			;
+	}
+
+	if (rx_buf && *rx_buf)
+		*(*rx_buf)++ = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_FIFO_REG);
+	else
+		loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_FIFO_REG);
+
+	return 0;
+}
+
+static unsigned int loongson_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
+{
+	unsigned int count;
+	const u8 *tx = xfer->tx_buf;
+	u8 *rx = xfer->rx_buf;
+
+	count = xfer->len;
+
+	do {
+		if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
+			goto out;
+		count--;
+	} while (count);
+
+out:
+	return xfer->len - count;
+}
+
+static void loongson_spi_work(struct work_struct *work)
+{
+	int param;
+	struct spi_message *m;
+	struct spi_device  *spi;
+	struct spi_transfer *t = NULL;
+	struct loongson_spi *loongson_spi = container_of(work, struct loongson_spi, work);
+
+	spin_lock(&loongson_spi->lock);
+	param = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, param&~1);
+	while (!list_empty(&loongson_spi->msg_queue)) {
+		m = container_of(loongson_spi->msg_queue.next, struct spi_message, queue);
+
+		list_del_init(&m->queue);
+		spin_unlock(&loongson_spi->lock);
+		spi = m->spi;
+		loongson_spi_set_cs(loongson_spi, spi, 0);
+
+		list_for_each_entry(t, &m->transfers, transfer_list) {
+			loongson_spi_update_state(loongson_spi, spi, t);
+			if (t->len)
+				m->actual_length += loongson_spi_write_read(spi, t);
+		}
+
+		loongson_spi_set_cs(loongson_spi, spi, 1);
+		m->complete(m->context);
+		spin_lock(&loongson_spi->lock);
+	}
+
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, param);
+	spin_unlock(&loongson_spi->lock);
+}
+
+static int loongson_spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+	struct loongson_spi *loongson_spi;
+	struct spi_transfer *t = NULL;
+
+	m->status = 0;
+	m->actual_length = 0;
+
+	if (list_empty(&m->transfers) || !m->complete)
+		return -EINVAL;
+
+	loongson_spi = spi_master_get_devdata(spi->master);
+
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+
+		if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
+			dev_err(&spi->dev, "invalid transfer data buffers\n");
+			goto msg_rejected;
+		}
+	}
+
+	spin_lock(&loongson_spi->lock);
+	list_add_tail(&m->queue, &loongson_spi->msg_queue);
+	queue_work(loongson_spi->wq, &loongson_spi->work);
+	spin_unlock(&loongson_spi->lock);
+
+	return 0;
+msg_rejected:
+	m->status = -EINVAL;
+
+	if (m->complete)
+		m->complete(m->context);
+
+	return -EINVAL;
+}
+
+static void loongson_spi_reginit(struct loongson_spi *loongson_spi_dev)
+{
+	unsigned char val;
+
+	val = loongson_spi_read_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG);
+	val &= ~LOONGSON_SPI_SPCR_SPE;
+	loongson_spi_write_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG, val);
+
+	loongson_spi_write_reg(loongson_spi_dev, LOONGSON_SPI_SPSR_REG,
+			       (LOONGSON_SPI_SPSR_SPIF | LOONGSON_SPI_SPSR_WCOL));
+
+	val = loongson_spi_read_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG);
+	val |= LOONGSON_SPI_SPCR_SPE;
+	loongson_spi_write_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG, val);
+}
+
+static int loongson_spi_init_master(struct device *dev, struct resource *res)
+{
+	struct spi_master *master;
+	struct loongson_spi *spi;
+	struct clk *clk;
+	int ret;
+
+	master = spi_alloc_master(dev, sizeof(struct loongson_spi));
+	if (master == NULL) {
+		dev_dbg(dev, "master allocation failed\n");
+		return -ENOMEM;
+	}
+
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+	master->setup = loongson_spi_setup;
+	master->transfer = loongson_spi_transfer;
+	master->num_chipselect = 4;
+	master->dev.of_node = of_node_get(dev->of_node);
+	dev_set_drvdata(dev, master);
+
+	spi = spi_master_get_devdata(master);
+
+	spi->master = master;
+
+	spi->wq	= create_singlethread_workqueue("loongson-spi");
+
+	spi->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (spi->base == NULL) {
+		dev_err(dev, "cannot map io\n");
+		ret = -ENXIO;
+		goto free_master;
+	}
+
+	clk = devm_clk_get(dev, NULL);
+	if (!IS_ERR(clk))
+		spi->clk_rate = clk_get_rate(clk);
+
+	loongson_spi_reginit(spi);
+
+	spi->mode = 0;
+	if (of_get_property(dev->of_node, "spi-nocs", NULL))
+		spi->mode |= SPI_NO_CS;
+
+	INIT_WORK(&spi->work, loongson_spi_work);
+
+	spin_lock_init(&spi->lock);
+	INIT_LIST_HEAD(&spi->msg_queue);
+
+	ret = spi_register_master(master);
+	if (ret < 0)
+		goto free_master;
+
+	return ret;
+
+free_master:
+	kfree(master);
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int loongson_spi_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		dev_err(dev, "cannot get io resource memory\n");
+		return -ENOENT;
+	}
+
+	ret = loongson_spi_init_master(dev, res);
+	if (ret)
+		dev_err(dev, "failed to initialize master\n");
+
+	return ret;
+}
+
+static int __maybe_unused loongson_spi_suspend(struct device *dev)
+{
+	struct loongson_spi *loongson_spi;
+	struct spi_master *master;
+
+	master = dev_get_drvdata(dev);
+	loongson_spi = spi_master_get_devdata(master);
+
+	loongson_spi->spcr = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
+	loongson_spi->sper = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
+	loongson_spi->spsr = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG);
+	loongson_spi->para = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
+	loongson_spi->sfcs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG);
+	loongson_spi->timi = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_TIMI_REG);
+
+	return 0;
+}
+
+static int __maybe_unused loongson_spi_resume(struct device *dev)
+{
+	struct loongson_spi *loongson_spi;
+	struct spi_master *master;
+
+	master = dev_get_drvdata(dev);
+	loongson_spi = spi_master_get_devdata(master);
+
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, loongson_spi->spcr);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, loongson_spi->sper);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPSR_REG, loongson_spi->spsr);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, loongson_spi->sfcs);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_TIMI_REG, loongson_spi->timi);
+
+	return 0;
+}
+
+static const struct dev_pm_ops loongson_spi_dev_pm_ops = {
+	.suspend = loongson_spi_suspend,
+	.resume = loongson_spi_resume,
+};
+
+static const struct of_device_id loongson_spi_id_table[] = {
+	{ .compatible = "loongson,ls2k-spi", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, loongson_spi_id_table);
+
+static struct platform_driver loongson_spi_driver = {
+	.probe = loongson_spi_platform_probe,
+	.driver	= {
+		.name	= "loongson-spi",
+		.owner	= THIS_MODULE,
+		.bus = &platform_bus_type,
+		.pm = &loongson_spi_dev_pm_ops,
+		.of_match_table = loongson_spi_id_table,
+	},
+};
+
+static int loongson_spi_pci_register(struct pci_dev *pdev,
+			const struct pci_device_id *ent)
+{
+	int ret;
+	unsigned char v8;
+	struct resource res[2];
+	struct device *dev = &pdev->dev;
+
+	ret = pci_enable_device(pdev);
+	if (ret < 0) {
+		dev_err(dev, "cannot enable pci device\n");
+		goto err_out;
+	}
+
+	ret = pci_request_region(pdev, 0, "loongson-spi io");
+	if (ret < 0) {
+		dev_err(dev, "cannot request region 0.\n");
+		goto err_out;
+	}
+
+	res[0].start = pci_resource_start(pdev, 0);
+	res[0].end = pci_resource_end(pdev, 0);
+	ret = pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &v8);
+
+	if (ret == PCIBIOS_SUCCESSFUL) {
+		res[1].start = v8;
+		res[1].end = v8;
+	}
+
+	ret = loongson_spi_init_master(dev, res);
+	if (ret)
+		dev_err(dev, "failed to initialize master\n");
+
+err_out:
+	return ret;
+}
+
+static void loongson_spi_pci_unregister(struct pci_dev *pdev)
+{
+	pci_release_region(pdev, 0);
+}
+
+static struct pci_device_id loongson_spi_devices[] = {
+	{PCI_DEVICE(0x14, 0x7a0b)},
+	{PCI_DEVICE(0x14, 0x7a1b)},
+	{0, 0, 0, 0, 0, 0, 0}
+};
+MODULE_DEVICE_TABLE(pci, loongson_spi_devices);
+
+static struct pci_driver loongson_spi_pci_driver = {
+	.name       = "loongson-spi-pci",
+	.id_table   = loongson_spi_devices,
+	.probe      = loongson_spi_pci_register,
+	.remove     = loongson_spi_pci_unregister,
+};
+
+static int __init loongson_spi_init(void)
+{
+	int ret;
+
+	ret =  platform_driver_register(&loongson_spi_driver);
+	if (ret)
+		return ret;
+
+	ret = pci_register_driver(&loongson_spi_pci_driver);
+	if (ret) {
+		platform_driver_unregister(&loongson_spi_driver);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit loongson_spi_exit(void)
+{
+	platform_driver_unregister(&loongson_spi_driver);
+	pci_unregister_driver(&loongson_spi_pci_driver);
+}
+
+subsys_initcall(loongson_spi_init);
+module_exit(loongson_spi_exit);
+
+MODULE_DESCRIPTION("Loongson spi driver");
+MODULE_LICENSE("GPL");
-- 
2.20.1

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[Krzysztof Kozlowski]

On 08/03/2023 03:59, Yinbo Zhu wrote:
> Add the Loongson platform spi binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml

Filename matching the compatible.

> 
> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> new file mode 100644
> index 000000000000..8a13a96b3818
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop the quotes. What was the base of your code here?

> +
> +title: Loongson SPI controller
> +
> +maintainers:
> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: loongson,ls2k-spi
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: boot

Drop clock-names, not needed for single entry.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +unevaluatedProperties: false


Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[Rob Herring]

On Wed, 08 Mar 2023 10:59:07 +0800, Yinbo Zhu wrote:
> Add the Loongson platform spi binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts:22.28-29 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230308025908.21491-2-zhuyinbo@loongson.cn

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

On Wed, Mar 08, 2023 at 10:59:08AM +0800, Yinbo Zhu wrote:

> +config SPI_LOONGSON
> +	tristate "Loongson SPI Controller Support"
> +	depends on LOONGARCH && OF && PCI

I'm not seeing any build time dependencies here (possibly PCI?) so
please add an || COMPILE_TEST to improve build coverage.  It'd be better
to have separate modules for the platform and PCI functionality, that
way someone who has a system without PCI can still use the driver even
with PCI support disabled.

> +++ b/drivers/spi/spi-loongson.c
> @@ -0,0 +1,502 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Loongson SPI Support
> + *
> + * Copyright (C) 2023 Loongson Technology Corporation Limited

Please make the entire comment block a C++ one so things look more
intentional.

> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
> +				     struct spi_device *spi, struct spi_transfer *t)
> +{
> +	unsigned int hz;
> +	unsigned int div, div_tmp;
> +	unsigned int bit;
> +	unsigned char val;
> +	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
> +
> +	hz  = t ? t->speed_hz : spi->max_speed_hz;

Please write normal conditional statements so that things are legible,
though in this case the core will ensure that there's a speed_hz in
every transfer so there's no need for any of the logic around ensuring
it's set.

> +static int loongson_spi_setup(struct spi_device *spi)
> +{
> +	struct loongson_spi *loongson_spi;
> +
> +	loongson_spi = spi_master_get_devdata(spi->master);
> +	if (spi->bits_per_word % 8)
> +		return -EINVAL;
> +
> +	if (spi->chip_select >= spi->master->num_chipselect)
> +		return -EINVAL;
> +
> +	loongson_spi_update_state(loongson_spi, spi, NULL);
> +	loongson_spi_set_cs(loongson_spi, spi, 1);

Note that setup() needs to be able to run for one device while there are
transfers for other devices on the same controller active.

> +static int loongson_spi_write_read_8bit(struct spi_device *spi, const u8 **tx_buf,
> +					u8 **rx_buf, unsigned int num)
> +{
> +	struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
> +
> +	if (tx_buf && *tx_buf) {
> +		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
> +		while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1)

> +			;

A timeout would be good on these spins in case the controller gets
stuck.  It'd also be polite to have a cpu_relax() somewhere either here
or in the caller given that it's busy waiting.

> +static void loongson_spi_work(struct work_struct *work)
> +{
> +	int param;
> +	struct spi_message *m;
> +	struct spi_device  *spi;
> +	struct spi_transfer *t = NULL;
> +	struct loongson_spi *loongson_spi = container_of(work, struct loongson_spi, work);
> +
> +	spin_lock(&loongson_spi->lock);
> +	param = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
> +	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, param&~1);
> +	while (!list_empty(&loongson_spi->msg_queue)) {
> +		m = container_of(loongson_spi->msg_queue.next, struct spi_message, queue);
> +

This all looks like it's open coding the core's message pump, only
without the heavy optimisation work that the core has and missing some
handling of cs_change and delays.  You should implement
spi_transfer_one() instead, this will save a lot of code and should be
more performant.

> +static int loongson_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{

In general you'd need an extremely strong reason to implement transfer()
in a new driver.

> +static int __maybe_unused loongson_spi_resume(struct device *dev)
> +{

> +static const struct dev_pm_ops loongson_spi_dev_pm_ops = {
> +	.suspend = loongson_spi_suspend,
> +	.resume = loongson_spi_resume,
> +};

The suspend/resume ops are assigned unconditionally.

> +subsys_initcall(loongson_spi_init);
> +module_exit(loongson_spi_exit);

Why not just a regular module initcall like most SPI drivers?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[zhuyinbo]

在 2023/3/8 下午7:30, Krzysztof Kozlowski 写道:
> On 08/03/2023 03:59, Yinbo Zhu wrote:
>> Add the Loongson platform spi binding with DT schema format using
>> json-schema.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>>   .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 +++
>>   2 files changed, 53 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> Filename matching the compatible.

loongson,ls-spi.yaml is for ls2k-spi and ls7a-spi, I will add following 
desription:


properties:
   compatible:
     enum:
       - loongson,ls2k-spi
       - loongson,ls7a-spi
>
>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>> new file mode 100644
>> index 000000000000..8a13a96b3818
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> Drop the quotes. What was the base of your code here?

okay, I will drop the quotes.    and I don't got it  about the code base 
that you said.

you meaning is advice me add a line  as follows ?

allOf:
   - $ref: /schemas/spi/spi-controller.yaml#

>
>> +
>> +title: Loongson SPI controller
>> +
>> +maintainers:
>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>> +
>> +allOf:
>> +  - $ref: /schemas/spi/spi-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: loongson,ls2k-spi
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: boot
> Drop clock-names, not needed for single entry.

if drop the clock-names entry, the yaml file will compile fail.


root@user-pc:/home/user/workspace/test/code/www.kernel.org/linux# make 
DT_CHECKER_FLAGS=-m dt_binding_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
   LINT    Documentation/devicetree/bindings
   CHKDT   Documentation/devicetree/bindings/processed-schema.json
   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
   DTEX Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts
   DTC_CHK Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb
/home/user/workspace/test/code/www.kernel.org/linux/Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb: 
spi@1fff0220: Unevaluated properties are not allowed ('clock-names' was 
unexpected)
     From schema: 
/home/user/workspace/test/code/www.kernel.org/linux/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
root@user-pc:/home/user/workspace/test/code/www.kernel.org/linux#

>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +unevaluatedProperties: false
>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[Krzysztof Kozlowski]

On 09/03/2023 03:08, zhuyinbo wrote:
> 
> 在 2023/3/8 下午7:30, Krzysztof Kozlowski 写道:
>> On 08/03/2023 03:59, Yinbo Zhu wrote:
>>> Add the Loongson platform spi binding with DT schema format using
>>> json-schema.
>>>
>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>> ---
>>>   .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>>>   MAINTAINERS                                   |  6 +++
>>>   2 files changed, 53 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>> Filename matching the compatible.
> 
> loongson,ls-spi.yaml is for ls2k-spi and ls7a-spi, I will add following 
> desription:
> 
> 
> properties:
>    compatible:
>      enum:
>        - loongson,ls2k-spi
>        - loongson,ls7a-spi

OK then.

>>
>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>> new file mode 100644
>>> index 000000000000..8a13a96b3818
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>> @@ -0,0 +1,47 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> Drop the quotes. What was the base of your code here?
> 
> okay, I will drop the quotes.    and I don't got it  about the code base 
> that you said.
> 
> you meaning is advice me add a line  as follows ?

I meant, from which other file did you copy it?

>>> +
>>> +  clock-names:
>>> +    const: boot
>> Drop clock-names, not needed for single entry.
> 
> if drop the clock-names entry, the yaml file will compile fail.

Obviously you have to also drop it from DTS and driver...


Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[zhuyinbo]

在 2023/3/9 下午2:23, Krzysztof Kozlowski 写道:
> On 09/03/2023 03:08, zhuyinbo wrote:
>> 在 2023/3/8 下午7:30, Krzysztof Kozlowski 写道:
>>> On 08/03/2023 03:59, Yinbo Zhu wrote:
>>>> Add the Loongson platform spi binding with DT schema format using
>>>> json-schema.
>>>>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> ---
>>>>    .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>>>>    MAINTAINERS                                   |  6 +++
>>>>    2 files changed, 53 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>> Filename matching the compatible.
>> loongson,ls-spi.yaml is for ls2k-spi and ls7a-spi, I will add following
>> desription:
>>
>>
>> properties:
>>     compatible:
>>       enum:
>>         - loongson,ls2k-spi
>>         - loongson,ls7a-spi
> OK then.

I was to explain why that yaml was name as "loongson,ls-spi.yaml"  
rather than "loongson,ls2k-spi.yaml"

because that need consider about  yaml filename to match 
"loongson,ls2k-spi" and "loongson,ls7a-spi".

>
>>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>> new file mode 100644
>>>> index 000000000000..8a13a96b3818
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>> @@ -0,0 +1,47 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>> Drop the quotes. What was the base of your code here?
>> okay, I will drop the quotes.    and I don't got it  about the code base
>> that you said.
>>
>> you meaning is advice me add a line  as follows ?
> I meant, from which other file did you copy it?
okay,  but I maybe forgot it,  I should be refer other spi yaml file.
>
>>>> +
>>>> +  clock-names:
>>>> +    const: boot
>>> Drop clock-names, not needed for single entry.
>> if drop the clock-names entry, the yaml file will compile fail.
> Obviously you have to also drop it from DTS and driver...

drop clock-names should be not  affect my driver,  but I notice other 
lots of arm64 platform dts

was keep clock-names and clock in dts when use grep search "clock-names".

[zhuyinbo@localhost www.kernel.org]$ grep -rns "clock-names" arch/arm64/

arch/arm64/boot/dts/sprd/sc9863a.dtsi:280:            clock-names = 
"apb_pclk";
arch/arm64/boot/dts/sprd/sc9863a.dtsi:305:            clock-names = 
"apb_pclk";
arch/arm64/boot/dts/sprd/sc9863a.dtsi:330:            clock-names = 
"apb_pclk";
arch/arm64/boot/dts/sprd/sc9863a.dtsi:367:            clock-names = 
"apb_pclk";

>
>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[zhuyinbo]

在 2023/3/8 下午10:06, Rob Herring 写道:
> On Wed, 08 Mar 2023 10:59:07 +0800, Yinbo Zhu wrote:
>> Add the Loongson platform spi binding with DT schema format using
>> json-schema.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>>   .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 +++
>>   2 files changed, 53 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts:22.28-29 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1512: dt_binding_check] Error 2
>
> doc reference errors (make refcheckdocs):

This yaml patch need depend on

https://lore.kernel.org/all/20230307115022.12846-1-zhuyinbo@loongson.cn/

, then yaml  compile will be successfull.

>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230308025908.21491-2-zhuyinbo@loongson.cn
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[Krzysztof Kozlowski]

On 10/03/2023 03:31, zhuyinbo wrote:
> 
> 在 2023/3/8 下午10:06, Rob Herring 写道:
>> On Wed, 08 Mar 2023 10:59:07 +0800, Yinbo Zhu wrote:
>>> Add the Loongson platform spi binding with DT schema format using
>>> json-schema.
>>>
>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>> ---
>>>   .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>>>   MAINTAINERS                                   |  6 +++
>>>   2 files changed, 53 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Error: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts:22.28-29 syntax error
>> FATAL ERROR: Unable to parse input tree
>> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb] Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> make: *** [Makefile:1512: dt_binding_check] Error 2
>>
>> doc reference errors (make refcheckdocs):
> 
> This yaml patch need depend on
> 
> https://lore.kernel.org/all/20230307115022.12846-1-zhuyinbo@loongson.cn/
> 
> , then yaml  compile will be successfull.

Nothing in the patch changelog (where it is preferred), not even cover
letter, mention dependencies.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[zhuyinbo]

在 2023/3/10 下午5:08, Krzysztof Kozlowski 写道:
> On 10/03/2023 03:31, zhuyinbo wrote:
>> 在 2023/3/8 下午10:06, Rob Herring 写道:
>>> On Wed, 08 Mar 2023 10:59:07 +0800, Yinbo Zhu wrote:
>>>> Add the Loongson platform spi binding with DT schema format using
>>>> json-schema.
>>>>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> ---
>>>>    .../bindings/spi/loongson,ls-spi.yaml         | 47 +++++++++++++++++++
>>>>    MAINTAINERS                                   |  6 +++
>>>>    2 files changed, 53 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>>
>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>>
>>> yamllint warnings/errors:
>>>
>>> dtschema/dtc warnings/errors:
>>> Error: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts:22.28-29 syntax error
>>> FATAL ERROR: Unable to parse input tree
>>> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb] Error 1
>>> make[1]: *** Waiting for unfinished jobs....
>>> make: *** [Makefile:1512: dt_binding_check] Error 2
>>>
>>> doc reference errors (make refcheckdocs):
>> This yaml patch need depend on
>>
>> https://lore.kernel.org/all/20230307115022.12846-1-zhuyinbo@loongson.cn/
>>
>> , then yaml  compile will be successfull.
> Nothing in the patch changelog (where it is preferred), not even cover
> letter, mention dependencies.

okay, I will add it in changelog  in next version.

>
> Best regards,
> Krzysztof

Re: [PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller

[zhuyinbo]

在 2023/3/8 下午11:03, Mark Brown 写道:
> On Wed, Mar 08, 2023 at 10:59:08AM +0800, Yinbo Zhu wrote:
>
>> +config SPI_LOONGSON
>> +	tristate "Loongson SPI Controller Support"
>> +	depends on LOONGARCH && OF && PCI
> I'm not seeing any build time dependencies here (possibly PCI?) so
> please add an || COMPILE_TEST to improve build coverage.  It'd be better
> to have separate modules for the platform and PCI functionality, that
> way someone who has a system without PCI can still use the driver even
> with PCI support disabled.

I will add an || COMPILE_TEST and drop && PCI then add some CONFIG_PCI 
macro

to limit some pci code.

>
>> +++ b/drivers/spi/spi-loongson.c
>> @@ -0,0 +1,502 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Loongson SPI Support
>> + *
>> + * Copyright (C) 2023 Loongson Technology Corporation Limited
> Please make the entire comment block a C++ one so things look more
> intentional.
okay, I got it.
>
>> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
>> +				     struct spi_device *spi, struct spi_transfer *t)
>> +{
>> +	unsigned int hz;
>> +	unsigned int div, div_tmp;
>> +	unsigned int bit;
>> +	unsigned char val;
>> +	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
>> +
>> +	hz  = t ? t->speed_hz : spi->max_speed_hz;
> Please write normal conditional statements so that things are legible,
> though in this case the core will ensure that there's a speed_hz in
> every transfer so there's no need for any of the logic around ensuring
> it's set.
Do you mean to achieve the following ?  and drop spi->max_speed_hz.

if (t)

       hz = t->speed_hz;


>
>> +static int loongson_spi_setup(struct spi_device *spi)
>> +{
>> +	struct loongson_spi *loongson_spi;
>> +
>> +	loongson_spi = spi_master_get_devdata(spi->master);
>> +	if (spi->bits_per_word % 8)
>> +		return -EINVAL;
>> +
>> +	if (spi->chip_select >= spi->master->num_chipselect)
>> +		return -EINVAL;
>> +
>> +	loongson_spi_update_state(loongson_spi, spi, NULL);
>> +	loongson_spi_set_cs(loongson_spi, spi, 1);
> Note that setup() needs to be able to run for one device while there are
> transfers for other devices on the same controller active.
okay, I will add a spin_lock for it.
>
>> +static int loongson_spi_write_read_8bit(struct spi_device *spi, const u8 **tx_buf,
>> +					u8 **rx_buf, unsigned int num)
>> +{
>> +	struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
>> +
>> +	if (tx_buf && *tx_buf) {
>> +		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
>> +		while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1)
>> +			;
> A timeout would be good on these spins in case the controller gets
> stuck.  It'd also be polite to have a cpu_relax() somewhere either here
> or in the caller given that it's busy waiting.
okay, I got it.
>
>> +static void loongson_spi_work(struct work_struct *work)
>> +{
>> +	int param;
>> +	struct spi_message *m;
>> +	struct spi_device  *spi;
>> +	struct spi_transfer *t = NULL;
>> +	struct loongson_spi *loongson_spi = container_of(work, struct loongson_spi, work);
>> +
>> +	spin_lock(&loongson_spi->lock);
>> +	param = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
>> +	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, param&~1);
>> +	while (!list_empty(&loongson_spi->msg_queue)) {
>> +		m = container_of(loongson_spi->msg_queue.next, struct spi_message, queue);
>> +
> This all looks like it's open coding the core's message pump, only
> without the heavy optimisation work that the core has and missing some
> handling of cs_change and delays.  You should implement
> spi_transfer_one() instead, this will save a lot of code and should be
> more performant.
okay, I will try to add a spi_transfer_one for this.
>
>> +static int loongson_spi_transfer(struct spi_device *spi, struct spi_message *m)
>> +{
> In general you'd need an extremely strong reason to implement transfer()
> in a new driver.
okay, I got it.
>
>> +static int __maybe_unused loongson_spi_resume(struct device *dev)
>> +{
>> +static const struct dev_pm_ops loongson_spi_dev_pm_ops = {
>> +	.suspend = loongson_spi_suspend,
>> +	.resume = loongson_spi_resume,
>> +};
> The suspend/resume ops are assigned unconditionally.
sorry, I don't got it,  you mean was to  add a CONFIG_PM to limit code ?
>
>> +subsys_initcall(loongson_spi_init);
>> +module_exit(loongson_spi_exit);
> Why not just a regular module initcall like most SPI drivers?
okay, I will use module_init for register spi drivers.

Re: [PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Fri, Mar 10, 2023 at 06:01:23PM +0800, zhuyinbo wrote:
> 在 2023/3/8 下午11:03, Mark Brown 写道:
> > On Wed, Mar 08, 2023 at 10:59:08AM +0800, Yinbo Zhu wrote:

> > > +	hz  = t ? t->speed_hz : spi->max_speed_hz;

> > Please write normal conditional statements so that things are legible,
> > though in this case the core will ensure that there's a speed_hz in
> > every transfer so there's no need for any of the logic around ensuring
> > it's set.

> Do you mean to achieve the following ?  and drop spi->max_speed_hz.

> if (t)
> 
>       hz = t->speed_hz;

Yes.

> > > +	loongson_spi_update_state(loongson_spi, spi, NULL);
> > > +	loongson_spi_set_cs(loongson_spi, spi, 1);

> > Note that setup() needs to be able to run for one device while there are
> > transfers for other devices on the same controller active.

> okay, I will add a spin_lock for it.

We also need to take care not to change the hardware
configuration for the currently running transfer (can't
remember if that's an issue here or not, but it's a common
one).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[zhuyinbo]

在 2023/3/9 下午3:22, zhuyinbo 写道:
>
> 在 2023/3/9 下午2:23, Krzysztof Kozlowski 写道:
>> On 09/03/2023 03:08, zhuyinbo wrote:
>>> 在 2023/3/8 下午7:30, Krzysztof Kozlowski 写道:
>>>> On 08/03/2023 03:59, Yinbo Zhu wrote:
>>>>> Add the Loongson platform spi binding with DT schema format using
>>>>> json-schema.
>>>>>
>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>> ---
>>>>>    .../bindings/spi/loongson,ls-spi.yaml         | 47 
>>>>> +++++++++++++++++++
>>>>>    MAINTAINERS                                   |  6 +++
>>>>>    2 files changed, 53 insertions(+)
>>>>>    create mode 100644 
>>>>> Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>> Filename matching the compatible.
>>> loongson,ls-spi.yaml is for ls2k-spi and ls7a-spi, I will add following
>>> desription:
>>>
>>>
>>> properties:
>>>     compatible:
>>>       enum:
>>>         - loongson,ls2k-spi
>>>         - loongson,ls7a-spi
>> OK then.
>
> I was to explain why that yaml was name as "loongson,ls-spi.yaml" 
> rather than "loongson,ls2k-spi.yaml"
>
> because that need consider about  yaml filename to match 
> "loongson,ls2k-spi" and "loongson,ls7a-spi".
>
>>
>>>>> diff --git 
>>>>> a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml 
>>>>> b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..8a13a96b3818
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>>> @@ -0,0 +1,47 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
>>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>>> Drop the quotes. What was the base of your code here?
>>> okay, I will drop the quotes.    and I don't got it  about the code 
>>> base
>>> that you said.
>>>
>>> you meaning is advice me add a line  as follows ?
>> I meant, from which other file did you copy it?
> okay,  but I maybe forgot it,  I should be refer other spi yaml file.
>>
>>>>> +
>>>>> +  clock-names:
>>>>> +    const: boot
>>>> Drop clock-names, not needed for single entry.
>>> if drop the clock-names entry, the yaml file will compile fail.
>> Obviously you have to also drop it from DTS and driver...
>
> drop clock-names should be not  affect my driver,  but I notice other 
> lots of arm64 platform dts
>
> was keep clock-names and clock in dts when use grep search "clock-names".
>
> [zhuyinbo@localhost www.kernel.org]$ grep -rns "clock-names" arch/arm64/
>
> arch/arm64/boot/dts/sprd/sc9863a.dtsi:280:            clock-names = 
> "apb_pclk";
> arch/arm64/boot/dts/sprd/sc9863a.dtsi:305:            clock-names = 
> "apb_pclk";
> arch/arm64/boot/dts/sprd/sc9863a.dtsi:330:            clock-names = 
> "apb_pclk";
> arch/arm64/boot/dts/sprd/sc9863a.dtsi:367:            clock-names = 
> "apb_pclk";

so , if you think it is okay I will keep clock-names and clock in yaml 
file like other platform.

>
>>
>>
>> Best regards,
>> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: spi: add loongson spi

[Krzysztof Kozlowski]

On 13/03/2023 03:09, zhuyinbo wrote:
> 
> 在 2023/3/9 下午3:22, zhuyinbo 写道:
>>
>> 在 2023/3/9 下午2:23, Krzysztof Kozlowski 写道:
>>> On 09/03/2023 03:08, zhuyinbo wrote:
>>>> 在 2023/3/8 下午7:30, Krzysztof Kozlowski 写道:
>>>>> On 08/03/2023 03:59, Yinbo Zhu wrote:
>>>>>> Add the Loongson platform spi binding with DT schema format using
>>>>>> json-schema.
>>>>>>
>>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>>> ---
>>>>>>    .../bindings/spi/loongson,ls-spi.yaml         | 47 
>>>>>> +++++++++++++++++++
>>>>>>    MAINTAINERS                                   |  6 +++
>>>>>>    2 files changed, 53 insertions(+)
>>>>>>    create mode 100644 
>>>>>> Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>>> Filename matching the compatible.
>>>> loongson,ls-spi.yaml is for ls2k-spi and ls7a-spi, I will add following
>>>> desription:
>>>>
>>>>
>>>> properties:
>>>>     compatible:
>>>>       enum:
>>>>         - loongson,ls2k-spi
>>>>         - loongson,ls7a-spi
>>> OK then.
>>
>> I was to explain why that yaml was name as "loongson,ls-spi.yaml" 
>> rather than "loongson,ls2k-spi.yaml"
>>
>> because that need consider about  yaml filename to match 
>> "loongson,ls2k-spi" and "loongson,ls7a-spi".
>>
>>>
>>>>>> diff --git 
>>>>>> a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml 
>>>>>> b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..8a13a96b3818
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>>>>> @@ -0,0 +1,47 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>> +
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: "http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#"
>>>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>>>> Drop the quotes. What was the base of your code here?
>>>> okay, I will drop the quotes.    and I don't got it  about the code 
>>>> base
>>>> that you said.
>>>>
>>>> you meaning is advice me add a line  as follows ?
>>> I meant, from which other file did you copy it?
>> okay,  but I maybe forgot it,  I should be refer other spi yaml file.
>>>
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    const: boot
>>>>> Drop clock-names, not needed for single entry.
>>>> if drop the clock-names entry, the yaml file will compile fail.
>>> Obviously you have to also drop it from DTS and driver...
>>
>> drop clock-names should be not  affect my driver,  but I notice other 
>> lots of arm64 platform dts
>>
>> was keep clock-names and clock in dts when use grep search "clock-names".
>>
>> [zhuyinbo@localhost www.kernel.org]$ grep -rns "clock-names" arch/arm64/
>>
>> arch/arm64/boot/dts/sprd/sc9863a.dtsi:280:            clock-names = 
>> "apb_pclk";
>> arch/arm64/boot/dts/sprd/sc9863a.dtsi:305:            clock-names = 
>> "apb_pclk";
>> arch/arm64/boot/dts/sprd/sc9863a.dtsi:330:            clock-names = 
>> "apb_pclk";
>> arch/arm64/boot/dts/sprd/sc9863a.dtsi:367:            clock-names = 
>> "apb_pclk";
> 
> so , if you think it is okay I will keep clock-names and clock in yaml 
> file like other platform.

No, it's not ok.

Best regards,
Krzysztof