[PATCH linux v2 0/3] spi: aspeed: Add a "ranges" property

[PATCH linux v2 0/3] spi: aspeed: Add a "ranges" property

[Cédric Le Goater]

Hello,

Currently, the Linux Aspeed SMC driver computes the decoding ranges of
each CS (AHB memory window on which the flash contents are mapped)
from the size of the detected flash device. It seems that some chips
have issues with the computed ranges and for these, it would be nice
to be able to define custom decoding ranges in the DT.

Here is a little series doing that. 

Thanks,

C. 

Changes in v2 :

 - Tested by Naresh Solanki
 - sent preliminary fix independently
   https://patchwork.kernel.org/project/linux-arm-kernel/patch/20221016155722.3520802-1-clg@kaod.org/
 - changed the sysfs file exposing the register values to debugfs.
 - refresh on 6.1-rc1

Cédric Le Goater (3):
  spi: dt-bindings: aspeed: Add a ranges property
  spi: aspeed: Handle custom decoding ranges
  spi: aspeed: Introduce a "ranges" debugfs file

 drivers/spi/spi-aspeed-smc.c                  | 131 +++++++++++++++++-
 .../bindings/spi/aspeed,ast2600-fmc.yaml      |   9 ++
 2 files changed, 138 insertions(+), 2 deletions(-)

-- 
2.37.3

[PATCH linux v2 1/3] spi: dt-bindings: aspeed: Add a ranges property

[Cédric Le Goater]

"ranges" predefines settings for the decoding ranges of each CS.

Cc: Naresh Solanki <naresh.solanki@9elements.com>
Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 .../devicetree/bindings/spi/aspeed,ast2600-fmc.yaml      | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml b/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
index fa8f4ac20985..a11cbc4c4c5c 100644
--- a/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
+++ b/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
@@ -38,6 +38,14 @@ properties:
   interrupts:
     maxItems: 1
 
+  ranges:
+    minItems: 1
+    maxItems: 5
+    description: |
+      Defines the address mapping for child devices with four integer
+      values for each chip-select line in use:
+      <cs-number> 0 <physical address of mapping> <size>
+
 required:
   - compatible
   - reg
