[PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

[PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

[Sean Anderson]

This series adds support for SPI on the Kendryte K210. This covers the MMC
slot (currently broken) and SPI flash on the Sipeed Maix Bit.

This series makes significant changes to the designware SPI driver. I would
really appreciate if the maintainers I CC'd could test this series and
ensure that SPI still works on all their devices. I have tried my best not
to affect existing devices, but I'd rather find out if this breaks stuff
now rather than later.

In particular:

Ley Foon Tan, can you try using dw_spi_dw32_init with the Stratix 10 and
Agilex SoCFPGAs? From their documentation, it is unclear whether one should
use the DFS or DFS_32 field. I have used dw_spi_dw16_init for these
devices, since that keeps the same behavior as before this series, but I am
interested in seeing whether the DFS_32 works.

Gregory Clement, Lars Povlsen, and Horatiu Vultur, can you confirm the
register layout of ctrlr0 and version of the device on the MSCC Ocelot and
Jaguar 2? I couldn't find any documentation for the device on those SoCs
online.

Alexey Brodkin and Eugeniy Paltsev, can you confirm the register layout of
ctrlr0 and version of the device on ARC SoCs? I couldn't find any
documentation for the device on those SoCs online. In addition, can you
clarify the nature of SSI_MAX_XFER_SIZE? Is it set once before the device
is fabricated, or is it set at runtime? Is there any way to detect it at
runtime? Is my supposition that it was introduced in version 3.23 correct?
Have there been any other breaking changes which I have overlooked? I tried
investigating some of this, but I was unable to find any definitive ruling
on the matter. All I could find was this forum post which doesn't really
explain anything [1].

[1] https://community.intel.com/t5/Intel-Makers/D2000-SPI-word-frame-size/td-p/276520

This series was previously part of
https://patchwork.ozlabs.org/project/uboot/list/?series=161576

This series depends on
https://patchwork.ozlabs.org/project/uboot/list/?series=185489

Known bugs:
- The MMC cannot be accessed with the dw_spi driver

Changes in v2:
- Add Gigadevice SPI chips to dependencies
- Add external gpio cs support
- Clean up exec_op
- Configure ctrlr0 register layout based on compatible string
- Convert debug calls to log_ instead of removing the ones which affect
  timing
- Document new compatible strings
- Limit data transfers to 64k
- Remove broken-wp property (implicit due to no wp gpio)
- Remove ctrl0 field offsets from device tree
- Switch to new compatible strings
- Switch to new pinmux binding style

Sean Anderson (10):
  spi: dw: Convert calls to debug to log_*
  spi: dw: Rename "cs-gpio" to "cs-gpios"
  spi: dw: Use generic function to read reg address
  spi: dw: Rearrange struct dw_spi_priv
  spi: dw: Add SoC-specific compatible strings
  spi: dw: Configure ctrlr0 layout based on compatible string
  spi: dw: Document devicetree binding
  spi: dw: Add mem_ops
  riscv: Add device tree bindings for SPI
  riscv: Add support for SPI on Kendryte K210

 arch/arc/dts/axs10x_mb.dtsi                   |   5 +-
 arch/arc/dts/hsdk-common.dtsi                 |   5 +-
 arch/arm/dts/socfpga.dtsi                     |   6 +-
 arch/arm/dts/socfpga_agilex.dtsi              |   6 +-
 arch/arm/dts/socfpga_arria10.dtsi             |   6 +-
 arch/arm/dts/socfpga_stratix10.dtsi           |   6 +-
 arch/mips/dts/mscc,jr2.dtsi                   |   2 +-
 arch/mips/dts/mscc,ocelot.dtsi                |   2 +-
 arch/riscv/dts/k210-maix-bit.dts              |  46 ++-
 arch/riscv/dts/k210.dtsi                      |  15 +-
 board/sipeed/maix/Kconfig                     |  11 +
 configs/sipeed_maix_bitm_defconfig            |   8 +
 doc/board/sipeed/maix.rst                     |  94 +++--
 .../spi/snps,dw-apb-ssi.txt                   |  56 +++
 drivers/spi/designware_spi.c                  | 381 ++++++++++++++----
 15 files changed, 528 insertions(+), 121 deletions(-)
 create mode 100644 doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt

-- 
2.28.0

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Sean Anderson]

This allows different log levels to be enabled or disabled depending on the
desired level of verbosity. In particular, it allows for general debug
information to be printed while excluding more verbose logging which may
interfere with timing.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- New

 drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index c9b14f9029..f7ea3edaab 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -9,6 +9,7 @@
  * Copyright (c) 2009, Intel Corporation.
  */
 
+#define LOG_CATEGORY UCLASS_SPI
 #include <common.h>
 #include <log.h>
 #include <asm-generic/gpio.h>
@@ -139,7 +140,7 @@ static int request_gpio_cs(struct udevice *bus)
 		return 0;
 
 	if (ret < 0) {
-		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
+		log_err("Error: %d: Can't get %s gpio!\n", ret, bus->name);
 		return ret;
 	}
 
@@ -148,7 +149,7 @@ static int request_gpio_cs(struct udevice *bus)
 				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
 	}
 
-	debug("%s: used external gpio for CS management\n", __func__);
+	log_info("Using external gpio for CS management\n");
 #endif
 	return 0;
 }
@@ -162,8 +163,7 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
 	/* Use 500KHz as a suitable default */
 	plat->frequency = dev_read_u32_default(bus, "spi-max-frequency",
 					       500000);
-	debug("%s: regs=%p max-frequency=%d\n", __func__, plat->regs,
-	      plat->frequency);
+	log_info("regs=%p max-frequency=%d\n", plat->regs, plat->frequency);
 
 	return request_gpio_cs(bus);
 }
@@ -196,7 +196,7 @@ static void spi_hw_init(struct dw_spi_priv *priv)
 		priv->fifo_len = (fifo == 1) ? 0 : fifo;
 		dw_write(priv, DW_SPI_TXFLTR, 0);
 	}
-	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
+	log_debug("fifo_len=%d\n", priv->fifo_len);
 }
 
 /*
@@ -221,8 +221,7 @@ __weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
 	if (!*rate)
 		goto err_rate;
 
-	debug("%s: get spi controller clk via device tree: %lu Hz\n",
-	      __func__, *rate);
+	log_debug("Got spi controller clk via device tree: %lu Hz\n", *rate);
 
 	return 0;
 
@@ -247,14 +246,14 @@ static int dw_spi_reset(struct udevice *bus)
 		if (ret == -ENOENT || ret == -ENOTSUPP)
 			return 0;
 
-		dev_warn(bus, "Can't get reset: %d\n", ret);
+		log_warning("Can't get reset: %d\n", ret);
 		return ret;
 	}
 
 	ret = reset_deassert_bulk(&priv->resets);
 	if (ret) {
 		reset_release_bulk(&priv->resets);
-		dev_err(bus, "Failed to reset: %d\n", ret);
+		log_err("Failed to reset: %d\n", ret);
 		return ret;
 	}
 
@@ -333,7 +332,7 @@ static void dw_writer(struct dw_spi_priv *priv)
 				txw = *(u16 *)(priv->tx);
 		}
 		dw_write(priv, DW_SPI_DR, txw);
-		debug("%s: tx=0x%02x\n", __func__, txw);
+		log_content("tx=0x%02x\n", txw);
 		priv->tx += priv->bits_per_word >> 3;
 	}
 }
@@ -345,7 +344,7 @@ static void dw_reader(struct dw_spi_priv *priv)
 
 	while (max--) {
 		rxw = dw_read(priv, DW_SPI_DR);
-		debug("%s: rx=0x%02x\n", __func__, rxw);
+		log_content("rx=0x%02x\n", rxw);
 
 		/* Care about rx if the transfer's original "rx" is not null */
 		if (priv->rx_end - priv->len) {
@@ -400,7 +399,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 
 	/* spi core configured to do 8 bit transfers */
 	if (bitlen % 8) {
-		debug("Non byte aligned SPI transfer.\n");
+		log_err("Non byte aligned SPI transfer.\n");
 		return -1;
 	}
 
@@ -427,7 +426,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	cr0 |= (priv->tmode << SPI_TMOD_OFFSET);
 
 	priv->len = bitlen >> 3;
-	debug("%s: rx=%p tx=%p len=%d [bytes]\n", __func__, rx, tx, priv->len);
+	log_debug("rx=%p tx=%p len=%d [bytes]\n", rx, tx, priv->len);
 
 	priv->tx = (void *)tx;
 	priv->tx_end = priv->tx + priv->len;
@@ -437,7 +436,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	/* Disable controller before writing control registers */
 	spi_enable_chip(priv, 0);
 
-	debug("%s: cr0=%08x\n", __func__, cr0);
+	log_debug("cr0=%08x\n", cr0);
 	/* Reprogram cr0 only if changed */
 	if (dw_read(priv, DW_SPI_CTRL0) != cr0)
 		dw_write(priv, DW_SPI_CTRL0, cr0);
@@ -497,8 +496,8 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed)
 	spi_enable_chip(priv, 1);
 
 	priv->freq = speed;
-	debug("%s: regs=%p speed=%d clk_div=%d\n", __func__, priv->regs,
-	      priv->freq, clk_div);
+	log_debug("regs=%p speed=%d clk_div=%d\n", priv->regs, priv->freq,
+		  clk_div);
 
 	return 0;
 }
@@ -513,7 +512,7 @@ static int dw_spi_set_mode(struct udevice *bus, uint mode)
 	 * real transfer function.
 	 */
 	priv->mode = mode;
-	debug("%s: regs=%p, mode=%d\n", __func__, priv->regs, priv->mode);
+	log_debug("regs=%p, mode=%d\n", priv->regs, priv->mode);
 
 	return 0;
 }
-- 
2.28.0

[PATCH v2 02/10] spi: dw: Rename "cs-gpio" to "cs-gpios"

[Sean Anderson]

This property is named differently than other SPI drivers with the same
property, as well as the property as used in Linux.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
AFAIK these device trees are not synced with Linux. However, if they are,
they have not been synced since this property was renamed in Linux.

This patch was previously part of
https://patchwork.ozlabs.org/project/uboot/list/?series=161576

(no changes since v1)

 arch/arc/dts/axs10x_mb.dtsi   | 3 ++-
 arch/arc/dts/hsdk-common.dtsi | 3 ++-
 drivers/spi/designware_spi.c  | 8 ++------
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/arc/dts/axs10x_mb.dtsi b/arch/arc/dts/axs10x_mb.dtsi
index 33b0593438..daf7ca68fb 100644
--- a/arch/arc/dts/axs10x_mb.dtsi
+++ b/arch/arc/dts/axs10x_mb.dtsi
@@ -97,7 +97,8 @@
 			spi-max-frequency = <4000000>;
 			clocks = <&apbclk>;
 			clock-names = "spi_clk";
