[v11,2/2] spi: loongson: add bus driver for the loongson spi controller

Message ID 20230522071030.5193-3-zhuyinbo@loongson.cn
State New
Headers
Series spi: loongson: add bus driver for the loongson spi |

Commit Message

Yinbo Zhu May 22, 2023, 7:10 a.m. UTC
  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                     |   4 +
 drivers/spi/Kconfig             |  26 +++
 drivers/spi/Makefile            |   3 +
 drivers/spi/spi-loongson-core.c | 279 ++++++++++++++++++++++++++++++++
 drivers/spi/spi-loongson-pci.c  |  61 +++++++
 drivers/spi/spi-loongson-plat.c |  46 ++++++
 drivers/spi/spi-loongson.h      |  47 ++++++
 7 files changed, 466 insertions(+)
 create mode 100644 drivers/spi/spi-loongson-core.c
 create mode 100644 drivers/spi/spi-loongson-pci.c
 create mode 100644 drivers/spi/spi-loongson-plat.c
 create mode 100644 drivers/spi/spi-loongson.h
  

Comments

Andy Shevchenko May 23, 2023, 12:54 p.m. UTC | #1
Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:
> 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.

It's polite to add reviewers of the previous versions to the Cc list.

...

> +static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz)
> +{
> +	unsigned char val;
> +	unsigned int div, div_tmp;

> +	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};

static?

> +
> +	div = clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
> +	div_tmp = rdiv[fls(div - 1)];
> +	loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
> +	loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;
> +	val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
> +	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->hz = hz;
> +}

...

> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
> +				struct spi_device *spi, struct spi_transfer *t)
> +{
> +	unsigned int hz;
> +
> +	if (t)
> +		hz = t->speed_hz;

And if t is NULL? hz will be uninitialized. Don't you get a compiler warning?
(Always test your code with `make W=1 ...`)

> +	if (hz && loongson_spi->hz != hz)
> +		loongson_spi_set_clk(loongson_spi, hz);
> +
> +	if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
> +		loongson_spi_set_mode(loongson_spi, spi);
> +
> +	return 0;
> +}

...

> +	readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
> +			   (loongson_spi->spsr & 0x1) != 1, 1, MSEC_PER_SEC);

Wouldn't be better to use ' == 0' in the conditional? Or if you think your
approach is better (to show the exact expectation) the definition of the bit 0
might help

#define LOONGSON_... BIT(0)


	readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
			   (loongson_spi->spsr & LOONGSON_...) != LOONGSON_...,
			   1, MSEC_PER_SEC);

...

> +	do {
> +		if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)

> +			goto out;

		break;

> +		count--;
> +	} while (count);

	} while (--count);

?

> +out:
> +	return xfer->len - count;

Shouldn't you return an error code if the write failed?

...

> +	master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));

> +	if (master == NULL)

	if (!master)

> +		return -ENOMEM;

Why do you use deprecated naming? Can you use spi_controller* instead of
spi_master* in all cases?

...

> +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;

	= SPI_MODE_X_MASK | SPI_CS_HIGH;

...

> +	clk = devm_clk_get_optional(dev, NULL);
> +	if (!IS_ERR(clk))
> +		spi->clk_rate = clk_get_rate(clk);

> +	else

Redundant. Just check for the error first as it's very usual pattern in the
Linux kernel.

> +		return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");

...

> +static void loongson_spi_pci_unregister(struct pci_dev *pdev)
> +{

> +	pcim_iounmap_regions(pdev, BIT(0));

Not needed due to 'm' in the API name, which means "managed".

> +	pci_disable_device(pdev);

This is simply wrong. We don't do explicit clean up for managed resources.

> +}

That said, drop the ->remove() completely.

...

> +static struct pci_device_id loongson_spi_devices[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a1b) },
> +	{ },

No comma for the terminator entry. It's by definition "terminating" something.

> +};

...

> +#include <linux/of.h>

There is no user of this header. Please, replace with what actually is being
used (presumably mod_devicetable.h and maybe others).

> +#include <linux/platform_device.h>
> +
> +#include "spi-loongson.h"
> +
> +static int loongson_spi_platform_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	void __iomem *reg_base;
> +	struct device *dev = &pdev->dev;
> +
> +	reg_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(reg_base))
> +		return PTR_ERR(reg_base);
> +
> +	ret = loongson_spi_init_master(dev, reg_base);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize master\n");
> +
> +	return ret;

	return 0;

