linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] spi: Add Renesas SPIBSC controller
@ 2019-12-03  3:45 Chris Brandt
  2019-12-03  3:45 ` [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support Chris Brandt
                   ` (5 more replies)
  0 siblings, 6 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

The Renesas SPI Bus Space Controller (SPIBSC) HW was specifically designed for
accessing SPI flash devices. In the hardware manuals, it is almost always
labeled as the "Renesas SPI Multi I/O Bus Controller". However, the HW IP is
usually referred to within Renesas as the "SPIBSC" block.

This driver has been tested on an RZ/A1H RSK and RZ/A2M EVB.

The testing mostly consisted of formatting an area as JFFS2 and doing copying
of files and such.

While the HW changed a little between the RZ/A1 and RZ/A2 generations, the IP
block in the RZ/A2M was taken from the R-Car H3 design, so in theory this
driver should work for R-Car Gen3 as well.



Chris Brandt (6):
  clk: renesas: mstp: Add critical clock from device tree support
  ARM: dts: r7s72100: Add SPIBSC clocks
  clk: renesas: r7s9210: Add SPIBSC clock
  spi: Add SPIBSC driver
  ARM: dts: r7s9210: Add SPIBSC Device support
  dt-bindings: spi: Document Renesas SPIBSC bindings

 .../bindings/spi/spi-renesas-spibsc.txt       |  48 ++
 arch/arm/boot/dts/r7s72100.dtsi               |  26 +-
 arch/arm/boot/dts/r7s9210.dtsi                |  10 +
 drivers/clk/renesas/clk-mstp.c                |  16 +-
 drivers/clk/renesas/r7s9210-cpg-mssr.c        |   9 +
 drivers/spi/Kconfig                           |   8 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-spibsc.c                      | 609 ++++++++++++++++++
 8 files changed, 719 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
 create mode 100644 drivers/spi/spi-spibsc.c

-- 
2.23.0


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support
  2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
