diff mbox series

[1/2] spi: Add MXIC controller driver

Message ID 1533175670-26878-1-git-send-email-masonccyang@mxic.com.tw (mailing list archive)
State New, archived
Headers show
Series [1/2] spi: Add MXIC controller driver | expand

Commit Message

Mason Yang Aug. 2, 2018, 2:07 a.m. UTC
From: Mason Yang <masonccyang@mxic.com.tw>

Add a driver for Macronix SPI controller IP.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/spi/Kconfig    |   6 +
 drivers/spi/Makefile   |   1 +
 drivers/spi/spi-mxic.c | 526 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 533 insertions(+)
 create mode 100644 drivers/spi/spi-mxic.c

Comments

Mark Brown Aug. 2, 2018, 10:07 a.m. UTC | #1
On Thu, Aug 02, 2018 at 10:07:49AM +0800, masonccyang@mxic.com.tw wrote:

This looks mostly good, one small technical thing and a style nit though:

> +++ b/drivers/spi/spi-mxic.c
> @@ -0,0 +1,526 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Macronix International Co., Ltd.

Please make the entire comment a C++ one, it makes things look more
intentional.

> +int mxic_spi_setup(struct spi_device *spi)
> +{
> +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);

> +	clk_prepare_enable(mxic->send_clk);
> +	clk_prepare_enable(mxic->send_dly_clk);

This will enable the clocks every time setup() is called (without
checking for errors!) but there's no corresponding disables so we're
leaking references to the clock enable.  What I'd recommend here is
moving the clock enables to runtime PM if you can, or just doing it in
probe if there's some problem with that.  Runtime PM should be slightly
better for power management but so slightly that it's not worth worrying
too much.
Trent Piepho Aug. 2, 2018, 7:27 p.m. UTC | #2
On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:
> +
> +#define GPIO			0xc4
> +#define GPIO_PT(x)		BIT(3 + ((x) * 16))
> +#define GPIO_RESET(x)		BIT(2 + ((x) * 16))
> +#define GPIO_HOLDB(x)		BIT(1 + ((x) * 16))
> +#define GPIO_WPB(x)		BIT((x) * 16)

Is this one of the device's registers?  Doesn't seem to be used.  In
fact, most of the registers that were defined here are not used.  The
above look strange.  Do you mean GPIO_RESET(0) is bit 2 and
GPIO_RESET(1) is bit 18?  There are just two gpios?  Or maybe, since
each GPIO has four bits associated with it, that should be x*4 instead
of x*16?

