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

[PATCH linux 0/5] 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, including a preliminary fix. Last
patch is a simple test for the AST2500 EVB. Comments welcome.

Thanks,

C. 

Cédric Le Goater (5):
  spi: aspeed: Fix window offset of CE1
  spi: aspeed: Introduce a "windows" device attribute
  spi: dt-bindings: aspeed: Add a ranges property
  spi: aspeed: Handle custom decoding ranges
  ARM: dts: aspeed: ast2500-evb: Add custom decoding ranges (TEST)

 drivers/spi/spi-aspeed-smc.c                  | 108 +++++++++++++++++-
 .../bindings/spi/aspeed,ast2600-fmc.yaml      |   9 ++
 arch/arm/boot/dts/aspeed-ast2500-evb.dts      |   3 +
 3 files changed, 118 insertions(+), 2 deletions(-)

-- 
2.35.3

[PATCH linux 1/5] spi: aspeed: Fix window offset of CE1

[Cédric Le Goater]

The offset value of the mapping window in the kernel structure is
calculated using the value of the previous window offset. This doesn't
reflect how the HW is configured and can lead to erroneous setting of
the second flash device (CE1).

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/spi/spi-aspeed-smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
index 3e891bf22470..5a995b5653f1 100644
--- a/drivers/spi/spi-aspeed-smc.c
+++ b/drivers/spi/spi-aspeed-smc.c
@@ -398,7 +398,7 @@ static void aspeed_spi_get_windows(struct aspeed_spi *aspi,
 		windows[cs].cs = cs;
 		windows[cs].size = data->segment_end(aspi, reg_val) -
 			data->segment_start(aspi, reg_val);
-		windows[cs].offset = cs ? windows[cs - 1].offset + windows[cs - 1].size : 0;
+		windows[cs].offset = data->segment_start(aspi, reg_val) - aspi->ahb_base_phy;
 		dev_vdbg(aspi->dev, "CE%d offset=0x%.8x size=0x%x\n", cs,
 			 windows[cs].offset, windows[cs].size);
 	}
-- 
2.35.3

[PATCH linux 2/5] spi: aspeed: Introduce a "windows" device attribute

[Cédric Le Goater]

This dumps the mapping windows, or decoding ranges, of all devices
possibly attached of the controller.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/spi/spi-aspeed-smc.c | 43 ++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
index 5a995b5653f1..1611c354c31f 100644
--- a/drivers/spi/spi-aspeed-smc.c
+++ b/drivers/spi/spi-aspeed-smc.c
@@ -382,6 +382,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 +397,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;
@@ -712,6 +714,41 @@ static void aspeed_spi_enable(struct aspeed_spi *aspi, bool enable)
 		aspeed_spi_chip_enable(aspi, cs, enable);
 }
 
+static int windows_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct aspeed_spi *aspi = dev_get_drvdata(dev);
+	struct aspeed_spi_window windows[ASPEED_SPI_MAX_NUM_CS] = { 0 };
+	u32 cs;
+	int len = 0;
+
+	if (aspi->data == &ast2400_spi_data)
+		return 0;
+
+	aspeed_spi_get_windows(aspi, windows);
+
+	len += sysfs_emit_at(buf, len, "     offset     size       register\n");
+	for (cs = 0; cs < aspi->data->max_cs; cs++) {
+		if (!windows[cs].reg)
+			len += sysfs_emit_at(buf, len, "CE%d: disabled\n", cs);
+		else
+			len += sysfs_emit_at(buf, len, "CE%d: 0x%.8x 0x%.8x 0x%x\n", cs,
+					     windows[cs].offset, windows[cs].size,
+					     windows[cs].reg);
+	}
+	return len;
+}
+
+static DEVICE_ATTR_RO(windows);
+
+static struct attribute *aspeed_spi_attributes[] = {
+	&dev_attr_windows.attr,
+	NULL,
+};
+
+static const struct attribute_group aspeed_spi_attribute_group = {
+	.attrs = aspeed_spi_attributes
+};
+
 static int aspeed_spi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -784,6 +821,12 @@ static int aspeed_spi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "spi_register_controller failed\n");
 		goto disable_clk;
 	}
+
+	ret = devm_device_add_group(&pdev->dev, &aspeed_spi_attribute_group);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot create attribute group\n");
+		goto disable_clk;
+	}
 	return 0;
 
 disable_clk:
-- 
2.35.3

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

[Cédric Le Goater]

"ranges" predefines settings of the decoding ranges for 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..bf53b07c64a4 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.35.3

[PATCH linux 4/5] spi: aspeed: Handle custom decoding ranges

[Cédric Le Goater]