-			cs-gpio = <&cs_gpio 0>;
+			num-cs = <1>;
+			cs-gpios = <&cs_gpio 0>;
 			spi_flash at 0 {
 				compatible = "jedec,spi-nor";
 				reg = <0>;
diff --git a/arch/arc/dts/hsdk-common.dtsi b/arch/arc/dts/hsdk-common.dtsi
index 9aa10e4b25..a4b348b948 100644
--- a/arch/arc/dts/hsdk-common.dtsi
+++ b/arch/arc/dts/hsdk-common.dtsi
@@ -135,7 +135,8 @@
 		spi-max-frequency = <4000000>;
 		clocks = <&cgu_clk CLK_SYS_SPI_REF>;
 		clock-names = "spi_clk";
-		cs-gpio = <&cs_gpio 0>;
+		num-cs = <1>;
+		cs-gpios = <&cs_gpio 0>;
 		spi_flash at 0 {
 			compatible = "jedec,spi-nor";
 			reg = <0>;
diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index f7ea3edaab..ab68a66e1e 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -135,7 +135,8 @@ static int request_gpio_cs(struct udevice *bus)
 	int ret;
 
 	/* External chip select gpio line is optional */
-	ret = gpio_request_by_name(bus, "cs-gpio", 0, &priv->cs_gpio, 0);
+	ret = gpio_request_by_name(bus, "cs-gpios", 0, &priv->cs_gpio,
+				   GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
 	if (ret == -ENOENT)
 		return 0;
 
@@ -144,11 +145,6 @@ static int request_gpio_cs(struct udevice *bus)
 		return ret;
 	}
 
-	if (dm_gpio_is_valid(&priv->cs_gpio)) {
-		dm_gpio_set_dir_flags(&priv->cs_gpio,
-				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
-	}
-
 	log_info("Using external gpio for CS management\n");
 #endif
 	return 0;
-- 
2.28.0

[PATCH v2 03/10] spi: dw: Use generic function to read reg address

[Sean Anderson]

Using an fdt-specific function causes problems when compiled with a live
tree.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
This patch was previously part of
https://patchwork.ozlabs.org/project/uboot/list/?series=161576

(no changes since v1)

 drivers/spi/designware_spi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index ab68a66e1e..e0bb44259d 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -154,7 +154,9 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
 {
 	struct dw_spi_platdata *plat = bus->platdata;
 
-	plat->regs = (struct dw_spi *)devfdt_get_addr(bus);
+	plat->regs = dev_read_addr_ptr(bus);
+	if (!plat->regs)
+		return -EINVAL;
 
 	/* Use 500KHz as a suitable default */
 	plat->frequency = dev_read_u32_default(bus, "spi-max-frequency",
-- 
2.28.0

[PATCH v2 04/10] spi: dw: Rearrange struct dw_spi_priv

[Sean Anderson]

This should reduce the size of the struct, and also groups more similar
fields together.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
-New

 drivers/spi/designware_spi.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index e0bb44259d..f92e17feac 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -95,27 +95,26 @@ struct dw_spi_platdata {
 };
 
 struct dw_spi_priv {
-	void __iomem *regs;
-	unsigned int freq;		/* Default frequency */
-	unsigned int mode;
 	struct clk clk;
-	unsigned long bus_clk_rate;
-
+	struct reset_ctl_bulk resets;
 	struct gpio_desc cs_gpio;	/* External chip-select gpio */
 
-	int bits_per_word;
-	u8 cs;			/* chip select pin */
-	u8 tmode;		/* TR/TO/RO/EEPROM */
-	u8 type;		/* SPI/SSP/MicroWire */
-	int len;
+	void __iomem *regs;
+	unsigned long bus_clk_rate;
+	unsigned int freq;		/* Default frequency */
+	unsigned int mode;
 
-	u32 fifo_len;		/* depth of the FIFO buffer */
-	void *tx;
-	void *tx_end;
+	const void *tx;
+	const void *tx_end;
 	void *rx;
 	void *rx_end;
+	u32 fifo_len;			/* depth of the FIFO buffer */
 
-	struct reset_ctl_bulk	resets;
+	int bits_per_word;
+	int len;
+	u8 cs;				/* chip select pin */
+	u8 tmode;			/* TR/TO/RO/EEPROM */
+	u8 type;			/* SPI/SSP/MicroWire */
 };
 
 static inline u32 dw_read(struct dw_spi_priv *priv, u32 offset)
-- 
2.28.0

[PATCH v2 05/10] spi: dw: Add SoC-specific compatible strings

[Sean Anderson]

This adds SoC-specific compatible strings to all users of the designware
spi device. This will allow for the correct driver to be selected for each
device. Where it is publicly documented, a compatible string for the
specific device version has also been added. Devices without
publicly-documented device versions include MSCC SoCs, and Arc Socs. All
compatible strings except those for SoCFPGAs and some of the versioned
strings have been taken from Linux.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- New

 arch/arc/dts/axs10x_mb.dtsi         |  2 +-
 arch/arc/dts/hsdk-common.dtsi       |  2 +-
 arch/arm/dts/socfpga.dtsi           |  6 ++++--
 arch/arm/dts/socfpga_agilex.dtsi    |  6 ++++--
 arch/arm/dts/socfpga_arria10.dtsi   |  6 ++++--
 arch/arm/dts/socfpga_stratix10.dtsi |  6 ++++--
 arch/mips/dts/mscc,jr2.dtsi         |  2 +-
 arch/mips/dts/mscc,ocelot.dtsi      |  2 +-
 arch/riscv/dts/k210.dtsi            | 13 ++++++++-----
 9 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/arch/arc/dts/axs10x_mb.dtsi b/arch/arc/dts/axs10x_mb.dtsi
index daf7ca68fb..d4ff4f7039 100644
--- a/arch/arc/dts/axs10x_mb.dtsi
+++ b/arch/arc/dts/axs10x_mb.dtsi
@@ -90,7 +90,7 @@
 		};
 
 		spi0: spi at 0 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "snps,axs10x-spi", "snps,dw-apb-ssi";
 			reg = <0x0 0x100>;
 			#address-cells = <1>;
 			#size-cells = <0>;
diff --git a/arch/arc/dts/hsdk-common.dtsi b/arch/arc/dts/hsdk-common.dtsi
index a4b348b948..3fc82e57d7 100644
--- a/arch/arc/dts/hsdk-common.dtsi
+++ b/arch/arc/dts/hsdk-common.dtsi
@@ -128,7 +128,7 @@
 	};
 
 	spi0: spi at f0020000 {
-		compatible = "snps,dw-apb-ssi";
+		compatible = "snps,hsdk-spi", "snps,dw-apb-ssi";
 		reg = <0xf0020000 0x1000>;
 		#address-cells = <1>;
 		#size-cells = <0>;
diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index eda558f2fe..ff79d335ac 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -804,7 +804,8 @@
 		};
 
 		spi0: spi at fff00000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "altr,socfpga-spi", "snps,dw-apb-ssi-3.20",
+				     "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xfff00000 0x1000>;
@@ -816,7 +817,8 @@
 		};
 
 		spi1: spi at fff01000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "altr,socfpga-spi", "snps,dw-apb-ssi-3.20",
+				     "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xfff01000 0x1000>;
diff --git a/arch/arm/dts/socfpga_agilex.dtsi b/arch/arm/dts/socfpga_agilex.dtsi
index 179b4d5591..c3ead2d72b 100644
--- a/arch/arm/dts/socfpga_agilex.dtsi
+++ b/arch/arm/dts/socfpga_agilex.dtsi
@@ -366,7 +366,8 @@
 		};
 
 		spi0: spi at ffda4000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "intel,agilex-spi",
+				     "snps,dw-apb-ssi-4.00a", "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xffda4000 0x1000>;
@@ -379,7 +380,8 @@
 		};
 
 		spi1: spi at ffda5000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "intel,agilex-spi",
+				     "snps,dw-apb-ssi-4.00a", "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xffda5000 0x1000>;
diff --git a/arch/arm/dts/socfpga_arria10.dtsi b/arch/arm/dts/socfpga_arria10.dtsi
index a598c75542..bab34ab56c 100644
--- a/arch/arm/dts/socfpga_arria10.dtsi
+++ b/arch/arm/dts/socfpga_arria10.dtsi
@@ -604,7 +604,8 @@
 		};
 
 		spi0: spi at ffda4000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "altr,socfpga-arria10-spi",
+				     "snps,dw-apb-ssi-3.22a", "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xffda4000 0x100>;
@@ -617,7 +618,8 @@
 		};
 
 		spi1: spi at ffda5000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "altr,socfpga-arria10-spi",
+				     "snps,dw-apb-ssi-3.22a", "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xffda5000 0x100>;
diff --git a/arch/arm/dts/socfpga_stratix10.dtsi b/arch/arm/dts/socfpga_stratix10.dtsi
index a8e61cf728..48c7046f62 100755
--- a/arch/arm/dts/socfpga_stratix10.dtsi
+++ b/arch/arm/dts/socfpga_stratix10.dtsi
@@ -268,7 +268,8 @@
 		 };
 
 		spi0: spi at ffda4000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "intel,stratix10-spi",
+				     "snps,dw-apb-ssi-4.00a", "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xffda4000 0x1000>;
@@ -281,7 +282,8 @@
 		};
 
 		spi1: spi at ffda5000 {
-			compatible = "snps,dw-apb-ssi";
+			compatible = "intel,stratix10-spi",
+				     "snps,dw-apb-ssi-4.00a", "snps,dw-apb-ssi";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xffda5000 0x1000>;
diff --git a/arch/mips/dts/mscc,jr2.dtsi b/arch/mips/dts/mscc,jr2.dtsi
index 7f5a96fecd..c44e9a2b3a 100644
--- a/arch/mips/dts/mscc,jr2.dtsi
+++ b/arch/mips/dts/mscc,jr2.dtsi
@@ -94,7 +94,7 @@
 		spi0: spi-master at 101000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-			compatible = "snps,dw-apb-ssi";
+			compatible = "mscc,jaguar2-spi", "snps,dw-apb-ssi";
 			reg = <0x101000 0x40>;
 			num-chipselect = <4>;
 			bus-num = <0>;
diff --git a/arch/mips/dts/mscc,ocelot.dtsi b/arch/mips/dts/mscc,ocelot.dtsi
index 9a187b6e58..aeb4bf8f4b 100644
--- a/arch/mips/dts/mscc,ocelot.dtsi
+++ b/arch/mips/dts/mscc,ocelot.dtsi
@@ -100,7 +100,7 @@
 		spi0: spi-master at 101000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-			compatible = "snps,dw-apb-ssi";
+			compatible = "mscc,ocelot-spi", "snps,dw-apb-ssi";
 			reg = <0x101000 0x40>;
 			num-chipselect = <4>;
 			bus-num = <0>;
diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
index fc7986b326..429891d651 100644
--- a/arch/riscv/dts/k210.dtsi
+++ b/arch/riscv/dts/k210.dtsi
@@ -283,7 +283,8 @@
 			};
 
 			spi2: spi at 50240000 {
-				compatible = "kendryte,k120-spislave",
+				compatible = "canaan,kendryte-k210-spi",
+					     "snps,dw-apb-ssi-4.01",
 					     "snps,dw-apb-ssi";
 				spi-slave;
 				reg = <0x50240000 0x100>;
@@ -556,7 +557,8 @@
 			spi0: spi at 52000000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				compatible = "kendryte,k210-spi",
+				compatible = "canaan,kendryte-k210-spi",
+					     "snps,dw-apb-ssi-4.01",
 					     "snps,dw-apb-ssi";
 				reg = <0x52000000 0x100>;
 				interrupts = <1>;
@@ -572,7 +574,8 @@
 			spi1: spi at 53000000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				compatible = "kendryte,k210-spi",
+				compatible = "canaan,kendryte-k210-spi",
+					     "snps,dw-apb-ssi-4.01",
 					     "snps,dw-apb-ssi";
 				reg = <0x53000000 0x100>;
 				interrupts = <2>;
@@ -588,8 +591,8 @@
 			spi3: spi at 54000000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				compatible = "kendryte,k210-spi",
-					     "snps,dw-apb-ssi";
+				compatible = "canaan,kendryte-k210-ssi",
+					     "snps,dwc-ssi-1.01a";
 				reg = <0x54000000 0x200>;
 				interrupts = <4>;
 				clocks = <&sysclk K210_CLK_SPI3>;
-- 
2.28.0

[PATCH v2 06/10] spi: dw: Configure ctrlr0 layout based on compatible string

[Sean Anderson]

Ctrlr0 can have several different layouts depending on the specific device
(dw-apb-ssi vs dwc-ssi), and specific version. Update the driver to support
three specific configurations: dw-apb-ssi 16-bit (default), dw-apb-ssi
32-bit, and dwc-ssi.

dw-apb-ssi 16-bit (versions up to 3.22) is the version of the device on
Altera Cyclone V, Arria V, and Arria 10 SoCFPGAs; and MSCC Ocelot and
Jaguar2 SoCs. This is the version this driver supported before this change.
The layout of ctrlr0 is:

|   31 .. 16  |
| other stuff |

|   15 .. 10  | 9 .. 8 | 7 .. 6 | 5 .. 4 | 3 .. 0 |
| other stuff |  tmod  |  mode  |  frf   |  dfs   |

dw-apb-ssi 32-bit (versions 3.23 and up) is the version of the device on
Intel Quark D20000 SoCs; Intel Stratix 10 and Agilex SoCFPGAs; Cobham
UT32M0R500 SoCs; and Canaaan Kendryte K210 SoCs. The layout of ctrlr0 is:

|   31 .. 23  | 22 .. 21 | 20 .. 16 |
| other stuff | spi_frf  |  dfs_32  |

|   15 .. 10  | 9 .. 8 | 7 .. 6 | 5 .. 4 | 3 .. 0 |
| other stuff |  tmod  |  mode  |  frf   |  dfs   |

The contents of the lower 16 bits have not changed, but the semantics have.
dfs "...is only valid when SSI_MAX_XFER_SIZE is configured to 16." If
SSI_MAX_XFER_SIZE is 32, then dfs_32 must be used instead. I have been
unable to determine if SSI_MAX_XFER_SIZE is a runtime constant, or if it is
possible to detect its value at runtime. Devices with an SSI_MAX_XFER_SIZE
of 16 can use the older ctrl0 layout. spi_frf is used to configure dual and
quad spi, which this driver does not yet support.

dwc-ssi is the version of the device on Intel Keem Bay SoCs; and Canaan
Kendryte K210 SoCs. The layout of ctrlr0 is:

|   31 .. 24  | 23 .. 22 |   21 .. 16  |
| other stuff | spi_frf  | other stuff |

|   15 .. 12  | 11 .. 10 | 9 .. 8 | 7 .. 6 | 4 .. 0 |
| other stuff |   tmod   |  mode  |  frf   | dfs_32 |

The semantics of the fields have not changed since the previous version.
However, SSI_MAX_XFER_SIZE is effectively always 32.

To support these three different layouts, we model our approach on the one
which the Linux kernel has taken. Each SoC which uses this device has had
its own compatible string added. In addition, a few generic compatible
strings have been added. During probe, the driver calls an init function
stored in driver_data. This init function is responsible for supplying the
update_cr0 function, as well as doing any SoC-specific initialization. At
the moment, there is no SoC-specific work to be done. In addition, there is
a generic init function which tries to detect the device type based on its
version. This is used when the non-specific "snps,dw-apb-ssi" compatible
string is matched.

Although the Intel Quark D20000 is not supported by either U-Boot or Linux,
I have added a compatible string for its version, since it was the earliest
documented version I could find which included a reference to the DFS_32
field.

The style of and information behind this commit is based on the Linux MMIO
driver for these devices. Specific reference was made to the series adding
support for Intel Keem Bay SoCs [1].

[1] https://lore.kernel.org/linux-spi/20200505130618.554-1-wan.ahmad.zainie.wan.mohamad at intel.com/

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Configure ctrlr0 register layout based on compatible string
- Split documentation off into its own patch

 drivers/spi/designware_spi.c | 205 ++++++++++++++++++++++++++++++-----
 1 file changed, 175 insertions(+), 30 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index f92e17feac..d9dc739d7d 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -3,6 +3,7 @@
  * Designware master SPI core controller driver
  *
  * Copyright (C) 2014 Stefan Roese <sr@denx.de>
+ * Copyright (C) 2020 Sean Anderson <seanga2@gmail.com>
  *
  * Very loosely based on the Linux driver:
  * drivers/spi/spi-dw.c, which is:
@@ -22,6 +23,7 @@
 #include <reset.h>
 #include <dm/device_compat.h>
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/compat.h>
 #include <linux/iopoll.h>
 #include <asm/io.h>
@@ -54,28 +56,43 @@
 #define DW_SPI_DR			0x60
 
 /* Bit fields in CTRLR0 */
-#define SPI_DFS_OFFSET			0
+#define CTRLR0_DFS_MASK			GENMASK(3, 0)
 
-#define SPI_FRF_OFFSET			4
-#define SPI_FRF_SPI			0x0
-#define SPI_FRF_SSP			0x1
-#define SPI_FRF_MICROWIRE		0x2
-#define SPI_FRF_RESV			0x3
+#define CTRLR0_FRF_MASK			GENMASK(5, 4)
+#define CTRLR0_FRF_SPI			0x0
+#define CTRLR0_FRF_SSP			0x1
+#define CTRLR0_FRF_MICROWIRE		0x2
+#define CTRLR0_FRF_RESV			0x3
 
-#define SPI_MODE_OFFSET			6
-#define SPI_SCPH_OFFSET			6
-#define SPI_SCOL_OFFSET			7
+#define CTRLR0_MODE_MASK		GENMASK(7, 6)
+#define CTRLR0_MODE_SCPH		0x1
+#define CTRLR0_MODE_SCPOL		0x2
 
-#define SPI_TMOD_OFFSET			8
-#define SPI_TMOD_MASK			(0x3 << SPI_TMOD_OFFSET)
-#define	SPI_TMOD_TR			0x0		/* xmit & recv */
-#define SPI_TMOD_TO			0x1		/* xmit only */
-#define SPI_TMOD_RO			0x2		/* recv only */
-#define SPI_TMOD_EPROMREAD		0x3		/* eeprom read mode */
+#define CTRLR0_TMOD_MASK		GENMASK(9, 8)
+#define	CTRLR0_TMOD_TR			0x0		/* xmit & recv */
+#define CTRLR0_TMOD_TO			0x1		/* xmit only */
+#define CTRLR0_TMOD_RO			0x2		/* recv only */
+#define CTRLR0_TMOD_EPROMREAD		0x3		/* eeprom read mode */
 
-#define SPI_SLVOE_OFFSET		10
-#define SPI_SRL_OFFSET			11
-#define SPI_CFS_OFFSET			12
+#define CTRLR0_SLVOE_OFFSET		10
+#define CTRLR0_SRL_OFFSET		11
+#define CTRLR0_CFS_MASK			GENMASK(15, 12)
+
+/* The next two fields are only present on version 4.* */
+#define CTRLR0_DFS_32_MASK		GENMASK(20, 16)
+
+#define CTRLR0_SPI_FRF_MASK		GENMASK(22, 21)
+#define CTRLR0_SPI_FRF_BYTE		0x0
+#define	CTRLR0_SPI_FRF_DUAL		0x1
+#define	CTRLR0_SPI_FRF_QUAD		0x2
+
+/* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
+#define DWC_SSI_CTRLR0_DFS_MASK		GENMASK(4, 0)
+#define DWC_SSI_CTRLR0_FRF_MASK		GENMASK(7, 6)
+#define DWC_SSI_CTRLR0_MODE_MASK	GENMASK(9, 8)
+#define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)
+#define DWC_SSI_CTRLR0_SRL_OFFSET	13
+#define DWC_SSI_CTRLR0_SPI_FRF_MASK	GENMASK(23, 22)
 
 /* Bit fields in SR, 7 bits */
 #define SR_MASK				GENMASK(6, 0)	/* cover 7 bits */
@@ -87,6 +104,11 @@
 #define SR_TX_ERR			BIT(5)
 #define SR_DCOL				BIT(6)
 
+/* Bit fields in VERSION */
+#define VERSION_MAJOR_MASK		GENMASK(31, 24)
+#define VERSION_MINOR_HIGH_MASK		GENMASK(23, 16)
+#define VERSION_MINOR_LOW_MASK		GENMASK(15, 8)
+
 #define RX_TIMEOUT			1000		/* timeout in ms */
 
 struct dw_spi_platdata {
@@ -99,6 +121,8 @@ struct dw_spi_priv {
 	struct reset_ctl_bulk resets;
 	struct gpio_desc cs_gpio;	/* External chip-select gpio */
 
+	u32 (*update_cr0)(struct dw_spi_priv *priv);
+
 	void __iomem *regs;
 	unsigned long bus_clk_rate;
 	unsigned int freq;		/* Default frequency */
@@ -127,6 +151,84 @@ static inline void dw_write(struct dw_spi_priv *priv, u32 offset, u32 val)
 	__raw_writel(val, priv->regs + offset);
 }
 
+static u32 dw_spi_dw16_update_cr0(struct dw_spi_priv *priv)
+{
+	return FIELD_PREP(CTRLR0_DFS_MASK, priv->bits_per_word - 1)
+	     | FIELD_PREP(CTRLR0_FRF_MASK, priv->type)
+	     | FIELD_PREP(CTRLR0_MODE_MASK, priv->mode)
+	     | FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode);
+}
+
+static u32 dw_spi_dw32_update_cr0(struct dw_spi_priv *priv)
+{
+	return FIELD_PREP(CTRLR0_DFS_32_MASK, priv->bits_per_word - 1)
+	     | FIELD_PREP(CTRLR0_FRF_MASK, priv->type)
+	     | FIELD_PREP(CTRLR0_MODE_MASK, priv->mode)
+	     | FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode);
+}
+
+static u32 dw_spi_dwc_update_cr0(struct dw_spi_priv *priv)
+{
+	return FIELD_PREP(DWC_SSI_CTRLR0_DFS_MASK, priv->bits_per_word - 1)
+	     | FIELD_PREP(DWC_SSI_CTRLR0_FRF_MASK, priv->type)
+	     | FIELD_PREP(DWC_SSI_CTRLR0_MODE_MASK, priv->mode)
+	     | FIELD_PREP(DWC_SSI_CTRLR0_TMOD_MASK, priv->tmode);
+}
+
+static int dw_spi_dw16_init(struct udevice *bus, struct dw_spi_priv *priv)
+{
+	priv->update_cr0 = dw_spi_dw16_update_cr0;
+	return 0;
+}
+
+static int dw_spi_dw32_init(struct udevice *bus, struct dw_spi_priv *priv)
+{
+	priv->update_cr0 = dw_spi_dw32_update_cr0;
+	return 0;
+}
+
+static void dw_spi_print_version(u32 version, char *driver)
+{
+	char *fmt;
+
+	if (driver)
+		fmt = "Device version %c.%c%c%c matches driver version %s\n";
+	else
+		fmt = "Device version %c.%c%c%c does not match any driver%.0s\n";
+
+	log_info(fmt, version >> 24, version >> 16, version >> 8, version,
+		 driver);
+}
+
+static int dw_spi_dw_generic_init(struct udevice *bus, struct dw_spi_priv *priv)
+{
+	u32 version;
+
+	log_warning("Compatible string did not specify device version.\n");
+
+	version = dw_read(priv, DW_SPI_VERSION);
+	switch (FIELD_GET(VERSION_MAJOR_MASK, version)) {
+	case '3':
+		if (FIELD_GET(VERSION_MINOR_HIGH_MASK, version) < '2' ||
+		    FIELD_GET(VERSION_MINOR_LOW_MASK, version) < '3') {
+			dw_spi_print_version(version, "<3.23");
+			return dw_spi_dw16_init(bus, priv);
+		}
+	case '4':
+		dw_spi_print_version(version, ">=3.23");
+		return dw_spi_dw32_init(bus, priv);
+	default:
+		dw_spi_print_version(version, NULL);
+		return -ENOTSUPP;
+	}
+}
+
+static int dw_spi_dwc_init(struct udevice *bus, struct dw_spi_priv *priv)
+{
+	priv->update_cr0 = dw_spi_dwc_update_cr0;
+	return 0;
+}
+
 static int request_gpio_cs(struct udevice *bus)
 {
 #if CONFIG_IS_ENABLED(DM_GPIO) && !defined(CONFIG_SPL_BUILD)
@@ -160,6 +262,9 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
 	/* Use 500KHz as a suitable default */
 	plat->frequency = dev_read_u32_default(bus, "spi-max-frequency",
 					       500000);
+	if (dev_read_bool(bus, "spi-slave"))
+		return -EINVAL;
+
 	log_info("regs=%p max-frequency=%d\n", plat->regs, plat->frequency);
 
 	return request_gpio_cs(bus);
@@ -257,8 +362,11 @@ static int dw_spi_reset(struct udevice *bus)
 	return 0;
 }
 
