Message ID | 1607413467-17698-1-git-send-email-zhangqing@loongson.cn |
---|---|
State | New |
Headers | show |
Series | [v2,1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support | expand |
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 12/8/20 1:47 PM, zhangqing wrote: >>> Add spi-ls7a binding documentation. >>> >>> Signed-off-by: Qing Zhang <zhangqing@loongson.cn> >>> --- >>> 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
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 <lkp@intel.com> 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/kbuild-all@lists.01.org
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 <zhangqing@loongson.cn> >>>> --- >>>> 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 > >
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.
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 <gaojuxin@loongson.cn>"); +MODULE_AUTHOR("Qing Zhang <zhangqing@loongson.cn>"); +MODULE_DESCRIPTION("Loongson LS7A SPI controller driver");