> +}

...

> +#ifndef __LINUX_SPI_LOONGSON_H
> +#define __LINUX_SPI_LOONGSON_H
> +
> +#include <linux/bits.h>

> +#include <linux/device.h>

This header is not used.

> +#include <linux/pm.h>

> +#include <linux/spi/spi.h>

This neither.

> +#include <linux/types.h>


For them use forward declarations

struct device;
struct spi_controller;

The rest of the inclusions is correct.

...

> +#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

Where is this used outside of the main driver?

> +/* Bits definition for Loongson SPI register */
> +#define	LOONGSON_SPI_PARA_MEM_EN	BIT(0)
> +#define	LOONGSON_SPI_SPCR_CPHA	BIT(2)
> +#define	LOONGSON_SPI_SPCR_CPOL	BIT(3)
> +#define	LOONGSON_SPI_SPCR_SPE	BIT(6)
> +#define	LOONGSON_SPI_SPSR_WCOL	BIT(6)
> +#define	LOONGSON_SPI_SPSR_SPIF	BIT(7)
> +
> +struct loongson_spi {
> +	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;
> +	unsigned int		mode;
> +	u64			clk_rate;
> +};
> +
> +int loongson_spi_init_master(struct device *dev, void __iomem *reg);
> +extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
> +#endif /* __LINUX_SPI_LOONGSON_H */
  
Yinbo Zhu May 24, 2023, 7:52 a.m. UTC | #2
在 2023/5/23 下午8:54, andy.shevchenko@gmail.com 写道:
> Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:
>> 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.
> 
> It's polite to add reviewers of the previous versions to the Cc list.

okay, I got it.
> 
> ...
> 
>> +static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz)
>> +{
>> +	unsigned char val;
>> +	unsigned int div, div_tmp;
> 
>> +	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
> 
> static?

okay, I will define "static const char rdiv".

> 
>> +
>> +	div = clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
>> +	div_tmp = rdiv[fls(div - 1)];
>> +	loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
>> +	loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;
>> +	val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
>> +	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->hz = hz;
>> +}
> 
> ...
> 
>> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
>> +				struct spi_device *spi, struct spi_transfer *t)
>> +{
>> +	unsigned int hz;
>> +
>> +	if (t)
>> +		hz = t->speed_hz;
> 
> And if t is NULL? hz will be uninitialized. Don't you get a compiler warning?
> (Always test your code with `make W=1 ...`)

I alwasy use `make W=1` and I don't find any warnig, but that what you
said was right and I will initial hz.

> 
>> +	if (hz && loongson_spi->hz != hz)
>> +		loongson_spi_set_clk(loongson_spi, hz);
>> +
>> +	if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
>> +		loongson_spi_set_mode(loongson_spi, spi);
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +	readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
>> +			   (loongson_spi->spsr & 0x1) != 1, 1, MSEC_PER_SEC);
> 
> Wouldn't be better to use ' == 0' in the conditional? Or if you think your
> approach is better (to show the exact expectation) the definition of the bit 0
> might help
> 
> #define LOONGSON_... BIT(0)

okay, I got it.
> 
> 
> 	readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
> 			   (loongson_spi->spsr & LOONGSON_...) != LOONGSON_...,
> 			   1, MSEC_PER_SEC);
> 
> ...
> 
>> +	do {
>> +		if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
> 
>> +			goto out;
> 
> 		break;
> 
>> +		count--;
>> +	} while (count);
> 
> 	} while (--count);
> 
> ?

okay, I got it.

> 
>> +out:
>> +	return xfer->len - count;
> 
> Shouldn't you return an error code if the write failed?

okay, I got it. I will add a error code for return when write failed.

> 
> ...
> 
>> +	master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
> 
>> +	if (master == NULL)
> 
> 	if (!master)
> 
>> +		return -ENOMEM;
> 
> Why do you use deprecated naming? Can you use spi_controller* instead of
> spi_master* in all cases?


It seems was a personal code style issue and I don't find the
differences between spi_controller and spi_master, Using spi_controller*
is also acceptable to me and I will use spi_controller* instead of
spi_master* in all cases.