+typedef int (*dw_spi_init_t)(struct udevice *bus, struct dw_spi_priv *priv);
+
 static int dw_spi_probe(struct udevice *bus)
 {
+	dw_spi_init_t init = (dw_spi_init_t)dev_get_driver_data(bus);
 	struct dw_spi_platdata *plat = dev_get_platdata(bus);
 	struct dw_spi_priv *priv = dev_get_priv(bus);
 	int ret;
@@ -274,6 +382,12 @@ static int dw_spi_probe(struct udevice *bus)
 	if (ret)
 		return ret;
 
+	if (!init)
+		return -EINVAL;
+	ret = init(bus, priv);
+	if (ret)
+		return ret;
+
 	/* Currently only bits_per_word == 8 supported */
 	priv->bits_per_word = 8;
 
@@ -404,23 +518,18 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	if (flags & SPI_XFER_BEGIN)
 		external_cs_manage(dev, false);
 
-	cr0 = (priv->bits_per_word - 1) | (priv->type << SPI_FRF_OFFSET) |
-		(priv->mode << SPI_MODE_OFFSET) |
-		(priv->tmode << SPI_TMOD_OFFSET);
-
 	if (rx && tx)
-		priv->tmode = SPI_TMOD_TR;
+		priv->tmode = CTRLR0_TMOD_TR;
 	else if (rx)
-		priv->tmode = SPI_TMOD_RO;
+		priv->tmode = CTRLR0_TMOD_RO;
 	else
 		/*
-		 * In transmit only mode (SPI_TMOD_TO) input FIFO never gets
+		 * In transmit only mode (CTRL0_TMOD_TO) input FIFO never gets
 		 * any data which breaks our logic in poll_transfer() above.
 		 */
-		priv->tmode = SPI_TMOD_TR;
+		priv->tmode = CTRLR0_TMOD_TR;
 
-	cr0 &= ~SPI_TMOD_MASK;
-	cr0 |= (priv->tmode << SPI_TMOD_OFFSET);
+	cr0 = priv->update_cr0(priv);
 
 	priv->len = bitlen >> 3;
 	log_debug("rx=%p tx=%p len=%d [bytes]\n", rx, tx, priv->len);
@@ -474,7 +583,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 
 static int dw_spi_set_speed(struct udevice *bus, uint speed)
 {
-	struct dw_spi_platdata *plat = bus->platdata;
+	struct dw_spi_platdata *plat = dev_get_platdata(bus);
 	struct dw_spi_priv *priv = dev_get_priv(bus);
 	u16 clk_div;
 
@@ -546,7 +655,43 @@ static const struct dm_spi_ops dw_spi_ops = {
 };
 
 static const struct udevice_id dw_spi_ids[] = {
-	{ .compatible = "snps,dw-apb-ssi" },
+	/* Generic compatible strings */
+
+	{ .compatible = "snps,dw-apb-ssi", .data = (ulong)dw_spi_dw_generic_init },
+	{ .compatible = "snps,dw-apb-ssi-3.20a", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "snps,dw-apb-ssi-3.22a", .data = (ulong)dw_spi_dw16_init },
+	/*
+	 * The exact version of the Intel Quark D20000 SPI device is documented
+	 * as 3.23*, so a "patch" version is not included.
+	 */
+	{ .compatible = "snps,dw-apb-ssi-3.23", .data = (ulong)dw_spi_dw32_init },
+	{ .compatible = "snps,dw-apb-ssi-4.00a", .data = (ulong)dw_spi_dw32_init },
+	/*
+	 * Similarly, the version of spi0-2 on the Canaan Kendryte K210 is
+	 * undocumented, and was determined empherically to be 4.01*.
+	 */
+	{ .compatible = "snps,dw-apb-ssi-4.01", .data = (ulong)dw_spi_dw32_init },
+	{ .compatible = "snps,dwc-ssi-1.01a", .data = (ulong)dw_spi_dwc_init },
+
+	/* Compatible strings for specific SoCs */
+
+	/*
+	 * Both the Cyclone V and Arria V share a device tree and have the same
+	 * version of this device. This compatible string is used for those
+	 * devices, and is not used for sofpgas in general.
+	 */
+	{ .compatible = "altr,socfpga-spi", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "altr,socfpga-arria10-spi", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "canaan,kendryte-k210-spi", .data = (ulong)dw_spi_dw32_init },
+	{ .compatible = "canaan,kendryte-k210-ssi", .data = (ulong)dw_spi_dwc_init },
+	/* FIXME: next two should be dw_spi_dw32_init; but they need testing */
+	{ .compatible = "intel,stratix10-spi", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "intel,agilex-spi", .data = (ulong)dw_spi_dw16_init },
+	/* FIXME: is this the correct version for the next four? */
+	{ .compatible = "mscc,ocelot-spi", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "mscc,jaguar2-spi", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "snps,axs10x-spi", .data = (ulong)dw_spi_dw16_init },
+	{ .compatible = "snps,hsdk-spi", .data = (ulong)dw_spi_dw16_init },
 	{ }
 };
 
-- 
2.28.0

[PATCH v2 07/10] spi: dw: Document devicetree binding

[Sean Anderson]

This documentation has been taken from Linux commit 3d7db0f11c7a,
immediately before the file was deleted and replaced with a yaml version.
Additional compatible strings from newer versions have been added, as well
as a few U-Boot-specific ones.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Document new compatible strings
- Split off from ctrlr0 commit

 .../spi/snps,dw-apb-ssi.txt                   | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt

diff --git a/doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt b/doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt
new file mode 100644
index 0000000000..bbdc9608dd
--- /dev/null
+++ b/doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt
@@ -0,0 +1,56 @@
+Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface
+and Synopsys DesignWare High Performance Synchronous Serial Interface
+
+Required properties:
+- compatible : One of
+  "altr,socfpga-arria5-ssi",
+  "altr,socfpga-arria10-ssi",
+  "altr,socfpga-cyclone5-ssi",
+  "canaan,kendryte-k210-dw-apb-ssi",
+  "canaan,kendryte-k210-dwc-ssi",
+  "intel,stratix10-ssi",
+  "intel,agilex-ssi",
+  "mscc,ocelot-spi",
+  or "mscc,jaguar2-spi";
+  and one of
+  "snps,dw-apb-ssi-3.20a",
+  "snps,dw-apb-ssi-3.22a",
+  "snps,dw-apb-ssi-3.23",
+  "snps,dw-apb-ssi-4.00a",
+  "snps,dw-apb-ssi-4.01",
+  or "snps,dwc-ssi-1.01a".
+  "snps,dw-apb-ssi" may also be used, but is deprecated in favor of specific
+  version strings.
+- reg : The register base for the controller. For "mscc,<soc>-spi", a second
+  register set is required (named ICPU_CFG:SPI_MST)
+- #address-cells : <1>, as required by generic SPI binding.
+- #size-cells : <0>, also as required by generic SPI binding.
+- clocks : phandles for the clocks, see the description of clock-names below.
+   The phandle for the "ssi_clk" is required. The phandle for the "pclk" clock
+   is optional. If a single clock is specified but no clock-name, it is the
+   "ssi_clk" clock. If both clocks are listed, the "ssi_clk" must be first.
+
+Optional properties:
+- clock-names : Contains the names of the clocks:
+    "ssi_clk", for the core clock used to generate the external SPI clock.
+    "pclk", the interface clock, required for register access.
+- cs-gpios : Specifies the gpio pins to be used for chipselects.
+- num-cs : The number of chipselects. If omitted, this will default to 4.
+- reg-io-width : The I/O register width (in bytes) implemented by this
+  device.  Supported values are 2 or 4 (the default).
+
+Child nodes as per the generic SPI binding.
+
+Example:
+
+	spi at fff00000 {
+		compatible = "snps,dw-apb-ssi";
+		reg = <0xfff00000 0x1000>;
+		interrupts = <0 154 4>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&spi_m_clk>;
+		num-cs = <2>;
+		cs-gpios = <&gpio0 13 0>,
+			   <&gpio0 14 0>;
+	};
-- 
2.28.0

[PATCH v2 08/10] spi: dw: Add mem_ops

[Sean Anderson]

The designware ssi device has "broken" chip select behaviour [1], and needs
specific manipulation to use the built-in chip select. The existing fix is
to use an external GPIO for chip select, but typically the K210 has SPI3
directly connected to a flash chip with dedicated pins. This makes it
impossible to use the spi_xfer function to use spi, since the CS is
de-asserted in between calls.  This patch adds an implementation of
exec_op, which gives correct behaviour when reading/writing spi flash.

[1] https://lkml.org/lkml/2015/12/23/132

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
This patch was previously part of
https://patchwork.ozlabs.org/project/uboot/list/?series=161576

Changes in v2:
- Add external gpio cs support
- Clean up exec_op
- Convert debug to log_*
- Limit data transfers to 64k

 drivers/spi/designware_spi.c | 104 +++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index d9dc739d7d..62c0bda4cc 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -19,6 +19,7 @@
 #include <errno.h>
 #include <malloc.h>
 #include <spi.h>
+#include <spi-mem.h>
 #include <fdtdec.h>
 #include <reset.h>
 #include <dm/device_compat.h>
@@ -581,6 +582,108 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	return ret;
 }
 
+/*
+ * This function is necessary for reading SPI flash with the native CS
+ * c.f. https://lkml.org/lkml/2015/12/23/132
+ */
+static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
+{
+	bool read = op->data.dir == SPI_MEM_DATA_IN;
+	int pos, i, ret = 0;
+	struct udevice *bus = slave->dev->parent;
+	struct dw_spi_priv *priv = dev_get_priv(bus);
+	u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
+	u8 op_buf[op_len];
+	u32 cr0;
+
+	if (read)
+		priv->tmode = CTRLR0_TMOD_EPROMREAD;
+	else
+		priv->tmode = CTRLR0_TMOD_TO;
+
+	log_debug("buf=%p len=%u [bytes]\n", op->data.buf.in, op->data.nbytes);
+
+	cr0 = priv->update_cr0(priv);
+	log_debug("cr0=%08x\n", cr0);
+
+	spi_enable_chip(priv, 0);
+	dw_write(priv, DW_SPI_CTRL0, cr0);
+	if (read)
+		dw_write(priv, DW_SPI_CTRL1, op->data.nbytes - 1);
+	spi_enable_chip(priv, 1);
+
+	/* From spi_mem_exec_op */
+	pos = 0;
+	op_buf[pos++] = op->cmd.opcode;
+	if (op->addr.nbytes) {
+		for (i = 0; i < op->addr.nbytes; i++)
+			op_buf[pos + i] = op->addr.val >>
+				(8 * (op->addr.nbytes - i - 1));
+
+		pos += op->addr.nbytes;
+	}
+	if (op->dummy.nbytes)
+		memset(op_buf + pos, 0xff, op->dummy.nbytes);
+
+	external_cs_manage(slave->dev, false);
+
+	priv->tx = &op_buf;
+	priv->tx_end = priv->tx + op_len;
+	priv->rx = NULL;
+	priv->rx_end = NULL;
+	while (priv->tx != priv->tx_end)
+		dw_writer(priv);
+
+	/*
+	 * XXX: The following are tight loops! Enabling debug messages may cause
+	 * them to fail because we are not reading/writing the fifo fast enough.
+	 */
+	if (read) {
+		priv->rx = op->data.buf.in;
+		priv->rx_end = priv->rx + op->data.nbytes;
+
+		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
+		while (priv->rx != priv->rx_end)
+			dw_reader(priv);
+	} else {
+		u32 val;
+
+		priv->tx = op->data.buf.out;
+		priv->tx_end = priv->tx + op->data.nbytes;
+
+		/* Fill up the write fifo before starting the transfer */
+		dw_writer(priv);
+		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
+		while (priv->tx != priv->tx_end)
+			dw_writer(priv);
+
+		if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
+				       (val & SR_TF_EMPT) && !(val & SR_BUSY),
+				       RX_TIMEOUT * 1000)) {
+			ret = -ETIMEDOUT;
+		}
+	}
+
+	dw_write(priv, DW_SPI_SER, 0);
+	external_cs_manage(slave->dev, true);
+
+	log_debug("%u bytes xfered\n", op->data.nbytes);
+	return ret;
+}
+
+/* The size of ctrl1 limits data transfers to 64K */
+static int dw_spi_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op)
+{
+	op->data.nbytes = min(op->data.nbytes, (unsigned int)SZ_64K);
+
+	return 0;
+}
+
+static const struct spi_controller_mem_ops dw_spi_mem_ops = {
+	.exec_op = dw_spi_exec_op,
+	.adjust_op_size = dw_spi_adjust_op_size,
+};
+
 static int dw_spi_set_speed(struct udevice *bus, uint speed)
 {
 	struct dw_spi_platdata *plat = dev_get_platdata(bus);
@@ -646,6 +749,7 @@ static int dw_spi_remove(struct udevice *bus)
 
 static const struct dm_spi_ops dw_spi_ops = {
 	.xfer		= dw_spi_xfer,
+	.mem_ops	= &dw_spi_mem_ops,
 	.set_speed	= dw_spi_set_speed,
 	.set_mode	= dw_spi_set_mode,
 	/*
-- 
2.28.0

[PATCH v2 09/10] riscv: Add device tree bindings for SPI

[Sean Anderson]

This patch adds bindings for the MMC slot and SPI flash on the Sipeed Maix
Bit.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
This patch was previously part of
https://patchwork.ozlabs.org/project/uboot/list/?series=161576

Changes in v2:
- Remove broken-wp property (implicit due to no wp gpio)
- Remove ctrl0 field offsets from device tree
- Switch to new compatible strings
- Switch to new pinmux binding style

 arch/riscv/dts/k210-maix-bit.dts | 46 +++++++++++++++++++++++++++++++-
 arch/riscv/dts/k210.dtsi         |  2 ++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/dts/k210-maix-bit.dts b/arch/riscv/dts/k210-maix-bit.dts
index e840e04ada..73892c6450 100644
--- a/arch/riscv/dts/k210-maix-bit.dts
+++ b/arch/riscv/dts/k210-maix-bit.dts
@@ -141,7 +141,7 @@
 		pinmux = <K210_FPIOA(26, K210_PCF_SPI1_D1)>,
 			 <K210_FPIOA(27, K210_PCF_SPI1_SCLK)>,
 			 <K210_FPIOA(28, K210_PCF_SPI1_D0)>,
-			 <K210_FPIOA(29, K210_PCF_GPIOHS13)>;
+			 <K210_FPIOA(29, K210_PCF_GPIOHS13)>; /* cs */
 	};
 };
 
