diff mbox

[2/3] spi: mediatek: Add spi bus for Mediatek MT8173

Message ID 1431075343-7887-3-git-send-email-leilk.liu@mediatek.com
State New
Headers show

Commit Message

lei liu May 8, 2015, 8:55 a.m. UTC
From: Leilk Liu <leilk.liu@mediatek.com>

This patch adds basic spi bus for MT8173.

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 drivers/spi/Kconfig      |  10 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-mt65xx.c | 622 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 633 insertions(+)
 create mode 100644 drivers/spi/spi-mt65xx.c

Comments

Mark Brown May 8, 2015, 5:53 p.m. UTC | #1
On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> From: Leilk Liu <leilk.liu@mediatek.com>
> 
> This patch adds basic spi bus for MT8173.

Please try to only CC relevant people on patches, you've got a very
broad CC list here and I'm not sure I understand why everyone is on it.
Sending people irrelevant patches adds to the volume of mail people have
to handle which takes time away from other things.

> +config SPI_MT65XX
> +	tristate "MediaTek SPI controller"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	select SPI_BITBANG

Unless your controller is geniunely bitbanging you shouldn't be using
the bitbang framework, modern drivers should implement set_cs() and
transfer_one() instead.  You should also be using the core helpers for
DMA mapping.

The driver looks basically good other than this, it shouldn't be too
much work (and ought to save you some code) to refactor to the modern
interfaces.

> +#define IDLE 0
> +#define INPROGRESS 1
> +#define PAUSED 2
> +
> +#define PACKET_SIZE 1024

You should namespace the constants you're using to avoid clashes with
headers.

> +static const struct of_device_id mtk_spi_of_match[] = {
> +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);

There were three compatible strings listed in the DT binding but only
two here.

> +	/*set the software reset bit in SPI_CMD_REG.*/

/* Spaces please */

> +static unsigned long mtk_get_device_prop(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +
> +	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
> +	return (unsigned long)match->data;
> +}

This wrapper doesn't seem to be doing a lot and will crash if we somehow
manage to get the driver loaded without a match (eg, if someone tries to
instantiate as a regular platform device).

> +	ret = clk_prepare_enable(mdata->clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
> +		goto err;
> +	}

I'd expect to see runtime PM callbacks to enable and disable this clock
in order to minimise power consumption.
lei liu May 12, 2015, 12:39 p.m. UTC | #2
Thanks for your review.

On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> > From: Leilk Liu <leilk.liu@mediatek.com>
> > 
> > This patch adds basic spi bus for MT8173.
> 
> Please try to only CC relevant people on patches, you've got a very
> broad CC list here and I'm not sure I understand why everyone is on it.
> Sending people irrelevant patches adds to the volume of mail people have
> to handle which takes time away from other things.
> 

I use scripts/get_maintainer.pl on the patches and keep all maintainers.
I'll further refine the list in the future.

> > +config SPI_MT65XX
> > +	tristate "MediaTek SPI controller"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	select SPI_BITBANG
> 
> Unless your controller is geniunely bitbanging you shouldn't be using
> the bitbang framework, modern drivers should implement set_cs() and
> transfer_one() instead.  You should also be using the core helpers for
> DMA mapping.
> 

I will implement set_cs() and transfer_one() instead in next patch.

Could you tell me more details about "You should also be using the core
helpers for DMA mapping"?

> The driver looks basically good other than this, it shouldn't be too
> much work (and ought to save you some code) to refactor to the modern
> interfaces.
> 
> > +#define IDLE 0
> > +#define INPROGRESS 1
> > +#define PAUSED 2
> > +
> > +#define PACKET_SIZE 1024
> 
> You should namespace the constants you're using to avoid clashes with
> headers.
> 
> > +static const struct of_device_id mtk_spi_of_match[] = {
> > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> There were three compatible strings listed in the DT binding but only
> two here.
> 

MT6589 and MT8135 is compatible; 
For MT8135 IC, it can use the follow way in dts to probe:
  compatible = "mediatek,mt8135-spi", 
	       "mediatek,mt6589-spi";
And I test it's ok on MT8135 platform. So I add struct of_device_id
mtk_spi_of_match like this in spi driver code.

> > +	/*set the software reset bit in SPI_CMD_REG.*/
> 
> /* Spaces please */
> 
> > +static unsigned long mtk_get_device_prop(struct platform_device *pdev)
> > +{
> > +	const struct of_device_id *match;
> > +
> > +	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
> > +	return (unsigned long)match->data;
> > +}
> 
> This wrapper doesn't seem to be doing a lot and will crash if we somehow
> manage to get the driver loaded without a match (eg, if someone tries to
> instantiate as a regular platform device).
> 
> > +	ret = clk_prepare_enable(mdata->clk);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
> > +		goto err;
> > +	}
> 
> I'd expect to see runtime PM callbacks to enable and disable this clock
> in order to minimise power consumption.