> 
> ...
> 
>> +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
> 
> 	= SPI_MODE_X_MASK | SPI_CS_HIGH;
> 
> ...
> 
>> +	clk = devm_clk_get_optional(dev, NULL);
>> +	if (!IS_ERR(clk))
>> +		spi->clk_rate = clk_get_rate(clk);
> 
>> +	else
> 
> Redundant. Just check for the error first as it's very usual pattern in the
> Linux kernel.

Like below ?

         clk = devm_clk_get_optional(dev, NULL);
-       if (!IS_ERR(clk))
-               spi->clk_rate = clk_get_rate(clk);
-       else
+       if (IS_ERR(clk))
                 return dev_err_probe(dev, PTR_ERR(clk), "unable to get 
clock\n");

+       spi->clk_rate = clk_get_rate(clk);
         loongson_spi_reginit(spi);


> 
>> +		return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");
> 
> ...
> 
>> +static void loongson_spi_pci_unregister(struct pci_dev *pdev)
>> +{
> 
>> +	pcim_iounmap_regions(pdev, BIT(0));
> 
> Not needed due to 'm' in the API name, which means "managed".

> 
>> +	pci_disable_device(pdev);
> 
> This is simply wrong. We don't do explicit clean up for managed resources.
> 
>> +}
> 
> That said, drop the ->remove() completely.

okay,  I will drop the ->remove() completely.
> 
> ...
> 
>> +static struct pci_device_id loongson_spi_devices[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a1b) },
>> +	{ },
> 
> No comma for the terminator entry. It's by definition "terminating" something.

okay, I got it.
> 
>> +};
> 
> ...
> 
>> +#include <linux/of.h>
> 
> There is no user of this header. Please, replace with what actually is being
> used (presumably mod_devicetable.h and maybe others).
> 

okay, I got it.

>> +#include <linux/platform_device.h>
>> +
>> +#include "spi-loongson.h"
>> +
>> +static int loongson_spi_platform_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	void __iomem *reg_base;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	reg_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(reg_base))
>> +		return PTR_ERR(reg_base);
>> +
>> +	ret = loongson_spi_init_master(dev, reg_base);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to initialize master\n");
>> +
>> +	return ret;
> 
> 	return 0;

It seems was more appropriate that initialize ret then return ret.
Do you think so ?

> 
>> +}
> 
> ...
> 
>> +#ifndef __LINUX_SPI_LOONGSON_H
>> +#define __LINUX_SPI_LOONGSON_H
>> +
>> +#include <linux/bits.h>
> 
>> +#include <linux/device.h>
> 
> This header is not used.

okay, I got it.
> 
>> +#include <linux/pm.h>
> 
>> +#include <linux/spi/spi.h>
> 
> This neither.

That other .c file seems to need it and I will move it to other .c
code file.
> 
>> +#include <linux/types.h>
> 
> 
> For them use forward declarations
> 
> struct device;
> struct spi_controller;
> 
> The rest of the inclusions is correct.

okay, I got it.
> 
> ...
> 
>> +#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
> 
> Where is this used outside of the main driver?

These definitions are only used in core.c

> 
>> +/* Bits definition for Loongson SPI register */
>> +#define	LOONGSON_SPI_PARA_MEM_EN	BIT(0)
>> +#define	LOONGSON_SPI_SPCR_CPHA	BIT(2)
>> +#define	LOONGSON_SPI_SPCR_CPOL	BIT(3)
>> +#define	LOONGSON_SPI_SPCR_SPE	BIT(6)
>> +#define	LOONGSON_SPI_SPSR_WCOL	BIT(6)
>> +#define	LOONGSON_SPI_SPSR_SPIF	BIT(7)
>> +
>> +struct loongson_spi {
>> +	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;
>> +	unsigned int		mode;
>> +	u64			clk_rate;
>> +};
>> +
>> +int loongson_spi_init_master(struct device *dev, void __iomem *reg);
>> +extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
>> +#endif /* __LINUX_SPI_LOONGSON_H */
>
  
Andy Shevchenko May 24, 2023, 8:42 a.m. UTC | #3
On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
> 在 2023/5/23 下午8:54, andy.shevchenko@gmail.com 写道:
> > Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:

...

