The SPI controller has the following characteristics:
- Full-duplex synchronous serial data transmission
- Support up to 4 variable length byte transmission
- Main mode support
- Mode failure generates an error flag and issues an interrupt request
- Double buffer receiver
- Serial clock with programmable polarity and phase
- SPI can be controlled in wait mode
- Support boot from SPI
Use mtd_debug tool to earse/write/read /dev/mtd0 on development.
eg:
[root@linux mtd-utils-1.0.0]# mtd_debug erase /dev/mtd0 0x20000 0x40000
Erased 262144 bytes from address 0x00020000 in flash
[root@linux mtd-utils-1.0.0]# mtd_debug write /dev/mtd0 0x20000 13 1.img
Copied 13 bytes from 1.img to address 0x00020000 in flash
[root@linux mtd-utils-1.0.0]# mtd_debug read /dev/mtd0 0x20000 13 2.img
Copied 13 bytes from address 0x00020000 in flash to 2.img
[root@linux mtd-utils-1.0.0]# cmp -l 1.img 2.img
Signed-off-by: Juxin Gao <[email protected]>
Signed-off-by: Qing Zhang <[email protected]>
---
v2:
- keep Kconfig and Makefile sorted
- make the entire comment a C++ one so things look more intentional
- Fix unclear indentation
- make conditional statements to improve legibility
- Don't use static inline
- the core handle message queue
- Add a new binding document
- Fix probe part mixed pdev and PCI
---
drivers/spi/Kconfig | 7 ++
drivers/spi/Makefile | 1 +
drivers/spi/spi-ls7a.c | 324 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 332 insertions(+)
create mode 100644 drivers/spi/spi-ls7a.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index aadaea0..af7c0d4 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -413,6 +413,13 @@ config SPI_LP8841_RTC
Say N here unless you plan to run the kernel on an ICP DAS
LP-8x4x industrial computer.
+config SPI_LS7A
+ tristate "Loongson LS7A SPI Controller Support"
+ depends on CPU_LOONGSON64 || COMPILE_TEST
+ help
+ This drivers supports the Loongson LS7A SPI controller in master
+ SPI mode.
+
config SPI_MPC52xx
tristate "Freescale MPC52xx SPI (non-PSC) controller support"
depends on PPC_MPC52xx
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 6fea582..d015cf2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -61,6 +61,7 @@ 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_LP8841_RTC) += spi-lp8841-rtc.o
+obj-$(CONFIG_SPI_LS7A) += spi-ls7a.o
obj-$(CONFIG_SPI_MESON_SPICC) += spi-meson-spicc.o
obj-$(CONFIG_SPI_MESON_SPIFC) += spi-meson-spifc.o
obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o
diff --git a/drivers/spi/spi-ls7a.c b/drivers/spi/spi-ls7a.c
new file mode 100644
index 0000000..21ca1ab
--- /dev/null
+++ b/drivers/spi/spi-ls7a.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Loongson LS7A SPI Controller driver
+ *
+ * Copyright (C) 2020 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/of.h>
+
+/* define spi register */
+#define SPCR 0x00
+#define SPSR 0x01
+#define FIFO 0x02
+#define SPER 0x03
+#define PARA 0x04
+#define SFCS 0x05
+#define TIMI 0x06
+
+struct ls7a_spi {
+ spinlock_t lock;
+ struct spi_master *master;
+ void __iomem *base;
+ int cs_active;
+ unsigned int hz;
+ unsigned char spcr, sper;
+ unsigned int mode;
+};
+
+static void ls7a_spi_write_reg(struct ls7a_spi *spi,
+ unsigned char reg,
+ unsigned char data)
+{
+ writeb(data, spi->base + reg);
+}
+
+static char ls7a_spi_read_reg(struct ls7a_spi *spi,
+ unsigned char reg)
+{
+ return readb(spi->base + reg);
+}
+
+static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val)
+{
+ int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
+
+ if (spi->mode & SPI_CS_HIGH)
+ val = !val;
+ ls7a_spi_write_reg(ls7a_spi, SFCS,
+ (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
+
+ return 0;
+}
+
+static int ls7a_spi_do_transfer(struct ls7a_spi *ls7a_spi,
+ struct spi_device *spi,
+ struct spi_transfer *t)
+{
+ unsigned int hz;
+ unsigned int div, div_tmp;
+ unsigned int bit;
+ unsigned long clk;
+ unsigned char val;
+ const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
+
+ if (t) {
+ hz = t->speed_hz;
+ if (!hz)
+ hz = spi->max_speed_hz;
+ } else
+ hz = spi->max_speed_hz;
+
+ if (((spi->mode ^ ls7a_spi->mode) & (SPI_CPOL | SPI_CPHA))
+ || (hz && ls7a_spi->hz != hz)) {
+ clk = 100000000;
+
+ div = DIV_ROUND_UP(clk, 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 = %ld hz = %d div_tmp = %d bit = %d\n",
+ clk, hz, div_tmp, bit);
+
+ ls7a_spi->hz = hz;
+ ls7a_spi->spcr = div_tmp & 3;
+ ls7a_spi->sper = (div_tmp >> 2) & 3;
+
+ val = ls7a_spi_read_reg(ls7a_spi, SPCR);
+ val &= ~0xc;
+ if (spi->mode & SPI_CPOL)
+ val |= 8;
+ if (spi->mode & SPI_CPHA)
+ val |= 4;
+ ls7a_spi_write_reg(ls7a_spi, SPCR, (val & ~3) | ls7a_spi->spcr);
+ val = ls7a_spi_read_reg(ls7a_spi, SPER);
+ ls7a_spi_write_reg(ls7a_spi, SPER, (val & ~3) | ls7a_spi->sper);
+ ls7a_spi->mode = spi->mode;
+ }
+ return 0;
+}
+
+static int ls7a_spi_setup(struct spi_device *spi)
+{
+ struct ls7a_spi *ls7a_spi;
+
+ ls7a_spi = spi_master_get_devdata(spi->master);
+
+ if (spi->chip_select >= spi->master->num_chipselect)
+ return -EINVAL;
+
+ set_cs(ls7a_spi, spi, 1);
+
+ return 0;
+}
+
+static int ls7a_spi_write_read_8bit(struct spi_device *spi,
+ const u8 **tx_buf, u8 **rx_buf,
+ unsigned int num)
+{
+ struct ls7a_spi *ls7a_spi;
+
+ ls7a_spi = spi_master_get_devdata(spi->master);
+
+ if (tx_buf && *tx_buf) {
+ ls7a_spi_write_reg(ls7a_spi, FIFO, *((*tx_buf)++));
+
+ while ((ls7a_spi_read_reg(ls7a_spi, SPSR) & 0x1) == 1)
+ ;
+ } else {
+ ls7a_spi_write_reg(ls7a_spi, FIFO, 0);
+
+ while ((ls7a_spi_read_reg(ls7a_spi, SPSR) & 0x1) == 1)
+ ;
+ }
+
+ if (rx_buf && *rx_buf)
+ *(*rx_buf)++ = ls7a_spi_read_reg(ls7a_spi, FIFO);
+ else
+ ls7a_spi_read_reg(ls7a_spi, FIFO);
+
+ return 1;
+}
+
+static unsigned int ls7a_spi_write_read(struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ struct ls7a_spi *ls7a_spi;
+ unsigned int count;
+ const u8 *tx = xfer->tx_buf;
+ u8 *rx = xfer->rx_buf;
+
+ ls7a_spi = spi_master_get_devdata(spi->master);
+ count = xfer->len;
+
+ do {
+ if (ls7a_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
+ goto out;
+ count--;
+ } while (count);
+
+out:
+ return xfer->len - count;
+}
+
+static int ls7a_spi_transfer_one_message(struct spi_master *master,
+ struct spi_message *m)
+{
+ struct ls7a_spi *ls7a_spi;
+ struct spi_device *spi;
+ struct spi_transfer *t = NULL;
+ int param, status, r;
+ unsigned int total_len = 0;
+ unsigned int cs_change;
+ const int nsecs = 50;
+
+ ls7a_spi = spi_master_get_devdata(master);
+ spi = m->spi;
+
+ cs_change = 1;
+
+ spin_lock(&ls7a_spi->lock);
+ param = ls7a_spi_read_reg(ls7a_spi, PARA);
+ ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
+ spin_unlock(&ls7a_spi->lock);
+ list_for_each_entry(t, &m->transfers, transfer_list) {
+ if (cs_change) {
+ set_cs(ls7a_spi, spi, 0);
+ ls7a_spi_do_transfer(ls7a_spi, spi, t);
+ ndelay(nsecs);
+ }
+ cs_change = t->cs_change;
+
+ r = ls7a_spi_write_read(spi, t);
+ if (r < 0) {
+ status = r;
+ goto error;
+ }
+ total_len += r;
+
+ spi_transfer_delay_exec(t);
+
+ if (cs_change) {
+ set_cs(ls7a_spi, spi, 1);
+ ndelay(nsecs);
+ }
+ }
+
+ spin_lock(&ls7a_spi->lock);
+ ls7a_spi_write_reg(ls7a_spi, PARA, param);
+ spin_unlock(&ls7a_spi->lock);
+
+ if (!cs_change) {
+ ndelay(nsecs);
+ set_cs(ls7a_spi, spi, 1);
+ }
+
+error:
+ m->status = status;
+ m->actual_length = total_len;
+ spi_finalize_current_message(master);
+ return status;
+}
+
+static int ls7a_spi_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ struct device *dev = &pdev->dev;
+ struct spi_master *master;
+ struct ls7a_spi *spi;
+ int ret;
+
+ master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
+ if (!master)
+ return -ENOMEM;
+
+ spi = spi_master_get_devdata(master);
+ ret = pcim_enable_device(pdev);
+ if (ret)
+ goto err_free_master;
+
+ ret = pci_request_regions(pdev, "ls7a-spi");
+ if (ret)
+ goto err_free_master;
+
+ spi->base = pcim_iomap(pdev, 0, pci_resource_len(pdev, 0));
+ if (!spi->base) {
+ ret = -EINVAL;
+ goto err_free_master;
+ }
+ ls7a_spi_write_reg(spi, SPCR, 0x51);
+ ls7a_spi_write_reg(spi, SPER, 0x00);
+ ls7a_spi_write_reg(spi, TIMI, 0x01);
+ ls7a_spi_write_reg(spi, PARA, 0x40);
+ spi->mode = 0;
+
+ master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+ master->setup = ls7a_spi_setup;
+ master->transfer_one_message = ls7a_spi_transfer_one_message;
+ master->bits_per_word_mask = SPI_BPW_MASK(8);
+ master->num_chipselect = 4;
+ master->dev.of_node = of_node_get(pdev->dev.of_node);
+
+ spi->master = master;
+
+ pci_set_drvdata(pdev, master);
+
+ ret = devm_spi_register_master(dev, master);
+ if (ret)
+ goto err_free_master;
+
+ return 0;
+
+err_free_master:
+ pci_release_regions(pdev);
+ spi_master_put(master);
+ return ret;
+}
+
+static void ls7a_spi_pci_remove(struct pci_dev *pdev)
+{
+ struct spi_master *master = pci_get_drvdata(pdev);
+ struct ls7a_spi *spi;
+
+ spi = spi_master_get_devdata(master);
+ if (!spi)
+ return;
+
+ pci_release_regions(pdev);
+}
+
+static const struct pci_device_id ls7a_spi_pci_id_table[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) },
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, ls7a_spi_pci_id_table);
+
+static struct pci_driver ls7a_spi_pci_driver = {
+ .name = "ls7a-spi",
+ .id_table = ls7a_spi_pci_id_table,
+ .probe = ls7a_spi_pci_probe,
+ .remove = ls7a_spi_pci_remove,
+};
+
+module_pci_driver(ls7a_spi_pci_driver);
+
+MODULE_AUTHOR("Juxin Gao <[email protected]>");
+MODULE_AUTHOR("Qing Zhang <[email protected]>");
+MODULE_DESCRIPTION("Loongson LS7A SPI controller driver");
--
2.1.0
Add spi-ls7a binding documentation.
Signed-off-by: Qing Zhang <[email protected]>
---
Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
new file mode 100644
index 0000000..56247b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
@@ -0,0 +1,31 @@
+Binding for LOONGSON LS7A SPI controller
+
+Required properties:
+- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
+- reg: reference IEEE Std 1275-1994.
+- #address-cells: <1>, as required by generic SPI binding.
+- #size-cells: <0>, also as required by generic SPI binding.
+- #interrupts: No hardware interrupt.
+
+Child nodes as per the generic SPI binding.
+
+Example:
+
+ spi@16,0 {
+ compatible = "pci0014,7a0b.0",
+ "pci0014,7a0b",
+ "pciclass088000",
+ "pciclass0880";
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0xb000 0x0 0x0 0x0 0x0>;
+ num-chipselects = <0>;
+ spiflash: s25fl016k@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible ="spansion,s25fl016k","jedec,spi-nor";
+ spi-max-frequency=<50000000>;
+ reg=<0>;
+ };
+ };
--
2.1.0
Hello!
On 08.12.2020 10:44, Qing Zhang wrote:
> Add spi-ls7a binding documentation.
>
> Signed-off-by: Qing Zhang <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> new file mode 100644
> index 0000000..56247b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> @@ -0,0 +1,31 @@
> +Binding for LOONGSON LS7A SPI controller
> +
> +Required properties:
> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
> +- reg: reference IEEE Std 1275-1994.
> +- #address-cells: <1>, as required by generic SPI binding.
> +- #size-cells: <0>, also as required by generic SPI binding.
> +- #interrupts: No hardware interrupt.
You say it's a required prop, yet yuoe example doesn't have it...
> +
> +Child nodes as per the generic SPI binding.
> +
> +Example:
> +
> + spi@16,0 {
> + compatible = "pci0014,7a0b.0",
> + "pci0014,7a0b",
> + "pciclass088000",
> + "pciclass0880";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0xb000 0x0 0x0 0x0 0x0>;
> + num-chipselects = <0>;
> + spiflash: s25fl016k@0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
Once more?
> + compatible ="spansion,s25fl016k","jedec,spi-nor";
Once more?
> + spi-max-frequency=<50000000>;
> + reg=<0>;
Once more? Did you mean this for a child node?
> + };
> + };
MBR, Sergei
Hi Sergei Shtylyov,
On 12/08/2020 04:40 PM, Sergei Shtylyov wrote:
> Hello!
>
> On 08.12.2020 10:44, Qing Zhang wrote:
>
>> Add spi-ls7a binding documentation.
>>
>> Signed-off-by: Qing Zhang <[email protected]>
>> ---
>> Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31
>> ++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>> b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>> new file mode 100644
>> index 0000000..56247b5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>> @@ -0,0 +1,31 @@
>> +Binding for LOONGSON LS7A SPI controller
>> +
>> +Required properties:
>> +- compatible: should be
>> "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
>> +- reg: reference IEEE Std 1275-1994.
>> +- #address-cells: <1>, as required by generic SPI binding.
>> +- #size-cells: <0>, also as required by generic SPI binding.
>> +- #interrupts: No hardware interrupt.
>
> You say it's a required prop, yet yuoe example doesn't have it...
I want to emphasize here that LS7A SPI has no hardware
interrupts, and DT is not actually used.
>
>> +
>> +Child nodes as per the generic SPI binding.
>> +
>> +Example:
>> +
>> + spi@16,0 {
>> + compatible = "pci0014,7a0b.0",
>> + "pci0014,7a0b",
>> + "pciclass088000",
>> + "pciclass0880";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <0xb000 0x0 0x0 0x0 0x0>;
>> + num-chipselects = <0>;
>> + spiflash: s25fl016k@0 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>
> Once more?
>
>> + compatible ="spansion,s25fl016k","jedec,spi-nor";
>
> Once more?
>
>> + spi-max-frequency=<50000000>;
>> + reg=<0>;
>
> Once more? Did you mean this for a child node?
Yes, these are child node attributes, the child node splash is
not necessary.
>
>> + };
>> + };
>
Thanks
-Qing
> MBR, Sergei
Hi Qing,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on spi/for-next]
[also build test WARNING on robh/for-next linus/master v5.10-rc7 next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
>> drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
162 | struct ls7a_spi *ls7a_spi;
| ^~~~~~~~
vim +/ls7a_spi +162 drivers/spi/spi-ls7a.c
158
159 static unsigned int ls7a_spi_write_read(struct spi_device *spi,
160 struct spi_transfer *xfer)
161 {
> 162 struct ls7a_spi *ls7a_spi;
163 unsigned int count;
164 const u8 *tx = xfer->tx_buf;
165 u8 *rx = xfer->rx_buf;
166
167 ls7a_spi = spi_master_get_devdata(spi->master);
168 count = xfer->len;
169
170 do {
171 if (ls7a_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
172 goto out;
173 count--;
174 } while (count);
175
176 out:
177 return xfer->len - count;
178 }
179
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote:
> v2:
> - keep Kconfig and Makefile sorted
> - make the entire comment a C++ one so things look more intentional
You say this but...
> +++ b/drivers/spi/spi-ls7a.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Loongson LS7A SPI Controller driver
> + *
> + * Copyright (C) 2020 Loongson Technology Corporation Limited
> + */
...this is still a mix of C and C++ comments?
> +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val)
> +{
> + int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
> +
> + if (spi->mode & SPI_CS_HIGH)
> + val = !val;
> + ls7a_spi_write_reg(ls7a_spi, SFCS,
> + (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
> +
> + return 0;
> +}
Why not just expose this to the core and let it handle things?
Please also write normal conditional statements to improve legibility.
There's quite a lot of coding style issues in this with things like
missing spaces
> + if (t) {
> + hz = t->speed_hz;
> + if (!hz)
> + hz = spi->max_speed_hz;
> + } else
> + hz = spi->max_speed_hz;
If one branch of the conditional has braces please use them on both to
improve legibility.
> +static int ls7a_spi_transfer_one_message(struct spi_master *master,
> + struct spi_message *m)
I don't understand why the driver is implementing transfer_one_message()
- it looks like this is just open coding the standard loop that the
framework provides and should just be using transfer_one().
> + r = ls7a_spi_write_read(spi, t);
> + if (r < 0) {
> + status = r;
> + goto error;
> + }
The indentation here isn't following the kernel coding style.
> + master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
> + if (!master)
> + return -ENOMEM;
Why not use devm_ here?
> + ret = devm_spi_register_master(dev, master);
> + if (ret)
> + goto err_free_master;
The driver uses devm_spi_register_master() here but...
> +static void ls7a_spi_pci_remove(struct pci_dev *pdev)
> +{
> + struct spi_master *master = pci_get_drvdata(pdev);
> + struct ls7a_spi *spi;
> +
> + spi = spi_master_get_devdata(master);
> + if (!spi)
> + return;
> +
> + pci_release_regions(pdev);
...releases the PCI regions in the remove() function before the SPI
controller is freed so the controller could still be active.
On Tue, Dec 08, 2020 at 06:47:10PM +0800, zhangqing wrote:
> On 12/08/2020 04:40 PM, Sergei Shtylyov wrote:
> > > +Required properties:
> > > +- #interrupts: No hardware interrupt.
> > You say it's a required prop, yet yuoe example doesn't have it...
> I want to emphasize here that LS7A SPI has no hardware interrupts,
> and DT is not actually used.
There is no need to do this, and documenting the property as required
just makes things confusing here.
On 12/8/20 1:47 PM, zhangqing wrote:
>>> Add spi-ls7a binding documentation.
>>>
>>> Signed-off-by: Qing Zhang <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
>>> 1 file changed, 31 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>> new file mode 100644
>>> index 0000000..56247b5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>> @@ -0,0 +1,31 @@
>>> +Binding for LOONGSON LS7A SPI controller
>>> +
>>> +Required properties:
>>> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
>>> +- reg: reference IEEE Std 1275-1994.
>>> +- #address-cells: <1>, as required by generic SPI binding.
>>> +- #size-cells: <0>, also as required by generic SPI binding.
>>> +- #interrupts: No hardware interrupt.
>>
>> You say it's a required prop, yet yuoe example doesn't have it...
> I want to emphasize here that LS7A SPI has no hardware interrupts, and DT is not actually used.
The why document the property at all?
>>> +
>>> +Child nodes as per the generic SPI binding.
>>> +
>>> +Example:
>>> +
>>> + spi@16,0 {
>>> + compatible = "pci0014,7a0b.0",
>>> + "pci0014,7a0b",
>>> + "pciclass088000",
>>> + "pciclass0880";
>>> +
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + reg = <0xb000 0x0 0x0 0x0 0x0>;
>>> + num-chipselects = <0>;
>>> + spiflash: s25fl016k@0 {
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>
>> Once more?
>>
>>> + compatible ="spansion,s25fl016k","jedec,spi-nor";
>>
>> Once more?
>>
>>> + spi-max-frequency=<50000000>;
>>> + reg=<0>;
>>
>> Once more? Did you mean this for a child node?
> Yes, these are child node attributes, the child node splash is not necessary.
You should indent the child nodes with 1 more tab...
>>
>>> + };
>>> + };
>>
> Thanks
>
> -Qing
MBR, Sergei
?? 2020/12/8 15:44, Qing Zhang д??:
> Add spi-ls7a binding documentation.
>
> Signed-off-by: Qing Zhang <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
Hi Qing,
Please use dt schema instead.
Thanks.
- Jiaxun
> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> new file mode 100644
> index 0000000..56247b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> @@ -0,0 +1,31 @@
> +Binding for LOONGSON LS7A SPI controller
> +
> +Required properties:
> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
> +- reg: reference IEEE Std 1275-1994.
> +- #address-cells: <1>, as required by generic SPI binding.
> +- #size-cells: <0>, also as required by generic SPI binding.
> +- #interrupts: No hardware interrupt.
> +
> +Child nodes as per the generic SPI binding.
> +
> +Example:
> +
> + spi@16,0 {
> + compatible = "pci0014,7a0b.0",
> + "pci0014,7a0b",
> + "pciclass088000",
> + "pciclass0880";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0xb000 0x0 0x0 0x0 0x0>;
> + num-chipselects = <0>;
> + spiflash: s25fl016k@0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible ="spansion,s25fl016k","jedec,spi-nor";
> + spi-max-frequency=<50000000>;
> + reg=<0>;
> + };
> + };
Hi Qing,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on spi/for-next]
[also build test ERROR on robh/for-next linus/master v5.10-rc7 next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
162 | struct ls7a_spi *ls7a_spi;
| ^~~~~~~~
drivers/spi/spi-ls7a.c: At top level:
>> drivers/spi/spi-ls7a.c:320:1: warning: data definition has no type or storage class
320 | module_pci_driver(ls7a_spi_pci_driver);
| ^~~~~~~~~~~~~~~~~
>> drivers/spi/spi-ls7a.c:320:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-ls7a.c:320:1: warning: parameter names (without types) in function declaration
drivers/spi/spi-ls7a.c:313:26: warning: 'ls7a_spi_pci_driver' defined but not used [-Wunused-variable]
313 | static struct pci_driver ls7a_spi_pci_driver = {
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +320 drivers/spi/spi-ls7a.c
319
> 320 module_pci_driver(ls7a_spi_pci_driver);
321
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 12/08/2020 10:48 PM, Sergei Shtylyov wrote:
> On 12/8/20 1:47 PM, zhangqing wrote:
>
>>>> Add spi-ls7a binding documentation.
>>>>
>>>> Signed-off-by: Qing Zhang <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
>>>> 1 file changed, 31 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>> new file mode 100644
>>>> index 0000000..56247b5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>> @@ -0,0 +1,31 @@
>>>> +Binding for LOONGSON LS7A SPI controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
>>>> +- reg: reference IEEE Std 1275-1994.
>>>> +- #address-cells: <1>, as required by generic SPI binding.
>>>> +- #size-cells: <0>, also as required by generic SPI binding.
>>>> +- #interrupts: No hardware interrupt.
>>> You say it's a required prop, yet yuoe example doesn't have it...
>> I want to emphasize here that LS7A SPI has no hardware interrupts, and DT is not actually used.
> The why document the property at all?
Thank you for your reply again,
I will remove the #interrupt attribute in the third edition.
>
>>>> +
>>>> +Child nodes as per the generic SPI binding.
>>>> +
>>>> +Example:
>>>> +
>>>> + spi@16,0 {
>>>> + compatible = "pci0014,7a0b.0",
>>>> + "pci0014,7a0b",
>>>> + "pciclass088000",
>>>> + "pciclass0880";
>>>> +
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + reg = <0xb000 0x0 0x0 0x0 0x0>;
>>>> + num-chipselects = <0>;
>>>> + spiflash: s25fl016k@0 {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <1>;
>>> Once more?
>>>
>>>> + compatible ="spansion,s25fl016k","jedec,spi-nor";
>>> Once more?
>>>
>>>> + spi-max-frequency=<50000000>;
>>>> + reg=<0>;
>>> Once more? Did you mean this for a child node?
>> Yes, these are child node attributes, the child node splash is not necessary.
> You should indent the child nodes with 1 more tab...
I will do it and send the v3 in the soon.
>
>>>> + };
>>>> + };
>> Thanks
>>
>> -Qing
> MBR, Sergei
On 12/08/2020 09:58 PM, Mark Brown wrote:
> On Tue, Dec 08, 2020 at 06:47:10PM +0800, zhangqing wrote:
>> On 12/08/2020 04:40 PM, Sergei Shtylyov wrote:
>>>> +Required properties:
>>>> +- #interrupts: No hardware interrupt.
>>> You say it's a required prop, yet yuoe example doesn't have it...
>> I want to emphasize here that LS7A SPI has no hardware interrupts,
>> and DT is not actually used.
> There is no need to do this, and documenting the property as required
> just makes things confusing here.
Thanks for your suggestion,
I will remove the #interrupt attribute in the third edition.
Thanks,
-Qing
>
>
Hi Brown,
Thank you for your suggestions, these are achievable, I will send v3 in
the soon.
Before sending v3, I would like to trouble you to see if this is
correct. It has been tested locally.
On 12/08/2020 09:56 PM, Mark Brown wrote:
> On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote:
>
>> v2:
>> - keep Kconfig and Makefile sorted
>> - make the entire comment a C++ one so things look more intentional
> You say this but...
>
>> +++ b/drivers/spi/spi-ls7a.c
>> @@ -0,0 +1,324 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Loongson LS7A SPI Controller driver
>> + *
>> + * Copyright (C) 2020 Loongson Technology Corporation Limited
>> + */
> ...this is still a mix of C and C++ comments?
Replace all with //
>
>> +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val)
>> +{
>> + int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
>> +
>> + if (spi->mode & SPI_CS_HIGH)
>> + val = !val;
>> + ls7a_spi_write_reg(ls7a_spi, SFCS,
>> + (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
>> +
>> + return 0;
>> +}
> Why not just expose this to the core and let it handle things?
>
> Please also write normal conditional statements to improve legibility.
> There's quite a lot of coding style issues in this with things like
> missing spaces
static void ls7a_spi_set_cs(struct spi_device *spi, bool enable)
{
struct ls7a_spi *ls7a_spi;
int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 <<
spi->chip_select));
ls7a_spi = spi_master_get_devdata(spi->master);
if (!!(spi->mode & SPI_CS_HIGH) == enable)
val = (0x11 << spi->chip_select) | cs;
else
val = (0x1 << spi->chip_select) | cs;
ls7a_spi_write_reg(ls7a_spi, SFCS, val);
}
static int ls7a_spi_pci_probe---->
+master->set_cs = ls7a_spi_set_cs;
>
>> + if (t) {
>> + hz = t->speed_hz;
>> + if (!hz)
>> + hz = spi->max_speed_hz;
>> + } else
>> + hz = spi->max_speed_hz;
> If one branch of the conditional has braces please use them on both to
> improve legibility.
>
>> +static int ls7a_spi_transfer_one_message(struct spi_master *master,
>> + struct spi_message *m)
> I don't understand why the driver is implementing transfer_one_message()
> - it looks like this is just open coding the standard loop that the
> framework provides and should just be using transfer_one().
static int ls7a_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *t)
{
struct ls7a_spi *ls7a_spi;
int param, status;
ls7a_spi = spi_master_get_devdata(master);
spin_lock(&ls7a_spi->lock);
param = ls7a_spi_read_reg(ls7a_spi, PARA);
ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
spin_unlock(&ls7a_spi->lock);
status = ls7a_spi_do_transfer(ls7a_spi, spi, t);
if(status < 0)
return status;
if(t->len)
r = ls7a_spi_write_read(spi, t);
spin_lock(&ls7a_spi->lock);
ls7a_spi_write_reg(ls7a_spi, PARA, param);
spin_unlock(&ls7a_spi->lock);
return status;
}
static int ls7a_spi_pci_probe---->
- master->transfer_one_message = ls7a_spi_transfer_one_message;
+master->transfer_one = ls7a_spi_transfer_one;
>
>> + r = ls7a_spi_write_read(spi, t);
>> + if (r < 0) {
>> + status = r;
>> + goto error;
>> + }
> The indentation here isn't following the kernel coding style.
>
>> + master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
>> + if (!master)
>> + return -ENOMEM;
> Why not use devm_ here?
- master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
error:
- spi_put_master(master);
+ master = devm_spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
>
>> + ret = devm_spi_register_master(dev, master);
>> + if (ret)
>> + goto err_free_master;
> The driver uses devm_spi_register_master() here but...
>
>> +static void ls7a_spi_pci_remove(struct pci_dev *pdev)
>> +{
>> + struct spi_master *master = pci_get_drvdata(pdev);
>> + struct ls7a_spi *spi;
>> +
>> + spi = spi_master_get_devdata(master);
>> + if (!spi)
>> + return;
>> +
>> + pci_release_regions(pdev);
> ...releases the PCI regions in the remove() function before the SPI
> controller is freed so the controller could still be active.
static void ls7a_spi_pci_remove(struct pci_dev *pdev)
{
struct spi_master *master = pci_get_drvdata(pdev);
+ spi_unregister_master(master);
pci_release_regions(pdev);
}
Thanks,
-Qing
On Wed, Dec 09, 2020 at 03:24:15PM +0800, zhangqing wrote:
> > > +static int ls7a_spi_transfer_one_message(struct spi_master *master,
> > > + struct spi_message *m)
> > I don't understand why the driver is implementing transfer_one_message()
> > - it looks like this is just open coding the standard loop that the
> > framework provides and should just be using transfer_one().
> static int ls7a_spi_transfer_one(struct spi_master *master,
> struct spi_device *spi,
> struct spi_transfer *t)
> {
> struct ls7a_spi *ls7a_spi;
> int param, status;
>
> ls7a_spi = spi_master_get_devdata(master);
>
> spin_lock(&ls7a_spi->lock);
> param = ls7a_spi_read_reg(ls7a_spi, PARA);
> ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
> spin_unlock(&ls7a_spi->lock);
I don't know what this does but is it better split out into a
prepare_message()? It was only done once per message in your previous
implementation. Or possibly runtime PM would be even better if that's
what it's doing.
> > ...releases the PCI regions in the remove() function before the SPI
> > controller is freed so the controller could still be active.
>
> static void ls7a_spi_pci_remove(struct pci_dev *pdev)
> {
> struct spi_master *master = pci_get_drvdata(pdev);
>
> + spi_unregister_master(master);
> pci_release_regions(pdev);
> }
You also need to change to using plain spi_register_master() but yes.
Otherwise everything looked good.