@@ -58,6 +66,7 @@ examples:
         compatible = "aspeed,ast2600-fmc";
         clocks = <&syscon ASPEED_CLK_AHB>;
         interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+        ranges = <0 0 0x20000000 0x2000000>, <1 0 0x22000000 0x2000000>;
 
         flash@0 {
                 reg = < 0 >;
-- 
2.37.3

[PATCH linux v2 2/3] spi: aspeed: Handle custom decoding ranges

[Cédric Le Goater]

The "ranges" property predefines settings for the decoding ranges of
each CS. If found in the DT, the driver applies the settings at probe
time. The default behavior is to set the decoding range of each CS
using the flash device size when the spi slave is setup.

Cc: Naresh Solanki <naresh.solanki@9elements.com>
Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/spi/spi-aspeed-smc.c | 65 +++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
index b90571396a60..75e1d08bbd00 100644
--- a/drivers/spi/spi-aspeed-smc.c
+++ b/drivers/spi/spi-aspeed-smc.c
@@ -96,6 +96,7 @@ struct aspeed_spi {
 	u32			 ahb_base_phy;
 	u32			 ahb_window_size;
 	struct device		*dev;
+	bool                     fixed_windows;
 
 	struct clk		*clk;
 	u32			 clk_freq;
@@ -382,6 +383,7 @@ static const char *aspeed_spi_get_name(struct spi_mem *mem)
 
 struct aspeed_spi_window {
 	u32 cs;
+	u32 reg;
 	u32 offset;
 	u32 size;
 };
@@ -396,6 +398,7 @@ static void aspeed_spi_get_windows(struct aspeed_spi *aspi,
 	for (cs = 0; cs < aspi->data->max_cs; cs++) {
 		reg_val = readl(aspi->regs + CE0_SEGMENT_ADDR_REG + cs * 4);
 		windows[cs].cs = cs;
+		windows[cs].reg = reg_val;
 		windows[cs].size = data->segment_end(aspi, reg_val) -
 			data->segment_start(aspi, reg_val);
 		windows[cs].offset = data->segment_start(aspi, reg_val) - aspi->ahb_base_phy;
@@ -572,7 +575,8 @@ static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
 	if (op->data.dir != SPI_MEM_DATA_IN)
 		return -EOPNOTSUPP;
 
-	aspeed_spi_chip_adjust_window(chip, desc->info.offset, desc->info.length);
+	if (!aspi->fixed_windows)
+		aspeed_spi_chip_adjust_window(chip, desc->info.offset, desc->info.length);
 
 	if (desc->info.length > chip->ahb_window_size)
 		dev_warn(aspi->dev, "CE%d window (%dMB) too small for mapping",
@@ -712,6 +716,61 @@ static void aspeed_spi_enable(struct aspeed_spi *aspi, bool enable)
 		aspeed_spi_chip_enable(aspi, cs, enable);
 }
 
+static int aspeed_spi_chip_read_ranges(struct device_node *node, struct aspeed_spi *aspi)
+{
+	const char *range_prop = "ranges";
+	struct property *prop;
+	struct aspeed_spi_window ranges[ASPEED_SPI_MAX_NUM_CS];
+	int prop_size;
+	int count;
+	int ret;
+	int i;
+
+	prop = of_find_property(node, range_prop, &prop_size);
+	if (!prop)
+		return 0;
+
+	count = prop_size / sizeof(*ranges);
+	if (count > aspi->data->max_cs) {
+		dev_err(aspi->dev, "invalid '%s' property %d\n", range_prop, count);
+		return -EINVAL;
+	}
+
+	if (count < aspi->data->max_cs)
+		dev_dbg(aspi->dev, "'%s' property does not cover all CE\n",
+			range_prop);
+
+	ret = of_property_read_u32_array(node, range_prop, (u32 *)ranges, count * 4);
+	if (ret)
+		return ret;
+
+	dev_info(aspi->dev, "Using preset decoding ranges\n");
+	for (i = 0; i < count; i++) {
+		struct aspeed_spi_window *win = &ranges[i];
+
+		if (win->cs > aspi->data->max_cs) {
+			dev_err(aspi->dev, "CE%d range is invalid", win->cs);
+			return -EINVAL;
+		}
+
+		/* Trim top bit of the address to keep offset */
+		win->offset &= aspi->ahb_window_size - 1;
+
+		/* Minimal check */
+		if (win->offset + win->size > aspi->ahb_window_size) {
+			dev_warn(aspi->dev, "CE%d range is too large", win->cs);
+				return -EINVAL;
+		}
+
+		ret = aspeed_spi_set_window(aspi, win);
+		if (ret)
+			return ret;
+	}
+
+	aspi->fixed_windows = true;
+	return 0;
+}
+
 static int aspeed_spi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -767,6 +826,10 @@ static int aspeed_spi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = aspeed_spi_chip_read_ranges(dev->of_node, aspi);
+	if (ret)
+		return ret;
+
 	/* IRQ is for DMA, which the driver doesn't support yet */
 
 	ctlr->mode_bits = SPI_RX_DUAL | SPI_TX_DUAL | data->mode_bits;
-- 
2.37.3

[PATCH linux v2 3/3] spi: aspeed: Introduce a "ranges" debugfs file

[Cédric Le Goater]

This dumps the mapping windows, or decoding ranges, of all devices
possibly attached of the controller. To be noted that a top level
"spi" debugfs directory is created to hold the intermediate directory
of the driver instance.

Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/spi/spi-aspeed-smc.c | 66 +++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
index 75e1d08bbd00..a79e5cc8ac5b 100644
--- a/drivers/spi/spi-aspeed-smc.c
+++ b/drivers/spi/spi-aspeed-smc.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -102,6 +103,9 @@ struct aspeed_spi {
 	u32			 clk_freq;
 
 	struct aspeed_spi_chip	 chips[ASPEED_SPI_MAX_NUM_CS];
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	struct dentry           *debugfs;
+#endif
 };
 
 static u32 aspeed_spi_get_io_mode(const struct spi_mem_op *op)
@@ -716,6 +720,65 @@ static void aspeed_spi_enable(struct aspeed_spi *aspi, bool enable)
 		aspeed_spi_chip_enable(aspi, cs, enable);
 }
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int aspeed_spi_ranges_debug_show(struct seq_file *m, void *private)
+{
+	struct aspeed_spi *aspi = m->private;
+	struct aspeed_spi_window windows[ASPEED_SPI_MAX_NUM_CS] = { 0 };
+	u32 cs;
+
+	if (aspi->data == &ast2400_spi_data)
+		return 0;
+
+	aspeed_spi_get_windows(aspi, windows);
+
+	seq_puts(m, "     offset     size       register\n");
+	for (cs = 0; cs < aspi->data->max_cs; cs++) {
+		if (!windows[cs].reg)
+			seq_printf(m, "CE%d: disabled\n", cs);
+		else
+			seq_printf(m, "CE%d: 0x%.8x 0x%.8x 0x%x\n", cs,
+				   windows[cs].offset, windows[cs].size,
+				   windows[cs].reg);
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(aspeed_spi_ranges_debug);
+
+static int aspeed_spi_debugfs_init(struct spi_controller *ctlr)
+{
+	struct aspeed_spi *aspi = spi_controller_get_devdata(ctlr);
+	struct dentry *rootdir = NULL;
+
+	rootdir = debugfs_lookup("spi", NULL);
+	if (!rootdir)
+		rootdir = debugfs_create_dir("spi", NULL);
+
+	aspi->debugfs = debugfs_create_dir(dev_name(&ctlr->dev), rootdir);
+	if (!aspi->debugfs)
+		return -ENOMEM;
+
+	debugfs_create_file("ranges", 0444, aspi->debugfs, (void *)aspi,
+			    &aspeed_spi_ranges_debug_fops);
+	return 0;
+}
+
+static void aspeed_spi_debugfs_remove(struct aspeed_spi *aspi)
+{
+	debugfs_remove_recursive(aspi->debugfs);
+}
+
+#else
+static inline int aspeed_spi_debugfs_init(struct spi_controller *ctlr)
+{
+	return 0;
+}
+
+static inline void aspeed_spi_debugfs_remove(struct aspeed_spi *aspi)
+{
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
 static int aspeed_spi_chip_read_ranges(struct device_node *node, struct aspeed_spi *aspi)
 {
 	const char *range_prop = "ranges";
@@ -845,7 +908,7 @@ static int aspeed_spi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "spi_register_controller failed\n");
 		goto disable_clk;
 	}
-	return 0;
+	return aspeed_spi_debugfs_init(ctlr);
 
 disable_clk:
 	clk_disable_unprepare(aspi->clk);
@@ -856,6 +919,7 @@ static int aspeed_spi_remove(struct platform_device *pdev)
 {
 	struct aspeed_spi *aspi = platform_get_drvdata(pdev);
 
+	aspeed_spi_debugfs_remove(aspi);
 	aspeed_spi_enable(aspi, false);
 	clk_disable_unprepare(aspi->clk);
 	return 0;
-- 
2.37.3

Re: [PATCH linux v2 1/3] spi: dt-bindings: aspeed: Add a ranges property

[Rob Herring]

On Mon, Oct 17, 2022 at 11:16:22AM +0200, Cédric Le Goater wrote:
> "ranges" predefines settings for the decoding ranges of each CS.

Please explain the problem, not what the change is.

> Cc: Naresh Solanki <naresh.solanki@9elements.com>
> Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  .../devicetree/bindings/spi/aspeed,ast2600-fmc.yaml      | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml b/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
> index fa8f4ac20985..a11cbc4c4c5c 100644
> --- a/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
> +++ b/Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
> @@ -38,6 +38,14 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  ranges:
> +    minItems: 1
> +    maxItems: 5
> +    description: |
> +      Defines the address mapping for child devices with four integer
> +      values for each chip-select line in use:
> +      <cs-number> 0 <physical address of mapping> <size>
> +
>  required:
>    - compatible
>    - reg
> @@ -58,6 +66,7 @@ examples:
>          compatible = "aspeed,ast2600-fmc";
>          clocks = <&syscon ASPEED_CLK_AHB>;
>          interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> +        ranges = <0 0 0x20000000 0x2000000>, <1 0 0x22000000 0x2000000>;

By having ranges here, 'reg' in child nodes become translatable 
addresses. But they are not because they are SPI chip-selects. Only 
memory mapped addresses should be translatable. 

Probably the ranges here should be in 'reg' of the controller.

>  
>          flash@0 {
>                  reg = < 0 >;
> -- 
> 2.37.3
> 
> 

Re: [PATCH linux v2 2/3] spi: aspeed: Handle custom decoding ranges

[Rob Herring]

On Mon, Oct 17, 2022 at 11:16:23AM +0200, Cédric Le Goater wrote:
> The "ranges" property predefines settings for the decoding ranges of
> each CS. If found in the DT, the driver applies the settings at probe
> time. The default behavior is to set the decoding range of each CS
> using the flash device size when the spi slave is setup.
> 
> Cc: Naresh Solanki <naresh.solanki@9elements.com>
> Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  drivers/spi/spi-aspeed-smc.c | 65 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index b90571396a60..75e1d08bbd00 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -96,6 +96,7 @@ struct aspeed_spi {
>  	u32			 ahb_base_phy;
>  	u32			 ahb_window_size;
>  	struct device		*dev;
> +	bool                     fixed_windows;
>  
>  	struct clk		*clk;
>  	u32			 clk_freq;
> @@ -382,6 +383,7 @@ static const char *aspeed_spi_get_name(struct spi_mem *mem)
>  
>  struct aspeed_spi_window {
>  	u32 cs;
> +	u32 reg;
>  	u32 offset;
>  	u32 size;
>  };
> @@ -396,6 +398,7 @@ static void aspeed_spi_get_windows(struct aspeed_spi *aspi,
>  	for (cs = 0; cs < aspi->data->max_cs; cs++) {
>  		reg_val = readl(aspi->regs + CE0_SEGMENT_ADDR_REG + cs * 4);
>  		windows[cs].cs = cs;
> +		windows[cs].reg = reg_val;
>  		windows[cs].size = data->segment_end(aspi, reg_val) -
>  			data->segment_start(aspi, reg_val);
>  		windows[cs].offset = data->segment_start(aspi, reg_val) - aspi->ahb_base_phy;
> @@ -572,7 +575,8 @@ static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
>  	if (op->data.dir != SPI_MEM_DATA_IN)
>  		return -EOPNOTSUPP;
>  
> -	aspeed_spi_chip_adjust_window(chip, desc->info.offset, desc->info.length);
> +	if (!aspi->fixed_windows)
> +		aspeed_spi_chip_adjust_window(chip, desc->info.offset, desc->info.length);
>  
>  	if (desc->info.length > chip->ahb_window_size)
>  		dev_warn(aspi->dev, "CE%d window (%dMB) too small for mapping",
> @@ -712,6 +716,61 @@ static void aspeed_spi_enable(struct aspeed_spi *aspi, bool enable)
>  		aspeed_spi_chip_enable(aspi, cs, enable);
>  }
>  
> +static int aspeed_spi_chip_read_ranges(struct device_node *node, struct aspeed_spi *aspi)
> +{
> +	const char *range_prop = "ranges";
> +	struct property *prop;
> +	struct aspeed_spi_window ranges[ASPEED_SPI_MAX_NUM_CS];
> +	int prop_size;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	prop = of_find_property(node, range_prop, &prop_size);
> +	if (!prop)
> +		return 0;

Parsing common properties yourself is generally a bad sign.

> +
> +	count = prop_size / sizeof(*ranges);
> +	if (count > aspi->data->max_cs) {
> +		dev_err(aspi->dev, "invalid '%s' property %d\n", range_prop, count);
> +		return -EINVAL;
> +	}
> +
> +	if (count < aspi->data->max_cs)
> +		dev_dbg(aspi->dev, "'%s' property does not cover all CE\n",
> +			range_prop);
> +
> +	ret = of_property_read_u32_array(node, range_prop, (u32 *)ranges, count * 4);

You've just gotten it horribly wrong because you ignored #size-cells and 
#address-cells.

Rob