@@ -149,3 +149,47 @@
 	pinctrl-0 = <&fpioa_dvp>;
 	pinctrl-names = "default";
 };
+
+&spi0 {
+	pinctrl-0 = <&fpioa_spi0>;
+	pinctrl-names = "default";
+	num-cs = <1>;
+	cs-gpios = <&gpio0 20 0>;
+
+	panel at 0 {
+		compatible = "sitronix,st7789v";
+		reg = <0>;
+		reset-gpios = <&gpio0 21 GPIO_ACTIVE_LOW>;
+		dc-gpios = <&gpio0 22 0>;
+		spi-max-frequency = <15000000>;
+		status = "disabled";
+	};
+};
+
+&spi1 {
+	pinctrl-0 = <&fpioa_spi1>;
+	pinctrl-names = "default";
+	num-cs = <1>;
+	cs-gpios = <&gpio0 13 0>;
+	status = "okay";
+
+	slot at 0 {
+		compatible = "mmc-spi-slot";
+		reg = <0>;
+		spi-max-frequency = <25000000>;
+		voltage-ranges = <3300 3300>;
+		broken-cd;
+	};
+};
+
+&spi3 {
+	status = "okay";
+
+	spi-flash at 0 {
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+		spi-max-frequency = <50000000>;
+		m25p,fast-read;
+		broken-flash-reset;
+	};
+};
diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
index 429891d651..fe47ee7aaf 100644
--- a/arch/riscv/dts/k210.dtsi
+++ b/arch/riscv/dts/k210.dtsi
@@ -495,6 +495,8 @@
 				interrupts = <24>;
 				clocks = <&sysclk K210_CLK_DVP>;
 				resets = <&sysrst K210_RST_DVP>;
+				kendryte,sysctl = <&sysctl>;
+				kendryte,misc-offset = <K210_SYSCTL_MISC>;
 				status = "disabled";
 			};
 
-- 
2.28.0

[PATCH v2 10/10] riscv: Add support for SPI on Kendryte K210

[Sean Anderson]

This patch enables configs necessary for using SPI. It also adds some
documentation.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Add Gigadevice SPI chips to dependencies

 board/sipeed/maix/Kconfig          | 11 ++++
 configs/sipeed_maix_bitm_defconfig |  8 +++
 doc/board/sipeed/maix.rst          | 94 +++++++++++++++++++-----------
 3 files changed, 80 insertions(+), 33 deletions(-)

diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
index 4c42dd2087..48a4e9dc1a 100644
--- a/board/sipeed/maix/Kconfig
+++ b/board/sipeed/maix/Kconfig
@@ -53,4 +53,15 @@ config BOARD_SPECIFIC_OPTIONS
 	imply CMD_GPIO
 	imply LED
 	imply LED_GPIO
+	imply SPI
+	imply DESIGNWARE_SPI
+	imply SPI_FLASH_GIGADEVICE
+	imply SPI_FLASH_WINBOND
+	imply DM_MTD
+	imply SPI_FLASH_MTD
+	imply CMD_MTD
+	imply ENV_IS_IN_SPI_FLASH
+	imply MMC
+	imply MMC_BROKEN_CD
+	imply MMC_SPI
 endif
diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
index f48f7f06e9..7f644e7a37 100644
--- a/configs/sipeed_maix_bitm_defconfig
+++ b/configs/sipeed_maix_bitm_defconfig
@@ -1,6 +1,14 @@
 CONFIG_RISCV=y
+CONFIG_ENV_SIZE=0x2000
+CONFIG_ENV_SECT_SIZE=0x1000
+CONFIG_ENV_OFFSET=0x7C000
+CONFIG_ENV_OFFSET_REDUND=0x7E000
 CONFIG_TARGET_SIPEED_MAIX=y
 CONFIG_ARCH_RV64I=y
+CONFIG_USE_BOOTCOMMAND=y
+CONFIG_BOOTCOMMAND="sf probe;mtd read kernel 80000000;go 80000000"
+CONFIG_MTDIDS_DEFAULT="nor0=spi3.0"
+CONFIG_MTDPARTS_DEFAULT="spi3.0:496k(u-boot),16k(env),5632k(kernel),10240k(data)"
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 # CONFIG_NET is not set
 # CONFIG_INPUT is not set
diff --git a/doc/board/sipeed/maix.rst b/doc/board/sipeed/maix.rst
index 1865e2adfb..fa78bcd51c 100644
--- a/doc/board/sipeed/maix.rst
+++ b/doc/board/sipeed/maix.rst
@@ -46,42 +46,14 @@ Boot output should look like the following:
     U-Boot 2020.04-rc2-00087-g2221cc09c1-dirty (Feb 28 2020 - 13:53:09 -0500)
 
     DRAM:  8 MiB
+    MMC:   spi at 53000000:slot at 0: 0
     In:    serial at 38000000
     Out:   serial at 38000000
     Err:   serial at 38000000
-    =>
-
-Loading Images
-^^^^^^^^^^^^^^
-
-To load a kernel, transfer it over serial.
-
-.. code-block:: none
-
-    => loady 80000000 1500000
-    ## Switch baudrate to 1500000 bps and press ENTER ...
-
-    *** baud: 1500000
-
-    *** baud: 1500000 ***
-    ## Ready for binary (ymodem) download to 0x80000000 at 1500000 bps...
-    C
-    *** file: loader.bin
-    $ sz -vv loader.bin
-    Sending: loader.bin
-    Bytes Sent:2478208   BPS:72937
-    Sending:
-    Ymodem sectors/kbytes sent:   0/ 0k
-    Transfer complete
-
-    *** exit status: 0 ***
-    ## Total Size      = 0x0025d052 = 2478162 Bytes
-    ## Switch baudrate to 115200 bps and press ESC ...
-
-    *** baud: 115200
-
-    *** baud: 115200 ***
-    =>
+    Hit any key to stop autoboot:  0
+    SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
+    Reading 5242880 byte(s) at offset 0x00000000
+    ## Starting application at 0x80000000 ...
 
 Running Programs
 ^^^^^^^^^^^^^^^^
@@ -163,6 +135,62 @@ To run legacy images, use the ``bootm`` command:
     argv[0] = "<NULL>"
     Hit any key to exit ...
 
