diff mbox series

[4/6] spi: Add SPIBSC driver

Message ID 20191203034519.5640-5-chris.brandt@renesas.com (mailing list archive)
State Not Applicable, archived
Headers show
Series spi: Add Renesas SPIBSC controller | expand

Commit Message

Chris Brandt Dec. 3, 2019, 3:45 a.m. UTC
Add a driver for the SPIBSC controller in Renesas SoC devices.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/spi/Kconfig      |   8 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-spibsc.c | 609 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 618 insertions(+)
 create mode 100644 drivers/spi/spi-spibsc.c

Comments

Mark Brown Dec. 3, 2019, 2:19 p.m. UTC | #1
On Mon, Dec 02, 2019 at 10:45:17PM -0500, Chris Brandt wrote:

> +config SPI_SPIBSC
> +	tristate "Renesas SPI Multi I/O Bus Controller"
> +	depends on ARCH_R7S72100 || ARCH_R7S9210

I'm not seeing any build dependency here, please add an ||
COMPILE_TEST for build coverage.

> +++ b/drivers/spi/spi-spibsc.c
> @@ -0,0 +1,609 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SPI Bus Space Controller (SPIBSC) bus driver

Please make the entire comment block here a C++ one so things
look more intentional.

> +static void spibsc_write(struct spibsc_priv *sbsc, int reg, u32 val)
> +{
> +	iowrite32(val, sbsc->base + reg);
> +}
> +static void spibsc_write8(struct spibsc_priv *sbsc, int reg, u8 val)

Blank likes between functions, please see coding-style.rst.
Looking at a bunch of the stuff here it looks like you could
benefit from regmap, it's got lots of debug infrastructure.

> +	if (tx)
> +		pr_debug("spibsc: send data: ");
> +	else
> +		pr_debug("spibsc: recv data: ");

dev_dbg() if you're going to do tis.