"ranges" predefines settings of the decoding ranges for 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 | 63 +++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
index 1611c354c31f..791cbf753a85 100644
--- a/drivers/spi/spi-aspeed-smc.c
+++ b/drivers/spi/spi-aspeed-smc.c
@@ -101,6 +101,7 @@ struct aspeed_spi {
 	u32			 clk_freq;
 
 	struct aspeed_spi_chip	 chips[ASPEED_SPI_MAX_NUM_CS];
+	bool                     fixed_windows;
 };
 
 static u32 aspeed_spi_get_io_mode(const struct spi_mem_op *op)
@@ -574,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",
@@ -749,6 +751,61 @@ static const struct attribute_group aspeed_spi_attribute_group = {
 	.attrs = aspeed_spi_attributes
 };
 
+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;
@@ -806,6 +863,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.35.3

[PATCH linux 5/5] ARM: dts: aspeed: ast2500-evb: Add custom decoding ranges (TEST)

[Cédric Le Goater]

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>
---
 arch/arm/boot/dts/aspeed-ast2500-evb.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-ast2500-evb.dts b/arch/arm/boot/dts/aspeed-ast2500-evb.dts
index 1d24b394ea4c..397ca8f4e36a 100644
--- a/arch/arm/boot/dts/aspeed-ast2500-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2500-evb.dts
@@ -36,6 +36,9 @@ gfx_memory: framebuffer {
 
 &fmc {
 	status = "okay";
+        ranges = <0 0 0x20000000 0x2000000>,
+                 <1 0 0x22000000 0x2000000>,
+                 <2 0 0x24000000 0x2000000>;
 	flash@0 {
 		status = "okay";
 		m25p,fast-read;
-- 
2.35.3

Re: [PATCH linux 4/5] spi: aspeed: Handle custom decoding ranges

[Naresh Solanki]

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

Verified for its working.
Thanks


On Tue, 28 Jun 2022 at 21:51, Cédric Le Goater <clg@kaod.org> wrote:

> "ranges" predefines settings of the decoding ranges for 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 | 63 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index 1611c354c31f..791cbf753a85 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -101,6 +101,7 @@ struct aspeed_spi {
>         u32                      clk_freq;
>
>         struct aspeed_spi_chip   chips[ASPEED_SPI_MAX_NUM_CS];
> +       bool                     fixed_windows;
>  };
>
>  static u32 aspeed_spi_get_io_mode(const struct spi_mem_op *op)
> @@ -574,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",
> @@ -749,6 +751,61 @@ static const struct attribute_group
> aspeed_spi_attribute_group = {
>         .attrs = aspeed_spi_attributes
>  };
>
> +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;
> @@ -806,6 +863,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.35.3
>
>

[-- Attachment #2: Type: text/html, Size: 6222 bytes --]

Re: [PATCH linux 1/5] spi: aspeed: Fix window offset of CE1

[Joel Stanley]

On Tue, 28 Jun 2022 at 16:20, Cédric Le Goater <clg@kaod.org> wrote:
>
> The offset value of the mapping window in the kernel structure is
> calculated using the value of the previous window offset. This doesn't
> reflect how the HW is configured and can lead to erroneous setting of
> the second flash device (CE1).
>

Did you want to include a Fixes tag?

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  drivers/spi/spi-aspeed-smc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index 3e891bf22470..5a995b5653f1 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -398,7 +398,7 @@ static void aspeed_spi_get_windows(struct aspeed_spi *aspi,
>                 windows[cs].cs = cs;
>                 windows[cs].size = data->segment_end(aspi, reg_val) -
>                         data->segment_start(aspi, reg_val);
> -               windows[cs].offset = cs ? windows[cs - 1].offset + windows[cs - 1].size : 0;
> +               windows[cs].offset = data->segment_start(aspi, reg_val) - aspi->ahb_base_phy;

Is subtracting the base address correct for the 2400/2500?

>                 dev_vdbg(aspi->dev, "CE%d offset=0x%.8x size=0x%x\n", cs,
>                          windows[cs].offset, windows[cs].size);
>         }
> --
> 2.35.3
>

Re: [PATCH linux 2/5] spi: aspeed: Introduce a "windows" device attribute

[Cédric Le Goater]

On 6/28/22 18:20, Cédric Le Goater wrote:
> This dumps the mapping windows, or decoding ranges, of all devices
> possibly attached of the controller.

We might want to change the name of the sysfs attribute to "ranges" or
"decoding-ranges" to be in sync with the new DT property.

C.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   drivers/spi/spi-aspeed-smc.c | 43 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index 5a995b5653f1..1611c354c31f 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -382,6 +382,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 +397,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;
> @@ -712,6 +714,41 @@ static void aspeed_spi_enable(struct aspeed_spi *aspi, bool enable)
>   		aspeed_spi_chip_enable(aspi, cs, enable);
>   }
>   
> +static int windows_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct aspeed_spi *aspi = dev_get_drvdata(dev);
> +	struct aspeed_spi_window windows[ASPEED_SPI_MAX_NUM_CS] = { 0 };
> +	u32 cs;
> +	int len = 0;
> +
> +	if (aspi->data == &ast2400_spi_data)
> +		return 0;
> +
> +	aspeed_spi_get_windows(aspi, windows);
> +
> +	len += sysfs_emit_at(buf, len, "     offset     size       register\n");
> +	for (cs = 0; cs < aspi->data->max_cs; cs++) {
> +		if (!windows[cs].reg)
> +			len += sysfs_emit_at(buf, len, "CE%d: disabled\n", cs);
> +		else
> +			len += sysfs_emit_at(buf, len, "CE%d: 0x%.8x 0x%.8x 0x%x\n", cs,
> +					     windows[cs].offset, windows[cs].size,
> +					     windows[cs].reg);
> +	}
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RO(windows);
> +
> +static struct attribute *aspeed_spi_attributes[] = {
> +	&dev_attr_windows.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group aspeed_spi_attribute_group = {
> +	.attrs = aspeed_spi_attributes
> +};
> +
>   static int aspeed_spi_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -784,6 +821,12 @@ static int aspeed_spi_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "spi_register_controller failed\n");
>   		goto disable_clk;
>   	}
> +
> +	ret = devm_device_add_group(&pdev->dev, &aspeed_spi_attribute_group);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot create attribute group\n");
> +		goto disable_clk;
> +	}
>   	return 0;
>   
>   disable_clk:

Re: [PATCH linux 4/5] spi: aspeed: Handle custom decoding ranges

[Cédric Le Goater]

On 6/29/22 07:46, Naresh Solanki wrote:
> Verified for its working.

Thanks for the test ! When we upstream, a Tested-by tag would
be nice to have.

C.

> Thanks
> 
> 
> On Tue, 28 Jun 2022 at 21:51, Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote:
> 
>     "ranges" predefines settings of the decoding ranges for 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 <mailto:naresh.solanki@9elements.com>>
>     Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com <mailto:chin-ting_kuo@aspeedtech.com>>
>     Signed-off-by: Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>>
>     ---
>       drivers/spi/spi-aspeed-smc.c | 63 +++++++++++++++++++++++++++++++++++-
>       1 file changed, 62 insertions(+), 1 deletion(-)
> 
>     diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
>     index 1611c354c31f..791cbf753a85 100644
>     --- a/drivers/spi/spi-aspeed-smc.c
>     +++ b/drivers/spi/spi-aspeed-smc.c
>     @@ -101,6 +101,7 @@ struct aspeed_spi {
>              u32                      clk_freq;
> 
>              struct aspeed_spi_chip   chips[ASPEED_SPI_MAX_NUM_CS];
>     +       bool                     fixed_windows;
>       };
> 
>       static u32 aspeed_spi_get_io_mode(const struct spi_mem_op *op)
>     @@ -574,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",
>     @@ -749,6 +751,61 @@ static const struct attribute_group aspeed_spi_attribute_group = {
>              .attrs = aspeed_spi_attributes
>       };
> 
>     +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;
>     @@ -806,6 +863,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.35.3
> 

Re: [PATCH linux 1/5] spi: aspeed: Fix window offset of CE1

[Cédric Le Goater]

On 6/29/22 09:20, Joel Stanley wrote:
> On Tue, 28 Jun 2022 at 16:20, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The offset value of the mapping window in the kernel structure is
>> calculated using the value of the previous window offset. This doesn't
>> reflect how the HW is configured and can lead to erroneous setting of
>> the second flash device (CE1).
>>
> 
> Did you want to include a Fixes tag?

I should and one from the correct tree !

Fixes: e3228ed92893 ("spi: spi-mem: Convert Aspeed SMC driver to spi-mem")

> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   drivers/spi/spi-aspeed-smc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
>> index 3e891bf22470..5a995b5653f1 100644
>> --- a/drivers/spi/spi-aspeed-smc.c
>> +++ b/drivers/spi/spi-aspeed-smc.c
>> @@ -398,7 +398,7 @@ static void aspeed_spi_get_windows(struct aspeed_spi *aspi,
>>                  windows[cs].cs = cs;
>>                  windows[cs].size = data->segment_end(aspi, reg_val) -
>>                          data->segment_start(aspi, reg_val);
>> -               windows[cs].offset = cs ? windows[cs - 1].offset + windows[cs - 1].size : 0;
>> +               windows[cs].offset = data->segment_start(aspi, reg_val) - aspi->ahb_base_phy;
> 
> Is subtracting the base address correct for the 2400/2500?

The segment_start() handlers return absolute physical addresses.

Since we want the offset, we could simply  :

   data->segment_start(aspi, reg_val) & (aspi->ahb_window_size - 1);

May be better,

Thanks,

C.

> 
>>                  dev_vdbg(aspi->dev, "CE%d offset=0x%.8x size=0x%x\n", cs,
>>                           windows[cs].offset, windows[cs].size);
>>          }
>> --
>> 2.35.3
>>