+Flashing Images
+---------------
+
+To flash a kernel, transfer it over serial, then write it to the kernel
+partition.
+
+.. code-block:: none
+
+    => loady 80000000 1500000
+    ## Switch baudrate to 1500000 bps and press ENTER ...
+
+    *** baud: 1500000
+
+    *** baud: 1500000 ***
+    ## Ready for binary (ymodem) download to 0x80000000 at 1500000 bps...
+    C
+    *** file: loader.bin
+    $ sz -vv loader.bin
+    Sending: loader.bin
+    Bytes Sent:2478208   BPS:72937
+    Sending:
+    Ymodem sectors/kbytes sent:   0/ 0k
+    Transfer complete
+
+    *** exit status: 0 ***
+    ## Total Size      = 0x0025d052 = 2478162 Bytes
+    ## Switch baudrate to 115200 bps and press ESC ...
+
+    *** baud: 115200
+
+    *** baud: 115200 ***
+    => sf probe
+    SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
+    => mtd write kernel 80000000 0 25d052
+    Writing 2478162 byte(s) at offset 0x00000000
+
+Partition Scheme
+^^^^^^^^^^^^^^^^
+
+There is no partition scheme specified by the manufacturer. The only requirement
+imposed by the firmware is that offset 0 will be loaded and ran. The default
+partition scheme is
+
+========= ======== ======
+Partition Offset   Size
+========= ======== ======
+u-boot    0x000000 496k
+env       0x07C000 16k
+kernel    0x080000 5M
+data      0x580000 10.5M
+========= ======== ======
+
+**NB:** kflash adds a 5-byte header to payloads (and a 32-byte trailer) to all
+payloads it flashes. If you use kflash to flash your payload, you will need to
+account for this header when specifying what offset in spi flash to load from.
+
 Pin Assignment
 --------------
 
-- 
2.28.0

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Marek Vasut]

On 8/7/20 4:43 PM, Sean Anderson wrote:
[...]
> +#define LOG_CATEGORY UCLASS_SPI
>  #include <common.h>
>  #include <log.h>
>  #include <asm-generic/gpio.h>
> @@ -139,7 +140,7 @@ static int request_gpio_cs(struct udevice *bus)
>  		return 0;
>  
>  	if (ret < 0) {
> -		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
> +		log_err("Error: %d: Can't get %s gpio!\n", ret, bus->name);

Can't we use dev_info() / pr_info() , same as the linux kernel does ?

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Sean Anderson]

On 8/7/20 10:49 AM, Marek Vasut wrote:
> On 8/7/20 4:43 PM, Sean Anderson wrote:
> [...]
>> +#define LOG_CATEGORY UCLASS_SPI
>>  #include <common.h>
>>  #include <log.h>
>>  #include <asm-generic/gpio.h>
>> @@ -139,7 +140,7 @@ static int request_gpio_cs(struct udevice *bus)
>>  		return 0;
>>  
>>  	if (ret < 0) {
>> -		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
>> +		log_err("Error: %d: Can't get %s gpio!\n", ret, bus->name);
> 
> Can't we use dev_info() / pr_info() , same as the linux kernel does ?
> 

We can. It doesn't matter much to me. Simon, does U-Boot prefer a
particular log style for new code?

--Sean

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Heinrich Schuchardt]

On 8/7/20 4:43 PM, Sean Anderson wrote:
> This allows different log levels to be enabled or disabled depending on the
> desired level of verbosity. In particular, it allows for general debug
> information to be printed while excluding more verbose logging which may
> interfere with timing.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---/
>
> Changes in v2:
> - New
>
>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index c9b14f9029..f7ea3edaab 100644
> --- a/drivers/spi/designware_spi.c
> +++ b/drivers/spi/designware_spi.c
> @@ -9,6 +9,7 @@
>   * Copyright (c) 2009, Intel Corporation.
>   */
>
> +#define LOG_CATEGORY UCLASS_SPI
>  #include <common.h>
>  #include <log.h>
>  #include <asm-generic/gpio.h>
> @@ -139,7 +140,7 @@ static int request_gpio_cs(struct udevice *bus)
>  		return 0;
>
>  	if (ret < 0) {
> -		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
> +		log_err("Error: %d: Can't get %s gpio!\n", ret, bus->name);

Thanks for caring to convert this to the more flexible logging.

	Error: -23: Can't get spi at 53000000 gpio!

Shouldn't we remove one colon:

	Error -23: Can't get spi at 53000000 gpio!

>  		return ret;
>  	}
>
> @@ -148,7 +149,7 @@ static int request_gpio_cs(struct udevice *bus)
>  				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>  	}
>
> -	debug("%s: used external gpio for CS management\n", __func__);
> +	log_info("Using external gpio for CS management\n");

On systems with CONFIG_LOG=n log_info() messages are always printed. By
default the function name is not printed in log messages.

Showing this message to the end-user of the device on every boot
provides no benefit.

log_debug() seems more adequate.

>  #endif
>  	return 0;
>  }
> @@ -162,8 +163,7 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
>  	/* Use 500KHz as a suitable default */
>  	plat->frequency = dev_read_u32_default(bus, "spi-max-frequency",
>  					       500000);
> -	debug("%s: regs=%p max-frequency=%d\n", __func__, plat->regs,
> -	      plat->frequency);
> +	log_info("regs=%p max-frequency=%d\n", plat->regs, plat->frequency);

The output will look like this:

	regs=0x1234 max-frequency=2000000

Such a message appearing on every boot will be irritating for end-users.

Please, use log_debug() here.

Can we replace 'regs=' by 'SPI@' for all messages?

>
>  	return request_gpio_cs(bus);
>  }
> @@ -196,7 +196,7 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>  		priv->fifo_len = (fifo == 1) ? 0 : fifo;
>  		dw_write(priv, DW_SPI_TXFLTR, 0);
>  	}
> -	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
> +	log_debug("fifo_len=%d\n", priv->fifo_len);
>  }
>
>  /*
> @@ -221,8 +221,7 @@ __weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
>  	if (!*rate)
>  		goto err_rate;
>
> -	debug("%s: get spi controller clk via device tree: %lu Hz\n",
> -	      __func__, *rate);
> +	log_debug("Got spi controller clk via device tree: %lu Hz\n", *rate);

%s/spi/SPI/

>
>  	return 0;
>
> @@ -247,14 +246,14 @@ static int dw_spi_reset(struct udevice *bus)
>  		if (ret == -ENOENT || ret == -ENOTSUPP)
>  			return 0;
>
> -		dev_warn(bus, "Can't get reset: %d\n", ret);
> +		log_warning("Can't get reset: %d\n", ret);

This message does not tell me that there is a problem with SPI. Please,
provide a useful text.

>  		return ret;
>  	}
>
>  	ret = reset_deassert_bulk(&priv->resets);
>  	if (ret) {
>  		reset_release_bulk(&priv->resets);
> -		dev_err(bus, "Failed to reset: %d\n", ret);
> +		log_err("Failed to reset: %d\n", ret);

What shall I do when reading a message like:

	"Failed to reset: -23".

Please, provide a more informative text.

>  		return ret;
>  	}
>
> @@ -333,7 +332,7 @@ static void dw_writer(struct dw_spi_priv *priv)
>  				txw = *(u16 *)(priv->tx);
>  		}
>  		dw_write(priv, DW_SPI_DR, txw);
> -		debug("%s: tx=0x%02x\n", __func__, txw);
> +		log_content("tx=0x%02x\n", txw);
>  		priv->tx += priv->bits_per_word >> 3;
>  	}
>  }
> @@ -345,7 +344,7 @@ static void dw_reader(struct dw_spi_priv *priv)
>
>  	while (max--) {
>  		rxw = dw_read(priv, DW_SPI_DR);
> -		debug("%s: rx=0x%02x\n", __func__, rxw);
> +		log_content("rx=0x%02x\n", rxw);
>
>  		/* Care about rx if the transfer's original "rx" is not null */
>  		if (priv->rx_end - priv->len) {
> @@ -400,7 +399,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>
>  	/* spi core configured to do 8 bit transfers */
>  	if (bitlen % 8) {
> -		debug("Non byte aligned SPI transfer.\n");
> +		log_err("Non byte aligned SPI transfer.\n");
>  		return -1;
>  	}
>
> @@ -427,7 +426,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	cr0 |= (priv->tmode << SPI_TMOD_OFFSET);
>
>  	priv->len = bitlen >> 3;
> -	debug("%s: rx=%p tx=%p len=%d [bytes]\n", __func__, rx, tx, priv->len);
> +	log_debug("rx=%p tx=%p len=%d [bytes]\n", rx, tx, priv->len);

Please, use log_debug_io() here.

>
>  	priv->tx = (void *)tx;
>  	priv->tx_end = priv->tx + priv->len;
> @@ -437,7 +436,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	/* Disable controller before writing control registers */
>  	spi_enable_chip(priv, 0);
>
> -	debug("%s: cr0=%08x\n", __func__, cr0);
> +	log_debug("cr0=%08x\n", cr0);

log_debug_io()

Best regards

Heinrich

>  	/* Reprogram cr0 only if changed */
>  	if (dw_read(priv, DW_SPI_CTRL0) != cr0)
>  		dw_write(priv, DW_SPI_CTRL0, cr0);
> @@ -497,8 +496,8 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed)
>  	spi_enable_chip(priv, 1);
>
>  	priv->freq = speed;
> -	debug("%s: regs=%p speed=%d clk_div=%d\n", __func__, priv->regs,
> -	      priv->freq, clk_div);
> +	log_debug("regs=%p speed=%d clk_div=%d\n", priv->regs, priv->freq,
> +		  clk_div);
>
>  	return 0;
>  }
> @@ -513,7 +512,7 @@ static int dw_spi_set_mode(struct udevice *bus, uint mode)
>  	 * real transfer function.
>  	 */
>  	priv->mode = mode;
> -	debug("%s: regs=%p, mode=%d\n", __func__, priv->regs, priv->mode);
> +	log_debug("regs=%p, mode=%d\n", priv->regs, priv->mode);
>
>  	return 0;
>  }
>

[PATCH v2 10/10] riscv: Add support for SPI on Kendryte K210

[Heinrich Schuchardt]

On 8/7/20 4:43 PM, Sean Anderson wrote:
> This patch enables configs necessary for using SPI. It also adds some
> documentation.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - Add Gigadevice SPI chips to dependencies
>
>  board/sipeed/maix/Kconfig          | 11 ++++
>  configs/sipeed_maix_bitm_defconfig |  8 +++
>  doc/board/sipeed/maix.rst          | 94 +++++++++++++++++++-----------
>  3 files changed, 80 insertions(+), 33 deletions(-)
>
> diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
> index 4c42dd2087..48a4e9dc1a 100644
> --- a/board/sipeed/maix/Kconfig
> +++ b/board/sipeed/maix/Kconfig
> @@ -53,4 +53,15 @@ config BOARD_SPECIFIC_OPTIONS
>  	imply CMD_GPIO
>  	imply LED
>  	imply LED_GPIO
> +	imply SPI
> +	imply DESIGNWARE_SPI
> +	imply SPI_FLASH_GIGADEVICE
> +	imply SPI_FLASH_WINBOND
> +	imply DM_MTD
> +	imply SPI_FLASH_MTD
> +	imply CMD_MTD
> +	imply ENV_IS_IN_SPI_FLASH
> +	imply MMC
> +	imply MMC_BROKEN_CD
> +	imply MMC_SPI
>  endif
> diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
> index f48f7f06e9..7f644e7a37 100644
> --- a/configs/sipeed_maix_bitm_defconfig
> +++ b/configs/sipeed_maix_bitm_defconfig
> @@ -1,6 +1,14 @@
>  CONFIG_RISCV=y
> +CONFIG_ENV_SIZE=0x2000
> +CONFIG_ENV_SECT_SIZE=0x1000
> +CONFIG_ENV_OFFSET=0x7C000
> +CONFIG_ENV_OFFSET_REDUND=0x7E000
>  CONFIG_TARGET_SIPEED_MAIX=y
>  CONFIG_ARCH_RV64I=y
> +CONFIG_USE_BOOTCOMMAND=y
> +CONFIG_BOOTCOMMAND="sf probe;mtd read kernel 80000000;go 80000000"

If no kernel is present, this will let your system hang.

"go" does not check that whatever you loaded from mtd is a kernel. That
is why we have commands like bootm.

> +CONFIG_MTDIDS_DEFAULT="nor0=spi3.0"
> +CONFIG_MTDPARTS_DEFAULT="spi3.0:496k(u-boot),16k(env),5632k(kernel),10240k(data)"

The size of u-boot.bin may exceed 496 KiB depending on the options
selected. Mine is currently 543589 bytes. When starting with OpenSBI
another 70+ KiB will be needed.

If we overwrite 5632 KiB from the kernel partition, this may overwrite
the relocated U-Boot.

When an SD-card is worn out I can replace it. When the SPI flash is worn
out I have to replace the device.

We can put the kernel and the environment onto the SD card. I would
suggest to use SPI only for OpenSBI and U-Boot. Please, use
CONFIG_DISTRO_BOOT.

>  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
>  # CONFIG_NET is not set
>  # CONFIG_INPUT is not set
> diff --git a/doc/board/sipeed/maix.rst b/doc/board/sipeed/maix.rst
> index 1865e2adfb..fa78bcd51c 100644
> --- a/doc/board/sipeed/maix.rst
> +++ b/doc/board/sipeed/maix.rst
> @@ -46,42 +46,14 @@ Boot output should look like the following:
>      U-Boot 2020.04-rc2-00087-g2221cc09c1-dirty (Feb 28 2020 - 13:53:09 -0500)
>
>      DRAM:  8 MiB
> +    MMC:   spi at 53000000:slot at 0: 0

Actual output is

MMC:   regs=0000000053000000 max-frequency=25000000

>      In:    serial at 38000000
>      Out:   serial at 38000000
>      Err:   serial at 38000000

As we do not check if a kernel is available:

MTD device kernel not found, ret -19
## Starting application at 0x80000000 ...

And then hanging.

Best regards

Heinrich