@ 2019-12-03  3:45 ` Chris Brandt
  2019-12-03 18:32   ` Geert Uytterhoeven
  2019-12-03  3:45 ` [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks Chris Brandt
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

Allow critical clocks to be specified in the Device Tree.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/clk/renesas/clk-mstp.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 003e9ce45757..8e28e9671265 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -148,7 +148,7 @@ static const struct clk_ops cpg_mstp_clock_ops = {
 
 static struct clk * __init cpg_mstp_clock_register(const char *name,
 	const char *parent_name, unsigned int index,
-	struct mstp_clock_group *group)
+	struct mstp_clock_group *group, unsigned long flags)
 {
 	struct clk_init_data init;
 	struct mstp_clock *clock;
@@ -160,12 +160,12 @@ static struct clk * __init cpg_mstp_clock_register(const char *name,
 
 	init.name = name;
 	init.ops = &cpg_mstp_clock_ops;
-	init.flags = CLK_SET_RATE_PARENT;
+	init.flags = CLK_SET_RATE_PARENT | flags;
 	/* INTC-SYS is the module clock of the GIC, and must not be disabled */
-	if (!strcmp(name, "intc-sys")) {
-		pr_debug("MSTP %s setting CLK_IS_CRITICAL\n", name);
+	if (!strcmp(name, "intc-sys"))
 		init.flags |= CLK_IS_CRITICAL;
-	}
+	if (init.flags & CLK_IS_CRITICAL)
+		pr_debug("MSTP %s setting CLK_IS_CRITICAL\n", name);
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 
@@ -187,6 +187,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	const char *idxname;
 	struct clk **clks;
 	unsigned int i;
+	unsigned long flags;
 
 	group = kzalloc(struct_size(group, clks, MSTP_MAX_CLOCKS), GFP_KERNEL);
 	if (!group)
@@ -239,8 +240,11 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 			continue;
 		}
 
+		flags = 0;
+		of_clk_detect_critical(np, i, &flags);
+
 		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
-						       clkidx, group);
+						       clkidx, group, flags);
 		if (!IS_ERR(clks[clkidx])) {
 			group->data.clk_num = max(group->data.clk_num,
 						  clkidx + 1);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
  2019-12-03  3:45 ` [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support Chris Brandt
@ 2019-12-03  3:45 ` Chris Brandt
  2019-12-03 18:42   ` Geert Uytterhoeven
  2019-12-03  3:45 ` [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock Chris Brandt
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

The SPIBSC-0 clock is marked as critical because for XIP systems, this
is the SPI flash controller it will boot from and the kernel will also
be running from so it cannot be turned off.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s72100.dtsi | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index d03dcd919d6f..a422bbe872bc 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -101,6 +101,26 @@
 		#size-cells = <1>;
 		ranges;
 
+		spibsc0: spi@3fefa000 {
+			compatible = "renesas,r7s72100-spibsc", "renesas,spibsc";
+			reg = <0x3fefa000 0x100>, <0x18000000 0x4000000>;
+			clocks = <&mstp9_clks R7S72100_CLK_SPIBSC0>;
+			power-domains = <&cpg_clocks>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		spibsc1: spi@3fefb000 {
+			compatible = "renesas,r7s72100-spibsc", "renesas,spibsc";
+			reg = <0x3fefb000 0x100>, <0x1c000000 0x4000000>;
+			clocks = <&mstp9_clks R7S72100_CLK_SPIBSC1>;
+			power-domains = <&cpg_clocks>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		L2: cache-controller@3ffff000 {
 			compatible = "arm,pl310-cache";
 			reg = <0x3ffff000 0x1000>;
@@ -467,11 +487,13 @@
 			#clock-cells = <1>;
 			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
 			reg = <0xfcfe0438 4>;
-			clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>;
+			clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>, <&b_clk>, <&b_clk>;
 			clock-indices = <
 				R7S72100_CLK_I2C0 R7S72100_CLK_I2C1 R7S72100_CLK_I2C2 R7S72100_CLK_I2C3
+				R7S72100_CLK_SPIBSC0 R7S72100_CLK_SPIBSC1
 			>;
-			clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3";
+			clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "spibsc0", "spibsc1";
+			clock-critical = <4>; /* spibsc0 */
 		};
 
 		mstp10_clks: mstp10_clks@fcfe043c {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock
  2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
  2019-12-03  3:45 ` [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support Chris Brandt
  2019-12-03  3:45 ` [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks Chris Brandt
@ 2019-12-03  3:45 ` Chris Brandt
  2019-12-03 18:49   ` Geert Uytterhoeven
  2019-12-03  3:45 ` [PATCH 4/6] spi: Add SPIBSC driver Chris Brandt
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

The SPIBSC clocks are marked as critical because for XIP systems, the
kernel will be running from QSPI flash and cannot be turned off.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/clk/renesas/r7s9210-cpg-mssr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/renesas/r7s9210-cpg-mssr.c b/drivers/clk/renesas/r7s9210-cpg-mssr.c
index 14093503c085..153d3a49eee0 100644
--- a/drivers/clk/renesas/r7s9210-cpg-mssr.c
+++ b/drivers/clk/renesas/r7s9210-cpg-mssr.c
@@ -93,6 +93,7 @@ static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = {
 	DEF_MOD_STB("ether1",	 64,	R7S9210_CLK_B),
 	DEF_MOD_STB("ether0",	 65,	R7S9210_CLK_B),
 
+	DEF_MOD_STB("spibsc",	 83,	R7S9210_CLK_P1),
 	DEF_MOD_STB("i2c3",	 84,	R7S9210_CLK_P1),
 	DEF_MOD_STB("i2c2",	 85,	R7S9210_CLK_P1),
 	DEF_MOD_STB("i2c1",	 86,	R7S9210_CLK_P1),
@@ -112,6 +113,10 @@ static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = {
 	DEF_MOD_STB("vdc6",	 81,	R7S9210_CLK_P1),
 };
 
+static const unsigned int r7s9210_crit_mod_clks[] __initconst = {
+	MOD_CLK_ID_10(83),	/* SPIBSC */
+};
+
 /* The clock dividers in the table vary based on DT and register settings */
 static void __init r7s9210_update_clk_table(struct clk *extal_clk,
 					    void __iomem *base)
@@ -213,6 +218,10 @@ const struct cpg_mssr_info r7s9210_cpg_mssr_info __initconst = {
 	.num_mod_clks = ARRAY_SIZE(r7s9210_mod_clks),
 	.num_hw_mod_clks = 11 * 32, /* includes STBCR0 which doesn't exist */
 
+	/* Critical Module Clocks */
+	.crit_mod_clks = r7s9210_crit_mod_clks,
+	.num_crit_mod_clks = ARRAY_SIZE(r7s9210_crit_mod_clks),
+
 	/* Callbacks */
 	.cpg_clk_register = rza2_cpg_clk_register,
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
                   ` (2 preceding siblings ...)
  2019-12-03  3:45 ` [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock Chris Brandt
@ 2019-12-03  3:45 ` Chris Brandt
  2019-12-03 14:19   ` Mark Brown
  2019-12-03 18:28   ` Geert Uytterhoeven
  2019-12-03  3:45 ` [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support Chris Brandt
  2019-12-03  3:45 ` [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings Chris Brandt
  5 siblings, 2 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

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

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");
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support
  2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
                   ` (3 preceding siblings ...)
  2019-12-03  3:45 ` [PATCH 4/6] spi: Add SPIBSC driver Chris Brandt
@ 2019-12-03  3:45 ` Chris Brandt
  2019-12-03 18:59   ` Geert Uytterhoeven
  2019-12-03  3:45 ` [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings Chris Brandt
  5 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

Add SPIBSC Device support for RZ/A2.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s9210.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
index 72b79770e336..ac4949c9e47a 100644
--- a/arch/arm/boot/dts/r7s9210.dtsi
+++ b/arch/arm/boot/dts/r7s9210.dtsi
@@ -68,6 +68,16 @@
 			cache-level = <2>;
 		};
 
+		spibsc: spi@1f800000 {
+			compatible = "renesas,r7s9210-spibsc", "renesas,spibsc";
+			reg = <0x1f800000 0x8c>, <0x20000000 0x10000000 >;
+			clocks = <&cpg CPG_MOD 83>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		scif0: serial@e8007000 {
 			compatible = "renesas,scif-r7s9210";
 			reg = <0xe8007000 0x18>;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
                   ` (4 preceding siblings ...)
  2019-12-03  3:45 ` [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support Chris Brandt
@ 2019-12-03  3:45 ` Chris Brandt
  2019-12-03  9:14   ` Sergei Shtylyov
  2019-12-03 18:57   ` Geert Uytterhoeven
  5 siblings, 2 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-03  3:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang,
	Sergei Shtylyov, Chris Brandt

Document the bindings used by the Renesas SPI bus space controller.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 .../bindings/spi/spi-renesas-spibsc.txt       | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
new file mode 100644
index 000000000000..b5f7081d2d1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
@@ -0,0 +1,48 @@
+Renesas SPI Bus Space Controller (SPIBSC) Device Tree Bindings
+
+Otherwise referred to as the "SPI Multi I/O Bus Controller" in SoC hardware
+manuals. This controller was designed specifically for accessing SPI flash
+devices.
+
+Required properties:
+- compatible: should be an SoC-specific compatible value, followed by
+		"renesas,spibsc" as a fallback.
+		supported SoC-specific values are:
+		"renesas,r7s72100-spibsc"	(RZ/A1)
+		"renesas,r7s9210-spibsc"	(RZ/A2)
+- reg: should contain three register areas:
+       first for the base address of SPIBSC registers,
+       second for the direct mapping read mode
+- clocks: should contain the clock phandle/specifier pair for the module clock.
+- power-domains: should contain the power domain phandle/specifier pair.
+- #address-cells: should be 1
+- #size-cells: should be 0
+- flash: should be represented by a subnode of the SPIBSC node,
+	 its "compatible" property contains "jedec,spi-nor" if SPI is used.
+
+Example:
+
+	spibsc: spi@1f800000 {
+		compatible = "renesas,r7s9210-spibsc", "renesas,spibsc";
+		reg = <0x1f800000 0x8c>, <0x20000000 0x10000000 >;
+		clocks = <&cpg CPG_MOD 83>;
+		power-domains = <&cpg>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		flash@0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-max-frequency = <40000000>;
+
+			partitions {
+				compatible = "fixed-partitions";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				partition@0000000 {
+					label = "u-boot";
+					reg = <0x00000000 0x80000>;
+				};
+			};
+		};
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03  3:45 ` [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings Chris Brandt
@ 2019-12-03  9:14   ` Sergei Shtylyov
  2019-12-03 13:27     ` Chris Brandt
  2019-12-03 18:57   ` Geert Uytterhoeven
  1 sibling, 1 reply; 50+ messages in thread
From: Sergei Shtylyov @ 2019-12-03  9:14 UTC (permalink / raw)
  To: Chris Brandt, Mark Brown, Rob Herring, Mark Rutland,
	Geert Uytterhoeven, Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang

Hello!

On 03.12.2019 6:45, Chris Brandt wrote:

> Document the bindings used by the Renesas SPI bus space controller.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>   .../bindings/spi/spi-renesas-spibsc.txt       | 48 +++++++++++++++++++
>   1 file changed, 48 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
> new file mode 100644
> index 000000000000..b5f7081d2d1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
> @@ -0,0 +1,48 @@
> +Renesas SPI Bus Space Controller (SPIBSC) Device Tree Bindings
> +
> +Otherwise referred to as the "SPI Multi I/O Bus Controller" in SoC hardware
> +manuals. This controller was designed specifically for accessing SPI flash
> +devices.
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> +		"renesas,spibsc" as a fallback.
> +		supported SoC-specific values are:
> +		"renesas,r7s72100-spibsc"	(RZ/A1)
> +		"renesas,r7s9210-spibsc"	(RZ/A2)
> +- reg: should contain three register areas:
> +       first for the base address of SPIBSC registers,
> +       second for the direct mapping read mode

    That's only 2 areas, not 3. :-)

> +- clocks: should contain the clock phandle/specifier pair for the module clock.
> +- power-domains: should contain the power domain phandle/specifier pair.
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- flash: should be represented by a subnode of the SPIBSC node,
> +	 its "compatible" property contains "jedec,spi-nor" if SPI is used.

    Are any other flash variants supported?

> +
> +Example:
> +
> +	spibsc: spi@1f800000 {
> +		compatible = "renesas,r7s9210-spibsc", "renesas,spibsc";
> +		reg = <0x1f800000 0x8c>, <0x20000000 0x10000000 >;
> +		clocks = <&cpg CPG_MOD 83>;
> +		power-domains = <&cpg>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		flash@0 {
> +			compatible = "jedec,spi-nor";
> +			reg = <0>;
> +			spi-max-frequency = <40000000>;
> +
> +			partitions {
> +				compatible = "fixed-partitions";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +
> +				partition@0000000 {
> +					label = "u-boot";
> +					reg = <0x00000000 0x80000>;
> +				};
> +			};
> +		};

MBR, Sergei

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03  9:14   ` Sergei Shtylyov
@ 2019-12-03 13:27     ` Chris Brandt
  2019-12-03 16:04       ` Sergei Shtylyov
  0 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 13:27 UTC (permalink / raw)
  To: Sergei Shtylyov, Mark Brown, Rob Herring, Mark Rutland,
	Geert Uytterhoeven, Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang

On Tue, Dec 3, 2019, Sergei Shtylyov wrote:
>     That's only 2 areas, not 3. :-)
Thank you!

> > +- flash: should be represented by a subnode of the SPIBSC node,
> > +	 its "compatible" property contains "jedec,spi-nor" if SPI is used.
> 
>     Are any other flash variants supported?

Do you mean other types of SPI flash?
I've only used SPI flash devices that are auto detected using 
"jedec,spi-nor".
In theory, you could use other types of SPI flash like "atmel,at45", but
no one has every tried it, mostly because the SoC will only boot from
JEDEC compatible SPI flash.

Chris

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03  3:45 ` [PATCH 4/6] spi: Add SPIBSC driver Chris Brandt
@ 2019-12-03 14:19   ` Mark Brown
  2019-12-03 15:00     ` Chris Brandt
                       ` (2 more replies)
  2019-12-03 18:28   ` Geert Uytterhoeven
  1 sibling, 3 replies; 50+ messages in thread
From: Mark Brown @ 2019-12-03 14:19 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Rob Herring, Mark Rutland, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, linux-spi, devicetree, linux-renesas-soc,
	linux-clk, Mason Yang, Sergei Shtylyov

[-- Attachment #1: Type: text/plain, Size: 2990 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03 14:19   ` Mark Brown
@ 2019-12-03 15:00     ` Chris Brandt
  2019-12-03 18:29     ` Geert Uytterhoeven
  2019-12-04 15:51     ` Chris Brandt
  2 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 15:00 UTC (permalink / raw)
  To: Mark Brown
  Cc: Rob Herring, Mark Rutland, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, linux-spi, devicetree, linux-renesas-soc,
	linux-clk, Mason Yang, Sergei Shtylyov

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 13:27     ` Chris Brandt
@ 2019-12-03 16:04       ` Sergei Shtylyov
  2019-12-03 16:35         ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Sergei Shtylyov @ 2019-12-03 16:04 UTC (permalink / raw)
  To: Chris Brandt, Mark Brown, Rob Herring, Mark Rutland,
	Geert Uytterhoeven, Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang

On 12/03/2019 04:27 PM, Chris Brandt wrote:

>>> +- flash: should be represented by a subnode of the SPIBSC node,
>>> +	 its "compatible" property contains "jedec,spi-nor" if SPI is used.
>>
>>     Are any other flash variants supported?
> 
> Do you mean other types of SPI flash?

   No, I mean flashes connected via different buses, like HyperBus with the gen3 SoC RPC-IF.
If SPI's the only bu supported, there's no point saying "if SPI is used".

[...]
> Chris

MBR, Sergei

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 16:04       ` Sergei Shtylyov
@ 2019-12-03 16:35         ` Chris Brandt
  0 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 16:35 UTC (permalink / raw)
  To: Sergei Shtylyov, Mark Brown, Rob Herring, Mark Rutland,
	Geert Uytterhoeven, Michael Turquette, Stephen Boyd
  Cc: linux-spi, devicetree, linux-renesas-soc, linux-clk, Mason Yang

On Tue, Dec 3, 2019 1, Sergei Shtylyov wrote:
> > Do you mean other types of SPI flash?
> 
>    No, I mean flashes connected via different buses, like HyperBus with the
> gen3 SoC RPC-IF.
> If SPI's the only bu supported, there's no point saying "if SPI is used".

OK, I see your point. I will remove the 'if'.

The hardware in RZ/A2 also supports HyperFlash and OctaFlash (same as gen3), but
RZ/A1 only supports SPI flash.
Therefore this driver does not touch PHYCNT.PHYMEM and assumes it is at 0.

Chris

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03  3:45 ` [PATCH 4/6] spi: Add SPIBSC driver Chris Brandt
  2019-12-03 14:19   ` Mark Brown
@ 2019-12-03 18:28   ` Geert Uytterhoeven
  2019-12-04 11:18     ` Mark Brown
  2019-12-04 22:12     ` Chris Brandt
  1 sibling, 2 replies; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:28 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03 14:19   ` Mark Brown
  2019-12-03 15:00     ` Chris Brandt
@ 2019-12-03 18:29     ` Geert Uytterhoeven
  2019-12-04 11:25       ` Mark Brown
  2019-12-04 15:51     ` Chris Brandt
  2 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:29 UTC (permalink / raw)
  To: Mark Brown
  Cc: Chris Brandt, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

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

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support
  2019-12-03  3:45 ` [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support Chris Brandt
@ 2019-12-03 18:32   ` Geert Uytterhoeven
  2019-12-03 18:46     ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:32 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> Allow critical clocks to be specified in the Device Tree.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Notes:
  1. Assumed we really need this,
  2. Minor nit below.

> --- a/drivers/clk/renesas/clk-mstp.c
> +++ b/drivers/clk/renesas/clk-mstp.c

> @@ -187,6 +187,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
>         const char *idxname;
>         struct clk **clks;
>         unsigned int i;
> +       unsigned long flags;

= 0 here...

>
>         group = kzalloc(struct_size(group, clks, MSTP_MAX_CLOCKS), GFP_KERNEL);
>         if (!group)
> @@ -239,8 +240,11 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
>                         continue;
>                 }
>
> +               flags = 0;

... instead of here?

> +               of_clk_detect_critical(np, i, &flags);
> +
>                 clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
> -                                                      clkidx, group);
> +                                                      clkidx, group, flags);
>                 if (!IS_ERR(clks[clkidx])) {
>                         group->data.clk_num = max(group->data.clk_num,
>                                                   clkidx + 1);

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-03  3:45 ` [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks Chris Brandt
@ 2019-12-03 18:42   ` Geert Uytterhoeven
  2019-12-03 18:57     ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:42 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> The SPIBSC-0 clock is marked as critical because for XIP systems, this
> is the SPI flash controller it will boot from and the kernel will also
> be running from so it cannot be turned off.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -101,6 +101,26 @@
>                 #size-cells = <1>;
>                 ranges;
>
> +               spibsc0: spi@3fefa000 {
> +                       compatible = "renesas,r7s72100-spibsc", "renesas,spibsc";
> +                       reg = <0x3fefa000 0x100>, <0x18000000 0x4000000>;

The second region conflicts with the XIP flash@18000000 in
arch/arm/boot/dts/r7s72100-gr-peach.dts.
Yes, I know it is the same device ;-)

> +                       clocks = <&mstp9_clks R7S72100_CLK_SPIBSC0>;
> +                       power-domains = <&cpg_clocks>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       status = "disabled";
> +               };
> +
> +               spibsc1: spi@3fefb000 {
> +                       compatible = "renesas,r7s72100-spibsc", "renesas,spibsc";
> +                       reg = <0x3fefb000 0x100>, <0x1c000000 0x4000000>;
> +                       clocks = <&mstp9_clks R7S72100_CLK_SPIBSC1>;
> +                       power-domains = <&cpg_clocks>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       status = "disabled";
> +               };
> +
>                 L2: cache-controller@3ffff000 {
>                         compatible = "arm,pl310-cache";
>                         reg = <0x3ffff000 0x1000>;
> @@ -467,11 +487,13 @@
>                         #clock-cells = <1>;
>                         compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
>                         reg = <0xfcfe0438 4>;
> -                       clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>;
> +                       clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>, <&b_clk>, <&b_clk>;
>                         clock-indices = <
>                                 R7S72100_CLK_I2C0 R7S72100_CLK_I2C1 R7S72100_CLK_I2C2 R7S72100_CLK_I2C3
> +                               R7S72100_CLK_SPIBSC0 R7S72100_CLK_SPIBSC1
>                         >;
> -                       clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3";
> +                       clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "spibsc0", "spibsc1";
> +                       clock-critical = <4>; /* spibsc0 */

Iff we go this clock-critical route, I think this should be specified in the
board-specific .dts instead of in the SoC-specific .dtsi.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support
  2019-12-03 18:32   ` Geert Uytterhoeven
@ 2019-12-03 18:46     ` Chris Brandt
  2019-12-03 18:51       ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 18:46 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

Thank you for your review!

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +       unsigned long flags;
> 
> = 0 here...
> >
> > +               flags = 0;
> 
> ... instead of here?

That was my first thought too...and it was wrong.

If of_clk_detect_critical does NOT detect a critical clock, it does not 
touch flags at all.
And since it is a loop, what you get is after the first clock is marked 
as CRITICAL, all the remaining clocks also get marked CRITICAL. In this 
case, both spibsc0 and spibsc1 get marked critical. That's why I have to
reset it for each loop.

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock
  2019-12-03  3:45 ` [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock Chris Brandt
@ 2019-12-03 18:49   ` Geert Uytterhoeven
  2019-12-03 19:09     ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:49 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> The SPIBSC clocks are marked as critical because for XIP systems, the
> kernel will be running from QSPI flash and cannot be turned off.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r7s9210-cpg-mssr.c
> +++ b/drivers/clk/renesas/r7s9210-cpg-mssr.c
> @@ -93,6 +93,7 @@ static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = {
>         DEF_MOD_STB("ether1",    64,    R7S9210_CLK_B),
>         DEF_MOD_STB("ether0",    65,    R7S9210_CLK_B),
>
> +       DEF_MOD_STB("spibsc",    83,    R7S9210_CLK_P1),

OK.

>         DEF_MOD_STB("i2c3",      84,    R7S9210_CLK_P1),
>         DEF_MOD_STB("i2c2",      85,    R7S9210_CLK_P1),
>         DEF_MOD_STB("i2c1",      86,    R7S9210_CLK_P1),
> @@ -112,6 +113,10 @@ static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = {
>         DEF_MOD_STB("vdc6",      81,    R7S9210_CLK_P1),
>  };
>
> +static const unsigned int r7s9210_crit_mod_clks[] __initconst = {
> +       MOD_CLK_ID_10(83),      /* SPIBSC */

This is only a critical clock if XIP is in use, right?
Can we do better, and only mark it critical if we detect the FLASH is
used in XIP mode?
E.g. by using for_each_compatible_node(..., "mtd-rom"), and checking if
any of the corresponding register blocks matches the SPIBSC FLASH
memory window?

> +};
> +
>  /* The clock dividers in the table vary based on DT and register settings */
>  static void __init r7s9210_update_clk_table(struct clk *extal_clk,
>                                             void __iomem *base)
> @@ -213,6 +218,10 @@ const struct cpg_mssr_info r7s9210_cpg_mssr_info __initconst = {
>         .num_mod_clks = ARRAY_SIZE(r7s9210_mod_clks),
>         .num_hw_mod_clks = 11 * 32, /* includes STBCR0 which doesn't exist */
>
> +       /* Critical Module Clocks */
> +       .crit_mod_clks = r7s9210_crit_mod_clks,
> +       .num_crit_mod_clks = ARRAY_SIZE(r7s9210_crit_mod_clks),
> +
>         /* Callbacks */
>         .cpg_clk_register = rza2_cpg_clk_register,

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support
  2019-12-03 18:46     ` Chris Brandt
@ 2019-12-03 18:51       ` Geert Uytterhoeven
  0 siblings, 0 replies; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:51 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 7:46 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > +       unsigned long flags;
> >
> > = 0 here...
> > >
> > > +               flags = 0;
> >
> > ... instead of here?
>
> That was my first thought too...and it was wrong.
>
> If of_clk_detect_critical does NOT detect a critical clock, it does not
> touch flags at all.
> And since it is a loop, what you get is after the first clock is marked
> as CRITICAL, all the remaining clocks also get marked CRITICAL. In this
> case, both spibsc0 and spibsc1 get marked critical. That's why I have to
> reset it for each loop.

Thanks, I missed this is done inside a loop.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03  3:45 ` [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings Chris Brandt
  2019-12-03  9:14   ` Sergei Shtylyov
@ 2019-12-03 18:57   ` Geert Uytterhoeven
  2019-12-03 20:39     ` Geert Uytterhoeven
  2019-12-03 22:33     ` Chris Brandt
  1 sibling, 2 replies; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:57 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 4:47 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> Document the bindings used by the Renesas SPI bus space controller.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt

Checkpatch.pl says:
WARNING: DT bindings should be in DT schema format. See:
Documentation/devicetree/writing-schema.rst

> @@ -0,0 +1,48 @@
> +Renesas SPI Bus Space Controller (SPIBSC) Device Tree Bindings
> +
> +Otherwise referred to as the "SPI Multi I/O Bus Controller" in SoC hardware
> +manuals. This controller was designed specifically for accessing SPI flash
> +devices.
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> +               "renesas,spibsc" as a fallback.
> +               supported SoC-specific values are:
> +               "renesas,r7s72100-spibsc"       (RZ/A1)
> +               "renesas,r7s9210-spibsc"        (RZ/A2)

Is the fallback valid for RZ/A1, which has its own special match entry
in the driver?
Will it be valid for R-Car Gen3?
If not, you may want to drop it completely.

> +- reg: should contain three register areas:
> +       first for the base address of SPIBSC registers,
> +       second for the direct mapping read mode
> +- clocks: should contain the clock phandle/specifier pair for the module clock.
> +- power-domains: should contain the power domain phandle/specifier pair.
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- flash: should be represented by a subnode of the SPIBSC node,
> +        its "compatible" property contains "jedec,spi-nor" if SPI is used.

What about the "mtd-rom" use for e.g. XIP?

interrupts? RZ/A2M seems to have an SPIBSC interrupt, RZ/A1 hasn't.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-03 18:42   ` Geert Uytterhoeven
@ 2019-12-03 18:57     ` Chris Brandt
  2019-12-03 19:12       ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 18:57 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +                       reg = <0x3fefa000 0x100>, <0x18000000
> > + 0x4000000>;
> 
> The second region conflicts with the XIP flash@18000000 in
> arch/arm/boot/dts/r7s72100-gr-peach.dts.
> Yes, I know it is the same device ;-)

Is that just an FYI?? Or do you have some suggestion??


> > +                       clock-critical = <4>; /* spibsc0 */
> 
> Iff we go this clock-critical route, I think this should be specified in the
> board-specific .dts instead of in the SoC-specific .dtsi.

OK, that's fine. It makes more sense to be in the .dts because it's a board
design decision. I will remove it from the patch.

The one 'tricky' thing is that the <4> is the index into the number of clocks.

So in the Renesas BSP where it includes the VDC5 LCD controllers,

clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1";

the <4> needs to become a <6>.

Chris

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support
  2019-12-03  3:45 ` [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support Chris Brandt
@ 2019-12-03 18:59   ` Geert Uytterhoeven
  2019-12-03 22:38     ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 18:59 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add SPIBSC Device support for RZ/A2.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- a/arch/arm/boot/dts/r7s9210.dtsi
> +++ b/arch/arm/boot/dts/r7s9210.dtsi
> @@ -68,6 +68,16 @@
>                         cache-level = <2>;
>                 };
>
> +               spibsc: spi@1f800000 {
> +                       compatible = "renesas,r7s9210-spibsc", "renesas,spibsc";
> +                       reg = <0x1f800000 0x8c>, <0x20000000 0x10000000 >;

Any specific reason you're using 0x8c, not 0x100?

> +                       clocks = <&cpg CPG_MOD 83>;
> +                       power-domains = <&cpg>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;

interrupts?

> +                       status = "disabled";
> +               };
> +
>                 scif0: serial@e8007000 {
>                         compatible = "renesas,scif-r7s9210";
>                         reg = <0xe8007000 0x18>;

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock
  2019-12-03 18:49   ` Geert Uytterhoeven
@ 2019-12-03 19:09     ` Chris Brandt
  2019-12-03 20:40       ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 19:09 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +static const unsigned int r7s9210_crit_mod_clks[] __initconst = {
> > +       MOD_CLK_ID_10(83),      /* SPIBSC */
> 
> This is only a critical clock if XIP is in use, right?

Correct.


> Can we do better, and only mark it critical if we detect the FLASH is used in
> XIP mode?
> E.g. by using for_each_compatible_node(..., "mtd-rom"), and checking if any
> of the corresponding register blocks matches the SPIBSC FLASH memory window?

Well...technically...you don't need the "mtd-rom" partition when using the AXFS
file system. But, we can make a rule that you have to use it regardless.

Instead, I think it would be better if we could flag the clock as critical 
in a board-specific .dts like we're going to do with the RZ/A1 MSTP driver.

Can we add something like  "clock-critical = <83>;" to the cpg-mssr driver?

What do you think?

Chris

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-03 18:57     ` Chris Brandt
@ 2019-12-03 19:12       ` Geert Uytterhoeven
  2019-12-04  8:38         ` Lee Jones
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 19:12 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Michael Turquette,
	Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov, Lee Jones

Hi Chris,

CC Lee (for clock-critical)

On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > +                       reg = <0x3fefa000 0x100>, <0x18000000
> > > + 0x4000000>;
> >
> > The second region conflicts with the XIP flash@18000000 in
> > arch/arm/boot/dts/r7s72100-gr-peach.dts.
> > Yes, I know it is the same device ;-)
>
> Is that just an FYI?? Or do you have some suggestion??

Can the flash subnode be compatible with "mtd-rom", even if the parent node
is kept disabled?

> > > +                       clock-critical = <4>; /* spibsc0 */
> >
> > Iff we go this clock-critical route, I think this should be specified in the
> > board-specific .dts instead of in the SoC-specific .dtsi.
>
> OK, that's fine. It makes more sense to be in the .dts because it's a board
> design decision. I will remove it from the patch.
>
> The one 'tricky' thing is that the <4> is the index into the number of clocks.
>
> So in the Renesas BSP where it includes the VDC5 LCD controllers,
>
> clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1";
>
> the <4> needs to become a <6>.

Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1],
and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT?

Unfortunately the exact semantics of clock-critical were never documented.
Lee?

Thanks!

[1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device
tree support"
    https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/


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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 18:57   ` Geert Uytterhoeven
@ 2019-12-03 20:39     ` Geert Uytterhoeven
  2019-12-04  2:54       ` Chris Brandt
  2019-12-04  8:40       ` Geert Uytterhoeven
  2019-12-03 22:33     ` Chris Brandt
  1 sibling, 2 replies; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 20:39 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 7:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 3, 2019 at 4:47 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> > Document the bindings used by the Renesas SPI bus space controller.
> >
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
> > @@ -0,0 +1,48 @@
> > +Renesas SPI Bus Space Controller (SPIBSC) Device Tree Bindings
> > +
> > +Otherwise referred to as the "SPI Multi I/O Bus Controller" in SoC hardware
> > +manuals. This controller was designed specifically for accessing SPI flash
> > +devices.
> > +
> > +Required properties:
> > +- compatible: should be an SoC-specific compatible value, followed by
> > +               "renesas,spibsc" as a fallback.
> > +               supported SoC-specific values are:
> > +               "renesas,r7s72100-spibsc"       (RZ/A1)
> > +               "renesas,r7s9210-spibsc"        (RZ/A2)
>
> Is the fallback valid for RZ/A1, which has its own special match entry
> in the driver?
> Will it be valid for R-Car Gen3?
> If not, you may want to drop it completely.
>
> > +- reg: should contain three register areas:
> > +       first for the base address of SPIBSC registers,
> > +       second for the direct mapping read mode
> > +- clocks: should contain the clock phandle/specifier pair for the module clock.
> > +- power-domains: should contain the power domain phandle/specifier pair.
> > +- #address-cells: should be 1
> > +- #size-cells: should be 0
> > +- flash: should be represented by a subnode of the SPIBSC node,
> > +        its "compatible" property contains "jedec,spi-nor" if SPI is used.
>
> What about the "mtd-rom" use for e.g. XIP?

I gave this some more thought. Basically there are two modes: SPI FLASH
and direct mapped emulation (HyperFLASH could be a third mode).
The bindings described above are for the SPI FLASH use-case.

For the direct mapped use-case, you need different bindings:
  1. Append "simple-pm-bus" to the list of compatible values,
  2. Add a "ranges" property,
  3. The flash subnode becomes directly mapped, and must be compatible
     with "mtd-rom", cfr. the CFI FLASH on ape6evm:
     arch/arm/boot/dts/r8a73a4.dtsi:bus@fec10000 and
     arch/arm/boot/dts/r8a73a4-ape6evm.dts:flash@0.

On the driver side, if your spibsc driver does not find a flash subnode
that is compatible with "jedec,spi-nor", it should return -ENODEV, so
drivers/bus/simple-pm-bus.c can take over for the second mode, if needed.

Once you have added basic Runtime PM support to
drivers/mtd/maps/physmap-core.c:physmap_flash_probe(), the module clock
should be kept enabled through the clock domain when using direct mapped
mode (hmm, as the driver currently lacks this, it means the FLASH on
ape6evm must rely on the bsc module clock being kept enabled through the
Ethernet controller connected to the same bsc module?).

Does this make sense?
Thanks!

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock
  2019-12-03 19:09     ` Chris Brandt
@ 2019-12-03 20:40       ` Geert Uytterhoeven
  2019-12-04  3:09         ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-03 20:40 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 8:09 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > +static const unsigned int r7s9210_crit_mod_clks[] __initconst = {
> > > +       MOD_CLK_ID_10(83),      /* SPIBSC */
> >
> > This is only a critical clock if XIP is in use, right?
>
> Correct.
>
>
> > Can we do better, and only mark it critical if we detect the FLASH is used in
> > XIP mode?
> > E.g. by using for_each_compatible_node(..., "mtd-rom"), and checking if any
> > of the corresponding register blocks matches the SPIBSC FLASH memory window?
>
> Well...technically...you don't need the "mtd-rom" partition when using the AXFS
> file system. But, we can make a rule that you have to use it regardless.

Just wondering, how does AXFS access the FLASH without it being mapped
using "mtd-rom"?

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 18:57   ` Geert Uytterhoeven
  2019-12-03 20:39     ` Geert Uytterhoeven
@ 2019-12-03 22:33     ` Chris Brandt
  2019-12-04  8:43       ` Geert Uytterhoeven
  1 sibling, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 22:33 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

As always, thank you for the review.

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
> 
> Checkpatch.pl says:
> WARNING: DT bindings should be in DT schema format. See:
> Documentation/devicetree/writing-schema.rst

:(


> > +Required properties:
> > +- compatible: should be an SoC-specific compatible value, followed by
> > +               "renesas,spibsc" as a fallback.
> > +               supported SoC-specific values are:
> > +               "renesas,r7s72100-spibsc"       (RZ/A1)
> > +               "renesas,r7s9210-spibsc"        (RZ/A2)
> 
> Is the fallback valid for RZ/A1, which has its own special match entry in the
> driver?
> Will it be valid for R-Car Gen3?
> If not, you may want to drop it completely.

The fallback would still work for RZ/A1, you just would not be able to
set the baud rate. But, I have no problem dropping the fallback. I'm fine
with having a compatible string for each SoC that is known to work for.

I have not tried it with Gen3, but I would guess there will be some minor
difference that will needed to be accounted for.


> > +- reg: should contain three register areas:
> > +       first for the base address of SPIBSC registers,
> > +       second for the direct mapping read mode
> > +- clocks: should contain the clock phandle/specifier pair for the module
> clock.
> > +- power-domains: should contain the power domain phandle/specifier pair.
> > +- #address-cells: should be 1
> > +- #size-cells: should be 0
> > +- flash: should be represented by a subnode of the SPIBSC node,
> > +        its "compatible" property contains "jedec,spi-nor" if SPI is used.
> 
> What about the "mtd-rom" use for e.g. XIP?

But "mtd-rom" doesn't really have anything to do with the functionality of the
driver when it is being used in "SPI mode".
Maybe I just remove any mention of this for now.


> interrupts? RZ/A2M seems to have an SPIBSC interrupt, RZ/A1 hasn't.

There was never any interrupts in the SPIBSC.
But it looks like when they added HyperFlash and OctaFlash support, they put
in some interrupts for that.
And now that I look at it, they are for pins labeled RPC_INT, RPC_WC, RPC_RESET.
(I just realized that "RPC" stands for "Reduced Pin Count")

So....am I supposed to add in that interrupt even though I'm not planning on using
it??

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support
  2019-12-03 18:59   ` Geert Uytterhoeven
@ 2019-12-03 22:38     ` Chris Brandt
  2019-12-04  7:57       ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-03 22:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +                       reg = <0x1f800000 0x8c>, <0x20000000
> > + 0x10000000 >;
> 
> Any specific reason you're using 0x8c, not 0x100?

Because....I keep forgetting what is the latest 'correct' size:
  A. The exact size of the register range
or
  B. The size rounded up to look nicer


> > +                       clocks = <&cpg CPG_MOD 83>;
> > +                       power-domains = <&cpg>;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <0>;
> 
> interrupts?

I guess I can put it in there just to correctly 'document the hardware'
in the Device Tree file.

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 20:39     ` Geert Uytterhoeven
@ 2019-12-04  2:54       ` Chris Brandt
  2019-12-04  8:56         ` Geert Uytterhoeven
  2019-12-04  8:40       ` Geert Uytterhoeven
  1 sibling, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-04  2:54 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > What about the "mtd-rom" use for e.g. XIP?
> 
> I gave this some more thought. Basically there are two modes: SPI FLASH and
> direct mapped emulation (HyperFLASH could be a third mode).
> The bindings described above are for the SPI FLASH use-case.

I would say in general, there are just two modes "SPI Mode" which was 
intended to do things like discover the attached flash and erase/writing.
And direct mapped which was intended only for reading. Both of those 
modes were intended to be used for QSPI flash, HyperFlash or OctaFlash. 
There's a register bit you set to tell the PHY what you are talking to.


> On the driver side, if your spibsc driver does not find a flash subnode that
> is compatible with "jedec,spi-nor", it should return -ENODEV, so
> drivers/bus/simple-pm-bus.c can take over for the second mode, if needed.

I think here is the bigger issue/question/decision.

This one IP block supports 3 different types of Flash: QSPI, Hyper, Octa.
Also, it runs in 2 mode:
 "SPI Mode" for writing and other stuff
 "Direct Mode" Read only, but faster and directly accessible.

 (QSPI also supports 1-bit,2-bit,4-bit, and 8-bit(dual)....but we'll
  forget about that for now )

So the question is if someone really wants to use it in "direct mode" 
most of the time, but also need to switch back into "SPI mode" to rewrite 
the flash, should this driver handle both cases?

Basically, it's like the 'role switch' in the USB OTG drivers.

This driver I created was just attempting to cover the "SPI mode" case 
for those that want to be able to re-write u-boot at run-time. And, it 
could be extended to support HyperFlash and OctaFlash in SPI mode as well 
(you use the same registers, but the commands are different).

So my suggestion is to forget about trying to 'support' direct mode in 
this driver at the moment. If you're using this HW for something like 
XIP, then don't enable this driver at all (which is what we have been 
doing).

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock
  2019-12-03 20:40       ` Geert Uytterhoeven
@ 2019-12-04  3:09         ` Chris Brandt
  0 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-04  3:09 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

> > Well...technically...you don't need the "mtd-rom" partition when using
> > the AXFS file system. But, we can make a rule that you have to use it
> regardless.
> 
> Just wondering, how does AXFS access the FLASH without it being mapped using
> "mtd-rom"?

ioreamp in the driver (the physical address gets passed as an argument).
But, as the DAX people found out, ioremap in a file system drivers is a 
big no-no when you are trying to get a new file system into mainline.
Nicolas (Pitre) also ran into this when he was adding XIP support to 
cramfs last year....hence mtd-rom is mandatory for that.

I would like us to be able to flag a clock as critical without a 
specific use case to look for (ie, looking for mtd-rom).

For RZ/A1, we're all set because we can do it in the board's .dst. Easy!

I just wish it was that easy for RZ/A2 (using the newer renesas-cpg-mssr
driver).


Chris

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support
  2019-12-03 22:38     ` Chris Brandt
@ 2019-12-04  7:57       ` Geert Uytterhoeven
  2019-12-04 11:04         ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-04  7:57 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 11:38 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > +                       reg = <0x1f800000 0x8c>, <0x20000000
> > > + 0x10000000 >;
> >
> > Any specific reason you're using 0x8c, not 0x100?
>
> Because....I keep forgetting what is the latest 'correct' size:
>   A. The exact size of the register range
> or
>   B. The size rounded up to look nicer

C. The size used by the on-chip address decoder providing the module's
   select signal? I doubt that's not a power of two ;-)

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-03 19:12       ` Geert Uytterhoeven
@ 2019-12-04  8:38         ` Lee Jones
  2019-12-04  9:03           ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Lee Jones @ 2019-12-04  8:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Chris Brandt, Mark Brown, Rob Herring, Mark Rutland,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

On Tue, 03 Dec 2019, Geert Uytterhoeven wrote:

> Hi Chris,
> 
> CC Lee (for clock-critical)
> 
> On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > > +                       reg = <0x3fefa000 0x100>, <0x18000000
> > > > + 0x4000000>;
> > >
> > > The second region conflicts with the XIP flash@18000000 in
> > > arch/arm/boot/dts/r7s72100-gr-peach.dts.
> > > Yes, I know it is the same device ;-)
> >
> > Is that just an FYI?? Or do you have some suggestion??
> 
> Can the flash subnode be compatible with "mtd-rom", even if the parent node
> is kept disabled?
> 
> > > > +                       clock-critical = <4>; /* spibsc0 */
> > >
> > > Iff we go this clock-critical route, I think this should be specified in the
> > > board-specific .dts instead of in the SoC-specific .dtsi.
> >
> > OK, that's fine. It makes more sense to be in the .dts because it's a board
> > design decision. I will remove it from the patch.
> >
> > The one 'tricky' thing is that the <4> is the index into the number of clocks.
> >
> > So in the Renesas BSP where it includes the VDC5 LCD controllers,
> >
> > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1";
> >
> > the <4> needs to become a <6>.
> 
> Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1],
> and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT?
> 
> Unfortunately the exact semantics of clock-critical were never documented.
> Lee?

/**
 * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
 * @np: Device node pointer associated with clock provider
 * @index: clock index
 * @flags: pointer to top-level framework flags
 *
 * Detects if the clock-critical property exists and, if so, sets the
 * corresponding CLK_IS_CRITICAL flag.
 *
 * Do not use this function. It exists only for legacy Device Tree
 * bindings, such as the one-clock-per-node style that are outdated.
 * Those bindings typically put all clock data into .dts and the Linux
 * driver has no clock data, thus making it impossible to set this flag
 * correctly from the driver. Only those drivers may call
 * of_clk_detect_critical from their setup functions.
 *
 * Return: error code or zero on success
 */

If this meets the criteria, the API is pretty simple/self
explanatory.  What are you having trouble with?

> Thanks!
> 
> [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device
> tree support"
>     https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 20:39     ` Geert Uytterhoeven
  2019-12-04  2:54       ` Chris Brandt
@ 2019-12-04  8:40       ` Geert Uytterhoeven
  1 sibling, 0 replies; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-04  8:40 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

On Tue, Dec 3, 2019 at 9:39 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 3, 2019 at 7:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Once you have added basic Runtime PM support to
> drivers/mtd/maps/physmap-core.c:physmap_flash_probe(), the module clock
> should be kept enabled through the clock domain when using direct mapped
> mode (hmm, as the driver currently lacks this, it means the FLASH on
> ape6evm must rely on the bsc module clock being kept enabled through the
> Ethernet controller connected to the same bsc module?).

JFTR, this is indeed broken: after removing the Ethernet node from
r8a73a4-ape6evm.dts, accessing the FLASH crashes the system with an
imprecise external abort.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-03 22:33     ` Chris Brandt
@ 2019-12-04  8:43       ` Geert Uytterhoeven
  2019-12-04 11:19         ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-04  8:43 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Tue, Dec 3, 2019 at 11:33 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-spibsc.txt
> > > +Required properties:
> > > +- compatible: should be an SoC-specific compatible value, followed by
> > > +               "renesas,spibsc" as a fallback.
> > > +               supported SoC-specific values are:
> > > +               "renesas,r7s72100-spibsc"       (RZ/A1)
> > > +               "renesas,r7s9210-spibsc"        (RZ/A2)
> >
> > Is the fallback valid for RZ/A1, which has its own special match entry in the
> > driver?
> > Will it be valid for R-Car Gen3?
> > If not, you may want to drop it completely.
>
> The fallback would still work for RZ/A1, you just would not be able to
> set the baud rate. But, I have no problem dropping the fallback. I'm fine
> with having a compatible string for each SoC that is known to work for.
>
> I have not tried it with Gen3, but I would guess there will be some minor
> difference that will needed to be accounted for.

OK.

> > > +- reg: should contain three register areas:
> > > +       first for the base address of SPIBSC registers,
> > > +       second for the direct mapping read mode
> > > +- clocks: should contain the clock phandle/specifier pair for the module
> > clock.
> > > +- power-domains: should contain the power domain phandle/specifier pair.
> > > +- #address-cells: should be 1
> > > +- #size-cells: should be 0
> > > +- flash: should be represented by a subnode of the SPIBSC node,
> > > +        its "compatible" property contains "jedec,spi-nor" if SPI is used.
> >
> > What about the "mtd-rom" use for e.g. XIP?
>
> But "mtd-rom" doesn't really have anything to do with the functionality of the
> driver when it is being used in "SPI mode".

Correct. But DT describes hardware.  If the FLASH is used in direct mapped
mode, that should be described in DT.

> Maybe I just remove any mention of this for now.
>
> > interrupts? RZ/A2M seems to have an SPIBSC interrupt, RZ/A1 hasn't.
>
> There was never any interrupts in the SPIBSC.
> But it looks like when they added HyperFlash and OctaFlash support, they put
> in some interrupts for that.
> And now that I look at it, they are for pins labeled RPC_INT, RPC_WC, RPC_RESET.
> (I just realized that "RPC" stands for "Reduced Pin Count")
>
> So....am I supposed to add in that interrupt even though I'm not planning on using
> it??

DT describes hardware, not driver limitations.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-04  2:54       ` Chris Brandt
@ 2019-12-04  8:56         ` Geert Uytterhoeven
  2019-12-04 13:31           ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-04  8:56 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Wed, Dec 4, 2019 at 3:55 AM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > What about the "mtd-rom" use for e.g. XIP?
> >
> > I gave this some more thought. Basically there are two modes: SPI FLASH and
> > direct mapped emulation (HyperFLASH could be a third mode).
> > The bindings described above are for the SPI FLASH use-case.
>
> I would say in general, there are just two modes "SPI Mode" which was
> intended to do things like discover the attached flash and erase/writing.
> And direct mapped which was intended only for reading. Both of those
> modes were intended to be used for QSPI flash, HyperFlash or OctaFlash.
> There's a register bit you set to tell the PHY what you are talking to.

OK.

> > On the driver side, if your spibsc driver does not find a flash subnode that
> > is compatible with "jedec,spi-nor", it should return -ENODEV, so
> > drivers/bus/simple-pm-bus.c can take over for the second mode, if needed.
>
> I think here is the bigger issue/question/decision.
>
> This one IP block supports 3 different types of Flash: QSPI, Hyper, Octa.
> Also, it runs in 2 mode:
>  "SPI Mode" for writing and other stuff
>  "Direct Mode" Read only, but faster and directly accessible.
>
>  (QSPI also supports 1-bit,2-bit,4-bit, and 8-bit(dual)....but we'll
>   forget about that for now )

To avoid future problems, you probably do want to specify
spi-tx-bus-width = <4> and spi-rx-bus-width = <4> in DTS now.

> So the question is if someone really wants to use it in "direct mode"
> most of the time, but also need to switch back into "SPI mode" to rewrite
> the flash, should this driver handle both cases?
>
> Basically, it's like the 'role switch' in the USB OTG drivers.

If you want to do that, both configurations should be described in DT,
and we need a way to specify what's being used.
I guess if the direct mapped mode is used, you always want to boot the
kernel using that mode, and only switch to SPI mode temporarily after
boot?  So that could be handled by manually unbinding the driver
from physmap-flash, and forcing a bind to spibsc, all from sysfs.
(Which cuts the branch the kernel is sitting on in the case of XIP...)

Suggestions are welcome!

> This driver I created was just attempting to cover the "SPI mode" case
> for those that want to be able to re-write u-boot at run-time. And, it
> could be extended to support HyperFlash and OctaFlash in SPI mode as well
> (you use the same registers, but the commands are different).

OK.

> So my suggestion is to forget about trying to 'support' direct mode in
> this driver at the moment. If you're using this HW for something like
> XIP, then don't enable this driver at all (which is what we have been
> doing).

Still, the direct-mapped mode should be described in DT, when used.
arch/arm/boot/dts/r7s72100-gr-peach.dts does describe the FLASH,
but fails to describe the exact topology (flash is a child of spibsc,
and thus relies on the spibsc module clock being turned on).

BTW, when using spibsc in direct-mapped mode: if you turn of and on
again the module clock, does the spibsc need reprogramming?

Thanks!

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-04  8:38         ` Lee Jones
@ 2019-12-04  9:03           ` Geert Uytterhoeven
  2019-12-04  9:47             ` Lee Jones
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-04  9:03 UTC (permalink / raw)
  To: Lee Jones
  Cc: Chris Brandt, Mark Brown, Rob Herring, Mark Rutland,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Lee,

On Wed, Dec 4, 2019 at 9:38 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 03 Dec 2019, Geert Uytterhoeven wrote:
> > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > > > +                       reg = <0x3fefa000 0x100>, <0x18000000
> > > > > + 0x4000000>;
> > > >
> > > > The second region conflicts with the XIP flash@18000000 in
> > > > arch/arm/boot/dts/r7s72100-gr-peach.dts.
> > > > Yes, I know it is the same device ;-)
> > >
> > > Is that just an FYI?? Or do you have some suggestion??
> >
> > Can the flash subnode be compatible with "mtd-rom", even if the parent node
> > is kept disabled?
> >
> > > > > +                       clock-critical = <4>; /* spibsc0 */
> > > >
> > > > Iff we go this clock-critical route, I think this should be specified in the
> > > > board-specific .dts instead of in the SoC-specific .dtsi.
> > >
> > > OK, that's fine. It makes more sense to be in the .dts because it's a board
> > > design decision. I will remove it from the patch.
> > >
> > > The one 'tricky' thing is that the <4> is the index into the number of clocks.
> > >
> > > So in the Renesas BSP where it includes the VDC5 LCD controllers,
> > >
> > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1";
> > >
> > > the <4> needs to become a <6>.
> >
> > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1],
> > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT?
> >
> > Unfortunately the exact semantics of clock-critical were never documented.
> > Lee?
>
> /**
>  * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
>  * @np: Device node pointer associated with clock provider
>  * @index: clock index
>  * @flags: pointer to top-level framework flags
>  *
>  * Detects if the clock-critical property exists and, if so, sets the
>  * corresponding CLK_IS_CRITICAL flag.
>  *
>  * Do not use this function. It exists only for legacy Device Tree
>  * bindings, such as the one-clock-per-node style that are outdated.
>  * Those bindings typically put all clock data into .dts and the Linux
>  * driver has no clock data, thus making it impossible to set this flag
>  * correctly from the driver. Only those drivers may call
>  * of_clk_detect_critical from their setup functions.
>  *
>  * Return: error code or zero on success
>  */
>
> If this meets the criteria, the API is pretty simple/self
> explanatory.  What are you having trouble with?

What exactly is the "index" parameter?  This value is matched against
the values of the "clock-critical" (array) property, but it is described
nowhere in the DT bindings.
stih407-clock.dtsi seems to assume this value is an index into the
clock-output-names array?
Can we use it as a value of "clock-indices" instead?

> > Thanks!
> >
> > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device
> > tree support"
> >     https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-04  9:03           ` Geert Uytterhoeven
@ 2019-12-04  9:47             ` Lee Jones
  2019-12-04 11:00               ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Lee Jones @ 2019-12-04  9:47 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Chris Brandt, Mark Brown, Rob Herring, Mark Rutland,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

On Wed, 04 Dec 2019, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> On Wed, Dec 4, 2019 at 9:38 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 03 Dec 2019, Geert Uytterhoeven wrote:
> > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> > > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > > > > > +                       reg = <0x3fefa000 0x100>, <0x18000000
> > > > > > + 0x4000000>;
> > > > >
> > > > > The second region conflicts with the XIP flash@18000000 in
> > > > > arch/arm/boot/dts/r7s72100-gr-peach.dts.
> > > > > Yes, I know it is the same device ;-)
> > > >
> > > > Is that just an FYI?? Or do you have some suggestion??
> > >
> > > Can the flash subnode be compatible with "mtd-rom", even if the parent node
> > > is kept disabled?
> > >
> > > > > > +                       clock-critical = <4>; /* spibsc0 */
> > > > >
> > > > > Iff we go this clock-critical route, I think this should be specified in the
> > > > > board-specific .dts instead of in the SoC-specific .dtsi.
> > > >
> > > > OK, that's fine. It makes more sense to be in the .dts because it's a board
> > > > design decision. I will remove it from the patch.
> > > >
> > > > The one 'tricky' thing is that the <4> is the index into the number of clocks.
> > > >
> > > > So in the Renesas BSP where it includes the VDC5 LCD controllers,
> > > >
> > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1";
> > > >
> > > > the <4> needs to become a <6>.
> > >
> > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1],
> > > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT?
> > >
> > > Unfortunately the exact semantics of clock-critical were never documented.
> > > Lee?
> >
> > /**
> >  * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
> >  * @np: Device node pointer associated with clock provider
> >  * @index: clock index
> >  * @flags: pointer to top-level framework flags
> >  *
> >  * Detects if the clock-critical property exists and, if so, sets the
> >  * corresponding CLK_IS_CRITICAL flag.
> >  *
> >  * Do not use this function. It exists only for legacy Device Tree
> >  * bindings, such as the one-clock-per-node style that are outdated.
> >  * Those bindings typically put all clock data into .dts and the Linux
> >  * driver has no clock data, thus making it impossible to set this flag
> >  * correctly from the driver. Only those drivers may call
> >  * of_clk_detect_critical from their setup functions.
> >  *
> >  * Return: error code or zero on success
> >  */
> >
> > If this meets the criteria, the API is pretty simple/self
> > explanatory.  What are you having trouble with?
> 
> What exactly is the "index" parameter?  This value is matched against
> the values of the "clock-critical" (array) property, but it is described
> nowhere in the DT bindings.
> stih407-clock.dtsi seems to assume this value is an index into the
> clock-output-names array?
> Can we use it as a value of "clock-indices" instead?

of_clk_detect_critical(), the consumer of the device tree property
'clock-critical', is a helper.  Neither deliver any auto-magical
functionality.  Simply providing an index into the property will not
do anything useful.  It's up to the vendor's clock driver to handle.

The vendor driver can call of_clk_detect_critical() with *any* index
it sees fit.  If it matches a number listed in the 'clock-critical'
array, the CLK_IS_CRITICAL flag is set in the flags pointed to by the
3rd function parameter.

Take a look at some of the call sites in drivers/clk/st/* for further
clarification.

> > > Thanks!
> > >
> > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device
> > > tree support"
> > >     https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks
  2019-12-04  9:47             ` Lee Jones
@ 2019-12-04 11:00               ` Chris Brandt
  0 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-04 11:00 UTC (permalink / raw)
  To: Lee Jones, Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Michael Turquette,
	Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Lee,
  Thank you.

Hi Geert,

On Wed, Dec 4, 2019, Lee Jones wrote:
> The vendor driver can call of_clk_detect_critical() with *any* index it sees
> fit.  If it matches a number listed in the 'clock-critical'
> array, the CLK_IS_CRITICAL flag is set in the flags pointed to by the 3rd
> function parameter.

Since this is the case, I'll change the driver code so we can use it how we
prefer:
ie,
   clock-critical = <R7S72100_CLK_SPIBSC0>;

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support
  2019-12-04  7:57       ` Geert Uytterhoeven
@ 2019-12-04 11:04         ` Chris Brandt
  0 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-04 11:04 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

On Wed, Dec 4, 2019, Geert Uytterhoeven wrote:
> > Because....I keep forgetting what is the latest 'correct' size:
> >   A. The exact size of the register range or
> >   B. The size rounded up to look nicer
> 
> C. The size used by the on-chip address decoder providing the module's
>    select signal? I doubt that's not a power of two ;-)

Point taken :)

I'll change it.

Chris

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03 18:28   ` Geert Uytterhoeven
@ 2019-12-04 11:18     ` Mark Brown
  2019-12-04 22:12     ` Chris Brandt
  1 sibling, 0 replies; 50+ messages in thread
From: Mark Brown @ 2019-12-04 11:18 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Chris Brandt, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-04  8:43       ` Geert Uytterhoeven
@ 2019-12-04 11:19         ` Chris Brandt
  0 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-04 11:19 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

> > But "mtd-rom" doesn't really have anything to do with the
> > functionality of the driver when it is being used in "SPI mode".
> 
> Correct. But DT describes hardware.  If the FLASH is used in direct mapped
> mode, that should be described in DT.

So it seems we are going back to my original plan:
Always enable the SPIBSC node in the .dts to ensure the clocks
stay on for the XIP kernel case. But, if there is no spi-nor partitions in
the node, then simple drop out and do not register the device as a SPI
controller.
However, you can have mtd-rom partitions in there which is what XIP Linux will
use, but we still don't need to register a spi controller for that.

Do you agree?????


> > > interrupts? RZ/A2M seems to have an SPIBSC interrupt, RZ/A1 hasn't.
> >
> > There was never any interrupts in the SPIBSC.
> > But it looks like when they added HyperFlash and OctaFlash support,
> > they put in some interrupts for that.
> > And now that I look at it, they are for pins labeled RPC_INT, RPC_WC,
> RPC_RESET.
> > (I just realized that "RPC" stands for "Reduced Pin Count")
> >
> > So....am I supposed to add in that interrupt even though I'm not
> > planning on using it??
> 
> DT describes hardware, not driver limitations.

OK. I will add the interrupt to the DT.

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03 18:29     ` Geert Uytterhoeven
@ 2019-12-04 11:25       ` Mark Brown
  2019-12-04 12:14         ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Mark Brown @ 2019-12-04 11:25 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Chris Brandt, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-04 11:25       ` Mark Brown
@ 2019-12-04 12:14         ` Geert Uytterhoeven
  0 siblings, 0 replies; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-04 12:14 UTC (permalink / raw)
  To: Mark Brown
  Cc: Chris Brandt, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

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

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-04  8:56         ` Geert Uytterhoeven
@ 2019-12-04 13:31           ` Chris Brandt
  2019-12-05 15:48             ` Geert Uytterhoeven
  0 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-04 13:31 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

> To avoid future problems, you probably do want to specify spi-tx-bus-width =
> <4> and spi-rx-bus-width = <4> in DTS now.

I didn't do that because if the MTD layer then thinks I 'want' to do 4-bit access, then that introduces a new problem the solve.
The MTD layer might start sending down QUAD READ commands to the external SPI and then the SPI Flash will start sending back data on all 4 lines, but the controller is only configured for 1-bit transfers.

I honestly don't know when/why the MTD layer decides on switch from 1-bit to 4-bit mode, so while the board hardware is wired for 4-bit (as the DT would document), we are not ready to be doing 4-bit just yet.
I just want to try and get the driver in at first....then we can make it do fancy stuff later.

If someone can tell me that even if "spi-rx-bus-width = <4>" is put I the board DTS, the spi will still only do 1-bit transfers until the application specially enables 4-bit mode, then I'm fine with add bus-width=<4> in the DTS.

Unless....I did not understand you meaning....

Did you mean put 'spi-rx-bus-width = <4>' in the .dtsi????  (then I can override it back to <1>  that in the board .dts)???



> > Basically, it's like the 'role switch' in the USB OTG drivers.
> 
> If you want to do that, both configurations should be described in DT, and we
> need a way to specify what's being used.
> I guess if the direct mapped mode is used, you always want to boot the kernel
> using that mode, and only switch to SPI mode temporarily after boot?  So that
> could be handled by manually unbinding the driver from physmap-flash, and
> forcing a bind to spibsc, all from sysfs.

Yes, I agree. That is what I would suggest to someone. (I suggest unbind/bind for a lot of situations that people ask me what to do).

> (Which cuts the branch the kernel is sitting on in the case of XIP...)
XIP is a special case all in it's own.....which is why we uses a completely special driver for R/W in XIP mode...which is out of scope from mainline.
The case I'm talking above would probably before an R-Car or RZ/G use case. But, since no one has stated a use case like that, it's very low on the priority list.


> > So my suggestion is to forget about trying to 'support' direct mode in
> > this driver at the moment. If you're using this HW for something like
> > XIP, then don't enable this driver at all (which is what we have been
> > doing).
> 
> Still, the direct-mapped mode should be described in DT, when used.
> arch/arm/boot/dts/r7s72100-gr-peach.dts does describe the FLASH, but fails to
> describe the exact topology (flash is a child of spibsc, and thus relies on
> the spibsc module clock being turned on).

I can agree with that....and we go back to my first idea: If you are 'using' the SPIBSC for anything (XIP or SPI mode) then you describe that in DT. And when the driver probes, if it does not see a 'spi-nor' partition, it does not register a spi-controller, but it does keep the clock (power) on the entire time (until removed).

For GR-PEACH, we would just have to go back and put the qspi@18000000 (which is currently at the root) node under a spibsc node.

Of course also if we do all this, we could drop all the patches for enabling 'critical clocks' that were needed to cover the XIP cases.


> BTW, when using spibsc in direct-mapped mode: if you turn of and on again the
> module clock, does the spibsc need reprogramming?

Nope. Everything will stay the same (just like all the other peripherals). The only thing you 'might' want to do is flush the read cache (especially if you disconnected it because you were going to go out and re-write some of the flash in SPI mode).

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03 14:19   ` Mark Brown
  2019-12-03 15:00     ` Chris Brandt
  2019-12-03 18:29     ` Geert Uytterhoeven
@ 2019-12-04 15:51     ` Chris Brandt
  2019-12-04 16:49       ` Mark Brown
  2 siblings, 1 reply; 50+ messages in thread
From: Chris Brandt @ 2019-12-04 15:51 UTC (permalink / raw)
  To: Mark Brown
  Cc: Rob Herring, Mark Rutland, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, linux-spi, devicetree, linux-renesas-soc,
	linux-clk, Mason Yang, Sergei Shtylyov

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-04 15:51     ` Chris Brandt
@ 2019-12-04 16:49       ` Mark Brown
  0 siblings, 0 replies; 50+ messages in thread
From: Mark Brown @ 2019-12-04 16:49 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Rob Herring, Mark Rutland, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, linux-spi, devicetree, linux-renesas-soc,
	linux-clk, Mason Yang, Sergei Shtylyov

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 4/6] spi: Add SPIBSC driver
  2019-12-03 18:28   ` Geert Uytterhoeven
  2019-12-04 11:18     ` Mark Brown
@ 2019-12-04 22:12     ` Chris Brandt
  1 sibling, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-04 22:12 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-04 13:31           ` Chris Brandt
@ 2019-12-05 15:48             ` Geert Uytterhoeven
  2019-12-05 16:00               ` Chris Brandt
  0 siblings, 1 reply; 50+ messages in thread
From: Geert Uytterhoeven @ 2019-12-05 15:48 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Chris,

On Wed, Dec 4, 2019 at 2:31 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> > To avoid future problems, you probably do want to specify spi-tx-bus-width =
> > <4> and spi-rx-bus-width = <4> in DTS now.
>
> I didn't do that because if the MTD layer then thinks I 'want' to do 4-bit access, then that introduces a new problem the solve.
> The MTD layer might start sending down QUAD READ commands to the external SPI and then the SPI Flash will start sending back data on all 4 lines, but the controller is only configured for 1-bit transfers.
>
> I honestly don't know when/why the MTD layer decides on switch from 1-bit to 4-bit mode, so while the board hardware is wired for 4-bit (as the DT would document), we are not ready to be doing 4-bit just yet.
> I just want to try and get the driver in at first....then we can make it do fancy stuff later.
>
> If someone can tell me that even if "spi-rx-bus-width = <4>" is put I the board DTS, the spi will still only do 1-bit transfers until the application specially enables 4-bit mode, then I'm fine with add bus-width=<4> in the DTS.

Your spibsc driver does:

    master->mode_bits = SPI_CPOL | SPI_CPHA;

i.e. SPI_[TR]X_{QUAD,DUAL} are not set, so it should not try those modes.

At least on RSK+RZA1, the FLASHes are wired in quad mode, so you
should describe the hardware in DT.

>
> Unless....I did not understand you meaning....
>
> Did you mean put 'spi-rx-bus-width = <4>' in the .dtsi????  (then I can override it back to <1>  that in the board .dts)???

No, in the board .dtb.

> > BTW, when using spibsc in direct-mapped mode: if you turn of and on again the
> > module clock, does the spibsc need reprogramming?
>
> Nope. Everything will stay the same (just like all the other peripherals). The only thing you 'might' want to do is flush the read cache (especially if you disconnected it because you were going to go out and re-write some of the flash in SPI mode).

Good. So that means the MTD driver can be modular.  Unused clocks are
turned off at boot, and can be turned on when the mtd-rom driver is loaded
and activated.

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* RE: [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings
  2019-12-05 15:48             ` Geert Uytterhoeven
@ 2019-12-05 16:00               ` Chris Brandt
  0 siblings, 0 replies; 50+ messages in thread
From: Chris Brandt @ 2019-12-05 16:00 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Rob Herring, Mark Rutland, Geert Uytterhoeven,
	Michael Turquette, Stephen Boyd, linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, linux-clk, Mason Yang, Sergei Shtylyov

Hi Geert,

On Thu, Dec 5, 2019 1, Geert Uytterhoeven wrote:
> Your spibsc driver does:
> 
>     master->mode_bits = SPI_CPOL | SPI_CPHA;
> 
> i.e. SPI_[TR]X_{QUAD,DUAL} are not set, so it should not try those modes.

OK, Thank you!
 
> At least on RSK+RZA1, the FLASHes are wired in quad mode, so you should
> describe the hardware in DT.

OK.

> > > BTW, when using spibsc in direct-mapped mode: if you turn of and on
> > > again the module clock, does the spibsc need reprogramming?
> >
> > Nope. Everything will stay the same (just like all the other peripherals).
> The only thing you 'might' want to do is flush the read cache (especially if
> you disconnected it because you were going to go out and re-write some of the
> flash in SPI mode).
> 
> Good. So that means the MTD driver can be modular.  Unused clocks are turned
> off at boot, and can be turned on when the mtd-rom driver is loaded and
> activated.

I'm going to do some testing now and then send out a V2 to see if we are
getting closer to a consensus.

Chris


^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2019-12-05 16:01 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03  3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
2019-12-03  3:45 ` [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support Chris Brandt
2019-12-03 18:32   ` Geert Uytterhoeven
2019-12-03 18:46     ` Chris Brandt
2019-12-03 18:51       ` Geert Uytterhoeven
2019-12-03  3:45 ` [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks Chris Brandt
2019-12-03 18:42   ` Geert Uytterhoeven
2019-12-03 18:57     ` Chris Brandt
2019-12-03 19:12       ` Geert Uytterhoeven
2019-12-04  8:38         ` Lee Jones
2019-12-04  9:03           ` Geert Uytterhoeven
2019-12-04  9:47             ` Lee Jones
2019-12-04 11:00               ` Chris Brandt
2019-12-03  3:45 ` [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock Chris Brandt
2019-12-03 18:49   ` Geert Uytterhoeven
2019-12-03 19:09     ` Chris Brandt
2019-12-03 20:40       ` Geert Uytterhoeven
2019-12-04  3:09         ` Chris Brandt
2019-12-03  3:45 ` [PATCH 4/6] spi: Add SPIBSC driver Chris Brandt
2019-12-03 14:19   ` Mark Brown
2019-12-03 15:00     ` Chris Brandt
2019-12-03 18:29     ` Geert Uytterhoeven
2019-12-04 11:25       ` Mark Brown
2019-12-04 12:14         ` Geert Uytterhoeven
2019-12-04 15:51     ` Chris Brandt
2019-12-04 16:49       ` Mark Brown
2019-12-03 18:28   ` Geert Uytterhoeven
2019-12-04 11:18     ` Mark Brown
2019-12-04 22:12     ` Chris Brandt
2019-12-03  3:45 ` [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support Chris Brandt
2019-12-03 18:59   ` Geert Uytterhoeven
2019-12-03 22:38     ` Chris Brandt
2019-12-04  7:57       ` Geert Uytterhoeven
2019-12-04 11:04         ` Chris Brandt
2019-12-03  3:45 ` [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings Chris Brandt
2019-12-03  9:14   ` Sergei Shtylyov
2019-12-03 13:27     ` Chris Brandt
2019-12-03 16:04       ` Sergei Shtylyov
2019-12-03 16:35         ` Chris Brandt
2019-12-03 18:57   ` Geert Uytterhoeven
2019-12-03 20:39     ` Geert Uytterhoeven
2019-12-04  2:54       ` Chris Brandt
2019-12-04  8:56         ` Geert Uytterhoeven
2019-12-04 13:31           ` Chris Brandt
2019-12-05 15:48             ` Geert Uytterhoeven
2019-12-05 16:00               ` Chris Brandt
2019-12-04  8:40       ` Geert Uytterhoeven
2019-12-03 22:33     ` Chris Brandt
2019-12-04  8:43       ` Geert Uytterhoeven
2019-12-04 11:19         ` Chris Brandt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).