> +
> +	for (i = 0; i < len; ) {
> +		sprintf(line_buffer + line_index, " %02X", buf[i]);

snprintf()!

> +static int spibsc_transfer_one_message(struct spi_controller *master,
> +				       struct spi_message *msg)
> +{
> +	struct spibsc_priv *sbsc = spi_controller_get_devdata(master);
> +	struct spi_transfer *t, *t_last;
> +	u8 tx_data[MAX_CMD_LEN];
> +	int tx_only;
> +	u8 tx_len;
> +	int ret;
> +
> +	t_last = list_last_entry(&msg->transfers, struct spi_transfer,
> +				 transfer_list);
> +	/* defaults */
> +	ret = 0;
> +	sbsc->last_xfer = 0;
> +	tx_only = 1;
> +
> +	/* Analyze the messages */
> +	t = list_first_entry(&msg->transfers, struct spi_transfer,
> +			     transfer_list);
> +	if (t->rx_buf) {
> +		dev_dbg(sbsc->dev, "Cannot Rx without Tx first!\n");
> +		return -EIO;

These errors should probably be -EINVAL, you're failing on
validation here.

> +	}
> +	list_for_each_entry(t, &msg->transfers, transfer_list) {

Blank line here please as well.

> +	if (spi->bits_per_word != 8) {
> +		dev_err(sbsc->dev, "bits_per_word must be 8\n");
> +		return -EIO;
> +	}

The core will validate this for you.

> +	master->num_chipselect	= 1;
> +	master->mode_bits		= SPI_CPOL | SPI_CPHA;
> +	master->setup			= spibsc_setup;
> +	master->transfer_one_message	= spibsc_transfer_one_message;

Set bits_per_word_mask here.

> +	dev_info(&pdev->dev, "probed\n");
> +

Remove this, it's just noise.

> +static int spibsc_remove(struct platform_device *pdev)
> +{
> +	struct spibsc_priv *sbsc = dev_get_drvdata(&pdev->dev);
> +
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);

There seems to be no purpose in the runtime PM code in this
driver, there's no PM operations of any kind and the driver holds
a runtime PM reference for the entire lifetime of the device.

> +	spi_unregister_controller(sbsc->master);

You registered the controller with devm_, there's no need to
unregister it and if you do you need to use a matching devm_
unregiser.
Chris Brandt Dec. 3, 2019, 3 p.m. UTC | #2
Hello Mark,

On Tue, Dec 3, 2019, Mark Brown wrote:
> On Mon, Dec 02, 2019 at 10:45:17PM -0500, Chris Brandt wrote:
> 
> > +config SPI_SPIBSC
> > +	tristate "Renesas SPI Multi I/O Bus Controller"
> > +	depends on ARCH_R7S72100 || ARCH_R7S9210
> 
> I'm not seeing any build dependency here, please add an || COMPILE_TEST for
> build coverage.
(snip)

Thank you for your review!

I have no complaints about your comments so I will make the changes and
send out a v2.

Chris
Geert Uytterhoeven Dec. 3, 2019, 6:28 p.m. UTC | #3
Hi Chris,

On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add a driver for the SPIBSC controller in Renesas SoC devices.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/spi/spi-spibsc.c
> @@ -0,0 +1,609 @@
> +// SPDX-License-Identifier: GPL-2.0

GPL-2.0-only

> +static void spibsc_print_data(struct spibsc_priv *sbsc, u8 tx, const u8 *buf,
> +                             int len)
> +{
> +#if defined(DEBUG_PRINT_DATA)
> +       char line_buffer[3*16+1];
> +       int i, line_index = 0;
> +
> +       if (tx)
> +               pr_debug("spibsc: send data: ");
> +       else
> +               pr_debug("spibsc: recv data: ");
> +
> +       for (i = 0; i < len; ) {
> +               sprintf(line_buffer + line_index, " %02X", buf[i]);
> +               line_index += 3;
> +               i++;
> +               if (i % 16 == 0) {
> +                       pr_debug(line_buffer);
> +                       line_index = 0;
> +               }
> +       }
> +       if (i % 16 != 0)
> +               pr_debug(line_buffer);
> +       else
> +               pr_debug("\n");
> +#endif

print_hex_dump_debug()?

> +}
> +
> +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc)
> +{
> +       int t = 256 * 100000;
> +
> +       while (t--) {
> +               if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND)
> +                       return 0;
> +               ndelay(1);
> +       }

So this may busy loop for up to 25.6 ms? Oops...

Can you use the interrupt (SPIHF) instead, signalling a completion?

> +
> +       dev_err(sbsc->dev, "Timeout waiting for TEND\n");
> +       return -ETIMEDOUT;
> +}


> +       /* Use 'Option Data' for 3rd-6th bytes */
> +       switch (tx_len) {
> +       case 6:
> +               dropr |= DROPR_OPD0(tx_data[5]);
> +               opde |= (1 << 0);

Both checkpatch and gcc tell you to add fallthrough coments.

> +       case 5:
> +               dropr |= DROPR_OPD1(tx_data[4]);
> +               opde |= (1 << 1);
> +       case 4:
> +               dropr |= DROPR_OPD2(tx_data[3]);
> +               opde |= (1 << 2);
> +       case 3:
> +               dropr |= DROPR_OPD3(tx_data[2]);
> +               opde |= (1 << 3);
> +               drenr |= DRENR_OPDE(opde);
> +       }

> +static int spibsc_transfer_one_message(struct spi_controller *master,
> +                                      struct spi_message *msg)
> +{
> +       struct spibsc_priv *sbsc = spi_controller_get_devdata(master);
> +       struct spi_transfer *t, *t_last;
> +       u8 tx_data[MAX_CMD_LEN];
> +       int tx_only;
> +       u8 tx_len;
> +       int ret;
> +
> +       t_last = list_last_entry(&msg->transfers, struct spi_transfer,
> +                                transfer_list);
> +       /* defaults */
> +       ret = 0;
> +       sbsc->last_xfer = 0;
> +       tx_only = 1;
> +
> +       /* Analyze the messages */
> +       t = list_first_entry(&msg->transfers, struct spi_transfer,
> +                            transfer_list);
> +       if (t->rx_buf) {
> +               dev_dbg(sbsc->dev, "Cannot Rx without Tx first!\n");
> +               return -EIO;
> +       }
> +       list_for_each_entry(t, &msg->transfers, transfer_list) {
> +               if (t->rx_buf) {
> +                       tx_only = 0;
> +                       if (t != t_last) {
> +                               dev_dbg(sbsc->dev, "RX transaction is not the last transaction!\n");
> +                               return -EIO;
> +                       }
> +               }
> +               if (t->cs_change) {
> +                       dev_err(sbsc->dev, "cs_change not supported");
> +                       return -EIO;
> +               }
> +       }

If you would do the checks above in .prepare_message()
(like e.g. rspi does)...

> +
> +       /* Tx Only (SPI Mode is used) */
> +       if (tx_only == 1) {
> +
> +               dev_dbg(sbsc->dev, "%s: TX only\n", __func__);
> +
> +               /* Initialize for SPI Mode */
> +               spibsc_write(sbsc, CMNCR, CMNCR_INIT);
> +
> +               /* Send messages */
> +               list_for_each_entry(t, &msg->transfers, transfer_list) {
> +
> +                       if (t == t_last)
> +                               sbsc->last_xfer = 1;
> +
> +                       ret = spibsc_send_data(sbsc, t->tx_buf, t->len);
> +                       if (ret)
> +                               break;
> +
> +                       msg->actual_length += t->len;
> +               }
> +
> +               /* Done */
> +               msg->status = ret;
> +               spi_finalize_current_message(master);
> +               return ret;
> +       }
> +
> +       /* Tx, then RX (Data Read Mode is used) */
> +       dev_dbg(sbsc->dev, "%s: Tx then Rx\n", __func__);
> +
> +       /* Buffer up the transmit portion (cmd + addr) so we can send it all at
> +        * once
> +        */
> +       tx_len = 0;
> +       list_for_each_entry(t, &msg->transfers, transfer_list) {
> +               if (t->tx_buf) {
> +                       if ((tx_len + t->len) > sizeof(tx_data)) {
> +                               dev_dbg(sbsc->dev, "Command too big (%d)\n",
> +                                       tx_len + t->len);
> +                               return -EIO;
> +                       }
> +                       memcpy(tx_data + tx_len, t->tx_buf, t->len);
> +                       tx_len += t->len;
> +               }
> +
> +               if (t->rx_buf)
> +                       ret = spibsc_send_recv_data(sbsc, tx_data, tx_len,
> +                                                   t->rx_buf,  t->len);
> +
> +               msg->actual_length += t->len;
> +       }
> +
> +       msg->status = ret;
> +       spi_finalize_current_message(master);

... can't you just use .transfer_one(), instead of duplicating the logic
from spi_transfer_one_message()?
Or is there some special detail that's not obvious to the casual reviewer?


> +static const struct of_device_id of_spibsc_match[] = {
> +       { .compatible = "renesas,spibsc"},
> +       { .compatible = "renesas,r7s72100-spibsc", .data = (void *)HAS_SPBCR},
> +       { .compatible = "renesas,r7s9210-spibsc"},

Do you need to match against all 3 in the driver?
Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR?
If not, the fallback to "renesas,spibsc" is not valid for RZ/A1.

> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_spibsc_match);
> +
> +static struct platform_driver spibsc_driver = {
> +       .probe = spibsc_probe,
> +       .remove = spibsc_remove,
> +       .driver = {
> +               .name = "spibsc",
> +               .owner = THIS_MODULE,
> +               .of_match_table = of_spibsc_match,
> +       },
> +};
> +module_platform_driver(spibsc_driver);
> +
> +MODULE_DESCRIPTION("Renesas SPIBSC SPI Flash driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Chris Brandt");
> --
> 2.23.0
>


--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven Dec. 3, 2019, 6:29 p.m. UTC | #4
Hi Mark,

On Tue, Dec 3, 2019 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Dec 02, 2019 at 10:45:17PM -0500, Chris Brandt wrote:
> > +static int spibsc_remove(struct platform_device *pdev)
> > +{
> > +     struct spibsc_priv *sbsc = dev_get_drvdata(&pdev->dev);
> > +
> > +     pm_runtime_put(&pdev->dev);
> > +     pm_runtime_disable(&pdev->dev);
>
> There seems to be no purpose in the runtime PM code in this
> driver, there's no PM operations of any kind and the driver holds
> a runtime PM reference for the entire lifetime of the device.

It matters for the clock domain (assumed the module clock is not always
marked as a critical clock).

Gr{oetje,eeting}s,

                        Geert
Mark Brown Dec. 4, 2019, 11:18 a.m. UTC | #5
On Tue, Dec 03, 2019 at 07:28:18PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:

> > +static const struct of_device_id of_spibsc_match[] = {
> > +       { .compatible = "renesas,spibsc"},
> > +       { .compatible = "renesas,r7s72100-spibsc", .data = (void *)HAS_SPBCR},
> > +       { .compatible = "renesas,r7s9210-spibsc"},

> Do you need to match against all 3 in the driver?
> Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR?
> If not, the fallback to "renesas,spibsc" is not valid for RZ/A1.

I do think it's useful to explicitly list all the compatibles in
the driver, it documents the handling needed for each of the
variants and it provides some robustness against DTs that neglect
to list the fallback compatibles.
Mark Brown Dec. 4, 2019, 11:25 a.m. UTC | #6
On Tue, Dec 03, 2019 at 07:29:45PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 3, 2019 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:

> > > +     pm_runtime_put(&pdev->dev);
> > > +     pm_runtime_disable(&pdev->dev);

> > There seems to be no purpose in the runtime PM code in this
> > driver, there's no PM operations of any kind and the driver holds
> > a runtime PM reference for the entire lifetime of the device.

> It matters for the clock domain (assumed the module clock is not always
> marked as a critical clock).

That seems like a problem with what the clock domains are doing
surely?  The default is supposed to be that if runtime PM isn't
enabled we get the behaviour the driver is manually implementing
here.  Besides, if this is actually impacting power management
I'd expect us to be letting the IP idle rather than holding it
powered up all the time.
Geert Uytterhoeven Dec. 4, 2019, 12:14 p.m. UTC | #7
Hi Mark,

On Wed, Dec 4, 2019 at 12:25 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Dec 03, 2019 at 07:29:45PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Dec 3, 2019 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:
> > > > +     pm_runtime_put(&pdev->dev);
> > > > +     pm_runtime_disable(&pdev->dev);
>
> > > There seems to be no purpose in the runtime PM code in this
> > > driver, there's no PM operations of any kind and the driver holds
> > > a runtime PM reference for the entire lifetime of the device.
>
> > It matters for the clock domain (assumed the module clock is not always
> > marked as a critical clock).
>
> That seems like a problem with what the clock domains are doing
> surely?  The default is supposed to be that if runtime PM isn't
> enabled we get the behaviour the driver is manually implementing

Unfortunately not: if the driver doesn't implement Runtime PM, the
default is to not do anything.  Later, unused clocks will be disabled,
and the device will stop functioning.

> here.  Besides, if this is actually impacting power management
> I'd expect us to be letting the IP idle rather than holding it
> powered up all the time.

That would be better, definitely.  However, that's just an optimization over
holding it powered up all the time.

Gr{oetje,eeting}s,

                        Geert
Chris Brandt Dec. 4, 2019, 3:51 p.m. UTC | #8
Hello Mark,

On Tue, Dec 3, 2019, Mark Brown wrote:
> > +static void spibsc_write(struct spibsc_priv *sbsc, int reg, u32 val)
> > +{
> > +	iowrite32(val, sbsc->base + reg);
> > +}
> > +static void spibsc_write8(struct spibsc_priv *sbsc, int reg, u8 val)
> 
> Looking at a bunch of the stuff here it looks like you could benefit from
> regmap, it's got lots of debug infrastructure.

Thank you for the suggestion, but I looked into using regmap, and there 
are a lot of drivers that use it, but I don't think it's going to work 
well for me.
Regmap assumes that all the registers will be the same size. I have to 
have functions that write with different widths (8/16/32) for a reason.


Chris
Mark Brown Dec. 4, 2019, 4:49 p.m. UTC | #9
On Wed, Dec 04, 2019 at 03:51:48PM +0000, Chris Brandt wrote:
> On Tue, Dec 3, 2019, Mark Brown wrote:

> > Looking at a bunch of the stuff here it looks like you could benefit from
> > regmap, it's got lots of debug infrastructure.

> Thank you for the suggestion, but I looked into using regmap, and there 
> are a lot of drivers that use it, but I don't think it's going to work 
> well for me.
> Regmap assumes that all the registers will be the same size. I have to 
> have functions that write with different widths (8/16/32) for a reason.

You *can* have more than one regmap for a device, or if it really
only is one or two registers open code accesses to just those
registers.
Chris Brandt Dec. 4, 2019, 10:12 p.m. UTC | #10
Hi Geert,

Thank you for your review and suggestions!

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc) {
> > +       int t = 256 * 100000;
> > +
> > +       while (t--) {
> > +               if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND)
> > +                       return 0;
> > +               ndelay(1);
> > +       }
> 
> So this may busy loop for up to 25.6 ms? Oops...
> 
> Can you use the interrupt (SPIHF) instead, signalling a completion?

RZ/A1 doesn't have any interrupts. And I think the interrupts in RZ/A2 
only work for HyperFlash (not QSPI).

But, 25.6ms is way too long!
I'll assume the slowest clock to be 1MHz and the max FIFO transfer of 4 bytes.
That's 32us, so that's what I'll use.


> > +               if (t->cs_change) {
> > +                       dev_err(sbsc->dev, "cs_change not supported");
> > +                       return -EIO;
> > +               }
> > +       }
> 
> If you would do the checks above in .prepare_message() (like e.g. rspi
> does)...

OK, Thanks. :)

> > +       case 6:
> > +               dropr |= DROPR_OPD0(tx_data[5]);
> > +               opde |= (1 << 0);
> 
> Both checkpatch and gcc tell you to add fallthrough coments.

OK, fixed.


> ... can't you just use .transfer_one(), instead of duplicating the logic from
> spi_transfer_one_message()?
> Or is there some special detail that's not obvious to the casual reviewer?

I guess .transfer_one() could be used if I kept shoving more stuff into 
.prepare_message().

But, the screwy thing about this controller is that messages that are 
'Tx only' are processed differently then 'Tx then Rx' messages and not 
like a traditional SPI controller.
By implementing both conditions in .spi_transfer_one_message(), it makes
that more clear for the next person in my opinion because you can see 
that sometimes we have to work with the entire message as a whole, not 
just individual pieces.


> > +static const struct of_device_id of_spibsc_match[] = {
> > +       { .compatible = "renesas,spibsc"},
> > +       { .compatible = "renesas,r7s72100-spibsc", .data = (void
> *)HAS_SPBCR},
> > +       { .compatible = "renesas,r7s9210-spibsc"},
> 
> Do you need to match against all 3 in the driver?
> Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR?
> If not, the fallback to "renesas,spibsc" is not valid for RZ/A1.

OK, I dropped "renesas,spibsc".
I like having individual SoC names anyway.

Chris
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 870f7797b56b..405d7e73963e 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -575,6 +575,14 @@  config SPI_RSPI
 	help
 	  SPI driver for Renesas RSPI and QSPI blocks.
 
+config SPI_SPIBSC
+	tristate "Renesas SPI Multi I/O Bus Controller"
+	depends on ARCH_R7S72100 || ARCH_R7S9210
+	help
+	  Also referred to as the SPI Bus Space Controller (SPIBSC).
+	  This controller was designed specifically for accessing QSPI flash
+	  devices.
+
 config SPI_QCOM_QSPI
 	tristate "QTI QSPI controller"
 	depends on ARCH_QCOM
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index bb49c9e6d0a0..9525256c4d51 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -99,6 +99,7 @@  obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
 obj-$(CONFIG_SPI_SIFIVE)		+= spi-sifive.o
 obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
 obj-$(CONFIG_SPI_SLAVE_MT27XX)          += spi-slave-mt27xx.o
+obj-$(CONFIG_SPI_SPIBSC)		+= spi-spibsc.o
 obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
 obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
 obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
diff --git a/drivers/spi/spi-spibsc.c b/drivers/spi/spi-spibsc.c
new file mode 100644
index 000000000000..ac7e6c84417c
--- /dev/null
+++ b/drivers/spi/spi-spibsc.c
@@ -0,0 +1,609 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SPI Bus Space Controller (SPIBSC) bus driver
+ * Otherwise known as a SPI Multi I/O Bus Controller
+ *
+ * Copyright (C) 2019 Renesas Electronics
+ *
+ */
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+
+/* SPIBSC registers */
+#define	CMNCR	0x00
+#define	SSLDR	0x04
+#define SPBCR	0x08
+#define DRCR	0x0c
+#define DRCMR	0x10
+#define DREAR	0x14
+#define DROPR	0x18
+#define DRENR	0x1c
+#define	SMCR	0x20
+#define	SMCMR	0x24
+#define	SMADR	0x28
+#define	SMOPR	0x2c
+#define	SMENR	0x30
+#define SMRDR0	0x38
+#define SMRDR1	0x3c
+#define	SMWDR0	0x40
+#define SMWDR1	0x44
+#define	CMNSR	0x48
+#define SMDMCR	0x60
+#define SMDRENR	0x64
+
+/* CMNCR */
+#define	CMNCR_MD	BIT(31)
+#define	CMNCR_SFDE	BIT(24)
+#define	CMNCR_MOIIO3(x)	(((u32)(x) & 0x3) << 22)
+#define	CMNCR_MOIIO2(x)	(((u32)(x) & 0x3) << 20)
+#define	CMNCR_MOIIO1(x)	(((u32)(x) & 0x3) << 18)
+#define	CMNCR_MOIIO0(x)	(((u32)(x) & 0x3) << 16)
+#define	CMNCR_IO3FV(x)	(((u32)(x) & 0x3) << 14)
+#define	CMNCR_IO2FV(x)	(((u32)(x) & 0x3) << 12)
+#define	CMNCR_IO0FV(x)	(((u32)(x) & 0x3) << 8)
+#define	CMNCR_CPHAT	BIT(6)
+#define	CMNCR_CPHAR	BIT(5)
+#define	CMNCR_SSLP	BIT(4)
+#define	CMNCR_CPOL	BIT(3)
+#define	CMNCR_BSZ(n)	(((u32)(n) & 0x3) << 0)
+#define	OUT_0		(0u)
+#define	OUT_1		(1u)
+#define	OUT_REV		(2u)
+#define	OUT_HIZ		(3u)
+#define	BSZ_SINGLE	(0)
+#define	BSZ_DUAL	(1)
+#define CMNCR_INIT	(CMNCR_MD | \
+			CMNCR_SFDE | \
+			CMNCR_MOIIO3(OUT_HIZ) | \
+			CMNCR_MOIIO2(OUT_HIZ) | \
+			CMNCR_MOIIO1(OUT_HIZ) | \
+			CMNCR_MOIIO0(OUT_HIZ) | \
+			CMNCR_IO3FV(OUT_HIZ) | \
+			CMNCR_IO2FV(OUT_HIZ) | \
+			CMNCR_IO0FV(OUT_HIZ) | \
+			CMNCR_CPHAR | \
+			CMNCR_BSZ(BSZ_SINGLE))
+
+/* SSLDR */
+#define	SSLDR_SPNDL(x)	(((u32)(x) & 0x7) << 16)
+#define	SSLDR_SLNDL(x)	(((u32)(x) & 0x7) << 8)
+#define	SSLDR_SCKDL(x)	(((u32)(x) & 0x7) << 0)
+#define	SSLDR_INIT	(SSLDR_SPNDL(3) | SSLDR_SLNDL(3) | SSLDR_SCKDL(3))
+
+/* SPBCR */
+#define	SPBCR_SPBR(x)	(((u32)(x) & 0xff) << 8)
+#define	SPBCR_BRDV(x)	(((u32)(x) & 0x3) << 0)
+
+/* DRCR (read mode) */
+#define	DRCR_SSLN	BIT(24)
+#define	DRCR_RBURST(x)	(((u32)(x) & 0xf) << 16)
+#define	DRCR_RCF	BIT(9)
+#define	DRCR_RBE	BIT(8)
+#define	DRCR_SSLE	BIT(0)
+
+/* DROPR (read mode) */
+#define	DROPR_OPD3(o)	(((u32)(o) & 0xff) << 24)
+#define	DROPR_OPD2(o)	(((u32)(o) & 0xff) << 16)
+#define	DROPR_OPD1(o)	(((u32)(o) & 0xff) << 8)
+#define	DROPR_OPD0(o)	(((u32)(o) & 0xff) << 0)
+
+/* DRENR (read mode) */
+#define	DRENR_CDE	BIT(14)
+#define	DRENR_OCDE	BIT(12)
+#define	DRENR_OPDE(o)	(((u32)(o) & 0xf) << 4)
+
+/* SMCR (spi mode) */
+#define	SMCR_SSLKP	BIT(8)
+#define	SMCR_SPIRE	BIT(2)
+#define	SMCR_SPIWE	BIT(1)
+#define	SMCR_SPIE	BIT(0)
+
+/* SMCMR (spi mode) */
+#define	SMCMR_CMD(c)	(((u32)(c) & 0xff) << 16)
+#define	SMCMR_OCMD(o)	(((u32)(o) & 0xff) << 0)
+
+/* SMENR (spi mode) */
+#define	SMENR_SPIDE(b)	(((u32)(b) & 0xf) << 0)
+#define	SPIDE_8BITS	(0x8)
+#define	SPIDE_16BITS	(0xc)
+#define	SPIDE_32BITS	(0xf)
+
+/* CMNSR (spi mode) */
+#define	CMNSR_SSLF	BIT(1)
+#define	CMNSR_TEND	BIT(0)
+
+#define MAX_CMD_LEN 6
+
+/* HW Option Flags */
+#define HAS_SPBCR BIT(0)
+
+struct spibsc_priv {
+	void __iomem *base;	/* register base */
+	void __iomem *mmio;	/* memory mapped io space of SPI Flash */
+	int mmio_size;
+	struct spi_controller *master;
+	struct device *dev;
+	struct clk *clk;
+	u8 last_xfer;
+	u32 flags;
+};
+
+static void spibsc_write(struct spibsc_priv *sbsc, int reg, u32 val)
+{
+	iowrite32(val, sbsc->base + reg);
+}
+static void spibsc_write8(struct spibsc_priv *sbsc, int reg, u8 val)
+{
+	iowrite8(val, sbsc->base + reg);
+}
+static void spibsc_write16(struct spibsc_priv *sbsc, int reg, u16 val)
+{
+	iowrite16(val, sbsc->base + reg);
+}
+static u32 spibsc_read(struct spibsc_priv *sbsc, int reg)
+{
+	return ioread32(sbsc->base + reg);
+}
+
+static void spibsc_print_data(struct spibsc_priv *sbsc, u8 tx, const u8 *buf,
+			      int len)
+{
+#if defined(DEBUG_PRINT_DATA)
+	char line_buffer[3*16+1];
+	int i, line_index = 0;
+
+	if (tx)
+		pr_debug("spibsc: send data: ");
+	else
+		pr_debug("spibsc: recv data: ");
+
+	for (i = 0; i < len; ) {
+		sprintf(line_buffer + line_index, " %02X", buf[i]);
+		line_index += 3;
+		i++;
+		if (i % 16 == 0) {
+			pr_debug(line_buffer);
+			line_index = 0;
+		}
+	}
+	if (i % 16 != 0)
+		pr_debug(line_buffer);
+	else
+		pr_debug("\n");
+#endif
+}
+
+static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc)
+{
+	int t = 256 * 100000;
+
+	while (t--) {
+		if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND)
+			return 0;
+		ndelay(1);
+	}
+
+	dev_err(sbsc->dev, "Timeout waiting for TEND\n");
+	return -ETIMEDOUT;
+}
+
+/*
+ * This function sends data using 'SPI operation mode'. It is intended to be
+ * used only with SPI Flash commands that do not require any reading back from
+ * the SPI flash.
+ */
+static int spibsc_send_data(struct spibsc_priv *sbsc, const u8 *data, int len)
+{
+	u32 smcr, smenr, smwdr0;
+	int ret, unit, sslkp;
+
+	/* print data (for debugging) */
+	spibsc_print_data(sbsc, 1, data, len);
+
+	while (len > 0) {
+		if (len >= 4) {
+			unit = 4;
+			smenr = SMENR_SPIDE(SPIDE_32BITS);
+		} else {
+			unit = len;
+			if (unit == 3)
+				unit = 2;
+
+			if (unit >= 2)
+				smenr = SMENR_SPIDE(SPIDE_16BITS);
+			else
+				smenr = SMENR_SPIDE(SPIDE_8BITS);
+		}
+
+		/* set 4bytes data, bit stream */
+		smwdr0 = *data++;
+		if (unit >= 2)
+			smwdr0 |= (u32)(*data++ << 8);
+		if (unit >= 3)
+			smwdr0 |= (u32)(*data++ << 16);
+		if (unit >= 4)
+			smwdr0 |= (u32)(*data++ << 24);
+
+		/* mask unwrite area */
+		if (unit == 3)
+			smwdr0 |= 0xFF000000;
+		else if (unit == 2)
+			smwdr0 |= 0xFFFF0000;
+		else if (unit == 1)
+			smwdr0 |= 0xFFFFFF00;
+
+		/* write send data. */
+		if (unit == 2)
+			spibsc_write16(sbsc, SMWDR0, (u16)smwdr0);
+		else if (unit == 1)
+			spibsc_write8(sbsc, SMWDR0, (u8)smwdr0);
+		else
+			spibsc_write(sbsc, SMWDR0, smwdr0);
+
+		len -= unit;
+		if ((len <= 0) && sbsc->last_xfer)
+			sslkp = 0;
+		else
+			sslkp = 1;
+
+		/* set params */
+		spibsc_write(sbsc, SMCMR, 0);
+		spibsc_write(sbsc, SMADR, 0);
+		spibsc_write(sbsc, SMOPR, 0);
+		spibsc_write(sbsc, SMENR, smenr);
+
+		/* start spi transfer */
+		smcr = SMCR_SPIE|SMCR_SPIWE;
+		if (sslkp)
+			smcr |= SMCR_SSLKP;
+		spibsc_write(sbsc, SMCR, smcr);
+
+		ret = spibsc_wait_trans_completion(sbsc);
+		if (ret)
+			return  ret;
+	}
+	return 0;
+}
+
+/*
+ * This function uses 'Data Read' mode to send the command (and address) then
+ * read data back out.
+ * The HW was designed such that you program the registers with the command,
+ * base address, additional command data, etc... But, that makes things too
+ * difficult because it means this driver has to pick out those parameters from
+ * the data stream that was passed.
+ * Instead, we will ignore how the HW was 'supposed' to be used and just
+ * blindly put the Tx data (commands) to send in the registers in the order
+ * in which we know they will be transmitted:
+ *
+ * [DRCMR.CMD]+[DRCMR.OCMD]+[DROPR.OPD3]+[DROPR.OPD2]+[DROPR.OPD1]+[DROPR.OPD0]
+ *
+ * We can send up to 6 bytes this way.
+ * We will tell the HW to skip sending the 'address' because we are secretly
+ * including it in the "command" and that way we can leave the address registers
+ * blank.
+ *
+ * Note that when reading data, the HW will read in 8-byte units which usually
+ * is not an issue for SPI Flash devices.
+ */
+static int spibsc_send_recv_data(struct spibsc_priv *sbsc, u8 *tx_data,
+				 int tx_len, u8 *rx_data, int rx_len)
+{
+	u32 drcmr, drenr, dropr;
+	u8 opde;
+
+	dev_dbg(sbsc->dev, "%s (tx=%d, rx=%d)\n", __func__, tx_len, rx_len);
+
+	if (tx_len > MAX_CMD_LEN) {
+		dev_err(sbsc->dev, "Command length too long (%d)", tx_len);
+		return -EIO;
+	}
+
+	if (rx_len > sbsc->mmio_size) {
+		dev_err(sbsc->dev, "Receive length too long (%d)", rx_len);
+		return -EIO;
+	}
+
+	/* Setup Data Read mode
+	 * Flush read cache and enable burst mode. Burst mode is required
+	 * in order to keep chip select low between read transfers, but it
+	 * also means data is read in 8-byte intervals.
+	 */
+	spibsc_write(sbsc, DRCR, DRCR_SSLN | DRCR_RCF | DRCR_RBE | DRCR_SSLE);
+	spibsc_read(sbsc, DRCR);
+
+	/* Enter Data Read mode */
+	spibsc_write(sbsc, CMNCR, 0x01FFF300);
+	drcmr = 0;
+	drenr = 0;
+	dropr = 0;
+	opde = 0;
+
+	if (tx_len >= 1) {
+		/* Use 'Command' register for the 1st byte */
+		drcmr |= SMCMR_CMD(tx_data[0]);
+		drenr |= DRENR_CDE;
+	}
+
+	if (tx_len >= 2) {
+		/* Use 'Optional Command' register for the 2nd byte */
+		drcmr |= SMCMR_OCMD(tx_data[1]);
+		drenr |= DRENR_OCDE;
+	}
+
+	/* Use 'Option Data' for 3rd-6th bytes */
+	switch (tx_len) {
+	case 6:
+		dropr |= DROPR_OPD0(tx_data[5]);
+		opde |= (1 << 0);
+	case 5:
+		dropr |= DROPR_OPD1(tx_data[4]);
+		opde |= (1 << 1);
+	case 4:
+		dropr |= DROPR_OPD2(tx_data[3]);
+		opde |= (1 << 2);
+	case 3:
+		dropr |= DROPR_OPD3(tx_data[2]);
+		opde |= (1 << 3);
+		drenr |= DRENR_OPDE(opde);
+	}
+
+	spibsc_write(sbsc, DRCMR, drcmr);
+	spibsc_write(sbsc, DROPR, dropr);
+	spibsc_write(sbsc, DRENR, drenr);
+
+	/* This internal bus access is what will trigger the actual Tx/Rx
+	 * operation. Remember, we don't care about the address.
+	 */
+	memcpy(rx_data, sbsc->mmio, rx_len);
+
+	/* Deactivate chip select */
+	spibsc_write(sbsc, DRCR, DRCR_SSLN);
+
+	/* Print data (for debugging) */
+	spibsc_print_data(sbsc, 1, tx_data, tx_len);
+	spibsc_print_data(sbsc, 0, rx_data, rx_len);
+
+	return 0;
+}
+
+static int spibsc_transfer_one_message(struct spi_controller *master,
+				       struct spi_message *msg)
+{
+	struct spibsc_priv *sbsc = spi_controller_get_devdata(master);
+	struct spi_transfer *t, *t_last;
+	u8 tx_data[MAX_CMD_LEN];
+	int tx_only;
+	u8 tx_len;
+	int ret;
+
+	t_last = list_last_entry(&msg->transfers, struct spi_transfer,
+				 transfer_list);
+	/* defaults */
+	ret = 0;
+	sbsc->last_xfer = 0;
+	tx_only = 1;
+
+	/* Analyze the messages */
+	t = list_first_entry(&msg->transfers, struct spi_transfer,
+			     transfer_list);
+	if (t->rx_buf) {
+		dev_dbg(sbsc->dev, "Cannot Rx without Tx first!\n");
+		return -EIO;
+	}
+	list_for_each_entry(t, &msg->transfers, transfer_list) {
+		if (t->rx_buf) {
+			tx_only = 0;
+			if (t != t_last) {
+				dev_dbg(sbsc->dev, "RX transaction is not the last transaction!\n");
+				return -EIO;
+			}
+		}
+		if (t->cs_change) {
+			dev_err(sbsc->dev, "cs_change not supported");
+			return -EIO;
+		}
+	}
+
+	/* Tx Only (SPI Mode is used) */
+	if (tx_only == 1) {
+
+		dev_dbg(sbsc->dev, "%s: TX only\n", __func__);
+
+		/* Initialize for SPI Mode */
+		spibsc_write(sbsc, CMNCR, CMNCR_INIT);
+
+		/* Send messages */
+		list_for_each_entry(t, &msg->transfers, transfer_list) {
+
+			if (t == t_last)
+				sbsc->last_xfer = 1;
+
+			ret = spibsc_send_data(sbsc, t->tx_buf, t->len);
+			if (ret)
+				break;
+
+			msg->actual_length += t->len;
+		}
+
+		/* Done */
+		msg->status = ret;
+		spi_finalize_current_message(master);
+		return ret;
+	}
+
+	/* Tx, then RX (Data Read Mode is used) */
+	dev_dbg(sbsc->dev, "%s: Tx then Rx\n", __func__);
+
+	/* Buffer up the transmit portion (cmd + addr) so we can send it all at
+	 * once
+	 */
+	tx_len = 0;
+	list_for_each_entry(t, &msg->transfers, transfer_list) {
+		if (t->tx_buf) {
+			if ((tx_len + t->len) > sizeof(tx_data)) {
+				dev_dbg(sbsc->dev, "Command too big (%d)\n",
+					tx_len + t->len);
+				return -EIO;
+			}
+			memcpy(tx_data + tx_len, t->tx_buf, t->len);
+			tx_len += t->len;
+		}
+
+		if (t->rx_buf)
+			ret = spibsc_send_recv_data(sbsc, tx_data, tx_len,
+						    t->rx_buf,  t->len);
+
+		msg->actual_length += t->len;
+	}
+
+	msg->status = ret;
+	spi_finalize_current_message(master);
+
+	return ret;
+}
+
+static int spibsc_setup(struct spi_device *spi)
+{
+	struct spibsc_priv *sbsc = spi_controller_get_devdata(spi->master);
+	u32 max_speed_hz;
+	unsigned long rate;
+	u8 spbr;
+	u8 div_ratio;
+
+	if (spi->bits_per_word != 8) {
+		dev_err(sbsc->dev, "bits_per_word must be 8\n");
+		return -EIO;
+	}
+
+	if (sbsc->flags & HAS_SPBCR) {
+		max_speed_hz = spi->max_speed_hz;
+		rate = clk_get_rate(sbsc->clk);
+
+		div_ratio = 2;
+		spbr = 1;
+		while ((rate / div_ratio) > max_speed_hz) {
+			spbr++;
+			div_ratio += 2;
+			if (spbr == 255)
+				break;
+		}
+		spibsc_write(sbsc, SPBCR, SPBCR_SPBR(spbr) | SPBCR_BRDV(0));
+		dev_dbg(sbsc->dev, "Clock set to %ld Hz\n", rate/div_ratio);
+	}
+
+	return 0;
+}
+
+static int spibsc_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct spi_controller *master;
+	struct spibsc_priv *sbsc;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*sbsc));
+	if (!master)
+		return -ENOMEM;
+
+	sbsc = spi_controller_get_devdata(master);
+	dev_set_drvdata(&pdev->dev, sbsc);
+	sbsc->master	= master;
+	sbsc->dev	= &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sbsc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(sbsc->base)) {
+		ret = PTR_ERR(sbsc->base);
+		goto error;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	sbsc->mmio_size = resource_size(res);
+	sbsc->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(sbsc->mmio)) {
+		ret = PTR_ERR(sbsc->mmio);
+		goto error;
+	}
+
+	sbsc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(sbsc->clk)) {
+		dev_err(&pdev->dev, "cannot get clock\n");
+		ret = PTR_ERR(sbsc->clk);
+		goto error;
+	}
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
+	sbsc->flags = (u32) of_device_get_match_data(&pdev->dev);
+
+	master->bus_num = pdev->id;
+	master->num_chipselect	= 1;
+	master->mode_bits		= SPI_CPOL | SPI_CPHA;
+	master->setup			= spibsc_setup;
+	master->transfer_one_message	= spibsc_transfer_one_message;
+	master->dev.of_node		= pdev->dev.of_node;
+
+	/* Initialize HW */
+	spibsc_write(sbsc, CMNCR, CMNCR_INIT);
+	spibsc_write(sbsc, DRCR, DRCR_RCF);
+	spibsc_write(sbsc, SSLDR, SSLDR_INIT);
+
+	ret = devm_spi_register_controller(&pdev->dev, master);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "spi_register_controller error.\n");
+		goto error;
+	}
+
+	dev_info(&pdev->dev, "probed\n");
+
+	return 0;
+
+error:
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	spi_controller_put(master);
+
+	return ret;
+}
+
+static int spibsc_remove(struct platform_device *pdev)
+{
+	struct spibsc_priv *sbsc = dev_get_drvdata(&pdev->dev);
+
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	spi_unregister_controller(sbsc->master);
+
+	return 0;
+}
+
+static const struct of_device_id of_spibsc_match[] = {
+	{ .compatible = "renesas,spibsc"},
+	{ .compatible = "renesas,r7s72100-spibsc", .data = (void *)HAS_SPBCR},
+	{ .compatible = "renesas,r7s9210-spibsc"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_spibsc_match);
+
+static struct platform_driver spibsc_driver = {
+	.probe = spibsc_probe,
+	.remove = spibsc_remove,
+	.driver = {
+		.name = "spibsc",
+		.owner = THIS_MODULE,
+		.of_match_table = of_spibsc_match,
+	},
+};
+module_platform_driver(spibsc_driver);
+
+MODULE_DESCRIPTION("Renesas SPIBSC SPI Flash driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Chris Brandt");