> -    =>
> -
> -Loading Images
> -^^^^^^^^^^^^^^
> -
> -To load a kernel, transfer it over serial.
> -
> -.. code-block:: none
> -
> -    => loady 80000000 1500000
> -    ## Switch baudrate to 1500000 bps and press ENTER ...
> -
> -    *** baud: 1500000
> -
> -    *** baud: 1500000 ***
> -    ## Ready for binary (ymodem) download to 0x80000000 at 1500000 bps...
> -    C
> -    *** file: loader.bin
> -    $ sz -vv loader.bin
> -    Sending: loader.bin
> -    Bytes Sent:2478208   BPS:72937
> -    Sending:
> -    Ymodem sectors/kbytes sent:   0/ 0k
> -    Transfer complete
> -
> -    *** exit status: 0 ***
> -    ## Total Size      = 0x0025d052 = 2478162 Bytes
> -    ## Switch baudrate to 115200 bps and press ESC ...
> -
> -    *** baud: 115200
> -
> -    *** baud: 115200 ***
> -    =>
> +    Hit any key to stop autoboot:  0
> +    SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> +    Reading 5242880 byte(s) at offset 0x00000000
> +    ## Starting application at 0x80000000 ...
>
>  Running Programs
>  ^^^^^^^^^^^^^^^^
> @@ -163,6 +135,62 @@ To run legacy images, use the ``bootm`` command:
>      argv[0] = "<NULL>"
>      Hit any key to exit ...
>
> +Flashing Images
> +---------------
> +
> +To flash a kernel, transfer it over serial, then write it to the kernel
> +partition.
> +
> +.. code-block:: none
> +
> +    => loady 80000000 1500000
> +    ## Switch baudrate to 1500000 bps and press ENTER ...
> +
> +    *** baud: 1500000
> +
> +    *** baud: 1500000 ***
> +    ## Ready for binary (ymodem) download to 0x80000000 at 1500000 bps...
> +    C
> +    *** file: loader.bin
> +    $ sz -vv loader.bin
> +    Sending: loader.bin
> +    Bytes Sent:2478208   BPS:72937
> +    Sending:
> +    Ymodem sectors/kbytes sent:   0/ 0k
> +    Transfer complete
> +
> +    *** exit status: 0 ***
> +    ## Total Size      = 0x0025d052 = 2478162 Bytes
> +    ## Switch baudrate to 115200 bps and press ESC ...
> +
> +    *** baud: 115200
> +
> +    *** baud: 115200 ***
> +    => sf probe
> +    SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> +    => mtd write kernel 80000000 0 25d052
> +    Writing 2478162 byte(s) at offset 0x00000000
> +
> +Partition Scheme
> +^^^^^^^^^^^^^^^^
> +
> +There is no partition scheme specified by the manufacturer. The only requirement
> +imposed by the firmware is that offset 0 will be loaded and ran. The default
> +partition scheme is
> +
> +========= ======== ======
> +Partition Offset   Size
> +========= ======== ======
> +u-boot    0x000000 496k
> +env       0x07C000 16k
> +kernel    0x080000 5M
> +data      0x580000 10.5M
> +========= ======== ======
> +
> +**NB:** kflash adds a 5-byte header to payloads (and a 32-byte trailer) to all
> +payloads it flashes. If you use kflash to flash your payload, you will need to
> +account for this header when specifying what offset in spi flash to load from.
> +
>  Pin Assignment
>  --------------
>
>

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Sean Anderson]

On 8/8/20 1:14 AM, Heinrich Schuchardt wrote:
> On 8/7/20 4:43 PM, Sean Anderson wrote:
>> This allows different log levels to be enabled or disabled depending on the
>> desired level of verbosity. In particular, it allows for general debug
>> information to be printed while excluding more verbose logging which may
>> interfere with timing.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---/
>>
>> Changes in v2:
>> - New
>>
>>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>> index c9b14f9029..f7ea3edaab 100644
>> --- a/drivers/spi/designware_spi.c
>> +++ b/drivers/spi/designware_spi.c
>> @@ -9,6 +9,7 @@
>>   * Copyright (c) 2009, Intel Corporation.
>>   */
>>
>> +#define LOG_CATEGORY UCLASS_SPI
>>  #include <common.h>
>>  #include <log.h>
>>  #include <asm-generic/gpio.h>
>> @@ -139,7 +140,7 @@ static int request_gpio_cs(struct udevice *bus)
>>  		return 0;
>>
>>  	if (ret < 0) {
>> -		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
>> +		log_err("Error: %d: Can't get %s gpio!\n", ret, bus->name);
> 
> Thanks for caring to convert this to the more flexible logging.

It was quite difficult to debug without these changes, since turning on
debug statements caused the fifos to over/underflow.

> 
> 	Error: -23: Can't get spi at 53000000 gpio!
> 
> Shouldn't we remove one colon:
> 
> 	Error -23: Can't get spi at 53000000 gpio!

Sure.

>>  		return ret;
>>  	}
>>
>> @@ -148,7 +149,7 @@ static int request_gpio_cs(struct udevice *bus)
>>  				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>>  	}
>>
>> -	debug("%s: used external gpio for CS management\n", __func__);
>> +	log_info("Using external gpio for CS management\n");
> 
> On systems with CONFIG_LOG=n log_info() messages are always printed. By
> default the function name is not printed in log messages.
> 
> Showing this message to the end-user of the device on every boot
> provides no benefit.
> 
> log_debug() seems more adequate.

Ok. I was generally unsure what log level to default to. README.log
doesn't really make it clear semantically which log levels should be
used.

>>  #endif
>>  	return 0;
>>  }
>> @@ -162,8 +163,7 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
>>  	/* Use 500KHz as a suitable default */
>>  	plat->frequency = dev_read_u32_default(bus, "spi-max-frequency",
>>  					       500000);
>> -	debug("%s: regs=%p max-frequency=%d\n", __func__, plat->regs,
>> -	      plat->frequency);
>> +	log_info("regs=%p max-frequency=%d\n", plat->regs, plat->frequency);
> 
> The output will look like this:
> 
> 	regs=0x1234 max-frequency=2000000
> 
> Such a message appearing on every boot will be irritating for end-users.
> 
> Please, use log_debug() here.

Ok.

> Can we replace 'regs=' by 'SPI@' for all messages?

Sure.

>>
>>  	return request_gpio_cs(bus);
>>  }
>> @@ -196,7 +196,7 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>>  		priv->fifo_len = (fifo == 1) ? 0 : fifo;
>>  		dw_write(priv, DW_SPI_TXFLTR, 0);
>>  	}
>> -	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
>> +	log_debug("fifo_len=%d\n", priv->fifo_len);
>>  }
>>
>>  /*
>> @@ -221,8 +221,7 @@ __weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
>>  	if (!*rate)
>>  		goto err_rate;
>>
>> -	debug("%s: get spi controller clk via device tree: %lu Hz\n",
>> -	      __func__, *rate);
>> +	log_debug("Got spi controller clk via device tree: %lu Hz\n", *rate);
> 
> %s/spi/SPI/

Ok.

>>
>>  	return 0;
>>
>> @@ -247,14 +246,14 @@ static int dw_spi_reset(struct udevice *bus)
>>  		if (ret == -ENOENT || ret == -ENOTSUPP)
>>  			return 0;
>>
>> -		dev_warn(bus, "Can't get reset: %d\n", ret);
>> +		log_warning("Can't get reset: %d\n", ret);
> 
> This message does not tell me that there is a problem with SPI. Please,
> provide a useful text.

Perhaps "Couldn't find or assert reset device configured for SPI"?

>>  		return ret;
>>  	}
>>
>>  	ret = reset_deassert_bulk(&priv->resets);
>>  	if (ret) {
>>  		reset_release_bulk(&priv->resets);
>> -		dev_err(bus, "Failed to reset: %d\n", ret);
>> +		log_err("Failed to reset: %d\n", ret);
> 
> What shall I do when reading a message like:
> 
> 	"Failed to reset: -23".
> 
> Please, provide a more informative text.

Perhaps "SPI: failed to deassert reset (error %d)"?

>>  		return ret;
>>  	}
>>
>> @@ -333,7 +332,7 @@ static void dw_writer(struct dw_spi_priv *priv)
>>  				txw = *(u16 *)(priv->tx);
>>  		}
>>  		dw_write(priv, DW_SPI_DR, txw);
>> -		debug("%s: tx=0x%02x\n", __func__, txw);
>> +		log_content("tx=0x%02x\n", txw);
>>  		priv->tx += priv->bits_per_word >> 3;
>>  	}
>>  }
>> @@ -345,7 +344,7 @@ static void dw_reader(struct dw_spi_priv *priv)
>>
>>  	while (max--) {
>>  		rxw = dw_read(priv, DW_SPI_DR);
>> -		debug("%s: rx=0x%02x\n", __func__, rxw);
>> +		log_content("rx=0x%02x\n", rxw);
>>
>>  		/* Care about rx if the transfer's original "rx" is not null */
>>  		if (priv->rx_end - priv->len) {
>> @@ -400,7 +399,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>
>>  	/* spi core configured to do 8 bit transfers */
>>  	if (bitlen % 8) {
>> -		debug("Non byte aligned SPI transfer.\n");
>> +		log_err("Non byte aligned SPI transfer.\n");
>>  		return -1;
>>  	}
>>
>> @@ -427,7 +426,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>  	cr0 |= (priv->tmode << SPI_TMOD_OFFSET);
>>
>>  	priv->len = bitlen >> 3;
>> -	debug("%s: rx=%p tx=%p len=%d [bytes]\n", __func__, rx, tx, priv->len);
>> +	log_debug("rx=%p tx=%p len=%d [bytes]\n", rx, tx, priv->len);
> 
> Please, use log_debug_io() here.

When should log_content/log_io be used? Here, we only log metadata about
the transfer. I would expect this to be a higher log level than (for
example) the actual log of the content as done in dw_reader/writer. In
addition, I am confused as to when log_io should be used over
log_content. The only clear-cut case I can see is perhaps that replacing
dw_write with something like

static inline void dw_write(struct dw_spi_priv *priv, u32 offset, u32 val)
{
	log_io("%x = %x", offset, val);
	__raw_writel(val, priv->regs + offset);
}

Is log_content intended for higher-level interfaces?

>>
>>  	priv->tx = (void *)tx;
>>  	priv->tx_end = priv->tx + priv->len;
>> @@ -437,7 +436,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>  	/* Disable controller before writing control registers */
>>  	spi_enable_chip(priv, 0);
>>
>> -	debug("%s: cr0=%08x\n", __func__, cr0);
>> +	log_debug("cr0=%08x\n", cr0);
> 
> log_debug_io()

Same comments/questions as above.

--Sean

[PATCH v2 10/10] riscv: Add support for SPI on Kendryte K210

[Sean Anderson]

On 8/8/20 1:48 AM, Heinrich Schuchardt wrote:
> On 8/7/20 4:43 PM, Sean Anderson wrote:
>> This patch enables configs necessary for using SPI. It also adds some
>> documentation.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> Changes in v2:
>> - Add Gigadevice SPI chips to dependencies
>>
>>  board/sipeed/maix/Kconfig          | 11 ++++
>>  configs/sipeed_maix_bitm_defconfig |  8 +++
>>  doc/board/sipeed/maix.rst          | 94 +++++++++++++++++++-----------
>>  3 files changed, 80 insertions(+), 33 deletions(-)
>>
>> diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
>> index 4c42dd2087..48a4e9dc1a 100644
>> --- a/board/sipeed/maix/Kconfig
>> +++ b/board/sipeed/maix/Kconfig
>> @@ -53,4 +53,15 @@ config BOARD_SPECIFIC_OPTIONS
>>  	imply CMD_GPIO
>>  	imply LED
>>  	imply LED_GPIO
>> +	imply SPI
>> +	imply DESIGNWARE_SPI
>> +	imply SPI_FLASH_GIGADEVICE
>> +	imply SPI_FLASH_WINBOND
>> +	imply DM_MTD
>> +	imply SPI_FLASH_MTD
>> +	imply CMD_MTD
>> +	imply ENV_IS_IN_SPI_FLASH
>> +	imply MMC
>> +	imply MMC_BROKEN_CD
>> +	imply MMC_SPI
>>  endif
>> diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
>> index f48f7f06e9..7f644e7a37 100644
>> --- a/configs/sipeed_maix_bitm_defconfig
>> +++ b/configs/sipeed_maix_bitm_defconfig
>> @@ -1,6 +1,14 @@
>>  CONFIG_RISCV=y
>> +CONFIG_ENV_SIZE=0x2000
>> +CONFIG_ENV_SECT_SIZE=0x1000
>> +CONFIG_ENV_OFFSET=0x7C000
>> +CONFIG_ENV_OFFSET_REDUND=0x7E000
>>  CONFIG_TARGET_SIPEED_MAIX=y
>>  CONFIG_ARCH_RV64I=y
>> +CONFIG_USE_BOOTCOMMAND=y
>> +CONFIG_BOOTCOMMAND="sf probe;mtd read kernel 80000000;go 80000000"
> 
> If no kernel is present, this will let your system hang.
> 
> "go" does not check that whatever you loaded from mtd is a kernel. That
> is why we have commands like bootm.

I think originally I was having problems with bootm because the *addr
variables were not set, so I used go and not bootm. I will try again
with your patch for those variables.

>> +CONFIG_MTDIDS_DEFAULT="nor0=spi3.0"
>> +CONFIG_MTDPARTS_DEFAULT="spi3.0:496k(u-boot),16k(env),5632k(kernel),10240k(data)"
> 
> The size of u-boot.bin may exceed 496 KiB depending on the options
> selected. Mine is currently 543589 bytes. When starting with OpenSBI
> another 70+ KiB will be needed.

Huh. I had only seen U-Boot with size in the 200-300M range. We can
definitely increase this. Again, this is effectively arbitrary, and I'm
still not sure if doing partitions like this is the best approach.
Perhaps we should be using UBI here? I wasn't able to figure out what
the "best practice" is for this sort of board.

> If we overwrite 5632 KiB from the kernel partition, this may overwrite
> the relocated U-Boot.
> 
> When an SD-card is worn out I can replace it. When the SPI flash is worn
> out I have to replace the device.

Yeah, I would like to be booting off the SD card here as well, but I
haven't gotten it working yet.

> We can put the kernel and the environment onto the SD card. I would
> suggest to use SPI only for OpenSBI and U-Boot. Please, use
> CONFIG_DISTRO_BOOT.

Ok.

>>  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
>>  # CONFIG_NET is not set
>>  # CONFIG_INPUT is not set
>> diff --git a/doc/board/sipeed/maix.rst b/doc/board/sipeed/maix.rst
>> index 1865e2adfb..fa78bcd51c 100644
>> --- a/doc/board/sipeed/maix.rst
>> +++ b/doc/board/sipeed/maix.rst
>> @@ -46,42 +46,14 @@ Boot output should look like the following:
>>      U-Boot 2020.04-rc2-00087-g2221cc09c1-dirty (Feb 28 2020 - 13:53:09 -0500)
>>
>>      DRAM:  8 MiB
>> +    MMC:   spi at 53000000:slot at 0: 0
> 
> Actual output is
> 
> MMC:   regs=0000000053000000 max-frequency=25000000

Ok, I will update that.

>>      In:    serial at 38000000
>>      Out:   serial at 38000000
>>      Err:   serial at 38000000
> 
> As we do not check if a kernel is available:
> 
> MTD device kernel not found, ret -19
> ## Starting application at 0x80000000 ...
> 
> And then hanging.
> 
> Best regards
> 
> Heinrich
> 
>> -    =>
>> -
>> -Loading Images
>> -^^^^^^^^^^^^^^
>> -
>> -To load a kernel, transfer it over serial.
>> -
>> -.. code-block:: none
>> -
>> -    => loady 80000000 1500000
>> -    ## Switch baudrate to 1500000 bps and press ENTER ...
>> -
>> -    *** baud: 1500000
>> -
>> -    *** baud: 1500000 ***
>> -    ## Ready for binary (ymodem) download to 0x80000000 at 1500000 bps...
>> -    C
>> -    *** file: loader.bin
>> -    $ sz -vv loader.bin
>> -    Sending: loader.bin
>> -    Bytes Sent:2478208   BPS:72937
>> -    Sending:
>> -    Ymodem sectors/kbytes sent:   0/ 0k
>> -    Transfer complete
>> -
>> -    *** exit status: 0 ***
>> -    ## Total Size      = 0x0025d052 = 2478162 Bytes
>> -    ## Switch baudrate to 115200 bps and press ESC ...
>> -
>> -    *** baud: 115200
>> -
>> -    *** baud: 115200 ***
>> -    =>
>> +    Hit any key to stop autoboot:  0
>> +    SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
>> +    Reading 5242880 byte(s) at offset 0x00000000
>> +    ## Starting application at 0x80000000 ...
>>
>>  Running Programs
>>  ^^^^^^^^^^^^^^^^
>> @@ -163,6 +135,62 @@ To run legacy images, use the ``bootm`` command:
>>      argv[0] = "<NULL>"
>>      Hit any key to exit ...
>>
>> +Flashing Images
>> +---------------
>> +
>> +To flash a kernel, transfer it over serial, then write it to the kernel
>> +partition.
>> +
>> +.. code-block:: none
>> +
>> +    => loady 80000000 1500000
>> +    ## Switch baudrate to 1500000 bps and press ENTER ...
>> +
>> +    *** baud: 1500000
>> +
>> +    *** baud: 1500000 ***
>> +    ## Ready for binary (ymodem) download to 0x80000000 at 1500000 bps...
>> +    C
>> +    *** file: loader.bin
>> +    $ sz -vv loader.bin
>> +    Sending: loader.bin
>> +    Bytes Sent:2478208   BPS:72937
>> +    Sending:
>> +    Ymodem sectors/kbytes sent:   0/ 0k
>> +    Transfer complete
>> +
>> +    *** exit status: 0 ***
>> +    ## Total Size      = 0x0025d052 = 2478162 Bytes
>> +    ## Switch baudrate to 115200 bps and press ESC ...
>> +
>> +    *** baud: 115200
>> +
>> +    *** baud: 115200 ***
>> +    => sf probe
>> +    SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
>> +    => mtd write kernel 80000000 0 25d052
>> +    Writing 2478162 byte(s) at offset 0x00000000
>> +
>> +Partition Scheme
>> +^^^^^^^^^^^^^^^^
>> +
>> +There is no partition scheme specified by the manufacturer. The only requirement
>> +imposed by the firmware is that offset 0 will be loaded and ran. The default
>> +partition scheme is
>> +
>> +========= ======== ======
>> +Partition Offset   Size
>> +========= ======== ======
>> +u-boot    0x000000 496k
>> +env       0x07C000 16k
>> +kernel    0x080000 5M
>> +data      0x580000 10.5M
>> +========= ======== ======
>> +
>> +**NB:** kflash adds a 5-byte header to payloads (and a 32-byte trailer) to all
>> +payloads it flashes. If you use kflash to flash your payload, you will need to
>> +account for this header when specifying what offset in spi flash to load from.
>> +
>>  Pin Assignment
>>  --------------
>>
>>
> 

[PATCH v2 08/10] spi: dw: Add mem_ops

[Sean Anderson]

On 8/7/20 10:43 AM, Sean Anderson wrote:
> +/* The size of ctrl1 limits data transfers to 64K */
> +static int dw_spi_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op)
> +{
> +	op->data.nbytes = min(op->data.nbytes, (unsigned int)SZ_64K);

This isn't defined for SoCFPGAs [1], so I'm using 0x10000 in the next
revision. Other than that CI is passing.

[1] https://dev.azure.com/seanga2/u-boot/_build/results?buildId=14&view=logs&j=673723c4-497f-506b-4750-995fcb7877b8&t=b5278753-2bf3-55ba-a142-eaf3481f5b3e&l=685

--Sean

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Heinrich Schuchardt]