> >> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
> >> +                            struct spi_device *spi, struct spi_transfer *t)
> >> +{
> >> +    unsigned int hz;
> >> +
> >> +    if (t)
> >> +            hz = t->speed_hz;
> >
> > And if t is NULL? hz will be uninitialized. Don't you get a compiler warning?
> > (Always test your code with `make W=1 ...`)
>
> I always use `make W=1` and I don't find any warning, but that what you
> said was right and I will initial hz.

Note, if hz == 0 when t == NULL, you can unify that check with the below.

> >> +    if (hz && loongson_spi->hz != hz)

Something like

  if (t && _spi->hz != t->speed_hz)
    ...(..., t->speed_hz);

In such a case you won't need a temporary variable.

> >> +            loongson_spi_set_clk(loongson_spi, hz);
> >> +
> >> +    if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
> >> +            loongson_spi_set_mode(loongson_spi, spi);
> >> +
> >> +    return 0;
> >> +}

...

> > Why do you use deprecated naming? Can you use spi_controller* instead of
> > spi_master* in all cases?
>
> It seems was a personal code style issue and I don't find the
> differences between spi_controller and spi_master, Using spi_controller*
> is also acceptable to me and I will use spi_controller* instead of
> spi_master* in all cases.

Read this section (#4) in full
https://kernel.org/doc/html/latest/process/coding-style.html#naming

...

> >> +    clk = devm_clk_get_optional(dev, NULL);
> >> +    if (!IS_ERR(clk))
> >> +            spi->clk_rate = clk_get_rate(clk);
> >
> >> +    else
> >
> > Redundant. Just check for the error first as it's very usual pattern in the
> > Linux kernel.
>
> Like below ?
>
>          clk = devm_clk_get_optional(dev, NULL);
> -       if (!IS_ERR(clk))
> -               spi->clk_rate = clk_get_rate(clk);
> -       else
> +       if (IS_ERR(clk))
>                  return dev_err_probe(dev, PTR_ERR(clk), "unable to get
> clock\n");
>
> +       spi->clk_rate = clk_get_rate(clk);

Yes.

>          loongson_spi_reginit(spi);

> >> +            return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");

...

> >> +    ret = loongson_spi_init_master(dev, reg_base);
> >> +    if (ret)
> >> +            return dev_err_probe(dev, ret, "failed to initialize master\n");
> >> +
> >> +    return ret;
> >
> >       return 0;
>
> It seems was more appropriate that initialize ret then return ret.
> Do you think so ?

What do you mean and how does it help here?


...

> >> +#include <linux/spi/spi.h>
> >
> > This neither.
>
> That other .c file seems to need it and I will move it to other .c
> code file.

Yes, please do.

...

> >> +#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
> >
> > Where is this used outside of the main driver?
>
> These definitions are only used in core.c

Then the obvious question, why are they located in *.h?

...

> >> +/* Bits definition for Loongson SPI register */
> >> +#define     LOONGSON_SPI_PARA_MEM_EN        BIT(0)
> >> +#define     LOONGSON_SPI_SPCR_CPHA  BIT(2)
> >> +#define     LOONGSON_SPI_SPCR_CPOL  BIT(3)
> >> +#define     LOONGSON_SPI_SPCR_SPE   BIT(6)
> >> +#define     LOONGSON_SPI_SPSR_WCOL  BIT(6)
> >> +#define     LOONGSON_SPI_SPSR_SPIF  BIT(7)

Similar question here.
  
Mark Brown May 24, 2023, 10:19 a.m. UTC | #4
On Wed, May 24, 2023 at 11:42:34AM +0300, Andy Shevchenko wrote:
> On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:

> > >> +#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

> > > Where is this used outside of the main driver?

> > These definitions are only used in core.c

> Then the obvious question, why are they located in *.h?

It's absolutely fine to put them in a header file, that's a perfectly
normal way of writing code - it helps keep the driver a bit smaller by
putting big piles of defines in a separate file, that can help make
things a bit more manageable.
  
Yinbo Zhu May 25, 2023, 3:34 a.m. UTC | #5
在 2023/5/24 下午4:42, Andy Shevchenko 写道:
> On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
>> 在 2023/5/23 下午8:54, andy.shevchenko@gmail.com 写道:
>>> Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:
> 
> ...
> 
>>>> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
>>>> +                            struct spi_device *spi, struct spi_transfer *t)
>>>> +{
>>>> +    unsigned int hz;
>>>> +
>>>> +    if (t)
>>>> +            hz = t->speed_hz;
>>>
>>> And if t is NULL? hz will be uninitialized. Don't you get a compiler warning?
>>> (Always test your code with `make W=1 ...`)
>>
>> I always use `make W=1` and I don't find any warning, but that what you
>> said was right and I will initial hz.
> 
> Note, if hz == 0 when t == NULL, you can unify that check with the below.
> 
>>>> +    if (hz && loongson_spi->hz != hz)
> 
> Something like
> 
>    if (t && _spi->hz != t->speed_hz)
>      ...(..., t->speed_hz);
> 
> In such a case you won't need a temporary variable.

okay, I got it.

> 
>>>> +            loongson_spi_set_clk(loongson_spi, hz);
>>>> +
>>>> +    if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
>>>> +            loongson_spi_set_mode(loongson_spi, spi);
>>>> +
>>>> +    return 0;
>>>> +}
> 
> ...
> 
>>> Why do you use deprecated naming? Can you use spi_controller* instead of
>>> spi_master* in all cases?
>>
>> It seems was a personal code style issue and I don't find the
>> differences between spi_controller and spi_master, Using spi_controller*
>> is also acceptable to me and I will use spi_controller* instead of
>> spi_master* in all cases.
> 
> Read this section (#4) in full
> https://kernel.org/doc/html/latest/process/coding-style.html#naming

okay, I got it.

> 
> ...
> 
>>>> +    clk = devm_clk_get_optional(dev, NULL);
>>>> +    if (!IS_ERR(clk))
>>>> +            spi->clk_rate = clk_get_rate(clk);
>>>
>>>> +    else
>>>
>>> Redundant. Just check for the error first as it's very usual pattern in the
>>> Linux kernel.
>>
>> Like below ?
>>
>>           clk = devm_clk_get_optional(dev, NULL);
>> -       if (!IS_ERR(clk))
>> -               spi->clk_rate = clk_get_rate(clk);
>> -       else
>> +       if (IS_ERR(clk))
>>                   return dev_err_probe(dev, PTR_ERR(clk), "unable to get
>> clock\n");
>>
>> +       spi->clk_rate = clk_get_rate(clk);
> 
> Yes.

okay, I got it.
> 
>>           loongson_spi_reginit(spi);
> 
>>>> +            return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");
> 
> ...
> 
>>>> +    ret = loongson_spi_init_master(dev, reg_base);
>>>> +    if (ret)
>>>> +            return dev_err_probe(dev, ret, "failed to initialize master\n");
>>>> +
>>>> +    return ret;
>>>
>>>        return 0;
>>
>> It seems was more appropriate that initialize ret then return ret.
>> Do you think so ?
> 
> What do you mean and how does it help here?

I'm sorry, I was wrong before and the ret varible seems not to be
initialized and it always record the return value for
loongson_spi_init_master.

It seems was appropriate that use "return ret" and I don't got your
point that in probe for use "return 0"

> 
> 
> ...
> 
>>>> +#include <linux/spi/spi.h>
>>>
>>> This neither.
>>
>> That other .c file seems to need it and I will move it to other .c
>> code file.
> 
> Yes, please do.

okay, I got it.

Thanks,
Yinbo
  
Andy Shevchenko May 25, 2023, 9:16 a.m. UTC | #6
On Thu, May 25, 2023 at 6:34 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
> 在 2023/5/24 下午4:42, Andy Shevchenko 写道:
> > On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
> >> 在 2023/5/23 下午8:54, andy.shevchenko@gmail.com 写道:
> >>> Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:

...

> >>>> +    ret = loongson_spi_init_master(dev, reg_base);
> >>>> +    if (ret)
> >>>> +            return dev_err_probe(dev, ret, "failed to initialize master\n");
> >>>> +
> >>>> +    return ret;
> >>>
> >>>        return 0;
> >>
> >> It seems was more appropriate that initialize ret then return ret.
> >> Do you think so ?
> >
> > What do you mean and how does it help here?
>
> I'm sorry, I was wrong before and the ret varible seems not to be
> initialized and it always record the return value for
> loongson_spi_init_master.
>
> It seems was appropriate that use "return ret" and I don't got your
> point that in probe for use "return 0"

In the above excerpt you will return anything except 0 with return
dev_err_probe(); line. Why do you still need to return ret; at the end
of the function?
  
Yinbo Zhu May 25, 2023, 9:28 a.m. UTC | #7
在 2023/5/25 下午5:16, Andy Shevchenko 写道:
> On Thu, May 25, 2023 at 6:34 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
>> 在 2023/5/24 下午4:42, Andy Shevchenko 写道:
>>> On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
>>>> 在 2023/5/23 下午8:54, andy.shevchenko@gmail.com 写道:
>>>>> Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:
> 
> ...
> 
>>>>>> +    ret = loongson_spi_init_master(dev, reg_base);
>>>>>> +    if (ret)
>>>>>> +            return dev_err_probe(dev, ret, "failed to initialize master\n");
>>>>>> +
>>>>>> +    return ret;
>>>>>
>>>>>         return 0;
>>>>
>>>> It seems was more appropriate that initialize ret then return ret.
>>>> Do you think so ?
>>>
>>> What do you mean and how does it help here?
>>
>> I'm sorry, I was wrong before and the ret varible seems not to be
>> initialized and it always record the return value for
>> loongson_spi_init_master.
>>
>> It seems was appropriate that use "return ret" and I don't got your
>> point that in probe for use "return 0"
> 
> In the above excerpt you will return anything except 0 with return
> dev_err_probe(); line. Why do you still need to return ret; at the end
> of the function?


I'm sorry, I misread it and you are right and I will "return 0".

Thanks,
Yinbo.
  

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index e49c04c53c99..fd63c5a1c886 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12206,6 +12206,10 @@  M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-spi@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
+F:	drivers/spi/spi-loongson-core.c
+F:	drivers/spi/spi-loongson-pci.c
+F:	drivers/spi/spi-loongson-plat.c
+F:	drivers/spi/spi-loongson.h
 
 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 3de2ebe8294a..6b953904792e 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -516,6 +516,32 @@  config SPI_LM70_LLP
 	  which interfaces to an LM70 temperature sensor using
 	  a parallel port.
 
+config SPI_LOONGSON_CORE
+	tristate
+	depends on LOONGARCH || COMPILE_TEST
+
+config SPI_LOONGSON_PCI
+	tristate "Loongson SPI Controller PCI Driver Support"
+	select SPI_LOONGSON_CORE
+	depends on PCI && (LOONGARCH || COMPILE_TEST)
+	help
+	  This bus driver supports the Loongson SPI hardware controller in
+	  the Loongson platforms and supports to use 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_LOONGSON_PLATFORM
+	tristate "Loongson SPI Controller Platform Driver Support"
+	select SPI_LOONGSON_CORE
+	depends on OF && (LOONGARCH || COMPILE_TEST)
+	help
+	  This bus driver supports the Loongson SPI hardware controller in
+	  the Loongson platforms and supports to use DTS 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 28c4817a8a74..3e933064d237 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -71,6 +71,9 @@  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_CORE)		+= spi-loongson-core.o
+obj-$(CONFIG_SPI_LOONGSON_PCI)		+= spi-loongson-pci.o
+obj-$(CONFIG_SPI_LOONGSON_PLATFORM)	+= spi-loongson-plat.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-core.c b/drivers/spi/spi-loongson-core.c
new file mode 100644
index 000000000000..a556c60155d6
--- /dev/null
+++ b/drivers/spi/spi-loongson-core.c
@@ -0,0 +1,279 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+// Loongson SPI Support
+// Copyright (C) 2023 Loongson Technology Corporation Limited
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "spi-loongson.h"
+
+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 void loongson_spi_set_cs(struct spi_device *spi, bool val)
+{
+	int cs;
+	struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
+
+	cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
+					   & ~(0x11 << spi_get_chipselect(spi, 0));
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG,
+				       (val ? (0x11 << spi_get_chipselect(spi, 0)) :
+				       (0x1 << spi_get_chipselect(spi, 0))) | cs);
+}
+
+static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz)
+{
+	unsigned char val;
+	unsigned int div, div_tmp;
+	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
+
+	div = clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
+	div_tmp = rdiv[fls(div - 1)];
+	loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
+	loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;
+	val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
+	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->hz = hz;
+}
+
+static void loongson_spi_set_mode(struct loongson_spi *loongson_spi,
+				  struct spi_device *spi)
+{
+	unsigned char val;
+
+	val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
+	val &= ~(LOONGSON_SPI_SPCR_CPOL | LOONGSON_SPI_SPCR_CPHA);
+	if (spi->mode & SPI_CPOL)
+		val |= LOONGSON_SPI_SPCR_CPOL;
+	if (spi->mode & SPI_CPHA)
+		val |= LOONGSON_SPI_SPCR_CPHA;
+
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, val);
+	loongson_spi->mode |= spi->mode;
+}
+
+static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
+				struct spi_device *spi, struct spi_transfer *t)
+{
+	unsigned int hz;
+
+	if (t)
+		hz = t->speed_hz;
+
+	if (hz && loongson_spi->hz != hz)
+		loongson_spi_set_clk(loongson_spi, hz);
+
+	if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
+		loongson_spi_set_mode(loongson_spi, spi);
+
+	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_get_chipselect(spi, 0) >= spi->master->num_chipselect)
+		return -EINVAL;
+
+	loongson_spi->hz = 0;
+	loongson_spi_set_cs(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)++));
+	else
+		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, 0);
+	readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
+			   (loongson_spi->spsr & 0x1) != 1, 1, MSEC_PER_SEC);
+
+	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 int loongson_spi_prepare_message(struct spi_controller *ctlr, struct spi_message *m)
+{
+	struct loongson_spi *loongson_spi = spi_controller_get_devdata(ctlr);
+
+	loongson_spi->para = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para & ~1);
+
+	return 0;
+}
+
+static int loongson_spi_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
+				     struct spi_transfer *xfer)
+{
+	struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
+
+	loongson_spi_update_state(loongson_spi, spi, xfer);
+	if (xfer->len)
+		xfer->len = loongson_spi_write_read(spi, xfer);
+
+	return 0;
+}
+
+static int loongson_spi_unprepare_message(struct spi_controller *ctrl, struct spi_message *m)
+{
+	struct loongson_spi *loongson_spi = spi_controller_get_devdata(ctrl);
+
+	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para);
+
+	return 0;
+}
+
+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);
+}
+
+int loongson_spi_init_master(struct device *dev, void __iomem *regs)
+{
+	struct spi_master *master;
+	struct loongson_spi *spi;
+	struct clk *clk;
+
+	master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
+	if (master == NULL)
+		return -ENOMEM;
+
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+	master->setup = loongson_spi_setup;
+	master->prepare_message = loongson_spi_prepare_message;
+	master->transfer_one = loongson_spi_transfer_one;
+	master->unprepare_message = loongson_spi_unprepare_message;
+	master->set_cs = loongson_spi_set_cs;
+	master->num_chipselect = 4;
+	device_set_node(&master->dev, dev_fwnode(dev));
+	dev_set_drvdata(dev, master);
+
+	spi = spi_master_get_devdata(master);
+	spi->base = regs;
+	spi->master = master;
+
+	clk = devm_clk_get_optional(dev, NULL);
+	if (!IS_ERR(clk))
+		spi->clk_rate = clk_get_rate(clk);
+	else
+		return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");
+
+	loongson_spi_reginit(spi);
+
+	spi->mode = 0;
+
+	return devm_spi_register_master(dev, master);
+}
+EXPORT_SYMBOL_NS_GPL(loongson_spi_init_master, SPI_LOONGSON_CORE);
+
+static int __maybe_unused loongson_spi_suspend(struct device *dev)
+{
+	struct loongson_spi *loongson_spi;
+	struct spi_master *master;
+
+	master = dev_get_drvdata(dev);
+	spi_master_suspend(master);
+
+	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);
+
+	spi_master_resume(master);
+
+	return 0;
+}
+
+const struct dev_pm_ops loongson_spi_dev_pm_ops = {
+	.suspend = loongson_spi_suspend,
+	.resume = loongson_spi_resume,
+};
+EXPORT_SYMBOL_NS_GPL(loongson_spi_dev_pm_ops, SPI_LOONGSON_CORE);
+
+MODULE_DESCRIPTION("Loongson SPI core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spi-loongson-pci.c b/drivers/spi/spi-loongson-pci.c
new file mode 100644
index 000000000000..c351a689150a
--- /dev/null
+++ b/drivers/spi/spi-loongson-pci.c
@@ -0,0 +1,61 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+// PCI interface driver for Loongson SPI Support
+// Copyright (C) 2023 Loongson Technology Corporation Limited
+
+#include <linux/pci.h>
+
+#include "spi-loongson.h"
+
+static int loongson_spi_pci_register(struct pci_dev *pdev,
+			const struct pci_device_id *ent)
+{
+	int ret;
+	void __iomem *reg_base;
+	struct device *dev = &pdev->dev;
+	int pci_bar = 0;
+
+	ret = pcim_enable_device(pdev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot enable pci device\n");
+
+	ret = pcim_iomap_regions(pdev, BIT(pci_bar), pci_name(pdev));
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request and remap memory\n");
+
+	reg_base = pcim_iomap_table(pdev)[pci_bar];
+
+	ret = loongson_spi_init_master(dev, reg_base);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize master\n");
+
+	return 0;
+}
+
+static void loongson_spi_pci_unregister(struct pci_dev *pdev)
+{
+	pcim_iounmap_regions(pdev, BIT(0));
+	pci_disable_device(pdev);
+}
+
+static struct pci_device_id loongson_spi_devices[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a1b) },
+	{ },
+};
+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,
+	.driver	= {
+		.bus = &pci_bus_type,
+		.pm = &loongson_spi_dev_pm_ops,
+	},
+};
+module_pci_driver(loongson_spi_pci_driver);
+
+MODULE_DESCRIPTION("Loongson spi pci driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SPI_LOONGSON_CORE);
diff --git a/drivers/spi/spi-loongson-plat.c b/drivers/spi/spi-loongson-plat.c
new file mode 100644
index 000000000000..2e0388d84044
--- /dev/null
+++ b/drivers/spi/spi-loongson-plat.c
@@ -0,0 +1,46 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+// Platform driver for Loongson SPI Support
+// Copyright (C) 2023 Loongson Technology Corporation Limited
+
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "spi-loongson.h"
+
+static int loongson_spi_platform_probe(struct platform_device *pdev)
+{
+	int ret;
+	void __iomem *reg_base;
+	struct device *dev = &pdev->dev;
+
+	reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(reg_base))
+		return PTR_ERR(reg_base);
+
+	ret = loongson_spi_init_master(dev, reg_base);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize master\n");
+
+	return ret;
+}
+
+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_plat_driver = {
+	.probe = loongson_spi_platform_probe,
+	.driver	= {
+		.name	= "loongson-spi",
+		.bus = &platform_bus_type,
+		.pm = &loongson_spi_dev_pm_ops,
+		.of_match_table = loongson_spi_id_table,
+	},
+};
+module_platform_driver(loongson_spi_plat_driver);
+
+MODULE_DESCRIPTION("Loongson spi platform driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SPI_LOONGSON_CORE);
diff --git a/drivers/spi/spi-loongson.h b/drivers/spi/spi-loongson.h
new file mode 100644
index 000000000000..5dca9750efa3
--- /dev/null
+++ b/drivers/spi/spi-loongson.h
@@ -0,0 +1,47 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* Header File for Loongson SPI Driver. */
+/* Copyright (C) 2023 Loongson Technology Corporation Limited */
+
+#ifndef __LINUX_SPI_LOONGSON_H
+#define __LINUX_SPI_LOONGSON_H
+
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
+#include <linux/types.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_SPCR_CPHA	BIT(2)
+#define	LOONGSON_SPI_SPCR_CPOL	BIT(3)
+#define	LOONGSON_SPI_SPCR_SPE	BIT(6)
+#define	LOONGSON_SPI_SPSR_WCOL	BIT(6)
+#define	LOONGSON_SPI_SPSR_SPIF	BIT(7)
+
+struct loongson_spi {
+	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;
+	unsigned int		mode;
+	u64			clk_rate;
+};
+
+int loongson_spi_init_master(struct device *dev, void __iomem *reg);
+extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
+#endif /* __LINUX_SPI_LOONGSON_H */