> +
> +#define HC_VER			0xd0
> +
> +#define HW_TEST(x)		(0xe0 + ((x) * 4))
> +
> +struct mxic_spi {
> +	struct clk *ps_clk;
> +	struct clk *send_clk;
> +	struct clk *send_dly_clk;
> +	void __iomem *regs;
> +	struct {
> +		void __iomem *map;
> +		dma_addr_t dma;
> +	} linear;

Nothing here uses linear.

> 
> +
> +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> +			      void *rxbuf, unsigned int len)
> +{
> +	unsigned int pos = 0;
> +
> +	while (pos < len) {
> +		unsigned int nbytes = len - pos;
> +		u32 data = 0xffffffff;
> +		u32 sts;
> +		int ret;
> +
> +		if (nbytes > 4)
> +			nbytes = 4;
> +
> +		if (txbuf)
> +			memcpy(&data, txbuf + pos, nbytes);
> +
> +		ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
> +					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> +		if (ret)
> +			return ret;
> +
> +		writel(data, mxic->regs + TXD(nbytes % 4));

This looks strange.  To send one byte of data, you write a 32-bit word
to the register at 0x18.  To send two bytes, a 32-bit word is written
to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14.  So
there are four TX data registers, each 32 bits wide, and which register
is written to controls how many bytes are sent?

> 
> +
> +int mxic_spi_setup(struct spi_device *spi)
> +{
> +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> +	unsigned long freq = spi->max_speed_hz;
> +	int ret;
> +
> +	ret = clk_set_rate(mxic->send_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_set_rate(mxic->send_dly_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	mxic_spi_set_input_delay_dqs(mxic, 0xf);
> +	ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));

Can also be written as freq*9/25000000.

> +	if (ret)
> +		return ret;
> +
> +	clk_prepare_enable(mxic->send_clk);
> +	clk_prepare_enable(mxic->send_dly_clk);
> +
> +	return 0;

Won't changing the clock rate in spi_setup break if there are two
slaves which use different rates?

>
Mason Yang Aug. 3, 2018, 7:06 a.m. UTC | #3
Hi Trent,


Trent Piepho <tpiepho@impinj.com> 已在 2018/08/03 上午 03:27:45 上寫入: 

> Trent Piepho <tpiepho@impinj.com> 
> 2018/08/03 上午 03:28
> 
> To
> 
> "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>, 
> "masonccyang@mxic.com.tw" <masonccyang@mxic.com.tw>, 
> "broonie@kernel.org" <broonie@kernel.org>, 
> 
> cc
> 
> "boris.brezillon@bootlin.com" <boris.brezillon@bootlin.com>, 
> "zhengxunli@mxic.com.tw" <zhengxunli@mxic.com.tw>, 
> "juliensu@mxic.com.tw" <juliensu@mxic.com.tw>
> 
> Subject
> 
> Re: [PATCH 1/2] spi: Add MXIC controller driver
> 
> On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:
> > +
> > +#define GPIO         0xc4
> > +#define GPIO_PT(x)      BIT(3 + ((x) * 16))
> > +#define GPIO_RESET(x)      BIT(2 + ((x) * 16))
> > +#define GPIO_HOLDB(x)      BIT(1 + ((x) * 16))
> > +#define GPIO_WPB(x)      BIT((x) * 16)
> 
> Is this one of the device's registers?  Doesn't seem to be used.  In
> fact, most of the registers that were defined here are not used.  The
> above look strange.  Do you mean GPIO_RESET(0) is bit 2 and
> GPIO_RESET(1) is bit 18?  There are just two gpios?  Or maybe, since
> each GPIO has four bits associated with it, that should be x*4 instead
> of x*16?
> 

Actually, our flash host controller supports serial and parallel flash, 
including NOR and NAND. Therefore, we defined all of device's registers 
here.
Up to two flash devices can be controlled by host controller.

The detail description of GPIO as follow:




> > +
> > +#define HC_VER         0xd0
> > +
> > +#define HW_TEST(x)      (0xe0 + ((x) * 4))
> > +
> > +struct mxic_spi {
> > +   struct clk *ps_clk;
> > +   struct clk *send_clk;
> > +   struct clk *send_dly_clk;
> > +   void __iomem *regs;
> > +   struct {
> > +      void __iomem *map;
> > +      dma_addr_t dma;
> > +   } linear;
> 
> Nothing here uses linear.

Our host controller operates in I/O mode, Linear read mode and DMA-slave 
mode.
Linear read mode is going to be patched once direct mapping mode is ready 
in spi-mem layer.

> 
> > 
> > +
> > +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void 
*txbuf,
> > +               void *rxbuf, unsigned int len)
> > +{
> > +   unsigned int pos = 0;
> > +
> > +   while (pos < len) {
> > +      unsigned int nbytes = len - pos;
> > +      u32 data = 0xffffffff;
> > +      u32 sts;
> > +      int ret;
> > +
> > +      if (nbytes > 4)
> > +         nbytes = 4;
> > +
> > +      if (txbuf)
> > +         memcpy(&data, txbuf + pos, nbytes);
> > +
> > +      ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
> > +                sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> > +      if (ret)
> > +         return ret;
> > +
> > +      writel(data, mxic->regs + TXD(nbytes % 4));
> 
> This looks strange.  To send one byte of data, you write a 32-bit word
> to the register at 0x18.  To send two bytes, a 32-bit word is written
> to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14.  So
> there are four TX data registers, each 32 bits wide, and which register
> is written to controls how many bytes are sent?
> 

yes, this is our host controller design.


> > 
> > +
> > +int mxic_spi_setup(struct spi_device *spi)
> > +{
> > +   struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> > +   unsigned long freq = spi->max_speed_hz;
> > +   int ret;
> > +
> > +   ret = clk_set_rate(mxic->send_clk, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   ret = clk_set_rate(mxic->send_dly_clk, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   mxic_spi_set_input_delay_dqs(mxic, 0xf);
> > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / 
freq));
> 
> Can also be written as freq*9/25000000.
> 
> > +   if (ret)
> > +      return ret;
> > +
> > +   clk_prepare_enable(mxic->send_clk);
> > +   clk_prepare_enable(mxic->send_dly_clk);
> > +
> > +   return 0;
> 
> Won't changing the clock rate in spi_setup break if there are two
> slaves which use different rates?
> 
> > 

SCLK is connected to two flash devices in our design.



thanks & best regards,
Mason

 



CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
<tt><font size=2>Hi Trent,</font></tt>
<br>
<br>
<br><tt><font size=2>Trent Piepho &lt;tpiepho@impinj.com&gt; 已在 2018/08/03
上午 03:27:45 上寫入: <br>
<br>
&gt; Trent Piepho &lt;tpiepho@impinj.com&gt; </font></tt>
<br><tt><font size=2>&gt; 2018/08/03 上午 03:28</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; To</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; &quot;linux-spi@vger.kernel.org&quot; &lt;linux-spi@vger.kernel.org&gt;,
<br>
&gt; &quot;masonccyang@mxic.com.tw&quot; &lt;masonccyang@mxic.com.tw&gt;,
<br>
&gt; &quot;broonie@kernel.org&quot; &lt;broonie@kernel.org&gt;, </font></tt>
<br><tt><font size=2>&gt; <br>
&gt; cc</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; &quot;boris.brezillon@bootlin.com&quot; &lt;boris.brezillon@bootlin.com&gt;,
<br>
&gt; &quot;zhengxunli@mxic.com.tw&quot; &lt;zhengxunli@mxic.com.tw&gt;,
<br>
&gt; &quot;juliensu@mxic.com.tw&quot; &lt;juliensu@mxic.com.tw&gt;</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; Subject</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; Re: [PATCH 1/2] spi: Add MXIC controller driver</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:<br>
&gt; &gt; +<br>
&gt; &gt; +#define GPIO &nbsp; &nbsp; &nbsp; &nbsp; 0xc4<br>
&gt; &gt; +#define GPIO_PT(x) &nbsp; &nbsp; &nbsp;BIT(3 + ((x) * 16))<br>
&gt; &gt; +#define GPIO_RESET(x) &nbsp; &nbsp; &nbsp;BIT(2 + ((x) * 16))<br>
&gt; &gt; +#define GPIO_HOLDB(x) &nbsp; &nbsp; &nbsp;BIT(1 + ((x) * 16))<br>
&gt; &gt; +#define GPIO_WPB(x) &nbsp; &nbsp; &nbsp;BIT((x) * 16)<br>
&gt; <br>
&gt; Is this one of the device's registers? &nbsp;Doesn't seem to be used.
&nbsp;In<br>
&gt; fact, most of the registers that were defined here are not used. &nbsp;The<br>
&gt; above look strange. &nbsp;Do you mean GPIO_RESET(0) is bit 2 and<br>
&gt; GPIO_RESET(1) is bit 18? &nbsp;There are just two gpios? &nbsp;Or
maybe, since<br>
&gt; each GPIO has four bits associated with it, that should be x*4 instead<br>
&gt; of x*16?<br>
&gt; </font></tt>
<br>
<br><tt><font size=2>Actually, our flash host controller supports serial
and parallel flash, </font></tt>
<br><tt><font size=2>including NOR and NAND. Therefore, we defined all
of device's registers here.</font></tt>
<br><tt><font size=2>Up to two flash devices can be controlled by host
controller.</font></tt>
<br>
<br><tt><font size=2>The detail description of GPIO as follow:</font></tt>
<br>
<br><img src=cid:_1_0B937B8C0B9375A000270E92482582DE>
<br>
<br><tt><font size=2><br>
&gt; &gt; +<br>
&gt; &gt; +#define HC_VER &nbsp; &nbsp; &nbsp; &nbsp; 0xd0<br>
&gt; &gt; +<br>
&gt; &gt; +#define HW_TEST(x) &nbsp; &nbsp; &nbsp;(0xe0 + ((x) * 4))<br>
&gt; &gt; +<br>
&gt; &gt; +struct mxic_spi {<br>
&gt; &gt; + &nbsp; struct clk *ps_clk;<br>
&gt; &gt; + &nbsp; struct clk *send_clk;<br>
&gt; &gt; + &nbsp; struct clk *send_dly_clk;<br>
&gt; &gt; + &nbsp; void __iomem *regs;<br>
&gt; &gt; + &nbsp; struct {<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;void __iomem *map;<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;dma_addr_t dma;<br>
&gt; &gt; + &nbsp; } linear;<br>
&gt; <br>
&gt; Nothing here uses linear.</font></tt>
<br>
<br><tt><font size=2>Our host controller operates in I/O mode, Linear read
mode and DMA-slave mode.</font></tt>
<br><tt><font size=2>Linear read mode is going to be patched once direct
mapping mode is ready </font></tt>
<br><tt><font size=2>in spi-mem layer.</font></tt>
<br><tt><font size=2><br>
&gt; <br>
&gt; &gt; <br>
&gt; &gt; +<br>
&gt; &gt; +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void
*txbuf,<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; void *rxbuf,
unsigned int len)<br>
&gt; &gt; +{<br>
&gt; &gt; + &nbsp; unsigned int pos = 0;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; while (pos &lt; len) {<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;unsigned int nbytes = len - pos;<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;u32 data = 0xffffffff;<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;u32 sts;<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;int ret;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;if (nbytes &gt; 4)<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp; &nbsp; nbytes = 4;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;if (txbuf)<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp; &nbsp; memcpy(&amp;data, txbuf + pos,
nbytes);<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;ret = readl_poll_timeout(mxic-&gt;regs
+ INT_STS, sts,<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;sts
&amp; INT_TX_EMPTY, 0, USEC_PER_SEC);<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;if (ret)<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp; &nbsp; return ret;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;writel(data, mxic-&gt;regs + TXD(nbytes
% 4));<br>
&gt; <br>
&gt; This looks strange. &nbsp;To send one byte of data, you write a 32-bit
word<br>
&gt; to the register at 0x18. &nbsp;To send two bytes, a 32-bit word is
written<br>
&gt; to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14. &nbsp;So<br>
&gt; there are four TX data registers, each 32 bits wide, and which register<br>
&gt; is written to controls how many bytes are sent?<br>
&gt; </font></tt>
<br>
<br><tt><font size=2>yes, this is our host controller design.</font></tt>
<br>
<br><tt><font size=2><br>
&gt; &gt; <br>
&gt; &gt; +<br>
&gt; &gt; +int mxic_spi_setup(struct spi_device *spi)<br>
&gt; &gt; +{<br>
&gt; &gt; + &nbsp; struct mxic_spi *mxic = spi_master_get_devdata(spi-&gt;master);<br>
&gt; &gt; + &nbsp; unsigned long freq = spi-&gt;max_speed_hz;<br>
&gt; &gt; + &nbsp; int ret;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; ret = clk_set_rate(mxic-&gt;send_clk, freq);<br>
&gt; &gt; + &nbsp; if (ret)<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;return ret;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; ret = clk_set_rate(mxic-&gt;send_dly_clk, freq);<br>
&gt; &gt; + &nbsp; if (ret)<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;return ret;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; mxic_spi_set_input_delay_dqs(mxic, 0xf);<br>
&gt; &gt; + &nbsp; ret = clk_set_phase(mxic-&gt;send_dly_clk, 360 / (1000000000
/ freq));<br>
&gt; <br>
&gt; Can also be written as freq*9/25000000.<br>
&gt; <br>
&gt; &gt; + &nbsp; if (ret)<br>
&gt; &gt; + &nbsp; &nbsp; &nbsp;return ret;<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; clk_prepare_enable(mxic-&gt;send_clk);<br>
&gt; &gt; + &nbsp; clk_prepare_enable(mxic-&gt;send_dly_clk);<br>
&gt; &gt; +<br>
&gt; &gt; + &nbsp; return 0;<br>
&gt; <br>
&gt; Won't changing the clock rate in spi_setup break if there are two<br>
&gt; slaves which use different rates?<br>
&gt; <br>
&gt; &gt; </font></tt>
<br>
<br><tt><font size=2>SCLK is connected to two flash devices in our design.</font></tt>
<br>
<br><img src=cid:_1_0B94E56C0B94E11400270E92482582DE>
<br>
<br><tt><font size=2>thanks &amp; best regards,</font></tt>
<br><tt><font size=2>Mason</font></tt>
<br>
<br><tt><font size=2>&nbsp;</font></tt>
<br>
<br><tt><font size=2><br>
<br>
CONFIDENTIALITY NOTE:<br>
<br>
This e-mail and any attachments may contain confidential information and/or
personal data, which is protected by applicable laws. Please be reminded
that duplication, disclosure, distribution, or use of this e-mail (and/or
its attachments) or any part thereof is prohibited. If you receive this
e-mail in error, please notify us immediately and delete this mail as well
as its attachment(s) from your system. In addition, please be informed
that collection, processing, and/or use of personal data is prohibited
unless expressly permitted by personal data protection laws. Thank you
for your attention and cooperation.<br>
<br>
Macronix International Co., Ltd.<br>
<br>
=====================================================================<br>
</font></tt>
Mason Yang Aug. 3, 2018, 7:29 a.m. UTC | #4
Hi Mark,
Thanks for your comments.


Mark Brown <broonie@kernel.org> 已在 2018/08/02 下午 06:07:41 上寫入: 

> Mark Brown <broonie@kernel.org> 
> 2018/08/02 下午 06:07
> 
> To
> 
> masonccyang@mxic.com.tw, 
> 
> cc
> 
> linux-spi@vger.kernel.org, Boris Brezillon 
> <boris.brezillon@bootlin.com>, juliensu@mxic.com.tw, 
zhengxunli@mxic.com.tw
> 
> Subject
> 
> Re: [PATCH 1/2] spi: Add MXIC controller driver
> 
> On Thu, Aug 02, 2018 at 10:07:49AM +0800, masonccyang@mxic.com.tw wrote:
> 
> This looks mostly good, one small technical thing and a style nit 
though:
> 
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -0,0 +1,526 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 Macronix International Co., Ltd.
> 
> Please make the entire comment a C++ one, it makes things look more
> intentional.

I patched it as follows:

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index b786327..89889b0 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (C) 2018 Macronix International Co., Ltd.
  *
@@ -423,8 +423,13 @@ int mxic_spi_setup(struct spi_device *spi)
        if (ret)
                return ret;

-       clk_prepare_enable(mxic->send_clk);
+       ret = clk_prepare_enable(mxic->send_clk);
+       if (ret)
+               return ret;
+
        clk_prepare_enable(mxic->send_dly_clk);
+       if (ret)
+               return ret;

        return 0;
 }

is it OK?


> 
> > +int mxic_spi_setup(struct spi_device *spi)
> > +{
> > +   struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> 
> > +   clk_prepare_enable(mxic->send_clk);
> > +   clk_prepare_enable(mxic->send_dly_clk);
> 
> This will enable the clocks every time setup() is called (without
> checking for errors!) but there's no corresponding disables so we're
> leaking references to the clock enable.  What I'd recommend here is
> moving the clock enables to runtime PM if you can, or just doing it in
> probe if there's some problem with that.  Runtime PM should be slightly
> better for power management but so slightly that it's not worth worrying
> too much.
> 

As I know that mxic_spi_setup() is called just one time in 
spi_register_master()
and the SCLK of our SPI Host is always in idle if there is no any 
cmd/addr/data
on bus.

I patched it as follows:

@@ -504,6 +509,13 @@ static int mxic_spi_remove(struct platform_device 
*pdev)
        return 0;
 }

+static void mxic_spi_shutdown(struct platform_device *pdev)
+{
+       struct mxic_spi *mxic = platform_get_drvdata(pdev);
+
+       clk_disable_unprepare(mxic->ps_clk);
+}
+
 static const struct of_device_id mxic_spi_of_ids[] = {
        { .compatible = "mxicy,mx25f0a-spi", },
        { /* sentinel */ }
@@ -513,6 +525,7 @@ static int mxic_spi_remove(struct platform_device 
*pdev)
 static struct platform_driver mxic_spi_driver = {
        .probe = mxic_spi_probe,
        .remove = mxic_spi_remove,
+       .shutdown = mxic_spi_shutdown,
        .driver = {
                .name = "mxic-spi",
                .of_match_table = mxic_spi_of_ids,




If you think it's OK and I will send a new patch file for your review.

thanks & best regards,
Mason 

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
<tt><font size=2>Hi Mark,</font></tt>
<br><tt><font size=2>Thanks for your comments.</font></tt>
<br>
<br>
<br><tt><font size=2>Mark Brown &lt;broonie@kernel.org&gt; 已在 2018/08/02
下午 06:07:41 上寫入: <br>
<br>
&gt; Mark Brown &lt;broonie@kernel.org&gt; </font></tt>
<br><tt><font size=2>&gt; 2018/08/02 下午 06:07</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; To</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; masonccyang@mxic.com.tw, </font></tt>
<br><tt><font size=2>&gt; <br>
&gt; cc</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; linux-spi@vger.kernel.org, Boris Brezillon <br>
&gt; &lt;boris.brezillon@bootlin.com&gt;, juliensu@mxic.com.tw, zhengxunli@mxic.com.tw</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; Subject</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; Re: [PATCH 1/2] spi: Add MXIC controller driver</font></tt>
<br><tt><font size=2>&gt; <br>
&gt; On Thu, Aug 02, 2018 at 10:07:49AM +0800, masonccyang@mxic.com.tw
wrote:<br>
&gt; <br>
&gt; This looks mostly good, one small technical thing and a style nit
though:<br>
&gt; <br>
&gt; &gt; +++ b/drivers/spi/spi-mxic.c<br>
&gt; &gt; @@ -0,0 +1,526 @@<br>
&gt; &gt; +// SPDX-License-Identifier: GPL-2.0<br>
&gt; &gt; +/*<br>
&gt; &gt; + * Copyright (C) 2018 Macronix International Co., Ltd.<br>
&gt; <br>
&gt; Please make the entire comment a C++ one, it makes things look more<br>
&gt; intentional.</font></tt>
<br>
<br><tt><font size=2>I patched it as follows:</font></tt>
<br>
<br><tt><font size=2>diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c</font></tt>
<br><tt><font size=2>index b786327..89889b0 100644</font></tt>
<br><tt><font size=2>--- a/drivers/spi/spi-mxic.c</font></tt>
<br><tt><font size=2>+++ b/drivers/spi/spi-mxic.c</font></tt>
<br><tt><font size=2>@@ -1,4 +1,4 @@</font></tt>
<br><tt><font size=2>-// SPDX-License-Identifier: GPL-2.0</font></tt>
<br><tt><font size=2>+/* SPDX-License-Identifier: GPL-2.0 */</font></tt>
<br><tt><font size=2>&nbsp;/*</font></tt>
<br><tt><font size=2>&nbsp; * Copyright (C) 2018 Macronix International
Co., Ltd.</font></tt>
<br><tt><font size=2>&nbsp; *</font></tt>
<br><tt><font size=2>@@ -423,8 +423,13 @@ int mxic_spi_setup(struct spi_device
*spi)</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; if (ret)</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
return ret;</font></tt>
<br>
<br><tt><font size=2>- &nbsp; &nbsp; &nbsp; clk_prepare_enable(mxic-&gt;send_clk);</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; ret = clk_prepare_enable(mxic-&gt;send_clk);</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; if (ret)</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
return ret;</font></tt>
<br><tt><font size=2>+</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; clk_prepare_enable(mxic-&gt;send_dly_clk);</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; if (ret)</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
return ret;</font></tt>
<br>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; return 0;</font></tt>
<br><tt><font size=2>&nbsp;}</font></tt>
<br>
<br><tt><font size=2>is it OK?</font></tt>
<br>
<br><tt><font size=2><br>
&gt; <br>
&gt; &gt; +int mxic_spi_setup(struct spi_device *spi)<br>
&gt; &gt; +{<br>
&gt; &gt; + &nbsp; struct mxic_spi *mxic = spi_master_get_devdata(spi-&gt;master);<br>
&gt; <br>
&gt; &gt; + &nbsp; clk_prepare_enable(mxic-&gt;send_clk);<br>
&gt; &gt; + &nbsp; clk_prepare_enable(mxic-&gt;send_dly_clk);<br>
&gt; <br>
&gt; This will enable the clocks every time setup() is called (without<br>
&gt; checking for errors!) but there's no corresponding disables so we're<br>
&gt; leaking references to the clock enable. &nbsp;What I'd recommend here
is<br>
&gt; moving the clock enables to runtime PM if you can, or just doing it
in<br>
&gt; probe if there's some problem with that. &nbsp;Runtime PM should be
slightly<br>
&gt; better for power management but so slightly that it's not worth worrying<br>
&gt; too much.<br>
&gt; </font></tt>
<br>
<br><tt><font size=2>As I know that mxic_spi_setup() is called just one
time in spi_register_master()</font></tt>
<br><tt><font size=2>and the SCLK of our SPI Host is always in idle if
there is no any cmd/addr/data</font></tt>
<br><tt><font size=2>on bus.</font></tt>
<br>
<br><tt><font size=2>I patched it as follows:</font></tt>
<br>
<br><tt><font size=2>@@ -504,6 +509,13 @@ static int mxic_spi_remove(struct
platform_device *pdev)</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; return 0;</font></tt>
<br><tt><font size=2>&nbsp;}</font></tt>
<br>
<br><tt><font size=2>+static void mxic_spi_shutdown(struct platform_device
*pdev)</font></tt>
<br><tt><font size=2>+{</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; struct mxic_spi *mxic = platform_get_drvdata(pdev);</font></tt>
<br><tt><font size=2>+</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; clk_disable_unprepare(mxic-&gt;ps_clk);</font></tt>
<br><tt><font size=2>+}</font></tt>
<br><tt><font size=2>+</font></tt>
<br><tt><font size=2>&nbsp;static const struct of_device_id mxic_spi_of_ids[]
= {</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; { .compatible = &quot;mxicy,mx25f0a-spi&quot;,
},</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; { /* sentinel */ }</font></tt>
<br><tt><font size=2>@@ -513,6 +525,7 @@ static int mxic_spi_remove(struct
platform_device *pdev)</font></tt>
<br><tt><font size=2>&nbsp;static struct platform_driver mxic_spi_driver
= {</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; .probe = mxic_spi_probe,</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; .remove = mxic_spi_remove,</font></tt>
<br><tt><font size=2>+ &nbsp; &nbsp; &nbsp; .shutdown = mxic_spi_shutdown,</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; .driver = {</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
.name = &quot;mxic-spi&quot;,</font></tt>
<br><tt><font size=2>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
.of_match_table = mxic_spi_of_ids,</font></tt>
<br>
<br>
<br>
<br>
<br><tt><font size=2>If you think it's OK and I will send a new patch file
for your review.</font></tt>
<br>
<br><tt><font size=2>thanks &amp; best regards,</font></tt>
<br><tt><font size=2>Mason <br>
<br>
CONFIDENTIALITY NOTE:<br>
<br>
This e-mail and any attachments may contain confidential information and/or
personal data, which is protected by applicable laws. Please be reminded
that duplication, disclosure, distribution, or use of this e-mail (and/or
its attachments) or any part thereof is prohibited. If you receive this
e-mail in error, please notify us immediately and delete this mail as well
as its attachment(s) from your system. In addition, please be informed
that collection, processing, and/or use of personal data is prohibited
unless expressly permitted by personal data protection laws. Thank you
for your attention and cooperation.<br>
<br>
Macronix International Co., Ltd.<br>
<br>
=====================================================================<br>
</font></tt>
Boris Brezillon Aug. 3, 2018, 7:36 a.m. UTC | #5
Hi Mason,

On Fri, 3 Aug 2018 15:06:34 +0800
masonccyang@mxic.com.tw wrote:

> Hi Trent,
> 
> 
> Trent Piepho <tpiepho@impinj.com> 已在 2018/08/03 上午 03:27:45 上寫入: 
> 
> > Trent Piepho <tpiepho@impinj.com> 
> > 2018/08/03 上午 03:28
> > 
> > To
> > 
> > "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>, 
> > "masonccyang@mxic.com.tw" <masonccyang@mxic.com.tw>, 
> > "broonie@kernel.org" <broonie@kernel.org>, 
> > 
> > cc
> > 
> > "boris.brezillon@bootlin.com" <boris.brezillon@bootlin.com>, 
> > "zhengxunli@mxic.com.tw" <zhengxunli@mxic.com.tw>, 
> > "juliensu@mxic.com.tw" <juliensu@mxic.com.tw>
> > 
> > Subject
> > 
> > Re: [PATCH 1/2] spi: Add MXIC controller driver
> > 
> > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:  
> > > +
> > > +#define GPIO         0xc4
> > > +#define GPIO_PT(x)      BIT(3 + ((x) * 16))
> > > +#define GPIO_RESET(x)      BIT(2 + ((x) * 16))
> > > +#define GPIO_HOLDB(x)      BIT(1 + ((x) * 16))
> > > +#define GPIO_WPB(x)      BIT((x) * 16)  
> > 
> > Is this one of the device's registers?  Doesn't seem to be used.  In
> > fact, most of the registers that were defined here are not used.  The
> > above look strange.  Do you mean GPIO_RESET(0) is bit 2 and
> > GPIO_RESET(1) is bit 18?
> > There are just two gpios?  Or maybe, since
> > each GPIO has four bits associated with it, that should be x*4 instead
> > of x*16?
> >   
> 
> Actually, our flash host controller supports serial and parallel flash, 
> including NOR and NAND. Therefore, we defined all of device's registers 
> here.
> Up to two flash devices can be controlled by host controller.
> 
> The detail description of GPIO as follow:
> 
> 

You forgot the description :-).

> 
> 
> > > +
> > > +#define HC_VER         0xd0
> > > +
> > > +#define HW_TEST(x)      (0xe0 + ((x) * 4))
> > > +
> > > +struct mxic_spi {
> > > +   struct clk *ps_clk;
> > > +   struct clk *send_clk;
> > > +   struct clk *send_dly_clk;
> > > +   void __iomem *regs;
> > > +   struct {
> > > +      void __iomem *map;
> > > +      dma_addr_t dma;
> > > +   } linear;  
> > 
> > Nothing here uses linear.  
> 
> Our host controller operates in I/O mode, Linear read mode and DMA-slave 
> mode.
> Linear read mode is going to be patched once direct mapping mode is ready 
> in spi-mem layer.

That's true, but those fields are not used yet, so better introduce
them when we actually need them (i.e. when adding support for direct
mapping).

> 
> >   
> > > 
> > > +
> > > +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void   
> *txbuf,
> > > +               void *rxbuf, unsigned int len)
> > > +{
> > > +   unsigned int pos = 0;
> > > +
> > > +   while (pos < len) {
> > > +      unsigned int nbytes = len - pos;
> > > +      u32 data = 0xffffffff;
> > > +      u32 sts;
> > > +      int ret;
> > > +
> > > +      if (nbytes > 4)
> > > +         nbytes = 4;
> > > +
> > > +      if (txbuf)
> > > +         memcpy(&data, txbuf + pos, nbytes);
> > > +
> > > +      ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
> > > +                sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> > > +      if (ret)
> > > +         return ret;
> > > +
> > > +      writel(data, mxic->regs + TXD(nbytes % 4));  
> > 
> > This looks strange.  To send one byte of data, you write a 32-bit word
> > to the register at 0x18.  To send two bytes, a 32-bit word is written
> > to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14.  So
> > there are four TX data registers, each 32 bits wide, and which register
> > is written to controls how many bytes are sent?
> >   
> 
> yes, this is our host controller design.
> 
> 
> > > 
> > > +
> > > +int mxic_spi_setup(struct spi_device *spi)
> > > +{
> > > +   struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> > > +   unsigned long freq = spi->max_speed_hz;
> > > +   int ret;
> > > +
> > > +   ret = clk_set_rate(mxic->send_clk, freq);
> > > +   if (ret)
> > > +      return ret;
> > > +
> > > +   ret = clk_set_rate(mxic->send_dly_clk, freq);
> > > +   if (ret)
> > > +      return ret;
> > > +
> > > +   mxic_spi_set_input_delay_dqs(mxic, 0xf);
> > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 /   
> freq));
> > 
> > Can also be written as freq*9/25000000.
> >   
> > > +   if (ret)
> > > +      return ret;
> > > +
> > > +   clk_prepare_enable(mxic->send_clk);
> > > +   clk_prepare_enable(mxic->send_dly_clk);
> > > +
> > > +   return 0;  
> > 
> > Won't changing the clock rate in spi_setup break if there are two
> > slaves which use different rates?
> >   
> > >   
> 
> SCLK is connected to two flash devices in our design.

Well, this is board dependent, and we should support all cases even if
your specific board design use the same setup for both CS lines.
Trent's point is that we should change the frequency (if needed) when
doing a transfer not in ->spi_setup().

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon Aug. 3, 2018, 7:42 a.m. UTC | #6
Hi Trent,

On Thu, 2 Aug 2018 19:27:45 +0000
Trent Piepho <tpiepho@impinj.com> wrote:

> On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:
> > +
> > +#define GPIO			0xc4
> > +#define GPIO_PT(x)		BIT(3 + ((x) * 16))
> > +#define GPIO_RESET(x)		BIT(2 + ((x) * 16))
> > +#define GPIO_HOLDB(x)		BIT(1 + ((x) * 16))
> > +#define GPIO_WPB(x)		BIT((x) * 16)  
> 
> Is this one of the device's registers?

If by device you mean SPI controller, then yes it is.

> Doesn't seem to be used.  In
> fact, most of the registers that were defined here are not used.  The
> above look strange.  Do you mean GPIO_RESET(0) is bit 2 and
> GPIO_RESET(1) is bit 18?

It is correct. We have 2 sets of 4 pins (one set of pin per CS line),
so x is 0 or 1.

> There are just two gpios?

Nope 2 sets of GPIO and each set contains 4 GPIOs.

> Or maybe, since
> each GPIO has four bits associated with it, that should be x*4 instead
> of x*16?

Nope, the current GPIO_XXX defs are correct.

> 
> > +
> > +#define HC_VER			0xd0
> > +
> > +#define HW_TEST(x)		(0xe0 + ((x) * 4))
> > +
> > +struct mxic_spi {
> > +	struct clk *ps_clk;
> > +	struct clk *send_clk;
> > +	struct clk *send_dly_clk;
> > +	void __iomem *regs;
> > +	struct {
> > +		void __iomem *map;
> > +		dma_addr_t dma;
> > +	} linear;  
> 
> Nothing here uses linear.

Yep, we'll drop it.

> 
> > 
> > +
> > +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> > +			      void *rxbuf, unsigned int len)
> > +{
> > +	unsigned int pos = 0;
> > +
> > +	while (pos < len) {
> > +		unsigned int nbytes = len - pos;
> > +		u32 data = 0xffffffff;
> > +		u32 sts;
> > +		int ret;
> > +
> > +		if (nbytes > 4)
> > +			nbytes = 4;
> > +
> > +		if (txbuf)
> > +			memcpy(&data, txbuf + pos, nbytes);
> > +
> > +		ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
> > +					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> > +		if (ret)
> > +			return ret;
> > +
> > +		writel(data, mxic->regs + TXD(nbytes % 4));  
> 
> This looks strange.  To send one byte of data, you write a 32-bit word
> to the register at 0x18.  To send two bytes, a 32-bit word is written
> to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14.  So
> there are four TX data registers, each 32 bits wide, and which register
> is written to controls how many bytes are sent?

As pointed by Mason, it's not a mistake, the controller really work
like that.

> 
> > 
> > +
> > +int mxic_spi_setup(struct spi_device *spi)
> > +{
> > +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> > +	unsigned long freq = spi->max_speed_hz;
> > +	int ret;
> > +
> > +	ret = clk_set_rate(mxic->send_clk, freq);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_set_rate(mxic->send_dly_clk, freq);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mxic_spi_set_input_delay_dqs(mxic, 0xf);
> > +	ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));  
> 
> Can also be written as freq*9/25000000.

I'd like to keep 360 here. The phase is in degree, hence the 360. The
compiler should be smart enough to optimize that at compilation
time ;-).

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_prepare_enable(mxic->send_clk);
> > +	clk_prepare_enable(mxic->send_dly_clk);
> > +
> > +	return 0;  
> 
> Won't changing the clock rate in spi_setup break if there are two
> slaves which use different rates?

Correct. We'll calculate the expected phase and clk rate, keep an
internal cache in the spi controller struct and change it if needed
when a transfer is done or a spi_mem_op is executed.

Thanks for your review.

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon Aug. 3, 2018, 7:44 a.m. UTC | #7
On Thu, 2 Aug 2018 11:07:41 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Aug 02, 2018 at 10:07:49AM +0800, masonccyang@mxic.com.tw wrote:
> 
> This looks mostly good, one small technical thing and a style nit though:
> 
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -0,0 +1,526 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 Macronix International Co., Ltd.  
> 
> Please make the entire comment a C++ one, it makes things look more
> intentional.

Sure, we will.

> 
> > +int mxic_spi_setup(struct spi_device *spi)
> > +{
> > +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);  
> 
> > +	clk_prepare_enable(mxic->send_clk);
> > +	clk_prepare_enable(mxic->send_dly_clk);  
> 
> This will enable the clocks every time setup() is called (without
> checking for errors!) but there's no corresponding disables so we're
> leaking references to the clock enable.  What I'd recommend here is
> moving the clock enables to runtime PM if you can, or just doing it in
> probe if there's some problem with that.  Runtime PM should be slightly
> better for power management but so slightly that it's not worth worrying
> too much.

I think we'll go for the runtime PM approach.

Thanks for your review.

Boris

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon Aug. 3, 2018, 7:52 a.m. UTC | #8
Hi Mason,

On Fri, 3 Aug 2018 15:29:02 +0800
masonccyang@mxic.com.tw wrote:

> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index b786327..89889b0 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -1,4 +1,4 @@
> -// SPDX-License-Identifier: GPL-2.0
> +/* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (C) 2018 Macronix International Co., Ltd.
>   *

Actually Mark asked you to do the opposite:

// SPDX-License-Identifier: GPL-2.0
//
// Copyright (C) 2018 Macronix International Co., Ltd.
// ...
// ...

> @@ -423,8 +423,13 @@ int mxic_spi_setup(struct spi_device *spi)
>         if (ret)
>                 return ret;
> 
> -       clk_prepare_enable(mxic->send_clk);
> +       ret = clk_prepare_enable(mxic->send_clk);
> +       if (ret)
> +               return ret;
> +
>         clk_prepare_enable(mxic->send_dly_clk);
> +       if (ret)
> +               return ret;

You should disable mxic->send_clk in case
prepare_enabled fails of mxic->send_dly_clk.

Anyway, that'

> 
>         return 0;
>  }
> 
> is it OK?
> 
> 
> >   
> > > +int mxic_spi_setup(struct spi_device *spi)
> > > +{
> > > +   struct mxic_spi *mxic = spi_master_get_devdata(spi->master);  
> >   
> > > +   clk_prepare_enable(mxic->send_clk);
> > > +   clk_prepare_enable(mxic->send_dly_clk);  
> > 
> > This will enable the clocks every time setup() is called (without
> > checking for errors!) but there's no corresponding disables so we're
> > leaking references to the clock enable.  What I'd recommend here is
> > moving the clock enables to runtime PM if you can, or just doing it in
> > probe if there's some problem with that.  Runtime PM should be slightly
> > better for power management but so slightly that it's not worth worrying
> > too much.
> >   
> 
> As I know that mxic_spi_setup() is called just one time in 
> spi_register_master()
> and the SCLK of our SPI Host is always in idle if there is no any 
> cmd/addr/data
> on bus.

That's not quite true. SPI device drivers can call spi_setup() on their
own, so this hook can called several times. I'd suggest moving that to
runtime PM hooks.

> 
> I patched it as follows:
> 
> @@ -504,6 +509,13 @@ static int mxic_spi_remove(struct platform_device 
> *pdev)
>         return 0;
>  }
> 
> +static void mxic_spi_shutdown(struct platform_device *pdev)
> +{
> +       struct mxic_spi *mxic = platform_get_drvdata(pdev);
> +
> +       clk_disable_unprepare(mxic->ps_clk);
> +}

I might be wrong, but I think you don't need special handling for
shutdown if you implement the runtime PM suspend/resume hooks.

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Aug. 3, 2018, 9:27 a.m. UTC | #9
On Thu, Aug 02, 2018 at 07:27:45PM +0000, Trent Piepho wrote:
> On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:

> > +	mxic_spi_set_input_delay_dqs(mxic, 0xf);
> > +	ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));

> Can also be written as freq*9/25000000.

> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_prepare_enable(mxic->send_clk);
> > +	clk_prepare_enable(mxic->send_dly_clk);
> > +
> > +	return 0;

> Won't changing the clock rate in spi_setup break if there are two
> slaves which use different rates?

Gah, I missed that the rate is set by clk_set_phase() - the
clk_set_rate() calls are setting constant values so they're fine.  Yes,
this is a bug.
Mark Brown Aug. 3, 2018, 9:30 a.m. UTC | #10
On Fri, Aug 03, 2018 at 03:29:02PM +0800, masonccyang@mxic.com.tw wrote:
> Mark Brown <broonie@kernel.org> 已在 2018/08/02 下午 06:07:41 上寫入: 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index b786327..89889b0 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -1,4 +1,4 @@
> -// SPDX-License-Identifier: GPL-2.0
> +/* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (C) 2018 Macronix International Co., Ltd.
>   *

Other way, change it all to C++.

> @@ -423,8 +423,13 @@ int mxic_spi_setup(struct spi_device *spi)
>         if (ret)
>                 return ret;
> 
> -       clk_prepare_enable(mxic->send_clk);
> +       ret = clk_prepare_enable(mxic->send_clk);
> +       if (ret)
> +               return ret;
> +
>         clk_prepare_enable(mxic->send_dly_clk);
> +       if (ret)
> +               return ret;
> 
>         return 0;
>  }
> 
> is it OK?

Like Boris says I was also asking for the enables to be moved out of
spi_setup(), and as Trent noticed there's an issue with the rate setting
too - that that should probably be moved into the transfer code.
Geert Uytterhoeven Aug. 3, 2018, 11:06 a.m. UTC | #11
Hi Boris,

On Fri, Aug 3, 2018 at 9:48 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Thu, 2 Aug 2018 19:27:45 +0000
> Trent Piepho <tpiepho@impinj.com> wrote:
> > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:
> > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));
> >
> > Can also be written as freq*9/25000000.
>
> I'd like to keep 360 here. The phase is in degree, hence the 360. The
> compiler should be smart enough to optimize that at compilation
> time ;-).

It may have a hard time optimizing that, while preserving the semantics
of the original expression (rounding behavior!)...

Speaking about that, there's a serious issue due to loss of accuracy when
doing two divisions like that. "360 * freq / 1000000000" is better, assumed
the multiplication will be done in 64-bit arithmetic to avoid overflow:

    u64 t = 360ULL * freq;
    ret = clk_set_phase(mxic->send_dly_clk, do_div(t, 1000000000));

BTW, can freq < 1000000000 / 360?

Gr{oetje,eeting}s,

                        Geert
Boris Brezillon Aug. 3, 2018, 12:44 p.m. UTC | #12
On Fri, 3 Aug 2018 13:06:46 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Boris,
> 
> On Fri, Aug 3, 2018 at 9:48 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Thu, 2 Aug 2018 19:27:45 +0000
> > Trent Piepho <tpiepho@impinj.com> wrote:  
> > > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:  
> > > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));  
> > >
> > > Can also be written as freq*9/25000000.  
> >
> > I'd like to keep 360 here. The phase is in degree, hence the 360. The
> > compiler should be smart enough to optimize that at compilation
> > time ;-).  
> 
> It may have a hard time optimizing that, while preserving the semantics
> of the original expression (rounding behavior!)...
> 
> Speaking about that, there's a serious issue due to loss of accuracy when
> doing two divisions like that. "360 * freq / 1000000000" is better, assumed
> the multiplication will be done in 64-bit arithmetic to avoid overflow:
> 
>     u64 t = 360ULL * freq;
>     ret = clk_set_phase(mxic->send_dly_clk, do_div(t, 1000000000));

True.

> 
> BTW, can freq < 1000000000 / 360?

Theoretically yes, in practice, that's unlikely.

Anyway, I still don't quite understand where this formula comes from,
so I'll let Mason details why the phase has to be set to this value...
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trent Piepho Aug. 6, 2018, 5:36 p.m. UTC | #13
On Fri, 2018-08-03 at 13:06 +0200, Geert Uytterhoeven wrote:
> Hi Boris,
> 
> On Fri, Aug 3, 2018 at 9:48 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Thu, 2 Aug 2018 19:27:45 +0000
> > Trent Piepho <tpiepho@impinj.com> wrote:
> > > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@mxic.com.tw wrote:
> > > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));
> > > 
> > > Can also be written as freq*9/25000000.
> > 
> > I'd like to keep 360 here. The phase is in degree, hence the 360. The
> > compiler should be smart enough to optimize that at compilation
> > time ;-).
> 
> It may have a hard time optimizing that, while preserving the semantics
> of the original expression (rounding behavior!)...

Yes, that is the issue.  Two integer divisions must be done.

> 
> Speaking about that, there's a serious issue due to loss of accuracy when
> doing two divisions like that. "360 * freq / 1000000000" is better, assumed
> the multiplication will be done in 64-bit arithmetic to avoid overflow:

That's why I suggested 9 * freq / 25000000.  Take the factor of 40 out
of both sides to raise the freq needed to overflow to above the max
speed of the controller.  It doesn't change the result to do that.

gcc ends up being smart enough to optimize this into:

((freq + freq << 3) * 1441151881) >> 55

to avoid multiplication by 9 ("add r0,r0,r0,lsl #3") and turn the
division into a multiply and shift.  Division is slow.
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..7e900b5 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -633,6 +633,12 @@  config SPI_SUN6I
 	help
 	  This enables using the SPI controller on the Allwinner A31 SoCs.
 
+config SPI_MXIC
+        tristate "Macronix MX25F0A SPI controller"
+        depends on SPI_MASTER
+        help
+          This selects the Macronix MX25F0A SPI controller driver.
+
 config SPI_MXS
 	tristate "Freescale MXS SPI controller"
 	depends on ARCH_MXS
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..d7a1ceb 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -57,6 +57,7 @@  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_MXIC)			+= spi-mxic.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-mxic.c b/drivers/spi/spi-mxic.c
new file mode 100644
index 0000000..b786327
--- /dev/null
+++ b/drivers/spi/spi-mxic.c
@@ -0,0 +1,526 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Macronix International Co., Ltd.
+ *
+ * Authors:
+ *	Mason Yang <masonccyang@mxic.com.tw>
+ *	zhengxunli <zhengxunli@mxic.com.tw>
+ *	Boris Brezillon <boris.brezillon@bootlin.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+#define HC_CFG			0x0
+#define HC_CFG_IF_CFG(x)	((x) << 27)
+#define HC_CFG_DUAL_SLAVE	BIT(31)
+#define HC_CFG_INDIVIDUAL	BIT(30)
+#define HC_CFG_NIO(x)		(((x) / 4) << 27)
+#define HC_CFG_TYPE(s, t)	((t) << (23 + ((s) * 2)))
+#define HC_CFG_TYPE_SPI_NOR	0
+#define HC_CFG_TYPE_SPI_NAND	1
+#define HC_CFG_TYPE_SPI_RAM	2
+#define HC_CFG_TYPE_RAW_NAND	3
+#define HC_CFG_SLV_ACT(x)	((x) << 21)
+#define HC_CFG_CLK_PH_EN	BIT(20)
+#define HC_CFG_CLK_POL_INV	BIT(19)
+#define HC_CFG_BIG_ENDIAN	BIT(18)
+#define HC_CFG_DATA_PASS	BIT(17)
+#define HC_CFG_IDLE_SIO_LVL(x)	((x) << 16)
+#define HC_CFG_MAN_START_EN	BIT(3)
+#define HC_CFG_MAN_START	BIT(2)
+#define HC_CFG_MAN_CS_EN	BIT(1)
+#define HC_CFG_MAN_CS_ASSERT	BIT(0)
+
+#define INT_STS			0x4
+#define INT_STS_EN		0x8
+#define INT_SIG_EN		0xc
+#define INT_STS_ALL		GENMASK(31, 0)
+#define INT_RDY_PIN		BIT(26)
+#define INT_RDY_SR		BIT(25)
+#define INT_LNR_SUSP		BIT(24)
+#define INT_ECC_ERR		BIT(17)
+#define INT_CRC_ERR		BIT(16)
+#define INT_LWR_DIS		BIT(12)
+#define INT_LRD_DIS		BIT(11)
+#define INT_SDMA_INT		BIT(10)
+#define INT_DMA_FINISH		BIT(9)
+#define INT_RX_NOT_FULL		BIT(3)
+#define INT_RX_NOT_EMPTY	BIT(2)
+#define INT_TX_NOT_FULL		BIT(1)
+#define INT_TX_EMPTY		BIT(0)
+
+#define HC_EN			0x10
+#define HC_EN_BIT		BIT(0)
+
+#define TXD(x)			(0x14 + ((x) * 4))
+#define RXD			0x24
+
+#define SS_CTRL(s)		(0x30 + ((s) * 4))
+#define LRD_CFG			0x44
+#define LWR_CFG			0x80
+#define RWW_CFG			0x70
+#define OP_READ			BIT(23)
+#define OP_DUMMY_CYC(x)		((x) << 17)
+#define OP_ADDR_BYTES(x)	((x) << 14)
+#define OP_CMD_BYTES(x)		(((x) - 1) << 13)
+#define OP_OCTA_CRC_EN		BIT(12)
+#define OP_DQS_EN		BIT(11)
+#define OP_ENHC_EN		BIT(10)
+#define OP_PREAMBLE_EN		BIT(9)
+#define OP_DATA_DDR		BIT(8)
+#define OP_DATA_BUSW(x)		((x) << 6)
+#define OP_ADDR_DDR		BIT(5)
+#define OP_ADDR_BUSW(x)		((x) << 3)
+#define OP_CMD_DDR		BIT(2)
+#define OP_CMD_BUSW(x)		(x)
+#define OP_BUSW_1		0
+#define OP_BUSW_2		1
+#define OP_BUSW_4		2
+#define OP_BUSW_8		3
+
+#define OCTA_CRC		0x38
+#define OCTA_CRC_IN_EN(s)	BIT(3 + ((s) * 16))
+#define OCTA_CRC_CHUNK(s, x)	((fls((x) / 32)) << (1 + ((s) * 16)))
+#define OCTA_CRC_OUT_EN(s)	BIT(0 + ((s) * 16))
+
+#define ONFI_DIN_CNT(s)		(0x3c + (s))
+
+#define LRD_CTRL		0x48
+#define RWW_CTRL		0x74
+#define LWR_CTRL		0x84
+#define LMODE_EN		BIT(31)
+#define LMODE_SLV_ACT(x)	((x) << 21)
+#define LMODE_CMD1(x)		((x) << 8)
+#define LMODE_CMD0(x)		(x)
+
+#define LRD_ADDR		0x4c
+#define LWR_ADDR		0x88
+#define LRD_RANGE		0x50
+#define LWR_RANGE		0x8c
+
+#define AXI_SLV_ADDR		0x54
+
+#define DMAC_RD_CFG		0x58
+#define DMAC_WR_CFG		0x94
+#define DMAC_CFG_PERIPH_EN	BIT(31)
+#define DMAC_CFG_ALLFLUSH_EN	BIT(30)
+#define DMAC_CFG_LASTFLUSH_EN	BIT(29)
+#define DMAC_CFG_QE(x)		(((x) + 1) << 16)
+#define DMAC_CFG_BURST_LEN(x)	(((x) + 1) << 12)
+#define DMAC_CFG_BURST_SZ(x)	((x) << 8)
+#define DMAC_CFG_DIR_READ	BIT(1)
+#define DMAC_CFG_START		BIT(0)
+
+#define DMAC_RD_CNT		0x5c
+#define DMAC_WR_CNT		0x98
+
+#define SDMA_ADDR		0x60
+
+#define DMAM_CFG		0x64
+#define DMAM_CFG_START		BIT(31)
+#define DMAM_CFG_CONT		BIT(30)
+#define DMAM_CFG_SDMA_GAP(x)	(fls((x) / 8192) << 2)
+#define DMAM_CFG_DIR_READ	BIT(1)
+#define DMAM_CFG_EN		BIT(0)
+
+#define DMAM_CNT		0x68
+
+#define LNR_TIMER_TH		0x6c
+
+#define RDM_CFG0		0x78
+#define RDM_CFG0_POLY(x)	(x)
+
+#define RDM_CFG1		0x7c
+#define RDM_CFG1_RDM_EN		BIT(31)
+#define RDM_CFG1_SEED(x)	(x)
+
+#define LWR_SUSP_CTRL		0x90
+#define LWR_SUSP_CTRL_EN	BIT(31)
+
+#define DMAS_CTRL		0x9c
+#define DMAS_CTRL_DIR_READ	BIT(31)
+#define DMAS_CTRL_EN		BIT(30)
+
+#define DATA_STROB		0xa0
+#define DATA_STROB_EDO_EN	BIT(2)
+#define DATA_STROB_INV_POL	BIT(1)
+#define DATA_STROB_DELAY_2CYC	BIT(0)
+
+#define IDLY_CODE(x)		(0xa4 + ((x) * 4))
+#define IDLY_CODE_VAL(x, v)	((v) << (((x) % 4) * 8))
+
+#define GPIO			0xc4
+#define GPIO_PT(x)		BIT(3 + ((x) * 16))
+#define GPIO_RESET(x)		BIT(2 + ((x) * 16))
+#define GPIO_HOLDB(x)		BIT(1 + ((x) * 16))
+#define GPIO_WPB(x)		BIT((x) * 16)
+
+#define HC_VER			0xd0
+
+#define HW_TEST(x)		(0xe0 + ((x) * 4))
+
+struct mxic_spi {
+	struct clk *ps_clk;
+	struct clk *send_clk;
+	struct clk *send_dly_clk;
+	void __iomem *regs;
+	struct {
+		void __iomem *map;
+		dma_addr_t dma;
+	} linear;
+};
+
+static void mxic_spi_hw_init(struct mxic_spi *mxic)
+{
+	writel(0, mxic->regs + DATA_STROB);
+	writel(INT_STS_ALL, mxic->regs + INT_STS_EN);
+	writel(0, mxic->regs + HC_EN);
+	writel(0, mxic->regs + LRD_CFG);
+	writel(0, mxic->regs + LRD_CTRL);
+	writel(HC_CFG_NIO(1) | HC_CFG_TYPE(0, HC_CFG_TYPE_SPI_NAND) |
+	       HC_CFG_SLV_ACT(0) | HC_CFG_MAN_CS_EN | HC_CFG_IDLE_SIO_LVL(1),
+	       mxic->regs + HC_CFG);
+}
+
+static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
+			      void *rxbuf, unsigned int len)
+{
+	unsigned int pos = 0;
+
+	while (pos < len) {
+		unsigned int nbytes = len - pos;
+		u32 data = 0xffffffff;
+		u32 sts;
+		int ret;
+
+		if (nbytes > 4)
+			nbytes = 4;
+
+		if (txbuf)
+			memcpy(&data, txbuf + pos, nbytes);
+
+		ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
+		if (ret)
+			return ret;
+
+		writel(data, mxic->regs + TXD(nbytes % 4));
+
+		if (rxbuf) {
+			ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+						 sts & INT_TX_EMPTY, 0,
+						 USEC_PER_SEC);
+			if (ret)
+				return ret;
+
+			ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+						 sts & INT_RX_NOT_EMPTY, 0,
+						 USEC_PER_SEC);
+			if (ret)
+				return ret;
+
+			data = readl(mxic->regs + RXD);
+			data >>= (8 * (4 - nbytes));
+			memcpy(rxbuf + pos, &data, nbytes);
+			WARN_ON(readl(mxic->regs + INT_STS) & INT_RX_NOT_EMPTY);
+		} else {
+			readl(mxic->regs + RXD);
+		}
+		WARN_ON(readl(mxic->regs + INT_STS) & INT_RX_NOT_EMPTY);
+
+		pos += nbytes;
+	}
+
+	return 0;
+}
+
+static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
+				     const struct spi_mem_op *op)
+{
+	if (op->data.buswidth > 4 || op->addr.buswidth > 4 ||
+	    op->dummy.buswidth > 4 || op->cmd.buswidth > 4)
+		return false;
+
+	if (op->data.nbytes && op->dummy.nbytes &&
+	    op->data.buswidth != op->dummy.buswidth)
+		return false;
+
+	if (op->addr.nbytes > 7)
+		return false;
+
+	return true;
+}
+
+static int mxic_spi_mem_exec_op(struct spi_mem *mem,
+				const struct spi_mem_op *op)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(mem->spi->master);
+	int nio = 1, i, ret;
+	u32 ss_ctrl;
+	u8 addr[8];
+
+	if (mem->spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
+		nio = 4;
+	else if (mem->spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
+		nio = 2;
+
+	writel(HC_CFG_NIO(nio) |
+	       HC_CFG_TYPE(mem->spi->chip_select, HC_CFG_TYPE_SPI_NOR) |
+	       HC_CFG_SLV_ACT(mem->spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1) |
+	       HC_CFG_MAN_CS_EN,
+	       mxic->regs + HC_CFG);
+	writel(HC_EN_BIT, mxic->regs + HC_EN);
+
+	ss_ctrl = OP_CMD_BYTES(1) | OP_CMD_BUSW(fls(op->cmd.buswidth) - 1);
+
+	if (op->addr.nbytes)
+		ss_ctrl |= OP_ADDR_BYTES(op->addr.nbytes) |
+			   OP_ADDR_BUSW(fls(op->addr.buswidth) - 1);
+
+	if (op->dummy.nbytes)
+		ss_ctrl |= OP_DUMMY_CYC(op->dummy.nbytes);
+
+	if (op->data.nbytes) {
+		ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1);
+		if (op->data.dir == SPI_MEM_DATA_IN)
+			ss_ctrl |= OP_READ;
+	}
+
+	writel(ss_ctrl, mxic->regs + SS_CTRL(mem->spi->chip_select));
+
+	writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
+	       mxic->regs + HC_CFG);
+
+	ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
+	if (ret)
+		goto out;
+
+	for (i = 0; i < op->addr.nbytes; i++)
+		addr[i] = op->addr.val >> (8 * (op->addr.nbytes - i - 1));
+
+	ret = mxic_spi_data_xfer(mxic, addr, NULL, op->addr.nbytes);
+	if (ret)
+		goto out;
+
+	ret = mxic_spi_data_xfer(mxic, NULL, NULL, op->dummy.nbytes);
+	if (ret)
+		goto out;
+
+	ret = mxic_spi_data_xfer(mxic,
+				 op->data.dir == SPI_MEM_DATA_OUT ?
+				 op->data.buf.out : NULL,
+				 op->data.dir == SPI_MEM_DATA_IN ?
+				 op->data.buf.in : NULL,
+				 op->data.nbytes);
+
+out:
+	writel(readl(mxic->regs + HC_CFG) & ~HC_CFG_MAN_CS_ASSERT,
+	       mxic->regs + HC_CFG);
+	writel(0, mxic->regs + HC_EN);
+
+	return ret;
+}
+
+static const struct spi_controller_mem_ops mxic_spi_mem_ops = {
+	.supports_op = mxic_spi_mem_supports_op,
+	.exec_op = mxic_spi_mem_exec_op,
+};
+
+static void mxic_spi_set_cs(struct spi_device *spi, bool lvl)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
+
+	if (!lvl) {
+		writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_EN,
+		       mxic->regs + HC_CFG);
+		writel(HC_EN_BIT, mxic->regs + HC_EN);
+		writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
+		       mxic->regs + HC_CFG);
+	} else {
+		writel(readl(mxic->regs + HC_CFG) & ~HC_CFG_MAN_CS_ASSERT,
+		       mxic->regs + HC_CFG);
+		writel(0, mxic->regs + HC_EN);
+	}
+}
+
+static int mxic_spi_transfer_one(struct spi_master *master,
+				 struct spi_device *spi,
+				 struct spi_transfer *t)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(master);
+	unsigned int busw = OP_BUSW_1;
+	int ret;
+
+	if (t->rx_buf && t->tx_buf) {
+		if (((spi->mode & SPI_TX_QUAD) &&
+		     !(spi->mode & SPI_RX_QUAD)) ||
+		    ((spi->mode & SPI_TX_DUAL) &&
+		     !(spi->mode & SPI_RX_DUAL)))
+			return -ENOTSUPP;
+	}
+
+	if (t->tx_buf) {
+		if (spi->mode & SPI_TX_QUAD)
+			busw = OP_BUSW_4;
+		else if (spi->mode & SPI_TX_DUAL)
+			busw = OP_BUSW_2;
+	} else if (t->rx_buf) {
+		if (spi->mode & SPI_RX_QUAD)
+			busw = OP_BUSW_4;
+		else if (spi->mode & SPI_RX_DUAL)
+			busw = OP_BUSW_2;
+	}
+
+	writel(OP_CMD_BYTES(1) | OP_CMD_BUSW(busw) |
+	       OP_DATA_BUSW(busw) | (t->rx_buf ? OP_READ : 0),
+	       mxic->regs + SS_CTRL(0));
+
+	ret = mxic_spi_data_xfer(mxic, t->tx_buf, t->rx_buf, t->len);
+	if (ret)
+		return ret;
+
+	spi_finalize_current_transfer(master);
+
+	return 0;
+}
+
+static void mxic_spi_set_input_delay_dqs(struct mxic_spi *mxic, u8 idly_code)
+{
+	writel(IDLY_CODE_VAL(0, idly_code) |
+	       IDLY_CODE_VAL(1, idly_code) |
+	       IDLY_CODE_VAL(2, idly_code) |
+	       IDLY_CODE_VAL(3, idly_code),
+	       mxic->regs + IDLY_CODE(0));
+	writel(IDLY_CODE_VAL(4, idly_code) |
+	       IDLY_CODE_VAL(5, idly_code) |
+	       IDLY_CODE_VAL(6, idly_code) |
+	       IDLY_CODE_VAL(7, idly_code),
+	       mxic->regs + IDLY_CODE(1));
+}
+
+int mxic_spi_setup(struct spi_device *spi)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
+	unsigned long freq = spi->max_speed_hz;
+	int ret;
+
+	ret = clk_set_rate(mxic->send_clk, freq);
+	if (ret)
+		return ret;
+
+	ret = clk_set_rate(mxic->send_dly_clk, freq);
+	if (ret)
+		return ret;
+
+	mxic_spi_set_input_delay_dqs(mxic, 0xf);
+	ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));
+	if (ret)
+		return ret;
+
+	clk_prepare_enable(mxic->send_clk);
+	clk_prepare_enable(mxic->send_dly_clk);
+
+	return 0;
+}
+
+static int mxic_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct resource *res;
+	struct mxic_spi *mxic;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct mxic_spi));
+	if (!master)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, master);
+
+	mxic = spi_master_get_devdata(master);
+
+	master->dev.of_node = pdev->dev.of_node;
+
+	mxic->ps_clk = devm_clk_get(&pdev->dev, "ps_clk");
+	if (IS_ERR(mxic->ps_clk))
+		return PTR_ERR(mxic->ps_clk);
+
+	mxic->send_clk = devm_clk_get(&pdev->dev, "send_clk");
+	if (IS_ERR(mxic->send_clk))
+		return PTR_ERR(mxic->send_clk);
+
+	mxic->send_dly_clk = devm_clk_get(&pdev->dev, "send_dly_clk");
+	if (IS_ERR(mxic->send_dly_clk))
+		return PTR_ERR(mxic->send_dly_clk);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	mxic->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mxic->regs))
+		return PTR_ERR(mxic->regs);
+
+	mxic->linear.dma = res->start;
+
+	ret = clk_prepare_enable(mxic->ps_clk);
+	if (ret)
+		return ret;
+
+	master->num_chipselect = 1;
+	master->setup = mxic_spi_setup;
+	master->mem_ops = &mxic_spi_mem_ops;
+
+	master->set_cs = mxic_spi_set_cs;
+	master->transfer_one = mxic_spi_transfer_one;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->mode_bits = SPI_CPOL | SPI_CPHA |
+			SPI_RX_DUAL | SPI_TX_DUAL |
+			SPI_RX_QUAD | SPI_TX_QUAD;
+
+	mxic_spi_hw_init(mxic);
+
+	ret = spi_register_master(master);
+	if (ret) {
+		dev_err(&pdev->dev, "spi_register_master failed\n");
+		goto err_put_master;
+	}
+
+	return 0;
+
+err_put_master:
+	spi_master_put(master);
+	return ret;
+}
+
+static int mxic_spi_remove(struct platform_device *pdev)
+{
+	struct mxic_spi *mxic = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(mxic->ps_clk);
+
+	return 0;
+}
+
+static const struct of_device_id mxic_spi_of_ids[] = {
+	{ .compatible = "mxicy,mx25f0a-spi", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxic_spi_of_ids);
+
+static struct platform_driver mxic_spi_driver = {
+	.probe = mxic_spi_probe,
+	.remove = mxic_spi_remove,
+	.driver = {
+		.name = "mxic-spi",
+		.of_match_table = mxic_spi_of_ids,
+	},
+};
+module_platform_driver(mxic_spi_driver);
+
+MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
+MODULE_DESCRIPTION("MX25F0A SPI controller driver");
+MODULE_LICENSE("GPL v2");
+