On 8/8/20 1:07 PM, Sean Anderson wrote:
> On 8/8/20 1:14 AM, Heinrich Schuchardt wrote:
>> On 8/7/20 4:43 PM, Sean Anderson wrote:
>>> This allows different log levels to be enabled or disabled depending on the
>>> desired level of verbosity. In particular, it allows for general debug
>>> information to be printed while excluding more verbose logging which may
>>> interfere with timing.
>>>
>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>> ---/
>>>
>>> Changes in v2:
>>> - New
>>>
>>>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>>> index c9b14f9029..f7ea3edaab 100644
>>> --- a/drivers/spi/designware_spi.c
>>> +++ b/drivers/spi/designware_spi.c
>>> @@ -9,6 +9,7 @@
>>>   * Copyright (c) 2009, Intel Corporation.
>>>   */
>>>
>>> +#define LOG_CATEGORY UCLASS_SPI
>>>  #include <common.h>
>>>  #include <log.h>
>>>  #include <asm-generic/gpio.h>
>>> @@ -139,7 +140,7 @@ static int request_gpio_cs(struct udevice *bus)
>>>  		return 0;
>>>
>>>  	if (ret < 0) {
>>> -		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
>>> +		log_err("Error: %d: Can't get %s gpio!\n", ret, bus->name);
>>
>> Thanks for caring to convert this to the more flexible logging.
>
> It was quite difficult to debug without these changes, since turning on
> debug statements caused the fifos to over/underflow.
>
>>
>> 	Error: -23: Can't get spi at 53000000 gpio!
>>
>> Shouldn't we remove one colon:
>>
>> 	Error -23: Can't get spi at 53000000 gpio!
>
> Sure.
>
>>>  		return ret;
>>>  	}
>>>
>>> @@ -148,7 +149,7 @@ static int request_gpio_cs(struct udevice *bus)
>>>  				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>>>  	}
>>>
>>> -	debug("%s: used external gpio for CS management\n", __func__);
>>> +	log_info("Using external gpio for CS management\n");
>>
>> On systems with CONFIG_LOG=n log_info() messages are always printed. By
>> default the function name is not printed in log messages.
>>
>> Showing this message to the end-user of the device on every boot
>> provides no benefit.
>>
>> log_debug() seems more adequate.
>
> Ok. I was generally unsure what log level to default to. README.log
> doesn't really make it clear semantically which log levels should be
> used.
>
>>>  #endif
>>>  	return 0;
>>>  }
>>> @@ -162,8 +163,7 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
>>>  	/* Use 500KHz as a suitable default */
>>>  	plat->frequency = dev_read_u32_default(bus, "spi-max-frequency",
>>>  					       500000);
>>> -	debug("%s: regs=%p max-frequency=%d\n", __func__, plat->regs,
>>> -	      plat->frequency);
>>> +	log_info("regs=%p max-frequency=%d\n", plat->regs, plat->frequency);
>>
>> The output will look like this:
>>
>> 	regs=0x1234 max-frequency=2000000
>>
>> Such a message appearing on every boot will be irritating for end-users.
>>
>> Please, use log_debug() here.
>
> Ok.
>
>> Can we replace 'regs=' by 'SPI@' for all messages?
>
> Sure.
>
>>>
>>>  	return request_gpio_cs(bus);
>>>  }
>>> @@ -196,7 +196,7 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>>>  		priv->fifo_len = (fifo == 1) ? 0 : fifo;
>>>  		dw_write(priv, DW_SPI_TXFLTR, 0);
>>>  	}
>>> -	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
>>> +	log_debug("fifo_len=%d\n", priv->fifo_len);
>>>  }
>>>
>>>  /*
>>> @@ -221,8 +221,7 @@ __weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
>>>  	if (!*rate)
>>>  		goto err_rate;
>>>
>>> -	debug("%s: get spi controller clk via device tree: %lu Hz\n",
>>> -	      __func__, *rate);
>>> +	log_debug("Got spi controller clk via device tree: %lu Hz\n", *rate);
>>
>> %s/spi/SPI/
>
> Ok.
>
>>>
>>>  	return 0;
>>>
>>> @@ -247,14 +246,14 @@ static int dw_spi_reset(struct udevice *bus)
>>>  		if (ret == -ENOENT || ret == -ENOTSUPP)
>>>  			return 0;
>>>
>>> -		dev_warn(bus, "Can't get reset: %d\n", ret);
>>> +		log_warning("Can't get reset: %d\n", ret);
>>
>> This message does not tell me that there is a problem with SPI. Please,
>> provide a useful text.
>
> Perhaps "Couldn't find or assert reset device configured for SPI"?

or

"Failed to find reset for SPI"

if you want to keep it short.

>
>>>  		return ret;
>>>  	}
>>>
>>>  	ret = reset_deassert_bulk(&priv->resets);
>>>  	if (ret) {
>>>  		reset_release_bulk(&priv->resets);
>>> -		dev_err(bus, "Failed to reset: %d\n", ret);
>>> +		log_err("Failed to reset: %d\n", ret);
>>
>> What shall I do when reading a message like:
>>
>> 	"Failed to reset: -23".
>>
>> Please, provide a more informative text.
>
> Perhaps "SPI: failed to deassert reset (error %d)"?

or

Failed to reset SPI (err %d)

to keep it short.

>
>>>  		return ret;
>>>  	}
>>>
>>> @@ -333,7 +332,7 @@ static void dw_writer(struct dw_spi_priv *priv)
>>>  				txw = *(u16 *)(priv->tx);
>>>  		}
>>>  		dw_write(priv, DW_SPI_DR, txw);
>>> -		debug("%s: tx=0x%02x\n", __func__, txw);
>>> +		log_content("tx=0x%02x\n", txw);
>>>  		priv->tx += priv->bits_per_word >> 3;
>>>  	}
>>>  }
>>> @@ -345,7 +344,7 @@ static void dw_reader(struct dw_spi_priv *priv)
>>>
>>>  	while (max--) {
>>>  		rxw = dw_read(priv, DW_SPI_DR);
>>> -		debug("%s: rx=0x%02x\n", __func__, rxw);
>>> +		log_content("rx=0x%02x\n", rxw);
>>>
>>>  		/* Care about rx if the transfer's original "rx" is not null */
>>>  		if (priv->rx_end - priv->len) {
>>> @@ -400,7 +399,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>>
>>>  	/* spi core configured to do 8 bit transfers */
>>>  	if (bitlen % 8) {
>>> -		debug("Non byte aligned SPI transfer.\n");
>>> +		log_err("Non byte aligned SPI transfer.\n");
>>>  		return -1;
>>>  	}
>>>
>>> @@ -427,7 +426,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>>  	cr0 |= (priv->tmode << SPI_TMOD_OFFSET);
>>>
>>>  	priv->len = bitlen >> 3;
>>> -	debug("%s: rx=%p tx=%p len=%d [bytes]\n", __func__, rx, tx, priv->len);
>>> +	log_debug("rx=%p tx=%p len=%d [bytes]\n", rx, tx, priv->len);
>>
>> Please, use log_debug_io() here.
>
> When should log_content/log_io be used? Here, we only log metadata about
> the transfer. I would expect this to be a higher log level than (for
> example) the actual log of the content as done in dw_reader/writer. In
> addition, I am confused as to when log_io should be used over
> log_content. The only clear-cut case I can see is perhaps that replacing
> dw_write with something like
>

When debugging you can easily be overwhelmed by thousands of messages
when a message is written per sector read from a device.

So I think log_debug() is fine for messages occurring only once during
initialization. But for messages that would otherwise flood the screen
log_content() or log_io() should be used. Cf. drivers/mtd/spi/sandbox.c.

LOGL_DEBUG,???????? /* Basic debug-level message */
LOGL_DEBUG_CONTENT, /* Debug message showing full message content */
LOGL_DEBUG_IO,????? /* Debug message showing hardware I/O access */

Sounds like log_content() would be fine if written once per read.

Best regards

Heinrich

> static inline void dw_write(struct dw_spi_priv *priv, u32 offset, u32 val)
> {
> 	log_io("%x = %x", offset, val);
> 	__raw_writel(val, priv->regs + offset);
> }
>
> Is log_content intended for higher-level interfaces?
>
>>>
>>>  	priv->tx = (void *)tx;
>>>  	priv->tx_end = priv->tx + priv->len;
>>> @@ -437,7 +436,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>>  	/* Disable controller before writing control registers */
>>>  	spi_enable_chip(priv, 0);
>>>
>>> -	debug("%s: cr0=%08x\n", __func__, cr0);
>>> +	log_debug("cr0=%08x\n", cr0);
>>
>> log_debug_io()
>
> Same comments/questions as above.
>
> --Sean
>

[PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

[Eugeniy Paltsev]

Hi Sean,

do you have any public git branch with this patch series?
I want to test these changes on our board with DW SPI controller.

Thanks.
---
 Eugeniy Paltsev


________________________________________
From: Sean Anderson <seanga2@gmail.com>
Sent: Friday, August 7, 2020 17:43
To: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
Cc: Heinrich Schuchardt; Jagan Teki; Eugeniy Paltsev; Horatiu Vultur; Marek Vasut; Sean Anderson; Alexey Brodkin; Alexey Brodkin; Daniel Schwierzeck; Gregory CLEMENT; Lars Povlsen; Ley Foon Tan; Rick Chen; Simon Goldschmidt
Subject: [PATCH v2 00/10] riscv: Add SPI support for Kendryte K210


This series adds support for SPI on the Kendryte K210. This covers the MMC
slot (currently broken) and SPI flash on the Sipeed Maix Bit.

This series makes significant changes to the designware SPI driver. I would
really appreciate if the maintainers I CC'd could test this series and
ensure that SPI still works on all their devices. I have tried my best not
to affect existing devices, but I'd rather find out if this breaks stuff
now rather than later.

In particular:

Ley Foon Tan, can you try using dw_spi_dw32_init with the Stratix 10 and
Agilex SoCFPGAs? From their documentation, it is unclear whether one should
use the DFS or DFS_32 field. I have used dw_spi_dw16_init for these
devices, since that keeps the same behavior as before this series, but I am
interested in seeing whether the DFS_32 works.

Gregory Clement, Lars Povlsen, and Horatiu Vultur, can you confirm the
register layout of ctrlr0 and version of the device on the MSCC Ocelot and
Jaguar 2? I couldn't find any documentation for the device on those SoCs
online.

Alexey Brodkin and Eugeniy Paltsev, can you confirm the register layout of
ctrlr0 and version of the device on ARC SoCs? I couldn't find any
documentation for the device on those SoCs online. In addition, can you
clarify the nature of SSI_MAX_XFER_SIZE? Is it set once before the device
is fabricated, or is it set at runtime? Is there any way to detect it at
runtime? Is my supposition that it was introduced in version 3.23 correct?
Have there been any other breaking changes which I have overlooked? I tried
investigating some of this, but I was unable to find any definitive ruling
on the matter. All I could find was this forum post which doesn't really
explain anything [1].

[1] https://urldefense.com/v3/__https://community.intel.com/t5/Intel-Makers/D2000-SPI-word-frame-size/td-p/276520__;!!A4F2R9G_pg!PEqqmGgI-XfH0K6kaLF8oIuYYMfBugTtH4ysThH7wyY4Q01OMi_rGRiRi2hs0S7bdAf8nWI$

This series was previously part of
https://urldefense.com/v3/__https://patchwork.ozlabs.org/project/uboot/list/?series=161576__;!!A4F2R9G_pg!PEqqmGgI-XfH0K6kaLF8oIuYYMfBugTtH4ysThH7wyY4Q01OMi_rGRiRi2hs0S7bo-OeOVc$

This series depends on
https://urldefense.com/v3/__https://patchwork.ozlabs.org/project/uboot/list/?series=185489__;!!A4F2R9G_pg!PEqqmGgI-XfH0K6kaLF8oIuYYMfBugTtH4ysThH7wyY4Q01OMi_rGRiRi2hs0S7b478gncg$

Known bugs:
- The MMC cannot be accessed with the dw_spi driver

Changes in v2:
- Add Gigadevice SPI chips to dependencies
- Add external gpio cs support
- Clean up exec_op
- Configure ctrlr0 register layout based on compatible string
- Convert debug calls to log_ instead of removing the ones which affect
  timing
- Document new compatible strings
- Limit data transfers to 64k
- Remove broken-wp property (implicit due to no wp gpio)
- Remove ctrl0 field offsets from device tree
- Switch to new compatible strings
- Switch to new pinmux binding style

Sean Anderson (10):
  spi: dw: Convert calls to debug to log_*
  spi: dw: Rename "cs-gpio" to "cs-gpios"
  spi: dw: Use generic function to read reg address
  spi: dw: Rearrange struct dw_spi_priv
  spi: dw: Add SoC-specific compatible strings
  spi: dw: Configure ctrlr0 layout based on compatible string
  spi: dw: Document devicetree binding
  spi: dw: Add mem_ops
  riscv: Add device tree bindings for SPI
  riscv: Add support for SPI on Kendryte K210

 arch/arc/dts/axs10x_mb.dtsi                   |   5 +-
 arch/arc/dts/hsdk-common.dtsi                 |   5 +-
 arch/arm/dts/socfpga.dtsi                     |   6 +-
 arch/arm/dts/socfpga_agilex.dtsi              |   6 +-
 arch/arm/dts/socfpga_arria10.dtsi             |   6 +-
 arch/arm/dts/socfpga_stratix10.dtsi           |   6 +-
 arch/mips/dts/mscc,jr2.dtsi                   |   2 +-
 arch/mips/dts/mscc,ocelot.dtsi                |   2 +-
 arch/riscv/dts/k210-maix-bit.dts              |  46 ++-
 arch/riscv/dts/k210.dtsi                      |  15 +-
 board/sipeed/maix/Kconfig                     |  11 +
 configs/sipeed_maix_bitm_defconfig            |   8 +
 doc/board/sipeed/maix.rst                     |  94 +++--
 .../spi/snps,dw-apb-ssi.txt                   |  56 +++
 drivers/spi/designware_spi.c                  | 381 ++++++++++++++----
 15 files changed, 528 insertions(+), 121 deletions(-)
 create mode 100644 doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt

--
2.28.0

[PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

[Sean Anderson]

On 8/10/20 6:49 AM, Eugeniy Paltsev wrote:
> Hi Sean,
> 
> do you have any public git branch with this patch series?
> I want to test these changes on our board with DW SPI controller.

https://github.com/Forty-Bot/u-boot/tree/maix_spi

--Sean

[PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

[Eugeniy Paltsev]

FYI: I've tested on commit aa68b00a8259aa026591475f21a5c51311252ef2 (current branch head) and I don't see any build/runtime issues.

Tested-by Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>

---
 Eugeniy Paltsev


________________________________________
From: Sean Anderson <seanga2@gmail.com>
Sent: Monday, August 10, 2020 14:13
To: Eugeniy Paltsev; u-boot at lists.denx.de; uboot-snps-arc at synopsys.com
Cc: Heinrich Schuchardt; Jagan Teki; Horatiu Vultur; Marek Vasut; Alexey Brodkin; Daniel Schwierzeck; Gregory CLEMENT; Lars Povlsen; Ley Foon Tan; Rick Chen; Simon Goldschmidt
Subject: Re: [PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

On 8/10/20 6:49 AM, Eugeniy Paltsev wrote:
> Hi Sean,
>
> do you have any public git branch with this patch series?
> I want to test these changes on our board with DW SPI controller.

https://urldefense.com/v3/__https://github.com/Forty-Bot/u-boot/tree/maix_spi__;!!A4F2R9G_pg!JnihRPoojBUo2j8mMgJNH_K5dis6o-g7c2YGmb0SYh2wBkWGASlPXxMFW-TPbdSOtlYUiTs$

--Sean

[PATCH v2 09/10] riscv: Add device tree bindings for SPI

[Rick Chen]

> This patch adds bindings for the MMC slot and SPI flash on the Sipeed Maix
> Bit.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> This patch was previously part of
> https://patchwork.ozlabs.org/project/uboot/list/?series=161576
>
> Changes in v2:
> - Remove broken-wp property (implicit due to no wp gpio)
> - Remove ctrl0 field offsets from device tree
> - Switch to new compatible strings
> - Switch to new pinmux binding style
>
>  arch/riscv/dts/k210-maix-bit.dts | 46 +++++++++++++++++++++++++++++++-
>  arch/riscv/dts/k210.dtsi         |  2 ++
>  2 files changed, 47 insertions(+), 1 deletion(-)

Acked-by: Rick Chen <rick@andestech.com>

I am OK that this patch can be pulled via SPI tree.

Thanks,
Rick

>
> diff --git a/arch/riscv/dts/k210-maix-bit.dts b/arch/riscv/dts/k210-maix-bit.dts
> index e840e04ada..73892c6450 100644
> --- a/arch/riscv/dts/k210-maix-bit.dts
> +++ b/arch/riscv/dts/k210-maix-bit.dts
> @@ -141,7 +141,7 @@
>                 pinmux = <K210_FPIOA(26, K210_PCF_SPI1_D1)>,
>                          <K210_FPIOA(27, K210_PCF_SPI1_SCLK)>,
>                          <K210_FPIOA(28, K210_PCF_SPI1_D0)>,
> -                        <K210_FPIOA(29, K210_PCF_GPIOHS13)>;
> +                        <K210_FPIOA(29, K210_PCF_GPIOHS13)>; /* cs */
>         };
>  };
>
> @@ -149,3 +149,47 @@
>         pinctrl-0 = <&fpioa_dvp>;
>         pinctrl-names = "default";
>  };
> +
> +&spi0 {
> +       pinctrl-0 = <&fpioa_spi0>;
> +       pinctrl-names = "default";
> +       num-cs = <1>;
> +       cs-gpios = <&gpio0 20 0>;
> +
> +       panel at 0 {
> +               compatible = "sitronix,st7789v";
> +               reg = <0>;
> +               reset-gpios = <&gpio0 21 GPIO_ACTIVE_LOW>;
> +               dc-gpios = <&gpio0 22 0>;
> +               spi-max-frequency = <15000000>;
> +               status = "disabled";
> +       };
> +};
> +
> +&spi1 {
> +       pinctrl-0 = <&fpioa_spi1>;
> +       pinctrl-names = "default";
> +       num-cs = <1>;
> +       cs-gpios = <&gpio0 13 0>;
> +       status = "okay";
> +
> +       slot at 0 {
> +               compatible = "mmc-spi-slot";
> +               reg = <0>;
> +               spi-max-frequency = <25000000>;
> +               voltage-ranges = <3300 3300>;
> +               broken-cd;
> +       };
> +};
> +
> +&spi3 {
> +       status = "okay";
> +
> +       spi-flash at 0 {
> +               compatible = "jedec,spi-nor";
> +               reg = <0>;
> +               spi-max-frequency = <50000000>;
> +               m25p,fast-read;
> +               broken-flash-reset;
> +       };
> +};
> diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
> index 429891d651..fe47ee7aaf 100644
> --- a/arch/riscv/dts/k210.dtsi
> +++ b/arch/riscv/dts/k210.dtsi
> @@ -495,6 +495,8 @@
>                                 interrupts = <24>;
>                                 clocks = <&sysclk K210_CLK_DVP>;
>                                 resets = <&sysrst K210_RST_DVP>;
> +                               kendryte,sysctl = <&sysctl>;
> +                               kendryte,misc-offset = <K210_SYSCTL_MISC>;
>                                 status = "disabled";
>                         };
>
> --
> 2.28.0
>

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Simon Glass]