In the case of other review comments, I will also fix them in next/OK.
Mark Brown May 12, 2015, 4:05 p.m. UTC | #3
On Tue, May 12, 2015 at 08:39:16PM +0800, leilk liu wrote:
> On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> > On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:

> Could you tell me more details about "You should also be using the core
> helpers for DMA mapping"?

Implement can_dma() - look for drivers providing that for examples.

> > > +static const struct of_device_id mtk_spi_of_match[] = {
> > > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);

> > There were three compatible strings listed in the DT binding but only
> > two here.

> MT6589 and MT8135 is compatible; 
> For MT8135 IC, it can use the follow way in dts to probe:
>   compatible = "mediatek,mt8135-spi", 
> 	       "mediatek,mt6589-spi";

> And I test it's ok on MT8135 platform. So I add struct of_device_id
> mtk_spi_of_match like this in spi driver code.

You should list all the compatibles documented in the binding here, if
some of them are the same just have them map to a single constant.
Yingjoe Chen May 13, 2015, 9:26 a.m. UTC | #4
On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 08:39:16PM +0800, leilk liu wrote:
<...>
> > > > +static const struct of_device_id mtk_spi_of_match[] = {
> > > > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > > > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > > > +	{}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> > > There were three compatible strings listed in the DT binding but only
> > > two here.
> 
> > MT6589 and MT8135 is compatible; 
> > For MT8135 IC, it can use the follow way in dts to probe:
> >   compatible = "mediatek,mt8135-spi", 
> > 	       "mediatek,mt6589-spi";
> 
> > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > mtk_spi_of_match like this in spi driver code.
> 
> You should list all the compatibles documented in the binding here, if
> some of them are the same just have them map to a single constant.

Hi Mark,

Just for clarification. If we want to add spi support for a new soc, say
mt8127, which we think is compatible to mt6589. Since it may turn out we
need special handling for this soc latter, it is suggested to write
compatible like this in mt8127.dtsi:

   compatible = "mediatek,mt8127-spi",
 	        "mediatek,mt6589-spi";

Device tree binding should list all possible compatible string in .dts,
so we'll have to add that to binding as well.

     - mediatek,mt6589-spi: for mt6589 platforms
+    - mediatek,mt8127-spi: for mt8127 platforms

Then we'll also need to add this to source code to reflect this.

	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
+	{ .compatible = "mediatek,mt8127-spi", .data = (void *)COMPAT_MT6589},

This seems to introduce lots of trivial patches just to add a compatible
device to me...

Joe.C
Mark Brown May 13, 2015, 11:10 a.m. UTC | #5
On Wed, May 13, 2015 at 05:26:06PM +0800, Yingjoe Chen wrote:
> On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:

> > > > There were three compatible strings listed in the DT binding but only
> > > > two here.

> > > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > > mtk_spi_of_match like this in spi driver code.

> > You should list all the compatibles documented in the binding here, if
> > some of them are the same just have them map to a single constant.

> Just for clarification. If we want to add spi support for a new soc, say
> mt8127, which we think is compatible to mt6589. Since it may turn out we
> need special handling for this soc latter, it is suggested to write
> compatible like this in mt8127.dtsi:

>    compatible = "mediatek,mt8127-spi",
>  	        "mediatek,mt6589-spi";

> Device tree binding should list all possible compatible string in .dts,
> so we'll have to add that to binding as well.

>      - mediatek,mt6589-spi: for mt6589 platforms
> +    - mediatek,mt8127-spi: for mt8127 platforms

> Then we'll also need to add this to source code to reflect this.

> 	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> +	{ .compatible = "mediatek,mt8127-spi", .data = (void *)COMPAT_MT6589},

> This seems to introduce lots of trivial patches just to add a compatible
> device to me...

Yes, but that's how DT works and as you say these patches are all
trivial so it's not like they take any appreciable effort.
Yingjoe Chen May 13, 2015, 1:58 p.m. UTC | #6
On Wed, 2015-05-13 at 12:10 +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 05:26:06PM +0800, Yingjoe Chen wrote:
> > On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> 
> > > > > There were three compatible strings listed in the DT binding but only
> > > > > two here.
> 
> > > > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > > > mtk_spi_of_match like this in spi driver code.
> 
> > > You should list all the compatibles documented in the binding here, if
> > > some of them are the same just have them map to a single constant.
> 
> > Just for clarification. If we want to add spi support for a new soc, say
> > mt8127, which we think is compatible to mt6589. Since it may turn out we
> > need special handling for this soc latter, it is suggested to write
> > compatible like this in mt8127.dtsi:
> 
> >    compatible = "mediatek,mt8127-spi",
> >  	        "mediatek,mt6589-spi";
> 
> > Device tree binding should list all possible compatible string in .dts,
> > so we'll have to add that to binding as well.
> 
> >      - mediatek,mt6589-spi: for mt6589 platforms
> > +    - mediatek,mt8127-spi: for mt8127 platforms
> 
> > Then we'll also need to add this to source code to reflect this.
> 
> > 	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > +	{ .compatible = "mediatek,mt8127-spi", .data = (void *)COMPAT_MT6589},
> 
> > This seems to introduce lots of trivial patches just to add a compatible
> > device to me...
> 
> Yes, but that's how DT works and as you say these patches are all
> trivial so it's not like they take any appreciable effort.

Yes, that's shouldn't be a big effort.
Thanks for clarification.

Joe.C
lei liu May 15, 2015, 7:38 a.m. UTC | #7
Dear Mark,

Thanks for your reply.

On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 08:39:16PM +0800, leilk liu wrote:
> > On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> > > On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> 
> > Could you tell me more details about "You should also be using the core
> > helpers for DMA mapping"?
> 
> Implement can_dma() - look for drivers providing that for examples.
> 

MTK spi hardware uses the dmaengine in spi controller. According to
datasheet, spi driver just need to enable dma register bit and write a
physical address to relevant dma address register, so I think it may be
complex while the driver supports can_dma.

> > > > +static const struct of_device_id mtk_spi_of_match[] = {
> > > > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > > > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > > > +	{}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> > > There were three compatible strings listed in the DT binding but only
> > > two here.
> 
> > MT6589 and MT8135 is compatible; 
> > For MT8135 IC, it can use the follow way in dts to probe:
> >   compatible = "mediatek,mt8135-spi", 
> > 	       "mediatek,mt6589-spi";
> 
> > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > mtk_spi_of_match like this in spi driver code.
> 
> You should list all the compatibles documented in the binding here, if
> some of them are the same just have them map to a single constant.
Mark Brown May 15, 2015, 9:25 a.m. UTC | #8
On Fri, May 15, 2015 at 03:38:42PM +0800, leilk liu wrote:
> On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:

> > Implement can_dma() - look for drivers providing that for examples.

> MTK spi hardware uses the dmaengine in spi controller. According to
> datasheet, spi driver just need to enable dma register bit and write a
> physical address to relevant dma address register, so I think it may be
> complex while the driver supports can_dma.

That's how a very large proportion of devices that work with DMA are
done - why would this be complicated?  All can_dma() does is report if
DMA is possible.
Eddie Huang June 8, 2015, 10:15 a.m. UTC | #9
Hi Mark,

On Fri, 2015-05-15 at 17:25 +0800, Mark Brown wrote:
> On Fri, May 15, 2015 at 03:38:42PM +0800, leilk liu wrote:
> > On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> 
> > > Implement can_dma() - look for drivers providing that for examples.
> 
> > MTK spi hardware uses the dmaengine in spi controller. According to
> > datasheet, spi driver just need to enable dma register bit and write a
> > physical address to relevant dma address register, so I think it may be
> > complex while the driver supports can_dma.
> 
> That's how a very large proportion of devices that work with DMA are
> done - why would this be complicated?  All can_dma() does is report if
> DMA is possible.

In include/linux/spi/spi.h, it describes if can_dma() exists and returns
true, dma_tx and dma_rx must be set.But Medaitek SPI controller has its
own dma hardware, which means this dma resides in the same base address
range with SPI controller, and only used by SPI, so we don't implement
generic DMA driver, such that can't provide dma channel and assign to
dmx_tx, dmx_rx parameter. We think it's strange to implement generic dma
driver for dma that only used by specific hardware.Can we just provide
can_dma() function and return false ? But I think it's a little odd that
there actually has dma. So can we just skip can_dma() function let it be
NULL ?

Eddie
Thanks
Mark Brown June 8, 2015, 5:59 p.m. UTC | #10
On Mon, Jun 08, 2015 at 06:15:46PM +0800, Eddie Huang wrote:
> On Fri, 2015-05-15 at 17:25 +0800, Mark Brown wrote:

> > That's how a very large proportion of devices that work with DMA are
> > done - why would this be complicated?  All can_dma() does is report if
> > DMA is possible.

> In include/linux/spi/spi.h, it describes if can_dma() exists and returns
> true, dma_tx and dma_rx must be set.But Medaitek SPI controller has its
> own dma hardware, which means this dma resides in the same base address
> range with SPI controller, and only used by SPI, so we don't implement
> generic DMA driver, such that can't provide dma channel and assign to
> dmx_tx, dmx_rx parameter. We think it's strange to implement generic dma
> driver for dma that only used by specific hardware.Can we just provide
> can_dma() function and return false ? But I think it's a little odd that
> there actually has dma. So can we just skip can_dma() function let it be
> NULL ?

If it's simply the unavailbility of a struct dma_chan we must be able to
get a better solution than just open coding all the DMA mapping and
unmapping in the driver.  The only thing we actually use the channel for
is to get the device we need to use to do the mapping and unmapping,
either we need a way for devices to provide their own channels easily or
a way for SPI drivers to specify a device here instead of a channel.
The latter seems easier if a bit clunky (with having to work with both).
diff mbox

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..01239e0 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -324,6 +324,16 @@  config SPI_MESON_SPIFC
 	  This enables master mode support for the SPIFC (SPI flash
 	  controller) available in Amlogic Meson SoCs.
 
+config SPI_MT65XX
+	tristate "MediaTek SPI controller"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	select SPI_BITBANG
+	help
+	  This selects the MediaTek(R) SPI bus driver.
+	  If you want to use MediaTek(R) SPI interface,
+	  say Y or M here.If you are not sure, say N.
+	  SPI drivers for Mediatek mt65XX series ARM SoCs.
+
 config SPI_OC_TINY
 	tristate "OpenCores tiny SPI"
 	depends on GPIOLIB
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..ab332ef 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -48,6 +48,7 @@  obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
 obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
+obj-$(CONFIG_SPI_MT65XX)                += spi-mt65xx.o
 obj-$(CONFIG_SPI_MXS)			+= spi-mxs.o
 obj-$(CONFIG_SPI_NUC900)		+= spi-nuc900.o
 obj-$(CONFIG_SPI_OC_TINY)		+= spi-oc-tiny.o
diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
new file mode 100644
index 0000000..92c119d
--- /dev/null
+++ b/drivers/spi/spi-mt65xx.c
@@ -0,0 +1,622 @@ 
+/*
+ * Copyright (c) 2015 MediaTek Inc.
+ * Author: Leilk Liu <leilk.liu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/ioport.h>
+#include <linux/errno.h>
+#include <linux/spi/spi.h>
+#include <linux/workqueue.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/sched.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/kernel.h>
+#include <linux/spi/spi_bitbang.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+
+#define SPI_CFG0_REG                      0x0000
+#define SPI_CFG1_REG                      0x0004
+#define SPI_TX_SRC_REG                    0x0008
+#define SPI_RX_DST_REG                    0x000c
+#define SPI_CMD_REG                       0x0018
+#define SPI_STATUS0_REG                   0x001c
+#define SPI_PAD_SEL_REG                   0x0024
+
+#define SPI_CFG0_SCK_HIGH_OFFSET          0
+#define SPI_CFG0_SCK_LOW_OFFSET           8
+#define SPI_CFG0_CS_HOLD_OFFSET           16
+#define SPI_CFG0_CS_SETUP_OFFSET          24
+
+#define SPI_CFG0_SCK_HIGH_MASK            0xff
+#define SPI_CFG0_SCK_LOW_MASK             0xff00
+#define SPI_CFG0_CS_HOLD_MASK             0xff0000
+#define SPI_CFG0_CS_SETUP_MASK            0xff000000
+
+#define SPI_CFG1_CS_IDLE_OFFSET           0
+#define SPI_CFG1_PACKET_LOOP_OFFSET       8
+#define SPI_CFG1_PACKET_LENGTH_OFFSET     16
+#define SPI_CFG1_GET_TICK_DLY_OFFSET      30
+
+#define SPI_CFG1_CS_IDLE_MASK             0xff
+#define SPI_CFG1_PACKET_LOOP_MASK         0xff00
+#define SPI_CFG1_PACKET_LENGTH_MASK       0x3ff0000
+#define SPI_CFG1_GET_TICK_DLY_MASK        0xc0000000
+
+#define SPI_CMD_ACT_OFFSET                0
+#define SPI_CMD_RESUME_OFFSET             1
+#define SPI_CMD_RST_OFFSET                2
+#define SPI_CMD_PAUSE_EN_OFFSET           4
+#define SPI_CMD_DEASSERT_OFFSET           5
+#define SPI_CMD_CPHA_OFFSET               8
+#define SPI_CMD_CPOL_OFFSET               9
+#define SPI_CMD_RX_DMA_OFFSET             10
+#define SPI_CMD_TX_DMA_OFFSET             11
+#define SPI_CMD_TXMSBF_OFFSET             12
+#define SPI_CMD_RXMSBF_OFFSET             13
+#define SPI_CMD_RX_ENDIAN_OFFSET          14
+#define SPI_CMD_TX_ENDIAN_OFFSET          15
+#define SPI_CMD_FINISH_IE_OFFSET          16
+#define SPI_CMD_PAUSE_IE_OFFSET           17
+
+#define SPI_CMD_RESUME_MASK               0x2
+#define SPI_CMD_RST_MASK                  0x4
+#define SPI_CMD_PAUSE_EN_MASK             0x10
+#define SPI_CMD_DEASSERT_MASK             0x20
+#define SPI_CMD_CPHA_MASK                 0x100
+#define SPI_CMD_CPOL_MASK                 0x200
+#define SPI_CMD_RX_DMA_MASK               0x400
+#define SPI_CMD_TX_DMA_MASK               0x800
+#define SPI_CMD_TXMSBF_MASK               0x1000
+#define SPI_CMD_RXMSBF_MASK               0x2000
+#define SPI_CMD_RX_ENDIAN_MASK            0x4000
+#define SPI_CMD_TX_ENDIAN_MASK            0x8000
+#define SPI_CMD_FINISH_IE_MASK            0x10000
+
+#define COMPAT_MT6589			(0x1 << 0)
+#define COMPAT_MT8173			(0x1 << 1)
+
+#define MT8173_MAX_PAD_SEL 3
+
+#define IDLE 0
+#define INPROGRESS 1
+#define PAUSED 2
+
+#define PACKET_SIZE 1024
+
+struct mtk_chip_config {
+	u32 setuptime;
+	u32 holdtime;
+	u32 high_time;
+	u32 low_time;
+	u32 cs_idletime;
+	u32 tx_mlsb;
+	u32 rx_mlsb;
+	u32 tx_endian;
+	u32 rx_endian;
+	u32 pause;
+	u32 finish_intr;
+	u32 deassert;
+	u32 tckdly;
+};
+
+struct mtk_spi_ddata {
+	struct spi_bitbang bitbang;
+	void __iomem *base;
+	u32 irq;
+	u32 state;
+	u32 platform_compat;
+	u32 pad_sel;
+	struct clk *clk;
+
+	const u8 *tx_buf;
+	u8 *rx_buf;
+	u32 tx_len, rx_len;
+	struct completion done;
+};
+
+/*
+ * A piece of default chip info unless the platform
+ * supplies it.
+ */
+static const struct mtk_chip_config mtk_default_chip_info = {
+	.setuptime = 10,
+	.holdtime = 12,
+	.high_time = 6,
+	.low_time = 6,
+	.cs_idletime = 12,
+	.rx_mlsb = 1,
+	.tx_mlsb = 1,
+	.tx_endian = 0,
+	.rx_endian = 0,
+	.pause = 0,
+	.finish_intr = 1,
+	.deassert = 0,
+	.tckdly = 0,
+};
+
+static const struct of_device_id mtk_spi_of_match[] = {
+	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
+	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
+
+static void mtk_spi_reset(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	/*set the software reset bit in SPI_CMD_REG.*/
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_RST_MASK;
+	reg_val |= 1 << SPI_CMD_RST_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_RST_MASK;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static void mtk_set_pause_bit(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val |= 1 << SPI_CMD_PAUSE_EN_OFFSET;
+	reg_val |= 1 << SPI_CMD_PAUSE_IE_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static void mtk_clear_pause_bit(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_PAUSE_EN_MASK;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static int mtk_spi_config(struct mtk_spi_ddata *mdata,
+			  struct mtk_chip_config *chip_config)
+{
+	u32 reg_val;
+
+	/* set the timing */
+	reg_val = readl(mdata->base + SPI_CFG0_REG);
+	reg_val &= ~(SPI_CFG0_SCK_HIGH_MASK | SPI_CFG0_SCK_LOW_MASK);
+	reg_val &= ~(SPI_CFG0_CS_HOLD_MASK | SPI_CFG0_CS_SETUP_MASK);
+	reg_val |= ((chip_config->high_time - 1) << SPI_CFG0_SCK_HIGH_OFFSET);
+	reg_val |= ((chip_config->low_time - 1) << SPI_CFG0_SCK_LOW_OFFSET);
+	reg_val |= ((chip_config->holdtime - 1) << SPI_CFG0_CS_HOLD_OFFSET);
+	reg_val |= ((chip_config->setuptime - 1) << SPI_CFG0_CS_SETUP_OFFSET);
+	writel(reg_val, mdata->base + SPI_CFG0_REG);
+
+	reg_val = readl(mdata->base + SPI_CFG1_REG);
+	reg_val &= ~SPI_CFG1_CS_IDLE_MASK;
+	reg_val |= ((chip_config->cs_idletime - 1) << SPI_CFG1_CS_IDLE_OFFSET);
+	reg_val &= ~SPI_CFG1_GET_TICK_DLY_MASK;
+	reg_val |= ((chip_config->tckdly) << SPI_CFG1_GET_TICK_DLY_OFFSET);
+	writel(reg_val, mdata->base + SPI_CFG1_REG);
+
+	/* set the mlsbx and mlsbtx */
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~(SPI_CMD_TX_ENDIAN_MASK | SPI_CMD_RX_ENDIAN_MASK);
+	reg_val &= ~(SPI_CMD_TXMSBF_MASK | SPI_CMD_RXMSBF_MASK);
+	reg_val |= (chip_config->tx_mlsb << SPI_CMD_TXMSBF_OFFSET);
+	reg_val |= (chip_config->rx_mlsb << SPI_CMD_RXMSBF_OFFSET);
+	reg_val |= (chip_config->tx_endian << SPI_CMD_TX_ENDIAN_OFFSET);
+	reg_val |= (chip_config->rx_endian << SPI_CMD_RX_ENDIAN_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	/* set finish and pause interrupt always enable */
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_FINISH_IE_MASK;
+	reg_val |= (chip_config->finish_intr << SPI_CMD_FINISH_IE_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val |= 1 << SPI_CMD_TX_DMA_OFFSET;
+	reg_val |= 1 << SPI_CMD_RX_DMA_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	/* set deassert mode */
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_DEASSERT_MASK;
+	reg_val |= (chip_config->deassert << SPI_CMD_DEASSERT_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	/* pad select */
+	if (mdata->platform_compat & COMPAT_MT8173)
+		writel(mdata->pad_sel, mdata->base + SPI_PAD_SEL_REG);
+
+	return 0;
+}
+
+static int mtk_spi_setup_transfer(struct spi_device *spi,
+				  struct spi_transfer *t)
+{
+	u32 reg_val;
+	struct spi_master *master = spi->master;
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(master);
+	struct spi_message *m = master->cur_msg;
+	struct mtk_chip_config *chip_config;
+
+	u8 cpha	= spi->mode & SPI_CPHA ? 1 : 0;
+	u8 cpol = spi->mode & SPI_CPOL ? 1 : 0;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~(SPI_CMD_CPHA_MASK | SPI_CMD_CPOL_MASK);
+	reg_val |= (cpha << SPI_CMD_CPHA_OFFSET);
+	reg_val |= (cpol << SPI_CMD_CPOL_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	if (t->cs_change) {
+		if (!(list_is_last(&t->transfer_list, &m->transfers)))
+			mdata->state = IDLE;
+	} else {
+		mdata->state = IDLE;
+		mtk_spi_reset(mdata);
+	}
+
+	chip_config = (struct mtk_chip_config *)spi->controller_data;
+	if (!chip_config) {
+		chip_config = (void *)&mtk_default_chip_info;
+		spi->controller_data = chip_config;
+		mdata->state = IDLE;
+	}
+
+	mtk_spi_config(mdata, chip_config);
+
+	return 0;
+}
+
+static void mtk_spi_chipselect(struct spi_device *spi, int is_on)
+{
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(spi->master);
+
+	switch (is_on) {
+	case BITBANG_CS_ACTIVE:
+		mtk_set_pause_bit(mdata);
+		break;
+	case BITBANG_CS_INACTIVE:
+		mtk_clear_pause_bit(mdata);
+		break;
+	}
+}
+
+static void mtk_spi_start_transfer(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val |= 1 << SPI_CMD_ACT_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static void mtk_spi_resume_transfer(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_RESUME_MASK;
+	reg_val |= 1 << SPI_CMD_RESUME_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static int mtk_spi_setup_packet(struct mtk_spi_ddata *mdata,
+				struct spi_transfer *xfer)
+{
+	struct device *dev = &mdata->bitbang.master->dev;
+	u32 packet_size, packet_loop, reg_val;
+
+	packet_size = min_t(unsigned, xfer->len, PACKET_SIZE);
+
+	/* mtk hw has the restriction that xfer len must be a multiple of 1024,
+	 * when it is greater than 1024bytes.
+	 */
+	if (xfer->len % packet_size) {
+		dev_err(dev, "ERROR!The lens must be a multiple of %d, your len %d\n",
+			PACKET_SIZE, xfer->len);
+		return -EINVAL;
+	}
+
+	packet_loop = xfer->len / packet_size;
+
+	reg_val = readl(mdata->base + SPI_CFG1_REG);
+	reg_val &= ~(SPI_CFG1_PACKET_LENGTH_MASK + SPI_CFG1_PACKET_LOOP_MASK);
+	reg_val |= (packet_size - 1) << SPI_CFG1_PACKET_LENGTH_OFFSET;
+	reg_val |= (packet_loop - 1) << SPI_CFG1_PACKET_LOOP_OFFSET;
+	writel(reg_val, mdata->base + SPI_CFG1_REG);
+
+	return 0;
+}
+
+static int mtk_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *xfer)
+{
+	struct spi_master *master = spi->master;
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(master);
+	struct device *dev = &mdata->bitbang.master->dev;
+	int cmd, ret;
+
+	/* mtk spi hw tx/rx have 4bytes aligned restriction,
+	 * so kmalloc tx/rx buffer to workaround here.
+	 */
+	mdata->tx_buf = NULL;
+	mdata->rx_buf = NULL;
+	if (xfer->tx_buf) {
+		mdata->tx_buf = kmalloc(xfer->len, GFP_KERNEL);
+		if (!mdata->tx_buf) {
+			dev_err(dev, "malloc tx_buf failed.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+		memcpy((void *)mdata->tx_buf, xfer->tx_buf, xfer->len);
+	}
+	if (xfer->rx_buf) {
+		mdata->rx_buf = kmalloc(xfer->len, GFP_KERNEL);
+		if (!mdata->rx_buf) {
+			dev_err(dev, "malloc rx_buf failed.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	reinit_completion(&mdata->done);
+
+	xfer->tx_dma = DMA_ERROR_CODE;
+	xfer->rx_dma = DMA_ERROR_CODE;
+	if (xfer->tx_buf) {
+		xfer->tx_dma = dma_map_single(dev, (void *)mdata->tx_buf,
+					      xfer->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(dev, xfer->tx_dma)) {
+			dev_err(dev, "dma mapping tx_buf error.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+	if (xfer->rx_buf) {
+		xfer->rx_dma = dma_map_single(dev, mdata->rx_buf,
+					      xfer->len, DMA_FROM_DEVICE);
+		if (dma_mapping_error(dev, xfer->rx_dma)) {
+			if (xfer->tx_buf)
+				dma_unmap_single(dev, xfer->tx_dma,
+						 xfer->len, DMA_TO_DEVICE);
+			dev_err(dev, "dma mapping rx_buf error.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	ret = mtk_spi_setup_packet(mdata, xfer);
+	if (ret != 0)
+		goto err_free;
+
+	/* Here is mt8173 HW issue: RX must enable TX, then TX transfer
+	 * dummy data; TX don't need to enable RX. so enable TX dma for
+	 * RX to workaround.
+	 */
+	cmd = readl(mdata->base + SPI_CMD_REG);
+	if (xfer->tx_buf || (mdata->platform_compat & COMPAT_MT8173))
+		cmd |= 1 << SPI_CMD_TX_DMA_OFFSET;
+	if (xfer->rx_buf)
+		cmd |= 1 << SPI_CMD_RX_DMA_OFFSET;
+	writel(cmd, mdata->base + SPI_CMD_REG);
+
+	/* set up the DMA bus address */
+	if (xfer->tx_dma != DMA_ERROR_CODE)
+		writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
+	if (xfer->rx_dma != DMA_ERROR_CODE)
+		writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
+
+	if (mdata->state == IDLE)
+		mtk_spi_start_transfer(mdata);
+	else if (mdata->state == PAUSED)
+		mtk_spi_resume_transfer(mdata);
+	else
+		mdata->state = INPROGRESS;
+
+	wait_for_completion(&mdata->done);
+
+	if (xfer->tx_dma != DMA_ERROR_CODE) {
+		dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
+		xfer->tx_dma = DMA_ERROR_CODE;
+	}
+	if (xfer->rx_dma != DMA_ERROR_CODE) {
+		dma_unmap_single(dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
+		xfer->rx_dma = DMA_ERROR_CODE;
+	}
+
+	/* spi disable dma */
+	cmd = readl(mdata->base + SPI_CMD_REG);
+	cmd &= ~SPI_CMD_TX_DMA_MASK;
+	cmd &= ~SPI_CMD_RX_DMA_MASK;
+	writel(cmd, mdata->base + SPI_CMD_REG);
+
+	if (xfer->rx_buf)
+		memcpy(xfer->rx_buf, mdata->rx_buf, xfer->len);
+
+	ret = xfer->len;
+
+err_free:
+	kfree(mdata->tx_buf);
+	kfree(mdata->rx_buf);
+	return ret;
+}
+
+static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
+{
+	struct mtk_spi_ddata *mdata = dev_id;
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_STATUS0_REG);
+	if (reg_val & 0x2)
+		mdata->state = PAUSED;
+	else
+		mdata->state = IDLE;
+	complete(&mdata->done);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned long mtk_get_device_prop(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
+	return (unsigned long)match->data;
+}
+
+static int mtk_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct mtk_spi_ddata *mdata;
+	struct resource *res;
+	int	ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct mtk_spi_ddata));
+	if (!master) {
+		dev_err(&pdev->dev, "failed to alloc spi master\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, master);
+
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->num_chipselect = 1;
+	master->mode_bits = SPI_CPOL | SPI_CPHA;
+
+	mdata = spi_master_get_devdata(master);
+
+	mdata->bitbang.master = master;
+	mdata->bitbang.chipselect = mtk_spi_chipselect;
+	mdata->bitbang.setup_transfer = mtk_spi_setup_transfer;
+	mdata->bitbang.txrx_bufs = mtk_spi_txrx_bufs;
+	mdata->platform_compat = mtk_get_device_prop(pdev);
+
+	if (mdata->platform_compat & COMPAT_MT8173) {
+		ret = of_property_read_u32(pdev->dev.of_node, "pad-select",
+					   &mdata->pad_sel);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to read pad select: %d\n",
+				ret);
+			goto err;
+		}
+
+		if (mdata->pad_sel > MT8173_MAX_PAD_SEL) {
+			dev_err(&pdev->dev, "wrong pad-select: %u\n",
+				mdata->pad_sel);
+			goto err;
+		}
+	}
+
+	init_completion(&mdata->done);
+
+	mdata->clk = devm_clk_get(&pdev->dev, "main");
+	if (IS_ERR(mdata->clk)) {
+		ret = PTR_ERR(mdata->clk);
+		dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
+		goto err;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -ENODEV;
+		dev_err(&pdev->dev, "failed to determine base address\n");
+		goto err;
+	}
+
+	mdata->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mdata->base)) {
+		ret = PTR_ERR(mdata->base);
+		goto err;
+	}
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to get irq (%d)\n", ret);
+		goto err;
+	}
+
+	mdata->irq = ret;
+
+	if (!pdev->dev.dma_mask)
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+
+	mdata->bitbang.master->dev.dma_mask = pdev->dev.dma_mask;
+
+	ret = clk_prepare_enable(mdata->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
+		goto err;
+	}
+
+	ret = devm_request_irq(&pdev->dev, mdata->irq, mtk_spi_interrupt,
+			       IRQF_TRIGGER_NONE, dev_name(&pdev->dev), mdata);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register irq (%d)\n", ret);
+		goto err_disable_clk;
+	}
+
+	ret = spi_bitbang_start(&mdata->bitbang);
+	if (ret) {
+		dev_err(&pdev->dev, "spi_bitbang_start failed (%d)\n", ret);
+err_disable_clk:
+		clk_disable_unprepare(mdata->clk);
+err:
+		spi_master_put(master);
+	}
+
+	return ret;
+}
+
+static int mtk_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(master);
+
+	spi_bitbang_stop(&mdata->bitbang);
+	mtk_spi_reset(mdata);
+	clk_disable_unprepare(mdata->clk);
+	spi_master_put(master);
+
+	return 0;
+}
+
+struct platform_driver mtk_spi_driver = {
+	.driver = {
+		.name = "mtk-spi",
+		.of_match_table = mtk_spi_of_match,
+	},
+	.probe = mtk_spi_probe,
+	.remove = mtk_spi_remove,
+};
+
+module_platform_driver(mtk_spi_driver);
+
+MODULE_DESCRIPTION("MTK SPI Controller driver");
+MODULE_AUTHOR("Leilk Liu <leilk.liu@mediatek.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform: mtk_spi");