On Fri, 7 Aug 2020 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>
> This allows different log levels to be enabled or disabled depending on the
> desired level of verbosity. In particular, it allows for general debug
> information to be printed while excluding more verbose logging which may
> interfere with timing.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - New
>
>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Marek Vasut]

On 8/20/20 3:26 PM, Simon Glass wrote:
> On Fri, 7 Aug 2020 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> This allows different log levels to be enabled or disabled depending on the
>> desired level of verbosity. In particular, it allows for general debug
>> information to be printed while excluding more verbose logging which may
>> interfere with timing.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> Changes in v2:
>> - New
>>
>>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

NAK, please use dev_dbg() instead.

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Tom Rini]

On Thu, Aug 20, 2020 at 03:36:31PM +0200, Marek Vasut wrote:
> On 8/20/20 3:26 PM, Simon Glass wrote:
> > On Fri, 7 Aug 2020 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> This allows different log levels to be enabled or disabled depending on the
> >> desired level of verbosity. In particular, it allows for general debug
> >> information to be printed while excluding more verbose logging which may
> >> interfere with timing.
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> ---
> >>
> >> Changes in v2:
> >> - New
> >>
> >>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
> >>  1 file changed, 16 insertions(+), 17 deletions(-)
> >>
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> NAK, please use dev_dbg() instead.

For the record, Marek and I (and Sean) had talked a bit on IRC.  These
messages being log_xxx() and not dev_xx() instead make the more
functionally useful as dev_xxx() converts to printf() and messes with
the timing.  log_xxx() puts them in the buffer and will not break the
timing.  Marek objects to the fundamentals that dev_xxx() does not
function like it does in Linux (and please correct me if you feel I'm
mis-representing your views).  This could be solved by plumbing
dev_xxx() to use log_xxx() if LOG is enabled automagically.  I don't see
transitions like that as blocking for this patch, Marek does.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200820/1eab0366/attachment.sig>

[PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

[Marek Vasut]

On 8/21/20 2:08 AM, Tom Rini wrote:
> On Thu, Aug 20, 2020 at 03:36:31PM +0200, Marek Vasut wrote:
>> On 8/20/20 3:26 PM, Simon Glass wrote:
>>> On Fri, 7 Aug 2020 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> This allows different log levels to be enabled or disabled depending on the
>>>> desired level of verbosity. In particular, it allows for general debug
>>>> information to be printed while excluding more verbose logging which may
>>>> interfere with timing.
>>>>
>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - New
>>>>
>>>>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>>>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>>>
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> NAK, please use dev_dbg() instead.
> 
> For the record, Marek and I (and Sean) had talked a bit on IRC.  These
> messages being log_xxx() and not dev_xx() instead make the more
> functionally useful as dev_xxx() converts to printf() and messes with
> the timing.

The ones which were printf() before are not impacted, since nothing
changes there. The ones which were debug() were compiled out anyway or
turned into printf(), so nothing changes there either.

In this case , the timing argument simply does not apply.

> log_xxx() puts them in the buffer and will not break the
> timing.  Marek objects to the fundamentals that dev_xxx() does not
> function like it does in Linux (and please correct me if you feel I'm
> mis-representing your views).

I do NOT object to the way dev_err() works, see below.

> This could be solved by plumbing
> dev_xxx() to use log_xxx() if LOG is enabled automagically.  I don't see
> transitions like that as blocking for this patch, Marek does.

My objection is to yet another new logging facility, which is
incompatible with any of the Linux ones. Both pr_() and dev_() we
already have in U-Boot, and there is already ongoing conversion attempt
to move drivers to dev_(), but now another log_() one appears out of
nowhere and starts adding to the already bad mess. Moreover, there is
zero information or guideline on which logging primitive to use where.

It is likely others are used to those Linux logging facilities, because
embedded systems developers likely develop both U-Boot and Linux, likely
at the same time. So making the U-Boot more and more incompatible does
not help at all.

What I do propose is to put the new logging facility underneath the
existing logging primitives -- pr_() and dev_() -- if applicable and
make the dynamic logging configurable. That way the current mess will be
cleaned up. The use of log_() should then only be limited to where no
other logging primitives apply.